diff mbox series

[v2,6/6] lib: utils/fdt: Disable GPIO restart and poweroff nodes in fdt_fixups()

Message ID 20210709131059.1502293-7-anup.patel@wdc.com
State Superseded
Headers show
Series GPIO reset support | expand

Commit Message

Anup Patel July 9, 2021, 1:10 p.m. UTC 
We now have SBI SRST extension provided via generic GPIO restart and
poweroff similar to Linux kernel so let's disable corresponding
device tree nodes in fdt_fixups().

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 include/sbi_utils/fdt/fdt_fixup.h | 17 +++++++++++++++--
 lib/utils/fdt/fdt_fixup.c         | 25 +++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

Bin Meng July 9, 2021, 2:28 p.m. UTC | #1 
On Fri, Jul 9, 2021 at 9:13 PM Anup Patel <anup.patel@wdc.com> wrote:
>
> We now have SBI SRST extension provided via generic GPIO restart and
> poweroff similar to Linux kernel so let's disable corresponding
> device tree nodes in fdt_fixups().
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> ---
>  include/sbi_utils/fdt/fdt_fixup.h | 17 +++++++++++++++--
>  lib/utils/fdt/fdt_fixup.c         | 25 +++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
> index c38e5d9..cc86440 100644
> --- a/include/sbi_utils/fdt/fdt_fixup.h
> +++ b/include/sbi_utils/fdt/fdt_fixup.h
> @@ -21,6 +21,18 @@
>   */
>  void fdt_cpu_fixup(void *fdt);
>
> +/**
> + * Fix up the GPIO restart and poweroff nodes in the device tree
> + *
> + * This routine disabls the GPIO restart and poweroff nodes in the device

typo: disables

> + * tree because OpenSBI provides SBI SRST extension based on it.
> + *
> + * It is recommended that platform codes call this helper in their final_init()
> + *
> + * @param fdt: device tree blob
> + */
> +void fdt_gpio_reset_fixup(void *fdt);
> +
>  /**
>   * Fix up the PLIC node in the device tree
>   *
> @@ -64,8 +76,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt);
>   * General device tree fix-up
>   *
>   * This routine do all required device tree fix-ups for a typical platform.
> - * It fixes up the PLIC node and the reserved memory node in the device tree
> - * by calling the corresponding helper routines to accomplish the task.
> + * It fixes up the GPIO restart/poweroff node, PLIC node and the reserved
> + * memory node in the device tree by calling the corresponding helper
> + * routines to accomplish the task.
>   *
>   * It is recommended that platform codes call this helper in their final_init()
>   *
> diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
> index 1465500..52d1a0e 100644
> --- a/lib/utils/fdt/fdt_fixup.c
> +++ b/lib/utils/fdt/fdt_fixup.c
> @@ -51,6 +51,27 @@ void fdt_cpu_fixup(void *fdt)
>         }
>  }
>
> +void fdt_gpio_reset_fixup(void *fdt)
> +{
> +       int err, nodeoff;
> +
> +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-poweroff");
> +       if (nodeoff >= 0) {
> +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
> +               if (err < 0)
> +                       return;
> +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");

For OS that already supports gpio-poweroff/restart natively in the
kernel, but has not yet to support SBI SRST extension, this change
suddenly breaks them.

> +       }
> +
> +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-restart");
> +       if (nodeoff >= 0) {
> +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
> +               if (err < 0)
> +                       return;
> +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");
> +       }
> +}
> +
>  void fdt_plic_fixup(void *fdt)
>  {
>         u32 *cells;
> @@ -260,9 +281,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt)
>
>  void fdt_fixups(void *fdt)
>  {
> +       fdt_gpio_reset_fixup(fdt);
> +
>         fdt_plic_fixup(fdt);
>
>         fdt_reserved_memory_fixup(fdt);
>  }

Regards,
Bin
Atish Patra July 9, 2021, 7:09 p.m. UTC | #2 
On Fri, Jul 9, 2021 at 7:29 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Jul 9, 2021 at 9:13 PM Anup Patel <anup.patel@wdc.com> wrote:
> >
> > We now have SBI SRST extension provided via generic GPIO restart and
> > poweroff similar to Linux kernel so let's disable corresponding
> > device tree nodes in fdt_fixups().
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > ---
> >  include/sbi_utils/fdt/fdt_fixup.h | 17 +++++++++++++++--
> >  lib/utils/fdt/fdt_fixup.c         | 25 +++++++++++++++++++++++--
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
> > index c38e5d9..cc86440 100644
> > --- a/include/sbi_utils/fdt/fdt_fixup.h
> > +++ b/include/sbi_utils/fdt/fdt_fixup.h
> > @@ -21,6 +21,18 @@
> >   */
> >  void fdt_cpu_fixup(void *fdt);
> >
> > +/**
> > + * Fix up the GPIO restart and poweroff nodes in the device tree
> > + *
> > + * This routine disabls the GPIO restart and poweroff nodes in the device
>
> typo: disables
>
> > + * tree because OpenSBI provides SBI SRST extension based on it.
> > + *
> > + * It is recommended that platform codes call this helper in their final_init()
> > + *
> > + * @param fdt: device tree blob
> > + */
> > +void fdt_gpio_reset_fixup(void *fdt);
> > +
> >  /**
> >   * Fix up the PLIC node in the device tree
> >   *
> > @@ -64,8 +76,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt);
> >   * General device tree fix-up
> >   *
> >   * This routine do all required device tree fix-ups for a typical platform.
> > - * It fixes up the PLIC node and the reserved memory node in the device tree
> > - * by calling the corresponding helper routines to accomplish the task.
> > + * It fixes up the GPIO restart/poweroff node, PLIC node and the reserved
> > + * memory node in the device tree by calling the corresponding helper
> > + * routines to accomplish the task.
> >   *
> >   * It is recommended that platform codes call this helper in their final_init()
> >   *
> > diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
> > index 1465500..52d1a0e 100644
> > --- a/lib/utils/fdt/fdt_fixup.c
> > +++ b/lib/utils/fdt/fdt_fixup.c
> > @@ -51,6 +51,27 @@ void fdt_cpu_fixup(void *fdt)
> >         }
> >  }
> >
> > +void fdt_gpio_reset_fixup(void *fdt)
> > +{
> > +       int err, nodeoff;
> > +
> > +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-poweroff");
> > +       if (nodeoff >= 0) {
> > +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
> > +               if (err < 0)
> > +                       return;
> > +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");
>
> For OS that already supports gpio-poweroff/restart natively in the
> kernel, but has not yet to support SBI SRST extension, this change
> suddenly breaks them.
>

I agree. In addition to that, SRST extension is optional. So we
shouldn't disable the
DT node assuming that the OS will use SRST extension to restart/poweroff.

> > +       }
> > +
> > +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-restart");
> > +       if (nodeoff >= 0) {
> > +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
> > +               if (err < 0)
> > +                       return;
> > +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");
> > +       }
> > +}
> > +
> >  void fdt_plic_fixup(void *fdt)
> >  {
> >         u32 *cells;
> > @@ -260,9 +281,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt)
> >
> >  void fdt_fixups(void *fdt)
> >  {
> > +       fdt_gpio_reset_fixup(fdt);
> > +
> >         fdt_plic_fixup(fdt);
> >
> >         fdt_reserved_memory_fixup(fdt);
> >  }
>
> Regards,
> Bin
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel July 10, 2021, 5:11 a.m. UTC | #3 
On 10/07/21, 12:39 AM, "Atish Patra" <atishp@atishpatra.org> wrote:

    On Fri, Jul 9, 2021 at 7:29 AM Bin Meng <bmeng.cn@gmail.com> wrote:
    >
    > On Fri, Jul 9, 2021 at 9:13 PM Anup Patel <anup.patel@wdc.com> wrote:
    > >
    > > We now have SBI SRST extension provided via generic GPIO restart and
    > > poweroff similar to Linux kernel so let's disable corresponding
    > > device tree nodes in fdt_fixups().
    > >
    > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
    > > ---
    > >  include/sbi_utils/fdt/fdt_fixup.h | 17 +++++++++++++++--
    > >  lib/utils/fdt/fdt_fixup.c         | 25 +++++++++++++++++++++++--
    > >  2 files changed, 38 insertions(+), 4 deletions(-)
    > >
    > > diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
    > > index c38e5d9..cc86440 100644
    > > --- a/include/sbi_utils/fdt/fdt_fixup.h
    > > +++ b/include/sbi_utils/fdt/fdt_fixup.h
    > > @@ -21,6 +21,18 @@
    > >   */
    > >  void fdt_cpu_fixup(void *fdt);
    > >
    > > +/**
    > > + * Fix up the GPIO restart and poweroff nodes in the device tree
    > > + *
    > > + * This routine disabls the GPIO restart and poweroff nodes in the device
    >
    > typo: disables
    >
    > > + * tree because OpenSBI provides SBI SRST extension based on it.
    > > + *
    > > + * It is recommended that platform codes call this helper in their final_init()
    > > + *
    > > + * @param fdt: device tree blob
    > > + */
    > > +void fdt_gpio_reset_fixup(void *fdt);
    > > +
    > >  /**
    > >   * Fix up the PLIC node in the device tree
    > >   *
    > > @@ -64,8 +76,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt);
    > >   * General device tree fix-up
    > >   *
    > >   * This routine do all required device tree fix-ups for a typical platform.
    > > - * It fixes up the PLIC node and the reserved memory node in the device tree
    > > - * by calling the corresponding helper routines to accomplish the task.
    > > + * It fixes up the GPIO restart/poweroff node, PLIC node and the reserved
    > > + * memory node in the device tree by calling the corresponding helper
    > > + * routines to accomplish the task.
    > >   *
    > >   * It is recommended that platform codes call this helper in their final_init()
    > >   *
    > > diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
    > > index 1465500..52d1a0e 100644
    > > --- a/lib/utils/fdt/fdt_fixup.c
    > > +++ b/lib/utils/fdt/fdt_fixup.c
    > > @@ -51,6 +51,27 @@ void fdt_cpu_fixup(void *fdt)
    > >         }
    > >  }
    > >
    > > +void fdt_gpio_reset_fixup(void *fdt)
    > > +{
    > > +       int err, nodeoff;
    > > +
    > > +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-poweroff");
    > > +       if (nodeoff >= 0) {
    > > +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
    > > +               if (err < 0)
    > > +                       return;
    > > +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");
    >
    > For OS that already supports gpio-poweroff/restart natively in the
    > kernel, but has not yet to support SBI SRST extension, this change
    > suddenly breaks them.
    >

    I agree. In addition to that, SRST extension is optional. So we
    shouldn't disable the
    DT node assuming that the OS will use SRST extension to restart/poweroff.

Sure, no problem. I will drop this patch.

Regards,
Anup

    > > +       }
    > > +
    > > +       nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-restart");
    > > +       if (nodeoff >= 0) {
    > > +               err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
    > > +               if (err < 0)
    > > +                       return;
    > > +               fdt_setprop_string(fdt, nodeoff, "status", "disabled");
    > > +       }
    > > +}
    > > +
    > >  void fdt_plic_fixup(void *fdt)
    > >  {
    > >         u32 *cells;
    > > @@ -260,9 +281,9 @@ int fdt_reserved_memory_nomap_fixup(void *fdt)
    > >
    > >  void fdt_fixups(void *fdt)
    > >  {
    > > +       fdt_gpio_reset_fixup(fdt);
    > > +
    > >         fdt_plic_fixup(fdt);
    > >
    > >         fdt_reserved_memory_fixup(fdt);
    > >  }
    >
    > Regards,
    > Bin
    >
    > --
    > opensbi mailing list
    > opensbi@lists.infradead.org
    > http://lists.infradead.org/mailman/listinfo/opensbi



    -- 
    Regards,
    Atish
diff mbox series

Patch

diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
index c38e5d9..cc86440 100644
--- a/include/sbi_utils/fdt/fdt_fixup.h
+++ b/include/sbi_utils/fdt/fdt_fixup.h
@@ -21,6 +21,18 @@ 
  */
 void fdt_cpu_fixup(void *fdt);
 
