| Message ID | 20211015131925.22585-1-nikita.shubin@maquefel.me |
|---|---|
| Headers | show |
| Series | I2C framework, reboot Unmatched via PMIC | expand |
On 15 Oct 2021, at 14:19, Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > This series introduce rebooting via i2c da9063 PMIC, currently on > SiFive Unmatched board. > > "gpio-poweroff" remains with default priority of 128 - default priority, > da9063 is 1 by default the least priority. > > da9063-reset { > compatible = "da9063-reset"; > priority = <1>; > }; I don’t see why priority is needed; the DA9063 is known to always be a last-resort way to reboot. Just hard-code it as the lowest priority. I also don’t see why we need a device tree node, just make it a platform quirk, because this isn’t (and shouldn’t be) a standardised thing? Definitely don’t want it being exposed to anything other than OpenSBI, which is what ends up happening if it lives in the device tree... Jess
On Fri, 15 Oct 2021 14:56:40 +0100 Jessica Clarke <jrtc27@jrtc27.com> wrote: Hello Jessica, > The chip is common for RTC, power management and watchdog > functionality. Using it to reboot in this manner is not, as evidenced > by the fact that Linux does not have a driver for doing so and the > hardware vendor is unsure of how it’s even working the way it is. I > hope this driver is never needed for future RISC-V hardware (and I > don’t quite understand how it ended up being needed for the Unmatched, > when the Unleashed had a gpio-restart device, though that lacked a > gpio-poweroff I believe; seems like a very strange oversight). > > Jess > Implementing SBC reset via Watchdog or PMIC is quite a common practice.
On 15 Oct 2021, at 15:05, Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > On Fri, 15 Oct 2021 14:56:40 +0100 > Jessica Clarke <jrtc27@jrtc27.com> wrote: > > Hello Jessica, > >> The chip is common for RTC, power management and watchdog >> functionality. Using it to reboot in this manner is not, as evidenced >> by the fact that Linux does not have a driver for doing so and the >> hardware vendor is unsure of how it’s even working the way it is. I >> hope this driver is never needed for future RISC-V hardware (and I >> don’t quite understand how it ended up being needed for the Unmatched, >> when the Unleashed had a gpio-restart device, though that lacked a >> gpio-poweroff I believe; seems like a very strange oversight). >> >> Jess >> > > Implementing SBC reset via Watchdog or PMIC is quite a common practice. This sequence differs from the existing watchdog-based implementation though, and to quote one of Dialog’s own engineers on LKML: > Personally, if it was possible I think the RTC approach would be best as it's a > full reset and to me is far safer with regards to potential side effects, but as > that's not on the table then this seems the only other approach in your case. This is why I am suggesting it not be a generic device tree thing, as the vendor is basically saying it’s not a good option but the only choice you have, so should be kept a quirk. Jess
On Fri, Oct 15, 2021 at 4:31 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 15 Oct 2021, at 15:05, Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > > > On Fri, 15 Oct 2021 14:56:40 +0100 > > Jessica Clarke <jrtc27@jrtc27.com> wrote: > > > > Hello Jessica, > > > >> The chip is common for RTC, power management and watchdog > >> functionality. Using it to reboot in this manner is not, as evidenced > >> by the fact that Linux does not have a driver for doing so and the > >> hardware vendor is unsure of how it’s even working the way it is. I > >> hope this driver is never needed for future RISC-V hardware (and I > >> don’t quite understand how it ended up being needed for the Unmatched, > >> when the Unleashed had a gpio-restart device, though that lacked a > >> gpio-poweroff I believe; seems like a very strange oversight). > >> > >> Jess > >> > > > > Implementing SBC reset via Watchdog or PMIC is quite a common practice. > > This sequence differs from the existing watchdog-based implementation > though, and to quote one of Dialog’s own engineers on LKML: > > > Personally, if it was possible I think the RTC approach would be best as it's a > > full reset and to me is far safer with regards to potential side effects, but as > > that's not on the table then this seems the only other approach in your case. > > I don't know if you followed the discussion Nikita here: https://patchwork.kernel.org/project/linux-riscv/patch/20210921053356.1705833-1-alexandre.ghiti@canonical.com/ > This is why I am suggesting it not be a generic device tree thing, as > the vendor is basically saying it’s not a good option but the only > choice you have, so should be kept a quirk. This is not the only choice and Adam admits he does not know the "side-effects" of a partial reset like the one implemented here. But to implement the solution he proposes, we need to sacrifice the RTC as it may override user configuration. Maybe it's better to make sure the reset works as expected at the expense of the RTC, that's an open question and today I would tend to listen to him :) Alex > > Jess > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > This series introduce rebooting via i2c da9063 PMIC, currently on > SiFive Unmatched board. > > "gpio-poweroff" remains with default priority of 128 - default priority, > da9063 is 1 by default the least priority. > > da9063-reset { > compatible = "da9063-reset"; > priority = <1>; > }; > > Is required to be added as a child node of PMIC. > > tested via Linux and U-Boot with reset extension: > > OpenSBI: > Platform Reboot Device : da9063-reset > Platform Shutdown Device : gpio-reset > > v1 -> v2: > Added: > lib: sbi: add priority for reset handler > > Renamed read/write to smbus_write/smbus_read, as actually this > is not a "raw" read/write but a one with a register address provided, > later a "raw" version will be need but currently i don't have anything > to test it. > > To Xiang W: > I have analized your proposal of switching to sbi_list, > and switched i2c adapters array to list. Unfortunately > to switch drivers we require "init" functions. > This also has the merit of getting rid of this limit of 16 i2c adapters, that's nice. Maybe you could use that in the gpio library too? > to Alexander Ghiti: > > > No need for the struct i2c_adapter argument as it is globally accessible. > > It looks like a more clean and reusable way to me, let's here more opinions > > > Why should the da9063 device care about the adapter configuration here? > > I think It should just rely on an already configured/initialized i2c > > controller. > > With clocks init added to sifive_i2c configure it can become usable even if nobody > cared about controller initialization. I don't understand what you mean here. IMO, the device driver should only take care of the communication with the device, not the controller. That can be done in the init function of the controller driver, which in addition would simplify the callbacks and the device driver code. > > > Establishing the link between the i2c device and its adapter should > > somehow be implicitly done by the i2c library, IMO the device should not > > care about its controller. > > And how do you think it will look like ? In out case it's not a separate driver/client > entity but all togather for simplicity. In fdt_i2c_adapter_get, instead of passing a pointer to a struct i2c_adapter, we could pass a i2c_device structure which would get filled automatically with its adapter chip. But that implies introducing this i2c_device like I mentioned in the previous review :) In addition, I took a look at the gpio library and they did it this way too. Again, I don't see why the driver developer should care about its adapter since everything could be done behind the scenes for him. > > Nikita Shubin (5): > lib: utils/reset: add priority to gpio reset > lib: utils/i2c: Add generic I2C configuration library > lib: utils/i2c: Add simple FDT based I2C framework > lib: utils/i2c: Add minimal SiFive I2C driver > lib: utils/reset: Add generic da9063 reset driver > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++ > lib/utils/i2c/i2c.c | 85 +++++++++ > lib/utils/i2c/objects.mk | 12 ++ > lib/utils/reset/fdt_reset.c | 2 + > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > lib/utils/reset/fdt_reset_gpio.c | 18 +- > lib/utils/reset/objects.mk | 1 + > 10 files changed, 836 insertions(+), 3 deletions(-) > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > create mode 100644 include/sbi_utils/i2c/i2c.h > create mode 100644 lib/utils/i2c/fdt_i2c.c > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > create mode 100644 lib/utils/i2c/i2c.c > create mode 100644 lib/utils/i2c/objects.mk > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > -- > 2.31.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > From: Nikita Shubin <n.shubin@yadro.com> > > This series introduce rebooting via i2c da9063 PMIC, currently on > SiFive Unmatched board. > > "gpio-poweroff" remains with default priority of 128 - default priority, > da9063 is 1 by default the least priority. > > da9063-reset { > compatible = "da9063-reset"; > priority = <1>; > }; > > Is required to be added as a child node of PMIC. > > tested via Linux and U-Boot with reset extension: > > OpenSBI: > Platform Reboot Device : da9063-reset > Platform Shutdown Device : gpio-reset > > v1 -> v2: > Added: > lib: sbi: add priority for reset handler > > Renamed read/write to smbus_write/smbus_read, as actually this > is not a "raw" read/write but a one with a register address provided, > later a "raw" version will be need but currently i don't have anything > to test it. > > To Xiang W: > I have analized your proposal of switching to sbi_list, > and switched i2c adapters array to list. Unfortunately > to switch drivers we require "init" functions. > > to Alexander Ghiti: > > > No need for the struct i2c_adapter argument as it is globally accessible. > > It looks like a more clean and reusable way to me, let's here more opinions > > > Why should the da9063 device care about the adapter configuration here? > > I think It should just rely on an already configured/initialized i2c > > controller. > > With clocks init added to sifive_i2c configure it can become usable even if nobody > cared about controller initialization. > > > Establishing the link between the i2c device and its adapter should > > somehow be implicitly done by the i2c library, IMO the device should not > > care about its controller. > > And how do you think it will look like ? In out case it's not a separate driver/client > entity but all togather for simplicity. > > Nikita Shubin (5): > lib: utils/reset: add priority to gpio reset > lib: utils/i2c: Add generic I2C configuration library > lib: utils/i2c: Add simple FDT based I2C framework > lib: utils/i2c: Add minimal SiFive I2C driver > lib: utils/reset: Add generic da9063 reset driver > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > lib/utils/i2c/fdt_i2c_sifive.c | 271 +++++++++++++++++++++++++++++ > lib/utils/i2c/i2c.c | 85 +++++++++ > lib/utils/i2c/objects.mk | 12 ++ > lib/utils/reset/fdt_reset.c | 2 + > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > lib/utils/reset/fdt_reset_gpio.c | 18 +- > lib/utils/reset/objects.mk | 1 + > 10 files changed, 836 insertions(+), 3 deletions(-) > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > create mode 100644 include/sbi_utils/i2c/i2c.h > create mode 100644 lib/utils/i2c/fdt_i2c.c > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > create mode 100644 lib/utils/i2c/i2c.c > create mode 100644 lib/utils/i2c/objects.mk > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > -- > 2.31.1 > I'm testing your patchset and although it seems to work from Linux, it fails when used from u-boot at the first smbus_write in da9063_sanity_check. Maybe it relies on some initialization done by the Linux driver? Thanks, Alex > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
Hello Alexandre! On Wed, 20 Oct 2021 06:59:25 +0200 Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: > > > > From: Nikita Shubin <n.shubin@yadro.com> > > > > This series introduce rebooting via i2c da9063 PMIC, currently on > > SiFive Unmatched board. > > > > "gpio-poweroff" remains with default priority of 128 - default > > priority, da9063 is 1 by default the least priority. > > > > da9063-reset { > > compatible = "da9063-reset"; > > priority = <1>; > > }; > > > > Is required to be added as a child node of PMIC. > > > > tested via Linux and U-Boot with reset extension: > > > > OpenSBI: > > Platform Reboot Device : da9063-reset > > Platform Shutdown Device : gpio-reset > > > > v1 -> v2: > > Added: > > lib: sbi: add priority for reset handler > > > > Renamed read/write to smbus_write/smbus_read, as actually this > > is not a "raw" read/write but a one with a register address > > provided, later a "raw" version will be need but currently i don't > > have anything to test it. > > > > To Xiang W: > > I have analized your proposal of switching to sbi_list, > > and switched i2c adapters array to list. Unfortunately > > to switch drivers we require "init" functions. > > > > to Alexander Ghiti: > > > > > No need for the struct i2c_adapter argument as it is globally > > > accessible. > > > > It looks like a more clean and reusable way to me, let's here more > > opinions > > > Why should the da9063 device care about the adapter configuration > > > here? I think It should just rely on an already > > > configured/initialized i2c controller. > > > > With clocks init added to sifive_i2c configure it can become usable > > even if nobody cared about controller initialization. > > > > > Establishing the link between the i2c device and its adapter > > > should somehow be implicitly done by the i2c library, IMO the > > > device should not care about its controller. > > > > And how do you think it will look like ? In out case it's not a > > separate driver/client entity but all togather for simplicity. > > > > Nikita Shubin (5): > > lib: utils/reset: add priority to gpio reset > > lib: utils/i2c: Add generic I2C configuration library > > lib: utils/i2c: Add simple FDT based I2C framework > > lib: utils/i2c: Add minimal SiFive I2C driver > > lib: utils/reset: Add generic da9063 reset driver > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c | > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > lib/utils/reset/fdt_reset.c | 2 + > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > > lib/utils/reset/fdt_reset_gpio.c | 18 +- > > lib/utils/reset/objects.mk | 1 + > > 10 files changed, 836 insertions(+), 3 deletions(-) > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > create mode 100644 include/sbi_utils/i2c/i2c.h > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > create mode 100644 lib/utils/i2c/i2c.c > > create mode 100644 lib/utils/i2c/objects.mk > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > -- > > 2.31.1 > > > > I'm testing your patchset and although it seems to work from Linux, it > fails when used from u-boot at the first smbus_write in > da9063_sanity_check. Maybe it relies on some initialization done by > the Linux driver? > That's sound strange becouse i explicitly tested with u-boot SBI extension, well in fact i tested mostly via u-boot because it's quicker than booting Linux. Do you have CONFIG_SYS_I2C_OCORES enabled ? I tested on u-boot v2021.07 with Sifive Unmatched patches and SBI reset extension applied. May it is time for configure ;) ? Sharing my u-boot branch: https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07 > Thanks, > > Alex > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi
On Tue, 19 Oct 2021 13:57:35 +0200 Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: > > > > From: Nikita Shubin <n.shubin@yadro.com> > > > > This series introduce rebooting via i2c da9063 PMIC, currently on > > SiFive Unmatched board. > > > > "gpio-poweroff" remains with default priority of 128 - default > > priority, da9063 is 1 by default the least priority. > > > > da9063-reset { > > compatible = "da9063-reset"; > > priority = <1>; > > }; > > > > Is required to be added as a child node of PMIC. > > > > tested via Linux and U-Boot with reset extension: > > > > OpenSBI: > > Platform Reboot Device : da9063-reset > > Platform Shutdown Device : gpio-reset > > > > v1 -> v2: > > Added: > > lib: sbi: add priority for reset handler > > > > Renamed read/write to smbus_write/smbus_read, as actually this > > is not a "raw" read/write but a one with a register address > > provided, later a "raw" version will be need but currently i don't > > have anything to test it. > > > > To Xiang W: > > I have analized your proposal of switching to sbi_list, > > and switched i2c adapters array to list. Unfortunately > > to switch drivers we require "init" functions. > > > > This also has the merit of getting rid of this limit of 16 i2c > adapters, that's nice. Maybe you could use that in the gpio library > too? Indeed, but it's not a part of this patch series. > > > to Alexander Ghiti: > > > > > No need for the struct i2c_adapter argument as it is globally > > > accessible. > > > > It looks like a more clean and reusable way to me, let's here more > > opinions > > > Why should the da9063 device care about the adapter configuration > > > here? I think It should just rely on an already > > > configured/initialized i2c controller. > > > > With clocks init added to sifive_i2c configure it can become usable > > even if nobody cared about controller initialization. > > I don't understand what you mean here. IMO, the device driver should > only take care of the communication with the device, not the > controller. That can be done in the init function of the controller > driver, which in addition would simplify the callbacks and the device > driver code. Well to make use of I2C you should enable it togather with clocks, see u-boot/arch/riscv/dts/fu740-c000.dtsi for example: i2c0: i2c@10030000 { ... clocks = <&prci PRCI_CLK_PCLK>; ... It part of driver responsibility to enable clocks it requires, cause if not is required nobody will enable them. > > > > > > Establishing the link between the i2c device and its adapter > > > should somehow be implicitly done by the i2c library, IMO the > > > device should not care about its controller. > > > > And how do you think it will look like ? In out case it's not a > > separate driver/client entity but all togather for simplicity. > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct > i2c_adapter, we could pass a i2c_device structure which would get > filled automatically with its adapter chip. But that implies > introducing this i2c_device like I mentioned in the previous review :) > In addition, I took a look at the gpio library and they did it this > way too. > > Again, I don't see why the driver developer should care about its > adapter since everything could be done behind the scenes for him. Are you suggesting to separate the da9063_reset into client/driver parts ? Don't you think that it will add unnecessary complicity if our case ? Linux i2c_client has a reference to the adapter it's sitting on: https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336 u-boot is relying on some "default" i2c adapter/bus ideed, but if we look on: https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718 We see it's rather complex and hard to understand. > > > > > Nikita Shubin (5): > > lib: utils/reset: add priority to gpio reset > > lib: utils/i2c: Add generic I2C configuration library > > lib: utils/i2c: Add simple FDT based I2C framework > > lib: utils/i2c: Add minimal SiFive I2C driver > > lib: utils/reset: Add generic da9063 reset driver > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c | > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > lib/utils/reset/fdt_reset.c | 2 + > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > > lib/utils/reset/fdt_reset_gpio.c | 18 +- > > lib/utils/reset/objects.mk | 1 + > > 10 files changed, 836 insertions(+), 3 deletions(-) > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > create mode 100644 include/sbi_utils/i2c/i2c.h > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > create mode 100644 lib/utils/i2c/i2c.c > > create mode 100644 lib/utils/i2c/objects.mk > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > -- > > 2.31.1 > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi
On Wed, Oct 20, 2021 at 9:26 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > On Tue, 19 Oct 2021 13:57:35 +0200 > Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin > > <nikita.shubin@maquefel.me> wrote: > > > > > > From: Nikita Shubin <n.shubin@yadro.com> > > > > > > This series introduce rebooting via i2c da9063 PMIC, currently on > > > SiFive Unmatched board. > > > > > > "gpio-poweroff" remains with default priority of 128 - default > > > priority, da9063 is 1 by default the least priority. > > > > > > da9063-reset { > > > compatible = "da9063-reset"; > > > priority = <1>; > > > }; > > > > > > Is required to be added as a child node of PMIC. > > > > > > tested via Linux and U-Boot with reset extension: > > > > > > OpenSBI: > > > Platform Reboot Device : da9063-reset > > > Platform Shutdown Device : gpio-reset > > > > > > v1 -> v2: > > > Added: > > > lib: sbi: add priority for reset handler > > > > > > Renamed read/write to smbus_write/smbus_read, as actually this > > > is not a "raw" read/write but a one with a register address > > > provided, later a "raw" version will be need but currently i don't > > > have anything to test it. > > > > > > To Xiang W: > > > I have analized your proposal of switching to sbi_list, > > > and switched i2c adapters array to list. Unfortunately > > > to switch drivers we require "init" functions. > > > > > > > This also has the merit of getting rid of this limit of 16 i2c > > adapters, that's nice. Maybe you could use that in the gpio library > > too? > > Indeed, but it's not a part of this patch series. > > > > > > to Alexander Ghiti: > > > > > > > No need for the struct i2c_adapter argument as it is globally > > > > accessible. > > > > > > It looks like a more clean and reusable way to me, let's here more > > > opinions > > > > Why should the da9063 device care about the adapter configuration > > > > here? I think It should just rely on an already > > > > configured/initialized i2c controller. > > > > > > With clocks init added to sifive_i2c configure it can become usable > > > even if nobody cared about controller initialization. > > > > I don't understand what you mean here. IMO, the device driver should > > only take care of the communication with the device, not the > > controller. That can be done in the init function of the controller > > driver, which in addition would simplify the callbacks and the device > > driver code. > > Well to make use of I2C you should enable it togather with clocks, see > u-boot/arch/riscv/dts/fu740-c000.dtsi for example: > > i2c0: i2c@10030000 { > ... > clocks = <&prci PRCI_CLK_PCLK>; > ... > > It part of driver responsibility to enable clocks it requires, cause if > not is required nobody will enable them. I understand that but why couldn't this configuration be done in the adapter driver instead of the da9063 driver? At the initialization of the adapter driver, or at each beginning of a write sequence...etc. > > > > > > > > > > Establishing the link between the i2c device and its adapter > > > > should somehow be implicitly done by the i2c library, IMO the > > > > device should not care about its controller. > > > > > > And how do you think it will look like ? In out case it's not a > > > separate driver/client entity but all togather for simplicity. > > > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct > > i2c_adapter, we could pass a i2c_device structure which would get > > filled automatically with its adapter chip. But that implies > > introducing this i2c_device like I mentioned in the previous review :) > > In addition, I took a look at the gpio library and they did it this > > way too. > > > > Again, I don't see why the driver developer should care about its > > adapter since everything could be done behind the scenes for him. > > Are you suggesting to separate the da9063_reset into client/driver > parts ? Don't you think that it will add unnecessary complicity if our > case ? To make sure we understand each other, find below a diff of what it would look like the way I imagine it. It just makes the notion of adapter disappear from the i2c device driver, that's all. This is what is done in openSBI with gpio_pin structure, in Linux here https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication. diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h index 76cdb66..0f1671e 100644 --- a/include/sbi_utils/i2c/i2c.h +++ b/include/sbi_utils/i2c/i2c.h @@ -50,6 +50,10 @@ struct i2c_adapter { struct sbi_dlist node; }; +struct i2c_device { + struct i2c_adapter *adap; +}; + static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist *node) { return container_of(node, struct i2c_adapter, node); diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c index d23ac91..9c64d09 100644 --- a/lib/utils/i2c/i2c.c +++ b/lib/utils/i2c/i2c.c @@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia) return ia->configure(ia); } -int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr, +int i2c_smbus_write(struct i2c_device *dev, uint8_t addr, uint8_t reg, uint8_t *buffer, int len) { - if (!ia) + if (!dev) return SBI_EINVAL; - if (!ia->smbus_write) + if (!dev->adap || !dev->adap->smbus_write) return SBI_ENOSYS; - return ia->smbus_write(ia, addr, reg, buffer, len); + return dev->adap->smbus_write(dev, addr, reg, buffer, len); } -int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr, +int i2c_smbus_read(struct i2c_device *dev, uint8_t addr, uint8_t reg, uint8_t *buffer, int len) { - if (!ia) + if (!dev) return SBI_EINVAL; - if (!ia->smbus_read) + if (!dev->adap || !dev->adap->smbus_read) return SBI_ENOSYS; - return ia->smbus_read(ia, addr, reg, buffer, len); + return dev->adap->smbus_read(dev, addr, reg, buffer, len); } diff --git a/lib/utils/reset/fdt_reset_da9063.c b/lib/utils/reset/fdt_reset_da9063.c index e3c9ced..a14d941 100644 --- a/lib/utils/reset/fdt_reset_da9063.c +++ b/lib/utils/reset/fdt_reset_da9063.c @@ -45,7 +45,7 @@ #define PMIC_CHIP_ID_DA9063 0x61 static struct { - struct i2c_adapter *adapter; + struct i2c_device *dev; uint32_t reg; u8 priority; } da9063 = { @@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct i2c_adapter *adap, uint32_t reg) DA9063_REG_CONTROL_F, DA9063_CONTROL_F_SHUTDOWN); } -static inline int da9063_reset(struct i2c_adapter *adap, uint32_t reg) +static inline int da9063_reset(struct i2c_device *dev, uint32_t reg) { - int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, + int rc = i2c_smbus_reg_write(dev, da9063.reg, DA9063_REG_PAGE_CON, 0x00); if (rc) return rc; - rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, + rc = i2c_smbus_reg_write(dev, da9063.reg, DA9063_REG_CONTROL_F, DA9063_CONTROL_F_WAKEUP); if (rc) return rc; - return i2c_adapter_smbus_reg_write(adap, da9063.reg, + return i2c_smbus_reg_write(dev, da9063.reg, DA9063_REG_CONTROL_A, DA9063_CONTROL_A_M_POWER1_EN | DA9063_CONTROL_A_M_POWER_EN | @@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int nodeoff, { int rc, i2c_dev, i2c_bus, len; const fdt32_t *val; - struct i2c_adapter *adapter; uint64_t addr; /* find dlg,da9063 parent node */ @@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int nodeoff, return i2c_bus; /* i2c adapter get */ - rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter); + rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev); if (rc) return rc; - da9063.adapter = adapter; - sbi_system_reset_add_device(&da9063_reset_i2c); return 0; > > Linux i2c_client has a reference to the adapter it's sitting on: > https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336 > > u-boot is relying on some "default" i2c adapter/bus ideed, but if we > look on: > > https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718 > > We see it's rather complex and hard to understand. > > > > > > > > > > > Nikita Shubin (5): > > > lib: utils/reset: add priority to gpio reset > > > lib: utils/i2c: Add generic I2C configuration library > > > lib: utils/i2c: Add simple FDT based I2C framework > > > lib: utils/i2c: Add minimal SiFive I2C driver > > > lib: utils/reset: Add generic da9063 reset driver > > > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c | > > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > > lib/utils/reset/fdt_reset.c | 2 + > > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > > > lib/utils/reset/fdt_reset_gpio.c | 18 +- > > > lib/utils/reset/objects.mk | 1 + > > > 10 files changed, 836 insertions(+), 3 deletions(-) > > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > > create mode 100644 include/sbi_utils/i2c/i2c.h > > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > > create mode 100644 lib/utils/i2c/i2c.c > > > create mode 100644 lib/utils/i2c/objects.mk > > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > > > -- > > > 2.31.1 > > > > > > > > > -- > > > opensbi mailing list > > > opensbi@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi >
On Wed, 20 Oct 2021 10:11:56 +0200 Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > > > It part of driver responsibility to enable clocks it requires, > > cause if not is required nobody will enable them. > > I understand that but why couldn't this configuration be done in the > adapter driver instead of the da9063 driver? At the initialization of > the adapter driver, or at each beginning of a write sequence...etc. > I think we should stick to "don't touch until you really need", that's why we shouldn't do it in driver init, we can enable clocks on unused bus which will lead to more power consumption, and other's won't be aware that it needs to be gated off (and even so it's need to gated on once again). Also it's really good to disable interrupts when you don't need them. Doing it every read/write seems quite wrong to me, that's why i decided to add a separate method for such tasks. > > > > > > > > > > > > > > Establishing the link between the i2c device and its adapter > > > > > should somehow be implicitly done by the i2c library, IMO the > > > > > device should not care about its controller. > > > > > > > > And how do you think it will look like ? In out case it's not a > > > > separate driver/client entity but all togather for simplicity. > > > > > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct > > > i2c_adapter, we could pass a i2c_device structure which would get > > > filled automatically with its adapter chip. But that implies > > > introducing this i2c_device like I mentioned in the previous > > > review :) In addition, I took a look at the gpio library and they > > > did it this way too. > > > > > > Again, I don't see why the driver developer should care about its > > > adapter since everything could be done behind the scenes for him. > > > > > > > Are you suggesting to separate the da9063_reset into client/driver > > parts ? Don't you think that it will add unnecessary complicity if > > our case ? > > To make sure we understand each other, find below a diff of what it > would look like the way I imagine it. It just makes the notion of > adapter disappear from the i2c device driver, that's all. This is what > is done in openSBI with gpio_pin structure, in Linux here > https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication. > > diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h > index 76cdb66..0f1671e 100644 > --- a/include/sbi_utils/i2c/i2c.h > +++ b/include/sbi_utils/i2c/i2c.h > @@ -50,6 +50,10 @@ struct i2c_adapter { > struct sbi_dlist node; > }; > > +struct i2c_device { > + struct i2c_adapter *adap; > +}; > + > static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist > *node) { > return container_of(node, struct i2c_adapter, node); > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c > index d23ac91..9c64d09 100644 > --- a/lib/utils/i2c/i2c.c > +++ b/lib/utils/i2c/i2c.c > @@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia) > return ia->configure(ia); > } > > -int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr, > +int i2c_smbus_write(struct i2c_device *dev, uint8_t addr, > uint8_t reg, uint8_t *buffer, int len) > { > - if (!ia) > + if (!dev) > return SBI_EINVAL; > - if (!ia->smbus_write) > + if (!dev->adap || !dev->adap->smbus_write) > return SBI_ENOSYS; > > - return ia->smbus_write(ia, addr, reg, buffer, len); > + return dev->adap->smbus_write(dev, addr, reg, buffer, len); > } > > > -int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr, > +int i2c_smbus_read(struct i2c_device *dev, uint8_t addr, > uint8_t reg, uint8_t *buffer, int len) > { > - if (!ia) > + if (!dev) > return SBI_EINVAL; > - if (!ia->smbus_read) > + if (!dev->adap || !dev->adap->smbus_read) > return SBI_ENOSYS; > > - return ia->smbus_read(ia, addr, reg, buffer, len); > + return dev->adap->smbus_read(dev, addr, reg, buffer, len); > } > diff --git a/lib/utils/reset/fdt_reset_da9063.c > b/lib/utils/reset/fdt_reset_da9063.c > index e3c9ced..a14d941 100644 > --- a/lib/utils/reset/fdt_reset_da9063.c > +++ b/lib/utils/reset/fdt_reset_da9063.c > @@ -45,7 +45,7 @@ > #define PMIC_CHIP_ID_DA9063 0x61 > > static struct { > - struct i2c_adapter *adapter; > + struct i2c_device *dev; > uint32_t reg; > u8 priority; > } da9063 = { > @@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct > i2c_adapter *adap, uint32_t reg) > DA9063_REG_CONTROL_F, > DA9063_CONTROL_F_SHUTDOWN); > } > > -static inline int da9063_reset(struct i2c_adapter *adap, uint32_t > reg) +static inline int da9063_reset(struct i2c_device *dev, uint32_t > reg) { > - int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, > + int rc = i2c_smbus_reg_write(dev, da9063.reg, > DA9063_REG_PAGE_CON, 0x00); > > if (rc) > return rc; > > - rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, > + rc = i2c_smbus_reg_write(dev, da9063.reg, > DA9063_REG_CONTROL_F, > DA9063_CONTROL_F_WAKEUP); if (rc) > return rc; > > - return i2c_adapter_smbus_reg_write(adap, da9063.reg, > + return i2c_smbus_reg_write(dev, da9063.reg, > DA9063_REG_CONTROL_A, > DA9063_CONTROL_A_M_POWER1_EN | > DA9063_CONTROL_A_M_POWER_EN | > @@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int > nodeoff, { > int rc, i2c_dev, i2c_bus, len; > const fdt32_t *val; > - struct i2c_adapter *adapter; > uint64_t addr; > > /* find dlg,da9063 parent node */ > @@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int > nodeoff, return i2c_bus; > > /* i2c adapter get */ > - rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter); > + rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev); > if (rc) > return rc; > > - da9063.adapter = adapter; > - > sbi_system_reset_add_device(&da9063_reset_i2c); > > return 0; > So you indeed suggesting to add one level of abstraction. I don't see any strong benefit from this. Who will take care of device creation/initialisation in this case - the adapter ? > > > > > Linux i2c_client has a reference to the adapter it's sitting on: > > https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336 > > > > u-boot is relying on some "default" i2c adapter/bus ideed, but if we > > look on: > > > > https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718 > > > > We see it's rather complex and hard to understand. > > > > > > > > > > > > > > > > > Nikita Shubin (5): > > > > lib: utils/reset: add priority to gpio reset > > > > lib: utils/i2c: Add generic I2C configuration library > > > > lib: utils/i2c: Add simple FDT based I2C framework > > > > lib: utils/i2c: Add minimal SiFive I2C driver > > > > lib: utils/reset: Add generic da9063 reset driver > > > > > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c > > > > | 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > > > lib/utils/reset/fdt_reset.c | 2 + > > > > lib/utils/reset/fdt_reset_da9063.c | 214 > > > > +++++++++++++++++++++++ lib/utils/reset/fdt_reset_gpio.c | > > > > 18 +- lib/utils/reset/objects.mk | 1 + > > > > 10 files changed, 836 insertions(+), 3 deletions(-) > > > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > > > create mode 100644 include/sbi_utils/i2c/i2c.h > > > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > > > create mode 100644 lib/utils/i2c/i2c.c > > > > create mode 100644 lib/utils/i2c/objects.mk > > > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > > > > > -- > > > > 2.31.1 > > > > > > > > > > > > -- > > > > opensbi mailing list > > > > opensbi@lists.infradead.org > > > > http://lists.infradead.org/mailman/listinfo/opensbi > >
On Wed, Oct 20, 2021 at 10:42 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > On Wed, 20 Oct 2021 10:11:56 +0200 > Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > > > > > > It part of driver responsibility to enable clocks it requires, > > > cause if not is required nobody will enable them. > > > > I understand that but why couldn't this configuration be done in the > > adapter driver instead of the da9063 driver? At the initialization of > > the adapter driver, or at each beginning of a write sequence...etc. > > > > I think we should stick to "don't touch until you really need", that's > why we shouldn't do it in driver init, we can enable clocks on unused > bus which will lead to more power consumption, and other's won't be > aware that it needs to be gated off (and even so it's need to gated on > once again). > > Also it's really good to disable interrupts when you don't need them. > > Doing it every read/write seems quite wrong to me, that's why i decided > to add a separate method for such tasks. > > > > > > > > > > > > > > > > > > > Establishing the link between the i2c device and its adapter > > > > > > should somehow be implicitly done by the i2c library, IMO the > > > > > > device should not care about its controller. > > > > > > > > > > And how do you think it will look like ? In out case it's not a > > > > > separate driver/client entity but all togather for simplicity. > > > > > > > > In fdt_i2c_adapter_get, instead of passing a pointer to a struct > > > > i2c_adapter, we could pass a i2c_device structure which would get > > > > filled automatically with its adapter chip. But that implies > > > > introducing this i2c_device like I mentioned in the previous > > > > review :) In addition, I took a look at the gpio library and they > > > > did it this way too. > > > > > > > > Again, I don't see why the driver developer should care about its > > > > adapter since everything could be done behind the scenes for him. > > > > > > > > > > Are you suggesting to separate the da9063_reset into client/driver > > > parts ? Don't you think that it will add unnecessary complicity if > > > our case ? > > > > To make sure we understand each other, find below a diff of what it > > would look like the way I imagine it. It just makes the notion of > > adapter disappear from the i2c device driver, that's all. This is what > > is done in openSBI with gpio_pin structure, in Linux here > > https://www.kernel.org/doc/html/latest/i2c/writing-clients.html#smbus-communication. > > > > diff --git a/include/sbi_utils/i2c/i2c.h b/include/sbi_utils/i2c/i2c.h > > index 76cdb66..0f1671e 100644 > > --- a/include/sbi_utils/i2c/i2c.h > > +++ b/include/sbi_utils/i2c/i2c.h > > @@ -50,6 +50,10 @@ struct i2c_adapter { > > struct sbi_dlist node; > > }; > > > > +struct i2c_device { > > + struct i2c_adapter *adap; > > +}; > > + > > static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist > > *node) { > > return container_of(node, struct i2c_adapter, node); > > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c > > index d23ac91..9c64d09 100644 > > --- a/lib/utils/i2c/i2c.c > > +++ b/lib/utils/i2c/i2c.c > > @@ -61,25 +61,25 @@ int i2c_adapter_configure(struct i2c_adapter *ia) > > return ia->configure(ia); > > } > > > > -int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr, > > +int i2c_smbus_write(struct i2c_device *dev, uint8_t addr, > > uint8_t reg, uint8_t *buffer, int len) > > { > > - if (!ia) > > + if (!dev) > > return SBI_EINVAL; > > - if (!ia->smbus_write) > > + if (!dev->adap || !dev->adap->smbus_write) > > return SBI_ENOSYS; > > > > - return ia->smbus_write(ia, addr, reg, buffer, len); > > + return dev->adap->smbus_write(dev, addr, reg, buffer, len); > > } > > > > > > -int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr, > > +int i2c_smbus_read(struct i2c_device *dev, uint8_t addr, > > uint8_t reg, uint8_t *buffer, int len) > > { > > - if (!ia) > > + if (!dev) > > return SBI_EINVAL; > > - if (!ia->smbus_read) > > + if (!dev->adap || !dev->adap->smbus_read) > > return SBI_ENOSYS; > > > > - return ia->smbus_read(ia, addr, reg, buffer, len); > > + return dev->adap->smbus_read(dev, addr, reg, buffer, len); > > } > > diff --git a/lib/utils/reset/fdt_reset_da9063.c > > b/lib/utils/reset/fdt_reset_da9063.c > > index e3c9ced..a14d941 100644 > > --- a/lib/utils/reset/fdt_reset_da9063.c > > +++ b/lib/utils/reset/fdt_reset_da9063.c > > @@ -45,7 +45,7 @@ > > #define PMIC_CHIP_ID_DA9063 0x61 > > > > static struct { > > - struct i2c_adapter *adapter; > > + struct i2c_device *dev; > > uint32_t reg; > > u8 priority; > > } da9063 = { > > @@ -103,20 +103,20 @@ static inline int da9063_shutdown(struct > > i2c_adapter *adap, uint32_t reg) > > DA9063_REG_CONTROL_F, > > DA9063_CONTROL_F_SHUTDOWN); > > } > > > > -static inline int da9063_reset(struct i2c_adapter *adap, uint32_t > > reg) +static inline int da9063_reset(struct i2c_device *dev, uint32_t > > reg) { > > - int rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, > > + int rc = i2c_smbus_reg_write(dev, da9063.reg, > > DA9063_REG_PAGE_CON, 0x00); > > > > if (rc) > > return rc; > > > > - rc = i2c_adapter_smbus_reg_write(adap, da9063.reg, > > + rc = i2c_smbus_reg_write(dev, da9063.reg, > > DA9063_REG_CONTROL_F, > > DA9063_CONTROL_F_WAKEUP); if (rc) > > return rc; > > > > - return i2c_adapter_smbus_reg_write(adap, da9063.reg, > > + return i2c_smbus_reg_write(dev, da9063.reg, > > DA9063_REG_CONTROL_A, > > DA9063_CONTROL_A_M_POWER1_EN | > > DA9063_CONTROL_A_M_POWER_EN | > > @@ -169,7 +169,6 @@ static int da9063_reset_init(void *fdt, int > > nodeoff, { > > int rc, i2c_dev, i2c_bus, len; > > const fdt32_t *val; > > - struct i2c_adapter *adapter; > > uint64_t addr; > > > > /* find dlg,da9063 parent node */ > > @@ -192,12 +191,10 @@ static int da9063_reset_init(void *fdt, int > > nodeoff, return i2c_bus; > > > > /* i2c adapter get */ > > - rc = fdt_i2c_adapter_get(fdt, i2c_bus, &adapter); > > + rc = fdt_i2c_adapter_get(fdt, i2c_bus, &da9063.dev); > > if (rc) > > return rc; > > > > - da9063.adapter = adapter; > > - > > sbi_system_reset_add_device(&da9063_reset_i2c); > > > > return 0; > > > > So you indeed suggesting to add one level of abstraction. > > I don't see any strong benefit from this. Ok. > > Who will take care of device creation/initialisation in this case - the > adapter ? > > > > > > > > > Linux i2c_client has a reference to the adapter it's sitting on: > > > https://elixir.bootlin.com/linux/v5.15-rc6/source/include/linux/i2c.h#L336 > > > > > > u-boot is relying on some "default" i2c adapter/bus ideed, but if we > > > look on: > > > > > > https://elixir.bootlin.com/u-boot/latest/source/include/i2c.h#L718 > > > > > > We see it's rather complex and hard to understand. > > > > > > > > > > > > > > > > > > > > > > > Nikita Shubin (5): > > > > > lib: utils/reset: add priority to gpio reset > > > > > lib: utils/i2c: Add generic I2C configuration library > > > > > lib: utils/i2c: Add simple FDT based I2C framework > > > > > lib: utils/i2c: Add minimal SiFive I2C driver > > > > > lib: utils/reset: Add generic da9063 reset driver > > > > > > > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > > > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > > > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c > > > > > | 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > > > > lib/utils/reset/fdt_reset.c | 2 + > > > > > lib/utils/reset/fdt_reset_da9063.c | 214 > > > > > +++++++++++++++++++++++ lib/utils/reset/fdt_reset_gpio.c | > > > > > 18 +- lib/utils/reset/objects.mk | 1 + > > > > > 10 files changed, 836 insertions(+), 3 deletions(-) > > > > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > > > > create mode 100644 include/sbi_utils/i2c/i2c.h > > > > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > > > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > > > > create mode 100644 lib/utils/i2c/i2c.c > > > > > create mode 100644 lib/utils/i2c/objects.mk > > > > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > > > > > > > -- > > > > > 2.31.1 > > > > > > > > > > > > > > > -- > > > > > opensbi mailing list > > > > > opensbi@lists.infradead.org > > > > > http://lists.infradead.org/mailman/listinfo/opensbi > > > >
On Wed, Oct 20, 2021 at 8:17 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > Hello Alexandre! > > On Wed, 20 Oct 2021 06:59:25 +0200 > Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin > > <nikita.shubin@maquefel.me> wrote: > > > > > > From: Nikita Shubin <n.shubin@yadro.com> > > > > > > This series introduce rebooting via i2c da9063 PMIC, currently on > > > SiFive Unmatched board. > > > > > > "gpio-poweroff" remains with default priority of 128 - default > > > priority, da9063 is 1 by default the least priority. > > > > > > da9063-reset { > > > compatible = "da9063-reset"; > > > priority = <1>; > > > }; > > > > > > Is required to be added as a child node of PMIC. > > > > > > tested via Linux and U-Boot with reset extension: > > > > > > OpenSBI: > > > Platform Reboot Device : da9063-reset > > > Platform Shutdown Device : gpio-reset > > > > > > v1 -> v2: > > > Added: > > > lib: sbi: add priority for reset handler > > > > > > Renamed read/write to smbus_write/smbus_read, as actually this > > > is not a "raw" read/write but a one with a register address > > > provided, later a "raw" version will be need but currently i don't > > > have anything to test it. > > > > > > To Xiang W: > > > I have analized your proposal of switching to sbi_list, > > > and switched i2c adapters array to list. Unfortunately > > > to switch drivers we require "init" functions. > > > > > > to Alexander Ghiti: > > > > > > > No need for the struct i2c_adapter argument as it is globally > > > > accessible. > > > > > > It looks like a more clean and reusable way to me, let's here more > > > opinions > > > > Why should the da9063 device care about the adapter configuration > > > > here? I think It should just rely on an already > > > > configured/initialized i2c controller. > > > > > > With clocks init added to sifive_i2c configure it can become usable > > > even if nobody cared about controller initialization. > > > > > > > Establishing the link between the i2c device and its adapter > > > > should somehow be implicitly done by the i2c library, IMO the > > > > device should not care about its controller. > > > > > > And how do you think it will look like ? In out case it's not a > > > separate driver/client entity but all togather for simplicity. > > > > > > Nikita Shubin (5): > > > lib: utils/reset: add priority to gpio reset > > > lib: utils/i2c: Add generic I2C configuration library > > > lib: utils/i2c: Add simple FDT based I2C framework > > > lib: utils/i2c: Add minimal SiFive I2C driver > > > lib: utils/reset: Add generic da9063 reset driver > > > > > > include/sbi_utils/i2c/fdt_i2c.h | 26 +++ > > > include/sbi_utils/i2c/i2c.h | 99 +++++++++++ > > > lib/utils/i2c/fdt_i2c.c | 111 ++++++++++++ > > > lib/utils/i2c/fdt_i2c_sifive.c | 271 > > > +++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c | > > > 85 +++++++++ lib/utils/i2c/objects.mk | 12 ++ > > > lib/utils/reset/fdt_reset.c | 2 + > > > lib/utils/reset/fdt_reset_da9063.c | 214 +++++++++++++++++++++++ > > > lib/utils/reset/fdt_reset_gpio.c | 18 +- > > > lib/utils/reset/objects.mk | 1 + > > > 10 files changed, 836 insertions(+), 3 deletions(-) > > > create mode 100644 include/sbi_utils/i2c/fdt_i2c.h > > > create mode 100644 include/sbi_utils/i2c/i2c.h > > > create mode 100644 lib/utils/i2c/fdt_i2c.c > > > create mode 100644 lib/utils/i2c/fdt_i2c_sifive.c > > > create mode 100644 lib/utils/i2c/i2c.c > > > create mode 100644 lib/utils/i2c/objects.mk > > > create mode 100644 lib/utils/reset/fdt_reset_da9063.c > > > > > > -- > > > 2.31.1 > > > > > > > I'm testing your patchset and although it seems to work from Linux, it > > fails when used from u-boot at the first smbus_write in > > da9063_sanity_check. Maybe it relies on some initialization done by > > the Linux driver? > > > > That's sound strange becouse i explicitly tested with u-boot SBI > extension, well in fact i tested mostly via u-boot because it's quicker > than booting Linux. > > Do you have CONFIG_SYS_I2C_OCORES enabled ? > > I tested on u-boot v2021.07 with Sifive Unmatched patches and SBI reset > extension applied. May it is time for configure ;) ? > > Sharing my u-boot branch: > https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07 That's weird, it still does not work for me from u-boot with the following: - openSBI: https://github.com/AlexGhiti/opensbi/tree/int/alex/nikita_pmic_reset_v2 - u-boot: https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07 And this fails this way: => reset resetting ... da9063_system_reset: chip is not da9063 PMIC Alex > > > > Thanks, > > > > Alex > > > > > > > > > > -- > > > opensbi mailing list > > > opensbi@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi >
Hello Alexandre! On Tue, 26 Oct 2021 08:34:17 +0200 Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > That's weird, it still does not work for me from u-boot with the > following: > > - openSBI: > https://github.com/AlexGhiti/opensbi/tree/int/alex/nikita_pmic_reset_v2 > - u-boot: > https://github.com/maquefel/u-boot/tree/yadro/riscv/unmatched-v2021.07 > > And this fails this way: > > => reset > resetting ... > da9063_system_reset: chip is not da9063 PMIC > Thank you for testing once again! I'll try to hug the board and don't release it until i conduct some experiments. Currently i am fighting the RTC, to find out if it is usable: In short i get one and only one ALARM interrupt, and no further interrupts: https://forums.sifive.com/t/has-anyone-got-da9063-rtc-working-from-linux/5302 And currently trying to find out if reset is related to this somehow...