by Chris Packham

[permalink] [raw]

Subject: Re: [PATCH v9 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

On 15/06/22 12:03, Chris Packham wrote:
> Hi All,
>
>
> On 15/06/22 09:25, Chris Packham wrote:
>> +cc: Elad
>>
>> On 14/06/22 20:16, Vadym Kochan wrote:
>>> Hi Chris,
>>>
>>> On Tue, Jun 14, 2022 at 05:26:39AM +0000, Chris Packham wrote:
>>>> On 14/06/22 17:11, Chris Packham wrote:
>>>>> On 14/06/22 10:53, Vadym Kochan wrote:
>>>>>> From: Chris Packham <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Signed-off-by: Vadym Kochan <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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 v9 (proposed by Marvell):
>>>>>>         It was discussed with Chris that Marvell will add some
>>>>>> changes:
>>>>>>
>>>>>>            1) Rename "armada-" prefix in dts(i) file names to ac5,
>>>>>> because
>>>>>>               Armada has not much common with AC5 SoC.
>>>>>>
>>>>>>            2) Add clock fixes:
>>>>>>               - rename core_clock to cnm_clock
>>>>>>               - remove axi_clock
>>>>>>               - change cnm_clock to 325MHZ
>>>>>>               - use cnm_clock for the UART
>>>>>>
>>>>>>       Changes in v8:
>>>>>>       - Remove unnecessary clock-frequency property on armv8-timer
>>>>>>       - Remove unnecessary redistributor-stride property on gic
>>>>>>       - Add GIC_SPI interrupts for gpios
>>>>>>       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 +
>>>>>>    arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi | 291
>>>>>> ++++++++++++++++++
>>>>>>    .../boot/dts/marvell/ac5-98dx35xx-rd.dts      | 101 ++++++
>>>>>>    arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi | 13 +
>>>>>>    4 files changed, 406 insertions(+)
>>>>>>    create mode 100644 arch/arm64/boot/dts/marvell/ac5-98dx25xx.dtsi
>>>>>>    create mode 100644
>>>>>> arch/arm64/boot/dts/marvell/ac5-98dx35xx-rd.dts
>>>>>>    create mode 100644 arch/arm64/boot/dts/marvell/ac5-98dx35xx.dtsi
>>>>>>
>>> [snip]
>>>
>>>>>> +
>>>>>> +            uart0: serial@12000 {
>>>>>> +                compatible = "snps,dw-apb-uart";
>>>>>> +                reg = <0x12000 0x100>;
>>>>>> +                reg-shift = <2>;
>>>>>> +                interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
>>>>>> +                reg-io-width = <1>;
>>>>>> +                clocks = <&cnm_clock>;
>>>>> With this change I see some garbled output when the "Serial: AMBA
>>>>> PL011 UART" driver starts.
>>>>>
>>>>> It could be that my bootloader has the same wrong clock value as the
>>>>> earlier iteration of this device tree.
>>>> Fixing u-boot doesn't help but there are also references to
>>>> 328000000 in
>>>> mv-ddr and ATF so I'll look to see if updating them fixes the issue
>>>> tomorrow.
>>
>> Even with changing ATF and mv_ddr to use 325000000 I still end up
>> with the garbled output when the driver starts. I don't know if
>> there's something special about the fact I have a RD-AC5X-SR2 board.
>> As far as I know the only difference with the SR2 board was an
>> increased DDR size.
>>
> Actually you might be off the hook. I've applied your patches on top
> of v5.18 (which is what I was using last time I worked on the series)
> and I don't see the garbled output. So I think the problem is actually
> new between v5.18 and v5.19-rc2.

Just nudging this thread. Things seem to have been fixed with commit
07a22b61946f ("Revert "printk: add functions to prefer direct
printing"") so testing wise things are all good.

Tested-by: Chris Packham <[email protected]>

>>> Interesting, because Marvell suggested to use cnm_clock with 328MHz
>>> for AC5, and 325MHz
>>> for AC5X.
>>
>> Did you get that the right way round? The ac5-98dx25xx.dtsi is AC5 so
>> if what you have written is correct shouldn't the cnm clock-frequency
>> in ac5-98dx25xx.dtsi be 328MHz and the ac5-98dx35xx.dtsi (which is
>> AC5X) override this to 325MHz?
>>
>> Elad, can you shine any light on this?
>>
>>> [snip]
>>>
>>>>>> +
>>>>>> +    clocks {
>>>>>> +        cnm_clock: core-clock {
>>>>>> +            compatible = "fixed-clock";
>>>>>> +            #clock-cells = <0>;
>>>>>> +            clock-frequency = <325000000>;
>>>>>> +        };
>>>>>> +
>>>>>> +        spi_clock: spi-clock {
>>>>>> +            compatible = "fixed-clock";
>>>>>> +            #clock-cells = <0>;
>>>>>> +            clock-frequency = <200000000>;
>>>>>> +        };
>>>>>> +    };
>>>>>> +};
>>> [snip]
>>>
>>> Regards,