| Message ID | 20231009211420.3454026-1-lakshmiy@us.ibm.com |
|---|---|
| Headers | show |
| Series | hwmon: (pmbus/max31785) Add minimum delay between bus accesses | expand |
Correct Link to Andrew's previous proposal: https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ On 10/9/23, 4:21 PM, "Lakshmi Yadlapati" <lakshmiy@us.ibm.com <mailto:lakshmiy@us.ibm.com>> wrote: Reintroduce per-client throttling of transfers for improved compatibility. Some devices have experienced issues with small command turn-around times when using in-kernel device drivers. While a previous proposal was rejected due to concerns about error-prone open-coding of delays, recent upstream changes for similar problems in I2C devices (e.g., max15301 and ucd90320) and now max31785 make it sensible to reintroduce Andrew's generic solution. This change aims to improve compatibility for affected devices and may help avoid duplicating implementations of handlers for I2C and PMBus calls in driver code. Reference to Andrew's previous proposal: https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au <mailto:20200914122811.3295678-1-andrew@aj.id.au>/ Lakshmi Yadlapati (2): i2c: smbus: Allow throttling of transfers to client devices hwmon: (pmbus/max31785) Add minimum delay between bus accesses drivers/hwmon/pmbus/max31785.c | 8 ++ drivers/i2c/i2c-core-base.c | 8 +- drivers/i2c/i2c-core-smbus.c | 143 ++++++++++++++++++++++++++------- drivers/i2c/i2c-core.h | 23 ++++++ include/linux/i2c.h | 2 + 5 files changed, 153 insertions(+), 31 deletions(-)
Hi, thanks for this series! > Reference to Andrew's previous proposal: > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ I do totally agree with Guenter's comment[1], though. This just affects a few drivers and this patch is way too intrusive for the I2C core. The later suggested prepare_device() callback[2] sounds better to me. I still haven't fully understood why this all cannot be handled in the driver's probe. Could someone give me a small summary about that? All the best, Wolfram [1] https://lore.kernel.org/all/e7a64983-fe1d-1ba2-b0c3-ae4a791f7a75@roeck-us.net/ [2] https://lore.kernel.org/all/120342ec-f44a-4550-8c54-45b97db41024@www.fastmail.com/
On Tue, Oct 10, 2023 at 11:31:56AM +0200, Wolfram Sang wrote: > Hi, > > thanks for this series! > > > Reference to Andrew's previous proposal: > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ > > I do totally agree with Guenter's comment[1], though. This just affects > a few drivers and this patch is way too intrusive for the I2C core. The > later suggested prepare_device() callback[2] sounds better to me. I > still haven't fully understood why this all cannot be handled in the > driver's probe. Could someone give me a small summary about that? > Lots of PMBus devices have the same problem, we have always handled it in PMBus drivers by implementing local wait code, and your references point that out. What other summary are you looking for ? On a side note, if anyone plans to implement the prepare_device() callback, please make sure that it covers all requirements. It would be unfortunate if such a callback was implemented if that would still require per-driver code (besides the callback). Thanks, Guenter
Hi Guenter, > > > Reference to Andrew's previous proposal: > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ > > > > I do totally agree with Guenter's comment[1], though. This just affects > > a few drivers and this patch is way too intrusive for the I2C core. The > > later suggested prepare_device() callback[2] sounds better to me. I > > still haven't fully understood why this all cannot be handled in the > > driver's probe. Could someone give me a small summary about that? > > > > Lots of PMBus devices have the same problem, we have always handled > it in PMBus drivers by implementing local wait code, and your references > point that out. I am confused now. Reading your reply: "I am not sure if an implementation in the i2c core is desirable. It looks quite invasive to me, and it won't solve the problem for all devices since it isn't always a simple "wait <n> microseconds between accesses". For example, some devices may require a wait after a write but not after a read, or a wait only after certain commands (such as commands writing to an EEPROM)." I get the impression you don't prefer to have a generic mechanism in the I2C core. This I share. Your response now sounds like you do support that idea now? > What other summary are you looking for ? What the actual problem is with these devices. The cover letter only mentions "issues with small command turn-around times". More details would be nice. Is it between transfers? Or even between messages within one transfer? Has it been tried to lower the bus frequency? Stuff like this. > On a side note, if anyone plans to implement the prepare_device() callback, > please make sure that it covers all requirements. It would be unfortunate > if such a callback was implemented if that would still require per-driver > code (besides the callback). Is there a list of that somewhere? Or does it mean going through all the drivers and see what they currently do? Regards, Wolfram
On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote: > Hi Guenter, > > > > > Reference to Andrew's previous proposal: > > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ > > > > > > I do totally agree with Guenter's comment[1], though. This just affects > > > a few drivers and this patch is way too intrusive for the I2C core. The > > > later suggested prepare_device() callback[2] sounds better to me. I > > > still haven't fully understood why this all cannot be handled in the > > > driver's probe. Could someone give me a small summary about that? > > > > > > > Lots of PMBus devices have the same problem, we have always handled > > it in PMBus drivers by implementing local wait code, and your references > > point that out. > > I am confused now. Reading your reply: > > "I am not sure if an implementation in the i2c core is desirable. It > looks quite invasive to me, and it won't solve the problem for all > devices since it isn't always a simple "wait <n> microseconds between > accesses". For example, some devices may require a wait after a write > but not after a read, or a wait only after certain commands (such as > commands writing to an EEPROM)." > > I get the impression you don't prefer to have a generic mechanism in the > I2C core. This I share. Your response now sounds like you do support > that idea now? > I didn't (want to) say that. I am perfectly happy with driver specific code, and I would personally still very much prefer it. I only wanted to suggest that _if_ a generic solution is implemented, it should cover all existing use cases and not just this one. But, really, I'd rather leave that alone and not risk introducing regressions to existing drivers. > > What other summary are you looking for ? > > What the actual problem is with these devices. The cover letter only > mentions "issues with small command turn-around times". More details > would be nice. Is it between transfers? Or even between messages within > one transfer? Has it been tried to lower the bus frequency? Stuff like > this. > I don't know about this device, but in general the problem is that the devices need some delay between some or all transfers or otherwise react badly in one way or another. The problem is not always the same. Lower bus frequencies don't help, at least not for the devices where I have seen to problem myself. The issue is not bus speed, but time between transfers. Typically the underlying problem is that there is some microcontroller on the affected chips, and the microcode is less than perfect. For example, the microcode may not poll its I2C interface while it is busy writing into the chip's internal EEPROM or while it is updating some internal parameters as result of a previous I2C transfer. > > On a side note, if anyone plans to implement the prepare_device() callback, > > please make sure that it covers all requirements. It would be unfortunate > > if such a callback was implemented if that would still require per-driver > > code (besides the callback). > > Is there a list of that somewhere? Or does it mean going through all the > drivers and see what they currently do? > The latter. I never bothered trying to write up a list. Typically the behavior is not documented and needs to be tweaked a couple of times, and it may be different across chips supported by the same driver, or even across chip revisions. Any list trying to keep track of the various details would be difficult to maintain and notoriously be outdated. Guenter
On Wed, 11 Oct 2023, at 09:29, Guenter Roeck wrote: > On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote: >> Hi Guenter, >> >> > > > Reference to Andrew's previous proposal: >> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ >> > > >> > > I do totally agree with Guenter's comment[1], though. This just affects >> > > a few drivers and this patch is way too intrusive for the I2C core. The >> > > later suggested prepare_device() callback[2] sounds better to me. I >> > > still haven't fully understood why this all cannot be handled in the >> > > driver's probe. Could someone give me a small summary about that? >> > > >> > >> > Lots of PMBus devices have the same problem, we have always handled >> > it in PMBus drivers by implementing local wait code, and your references >> > point that out. >> >> I am confused now. Reading your reply: >> >> "I am not sure if an implementation in the i2c core is desirable. It >> looks quite invasive to me, and it won't solve the problem for all >> devices since it isn't always a simple "wait <n> microseconds between >> accesses". For example, some devices may require a wait after a write >> but not after a read, or a wait only after certain commands (such as >> commands writing to an EEPROM)." >> >> I get the impression you don't prefer to have a generic mechanism in the >> I2C core. This I share. Your response now sounds like you do support >> that idea now? >> > > I didn't (want to) say that. I am perfectly happy with driver specific > code, and I would personally still very much prefer it. I only wanted to > suggest that _if_ a generic solution is implemented, it should cover all > existing use cases and not just this one. But, really, I'd rather leave > that alone and not risk introducing regressions to existing drivers. We had an out-of-tree patch for the max31785[1] that I wrote a little after the initial discussion on this generic throttling and possibly somewhat before the other drivers had their delays added. Recently Joel pointed out the addition of the delays in the other drivers and I raised the idea that we could get rid of that out-of-tree patch by doing the same. Guenter's point about the work-arounds being very particular to the device is good justification for not trying to fix drivers that we can't immediately test - not that the series did that, but arguably if we're shooting for the generic solution then it should. So I agree with Guenter that we probably want to do down the path of adding the delays directly into the max31785 driver and not trying to over-generalise. Lakshmi: Apologies for misleading you in some way there - unfortunately I can't go back to understand exactly what I suggested as I've changed jobs in the mean time. Andrew [1]: https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be
Joel's suggestion to explore the previously proposed generic solution makes sense, especially considering the recent changes for PMBus devices that have added delays. Thank you for your review and input. I will modify the code to incorporate the necessary delay directly in the max31785 driver. Thanks, Lakshmi On 10/10/23, 10:54 PM, "Andrew Jeffery" <andrew@aj.id.au <mailto:andrew@aj.id.au>> wrote: On Wed, 11 Oct 2023, at 09:29, Guenter Roeck wrote: > On Tue, Oct 10, 2023 at 08:58:06PM +0200, Wolfram Sang wrote: >> Hi Guenter, >> >> > > > Reference to Andrew's previous proposal: >> > > > https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/ <https://lore.kernel.org/all/20200914122811.3295678-1-andrew@aj.id.au/> >> > > >> > > I do totally agree with Guenter's comment[1], though. This just affects >> > > a few drivers and this patch is way too intrusive for the I2C core. The >> > > later suggested prepare_device() callback[2] sounds better to me. I >> > > still haven't fully understood why this all cannot be handled in the >> > > driver's probe. Could someone give me a small summary about that? >> > > >> > >> > Lots of PMBus devices have the same problem, we have always handled >> > it in PMBus drivers by implementing local wait code, and your references >> > point that out. >> >> I am confused now. Reading your reply: >> >> "I am not sure if an implementation in the i2c core is desirable. It >> looks quite invasive to me, and it won't solve the problem for all >> devices since it isn't always a simple "wait <n> microseconds between >> accesses". For example, some devices may require a wait after a write >> but not after a read, or a wait only after certain commands (such as >> commands writing to an EEPROM)." >> >> I get the impression you don't prefer to have a generic mechanism in the >> I2C core. This I share. Your response now sounds like you do support >> that idea now? >> > > I didn't (want to) say that. I am perfectly happy with driver specific > code, and I would personally still very much prefer it. I only wanted to > suggest that _if_ a generic solution is implemented, it should cover all > existing use cases and not just this one. But, really, I'd rather leave > that alone and not risk introducing regressions to existing drivers. We had an out-of-tree patch for the max31785[1] that I wrote a little after the initial discussion on this generic throttling and possibly somewhat before the other drivers had their delays added. Recently Joel pointed out the addition of the delays in the other drivers and I raised the idea that we could get rid of that out-of-tree patch by doing the same. Guenter's point about the work-arounds being very particular to the device is good justification for not trying to fix drivers that we can't immediately test - not that the series did that, but arguably if we're shooting for the generic solution then it should. So I agree with Guenter that we probably want to do down the path of adding the delays directly into the max31785 driver and not trying to over-generalise. Lakshmi: Apologies for misleading you in some way there - unfortunately I can't go back to understand exactly what I suggested as I've changed jobs in the mean time. Andrew [1]: https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be <https://github.com/openbmc/linux/commit/44e1397368a70ffe9cdad1f9212ffdef8c16b9be>
Hi Guenter, > I didn't (want to) say that. I am perfectly happy with driver specific > code, and I would personally still very much prefer it. I only wanted to > suggest that _if_ a generic solution is implemented, it should cover all > existing use cases and not just this one. But, really, I'd rather leave > that alone and not risk introducing regressions to existing drivers. Okay, seems we are aligned again :) > I don't know about this device, but in general the problem is that the > devices need some delay between some or all transfers or otherwise react > badly in one way or another. The problem is not always the same. Ok, that again doesn't speak for a generic solution IMO. > Lower bus frequencies don't help, at least not for the devices where > I have seen to problem myself. The issue is not bus speed, but time between > transfers. Typically the underlying problem is that there is some > microcontroller on the affected chips, and the microcode is less than > perfect. For example, the microcode may not poll its I2C interface > while it is busy writing into the chip's internal EEPROM or while it is > updating some internal parameters as result of a previous I2C transfer. I see. Well, as you probably know, EEPROMs not reacting because they are busy with an erase cycle is well-known in the I2C world. The bus driver reports that the transfer couldn't get through, and then the EEPROM driver knows the details and does something apropriate, probably waiting a while. This assumes that the EEPROM can still play well on the I2C bus. If a faulty device will lock up a bus because of bad microcode while it is busy, then it surely needs handling of that :( And this convinces me just more that it should be in the driver... > The latter. I never bothered trying to write up a list. Typically the behavior > is not documented and needs to be tweaked a couple of times, and it may be > different across chips supported by the same driver, or even across chip > revisions. Any list trying to keep track of the various details would > be difficult to maintain and notoriously be outdated. ... especially because of that. If there is really some repeating pattern for some of the devices, we could introduce helper functions for the drivers to use maybe. But the I2C core functions are not the place to handle it. All the best, Wolfram