| Message ID | 1527530497-10392-2-git-send-email-ray.jui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: > Consolidate two SP805 binding documents "arm,sp805.txt" and > "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the > desired compatible string to be used > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > --- > .../devicetree/bindings/watchdog/arm,sp805.txt | 27 ++++++++++++++----- > .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ---------------------- > 2 files changed, 20 insertions(+), 38 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt Would be good to get a ACK from FSL/NXP person on this. It looks to me like the driver fetches the wrong clock as it gets the first one and the driver really wants 'wdog_clk'. In any case, their dts files should be updated. Reviewed-by: Rob Herring <robh@kernel.org> Rob
On 06/05/2018 12:41 PM, Rob Herring wrote: > On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: >> Consolidate two SP805 binding documents "arm,sp805.txt" and >> "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the >> desired compatible string to be used >> >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> --- >> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 ++++++++++++++----- >> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ---------------------- >> 2 files changed, 20 insertions(+), 38 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > Would be good to get a ACK from FSL/NXP person on this. It looks to me > like the driver fetches the wrong clock as it gets the first one and the > driver really wants 'wdog_clk'. In any case, their dts files should be > updated. > This is really confusing, since he deleted file lists apb_pclk first. Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear to me. arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at least it says so. The fsl dts files all have apb_pclk first. Either case, why are two clocks asked for in the first place ? Are there situations where the second clock is actually used/useful ? Guenter > Reviewed-by: Rob Herring <robh@kernel.org> > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On 06/05/2018 12:41 PM, Rob Herring wrote: >> >> On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: >>> >>> Consolidate two SP805 binding documents "arm,sp805.txt" and >>> "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the >>> desired compatible string to be used >>> >>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>> --- >>> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 >>> ++++++++++++++----- >>> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 >>> ---------------------- >>> 2 files changed, 20 insertions(+), 38 deletions(-) >>> delete mode 100644 >>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >> >> >> Would be good to get a ACK from FSL/NXP person on this. It looks to me >> like the driver fetches the wrong clock as it gets the first one and the >> driver really wants 'wdog_clk'. In any case, their dts files should be >> updated. >> > > This is really confusing, since he deleted file lists apb_pclk first. > Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear to me. > arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at > least > it says so. Note that that clock source is 32KHz. That is obviously a mistake because no one clocks their bus/register interface at 32KHz. Someone just filled in something that happened to work. > The fsl dts files all have apb_pclk first. It's all kind of a mess, but fortunately one we should be able to clean-up. The compatible string changes too, but AMBA bus devices don't actually use the compatible string as they use the ID registers to match. I suppose some other OS could do things differently. Worth the risk to clean-up IMO. > > Either case, why are two clocks asked for in the first place ? Are there > situations where the second clock is actually used/useful ? For clocks, the bus needs "apb_pclk" and the driver just gets the first clock. The driver is obviously going to want the functional clock that determines the counter rate. That should Primecell peripherals are about the only ones that have clear specs WRT clock inputs. Yet we've still managed to screw them up. There are 2 clocks in the spec, so the DT has (or should have) 2 clocks. Rob
On 6/6/2018 9:33 AM, Rob Herring wrote: > On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 06/05/2018 12:41 PM, Rob Herring wrote: >>> >>> On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: >>>> >>>> Consolidate two SP805 binding documents "arm,sp805.txt" and >>>> "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the >>>> desired compatible string to be used >>>> >>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>> --- >>>> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 >>>> ++++++++++++++----- >>>> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 >>>> ---------------------- >>>> 2 files changed, 20 insertions(+), 38 deletions(-) >>>> delete mode 100644 >>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>> >>> >>> Would be good to get a ACK from FSL/NXP person on this. It looks to me >>> like the driver fetches the wrong clock as it gets the first one and the >>> driver really wants 'wdog_clk'. In any case, their dts files should be >>> updated. >>> >> >> This is really confusing, since he deleted file lists apb_pclk first. >> Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear to me. >> arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at >> least >> it says so. > > Note that that clock source is 32KHz. That is obviously a mistake > because no one clocks their bus/register interface at 32KHz. Someone > just filled in something that happened to work. > >> The fsl dts files all have apb_pclk first. > > It's all kind of a mess, but fortunately one we should be able to clean-up. > It is indeed a mess. Note the SP805 driver only derive one clock from DT, and that's not done based on name. As a result, the first clock defined in DT will be fetched and the rate calculation will be carried out based on that clock rate. I assumed the clock entries and their names defined in the binding document are just placeholders, at least for the 2nd clock. Based on how the current driver is, the first clock needs to be the WDOGCLK for things to work properly. According to the SP805 TRM, APB clock is the PCLK, that drives the bus for register access. The relationship between WDOGCLK and PCLK is defined as: - the rising edges of WDOGCLK must be synchronous and balanced with a rising edge of PCLK - the WDOGCLK frequency cannot be greater than the PCLK frequency > The compatible string changes too, but AMBA bus devices don't actually > use the compatible string as they use the ID registers to match. I > suppose some other OS could do things differently. Worth the risk to > clean-up IMO. > >> >> Either case, why are two clocks asked for in the first place ? Are there >> situations where the second clock is actually used/useful ? > > For clocks, the bus needs "apb_pclk" and the driver just gets the > first clock. The driver is obviously going to want the functional > clock that determines the counter rate. That should > > Primecell peripherals are about the only ones that have clear specs > WRT clock inputs. Yet we've still managed to screw them up. There are > 2 clocks in the spec, so the DT has (or should have) 2 clocks. > > Rob > Let me know how you guys want to proceed with this? Thanks, Ray
Hi Guenter/Rob, Kindly let me know how you want to proceed with this? Thanks, Ray On 6/6/2018 4:39 PM, Ray Jui wrote: > > > On 6/6/2018 9:33 AM, Rob Herring wrote: >> On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> >> wrote: >>> On 06/05/2018 12:41 PM, Rob Herring wrote: >>>> >>>> On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: >>>>> >>>>> Consolidate two SP805 binding documents "arm,sp805.txt" and >>>>> "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the >>>>> desired compatible string to be used >>>>> >>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>> --- >>>>> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 >>>>> ++++++++++++++----- >>>>> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 >>>>> ---------------------- >>>>> 2 files changed, 20 insertions(+), 38 deletions(-) >>>>> delete mode 100644 >>>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>> >>>> >>>> Would be good to get a ACK from FSL/NXP person on this. It looks to me >>>> like the driver fetches the wrong clock as it gets the first one and >>>> the >>>> driver really wants 'wdog_clk'. In any case, their dts files should be >>>> updated. >>>> >>> >>> This is really confusing, since he deleted file lists apb_pclk first. >>> Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear >>> to me. >>> arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at >>> least >>> it says so. >> >> Note that that clock source is 32KHz. That is obviously a mistake >> because no one clocks their bus/register interface at 32KHz. Someone >> just filled in something that happened to work. >> >>> The fsl dts files all have apb_pclk first. >> >> It's all kind of a mess, but fortunately one we should be able to >> clean-up. >> > > It is indeed a mess. Note the SP805 driver only derive one clock from > DT, and that's not done based on name. As a result, the first clock > defined in DT will be fetched and the rate calculation will be carried > out based on that clock rate. > > I assumed the clock entries and their names defined in the binding > document are just placeholders, at least for the 2nd clock. > > Based on how the current driver is, the first clock needs to be the > WDOGCLK for things to work properly. > > According to the SP805 TRM, APB clock is the PCLK, that drives the bus > for register access. > > The relationship between WDOGCLK and PCLK is defined as: > > - the rising edges of WDOGCLK must be synchronous and > balanced with a rising edge of PCLK > > - the WDOGCLK frequency cannot be greater than the PCLK > frequency > >> The compatible string changes too, but AMBA bus devices don't actually >> use the compatible string as they use the ID registers to match. I >> suppose some other OS could do things differently. Worth the risk to >> clean-up IMO. >> >>> >>> Either case, why are two clocks asked for in the first place ? Are there >>> situations where the second clock is actually used/useful ? >> >> For clocks, the bus needs "apb_pclk" and the driver just gets the >> first clock. The driver is obviously going to want the functional >> clock that determines the counter rate. That should >> >> Primecell peripherals are about the only ones that have clear specs >> WRT clock inputs. Yet we've still managed to screw them up. There are >> 2 clocks in the spec, so the DT has (or should have) 2 clocks. >> >> Rob >> > > Let me know how you guys want to proceed with this? > > Thanks, > > Ray
On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: > Hi Guenter/Rob, > > Kindly let me know how you want to proceed with this? > If I recall correctly, the patch series does not add a new problem but merely exposes one. Is my recollection correct ? If so, maybe we should just add a note somewhere indicating what might be wrong and otherwise apply the series. Does this make sense ? Guenter > Thanks, > > Ray > > On 6/6/2018 4:39 PM, Ray Jui wrote: > > > > > >On 6/6/2018 9:33 AM, Rob Herring wrote: > >>On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> > >>wrote: > >>>On 06/05/2018 12:41 PM, Rob Herring wrote: > >>>> > >>>>On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: > >>>>> > >>>>>Consolidate two SP805 binding documents "arm,sp805.txt" and > >>>>>"sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the > >>>>>desired compatible string to be used > >>>>> > >>>>>Signed-off-by: Ray Jui <ray.jui@broadcom.com> > >>>>>--- > >>>>> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 > >>>>>++++++++++++++----- > >>>>> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 > >>>>>---------------------- > >>>>> 2 files changed, 20 insertions(+), 38 deletions(-) > >>>>> delete mode 100644 > >>>>>Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >>>> > >>>> > >>>>Would be good to get a ACK from FSL/NXP person on this. It looks to me > >>>>like the driver fetches the wrong clock as it gets the first one and > >>>>the > >>>>driver really wants 'wdog_clk'. In any case, their dts files should be > >>>>updated. > >>>> > >>> > >>>This is really confusing, since he deleted file lists apb_pclk first. > >>>Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear > >>>to me. > >>>arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at > >>>least > >>>it says so. > >> > >>Note that that clock source is 32KHz. That is obviously a mistake > >>because no one clocks their bus/register interface at 32KHz. Someone > >>just filled in something that happened to work. > >> > >>>The fsl dts files all have apb_pclk first. > >> > >>It's all kind of a mess, but fortunately one we should be able to > >>clean-up. > >> > > > >It is indeed a mess. Note the SP805 driver only derive one clock from DT, > >and that's not done based on name. As a result, the first clock defined in > >DT will be fetched and the rate calculation will be carried out based on > >that clock rate. > > > >I assumed the clock entries and their names defined in the binding > >document are just placeholders, at least for the 2nd clock. > > > >Based on how the current driver is, the first clock needs to be the > >WDOGCLK for things to work properly. > > > >According to the SP805 TRM, APB clock is the PCLK, that drives the bus for > >register access. > > > >The relationship between WDOGCLK and PCLK is defined as: > > > >- the rising edges of WDOGCLK must be synchronous and > >balanced with a rising edge of PCLK > > > >- the WDOGCLK frequency cannot be greater than the PCLK > >frequency > > > >>The compatible string changes too, but AMBA bus devices don't actually > >>use the compatible string as they use the ID registers to match. I > >>suppose some other OS could do things differently. Worth the risk to > >>clean-up IMO. > >> > >>> > >>>Either case, why are two clocks asked for in the first place ? Are there > >>>situations where the second clock is actually used/useful ? > >> > >>For clocks, the bus needs "apb_pclk" and the driver just gets the > >>first clock. The driver is obviously going to want the functional > >>clock that determines the counter rate. That should > >> > >>Primecell peripherals are about the only ones that have clear specs > >>WRT clock inputs. Yet we've still managed to screw them up. There are > >>2 clocks in the spec, so the DT has (or should have) 2 clocks. > >> > >>Rob > >> > > > >Let me know how you guys want to proceed with this? > > > >Thanks, > > > >Ray > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/27/2018 11:33 AM, Guenter Roeck wrote: > On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: >> Hi Guenter/Rob, >> >> Kindly let me know how you want to proceed with this? >> > > If I recall correctly, the patch series does not add a new problem > but merely exposes one. Is my recollection correct ? If so, maybe > we should just add a note somewhere indicating what might be wrong > and otherwise apply the series. > > Does this make sense ? Yes this makes a lot of sense to me. This patch series exposes potential problems in some SoCs that they might not be feeding the correct clock into WDT, at least based on clock names from their DT entries. This patch series does not change/affect how SP805 works on those systems. Where should the note be added? Many thanks! Ray > > Guenter > >> Thanks, >> >> Ray >> >> On 6/6/2018 4:39 PM, Ray Jui wrote: >>> >>> >>> On 6/6/2018 9:33 AM, Rob Herring wrote: >>>> On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck <linux@roeck-us.net> >>>> wrote: >>>>> On 06/05/2018 12:41 PM, Rob Herring wrote: >>>>>> >>>>>> On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: >>>>>>> >>>>>>> Consolidate two SP805 binding documents "arm,sp805.txt" and >>>>>>> "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the >>>>>>> desired compatible string to be used >>>>>>> >>>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/watchdog/arm,sp805.txt | 27 >>>>>>> ++++++++++++++----- >>>>>>> .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 >>>>>>> ---------------------- >>>>>>> 2 files changed, 20 insertions(+), 38 deletions(-) >>>>>>> delete mode 100644 >>>>>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>>>> >>>>>> >>>>>> Would be good to get a ACK from FSL/NXP person on this. It looks to me >>>>>> like the driver fetches the wrong clock as it gets the first one and >>>>>> the >>>>>> driver really wants 'wdog_clk'. In any case, their dts files should be >>>>>> updated. >>>>>> >>>>> >>>>> This is really confusing, since he deleted file lists apb_pclk first. >>>>> Does the watchdog driver need apb_pclk or wdog_clk ? That isn't clear >>>>> to me. >>>>> arch/arm64/boot/dts/hisilicon/hi3660.dtsi only provides apb_pclk, or at >>>>> least >>>>> it says so. >>>> >>>> Note that that clock source is 32KHz. That is obviously a mistake >>>> because no one clocks their bus/register interface at 32KHz. Someone >>>> just filled in something that happened to work. >>>> >>>>> The fsl dts files all have apb_pclk first. >>>> >>>> It's all kind of a mess, but fortunately one we should be able to >>>> clean-up. >>>> >>> >>> It is indeed a mess. Note the SP805 driver only derive one clock from DT, >>> and that's not done based on name. As a result, the first clock defined in >>> DT will be fetched and the rate calculation will be carried out based on >>> that clock rate. >>> >>> I assumed the clock entries and their names defined in the binding >>> document are just placeholders, at least for the 2nd clock. >>> >>> Based on how the current driver is, the first clock needs to be the >>> WDOGCLK for things to work properly. >>> >>> According to the SP805 TRM, APB clock is the PCLK, that drives the bus for >>> register access. >>> >>> The relationship between WDOGCLK and PCLK is defined as: >>> >>> - the rising edges of WDOGCLK must be synchronous and >>> balanced with a rising edge of PCLK >>> >>> - the WDOGCLK frequency cannot be greater than the PCLK >>> frequency >>> >>>> The compatible string changes too, but AMBA bus devices don't actually >>>> use the compatible string as they use the ID registers to match. I >>>> suppose some other OS could do things differently. Worth the risk to >>>> clean-up IMO. >>>> >>>>> >>>>> Either case, why are two clocks asked for in the first place ? Are there >>>>> situations where the second clock is actually used/useful ? >>>> >>>> For clocks, the bus needs "apb_pclk" and the driver just gets the >>>> first clock. The driver is obviously going to want the functional >>>> clock that determines the counter rate. That should >>>> >>>> Primecell peripherals are about the only ones that have clear specs >>>> WRT clock inputs. Yet we've still managed to screw them up. There are >>>> 2 clocks in the spec, so the DT has (or should have) 2 clocks. >>>> >>>> Rob >>>> >>> >>> Let me know how you guys want to proceed with this? >>> >>> Thanks, >>> >>> Ray >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 11:38:48AM -0700, Ray Jui wrote: > > > On 6/27/2018 11:33 AM, Guenter Roeck wrote: > >On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: > >>Hi Guenter/Rob, > >> > >>Kindly let me know how you want to proceed with this? > >> > > > >If I recall correctly, the patch series does not add a new problem > >but merely exposes one. Is my recollection correct ? If so, maybe > >we should just add a note somewhere indicating what might be wrong > >and otherwise apply the series. > > > >Does this make sense ? > > Yes this makes a lot of sense to me. This patch series exposes potential > problems in some SoCs that they might not be feeding the correct clock into > WDT, at least based on clock names from their DT entries. > > This patch series does not change/affect how SP805 works on those systems. > > Where should the note be added? > I would suggest to add a note into the driver where the clock is used, with the details discussed here. Does this make sense ? Thanks, Guenter
On 6/27/2018 11:42 AM, Guenter Roeck wrote: > On Wed, Jun 27, 2018 at 11:38:48AM -0700, Ray Jui wrote: >> >> >> On 6/27/2018 11:33 AM, Guenter Roeck wrote: >>> On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: >>>> Hi Guenter/Rob, >>>> >>>> Kindly let me know how you want to proceed with this? >>>> >>> >>> If I recall correctly, the patch series does not add a new problem >>> but merely exposes one. Is my recollection correct ? If so, maybe >>> we should just add a note somewhere indicating what might be wrong >>> and otherwise apply the series. >>> >>> Does this make sense ? >> >> Yes this makes a lot of sense to me. This patch series exposes potential >> problems in some SoCs that they might not be feeding the correct clock into >> WDT, at least based on clock names from their DT entries. >> >> This patch series does not change/affect how SP805 works on those systems. >> >> Where should the note be added? >> > > I would suggest to add a note into the driver where the clock is used, > with the details discussed here. I assume you meant adding the notes to the SP805 driver where the clock is used. If so, I think that makes sense. That notes deserves its own patch because it really has nothing to do with any of the change in this patch series. Do you want me to 1) embed that patch into this patch series and send out v5; or 2) leave the patch series as it is and send out a separate patch to add the notes to the driver? Thanks. > > Does this make sense ? > > Thanks, > Guenter >
On Wed, Jun 27, 2018 at 11:47:21AM -0700, Ray Jui wrote: > > > On 6/27/2018 11:42 AM, Guenter Roeck wrote: > >On Wed, Jun 27, 2018 at 11:38:48AM -0700, Ray Jui wrote: > >> > >> > >>On 6/27/2018 11:33 AM, Guenter Roeck wrote: > >>>On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: > >>>>Hi Guenter/Rob, > >>>> > >>>>Kindly let me know how you want to proceed with this? > >>>> > >>> > >>>If I recall correctly, the patch series does not add a new problem > >>>but merely exposes one. Is my recollection correct ? If so, maybe > >>>we should just add a note somewhere indicating what might be wrong > >>>and otherwise apply the series. > >>> > >>>Does this make sense ? > >> > >>Yes this makes a lot of sense to me. This patch series exposes potential > >>problems in some SoCs that they might not be feeding the correct clock into > >>WDT, at least based on clock names from their DT entries. > >> > >>This patch series does not change/affect how SP805 works on those systems. > >> > >>Where should the note be added? > >> > > > >I would suggest to add a note into the driver where the clock is used, > >with the details discussed here. > > I assume you meant adding the notes to the SP805 driver where the clock is > used. > > If so, I think that makes sense. That notes deserves its own patch because > it really has nothing to do with any of the change in this patch series. > > Do you want me to 1) embed that patch into this patch series and send out > v5; or 2) leave the patch series as it is and send out a separate patch to > add the notes to the driver? > 2) is fine. I don't have the series here right now; if I recall correctly all patches in the series are all marked as Reviewed-by: and/or Acked-by:. If so, I'll apply them to my tree tonight, or at least the ones that will go in through the watchdog tree. Guenter
Hi Guenter/Florian, On 6/27/2018 11:55 AM, Guenter Roeck wrote: > On Wed, Jun 27, 2018 at 11:47:21AM -0700, Ray Jui wrote: >> >> >> On 6/27/2018 11:42 AM, Guenter Roeck wrote: >>> On Wed, Jun 27, 2018 at 11:38:48AM -0700, Ray Jui wrote: >>>> >>>> >>>> On 6/27/2018 11:33 AM, Guenter Roeck wrote: >>>>> On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: >>>>>> Hi Guenter/Rob, >>>>>> >>>>>> Kindly let me know how you want to proceed with this? >>>>>> >>>>> >>>>> If I recall correctly, the patch series does not add a new problem >>>>> but merely exposes one. Is my recollection correct ? If so, maybe >>>>> we should just add a note somewhere indicating what might be wrong >>>>> and otherwise apply the series. >>>>> >>>>> Does this make sense ? >>>> >>>> Yes this makes a lot of sense to me. This patch series exposes potential >>>> problems in some SoCs that they might not be feeding the correct clock into >>>> WDT, at least based on clock names from their DT entries. >>>> >>>> This patch series does not change/affect how SP805 works on those systems. >>>> >>>> Where should the note be added? >>>> >>> >>> I would suggest to add a note into the driver where the clock is used, >>> with the details discussed here. >> >> I assume you meant adding the notes to the SP805 driver where the clock is >> used. >> >> If so, I think that makes sense. That notes deserves its own patch because >> it really has nothing to do with any of the change in this patch series. >> >> Do you want me to 1) embed that patch into this patch series and send out >> v5; or 2) leave the patch series as it is and send out a separate patch to >> add the notes to the driver? >> > 2) is fine. I don't have the series here right now; if I recall correctly > all patches in the series are all marked as Reviewed-by: and/or Acked-by:. > If so, I'll apply them to my tree tonight, or at least the ones that will > go in through the watchdog tree. > > Guenter > Guenter, please let us know once you take the first 4 patches. Florian, could you please help with the last 2, once Guenter confirmed the first 4 are merged. Thanks! Ray
On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote: > Consolidate two SP805 binding documents "arm,sp805.txt" and > "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the > desired compatible string to be used > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Reviewed-by: Rob Herring <robh@kernel.org> For the record: Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > .../devicetree/bindings/watchdog/arm,sp805.txt | 27 ++++++++++++++----- > .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ---------------------- > 2 files changed, 20 insertions(+), 38 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > diff --git a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt > index ca99d64..0fa3629 100644 > --- a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt > +++ b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt > @@ -1,17 +1,30 @@ > ARM AMBA Primecell SP805 Watchdog > > +SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that > +can be used to identify the peripheral type, vendor, and revision. > +This value can be used for driver matching. > + > +As SP805 WDT is a primecell IP, it follows the base bindings specified in > +'arm/primecell.txt' > + > Required properties: > -- compatible: Should be "arm,sp805" & "arm,primecell" > -- reg: Should contain location and length for watchdog timer register. > -- interrupts: Should contain the list of watchdog timer interrupts. > -- clocks: clocks driving the watchdog timer hardware. This list should be 2 > - clocks. With 2 clocks, the order is wdogclk clock, apb_pclk. > +- compatible: Should be "arm,sp805" & "arm,primecell" > +- reg: Should contain location and length for watchdog timer register > +- clocks: Clocks driving the watchdog timer hardware. This list should be > + 2 clocks. With 2 clocks, the order is wdog_clk, apb_pclk > + wdog_clk can be equal to or be a sub-multiple of the apb_pclk > + frequency > +- clock-names: Shall be "wdog_clk" for first clock and "apb_pclk" for the > + second one > + > +Optional properties: > +- interrupts: Should specify WDT interrupt number > > Example: > watchdog@66090000 { > compatible = "arm,sp805", "arm,primecell"; > reg = <0x66090000 0x1000>; > interrupts = <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>; > - clocks = <&apb_pclk>,<&apb_pclk>; > - clock-names = "wdogclk", "apb_pclk"; > + clocks = <&wdt_clk>, <&apb_pclk>; > + clock-names = "wdog_clk", "apb_pclk"; > }; > diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > deleted file mode 100644 > index edc4f0e..0000000 > --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > +++ /dev/null > @@ -1,31 +0,0 @@ > -* ARM SP805 Watchdog Timer (WDT) Controller > - > -SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that > -can be used to identify the peripheral type, vendor, and revision. > -This value can be used for driver matching. > - > -As SP805 WDT is a primecell IP, it follows the base bindings specified in > -'arm/primecell.txt' > - > -Required properties: > -- compatible : Should be "arm,sp805-wdt", "arm,primecell" > -- reg : Base address and size of the watchdog timer registers. > -- clocks : From common clock binding. > - First clock is PCLK and the second is WDOGCLK. > - WDOGCLK can be equal to or be a sub-multiple of the PCLK frequency. > -- clock-names : From common clock binding. > - Shall be "apb_pclk" for first clock and "wdog_clk" for the > - second one. > - > -Optional properties: > -- interrupts : Should specify WDT interrupt number. > - > -Examples: > - > - cluster1_core0_watchdog: wdt@c000000 { > - compatible = "arm,sp805-wdt", "arm,primecell"; > - reg = <0x0 0xc000000 0x0 0x1000>; > - clocks = <&clockgen 4 3>, <&clockgen 4 3>; > - clock-names = "apb_pclk", "wdog_clk"; > - }; > -
On 06/28/2018 04:50 PM, Ray Jui wrote: > Hi Guenter/Florian, > > On 6/27/2018 11:55 AM, Guenter Roeck wrote: >> On Wed, Jun 27, 2018 at 11:47:21AM -0700, Ray Jui wrote: >>> >>> >>> On 6/27/2018 11:42 AM, Guenter Roeck wrote: >>>> On Wed, Jun 27, 2018 at 11:38:48AM -0700, Ray Jui wrote: >>>>> >>>>> >>>>> On 6/27/2018 11:33 AM, Guenter Roeck wrote: >>>>>> On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote: >>>>>>> Hi Guenter/Rob, >>>>>>> >>>>>>> Kindly let me know how you want to proceed with this? >>>>>>> >>>>>> >>>>>> If I recall correctly, the patch series does not add a new problem >>>>>> but merely exposes one. Is my recollection correct ? If so, maybe >>>>>> we should just add a note somewhere indicating what might be wrong >>>>>> and otherwise apply the series. >>>>>> >>>>>> Does this make sense ? >>>>> >>>>> Yes this makes a lot of sense to me. This patch series exposes potential >>>>> problems in some SoCs that they might not be feeding the correct clock into >>>>> WDT, at least based on clock names from their DT entries. >>>>> >>>>> This patch series does not change/affect how SP805 works on those systems. >>>>> >>>>> Where should the note be added? >>>>> >>>> >>>> I would suggest to add a note into the driver where the clock is used, >>>> with the details discussed here. >>> >>> I assume you meant adding the notes to the SP805 driver where the clock is >>> used. >>> >>> If so, I think that makes sense. That notes deserves its own patch because >>> it really has nothing to do with any of the change in this patch series. >>> >>> Do you want me to 1) embed that patch into this patch series and send out >>> v5; or 2) leave the patch series as it is and send out a separate patch to >>> add the notes to the driver? >>> >> 2) is fine. I don't have the series here right now; if I recall correctly >> all patches in the series are all marked as Reviewed-by: and/or Acked-by:. >> If so, I'll apply them to my tree tonight, or at least the ones that will >> go in through the watchdog tree. >> >> Guenter >> > > Guenter, please let us know once you take the first 4 patches. > Patches are in my tree. Wim, it would be great if you can pick up the tree so it shows up in -next. Thanks, Guenter > Florian, could you please help with the last 2, once Guenter confirmed the first 4 are merged. > > Thanks! > > Ray > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt index ca99d64..0fa3629 100644 --- a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt +++ b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt @@ -1,17 +1,30 @@ ARM AMBA Primecell SP805 Watchdog +SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that +can be used to identify the peripheral type, vendor, and revision. +This value can be used for driver matching. + +As SP805 WDT is a primecell IP, it follows the base bindings specified in +'arm/primecell.txt' + Required properties: -- compatible: Should be "arm,sp805" & "arm,primecell" -- reg: Should contain location and length for watchdog timer register. -- interrupts: Should contain the list of watchdog timer interrupts. -- clocks: clocks driving the watchdog timer hardware. This list should be 2 - clocks. With 2 clocks, the order is wdogclk clock, apb_pclk. +- compatible: Should be "arm,sp805" & "arm,primecell" +- reg: Should contain location and length for watchdog timer register +- clocks: Clocks driving the watchdog timer hardware. This list should be + 2 clocks. With 2 clocks, the order is wdog_clk, apb_pclk + wdog_clk can be equal to or be a sub-multiple of the apb_pclk + frequency +- clock-names: Shall be "wdog_clk" for first clock and "apb_pclk" for the + second one + +Optional properties: +- interrupts: Should specify WDT interrupt number Example: watchdog@66090000 { compatible = "arm,sp805", "arm,primecell"; reg = <0x66090000 0x1000>; interrupts = <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>; - clocks = <&apb_pclk>,<&apb_pclk>; - clock-names = "wdogclk", "apb_pclk"; + clocks = <&wdt_clk>, <&apb_pclk>; + clock-names = "wdog_clk", "apb_pclk"; }; diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt deleted file mode 100644 index edc4f0e..0000000 --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt +++ /dev/null @@ -1,31 +0,0 @@ -* ARM SP805 Watchdog Timer (WDT) Controller - -SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that -can be used to identify the peripheral type, vendor, and revision. -This value can be used for driver matching. - -As SP805 WDT is a primecell IP, it follows the base bindings specified in -'arm/primecell.txt' - -Required properties: -- compatible : Should be "arm,sp805-wdt", "arm,primecell" -- reg : Base address and size of the watchdog timer registers. -- clocks : From common clock binding. - First clock is PCLK and the second is WDOGCLK. - WDOGCLK can be equal to or be a sub-multiple of the PCLK frequency. -- clock-names : From common clock binding. - Shall be "apb_pclk" for first clock and "wdog_clk" for the - second one. - -Optional properties: -- interrupts : Should specify WDT interrupt number. - -Examples: - - cluster1_core0_watchdog: wdt@c000000 { - compatible = "arm,sp805-wdt", "arm,primecell"; - reg = <0x0 0xc000000 0x0 0x1000>; - clocks = <&clockgen 4 3>, <&clockgen 4 3>; - clock-names = "apb_pclk", "wdog_clk"; - }; -
Consolidate two SP805 binding documents "arm,sp805.txt" and "sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the desired compatible string to be used Signed-off-by: Ray Jui <ray.jui@broadcom.com> --- .../devicetree/bindings/watchdog/arm,sp805.txt | 27 ++++++++++++++----- .../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ---------------------- 2 files changed, 20 insertions(+), 38 deletions(-) delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt