| Message ID | 20170111091144.6881-1-ricardo.ribalda@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Jan 11, 2017 at 11:11 AM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > On AMD's SB800 and upwards, the SMBus is shared with the Integrated > Micro Controller (IMC). > > The platform provides a hardware semaphore to avoid race conditions > among them. (Check page 288 of the SB800-Series Southbridges Register > Reference Guide http://support.amd.com/TechDocs/45482.pdf) > > Without this patch, many access to the SMBus end with an invalid > transaction or even with the bus stalled. > After addressing below comment FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>: > Credit-to: Alexandre Desnoyers <alex@qtec.com> It would be nice to put in plain test what you tell in the discussion here instead of non-standard (see submitting-patches.rst) tag. > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > > v2: Suggestions by Andy Shevchenko <andy.shevchenko@gmail.com>: > -Rename timeout to retries > -Use do {} while(--retries) pattern > -Group new variables > > > drivers/i2c/busses/i2c-piix4.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > index f0563f7ce01b..81d06be0a72d 100644 > --- a/drivers/i2c/busses/i2c-piix4.c > +++ b/drivers/i2c/busses/i2c-piix4.c > @@ -585,10 +585,29 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > u8 command, int size, union i2c_smbus_data *data) > { > struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap); > + unsigned short piix4_smba = adapdata->smba; > + int retries = MAX_TIMEOUT; > + int smbslvcnt; > u8 smba_en_lo; > u8 port; > int retval; > > + /* Request the SMBUS semaphore, avoid conflicts with the IMC */ > + smbslvcnt = inb_p(SMBSLVCNT); > + do { > + outb_p(smbslvcnt | 0x10, SMBSLVCNT); > + > + /* Check the semaphore status */ > + smbslvcnt = inb_p(SMBSLVCNT); > + if (smbslvcnt & 0x10) > + break; > + > + usleep_range(1000, 2000); > + } while (--retries); > + /* SMBus is still owned by the IMC, we give up */ > + if (!retries) > + return -EBUSY; > + > mutex_lock(&piix4_mutex_sb800); > > outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); > @@ -606,6 +625,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > > mutex_unlock(&piix4_mutex_sb800); > > + /* Release the semaphore */ > + outb_p(smbslvcnt | 0x20, SMBSLVCNT); > + > return retval; > } > > -- > 2.11.0 >
> After addressing below comment > FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>: @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/ > > Credit-to: Alexandre Desnoyers <alex@qtec.com> > > It would be nice to put in plain test what you tell in the discussion > here instead of non-standard (see submitting-patches.rst) tag. @ricardo: I agree. Is this Reported-by maybe? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 12, 2017 at 9:29 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> After addressing below comment >> FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>: > @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/ Oops, sorry, didn't notice that. I usually use a new line for them. >> > Credit-to: Alexandre Desnoyers <alex@qtec.com> >> >> It would be nice to put in plain test what you tell in the discussion >> here instead of non-standard (see submitting-patches.rst) tag. > > @ricardo: I agree. Is this Reported-by maybe? Yeah, since it was mentioned IIRC that guy is hardware engineer I don't think Suggested-by would work here better.
> > @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/ > > Oops, sorry, didn't notice that. I usually use a new line for them. Thanks for fixing it :) > > @ricardo: I agree. Is this Reported-by maybe? > > Yeah, since it was mentioned IIRC that guy is hardware engineer I > don't think Suggested-by would work here better. So, I changed that and applied to for-current so it can be in my next pull-request. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram and Andy, I have already sent a v3 with the latest suggested changes. On Thu, Jan 12, 2017 at 8:54 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> > @andy: Please don't prefix tags. Patchwork doesn't seem to find them like this :/ >> >> Oops, sorry, didn't notice that. I usually use a new line for them. > > Thanks for fixing it :) > >> > @ricardo: I agree. Is this Reported-by maybe? I still believe that we miss a Credit-to: or Co-author: tag ;). Most times we do not give enough credit to the hardware engineers. I would have taken me a long time through dark AMD doc to figure this out. But for the meantime Reported-by will do the job. Thanks all of you for your fast review!
> I still believe that we miss a Credit-to: or Co-author: tag ;). Most Co-author is just a second signed-off (in my book at least). > times we do not give enough credit to the hardware engineers. True. Yet again, I'd favor simply multiple SoB here. > Thanks all of you for your fast review! You are welcome!
On Thu, Jan 12, 2017 at 10:02 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: >>> > @ricardo: I agree. Is this Reported-by maybe? > > I still believe that we miss a Credit-to: or Co-author: tag ;) We have more than enough of tags.
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index f0563f7ce01b..81d06be0a72d 100644 --- a/drivers/i2c/busses/i2c-piix4.c +++ b/drivers/i2c/busses/i2c-piix4.c @@ -585,10 +585,29 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, u8 command, int size, union i2c_smbus_data *data) { struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap); + unsigned short piix4_smba = adapdata->smba; + int retries = MAX_TIMEOUT; + int smbslvcnt; u8 smba_en_lo; u8 port; int retval; + /* Request the SMBUS semaphore, avoid conflicts with the IMC */ + smbslvcnt = inb_p(SMBSLVCNT); + do { + outb_p(smbslvcnt | 0x10, SMBSLVCNT); + + /* Check the semaphore status */ + smbslvcnt = inb_p(SMBSLVCNT); + if (smbslvcnt & 0x10) + break; + + usleep_range(1000, 2000); + } while (--retries); + /* SMBus is still owned by the IMC, we give up */ + if (!retries) + return -EBUSY; + mutex_lock(&piix4_mutex_sb800); outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); @@ -606,6 +625,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, mutex_unlock(&piix4_mutex_sb800); + /* Release the semaphore */ + outb_p(smbslvcnt | 0x20, SMBSLVCNT); + return retval; }
On AMD's SB800 and upwards, the SMBus is shared with the Integrated Micro Controller (IMC). The platform provides a hardware semaphore to avoid race conditions among them. (Check page 288 of the SB800-Series Southbridges Register Reference Guide http://support.amd.com/TechDocs/45482.pdf) Without this patch, many access to the SMBus end with an invalid transaction or even with the bus stalled. Credit-to: Alexandre Desnoyers <alex@qtec.com> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- v2: Suggestions by Andy Shevchenko <andy.shevchenko@gmail.com>: -Rename timeout to retries -Use do {} while(--retries) pattern -Group new variables drivers/i2c/busses/i2c-piix4.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)