| 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: <b1cc2566-cd78-7cb4-f8a5-d6fc8065fe6e@canonical.com>
Date: Wed, 16 Mar 2022 16:50:52 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
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" <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
On 16/03/2022 16:41, Hawkins, Nick wrote:
> -----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.
>
>>> + 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.
>
>>> + 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.
Best regards,
Krzysztof
Powered by blists - more mailing lists