| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <PH0PR84MB1718292B1C11F4FE83326A5388119@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
Date: Wed, 16 Mar 2022 20:10:54 +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: Friday, March 11, 2022 4:30 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 10/03/2022 20:52, nick.hawkins@....com wrote:
>>>>>> From: Nick Hawkins <nick.hawkins@....com>>>>
>>>>>>
>>>>>> The HPE SoC is new to linux. This patch creates the basic device
>>>>>> tree layout with minimum required for linux to boot. This includes
>>>>>> timer and watchdog support.
>>>>>>
>>>>>> Signed-off-by: Nick Hawkins <nick.hawkins@....com>>>>
>>>>>> ---
>>>>>> arch/arm/boot/dts/Makefile | 2 +
>>>>>> arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
>>>>>> arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
>>>>>> 3 files changed, 177 insertions(+)
>>>>>> create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>>>>> index e41eca79c950..2823b359d373 100644
>>>>>> --- a/arch/arm/boot/dts/Makefile
>>>>>> +++ b/arch/arm/boot/dts/Makefile
>>>>>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>>>>> aspeed-bmc-vegman-n110.dtb \
>>>>>> aspeed-bmc-vegman-rx20.dtb \
>>>>>> aspeed-bmc-vegman-sx20.dtb
>>>>>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>>>>>> + hpe-bmc-dl360gen10.dtb
>>
>>>> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
>>
>> Done
>>
>>>>>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> new file mode 100644
>>>>>> index 000000000000..da5eac1213a8
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Device Tree file for HPE DL360Gen10 */
>>>>>> +
>>>>>> +/include/ "hpe-gxp.dtsi"
>>>>>> +
>>>>>> +/ {
>>>>>> + #address-cells = <1>>>>;
>>>>>> + #size-cells = <1>>>>;
>>>>>> + compatible = "hpe,gxp";
>>
>>>> Missing board compatible.
>>
>> Will become compatible = "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.
> Yes, except hpe,gxp goes at the end.
Done
>>
>>>>>> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>>>>> +
(...)
>>>>>> +
>>>>>> + usb0: usb@...e0000 {
>>>>>> + compatible = "generic-ehci";
>>
>>>> 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?
In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?
>>
>>>>>> + reg = <0xcefe0000 0x100>>>>;
>>>>>> + interrupts = <7>>>>;
>>>>>> + interrupt-parent = <&vic0>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + usb1: usb@...e0100 {
>>>>>> + compatible = "generic-ohci";
>>>>>> + reg = <0xcefe0100 0x110>>>>;
>>>>>> + interrupts = <6>>>>;
>>>>>> + interrupt-parent = <&vic0>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + vrom@...00000 {
>>>>>> + compatible = "mtd-ram";
>>>>>> + bank-width = <4>>>>;
>>>>>> + reg = <0x58000000 0x4000000>>>>;
>>>>>> + #address-cells = <1>>>>;
>>>>>> + #size-cells = <1>>>>;
>>>>>> + partition@0 {
>>>>>> + label = "vrom-prime";
>>>>>> + reg = <0x0 0x2000000>>>>;
>>>>>> + };
>>>>>> + partition@...0000 {
>>>>>> + label = "vrom-second";
>>>>>> + reg = <0x2000000 0x2000000>>>>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + i2cg: syscon@...000f8 {
>>
>>
>>>>>> + compatible = "simple-mfd", "syscon";
>>>>>> + reg = <0xc00000f8 0x08>>>>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + clocks {
>>>>>> + osc: osc {
>>
>>>> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
>>
>> We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
>>
>>>>>> + compatible = "fixed-clock";
>>>>>> + #clock-cells = <0>>>>;
>>>>>> + clock-output-names = "osc";
>>>>>> + clock-frequency = <33333333>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + iopclk: iopclk {
>>>>>> + compatible = "fixed-clock";
>>>>>> + #clock-cells = <0>>>>;
>>>>>> + clock-output-names = "iopclk";
>>>>>> + clock-frequency = <400000000>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + 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?
Thanks!
-Nick Hawkins
Powered by blists - more mailing lists