Message-ID: <PH0PR84MB1718A2CFBFC90C9C0CAA5515881E9@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Tue, 29 Mar 2022 19:38:27 +0000
From:   "Hawkins, Nick" <nick.hawkins@....com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        "Verdun, Jean-Marie" <verdun@....com>
CC:     Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>,
        "soc@...nel.org" <soc@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device
 tree

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@...onical.com] 
Sent: Thursday, March 17, 2022 3:37 AM
To: Hawkins, Nick <nick.hawkins@....com>; Verdun, Jean-Marie <verdun@....com>
Cc: Arnd Bergmann <arnd@...db.de>; Olof Johansson <olof@...om.net>; soc@...nel.org; Rob Herring <robh+dt@...nel.org>; linux-arm-kernel@...ts.infradead.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On 16/03/2022 21:10, Hawkins, Nick wrote:

(...)

>>>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>>>> only, right?
>>>
>>>> Yes there was, I removed the usb0: ehci@...e0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
>> 
>>> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
>> 
>> For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?

> My bad, I misread the generic-ehci binding, so your compatible is actually correct from bindings point of view. Still common practice is to add own compatible which allows later customization.

>> In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?

> Yes, add your compatible to that big enum with list of many implementations.

Done.

> (...)

>>>>>>>> +
>>>>>>>> +		memclk: memclk {
>>>>>>>> +			compatible = "fixed-clock";
>>>>>>>> +			#clock-cells = <0>>>>;
>>>>>>>> +			clock-output-names = "memclk";
>>>>>>>> +			clock-frequency = <800000000>>>>;
>>>>>>>> +		};
>>>
>>>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>>
>>>> This was the internal iopclk and memclk they were both internal to the chip.
>>>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
>> 
>>> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
>> 
>> I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?

> It all depends on the architecture of your SoC. I could imagine such one:
> 1. Few external (from SoC point of view) oscillators, usually provided on the board. It could be 24 MHz, could be 32767 Hz for Bluetooth etc.

> 2. One or several clock controllers inside the SoC which take as input these external clocks. The clock controller provides clocks for several other components/blocks. Allows also gating clocks, reparenting and changing dividers (rate).

> 3. Other blocks within your SoC, e.g. gxp-timer, use these clocks.

> The true question is where is that memclk or iopclk generated? Is it controllable (gate/mux/rate)? Even some fixed-frequency clocks, coming out of that clock controller (point 2.), are defined in the clock controller because that's the logical place for >them.

From the perspective of our SoC there is a PPUCLK that generates the clk signals for our watchdog and timer. This happens inside the SoC. I am trying to represent this below.

axi {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;
		dma-ranges;

		ahb@...00000 {
			compatible = "simple-bus";
			#address-cells = <1>;
			#size-cells = <1>;
			ranges = <0x0 0xc0000000 0x30000000>;

			...

			timer0: timer@80 {
				compatible = "hpe,gxp-timer";
				reg = <0x80 0x1>, <0x94 0x01>, <0x88 0x08>;
				interrupts = <0>;
				interrupt-parent = <&vic0>;
				clocks = <&ppuclk>;
				clock-names = "ppuclk";
				clock-frequency = <400000000>;
			};

			watchdog0: watchdog@90 {
				compatible = "hpe,gxp-wdt";
				reg = <0x90 0x02>, <0x96 0x01>;
			};

			...
	};

	clocks {
		ppuclk: ppuclk {
			compatible = "fixed-clock";
			#clock-cells = <0>;
			clock-output-names = "ppuclk";
			clock-frequency = <400000000>;
		};
	};

I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.

For instance with our watchdog controls we have:

@90 the countdown value
@96 the configuration

And for our timer we have:
@80 the countdown value
@94 the configuration
@88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.

What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers. 

 Thanks,

-Nick Hawkins

Powered by blists - more mailing lists