| Message ID | 20210302210003.21298-1-judge.packham@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Priyanka Jain |
| Headers | show |
| Series | common: board_f: Restore 85xx watchdog support | expand |
Hi Chris, On Tue, 2 Mar 2021 at 16:00, Chris Packham <judge.packham@gmail.com> wrote: > > In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we > lost the call to init_85xx_watchdog() which had the effect of disabling > support for the watchdog on 85xx and similar SoCs (i.e. the QorIQ P > Series and T Series). > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > --- > Admittedly this is a bit ugly but it's the most literal reinstatement of > the code that was lost. At the very least I should probably rename > init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of > the code. > > The other question is how has this gone unnoticed for ~5 years. I think > the answer is because only the keymile boards were using it. > > Finally I am wondering if this should be converted to a DM driver. But > given the fact that the watchdog is part of the core and not a > peripheral on the SoC I don't know how that would look (e.g. what would > I put in the DTS?). > > So I thought I'd run this up the flag pole as-is and see what feedback I > get. > > common/board_f.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/common/board_f.c b/common/board_f.c > index 0cddf0359dca..3778571a7196 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) > hw_watchdog_init(); > puts(" Watchdog enabled\n"); > # endif > +# ifdef CONFIG_MPC85xx > + init_85xx_watchdog(); That should be in a header file. > + puts(" Watchdog enabled\n"); > +# endif > + > WATCHDOG_RESET(); > > return 0; > -- > 2.30.1 > I don't think you have made the existing code worse...so I suppose it is OK. But really this should be converted to DM. I just noticed there is no watchdog uclass. - Simon
On Fri, 5 Mar 2021, 5:09 PM Simon Glass, <sjg@chromium.org> wrote: > Hi Chris, > > On Tue, 2 Mar 2021 at 16:00, Chris Packham <judge.packham@gmail.com> > wrote: > > > > In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we > > lost the call to init_85xx_watchdog() which had the effect of disabling > > support for the watchdog on 85xx and similar SoCs (i.e. the QorIQ P > > Series and T Series). > > > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > > --- > > Admittedly this is a bit ugly but it's the most literal reinstatement of > > the code that was lost. At the very least I should probably rename > > init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of > > the code. > > > > The other question is how has this gone unnoticed for ~5 years. I think > > the answer is because only the keymile boards were using it. > > > > Finally I am wondering if this should be converted to a DM driver. But > > given the fact that the watchdog is part of the core and not a > > peripheral on the SoC I don't know how that would look (e.g. what would > > I put in the DTS?). > > > > So I thought I'd run this up the flag pole as-is and see what feedback I > > get. > > > > common/board_f.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/common/board_f.c b/common/board_f.c > > index 0cddf0359dca..3778571a7196 100644 > > --- a/common/board_f.c > > +++ b/common/board_f.c > > @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) > > hw_watchdog_init(); > > puts(" Watchdog enabled\n"); > > # endif > > +# ifdef CONFIG_MPC85xx > > + init_85xx_watchdog(); > > That should be in a header file. > > > + puts(" Watchdog enabled\n"); > > +# endif > > + > > WATCHDOG_RESET(); > > > > return 0; > > -- > > 2.30.1 > > > > I don't think you have made the existing code worse...so I suppose it is > OK. > > But really this should be converted to DM. I just noticed there is no > watchdog uclass. > It's called UCLASS_WDT. In the end I went with implementing a DM driver. Assuming that lands there will be some old code that can be removed. I'd still like to here from the keymile maintainers as they're the only in tree user of this. > - Simon >
>-----Original Message----- >From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Chris Packham >Sent: Wednesday, March 3, 2021 2:30 AM >To: u-boot@lists.denx.de >Cc: York Sun <york.sun@nxp.com>; Rainer Boschung ><rainer.boschung@keymile.com>; Chris Packham <judge.packham@gmail.com>; >Masahiro Yamada <masahiroy@kernel.org>; Ovidiu Panait ><ovidiu.panait@windriver.com>; Patrick Delaunay ><patrick.delaunay@foss.st.com>; Simon Glass <sjg@chromium.org>; Stefan >Roese <sr@denx.de>; Stephen Warren <swarren@nvidia.com> >Subject: [PATCH] common: board_f: Restore 85xx watchdog support > >In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we lost >the call to init_85xx_watchdog() which had the effect of disabling support for the >watchdog on 85xx and similar SoCs (i.e. the QorIQ P Series and T Series). > >Signed-off-by: Chris Packham <judge.packham@gmail.com> >--- >Admittedly this is a bit ugly but it's the most literal reinstatement of the code that >was lost. At the very least I should probably rename >init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of the code. > >The other question is how has this gone unnoticed for ~5 years. I think the answer >is because only the keymile boards were using it. > >Finally I am wondering if this should be converted to a DM driver. But given the >fact that the watchdog is part of the core and not a peripheral on the SoC I don't >know how that would look (e.g. what would I put in the DTS?). > >So I thought I'd run this up the flag pole as-is and see what feedback I get. > > common/board_f.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/common/board_f.c b/common/board_f.c index >0cddf0359dca..3778571a7196 100644 >--- a/common/board_f.c >+++ b/common/board_f.c >@@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) > hw_watchdog_init(); > puts(" Watchdog enabled\n"); > # endif >+# ifdef CONFIG_MPC85xx >+ init_85xx_watchdog(); >+ puts(" Watchdog enabled\n"); >+# endif >+ > WATCHDOG_RESET(); > > return 0; >-- >2.30.1 Kindly fix below build error for non-fsl ppc boards powerpc: w+ mpc8308_p1m sbc8349 sbc8349_PCI_33 sbc8349_PCI_66 ve8313 caddy2 vme8349 hrcon hrcon_dh strider_con strider_con_dp strider_cpu strider_cpu_dp ids8313 TQM834x sbc8548 sbc8548_PCI_33 sbc8548_PCI_33_PCIE sbc8548_PCI_66 sbc8548_PCI_66_PCIE socrates UCP1020 kmcent2 kmcoge4 Cyrus_P5020 Cyrus_P5040 xpedite520x xpedite537x xpedite550x sbc8641d xpedite517x MCR3000 + controlcenterd_36BIT_SDCARD controlcenterd_36BIT_SDCARD_DEVELOP controlcenterd_TRAILBLAZER controlcenterd_TRAILBLAZER_DEVELOP +powerpc-linux-ld.bfd: common/built-in.o: in function `init_func_watchdog_init': 2021-03-23T11:09:10.9787128Z +common/board_f.c:115: undefined reference to `init_85xx_watchdog' 2021-03-23T11:09:10.9787600Z +make[1]: *** [u-boot] Error 1 2021-03-23T11:09:10.9787981Z +make: *** [sub-make] Error 2 Regards Priyanka
Hi Chris, On 23.03.21 13:35, Priyanka Jain wrote: > > >> -----Original Message----- >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Chris Packham >> Sent: Wednesday, March 3, 2021 2:30 AM >> To: u-boot@lists.denx.de >> Cc: York Sun <york.sun@nxp.com>; Rainer Boschung >> <rainer.boschung@keymile.com>; Chris Packham <judge.packham@gmail.com>; >> Masahiro Yamada <masahiroy@kernel.org>; Ovidiu Panait >> <ovidiu.panait@windriver.com>; Patrick Delaunay >> <patrick.delaunay@foss.st.com>; Simon Glass <sjg@chromium.org>; Stefan >> Roese <sr@denx.de>; Stephen Warren <swarren@nvidia.com> >> Subject: [PATCH] common: board_f: Restore 85xx watchdog support >> >> In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we lost >> the call to init_85xx_watchdog() which had the effect of disabling support for the >> watchdog on 85xx and similar SoCs (i.e. the QorIQ P Series and T Series). >> >> Signed-off-by: Chris Packham <judge.packham@gmail.com> >> --- >> Admittedly this is a bit ugly but it's the most literal reinstatement of the code that >> was lost. At the very least I should probably rename >> init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of the code. >> >> The other question is how has this gone unnoticed for ~5 years. I think the answer >> is because only the keymile boards were using it. >> >> Finally I am wondering if this should be converted to a DM driver. But given the >> fact that the watchdog is part of the core and not a peripheral on the SoC I don't >> know how that would look (e.g. what would I put in the DTS?). >> >> So I thought I'd run this up the flag pole as-is and see what feedback I get. >> >> common/board_f.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/common/board_f.c b/common/board_f.c index >> 0cddf0359dca..3778571a7196 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) >> hw_watchdog_init(); >> puts(" Watchdog enabled\n"); >> # endif >> +# ifdef CONFIG_MPC85xx >> + init_85xx_watchdog(); >> + puts(" Watchdog enabled\n"); >> +# endif >> + >> WATCHDOG_RESET(); >> >> return 0; >> -- >> 2.30.1 > > Kindly fix below build error for non-fsl ppc boards > powerpc: w+ mpc8308_p1m sbc8349 sbc8349_PCI_33 sbc8349_PCI_66 ve8313 caddy2 vme8349 hrcon hrcon_dh strider_con strider_con_dp strider_cpu strider_cpu_dp ids8313 TQM834x sbc8548 sbc8548_PCI_33 sbc8548_PCI_33_PCIE sbc8548_PCI_66 sbc8548_PCI_66_PCIE socrates UCP1020 kmcent2 kmcoge4 Cyrus_P5020 Cyrus_P5040 xpedite520x xpedite537x xpedite550x sbc8641d xpedite517x MCR3000 + controlcenterd_36BIT_SDCARD controlcenterd_36BIT_SDCARD_DEVELOP controlcenterd_TRAILBLAZER controlcenterd_TRAILBLAZER_DEVELOP > > > +powerpc-linux-ld.bfd: common/built-in.o: in function `init_func_watchdog_init': > 2021-03-23T11:09:10.9787128Z +common/board_f.c:115: undefined reference to `init_85xx_watchdog' > 2021-03-23T11:09:10.9787600Z +make[1]: *** [u-boot] Error 1 > 2021-03-23T11:09:10.9787981Z +make: *** [sub-make] Error 2 Is this patch needed which your DM WDT driver for Booke PPCs? Thanks, Stefan
On Wed, Mar 24, 2021 at 7:06 PM Stefan Roese <sr@denx.de> wrote: > > Hi Chris, > > On 23.03.21 13:35, Priyanka Jain wrote: > > > > > >> -----Original Message----- > >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Chris Packham > >> Sent: Wednesday, March 3, 2021 2:30 AM > >> To: u-boot@lists.denx.de > >> Cc: York Sun <york.sun@nxp.com>; Rainer Boschung > >> <rainer.boschung@keymile.com>; Chris Packham <judge.packham@gmail.com>; > >> Masahiro Yamada <masahiroy@kernel.org>; Ovidiu Panait > >> <ovidiu.panait@windriver.com>; Patrick Delaunay > >> <patrick.delaunay@foss.st.com>; Simon Glass <sjg@chromium.org>; Stefan > >> Roese <sr@denx.de>; Stephen Warren <swarren@nvidia.com> > >> Subject: [PATCH] common: board_f: Restore 85xx watchdog support > >> > >> In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we lost > >> the call to init_85xx_watchdog() which had the effect of disabling support for the > >> watchdog on 85xx and similar SoCs (i.e. the QorIQ P Series and T Series). > >> > >> Signed-off-by: Chris Packham <judge.packham@gmail.com> > >> --- > >> Admittedly this is a bit ugly but it's the most literal reinstatement of the code that > >> was lost. At the very least I should probably rename > >> init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of the code. > >> > >> The other question is how has this gone unnoticed for ~5 years. I think the answer > >> is because only the keymile boards were using it. > >> > >> Finally I am wondering if this should be converted to a DM driver. But given the > >> fact that the watchdog is part of the core and not a peripheral on the SoC I don't > >> know how that would look (e.g. what would I put in the DTS?). > >> > >> So I thought I'd run this up the flag pole as-is and see what feedback I get. > >> > >> common/board_f.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/common/board_f.c b/common/board_f.c index > >> 0cddf0359dca..3778571a7196 100644 > >> --- a/common/board_f.c > >> +++ b/common/board_f.c > >> @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) > >> hw_watchdog_init(); > >> puts(" Watchdog enabled\n"); > >> # endif > >> +# ifdef CONFIG_MPC85xx > >> + init_85xx_watchdog(); > >> + puts(" Watchdog enabled\n"); > >> +# endif > >> + > >> WATCHDOG_RESET(); > >> > >> return 0; > >> -- > >> 2.30.1 > > > > Kindly fix below build error for non-fsl ppc boards > > powerpc: w+ mpc8308_p1m sbc8349 sbc8349_PCI_33 sbc8349_PCI_66 ve8313 caddy2 vme8349 hrcon hrcon_dh strider_con strider_con_dp strider_cpu strider_cpu_dp ids8313 TQM834x sbc8548 sbc8548_PCI_33 sbc8548_PCI_33_PCIE sbc8548_PCI_66 sbc8548_PCI_66_PCIE socrates UCP1020 kmcent2 kmcoge4 Cyrus_P5020 Cyrus_P5040 xpedite520x xpedite537x xpedite550x sbc8641d xpedite517x MCR3000 + controlcenterd_36BIT_SDCARD controlcenterd_36BIT_SDCARD_DEVELOP controlcenterd_TRAILBLAZER controlcenterd_TRAILBLAZER_DEVELOP > > > > > > +powerpc-linux-ld.bfd: common/built-in.o: in function `init_func_watchdog_init': > > 2021-03-23T11:09:10.9787128Z +common/board_f.c:115: undefined reference to `init_85xx_watchdog' > > 2021-03-23T11:09:10.9787600Z +make[1]: *** [u-boot] Error 1 > > 2021-03-23T11:09:10.9787981Z +make: *** [sub-make] Error 2 > > Is this patch needed which your DM WDT driver for Booke PPCs? > The DM driver removes the need for this patch. Assuming we go with that there's a potential follow up to remove the vestiges of init_85xx_watchdog.
On 24.03.21 08:10, Chris Packham wrote: > On Wed, Mar 24, 2021 at 7:06 PM Stefan Roese <sr@denx.de> wrote: >> >> Hi Chris, >> >> On 23.03.21 13:35, Priyanka Jain wrote: >>> >>> >>>> -----Original Message----- >>>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Chris Packham >>>> Sent: Wednesday, March 3, 2021 2:30 AM >>>> To: u-boot@lists.denx.de >>>> Cc: York Sun <york.sun@nxp.com>; Rainer Boschung >>>> <rainer.boschung@keymile.com>; Chris Packham <judge.packham@gmail.com>; >>>> Masahiro Yamada <masahiroy@kernel.org>; Ovidiu Panait >>>> <ovidiu.panait@windriver.com>; Patrick Delaunay >>>> <patrick.delaunay@foss.st.com>; Simon Glass <sjg@chromium.org>; Stefan >>>> Roese <sr@denx.de>; Stephen Warren <swarren@nvidia.com> >>>> Subject: [PATCH] common: board_f: Restore 85xx watchdog support >>>> >>>> In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we lost >>>> the call to init_85xx_watchdog() which had the effect of disabling support for the >>>> watchdog on 85xx and similar SoCs (i.e. the QorIQ P Series and T Series). >>>> >>>> Signed-off-by: Chris Packham <judge.packham@gmail.com> >>>> --- >>>> Admittedly this is a bit ugly but it's the most literal reinstatement of the code that >>>> was lost. At the very least I should probably rename >>>> init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of the code. >>>> >>>> The other question is how has this gone unnoticed for ~5 years. I think the answer >>>> is because only the keymile boards were using it. >>>> >>>> Finally I am wondering if this should be converted to a DM driver. But given the >>>> fact that the watchdog is part of the core and not a peripheral on the SoC I don't >>>> know how that would look (e.g. what would I put in the DTS?). >>>> >>>> So I thought I'd run this up the flag pole as-is and see what feedback I get. >>>> >>>> common/board_f.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/common/board_f.c b/common/board_f.c index >>>> 0cddf0359dca..3778571a7196 100644 >>>> --- a/common/board_f.c >>>> +++ b/common/board_f.c >>>> @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) >>>> hw_watchdog_init(); >>>> puts(" Watchdog enabled\n"); >>>> # endif >>>> +# ifdef CONFIG_MPC85xx >>>> + init_85xx_watchdog(); >>>> + puts(" Watchdog enabled\n"); >>>> +# endif >>>> + >>>> WATCHDOG_RESET(); >>>> >>>> return 0; >>>> -- >>>> 2.30.1 >>> >>> Kindly fix below build error for non-fsl ppc boards >>> powerpc: w+ mpc8308_p1m sbc8349 sbc8349_PCI_33 sbc8349_PCI_66 ve8313 caddy2 vme8349 hrcon hrcon_dh strider_con strider_con_dp strider_cpu strider_cpu_dp ids8313 TQM834x sbc8548 sbc8548_PCI_33 sbc8548_PCI_33_PCIE sbc8548_PCI_66 sbc8548_PCI_66_PCIE socrates UCP1020 kmcent2 kmcoge4 Cyrus_P5020 Cyrus_P5040 xpedite520x xpedite537x xpedite550x sbc8641d xpedite517x MCR3000 + controlcenterd_36BIT_SDCARD controlcenterd_36BIT_SDCARD_DEVELOP controlcenterd_TRAILBLAZER controlcenterd_TRAILBLAZER_DEVELOP >>> >>> >>> +powerpc-linux-ld.bfd: common/built-in.o: in function `init_func_watchdog_init': >>> 2021-03-23T11:09:10.9787128Z +common/board_f.c:115: undefined reference to `init_85xx_watchdog' >>> 2021-03-23T11:09:10.9787600Z +make[1]: *** [u-boot] Error 1 >>> 2021-03-23T11:09:10.9787981Z +make: *** [sub-make] Error 2 >> >> Is this patch needed which your DM WDT driver for Booke PPCs? >> > > The DM driver removes the need for this patch. Assuming we go with > that Yes, I'm fine with this. > there's a potential follow up to remove the vestiges of > init_85xx_watchdog. Nice. Thanks, Stefan
diff --git a/common/board_f.c b/common/board_f.c index 0cddf0359dca..3778571a7196 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -111,6 +111,11 @@ static int init_func_watchdog_init(void) hw_watchdog_init(); puts(" Watchdog enabled\n"); # endif +# ifdef CONFIG_MPC85xx + init_85xx_watchdog(); + puts(" Watchdog enabled\n"); +# endif + WATCHDOG_RESET(); return 0;
In commit 75918afa649b ("powerpc: Drop old non-generic-board code") we lost the call to init_85xx_watchdog() which had the effect of disabling support for the watchdog on 85xx and similar SoCs (i.e. the QorIQ P Series and T Series). Signed-off-by: Chris Packham <judge.packham@gmail.com> --- Admittedly this is a bit ugly but it's the most literal reinstatement of the code that was lost. At the very least I should probably rename init_85xx_watchdog() to hw_watchdog_init() so it fits with the rest of the code. The other question is how has this gone unnoticed for ~5 years. I think the answer is because only the keymile boards were using it. Finally I am wondering if this should be converted to a DM driver. But given the fact that the watchdog is part of the core and not a peripheral on the SoC I don't know how that would look (e.g. what would I put in the DTS?). So I thought I'd run this up the flag pole as-is and see what feedback I get. common/board_f.c | 5 +++++ 1 file changed, 5 insertions(+)