| Message ID | 20211024164642.810274-1-wxjstz@126.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | lib: utils/reset: Add voltage level for gpio_reset | expand |
On 24 Oct 2021, at 17:46, Xiang W <wxjstz@126.com> wrote: > > The reset voltage may be different under different platforms, so fdt > should describe the reset level. > > Signed-off-by: Xiang W <wxjstz@126.com> > --- > lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/lib/utils/reset/fdt_reset_gpio.c b/lib/utils/reset/fdt_reset_gpio.c > index 4da1450..b834522 100644 > --- a/lib/utils/reset/fdt_reset_gpio.c > +++ b/lib/utils/reset/fdt_reset_gpio.c > @@ -21,16 +21,19 @@ > > struct gpio_reset { > struct gpio_pin pin; > + u32 active_level; > u32 active_delay; > u32 inactive_delay; > }; > > static struct gpio_reset poweroff = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > > static struct gpio_reset restart = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 reason) > > if (reset) { > /* drive it active, also inactive->active edge */ > - gpio_direction_output(&reset->pin, 1); > + gpio_direction_output(&reset->pin, reset->active_level); > sbi_timer_mdelay(reset->active_delay); > > /* drive inactive, also active->inactive edge */ > - gpio_set(&reset->pin, 0); > + gpio_set(&reset->pin, !reset->active_level); > sbi_timer_mdelay(reset->inactive_delay); > > /* drive it active, also inactive->active edge */ > - gpio_set(&reset->pin, 1); > + gpio_set(&reset->pin, reset->active_level); > } > /* hang !!! */ > sbi_hart_hang(); > @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int nodeoff, > return rc; > } > > + val = fdt_getprop(fdt, nodeoff, "active-level", &len); This property is neither documented in the binding specification for gpio-restart nor is it checked by Linux. > + if (len > 0) > + reset->active_level = !!fdt32_to_cpu(*val); > + > val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); In fact these are also wrong. The binding does not have a -ms suffix on the (in)active-delay property names. Jess
在 2021-10-24星期日的 17:53 +0100，Jessica Clarke写道： > On 24 Oct 2021, at 17:46, Xiang W <wxjstz@126.com> wrote: > > > > The reset voltage may be different under different platforms, so > > fdt > > should describe the reset level. > > > > Signed-off-by: Xiang W <wxjstz@126.com> > > --- > > lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/lib/utils/reset/fdt_reset_gpio.c > > b/lib/utils/reset/fdt_reset_gpio.c > > index 4da1450..b834522 100644 > > --- a/lib/utils/reset/fdt_reset_gpio.c > > +++ b/lib/utils/reset/fdt_reset_gpio.c > > @@ -21,16 +21,19 @@ > > > > struct gpio_reset { > > struct gpio_pin pin; > > + u32 active_level; > > u32 active_delay; > > u32 inactive_delay; > > }; > > > > static struct gpio_reset poweroff = { > > + .active_level = 1, > > .active_delay = 100, > > .inactive_delay = 100 > > }; > > > > static struct gpio_reset restart = { > > + .active_level = 1, > > .active_delay = 100, > > .inactive_delay = 100 > > }; > > @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 > > reason) > > > > if (reset) { > > /* drive it active, also inactive->active edge */ > > - gpio_direction_output(&reset->pin, 1); > > + gpio_direction_output(&reset->pin, reset- > > >active_level); > > sbi_timer_mdelay(reset->active_delay); > > > > /* drive inactive, also active->inactive edge */ > > - gpio_set(&reset->pin, 0); > > + gpio_set(&reset->pin, !reset->active_level); > > sbi_timer_mdelay(reset->inactive_delay); > > > > /* drive it active, also inactive->active edge */ > > - gpio_set(&reset->pin, 1); > > + gpio_set(&reset->pin, reset->active_level); > > } > > /* hang !!! */ > > sbi_hart_hang(); > > @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int > > nodeoff, > > return rc; > > } > > > > + val = fdt_getprop(fdt, nodeoff, "active-level", &len); > > This property is neither documented in the binding specification for > gpio-restart nor is it checked by Linux. This DT is only processed by opensbi, and linux should be reset through the sbi interface, instead of controlling gpio to perform the reset by itself. Regards, Xiang W > > > + if (len > 0) > > + reset->active_level = !!fdt32_to_cpu(*val); > > + > > val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); > > In fact these are also wrong. The binding does not have a -ms suffix > on > the (in)active-delay property names. > > Jess
On 24 Oct 2021, at 18:10, Xiang W <wxjstz@126.com> wrote: > > 在 2021-10-24星期日的 17:53 +0100，Jessica Clarke写道： >> On 24 Oct 2021, at 17:46, Xiang W <wxjstz@126.com> wrote: >>> >>> The reset voltage may be different under different platforms, so >>> fdt >>> should describe the reset level. >>> >>> Signed-off-by: Xiang W <wxjstz@126.com> >>> --- >>> lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/utils/reset/fdt_reset_gpio.c >>> b/lib/utils/reset/fdt_reset_gpio.c >>> index 4da1450..b834522 100644 >>> --- a/lib/utils/reset/fdt_reset_gpio.c >>> +++ b/lib/utils/reset/fdt_reset_gpio.c >>> @@ -21,16 +21,19 @@ >>> >>> struct gpio_reset { >>> struct gpio_pin pin; >>> + u32 active_level; >>> u32 active_delay; >>> u32 inactive_delay; >>> }; >>> >>> static struct gpio_reset poweroff = { >>> + .active_level = 1, >>> .active_delay = 100, >>> .inactive_delay = 100 >>> }; >>> >>> static struct gpio_reset restart = { >>> + .active_level = 1, >>> .active_delay = 100, >>> .inactive_delay = 100 >>> }; >>> @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 >>> reason) >>> >>> if (reset) { >>> /* drive it active, also inactive->active edge */ >>> - gpio_direction_output(&reset->pin, 1); >>> + gpio_direction_output(&reset->pin, reset- >>>> active_level); >>> sbi_timer_mdelay(reset->active_delay); >>> >>> /* drive inactive, also active->inactive edge */ >>> - gpio_set(&reset->pin, 0); >>> + gpio_set(&reset->pin, !reset->active_level); >>> sbi_timer_mdelay(reset->inactive_delay); >>> >>> /* drive it active, also inactive->active edge */ >>> - gpio_set(&reset->pin, 1); >>> + gpio_set(&reset->pin, reset->active_level); >>> } >>> /* hang !!! */ >>> sbi_hart_hang(); >>> @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int >>> nodeoff, >>> return rc; >>> } >>> >>> + val = fdt_getprop(fdt, nodeoff, "active-level", &len); >> >> This property is neither documented in the binding specification for >> gpio-restart nor is it checked by Linux. > > This DT is only processed by opensbi, and linux should be reset through > the sbi interface, instead of controlling gpio to perform the reset by > itself. Well, there are several things going on: 1. In general, you can’t just make up properties in the global namespace, that’s how you end up with the kind of mess device trees have the reputation of having 2. Linux doesn’t need this property and supports far more hardware than OpenSBI does, so why does OpenSBI need knowledge of this property but Linux doesn’t on any platform for any architecture? 3. OpenSBI doesn’t provide the device tree, in general, pre-OpenSBI firmware does, be it U-Boot SPL or something else, and in general that device tree *will* make its way all the way to the OS. Linux might go and ignore that and use its own device tree because the whole situation is a mess, but that’s not the point, this can and will be exposed to OSes running on top. Jess
在 2021-10-24星期日的 18:26 +0100，Jessica Clarke写道： > > > > > > This property is neither documented in the binding specification > > > for > > > gpio-restart nor is it checked by Linux. > > > > This DT is only processed by opensbi, and linux should be reset > > through > > the sbi interface, instead of controlling gpio to perform the reset > > by > > itself. > > Well, there are several things going on: > > 1. In general, you can’t just make up properties in the global > namespace, that’s how you end up with the kind of mess device > trees > have the reputation of having > 2. Linux doesn’t need this property and supports far more hardware > than > OpenSBI does, so why does OpenSBI need knowledge of this property > but > Linux doesn’t on any platform for any architecture? > 3. OpenSBI doesn’t provide the device tree, in general, pre-OpenSBI > firmware does, be it U-Boot SPL or something else, and in general > that > device tree *will* make its way all the way to the OS. Linux might > go and > ignore that and use its own device tree because the whole > situation is a > mess, but that’s not the point, this can and will be exposed to > OSes running > on top. > > Jess These attributes are under the gpio-restart/gpio-poweroff node and are not global attributes. If you need to rename, you can submit a new patch. The rename is added to the current patch, and the commit cannot be kept clean Regards, Xiang W
On 10/24/21 11:46 AM, Xiang W wrote: > The reset voltage may be different under different platforms, so fdt > should describe the reset level. FDT already has a way to describe the reset level: the flags cell in your GPIO specifier. The GPIO controller is responsible for inverting the levels when GPIO_FLAG_ACTIVE_LOW is present. That way, the inversion logic does not need to be duplicated in every GPIO consumer driver. > Signed-off-by: Xiang W <wxjstz@126.com> > --- > lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/lib/utils/reset/fdt_reset_gpio.c b/lib/utils/reset/fdt_reset_gpio.c > index 4da1450..b834522 100644 > --- a/lib/utils/reset/fdt_reset_gpio.c > +++ b/lib/utils/reset/fdt_reset_gpio.c > @@ -21,16 +21,19 @@ > > struct gpio_reset { > struct gpio_pin pin; > + u32 active_level; > u32 active_delay; > u32 inactive_delay; > }; > > static struct gpio_reset poweroff = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > > static struct gpio_reset restart = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 reason) > > if (reset) { > /* drive it active, also inactive->active edge */ > - gpio_direction_output(&reset->pin, 1); > + gpio_direction_output(&reset->pin, reset->active_level); "1" as an argument to gpio_direction_output/gpio_set always means the active level, which could be high or low, depending on if reset->pin has the GPIO_FLAG_ACTIVE_LOW flag. > sbi_timer_mdelay(reset->active_delay); > > /* drive inactive, also active->inactive edge */ > - gpio_set(&reset->pin, 0); > + gpio_set(&reset->pin, !reset->active_level); > sbi_timer_mdelay(reset->inactive_delay); > > /* drive it active, also inactive->active edge */ > - gpio_set(&reset->pin, 1); > + gpio_set(&reset->pin, reset->active_level); > } > /* hang !!! */ > sbi_hart_hang(); > @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int nodeoff, > return rc; > } > > + val = fdt_getprop(fdt, nodeoff, "active-level", &len); > + if (len > 0) > + reset->active_level = !!fdt32_to_cpu(*val); > + Like Jessica said, all device tree properties must be documented in the binding for that device. And the bindings for the "gpio-poweroff" and "gpio-restart" compatible strings live in the Linux kernel tree. So if you want to add a new property, that binding must be updated first. But here, no new property is needed, because there is already a way to do what you want within the existing binding. Regards, Samuel > val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); > if (len > 0) > reset->active_delay = fdt32_to_cpu(*val); >
On 24 Oct 2021, at 19:09, Xiang W <wxjstz@126.com> wrote: > > 在 2021-10-24星期日的 18:26 +0100，Jessica Clarke写道： >>>> >>>> This property is neither documented in the binding specification >>>> for >>>> gpio-restart nor is it checked by Linux. >>> >>> This DT is only processed by opensbi, and linux should be reset >>> through >>> the sbi interface, instead of controlling gpio to perform the reset >>> by >>> itself. >> >> Well, there are several things going on: >> >> 1. In general, you can’t just make up properties in the global >> namespace, that’s how you end up with the kind of mess device >> trees >> have the reputation of having >> 2. Linux doesn’t need this property and supports far more hardware >> than >> OpenSBI does, so why does OpenSBI need knowledge of this property >> but >> Linux doesn’t on any platform for any architecture? >> 3. OpenSBI doesn’t provide the device tree, in general, pre-OpenSBI >> firmware does, be it U-Boot SPL or something else, and in general >> that >> device tree *will* make its way all the way to the OS. Linux might >> go and >> ignore that and use its own device tree because the whole >> situation is a >> mess, but that’s not the point, this can and will be exposed to >> OSes running >> on top. >> >> Jess > > These attributes are under the gpio-restart/gpio-poweroff node and are > not global attributes. By global I mean they do not have a vendor prefix. They occupy the global namespace of gpio-restart/gpio-poweroff property names that is reserved for standard properties. To quote the devicetree specification: > Nonstandard property names should specify a unique string prefix, such > as a stock ticker symbol, identifying the name of the company or > organization that defined the property. This is a nonstandard property, therefore this patch is in violation of that. > If you need to rename, you can submit a new > patch. The rename is added to the current patch, and the commit cannot > be kept clean The onus is on the patch submitter to do the right thing, not the reviewer to fix the submitter’s mistakes. Jess
在 2021-10-24星期日的 13:42 -0500，Samuel Holland写道： > On 10/24/21 11:46 AM, Xiang W wrote: > > The reset voltage may be different under different platforms, so > > fdt > > should describe the reset level. > > FDT already has a way to describe the reset level: the flags cell in > your GPIO specifier. The GPIO controller is responsible for inverting > the levels when GPIO_FLAG_ACTIVE_LOW is present. That way, the > inversion > logic does not need to be duplicated in every GPIO consumer driver. Thanks, great hint! Regards, Xiang W > > > Signed-off-by: Xiang W <wxjstz@126.com> > > --- > > lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/lib/utils/reset/fdt_reset_gpio.c > > b/lib/utils/reset/fdt_reset_gpio.c > > index 4da1450..b834522 100644 > > --- a/lib/utils/reset/fdt_reset_gpio.c > > +++ b/lib/utils/reset/fdt_reset_gpio.c > > @@ -21,16 +21,19 @@ > > > > struct gpio_reset { > > struct gpio_pin pin; > > + u32 active_level; > > u32 active_delay; > > u32 inactive_delay; > > }; > > > > static struct gpio_reset poweroff = { > > + .active_level = 1, > > .active_delay = 100, > > .inactive_delay = 100 > > }; > > > > static struct gpio_reset restart = { > > + .active_level = 1, > > .active_delay = 100, > > .inactive_delay = 100 > > }; > > @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 > > reason) > > > > if (reset) { > > /* drive it active, also inactive->active edge */ > > - gpio_direction_output(&reset->pin, 1); > > + gpio_direction_output(&reset->pin, reset- > > >active_level); > > "1" as an argument to gpio_direction_output/gpio_set always means the > active level, which could be high or low, depending on if reset->pin > has > the GPIO_FLAG_ACTIVE_LOW flag. > > > sbi_timer_mdelay(reset->active_delay); > > > > /* drive inactive, also active->inactive edge */ > > - gpio_set(&reset->pin, 0); > > + gpio_set(&reset->pin, !reset->active_level); > > sbi_timer_mdelay(reset->inactive_delay); > > > > /* drive it active, also inactive->active edge */ > > - gpio_set(&reset->pin, 1); > > + gpio_set(&reset->pin, reset->active_level); > > } > > /* hang !!! */ > > sbi_hart_hang(); > > @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int > > nodeoff, > > return rc; > > } > > > > + val = fdt_getprop(fdt, nodeoff, "active-level", &len); > > + if (len > 0) > > + reset->active_level = !!fdt32_to_cpu(*val); > > + > > Like Jessica said, all device tree properties must be documented in > the > binding for that device. And the bindings for the "gpio-poweroff" and > "gpio-restart" compatible strings live in the Linux kernel tree. So > if > you want to add a new property, that binding must be updated first. > > But here, no new property is needed, because there is already a way > to > do what you want within the existing binding. > > Regards, > Samuel > > > val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); > > if (len > 0) > > reset->active_delay = fdt32_to_cpu(*val); > >
在 2021-10-25星期一的 00:46 +0800，Xiang W写道： > The reset voltage may be different under different platforms, so fdt > should describe the reset level. > > Signed-off-by: Xiang W <wxjstz@126.com> > --- > lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/lib/utils/reset/fdt_reset_gpio.c > b/lib/utils/reset/fdt_reset_gpio.c > index 4da1450..b834522 100644 > --- a/lib/utils/reset/fdt_reset_gpio.c > +++ b/lib/utils/reset/fdt_reset_gpio.c > @@ -21,16 +21,19 @@ > > struct gpio_reset { > struct gpio_pin pin; > + u32 active_level; > u32 active_delay; > u32 inactive_delay; > }; > > static struct gpio_reset poweroff = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > > static struct gpio_reset restart = { > + .active_level = 1, > .active_delay = 100, > .inactive_delay = 100 > }; > @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 > reason) > > if (reset) { > /* drive it active, also inactive->active edge */ > - gpio_direction_output(&reset->pin, 1); > + gpio_direction_output(&reset->pin, reset- > >active_level); > sbi_timer_mdelay(reset->active_delay); > > /* drive inactive, also active->inactive edge */ > - gpio_set(&reset->pin, 0); > + gpio_set(&reset->pin, !reset->active_level); > sbi_timer_mdelay(reset->inactive_delay); > > /* drive it active, also inactive->active edge */ > - gpio_set(&reset->pin, 1); > + gpio_set(&reset->pin, reset->active_level); > } > /* hang !!! */ > sbi_hart_hang(); > @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int > nodeoff, > return rc; > } > > + val = fdt_getprop(fdt, nodeoff, "active-level", &len); > + if (len > 0) > + reset->active_level = !!fdt32_to_cpu(*val); > + > val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); > if (len > 0) > reset->active_delay = fdt32_to_cpu(*val); sorry!!! revoke this patch. Regards, Xiang W
diff --git a/lib/utils/reset/fdt_reset_gpio.c b/lib/utils/reset/fdt_reset_gpio.c index 4da1450..b834522 100644 --- a/lib/utils/reset/fdt_reset_gpio.c +++ b/lib/utils/reset/fdt_reset_gpio.c @@ -21,16 +21,19 @@ struct gpio_reset { struct gpio_pin pin; + u32 active_level; u32 active_delay; u32 inactive_delay; }; static struct gpio_reset poweroff = { + .active_level = 1, .active_delay = 100, .inactive_delay = 100 }; static struct gpio_reset restart = { + .active_level = 1, .active_delay = 100, .inactive_delay = 100 }; @@ -68,15 +71,15 @@ static void gpio_system_reset(u32 type, u32 reason) if (reset) { /* drive it active, also inactive->active edge */ - gpio_direction_output(&reset->pin, 1); + gpio_direction_output(&reset->pin, reset->active_level); sbi_timer_mdelay(reset->active_delay); /* drive inactive, also active->inactive edge */ - gpio_set(&reset->pin, 0); + gpio_set(&reset->pin, !reset->active_level); sbi_timer_mdelay(reset->inactive_delay); /* drive it active, also inactive->active edge */ - gpio_set(&reset->pin, 1); + gpio_set(&reset->pin, reset->active_level); } /* hang !!! */ sbi_hart_hang(); @@ -107,6 +110,10 @@ static int gpio_reset_init(void *fdt, int nodeoff, return rc; } + val = fdt_getprop(fdt, nodeoff, "active-level", &len); + if (len > 0) + reset->active_level = !!fdt32_to_cpu(*val); + val = fdt_getprop(fdt, nodeoff, "active-delay-ms", &len); if (len > 0) reset->active_delay = fdt32_to_cpu(*val);
The reset voltage may be different under different platforms, so fdt should describe the reset level. Signed-off-by: Xiang W <wxjstz@126.com> --- lib/utils/reset/fdt_reset_gpio.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)