On 6/27/2018 11:33 AM, Guenter Roeck wrote:

On Wed, Jun 20, 2018 at 10:39:16AM -0700, Ray Jui wrote:

quoted

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

quoted

Thanks,

Ray

On 6/6/2018 4:39 PM, Ray Jui wrote:

quoted

On 6/6/2018 9:33 AM, Rob Herring wrote:

quoted

On Wed, Jun 6, 2018 at 11:19 AM, Guenter Roeck [off-list ref]
wrote:

quoted

On 06/05/2018 12:41 PM, Rob Herring wrote:

quoted

On Mon, May 28, 2018 at 11:01:32AM -0700, Ray Jui wrote:

quoted

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 <redacted>
---
 ?? .../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.

quoted

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

quoted

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.

quoted

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html