On 16/05/22 21:48, Marc Zyngier wrote:
> On Fri, 13 May 2022 02:26:21 +0100,
> Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>>>> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>>>>> ---
>>>>>
>>>>> 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.
>
