lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v7 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board
Date

On 16/05/22 21:48, Marc Zyngier wrote:
> On Fri, 13 May 2022 02:26:21 +0100,
> Chris Packham <Chris.Packham@alliedtelesis.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@alliedtelesis.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@alliedtelesis.co.nz>
>>>>> Reviewed-by: Andrew Lunn <andrew@lunn.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.
>
\
 
 \ /
  Last update: 2022-05-16 23:58    [W:0.167 / U:0.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site