[PATCH 1/1] lib: utils: support multiple reset drivers of same type

[Heinrich Schuchardt]

On 21.07.21 18:06, Jessica Clarke wrote:
> On 21 Jul 2021, at 16:55, Heinrich Schuchardt <xypron.glpk at 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().
>>
>> Let fdt_reset_init() loop over all entries.
>>
>> Fixes: e3d6919d10d7 ("lib: utils/reset: Add generic GPIO reset driver")
>> Fixes: 7cc6fa4d8a65 ("lib: utils: Add simple FDT reset framework")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> lib/utils/reset/fdt_reset.c | 34 ++++++++++++++++++++--------------
>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c
>> index aa5f59f..a77a680 100644
>> --- a/lib/utils/reset/fdt_reset.c
>> +++ b/lib/utils/reset/fdt_reset.c
>> @@ -7,6 +7,7 @@
>>   *   Anup Patel <anup.patel at wdc.com>
>>   */
>>
>> +#include <libfdt.h>
>> #include <sbi/sbi_error.h>
>> #include <sbi/sbi_scratch.h>
>> #include <sbi_utils/fdt/fdt_helper.h>
>> @@ -24,8 +25,14 @@ static struct fdt_reset *reset_drivers[] = {
>> 	&fdt_reset_thead,
>> };
>>
>> -static struct fdt_reset *current_driver = NULL;
>> -
>> +/**
>> + * fdt_reset_init() - initialize reset drivers
>> + *
>> + * For each reset driver we iterate over the match table. For each matching
>> + * entry we call the driver initialization code once. This is necessary as
>> + * drivers may use different compatible string for different reset functions,
>> + * e.g. both "gpio-poweroff" and "gpio-reset".
>> + */
>> int fdt_reset_init(void)
>> {
>> 	int pos, noff, rc;
>> @@ -35,20 +42,19 @@ int fdt_reset_init(void)
>>
>> 	for (pos = 0; pos < array_size(reset_drivers); pos++) {
>> 		drv = reset_drivers[pos];
>> -
>> -		noff = fdt_find_match(fdt, -1, drv->match_table, &match);
>> -		if (noff < 0)
>> -			continue;
>> -
>> -		if (drv->init) {
>> -			rc = drv->init(fdt, noff, match);
>> -			if (rc == SBI_ENODEV)
>> +		for(match = drv->match_table; match->compatible; ++match) {
>> +			noff = fdt_node_offset_by_compatible(fdt, -1,
>> +							     match->compatible);
>> +			if (noff < 0)
>
> I don’t understand this. Doesn’t fdt_find_match already contain an equivalent loop?

The SiFive boards use to separate nodes for gpio-restart and
gpio-poweroff. There is only one GPIO driver which has a match table
with those to strings. For each of the nodes we have to call the init
method of the GPIO driver.

If you pass a table of compatible strings to fdt_find_match() it will
return the position of the first node that matches any of the strings.
So if you have two nodes gpio-restart and gpio-poweroff and two entries
in the matchtable the init routine is only called for one the two nodes.

An alternative solution would be to separate the GPIO driver into two
drivers. One for gpio-restart and one for gpio-poweroff. The advantage
of such a solution would be that you still could use alternative
compatible strings for a single driver.

diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c
index aa5f59f..bf60547 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,
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 },
         { },
  };

Best regards

Heinrich