Re: [PATCH v4 8/8] riscv: dts: microchip: add the sundance polarberry

[Conor.Dooley]

On 09/05/2022 14:07, Heiko Stübner wrote:
> Am Montag, 9. Mai 2022, 13:24:12 CEST schrieb Conor.Dooley@xxxxxxxxxxxxx:
>> On 09/05/2022 12:10, Heiko Stübner wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Am Mittwoch, 4. Mai 2022, 22:30:52 CEST schrieb Conor Dooley:
>>>> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>>
>>>> Add a minimal device tree for the PolarFire SoC based Sundance
>>>> PolarBerry.
>>>>
>>>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>> ---
>>>>    arch/riscv/boot/dts/microchip/Makefile        |  1 +
>>>>    .../dts/microchip/mpfs-polarberry-fabric.dtsi | 16 +++
>>>>    .../boot/dts/microchip/mpfs-polarberry.dts    | 97 +++++++++++++++++++
>>>>    3 files changed, 114 insertions(+)
>>>>    create mode 100644 arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
>>>>    create mode 100644 arch/riscv/boot/dts/microchip/mpfs-polarberry.dts
>>>>
>>>> diff --git a/arch/riscv/boot/dts/microchip/Makefile b/arch/riscv/boot/dts/microchip/Makefile
>>>> index af3a5059b350..39aae7b04f1c 100644
>>>> --- a/arch/riscv/boot/dts/microchip/Makefile
>>>> +++ b/arch/riscv/boot/dts/microchip/Makefile
>>>> @@ -1,3 +1,4 @@
>>>>    # SPDX-License-Identifier: GPL-2.0
>>>>    dtb-$(CONFIG_SOC_MICROCHIP_POLARFIRE) += mpfs-icicle-kit.dtb
>>>> +dtb-$(CONFIG_SOC_MICROCHIP_POLARFIRE) += mpfs-polarberry.dtb
>>>>    obj-$(CONFIG_BUILTIN_DTB) += $(addsuffix .o, $(dtb-y))
>>>> diff --git a/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
>>>> new file mode 100644
>>>> index 000000000000..49380c428ec9
>>>> --- /dev/null
>>>> +++ b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi
>>>> @@ -0,0 +1,16 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>>> +/* Copyright (c) 2020-2022 Microchip Technology Inc */
>>>> +
>>>> +/ {
>>>> +     fabric_clk3: fabric-clk3 {
>>>> +             compatible = "fixed-clock";
>>>> +             #clock-cells = <0>;
>>>> +             clock-frequency = <62500000>;
>>>> +     };
>>>> +
>>>> +     fabric_clk1: fabric-clk1 {
>>>> +             compatible = "fixed-clock";
>>>> +             #clock-cells = <0>;
>>>> +             clock-frequency = <125000000>;
>>>> +     };
>>>> +};
>>>> diff --git a/arch/riscv/boot/dts/microchip/mpfs-polarberry.dts b/arch/riscv/boot/dts/microchip/mpfs-polarberry.dts
>>>> new file mode 100644
>>>> index 000000000000..1cad5b0d42e1
>>>> --- /dev/null
>>>> +++ b/arch/riscv/boot/dts/microchip/mpfs-polarberry.dts
>>>> @@ -0,0 +1,97 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>>> +/* Copyright (c) 2020-2022 Microchip Technology Inc */
>>>> +
>>>> +/dts-v1/;
>>>> +
>>>> +#include "mpfs.dtsi"
>>>> +#include "mpfs-polarberry-fabric.dtsi"
>>>> +
>>>> +/* Clock frequency (in Hz) of the rtcclk */
>>>> +#define MTIMER_FREQ  1000000
>>>> +
>>>> +/ {
>>>> +     model = "Sundance PolarBerry";
>>>> +     compatible = "sundance,polarberry", "microchip,mpfs";
>>>> +
>>>> +     aliases {
>>>> +             ethernet0 = &mac1;
>>>> +             serial0 = &mmuart0;
>>>> +     };
>>>> +
>>>> +     chosen {
>>>> +             stdout-path = "serial0:115200n8";
>>>> +     };
>>>> +
>>>> +     cpus {
>>>> +             timebase-frequency = <MTIMER_FREQ>;
>>>> +     };
>>>> +
>>>> +     ddrc_cache_lo: memory@80000000 {
>>>> +             device_type = "memory";
>>>> +             reg = <0x0 0x80000000 0x0 0x2e000000>;
>>>> +     };
>>>> +
>>>> +     ddrc_cache_hi: memory@1000000000 {
>>>> +             device_type = "memory";
>>>> +             reg = <0x10 0x00000000 0x0 0xC0000000>;
>>>> +     };
>>>> +};
>>>> +
>>>> +/*
>>>> + * phy0 is connected to mac0, but the port itself is on the (optional) carrier
>>>> + * board.
>>>> + */
>>>> +&mac0 {
>>>> +     status = "disabled";
>>>> +     phy-mode = "sgmii";
>>>> +     phy-handle = <&phy0>;
>>>
>>> nit: it makes it was easier recognizing the status if it's in the
>>> same place all the time (for example as the last property)
>>> like in &mmc below.
>>>
>>> Though that may just be my preference ;-) .
>>> The other option would be to adhere to stricter sorting
>>> because right now status is neither in one place nor sorted.
>>
>> My I had it in my head (and correct me if I am wrong please), that it is
>> okay to sort the phys after the status. It doesn't matter either way to
>> me, but there are plenty of dts that do it this way.
>>
>> I don't care either way, so I am happy to change if those are bad examples
>> to follow!
> 
> I guess which order to follow really is more a matter of taste and I
> don't think there is a definitive rulebook on what belongs where ;-) .
> 
> Though from past experience I do know that it makes reading devicetrees
> easier when you know which property to expect in which place - especially
> when their number increases and right now you have status above here,
> and below everything else in the mmc node for example.
> 
> In the end Palmer might not care that much about tiny odering
> differences, but I do think following one scheme is definitively an
> advantage over mixing different ones.