+/**
+ * Fix up the GPIO restart and poweroff nodes in the device tree
+ *
+ * This routine disabls the GPIO restart and poweroff nodes in the device
+ * tree because OpenSBI provides SBI SRST extension based on it.
+ *
+ * It is recommended that platform codes call this helper in their final_init()
+ *
+ * @param fdt: device tree blob
+ */
+void fdt_gpio_reset_fixup(void *fdt);
+
 /**
  * Fix up the PLIC node in the device tree
  *
@@ -64,8 +76,9 @@  int fdt_reserved_memory_nomap_fixup(void *fdt);
  * General device tree fix-up
  *
  * This routine do all required device tree fix-ups for a typical platform.
- * It fixes up the PLIC node and the reserved memory node in the device tree
- * by calling the corresponding helper routines to accomplish the task.
+ * It fixes up the GPIO restart/poweroff node, PLIC node and the reserved
+ * memory node in the device tree by calling the corresponding helper
+ * routines to accomplish the task.
  *
  * It is recommended that platform codes call this helper in their final_init()
  *
diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
index 1465500..52d1a0e 100644
--- a/lib/utils/fdt/fdt_fixup.c
+++ b/lib/utils/fdt/fdt_fixup.c
@@ -51,6 +51,27 @@  void fdt_cpu_fixup(void *fdt)
 	}
 }
 
+void fdt_gpio_reset_fixup(void *fdt)
+{
+	int err, nodeoff;
+
+	nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-poweroff");
+	if (nodeoff >= 0) {
+		err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
+		if (err < 0)
+			return;
+		fdt_setprop_string(fdt, nodeoff, "status", "disabled");
+	}
+
+	nodeoff = fdt_node_offset_by_compatible(fdt, 0, "gpio-restart");
+	if (nodeoff >= 0) {
+		err = fdt_open_into(fdt, fdt, fdt_totalsize(fdt) + 32);
+		if (err < 0)
+			return;
+		fdt_setprop_string(fdt, nodeoff, "status", "disabled");
+	}
+}
+
 void fdt_plic_fixup(void *fdt)
 {
 	u32 *cells;
@@ -260,9 +281,9 @@  int fdt_reserved_memory_nomap_fixup(void *fdt)
 
 void fdt_fixups(void *fdt)
 {
+	fdt_gpio_reset_fixup(fdt);
+
 	fdt_plic_fixup(fdt);
 
 	fdt_reserved_memory_fixup(fdt);
 }
-
-