| Message ID | 1436210695-19159-3-git-send-email-vaibhav.hiremath@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > Normally i2c controller works as master, so slave addr is not needed, or it > will impact some slave device (eg. ST NFC chip) i2c accesses, because it has > the same i2c address with controller. Just to make sure: Does it? As I read the code, slave interrupts are enabled later only when slave mode is selected? Is that a HW bug? And if so, can't the code just be moved into this #ifdef block later? > > Signed-off-by: Jett.Zhou <jtzhou@marvell.com> > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/i2c/busses/i2c-pxa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c > index f4ac8c5..023e59f 100644 > --- a/drivers/i2c/busses/i2c-pxa.c > +++ b/drivers/i2c/busses/i2c-pxa.c > @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) > writel(I2C_ISR_INIT, _ISR(i2c)); > writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c)); > > - if (i2c->reg_isar) > + if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) > writel(i2c->slave_addr, _ISAR(i2c)); > > /* set control register values */ > -- > 1.9.1 >
On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: > On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: >> Normally i2c controller works as master, so slave addr is not needed, or it >> will impact some slave device (eg. ST NFC chip) i2c accesses, because it has >> the same i2c address with controller. > > Just to make sure: Does it? As I read the code, slave interrupts are > enabled later only when slave mode is selected? Is that a HW bug? And if > so, can't the code just be moved into this #ifdef block later? > Yes we could, infact I thought about it; but I would break recommended sequence here. As per spec, it only says, The SocC writes this register before it enable TWSI operations. I decided not to break the existing sequence and just fix the code, as I do can not validate slave mode of operation functionally. I do not have slave mode setup with me. Thanks, Vaibhav >> >> Signed-off-by: Jett.Zhou <jtzhou@marvell.com> >> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org> >> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr> >> --- >> drivers/i2c/busses/i2c-pxa.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c >> index f4ac8c5..023e59f 100644 >> --- a/drivers/i2c/busses/i2c-pxa.c >> +++ b/drivers/i2c/busses/i2c-pxa.c >> @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) >> writel(I2C_ISR_INIT, _ISR(i2c)); >> writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c)); >> >> - if (i2c->reg_isar) >> + if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) >> writel(i2c->slave_addr, _ISAR(i2c)); >> >> /* set control register values */ >> -- >> 1.9.1 >>
On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: > > > On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: > >On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > >>Normally i2c controller works as master, so slave addr is not needed, or it > >>will impact some slave device (eg. ST NFC chip) i2c accesses, because it has > >>the same i2c address with controller. > > > >Just to make sure: Does it? As I read the code, slave interrupts are > >enabled later only when slave mode is selected? Is that a HW bug? And if > >so, can't the code just be moved into this #ifdef block later? > > > > Yes we could, infact I thought about it; > but I would break recommended sequence here. And did you set the "own slave address" to a value which one of your existing i2c slaves also has (without enabling slave mode)? Did it disturb communication?
On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote: > On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: >> >> >> On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: >>> On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: >>>> Normally i2c controller works as master, so slave addr is not needed, or it >>>> will impact some slave device (eg. ST NFC chip) i2c accesses, because it has >>>> the same i2c address with controller. >>> >>> Just to make sure: Does it? As I read the code, slave interrupts are >>> enabled later only when slave mode is selected? Is that a HW bug? And if >>> so, can't the code just be moved into this #ifdef block later? >>> >> >> Yes we could, infact I thought about it; >> but I would break recommended sequence here. > > And did you set the "own slave address" to a value which one of your > existing i2c slaves also has (without enabling slave mode)? Did it > disturb communication? > Since slave and master mode are mutual exclusive, I did not try this. But I can try. Thanks, Vaibhav
On Fri, Jul 10, 2015 at 07:55:31PM +0530, Vaibhav Hiremath wrote: > > > On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote: > >On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: > >> > >> > >>On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: > >>>On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: > >>>>Normally i2c controller works as master, so slave addr is not needed, or it > >>>>will impact some slave device (eg. ST NFC chip) i2c accesses, because it has > >>>>the same i2c address with controller. > >>> > >>>Just to make sure: Does it? As I read the code, slave interrupts are > >>>enabled later only when slave mode is selected? Is that a HW bug? And if > >>>so, can't the code just be moved into this #ifdef block later? > >>> > >> > >>Yes we could, infact I thought about it; > >>but I would break recommended sequence here. > > > >And did you set the "own slave address" to a value which one of your > >existing i2c slaves also has (without enabling slave mode)? Did it > >disturb communication? > > > > Since slave and master mode are mutual exclusive, > I did not try this. Ehrm, what I meant was. Did you see the issue mentioned in the above commit message? Can you reproduce it? You don't need to enable slave mode for that, no?
On Friday 10 July 2015 08:26 PM, Wolfram Sang wrote: > On Fri, Jul 10, 2015 at 07:55:31PM +0530, Vaibhav Hiremath wrote: >> >> >> On Friday 10 July 2015 07:44 PM, Wolfram Sang wrote: >>> On Fri, Jul 10, 2015 at 06:08:43PM +0530, Vaibhav Hiremath wrote: >>>> >>>> >>>> On Friday 10 July 2015 01:41 PM, Wolfram Sang wrote: >>>>> On Tue, Jul 07, 2015 at 12:54:46AM +0530, Vaibhav Hiremath wrote: >>>>>> Normally i2c controller works as master, so slave addr is not needed, or it >>>>>> will impact some slave device (eg. ST NFC chip) i2c accesses, because it has >>>>>> the same i2c address with controller. >>>>> >>>>> Just to make sure: Does it? As I read the code, slave interrupts are >>>>> enabled later only when slave mode is selected? Is that a HW bug? And if >>>>> so, can't the code just be moved into this #ifdef block later? >>>>> >>>> >>>> Yes we could, infact I thought about it; >>>> but I would break recommended sequence here. >>> >>> And did you set the "own slave address" to a value which one of your >>> existing i2c slaves also has (without enabling slave mode)? Did it >>> disturb communication? >>> >> >> Since slave and master mode are mutual exclusive, >> I did not try this. > > Ehrm, what I meant was. Did you see the issue mentioned in the above > commit message? Can you reproduce it? You don't need to enable slave > mode for that, no? > Ohh yes, I am able to reproduce this issue. I have pxa1928 based board, where I have only PMIC 88PM860 connected to I2C bus, and whenever I set slave address to 0x30 (PMIC slave address) all I2C transaction for PMIC goes for toss. Thanks, Vaibhav
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c index f4ac8c5..023e59f 100644 --- a/drivers/i2c/busses/i2c-pxa.c +++ b/drivers/i2c/busses/i2c-pxa.c @@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) writel(I2C_ISR_INIT, _ISR(i2c)); writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c)); - if (i2c->reg_isar) + if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) writel(i2c->slave_addr, _ISAR(i2c)); /* set control register values */