Message-ID: <db5c3366-ac81-261b-ff32-3ccf94a930f6@alliedtelesis.co.nz>
Date:   Mon, 16 May 2022 21:56:44 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Marc Zyngier <maz@...nel.org>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "sebastian.hesselbarth@...il.com" <sebastian.hesselbarth@...il.com>,
        "kostap@...vell.com" <kostap@...vell.com>,
        "robert.marko@...tura.hr" <robert.marko@...tura.hr>,
        "vadym.kochan@...ision.eu" <vadym.kochan@...ision.eu>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v7 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and
 RD-AC5X board


On 16/05/22 21:48, Marc Zyngier wrote:
> On Fri, 13 May 2022 02:26:21 +0100,
> Chris Packham <Chris.Packham@...iedtelesis.co.nz> wrote:
>> Hi Marc,
>>
>> On 13/05/22 10:10, Chris Packham wrote:
>>> Hi Marc,
>>>
>>> On 12/05/22 19:38, Marc Zyngier wrote:
>>>> On Thu, 12 May 2022 05:25:00 +0100,
>>>> Chris Packham <chris.packham@...iedtelesis.co.nz> wrote:
>>>>> The 98DX2530 SoC is the Control and Management CPU integrated into
>>>>> the Marvell 98DX25xx and 98DX35xx series of switch chip (internally
>>>>> referred to as AlleyCat5 and AlleyCat5X).
>>>>>
>>>>> These files have been taken from the Marvell SDK and lightly cleaned
>>>>> up with the License and copyright retained.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>>>>> Reviewed-by: Andrew Lunn <andrew@...n.ch>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>       The Marvell SDK has a number of new compatible strings. I've
>>>>> brought
>>>>>       through some of the drivers or where possible used an in-tree
>>>>>       alternative (e.g. there is SDK code for a ac5-gpio but two
>>>>> instances of
>>>>>       the existing marvell,orion-gpio seems to cover what is needed
>>>>> if you use
>>>>>       an appropriate binding). I expect that there will a new series of
>>>>>       patches when I get some different hardware (or additions to
>>>>> this series
>>>>>       depending on if/when it lands).
>>>>>            Changes in v7:
>>>>>       - Add missing compatible on usb1
>>>>>       - Add "rd-ac5x" compatible for board
>>>>>       - Move aliases to board dts
>>>>>       - Move board specific usb info to board dts
>>>>>       - Consolidate usb1 board settings and remove unnecessary
>>>>> compatible
>>>>>       - Add Allied Telesis copyright
>>>>>       - Rename files after mailng-list discussion
>>>>>       Changes in v6:
>>>>>       - Move CPU nodes above the SoC (Krzysztof)
>>>>>       - Minor formatting clean ups (Krzysztof)
>>>>>       - Run through `make dtbs_check`
>>>>>       - Move gic nodes inside SoC
>>>>>       - Group clocks under a clock node
>>>>>       Changes in v5:
>>>>>       - add #{address,size}-cells property to i2c nodes
>>>>>       - make i2c nodes disabled in the SoC and enable them in the board
>>>>>       - add interrupt controller attributes to gpio nodes
>>>>>       - Move fixed-clock nodes up a level and remove unnecessary @0
>>>>>       Changes in v4:
>>>>>       - use 'phy-handle' instead of 'phy'
>>>>>       - move status="okay" on usb nodes to board dts
>>>>>       - Add review from Andrew
>>>>>       Changes in v3:
>>>>>       - Move memory node to board
>>>>>       - Use single digit reg value for phy address
>>>>>       - Remove MMC node (driver needs work)
>>>>>       - Remove syscon & simple-mfd for pinctrl
>>>>>       Changes in v2:
>>>>>       - Make pinctrl a child node of a syscon node
>>>>>       - Use marvell,armada-8k-gpio instead of orion-gpio
>>>>>       - Remove nand peripheral. The Marvell SDK does have some
>>>>> changes for the
>>>>>         ac5-nand-controller but I currently lack hardware with NAND
>>>>> fitted so
>>>>>         I can't test it right now. I've therefore chosen to omit the
>>>>> node and
>>>>>         not attempted to bring in the driver or binding.
>>>>>       - Remove pcie peripheral. Again there are changes in the SDK
>>>>> and I have
>>>>>         no way of testing them.
>>>>>       - Remove prestera node.
>>>>>       - Remove "marvell,ac5-ehci" compatible from USB node as
>>>>>         "marvell,orion-ehci" is sufficient
>>>>>       - Remove watchdog node. There is a buggy driver for the ac5
>>>>> watchdog in
>>>>>         the SDK but it needs some work so I've dropped the node for now.
>>>>>
>>>>>    arch/arm64/boot/dts/marvell/Makefile          |   1 +
>>>>>    .../boot/dts/marvell/armada-98dx25xx.dtsi     | 295
>>>>> ++++++++++++++++++
>>>>>    .../boot/dts/marvell/armada-98dx35xx-rd.dts   | 101 ++++++
>>>>>    .../boot/dts/marvell/armada-98dx35xx.dtsi     |  13 +
>>>>>    4 files changed, 410 insertions(+)
>>>>>    create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
>>>>>    create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx35xx-rd.dts
>>>>>    create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx35xx.dtsi
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/marvell/Makefile
>>>>> b/arch/arm64/boot/dts/marvell/Makefile
>>>>> index 1c794cdcb8e6..b7a4c715afbb 100644
>>>>> --- a/arch/arm64/boot/dts/marvell/Makefile
>>>>> +++ b/arch/arm64/boot/dts/marvell/Makefile
>>>>> @@ -24,3 +24,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db.dtb
>>>>>    dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
>>>>>    dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>>>>>    dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
>>>>> +dtb-$(CONFIG_ARCH_MVEBU) += armada-98dx35xx-rd.dtb
>>>>> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
>>>>> b/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
>>>>> new file mode 100644
>>>>> index 000000000000..55ab4cd843a9
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
>>>>> @@ -0,0 +1,295 @@
>>>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>>>> +/*
>>>>> + * Device Tree For AC5.
>>>>> + *
>>>>> + * Copyright (C) 2021 Marvell
>>>>> + * Copyright (C) 2022 Allied Telesis Labs
>>>>> + */
>>>>> +
>>>>> +#include <dt-bindings/gpio/gpio.h>
>>>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> +
>>>>> +/ {
>>>>> +    model = "Marvell AC5 SoC";
>>>>> +    compatible = "marvell,ac5";
>>>>> +    interrupt-parent = <&gic>;
>>>>> +    #address-cells = <2>;
>>>>> +    #size-cells = <2>;
>>>>> +
>>>>> +    cpus {
>>>>> +        #address-cells = <2>;
>>>>> +        #size-cells = <0>;
>>>>> +
>>>>> +        cpu-map {
>>>>> +            cluster0 {
>>>>> +                core0 {
>>>>> +                    cpu = <&cpu0>;
>>>>> +                };
>>>>> +                core1 {
>>>>> +                    cpu = <&cpu1>;
>>>>> +                };
>>>>> +            };
>>>>> +        };
>>>>> +
>>>>> +        cpu0: cpu@0 {
>>>>> +            device_type = "cpu";
>>>>> +            compatible = "arm,armv8";
>>>>> +            reg = <0x0 0x0>;
>>>>> +            enable-method = "psci";
>>>>> +            next-level-cache = <&l2>;
>>>>> +        };
>>>>> +
>>>>> +        cpu1: cpu@1 {
>>>>> +            device_type = "cpu";
>>>>> +            compatible = "arm,armv8";
>>>>> +            reg = <0x0 0x100>;
>>>>> +            enable-method = "psci";
>>>>> +            next-level-cache = <&l2>;
>>>>> +        };
>>>>> +
>>>>> +        l2: l2-cache {
>>>>> +            compatible = "cache";
>>>>> +        };
>>>>> +    };
>>>>> +
>>>>> +
>>>>> +    psci {
>>>>> +        compatible = "arm,psci-0.2";
>>>>> +        method = "smc";
>>>>> +    };
>>>>> +
>>>>> +    timer {
>>>>> +        compatible = "arm,armv8-timer";
>>>>> +        interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>,
>>>>> +                 <GIC_PPI 8 IRQ_TYPE_LEVEL_HIGH>,
>>>>> +                 <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>,
>>>>> +                 <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +        clock-frequency = <25000000>;
>>>> I said no to this hack in a past version of this patch, and I'm going
>>>> to say it *again*.
>>> Sorry I must have missed it.
>>>> Please fix your firmware to program CNTFRQ_EL0, and
>>>> remove this useless property.
>>> I'm kind of at the mercy of what Marvell have provided for ATF. I am
>>> working on the bootloader portion in parallel and am getting things
>>> ready for submitting the u-boot support upstream. I was hoping to
>>> leave ATF alone I can at least see if they haven't fixed this already
>>> (the original dtsi I started with was fairly old) and if they haven't
>>> I'll raise it via their support system.
>> Seems to work fine without the clock so I'll drop it.
> Thanks. If you can, please verify that this is set on both CPUs (I
> have seen plenty of firmware only setting it on CPU0 in the past).
The arch_timer interrupts are counting up on both CPUs and things 
generally seem to be getting scheduled (I don't have much of a userland 
at the moment so it's not exactly a stress test). Do you think that is 
sufficient to say the clock property is unnecessary and whatever 
firmware I have is working as expected.
>>>> You are also missing a PPI for the EL2 virtual timer which is present
>>>> on any ARMv8.1+ CPU (and since this system is using A55, it definitely
>>>> has it).
>>>>
>>>> [...]
>>> Will add.
>> I assume you're talking about the 5th PPI per the
>> timer/arm,arch_timer.yaml ("hypervisor virtual timer irq").
> Indeed.
>
>> Helpfully
>> Marvell don't include the PPI interrupt numbers in their datasheet. But
>> then I also notice that none of the other boards that have a
>> "arm,armv8-timer" provide a 5th interrupt either, have I misunderstood
>> something?
> This was only recently added to the DT binding, but the interrupt
> definitely exist at the CPU level for anything that implements ARMv8.1
> and up. AFAIK, the M1 is the only machine to expose this interrupt in
> DT, but this doesn't mean the interrupt doesn't exist on all the other
> systems that have the same architecture revision.
>
> If you have contacts in Marvell, maybe try and find out whether they
> have simply decided not to wire the interrupt (I wouldn't be
> surprised). In this case, please add a comment.

I've reached out via their customer support portal. So far they just 
want to know why I'm refusing to use their out of date SDK (maybe I 
should direct them at some of Jon Corbet's presentations :P).

These integrated chips are sometimes a bit problematic because the 
support goes via the Switching group but these questions are really 
about IP blocks that have been taken from the SoC group. It may take a 
while before I get a response from someone that actually knows the 
internals.

>
> Thanks,
>
> 	M.
>

Powered by blists - more mailing lists