| Message ID | 1432818224-17070-13-git-send-email-vaibhav.hiremath@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: > From: Yi Zhang <yizhang@marvell.com> > > Enable i2c module/unit before transmission and disable when it finishes. > > why? > It's because the i2c bus may be distrubed if the slave device, > typically a touch, powers on. "disturbed" I'd recommend that this is a DT property - not every platform is going to want this, and as there is rudimentary I2C slave support in this driver, this change breaks that.
On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: > On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: >> From: Yi Zhang <yizhang@marvell.com> >> >> Enable i2c module/unit before transmission and disable when it finishes. >> >> why? >> It's because the i2c bus may be distrubed if the slave device, >> typically a touch, powers on. > > "disturbed" > > I'd recommend that this is a DT property - not every platform is going to > want this, and as there is rudimentary I2C slave support in this driver, > this change breaks that. > I would take it as two different comments here, and I believe you also meant same, 1. Not breaking I2C slave support Not sure whether enabling/disabling module in ISR would suffice here. To be specific, in the functions i2c_pxa_slave_start() & i2c_pxa_slave_stop() Another important point is, I am not sure on validation part here, as the platform I have doesn't have multi masters on the bus. 2. DT property I forsee conflict between, DT property and SLAVE mode of operation. where we may want to support master-&-slave together. Also, we may not need this if we fix I2C Slave support issue, but testing is challenge for me (atleast). Do you see any issues with first approach? if you are ok with first approach, I can change the code. Thanks, Vaibhav
On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: > > > On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: >> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: >>> From: Yi Zhang <yizhang@marvell.com> >>> >>> Enable i2c module/unit before transmission and disable when it finishes. >>> >>> why? >>> It's because the i2c bus may be distrubed if the slave device, >>> typically a touch, powers on. >> >> "disturbed" >> >> I'd recommend that this is a DT property - not every platform is going to >> want this, and as there is rudimentary I2C slave support in this driver, >> this change breaks that. >> > > I would take it as two different comments here, and I believe you also > meant same, > > 1. Not breaking I2C slave support > > Not sure whether enabling/disabling module in ISR would suffice here. > To be specific, in the functions i2c_pxa_slave_start() & > i2c_pxa_slave_stop() > > Please ignore this option, as enable is important to generate interrupt on slave start. I will add DT property with clear note on SLAVE support issue, so that it can be disabled and SLAVE support should work as is. Thanks, Vaibhav
On Tue, Jun 02, 2015 at 10:29:23PM +0530, Vaibhav Hiremath wrote: > > > On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: > > > > > >On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: > >>On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: > >>>From: Yi Zhang <yizhang@marvell.com> > >>> > >>>Enable i2c module/unit before transmission and disable when it finishes. > >>> > >>>why? > >>>It's because the i2c bus may be distrubed if the slave device, > >>>typically a touch, powers on. > >> > >>"disturbed" > >> > >>I'd recommend that this is a DT property - not every platform is going to > >>want this, and as there is rudimentary I2C slave support in this driver, > >>this change breaks that. > >> > > > >I would take it as two different comments here, and I believe you also > >meant same, > > > >1. Not breaking I2C slave support > > > >Not sure whether enabling/disabling module in ISR would suffice here. > >To be specific, in the functions i2c_pxa_slave_start() & > >i2c_pxa_slave_stop() > > > > > > Please ignore this option, as enable is important to generate interrupt > on slave start. Yes, you are right, enabling i2c controller first is must for generating the interrupt > > I will add DT property with clear note on SLAVE support issue, so that > it can be disabled and SLAVE support should work as is. > > Thanks, > Vaibhav
On Wednesday 03 June 2015 04:26 PM, Yi Zhang wrote: > On Tue, Jun 02, 2015 at 10:29:23PM +0530, Vaibhav Hiremath wrote: >> >> >> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: >>> >>> >>> On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: >>>> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: >>>>> From: Yi Zhang <yizhang@marvell.com> >>>>> >>>>> Enable i2c module/unit before transmission and disable when it finishes. >>>>> >>>>> why? >>>>> It's because the i2c bus may be distrubed if the slave device, >>>>> typically a touch, powers on. >>>> >>>> "disturbed" >>>> >>>> I'd recommend that this is a DT property - not every platform is going to >>>> want this, and as there is rudimentary I2C slave support in this driver, >>>> this change breaks that. >>>> >>> >>> I would take it as two different comments here, and I believe you also >>> meant same, >>> >>> 1. Not breaking I2C slave support >>> >>> Not sure whether enabling/disabling module in ISR would suffice here. >>> To be specific, in the functions i2c_pxa_slave_start() & >>> i2c_pxa_slave_stop() >>> >>> >> >> Please ignore this option, as enable is important to generate interrupt >> on slave start. > > Yes, you are right, enabling i2c controller first is must for > generating the interrupt Zang, Probably you can help me, I am trying to introduce standard DT propertied for sclk adjustment, meant for ILCR and IWCR register. The datasheet I have has some confusion in terms of usage of load counters and wait counters. Can you please help me understand on below queries - 1. Any additional calculation (any offset or something) I need to add for calculating counter values from time in nsec/usec? 2. Counters just counts the input clock (of either 26MHz or 69MHz)? 3. Are all counters to respective modes mutual exclusive? I mean, if I set fast mode, any dependency on other counters? I see IWCR.COUNT is common for both standard and fast mode, what about high speed mode? 4. IWCR.COUNT field is meant for tHD.DATA ??? Thanks, Vaibhav
On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote: > > > On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: > > > > > > On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: > >> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: > >>> From: Yi Zhang <yizhang@marvell.com> > >>> > >>> Enable i2c module/unit before transmission and disable when it finishes. > >>> > >>> why? > >>> It's because the i2c bus may be distrubed if the slave device, > >>> typically a touch, powers on. > >> > >> "disturbed" > >> > >> I'd recommend that this is a DT property - not every platform is going to > >> want this, and as there is rudimentary I2C slave support in this driver, > >> this change breaks that. > >> > > > > I would take it as two different comments here, and I believe you also > > meant same, > > > > 1. Not breaking I2C slave support > > > > Not sure whether enabling/disabling module in ISR would suffice here. > > To be specific, in the functions i2c_pxa_slave_start() & > > i2c_pxa_slave_stop() > > > > > > Please ignore this option, as enable is important to generate interrupt > on slave start. Hi, Vaibhav: sorry there is something wrong with my mailbox, so I list the LCR/WCR information here, hoping not to confuse you; LCR: it's used to minor adjustments to the SCL, increasing it decreases the SCL frequency, vice versa; yes, it has relation with the I2C input clock: if input_clk = 32MHz, LCR = 0x0804_1c96 LCR[SLV] = 0x096, bit 8~0 LCR[FLV] = 0xe, bit 17~9 LCR[HLVH] = 0x1, bit 26~18 LCR[HLVL] = 0x1, bit 31~27 if input_clk = 52MHz, LCR = 0x081c_60f9 if input_clk = 62.4MHz, LCR = 0x082c_8330 WCR: it's used to control setup and hold timing during slave mode WCR[31: 15], reserved, always write 0; WCR[14: 10], default 0x5, and should not be changed WCR[9: 5], default 0x1, and should not be changed WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a; if input_clk = 66MHz, it's 0x14; hope it's helpful; --- Yi Zhang <yizhang@marvell.com> > > I will add DT property with clear note on SLAVE support issue, so that > it can be disabled and SLAVE support should work as is. > > Thanks, > Vaibhav
On Thursday 04 June 2015 11:59 AM, Yi Zhang wrote: > On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote: >> >> >> On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: >>> >>> >>> On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: >>>> On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: >>>>> From: Yi Zhang <yizhang@marvell.com> >>>>> >>>>> Enable i2c module/unit before transmission and disable when it finishes. >>>>> >>>>> why? >>>>> It's because the i2c bus may be distrubed if the slave device, >>>>> typically a touch, powers on. >>>> >>>> "disturbed" >>>> >>>> I'd recommend that this is a DT property - not every platform is going to >>>> want this, and as there is rudimentary I2C slave support in this driver, >>>> this change breaks that. >>>> >>> >>> I would take it as two different comments here, and I believe you also >>> meant same, >>> >>> 1. Not breaking I2C slave support >>> >>> Not sure whether enabling/disabling module in ISR would suffice here. >>> To be specific, in the functions i2c_pxa_slave_start() & >>> i2c_pxa_slave_stop() >>> >>> >> >> Please ignore this option, as enable is important to generate interrupt >> on slave start. > > Hi, Vaibhav: > > sorry there is something wrong with my mailbox, so I list the LCR/WCR > information here, hoping not to confuse you;genr > Thanks for the info. > LCR: it's used to minor adjustments to the SCL, increasing it > decreases the SCL frequency, vice versa; > yes, it has relation with the I2C input clock: > if input_clk = 32MHz, LCR = 0x0804_1c96 > LCR[SLV] = 0x096, bit 8~0 > LCR[FLV] = 0xe, bit 17~9 > LCR[HLVH] = 0x1, bit 26~18 > LCR[HLVL] = 0x1, bit 31~27 > if input_clk = 52MHz, LCR = 0x081c_60f9 > if input_clk = 62.4MHz, LCR = 0x082c_8330 > Whether they are simply counters which count downs to generate required timing profile on the bus. So would be possible for you to provide timing values generated by this? Or please confirm below understanding, @32MHz, SLV = 0x96, thigh/tlow = 31nsec * 150 = 4.650 usec thigh/tlow = 32nsec * 150 = 4.8 usec [using ceil()] and as per I2C spec, thigh,min = 4 usec tlow,min = 4.7usec I believe my understanding is correct, please confirm. > WCR: it's used to control setup and hold timing during slave mode This is is useful info. So in master mode we can ignore this. Just to clarify, this bit is don't care in master-transmit and master-receive, right? > WCR[31: 15], reserved, always write 0; > WCR[14: 10], default 0x5, and should not be changed > WCR[9: 5], default 0x1, and should not be changed > WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a; > if input_clk = 66MHz, it's 0x14; > > hope it's helpful; Yes certainly. Thanks. Thanks, Vaibhav > > --- > Yi Zhang <yizhang@marvell.com> > >> >> I will add DT property with clear note on SLAVE support issue, so that >> it can be disabled and SLAVE support should work as is. >> >> Thanks, >> Vaibhav
On Thu, Jun 04, 2015 at 12:49:48PM +0530, Vaibhav Hiremath wrote: > > > On Thursday 04 June 2015 11:59 AM, Yi Zhang wrote: > >On Wed, Jun 03, 2015 at 12:59:23AM +0800, Vaibhav Hiremath wrote: > >> > >> > >>On Tuesday 02 June 2015 10:22 PM, Vaibhav Hiremath wrote: > >>> > >>> > >>>On Thursday 28 May 2015 06:53 PM, Russell King - ARM Linux wrote: > >>>>On Thu, May 28, 2015 at 06:33:44PM +0530, Vaibhav Hiremath wrote: > >>>>>From: Yi Zhang <yizhang@marvell.com> > >>>>> > >>>>>Enable i2c module/unit before transmission and disable when it finishes. > >>>>> > >>>>>why? > >>>>>It's because the i2c bus may be distrubed if the slave device, > >>>>>typically a touch, powers on. > >>>> > >>>>"disturbed" > >>>> > >>>>I'd recommend that this is a DT property - not every platform is going to > >>>>want this, and as there is rudimentary I2C slave support in this driver, > >>>>this change breaks that. > >>>> > >>> > >>>I would take it as two different comments here, and I believe you also > >>>meant same, > >>> > >>>1. Not breaking I2C slave support > >>> > >>>Not sure whether enabling/disabling module in ISR would suffice here. > >>>To be specific, in the functions i2c_pxa_slave_start() & > >>>i2c_pxa_slave_stop() > >>> > >>> > >> > >>Please ignore this option, as enable is important to generate interrupt > >>on slave start. > > > > Hi, Vaibhav: > > > > sorry there is something wrong with my mailbox, so I list the LCR/WCR > > information here, hoping not to confuse you;genr > > > > Thanks for the info. > > > > LCR: it's used to minor adjustments to the SCL, increasing it > > decreases the SCL frequency, vice versa; > > yes, it has relation with the I2C input clock: > > if input_clk = 32MHz, LCR = 0x0804_1c96 > > LCR[SLV] = 0x096, bit 8~0 > > LCR[FLV] = 0xe, bit 17~9 > > LCR[HLVH] = 0x1, bit 26~18 > > LCR[HLVL] = 0x1, bit 31~27 > > if input_clk = 52MHz, LCR = 0x081c_60f9 > > if input_clk = 62.4MHz, LCR = 0x082c_8330 > > > > Whether they are simply counters which count downs to generate required > timing profile on the bus. > > So would be possible for you to provide timing values generated by this? > Or please confirm below understanding, > > > @32MHz, SLV = 0x96, > thigh/tlow = 31nsec * 150 = 4.650 usec > thigh/tlow = 32nsec * 150 = 4.8 usec [using ceil()] > > > and as per I2C spec, > > thigh,min = 4 usec > tlow,min = 4.7usec > > I believe my understanding is correct, please confirm. Yes, you are right, as the name shows it, SLV covers standard mode, FLV is for fastmode, HLVH is for high phase of high speed mode, while HLVL is for low phase of low speed mode; > > > > WCR: it's used to control setup and hold timing during slave mode > > This is is useful info. So in master mode we can ignore this. > > Just to clarify, this bit is don't care in master-transmit and > master-receive, right? Yes, you are right; > > > WCR[31: 15], reserved, always write 0; > > WCR[14: 10], default 0x5, and should not be changed > > WCR[9: 5], default 0x1, and should not be changed > > WCR[4:0], default 0x1a, if input_clk = 33MHz, it's 0x0a; > > if input_clk = 66MHz, it's 0x14; > > > > hope it's helpful; > > > Yes certainly. Thanks. > > Thanks, > Vaibhav > > > > --- > > Yi Zhang <yizhang@marvell.com> > > > >> > >>I will add DT property with clear note on SLAVE support issue, so that > >>it can be disabled and SLAVE support should work as is. > >> > >>Thanks, > >>Vaibhav --- Yi Zhang <yizhang@marvell.com>
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c index 844e1fc..f51d512 100644 --- a/drivers/i2c/busses/i2c-pxa.c +++ b/drivers/i2c/busses/i2c-pxa.c @@ -368,6 +368,16 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why) static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret); static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id); +/* enable/disable i2c unit */ +static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable) +{ + if (enable) + writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); + else + writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c)); + udelay(100); +} + static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c) { return !(readl(_ICR(i2c)) & ICR_SCLE); @@ -575,8 +585,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) i2c_pxa_set_slave(i2c, 0); /* enable unit */ - writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); - udelay(100); + i2c_pxa_enable(i2c, true); } @@ -933,6 +942,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, struct pxa_i2c *i2c = adap->algo_data; int ret, i; + /* Enable i2c unit */ + i2c_pxa_enable(i2c, true); + /* If the I2C controller is disabled we need to reset it (probably due to a suspend/resume destroying state). We do this here as we can then avoid worrying about resuming the @@ -953,6 +965,10 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, ret = -EREMOTEIO; out: i2c_pxa_set_slave(i2c, ret); + + /* disable i2c unit */ + i2c_pxa_enable(i2c, false); + return ret; } @@ -1168,6 +1184,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num struct pxa_i2c *i2c = adap->algo_data; int ret, i; + /* Enable i2c unit */ + i2c_pxa_enable(i2c, true); + enable_irq(i2c->irq); for (i = adap->retries; i >= 0; i--) { ret = i2c_pxa_do_xfer(i2c, msgs, num); @@ -1183,6 +1202,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num out: i2c_pxa_set_slave(i2c, ret); disable_irq(i2c->irq); + /* disable i2c unit */ + i2c_pxa_enable(i2c, false); + return ret; } @@ -1407,6 +1429,9 @@ static int i2c_pxa_probe(struct platform_device *dev) ret = 0; } } + + i2c_pxa_enable(i2c, false); + #ifdef CONFIG_I2C_PXA_SLAVE pr_info("I2C: %s: PXA I2C adapter, slave address %d\n", dev_name(&i2c->adap.dev), i2c->slave_addr);