Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fb4def96-4b20-604f-d15d-fef87eb2232d@canonical.com>
Date:   Thu, 17 Mar 2022 09:36:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device
 tree
Content-Language: en-US
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>,
        "Verdun, Jean-Marie" <verdun@hpe.com>
Cc:     Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
        "soc@kernel.org" <soc@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20220310195229.109477-1-nick.hawkins@hpe.com>
 <20220310195229.109477-9-nick.hawkins@hpe.com>
 <eb66cc83-2da9-8e19-db69-633e34fef946@canonical.com>
 <PH0PR84MB1718C31DB71AA2A67FEC5F6E88119@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
 <b1cc2566-cd78-7cb4-f8a5-d6fc8065fe6e@canonical.com>
 <PH0PR84MB1718292B1C11F4FE83326A5388119@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <PH0PR84MB1718292B1C11F4FE83326A5388119@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@cefe0000. 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.

(...)

>>>>>>> +
>>>>>>> +		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.


Best regards,
Krzysztof