Re: [PATCH 3/4] arm64: dts: ti: Introduce AM62A7 family of SoCs

[Vignesh Raghavendra <vigneshr@xxxxxx>]

On 30/08/22 15:26, Krzysztof Kozlowski wrote:
> On 29/08/2022 11:21, Vignesh Raghavendra wrote:
> 
> (...)
> 
>> +		/*
>> +		 * vcpumntirq:
>> +		 * virtual CPU interface maintenance interrupt
>> +		 */
>> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +		gic_its: msi-controller@1820000 {
>> +			compatible = "arm,gic-v3-its";
>> +			reg = <0x00 0x01820000 0x00 0x10000>;
>> +			socionext,synquacer-pre-its = <0x1000000 0x400000>;
>> +			msi-controller;
>> +			#msi-cells = <1>;
>> +		};
>> +	};
>> +
>> +	main_conf: syscon@100000 {
>> +		compatible = "syscon", "simple-mfd";
> 
> No, these are not allowed alone.

Will add device specific compatible

compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";

> 
>> +		reg = <0x00 0x00100000 0x00 0x20000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x00 0x00 0x00100000 0x20000>;
>> +	};
>> +
>> +	dmss: bus@48000000 {
>> +		compatible = "simple-mfd";
> 
> No. Not allowed alone.

This can be "simple-bus". Will change accordingly

> 
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		dma-ranges;
>> +		ranges = <0x00 0x48000000 0x00 0x48000000 0x00 0x06000000>;
>> +
>> +		ti,sci-dev-id = <25>;
>> +
>> +		secure_proxy_main: mailbox@4d000000 {
>> +			compatible = "ti,am654-secure-proxy";
>> +			#mbox-cells = <1>;
>> +			reg-names = "target_data", "rt", "scfg";
>> +			reg = <0x00 0x4d000000 0x00 0x80000>,
>> +			      <0x00 0x4a600000 0x00 0x80000>,
>> +			      <0x00 0x4a400000 0x00 0x80000>;
>> +			interrupt-names = "rx_012";
>> +			interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +	};
>> +
>> +	dmsc: system-controller@44043000 {
>> +		compatible = "ti,k2g-sci";
>> +		ti,host-id = <12>;
>> +		mbox-names = "rx", "tx";
>> +		mboxes= <&secure_proxy_main 12>,
>> +			<&secure_proxy_main 13>;
>> +		reg-names = "debug_messages";
>> +		reg = <0x00 0x44043000 0x00 0xfe0>;
> 
> First compatible, then reg, then the reset of properties.  This applies
> everywhere.

Will fix

> 
>> +
>> +		k3_pds: power-controller {
>> +			compatible = "ti,sci-pm-domain";
>> +			#power-domain-cells = <2>;
>> +		};
>> +
>> +		k3_clks: clock-controller {
>> +			compatible = "ti,k2g-sci-clk";
>> +			#clock-cells = <2>;
>> +		};
>> +
>> +		k3_reset: reset-controller {
>> +			compatible = "ti,sci-reset";
>> +			#reset-cells = <2>;
>> +		};
>> +	};
>> +
[...]
>> diff --git a/arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi
>> new file mode 100644
>> index 000000000000..fe6d682a0f33
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi
>> @@ -0,0 +1,54 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree Source for AM62A SoC Family Wakeup Domain peripherals
>> + *
>> + * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>> + */
>> +
>> +&cbass_wakeup {
>> +	wkup_conf: syscon@43000000 {
>> +		compatible = "syscon", "simple-mfd";
> 
> No. Not allowed alone.

Will change to

compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";

> 
>> +		reg = <0x00 0x43000000 0x00 0x20000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x00 0x00 0x43000000 0x20000>;
>> +
>> +		chipid: chipid@14 {
>> +			compatible = "ti,am654-chipid";
>> +			reg = <0x14 0x4>;
>> +		};
>> +	};
>> +
> 
> 

Thanks for the review!
-- 
Regards
Vignesh