Re: [PATCH 1/2] arm64: dts: ti: k3-am62p: Add nodes for more IPs

From: Bryan Brattlof
Date: Fri Oct 13 2023 - 12:48:23 EST

Hey Vignesh!

On October 12, 2023 thus sayeth Vignesh Raghavendra:
> On 10/10/23 09:29, Bryan Brattlof wrote:
> > From: Vignesh Raghavendra <vigneshr@xxxxxx>
> >
> > The am62px shares many of the same IP as the existing am62x family
> > of SoCs, Introduce more nodes for hardware available on the am62p5.
> >
> > Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> > Signed-off-by: Bryan Brattlof <bb@xxxxxx>
> > ---
> > arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 835 ++++++++++++++++++-
> > arch/arm64/boot/dts/ti/k3-am62p-mcu.dtsi | 191 +++++
> > arch/arm64/boot/dts/ti/k3-am62p-thermal.dtsi | 47 ++
> > arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 67 ++
> > arch/arm64/boot/dts/ti/k3-am62p.dtsi | 2 +
> > 5 files changed, 1141 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/boot/dts/ti/k3-am62p-thermal.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
> > index c24ff905437ff..b754c18c3325b 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
> > +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
> > @@ -40,9 +40,29 @@ gic_its: msi-controller@1820000 {
> > };
> > };
> >
> > + main_conf: syscon@100000 {
> > + compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";
>
> simple-bus is sufficient
>

Ah cool! I can fix that

> > + reg = <0x00 0x00100000 0x00 0x20000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0x0 0x00 0x00100000 0x20000>;
> > +
> > + phy_gmii_sel: phy@4044 {
> > + compatible = "ti,am654-phy-gmii-sel";
> > + reg = <0x4044 0x8>;
> > + #phy-cells = <1>;
> > + };
> > +
> > + epwm_tbclk: clock-controller@4130 {
> > + compatible = "ti,am62-epwm-tbclk";
> > + reg = <0x4130 0x4>;
> > + #clock-cells = <1>;
> > + };
> > + };
> > +
> > dmss: bus@48000000 {
> > bootph-all;
>
> Here and elsehwere, Lets keep this property at the last, prefer to have compatible,
> reg, reg-names to be at the beginning
>

Sounds good :)

> > - compatible = "simple-mfd";
> > + compatible = "simple-bus";
>
> This is already address by [0]
>
> [0] https://lore.kernel.org/linux-arm-kernel/20231005151302.1290363-2-vigneshr@xxxxxx/
>

Oops, I guess I haven't been paying attention :$

> > #address-cells = <2>;
> > #size-cells = <2>;
> > dma-ranges;

...

> > +
> > + main_gpio0: gpio@600000 {
> > + compatible = "ti,am64-gpio", "ti,keystone-gpio";
> > + reg = <0x0 0x00600000 0x0 0x100>;
>
>
> reg = <0x00 0x00600000 0x0 0x100>;
>

Ah I wasn't even looking for that.. I'll fix that everywhere

> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupt-parent = <&main_gpio_intr>;
> > + interrupts = <190>, <191>, <192>,
> > + <193>, <194>, <195>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + ti,ngpio = <92>;
> > + ti,davinci-gpio-unbanked = <0>;
> > + power-domains = <&k3_pds 77 TI_SCI_PD_EXCLUSIVE>;
> > + clocks = <&k3_clks 77 0>;
> > + clock-names = "gpio";
> > + };
> > +
> > + main_gpio1: gpio@601000 {
> > + compatible = "ti,am64-gpio", "ti,keystone-gpio";
> > + reg = <0x0 0x00601000 0x0 0x100>;
>
> Same here
>
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + interrupt-parent = <&main_gpio_intr>;
> > + interrupts = <180>, <181>, <182>,
> > + <183>, <184>, <185>;
> > + interrupt-controller;
> > + #interrupt-cells = <2>;
> > + ti,ngpio = <52>;
> > + ti,davinci-gpio-unbanked = <0>;
> > + power-domains = <&k3_pds 78 TI_SCI_PD_EXCLUSIVE>;
> > + clocks = <&k3_clks 78 0>;
> > + clock-names = "gpio";
> > + };
> > +
> > + sdhci0: mmc@fa10000 {
> > + compatible = "ti,am64-sdhci-8bit";
> > + reg = <0x00 0x0fa10000 0x00 0x1000>, <0x00 0x0fa18000 0x00 0x400>;
> > + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> > + power-domains = <&k3_pds 57 TI_SCI_PD_EXCLUSIVE>;
> > + clocks = <&k3_clks 57 1>, <&k3_clks 57 2>;
> > + clock-names = "clk_ahb", "clk_xin";
> > + assigned-clocks = <&k3_clks 57 2>;
> > + assigned-clock-parents = <&k3_clks 57 4>;
> > + mmc-ddr-1_8v;
> > + mmc-hs200-1_8v;
> > + mmc-hs400-1_8v;
> > + bus-width = <8>;
> > + ti,clkbuf-sel = <0x7>;
> > + ti,otap-del-sel-legacy = <0x0>;
> > + ti,otap-del-sel-mmc-hs = <0x0>;
> > + ti,otap-del-sel-ddr52 = <0x5>;
> > + ti,otap-del-sel-hs200 = <0x5>;
> > + ti,itap-del-sel-legacy = <0xa>;
> > + ti,itap-del-sel-mmc-hs = <0x1>;
> > + ti,otap-del-sel-hs400 = <0x5>;
> > + ti,strobe-sel = <0x77>;
>
> Do we have prelim datasheet? Are these values derived from such a source?
>

No datasheet that I'm aware of yet, I know HS200 and HS400 are somewhat
stable but these values would probably need to be adjusted.

> > + ti,trm-icp = <0x8>;
> > + status = "disabled";
> > + };

