From: Ray Jui <ray.jui@broadcom.com> To: Rob Herring <robh@kernel.org>, Guenter Roeck <linux@roeck-us.net> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>, Mark Rutland <mark.rutland@arm.com>, Frank Rowand <frowand.list@gmail.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Robin Murphy <robin.murphy@arm.com>, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com> Subject: Re: [PATCH v4 1/6] Documentation: DT: Consolidate SP805 binding docs Date: Wed, 6 Jun 2018 16:39:04 -0700 [thread overview] Message-ID: <09c870cd-0a44-6634-58d8-f57f9fcd0cb5@broadcom.com> (raw) In-Reply-To: <CABGGisxEvWDy16SKE2RG-vhUHBZHSCCHyfwJsPY16B1fQS-=cA@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: ray.jui@broadcom.com (Ray Jui) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v4 1/6] Documentation: DT: Consolidate SP805 binding docs Date: Wed, 6 Jun 2018 16:39:04 -0700 [thread overview] Message-ID: <09c870cd-0a44-6634-58d8-f57f9fcd0cb5@broadcom.com> (raw) In-Reply-To: <CABGGisxEvWDy16SKE2RG-vhUHBZHSCCHyfwJsPY16B1fQS-=cA@mail.gmail.com> 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
next prev parent reply other threads:[~2018-06-06 23:39 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-28 18:01 [PATCH v4 0/6] Enhance support for the SP805 WDT Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-05-28 18:01 ` [PATCH v4 1/6] Documentation: DT: Consolidate SP805 binding docs Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-06-05 19:41 ` Rob Herring 2018-06-05 19:41 ` Rob Herring 2018-06-06 16:19 ` Guenter Roeck 2018-06-06 16:19 ` Guenter Roeck 2018-06-06 16:33 ` Rob Herring 2018-06-06 16:33 ` Rob Herring 2018-06-06 23:39 ` Ray Jui [this message] 2018-06-06 23:39 ` Ray Jui 2018-06-20 17:39 ` Ray Jui 2018-06-20 17:39 ` Ray Jui 2018-06-27 18:33 ` Guenter Roeck 2018-06-27 18:33 ` Guenter Roeck 2018-06-27 18:38 ` Ray Jui 2018-06-27 18:38 ` Ray Jui 2018-06-27 18:42 ` Guenter Roeck 2018-06-27 18:42 ` Guenter Roeck 2018-06-27 18:47 ` Ray Jui 2018-06-27 18:47 ` Ray Jui 2018-06-27 18:55 ` Guenter Roeck 2018-06-27 18:55 ` Guenter Roeck 2018-06-28 23:50 ` Ray Jui 2018-06-28 23:50 ` Ray Jui 2018-06-29 3:00 ` Guenter Roeck 2018-06-29 3:00 ` Guenter Roeck 2018-06-29 2:55 ` [v4,1/6] " Guenter Roeck 2018-06-29 2:55 ` Guenter Roeck 2018-05-28 18:01 ` [PATCH v4 2/6] Documentation: DT: Add optional 'timeout-sec' property for sp805 Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-06-05 19:55 ` Rob Herring 2018-06-05 19:55 ` Rob Herring 2018-06-29 2:55 ` [v4, " Guenter Roeck 2018-06-29 2:55 ` Guenter Roeck 2018-05-28 18:01 ` [PATCH v4 3/6] watchdog: sp805: add 'timeout-sec' DT property support Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-05-28 18:01 ` [PATCH v4 4/6] watchdog: sp805: set WDOG_HW_RUNNING when appropriate Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-05-28 18:01 ` [PATCH v4 5/6] arm64: dt: set initial SR watchdog timeout to 60 seconds Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-07-09 17:40 ` Florian Fainelli 2018-07-09 17:40 ` Florian Fainelli 2018-05-28 18:01 ` [PATCH v4 6/6] arm64: defconfig: add CONFIG_ARM_SP805_WATCHDOG Ray Jui 2018-05-28 18:01 ` Ray Jui 2018-07-09 17:40 ` Florian Fainelli 2018-07-09 17:40 ` Florian Fainelli 2018-06-06 19:29 ` [PATCH v4 0/6] Enhance support for the SP805 WDT Florian Fainelli 2018-06-06 19:29 ` Florian Fainelli 2018-06-06 23:40 ` Ray Jui 2018-06-06 23:40 ` Ray Jui
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=09c870cd-0a44-6634-58d8-f57f9fcd0cb5@broadcom.com \ --to=ray.jui@broadcom.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=catalin.marinas@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=frowand.list@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-watchdog@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=mark.rutland@arm.com \ --cc=robh@kernel.org \ --cc=robin.murphy@arm.com \ --cc=will.deacon@arm.com \ --cc=wim@linux-watchdog.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.