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";
>>>> +};
>>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>