....

> > +
> > + usbss1: dwc3-usb@f910000 {
> > + compatible = "ti,am62-usb";
> > + reg = <0x00 0x0f910000 0x00 0x800>;
> > + clocks = <&k3_clks 162 3>;
> > + clock-names = "ref";
> > + ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;
>
> wkup_conf is no longer a syscon node. Does this still work?
>

Oh man, I didn't even notice, I was having trouble with the MDIO and I
turned it off, but for USB I don't recall even checking :$

I'll double check before I send out v2

> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;
> > + ranges;
> > + status = "disabled";
> > +
> > + usb1: usb@31100000 {
> > + compatible = "snps,dwc3";
> > + reg = <0x00 0x31100000 0x00 0x50000>;
> > + interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>, /* irq.0 */
> > + <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>; /* irq.0 */
> > + interrupt-names = "host", "peripheral";
> > + maximum-speed = "high-speed";
> > + dr_mode = "otg";
> > + };
> > + };

...

> > + cpsw3g: ethernet@8000000 {
> > + compatible = "ti,am642-cpsw-nuss";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + reg = <0x00 0x08000000 0x00 0x200000>;
> > + reg-names = "cpsw_nuss";
> > + ranges = <0x00 0x00 0x00 0x08000000 0x00 0x200000>;
> > + clocks = <&k3_clks 13 0>;
> > + assigned-clocks = <&k3_clks 13 3>;
> > + assigned-clock-parents = <&k3_clks 13 11>;
> > + clock-names = "fck";
> > + power-domains = <&k3_pds 13 TI_SCI_PD_EXCLUSIVE>;
> > +
> > + dmas = <&main_pktdma 0xc600 15>,
> > + <&main_pktdma 0xc601 15>,
> > + <&main_pktdma 0xc602 15>,
> > + <&main_pktdma 0xc603 15>,
> > + <&main_pktdma 0xc604 15>,
> > + <&main_pktdma 0xc605 15>,
> > + <&main_pktdma 0xc606 15>,
> > + <&main_pktdma 0xc607 15>,
> > + <&main_pktdma 0x4600 15>;
> > + dma-names = "tx0", "tx1", "tx2", "tx3", "tx4", "tx5", "tx6",
> > + "tx7", "rx";
> > +
> > + ethernet-ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + cpsw_port1: port@1 {
> > + reg = <1>;
> > + ti,mac-only;
> > + label = "port1";
> > + phys = <&phy_gmii_sel 1>;
> > + mac-address = [00 00 00 00 00 00];
> > + ti,syscon-efuse = <&wkup_conf 0x200>;
>
>
> Same here
>
> > + };
> > +
> > + cpsw_port2: port@2 {
> > + reg = <2>;
> > + ti,mac-only;
> > + label = "port2";
> > + phys = <&phy_gmii_sel 2>;
> > + mac-address = [00 00 00 00 00 00];
> > + };
> > + };
> > +
> > + cpsw3g_mdio: mdio@f00 {
> > + compatible = "ti,cpsw-mdio","ti,davinci_mdio";
> > + reg = <0x00 0xf00 0x00 0x100>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clocks = <&k3_clks 13 0>;
> > + clock-names = "fck";
> > + bus_freq = <1000000>;
> > + status = "disabled";
> > + };
> > +
> > + cpts@3d000 {
> > + compatible = "ti,j721e-cpts";
> > + reg = <0x00 0x3d000 0x00 0x400>;
> > + clocks = <&k3_clks 13 3>;
> > + clock-names = "cpts";
> > + interrupts-extended = <&gic500 GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "cpts";
> > + ti,cpts-ext-ts-inputs = <4>;
> > + ti,cpts-periodic-outputs = <2>;
> > + };
> > + };
> > +

...

> > +
> > + mcu_esm: esm@4100000 {
> > + bootph-pre-ram;
> > + compatible = "ti,j721e-esm";
> > + reg = <0x00 0x4100000 0x00 0x1000>;
> > + ti,esm-pins = <0>, <1>, <2>, <85>;
>
>
> Do we want to keep it reserved by default as this maybe under MCU FW control?
>

Yeah that's a better idea

Thanks for the review Vignesh!
~Bryan