Aye. I guess I will respin with the statuses at the end. If someone has
a problem, they're always free to raise an objection ;) Really doesn't
matter to me & if it makes reading the dt easier for some...


> 
> 
> Heiko
> 
> 
>>>> +};
>>>> +
>>>> +&mac1 {
>>>> +     status = "okay";
>>>> +     phy-mode = "sgmii";
>>>> +     phy-handle = <&phy1>;
>>>
>>> nit (1): same as above
>>> nit (2): blank line between properties and subnodes makes
>>>     everything more readable.
>>
>> Aye, not wrong. I'll fix this regardless of what happens with
>> the status ordering.
>> Thanks,
>> Conor.
>>
>>>
>>>> +     phy1: ethernet-phy@5 {
>>>> +             reg = <5>;
>>>> +             ti,fifo-depth = <0x01>;
>>>> +     };
>>>
>>> nit: blank line?
>>>
>>> Otherwise:
>>> Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>
>>>
>>>> +     phy0: ethernet-phy@4 {
>>>> +             reg = <4>;
>>>> +             ti,fifo-depth = <0x01>;
>>>> +     };
>>>> +};
>>>> +
>>>> +&mbox {
>>>> +     status = "okay";
>>>> +};
>>>> +
>>>> +&mmc {
>>>> +     bus-width = <4>;
>>>> +     disable-wp;
>>>> +     cap-sd-highspeed;
>>>> +     cap-mmc-highspeed;
>>>> +     card-detect-delay = <200>;
>>>> +     mmc-ddr-1_8v;
>>>> +     mmc-hs200-1_8v;
>>>> +     sd-uhs-sdr12;
>>>> +     sd-uhs-sdr25;
>>>> +     sd-uhs-sdr50;
>>>> +     sd-uhs-sdr104;
>>>> +     status = "okay";
>>>> +};
>>>> +
>>>> +&mmuart0 {
>>>> +     status = "okay";
>>>> +};
>>>> +
>>>> +&refclk {
>>>> +     clock-frequency = <125000000>;
>>>> +};
>>>> +
>>>> +&rtc {
>>>> +     status = "okay";
>>>> +};
>>>> +
>>>> +&syscontroller {
>>>> +     status = "okay";
>>>> +};
>>>>
>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 
>