| Message ID | 20220628194526.111501-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | i2c: Add new driver for Renesas RZ/V2M controller | expand |
Hi Phil, On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > Hi, > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series > add the driver. One annoying problem is that the SoC uses a single reset > line for two i2c controllers, and unfortunately one of the controllers > is managed by some firmware, not by Linux. Therefore, the driver just > deasserts the reset. This sounds scary. If the driver is never loaded, and the reset is never deasserted, what happens to the firmware trying to access the other i2c controller? Does it hang? Or write to the reset controller registers to deassert the reset? If so, is there any protection against concurrent access from firmware and reset controller driver? regards Philipp
Hi Philipp, On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series > > add the driver. One annoying problem is that the SoC uses a single reset > > line for two i2c controllers, and unfortunately one of the controllers > > is managed by some firmware, not by Linux. Therefore, the driver just > > deasserts the reset. > > This sounds scary. If the driver is never loaded, and the reset is > never deasserted, what happens to the firmware trying to access the > other i2c controller? Does it hang? Or write to the reset controller > registers to deassert the reset? If so, is there any protection against > concurrent access from firmware and reset controller driver? In response to v1, I wrote | That is actually an integration issue, not an i2c controller issue. | | Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL, | to be set by the reset provider? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Philipp, Geert, On 29 June 2022 18:18 Geert Uytterhoeven wrote: > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote: > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This > series > > > add the driver. One annoying problem is that the SoC uses a single > reset > > > line for two i2c controllers, and unfortunately one of the controllers > > > is managed by some firmware, not by Linux. Therefore, the driver just > > > deasserts the reset. > > > > This sounds scary. If the driver is never loaded, and the reset is > > never deasserted, what happens to the firmware trying to access the > > other i2c controller? Does it hang? Or write to the reset controller > > registers to deassert the reset? If so, is there any protection against > > concurrent access from firmware and reset controller driver? Where a common reset is used by Linux and some firmware, I think we have to ensure/assume that both only ever de-assert it. In this particular SoC, the register used to assert/de-assert the reset has write enable bits in the upper half of the reg. There shouldn't be any issues with both trying to de-assert the reset at the same time. > In response to v1, I wrote > > | That is actually an integration issue, not an i2c controller issue. > | > | Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL, > | to be set by the reset provider? From what I understand, there are two main use cases for resets: 1. Often reset lines may be asserted at power on and so a driver needs to de-assert them so that the module can be used. 2. A driver may need to reset the module for some reason. I have only seen this with watchdog timers with no way out. So if a driver does not need to reset the module, shouldn't the driver only ever be de-asserting the reset line? If so, it also doesn’t matter whether the reset is shared with other modules or not. If a driver needs to reset the module, then the reset cannot be shared with other modules used by firmware or Linux, or we cannot use any other modules that share the reset line. Have I missed something? Thanks Phil
Hi Phil, On Do, 2022-06-30 at 13:43 +0000, Phil Edworthy wrote: > Hi Philipp, Geert, > > On 29 June 2022 18:18 Geert Uytterhoeven wrote: > > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote: > > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This > > series > > > > add the driver. One annoying problem is that the SoC uses a single > > reset > > > > line for two i2c controllers, and unfortunately one of the controllers > > > > is managed by some firmware, not by Linux. Therefore, the driver just > > > > deasserts the reset. > > > > > > This sounds scary. If the driver is never loaded, and the reset is > > > never deasserted, what happens to the firmware trying to access the > > > other i2c controller? Does it hang? Or write to the reset controller > > > registers to deassert the reset? If so, is there any protection against > > > concurrent access from firmware and reset controller driver? > Where a common reset is used by Linux and some firmware, I think we have to > ensure/assume that both only ever de-assert it. We also have to make sure that no read-modify-write cycles are required to deassert the resets if we can't lock between firmware and kernel. Otherwise concurrent access could cause a deassert to be reverted. > In this particular SoC, the register used to assert/de-assert the reset > has write enable bits in the upper half of the reg. There shouldn't be any > issues with both trying to de-assert the reset at the same time. Which reset driver is handling the reset for this i2c module? > > In response to v1, I wrote > > > > > That is actually an integration issue, not an i2c controller issue. > > > > > > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL, > > > to be set by the reset provider? > > From what I understand, there are two main use cases for resets: > 1. Often reset lines may be asserted at power on and so a driver needs to > de-assert them so that the module can be used. There are resets that are not initially asserted (among them the self- deasserting resets) that are required to be asserted some time during boot, to put some hardware into a well defined state. I don't think those should be shared, but they sometimes are. > 2. A driver may need to reset the module for some reason. I have only > seen this with watchdog timers with no way out. Grep for device_reset() or reset_control_reset() for some examples. Also there are quite a few assert/udelay/deassert calls in drivers. Also many drivers assert the reset again during remove(). Whether that is always necessary or useful, I can't say. It's sometimes nice during development, to be able to reload a kernel module or rebind a driver to reset some locked up hardware. > So if a driver does not need to reset the module, shouldn't the driver > only ever be de-asserting the reset line? I'm not sure the driver can always know this if it is used on different platforms. > If so, it also doesn’t matter whether the reset is shared with other > modules or not. > If a driver needs to reset the module, then the reset cannot be shared > with other modules used by firmware or Linux, or we cannot use any > other modules that share the reset line. It can be shared for the special case of multiple modules requiring a shared reset line to be asserted once, at some time before the modules are used. The reset controller API supports this for the reset_control_reset() call. regards Philipp
Hi Philipp, On 30 June 2022 15:45 Philipp Zabel wrote: > On Do, 2022-06-30 at 13:43 +0000, Phil Edworthy wrote: > > On 29 June 2022 18:18 Geert Uytterhoeven wrote: > > > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel wrote: > > > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > > > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This > > > series > > > > > add the driver. One annoying problem is that the SoC uses a single > > > reset > > > > > line for two i2c controllers, and unfortunately one of the > controllers > > > > > is managed by some firmware, not by Linux. Therefore, the driver > just > > > > > deasserts the reset. > > > > > > > > This sounds scary. If the driver is never loaded, and the reset is > > > > never deasserted, what happens to the firmware trying to access the > > > > other i2c controller? Does it hang? Or write to the reset controller > > > > registers to deassert the reset? If so, is there any protection > against > > > > concurrent access from firmware and reset controller driver? > > Where a common reset is used by Linux and some firmware, I think we have > to > > ensure/assume that both only ever de-assert it. > > We also have to make sure that no read-modify-write cycles are required > to deassert the resets if we can't lock between firmware and kernel. > Otherwise concurrent access could cause a deassert to be reverted. Agreed > > In this particular SoC, the register used to assert/de-assert the reset > > has write enable bits in the upper half of the reg. There shouldn't be > any > > issues with both trying to de-assert the reset at the same time. > > Which reset driver is handling the reset for this i2c module? drivers/clk/renesas/rzg2l-cpg.c See rzg2l_cpg_assert() and rzg2l_cpg_deassert() Note this driver handles a few different SoCs, the SoC using this i2c driver is specified in drivers/clk/renesas/r9a09g011-cpg.c > > > In response to v1, I wrote > > > > > > > That is actually an integration issue, not an i2c controller issue. > > > > > > > > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL, > > > > to be set by the reset provider? > > > > From what I understand, there are two main use cases for resets: > > 1. Often reset lines may be asserted at power on and so a driver needs > to > > de-assert them so that the module can be used. > > There are resets that are not initially asserted (among them the self- > deasserting resets) that are required to be asserted some time during > boot, to put some hardware into a well defined state. > I don't think those should be shared, but they sometimes are. > > > 2. A driver may need to reset the module for some reason. I have only > > seen this with watchdog timers with no way out. > > Grep for device_reset() or reset_control_reset() for some examples. > Also there are quite a few assert/udelay/deassert calls in drivers. Ok, though I'm not convinced that the driver specifying the reset period is the right way to support lots of different platforms. > Also many drivers assert the reset again during remove(). Whether that > is always necessary or useful, I can't say. Quite. It's difficult to know if the module requires a reset or that's just what the driver developer used. > It's sometimes nice during development, to be able to reload a kernel > module or rebind a driver to reset some locked up hardware. Ok, that's a good use case! > > So if a driver does not need to reset the module, shouldn't the driver > > only ever be de-asserting the reset line? > > I'm not sure the driver can always know this if it is used on different > platforms. > > > If so, it also doesn’t matter whether the reset is shared with other > > modules or not. > > If a driver needs to reset the module, then the reset cannot be shared > > with other modules used by firmware or Linux, or we cannot use any > > other modules that share the reset line. > > It can be shared for the special case of multiple modules requiring a > shared reset line to be asserted once, at some time before the modules > are used. The reset controller API supports this for the > reset_control_reset() call. In order for drivers to work on lots of platforms, should all drivers use devm_reset_control_get_shared() instead of devm_reset_control_get(), unless there is a need to reset the hardware at a specific time after boot (e.g. watchdog with no way out)? So where do we go with this for this i2c driver? Thanks Phil
Hi Geert, On Mi, 2022-06-29 at 19:18 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Wed, Jun 29, 2022 at 6:21 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Di, 2022-06-28 at 20:45 +0100, Phil Edworthy wrote: > > > The Renesas RZ/V2M SoC (r9a09g011) has a new i2c controller. This series > > > add the driver. One annoying problem is that the SoC uses a single reset > > > line for two i2c controllers, and unfortunately one of the controllers > > > is managed by some firmware, not by Linux. Therefore, the driver just > > > deasserts the reset. > > > > This sounds scary. If the driver is never loaded, and the reset is > > never deasserted, what happens to the firmware trying to access the > > other i2c controller? Does it hang? Or write to the reset controller > > registers to deassert the reset? If so, is there any protection against > > concurrent access from firmware and reset controller driver? > > In response to v1, I wrote > > > That is actually an integration issue, not an i2c controller issue. > > > > Perhaps we need a RESET_IS_CRITICAL flag, cfr. CLK_IS_CRITICAL, > > to be set by the reset provider? I would just let the reset controller driver implement this by disabling _assert and _reset for those firmware-shared resets. regards Philipp
On Do, 2022-06-30 at 15:16 +0000, Phil Edworthy wrote: [...] > > Which reset driver is handling the reset for this i2c module? > drivers/clk/renesas/rzg2l-cpg.c > See rzg2l_cpg_assert() and rzg2l_cpg_deassert() > Note this driver handles a few different SoCs, the SoC using this i2c > driver is specified in drivers/clk/renesas/r9a09g011-cpg.c Thank you. [...] > > > In order for drivers to work on lots of platforms, should all drivers > use devm_reset_control_get_shared() instead of devm_reset_control_get(), > unless there is a need to reset the hardware at a specific time after > boot (e.g. watchdog with no way out)? Nobody should use devm_reset_control_get(). Those drivers that require direct control should use devm_reset_control_get_exclusive(). All others probably should use the _shared() variant, if it works for them. > So where do we go with this for this i2c driver? In this specific case letting the driver deassert the reset seems to be safe, so I'm fine with the way it is. You could also let the i2c driver call reset_control_assert() during remove() and modify the rzg2l-cpg.c driver to ignore it. That doesn't seem very useful on its own, but it would have the positive effect of documenting the shared-with-firmware reset in the reset controller driver. regards Philipp
Hi Philipp, On 01 July 2022 16:40 Philipp Zabel wrote: > On Do, 2022-06-30 at 15:16 +0000, Phil Edworthy wrote: > > In order for drivers to work on lots of platforms, should all drivers > > use devm_reset_control_get_shared() instead of devm_reset_control_get(), > > unless there is a need to reset the hardware at a specific time after > > boot (e.g. watchdog with no way out)? > > Nobody should use devm_reset_control_get(). Those drivers that require > direct control should use devm_reset_control_get_exclusive(). All > others probably should use the _shared() variant, if it works for them. Ok, got it! > > So where do we go with this for this i2c driver? > > In this specific case letting the driver deassert the reset seems to be > safe, so I'm fine with the way it is. > > You could also let the i2c driver call reset_control_assert() during > remove() and modify the rzg2l-cpg.c driver to ignore it. That doesn't > seem very useful on its own, but it would have the positive effect of > documenting the shared-with-firmware reset in the reset controller > driver. Ok, I'll skip the assert for the time being and when we get round to making the reset controller mask out shared resets, we can then modify the i2c driver. Thanks for your advice, Phil