| Message ID | 20210722105359.18622-3-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: utils: support both of gpio-poweroff, gpio-reset | expand |
On Thu, 2021-07-22 at 12:53 +0200, Heinrich Schuchardt wrote: > The generic GPIO reset driver has two entries in the match table: > "gpio-poweroff", "gpio-reset". Only the first entry is considered by > fdt_reset_init(). > > Define "gpio-poweroff" and "gpio-reset" as compatibility strings of > two > separate reset drivers. They still can share code. > > Fixes: e3d6919d10d7 ("lib: utils/reset: Add generic GPIO reset > driver") > Signed-off-by: Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> > --- > lib/utils/reset/fdt_reset.c | 3 ++- > lib/utils/reset/fdt_reset_gpio.c | 11 ++++++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/lib/utils/reset/fdt_reset.c > b/lib/utils/reset/fdt_reset.c > index 87f925c..0d84dc2 100644 > --- a/lib/utils/reset/fdt_reset.c > +++ b/lib/utils/reset/fdt_reset.c > @@ -12,12 +12,14 @@ > #include <sbi_utils/fdt/fdt_helper.h> > #include <sbi_utils/reset/fdt_reset.h> > > +extern struct fdt_reset fdt_poweroff_gpio; > extern struct fdt_reset fdt_reset_gpio; > extern struct fdt_reset fdt_reset_sifive_test; > extern struct fdt_reset fdt_reset_htif; > extern struct fdt_reset fdt_reset_thead; > > static struct fdt_reset *reset_drivers[] = { > + &fdt_poweroff_gpio, > &fdt_reset_gpio, > &fdt_reset_sifive_test, > &fdt_reset_htif, > @@ -45,7 +47,6 @@ int fdt_reset_init(void) > if (rc) > return rc; > } > - break; > } > > return 0; > diff --git a/lib/utils/reset/fdt_reset_gpio.c > b/lib/utils/reset/fdt_reset_gpio.c > index 7e0eb74..77e308a 100644 > --- a/lib/utils/reset/fdt_reset_gpio.c > +++ b/lib/utils/reset/fdt_reset_gpio.c > @@ -129,8 +129,17 @@ static int gpio_reset_init(void *fdt, int > nodeoff, > return 0; > } > > -static const struct fdt_match gpio_reset_match[] = { > +static const struct fdt_match gpio_poweroff_match[] = { > { .compatible = "gpio-poweroff", .data = (void *)FALSE }, > + { }, > +}; > + > +struct fdt_reset fdt_poweroff_gpio = { > + .match_table = gpio_poweroff_match, > + .init = gpio_reset_init, > +}; > + > +static const struct fdt_match gpio_reset_match[] = { > { .compatible = "gpio-restart", .data = (void *)TRUE }, > { }, > }; > -- > 2.30.2 > Reviewed-by: Atish Patra <atish.patra@wdc.com>
On 22 Jul 2021, at 11:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > The generic GPIO reset driver has two entries in the match table: > "gpio-poweroff", "gpio-reset". Only the first entry is considered by > fdt_reset_init(). This is inaccurate. fdt_reset_init does consider all entries in turn. The problem is that OpenSBI’s driver attachment is backwards from normal operating systems. Normal operating systems will iterate over all devices, and for each device will iterate over all drivers trying to find the best one, if any. This means multiple instances of the same driver (including for very different compatible strings) can exist. In OpenSBI, however, this is turned on its head, with it iterating over all drivers, trying to find a device that each driver can attach to. This means multiple instances of the same driver cannot exist, so whichever of gpio-poweroff and gpio-reset appears in the device tree first is the one that gets attached to. Splitting the two sets of functionality out into their own drivers therefore works around this limitation, though arguably it would be better to alter OpenSBI to follow a normal driver attachment process as I’m sure this will come up again eventually. Jess
On 7/22/21 9:33 PM, Jessica Clarke wrote: > On 22 Jul 2021, at 11:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> The generic GPIO reset driver has two entries in the match table: >> "gpio-poweroff", "gpio-reset". Only the first entry is considered by >> fdt_reset_init(). > > This is inaccurate. fdt_reset_init does consider all entries in turn. > The problem is that OpenSBI’s driver attachment is backwards from > normal operating systems. Normal operating systems will iterate over > all devices, and for each device will iterate over all drivers trying > to find the best one, if any. This means multiple instances of the same > driver (including for very different compatible strings) can exist. In > OpenSBI, however, this is turned on its head, with it iterating over > all drivers, trying to find a device that each driver can attach to. > This means multiple instances of the same driver cannot exist, so > whichever of gpio-poweroff and gpio-reset appears in the device tree > first is the one that gets attached to. Splitting the two sets of > functionality out into their own drivers therefore works around this > limitation, though arguably it would be better to alter OpenSBI to > follow a normal driver attachment process as I’m sure this will come up > again eventually. > > Jess > Hello Jess, Thanks for taking the discussion to the overall design. The same logic as in fdt_reset_init() is used in: * fdt_serial_init() - lib/utils/serial/fdt_serial.c * fdt_ipi_cold_init() - lib/utils/ipi/fdt_ipi.c * fdt_irqchip_cold_init() - lib/utils/irqchip/fdt_irqchip.c * fdt_timer_cold_init() - lib/utils/timer/fdt_timer.c Should we first merge this patch and then start to rework the whole driver binding process or would you suggest to apply only patch 1 of the series and do the redesign first? An advantage of iterating over the occurrences of "compatible" properties in the device-tree instead of iterating over drivers is that we parse the device-tree only once. Concerning the new design I see two alternatives: Currently we follow an early probing approach where the init routines of the drivers determine if a device is usable before it is ever requested. U-Boot follows a late probing approach where in a binding phase devices that could be available according to the device-tree are put into a tree structure. Actual probing occurs when the class of driver is actually accessed. This speeds up the boot process by not probing unused devices but increases the code complexity. Late probing in the case of the system reset extension would mean that you only probe the reset drivers once sbi_probe_extension() or a function of the extension is called for the first time. Best regards Heinrich
On 22 Jul 2021, at 23:46, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 7/22/21 9:33 PM, Jessica Clarke wrote: >> On 22 Jul 2021, at 11:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> >>> The generic GPIO reset driver has two entries in the match table: >>> "gpio-poweroff", "gpio-reset". Only the first entry is considered by >>> fdt_reset_init(). >> This is inaccurate. fdt_reset_init does consider all entries in turn. >> The problem is that OpenSBI’s driver attachment is backwards from >> normal operating systems. Normal operating systems will iterate over >> all devices, and for each device will iterate over all drivers trying >> to find the best one, if any. This means multiple instances of the same >> driver (including for very different compatible strings) can exist. In >> OpenSBI, however, this is turned on its head, with it iterating over >> all drivers, trying to find a device that each driver can attach to. >> This means multiple instances of the same driver cannot exist, so >> whichever of gpio-poweroff and gpio-reset appears in the device tree >> first is the one that gets attached to. Splitting the two sets of >> functionality out into their own drivers therefore works around this >> limitation, though arguably it would be better to alter OpenSBI to >> follow a normal driver attachment process as I’m sure this will come up >> again eventually. >> Jess > Hello Jess, > > Thanks for taking the discussion to the overall design. > > The same logic as in fdt_reset_init() is used in: > > * fdt_serial_init() - lib/utils/serial/fdt_serial.c > * fdt_ipi_cold_init() - lib/utils/ipi/fdt_ipi.c > * fdt_irqchip_cold_init() - lib/utils/irqchip/fdt_irqchip.c > * fdt_timer_cold_init() - lib/utils/timer/fdt_timer.c Yeah, this was indeed meant to be a general statement about all the device driver classes. > Should we first merge this patch and then start to rework the whole driver binding process or would you suggest to apply only patch 1 of the series and do the redesign first? Yes, I think given the importance of the functionality your patch adds I think it’s best to merge that and then think about a more robust design for OpenSBI. > An advantage of iterating over the occurrences of "compatible" properties in the device-tree instead of iterating over drivers is that we parse the device-tree only once. This is true, though I suspect that benefit isn’t really all that noticeable in the grand scheme, especially as newer, faster hardware comes out. > Concerning the new design I see two alternatives: > > Currently we follow an early probing approach where the init routines of the drivers determine if a device is usable before it is ever requested. > > U-Boot follows a late probing approach where in a binding phase devices that could be available according to the device-tree are put into a tree structure. Actual probing occurs when the class of driver is actually accessed. This speeds up the boot process by not probing unused devices but increases the code complexity. > > Late probing in the case of the system reset extension would mean that you only probe the reset drivers once sbi_probe_extension() or a function of the extension is called for the first time. Something else to throw into the mix: FreeBSD does things a bit more cleanly to Linux and all the various firmware projects that just copy Linux to varying degrees. Drivers have a probe method that is just “do you want to handle this device, and if so, how much?” (so that you can have specific drivers take priority over generic drivers). In OpenSBI the priority aspect isn’t needed so much as the iteration order is strictly defined by the manually-constructed lists (but in an OS of course it’s just a big list that is whatever order the files were linked in). The actual attaching itself is a separate attach method. I don’t know if probing all devices early in boot but attaching later as needed would be advantageous? It might just be additional unnecessary complexity though, I haven’t thought this through much. For something like power control though I think you would want to bind pretty early in boot, as you’ll want to be able to use that on error paths early in boot. Jess
On 23 Jul 2021, at 00:00, Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 22 Jul 2021, at 23:46, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 7/22/21 9:33 PM, Jessica Clarke wrote: >>> On 22 Jul 2021, at 11:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> >>>> The generic GPIO reset driver has two entries in the match table: >>>> "gpio-poweroff", "gpio-reset". Only the first entry is considered by >>>> fdt_reset_init(). >>> This is inaccurate. fdt_reset_init does consider all entries in turn. >>> The problem is that OpenSBI’s driver attachment is backwards from >>> normal operating systems. Normal operating systems will iterate over >>> all devices, and for each device will iterate over all drivers trying >>> to find the best one, if any. This means multiple instances of the same >>> driver (including for very different compatible strings) can exist. In >>> OpenSBI, however, this is turned on its head, with it iterating over >>> all drivers, trying to find a device that each driver can attach to. >>> This means multiple instances of the same driver cannot exist, so >>> whichever of gpio-poweroff and gpio-reset appears in the device tree >>> first is the one that gets attached to. Splitting the two sets of >>> functionality out into their own drivers therefore works around this >>> limitation, though arguably it would be better to alter OpenSBI to >>> follow a normal driver attachment process as I’m sure this will come up >>> again eventually. >>> Jess >> Hello Jess, >> >> Thanks for taking the discussion to the overall design. >> >> The same logic as in fdt_reset_init() is used in: >> >> * fdt_serial_init() - lib/utils/serial/fdt_serial.c >> * fdt_ipi_cold_init() - lib/utils/ipi/fdt_ipi.c >> * fdt_irqchip_cold_init() - lib/utils/irqchip/fdt_irqchip.c >> * fdt_timer_cold_init() - lib/utils/timer/fdt_timer.c > > Yeah, this was indeed meant to be a general statement about all the device driver classes. > >> Should we first merge this patch and then start to rework the whole driver binding process or would you suggest to apply only patch 1 of the series and do the redesign first? > > Yes, I think given the importance of the functionality your patch adds I think it’s best to merge that and then think about a more robust design for OpenSBI. (though with the commit message rewritten since as it stands it is rather incorrect) Jess
On Fri, Jul 23, 2021 at 12:49 AM Atish Patra <Atish.Patra@wdc.com> wrote: > > On Thu, 2021-07-22 at 12:53 +0200, Heinrich Schuchardt wrote: > > The generic GPIO reset driver has two entries in the match table: > > "gpio-poweroff", "gpio-reset". Only the first entry is considered by > > fdt_reset_init(). > > > > Define "gpio-poweroff" and "gpio-reset" as compatibility strings of > > two > > separate reset drivers. They still can share code. > > > > Fixes: e3d6919d10d7 ("lib: utils/reset: Add generic GPIO reset > > driver") > > Signed-off-by: Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> > > --- > > lib/utils/reset/fdt_reset.c | 3 ++- > > lib/utils/reset/fdt_reset_gpio.c | 11 ++++++++++- > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/lib/utils/reset/fdt_reset.c > > b/lib/utils/reset/fdt_reset.c > > index 87f925c..0d84dc2 100644 > > --- a/lib/utils/reset/fdt_reset.c > > +++ b/lib/utils/reset/fdt_reset.c > > @@ -12,12 +12,14 @@ > > #include <sbi_utils/fdt/fdt_helper.h> > > #include <sbi_utils/reset/fdt_reset.h> > > > > +extern struct fdt_reset fdt_poweroff_gpio; > > extern struct fdt_reset fdt_reset_gpio; > > extern struct fdt_reset fdt_reset_sifive_test; > > extern struct fdt_reset fdt_reset_htif; > > extern struct fdt_reset fdt_reset_thead; > > > > static struct fdt_reset *reset_drivers[] = { > > + &fdt_poweroff_gpio, > > &fdt_reset_gpio, > > &fdt_reset_sifive_test, > > &fdt_reset_htif, > > @@ -45,7 +47,6 @@ int fdt_reset_init(void) > > if (rc) > > return rc; > > } > > - break; > > } > > > > return 0; > > diff --git a/lib/utils/reset/fdt_reset_gpio.c > > b/lib/utils/reset/fdt_reset_gpio.c > > index 7e0eb74..77e308a 100644 > > --- a/lib/utils/reset/fdt_reset_gpio.c > > +++ b/lib/utils/reset/fdt_reset_gpio.c > > @@ -129,8 +129,17 @@ static int gpio_reset_init(void *fdt, int > > nodeoff, > > return 0; > > } > > > > -static const struct fdt_match gpio_reset_match[] = { > > +static const struct fdt_match gpio_poweroff_match[] = { > > { .compatible = "gpio-poweroff", .data = (void *)FALSE }, > > + { }, > > +}; > > + > > +struct fdt_reset fdt_poweroff_gpio = { > > + .match_table = gpio_poweroff_match, > > + .init = gpio_reset_init, > > +}; > > + > > +static const struct fdt_match gpio_reset_match[] = { > > { .compatible = "gpio-restart", .data = (void *)TRUE }, > > { }, > > }; > > -- > > 2.30.2 > > > Reviewed-by: Atish Patra <atish.patra@wdc.com> Forgot to reply here... Applied this patch to the riscv/opensbi repo. Thanks, Anup > > -- > Regards, > Atish > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c index 87f925c..0d84dc2 100644 --- a/lib/utils/reset/fdt_reset.c +++ b/lib/utils/reset/fdt_reset.c @@ -12,12 +12,14 @@ #include <sbi_utils/fdt/fdt_helper.h> #include <sbi_utils/reset/fdt_reset.h> +extern struct fdt_reset fdt_poweroff_gpio; extern struct fdt_reset fdt_reset_gpio; extern struct fdt_reset fdt_reset_sifive_test; extern struct fdt_reset fdt_reset_htif; extern struct fdt_reset fdt_reset_thead; static struct fdt_reset *reset_drivers[] = { + &fdt_poweroff_gpio, &fdt_reset_gpio, &fdt_reset_sifive_test, &fdt_reset_htif, @@ -45,7 +47,6 @@ int fdt_reset_init(void) if (rc) return rc; } - break; } return 0; diff --git a/lib/utils/reset/fdt_reset_gpio.c b/lib/utils/reset/fdt_reset_gpio.c index 7e0eb74..77e308a 100644 --- a/lib/utils/reset/fdt_reset_gpio.c +++ b/lib/utils/reset/fdt_reset_gpio.c @@ -129,8 +129,17 @@ static int gpio_reset_init(void *fdt, int nodeoff, return 0; } -static const struct fdt_match gpio_reset_match[] = { +static const struct fdt_match gpio_poweroff_match[] = { { .compatible = "gpio-poweroff", .data = (void *)FALSE }, + { }, +}; + +struct fdt_reset fdt_poweroff_gpio = { + .match_table = gpio_poweroff_match, + .init = gpio_reset_init, +}; + +static const struct fdt_match gpio_reset_match[] = { { .compatible = "gpio-restart", .data = (void *)TRUE }, { }, };
The generic GPIO reset driver has two entries in the match table: "gpio-poweroff", "gpio-reset". Only the first entry is considered by fdt_reset_init(). Define "gpio-poweroff" and "gpio-reset" as compatibility strings of two separate reset drivers. They still can share code. Fixes: e3d6919d10d7 ("lib: utils/reset: Add generic GPIO reset driver") Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- lib/utils/reset/fdt_reset.c | 3 ++- lib/utils/reset/fdt_reset_gpio.c | 11 ++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) -- 2.30.2