Re: [PATCH v3 1/6] arm64: dts: ti: k3-j784s4: Add Wave5 Video Encoder/Decoder Node

[Andrew Davis]

On 2/1/24 1:13 PM, Brnich, Brandon wrote:
> Hi Andrew,
>
> > -----Original Message-----
> > From: Davis, Andrew <afd@xxxxxx>
> > Sent: Thursday, February 1, 2024 12:35 PM
> > To: Brnich, Brandon <b-brnich@xxxxxx>; Menon, Nishanth <nm@xxxxxx>;
> > Raghavendra, Vignesh <vigneshr@xxxxxx>; Tero Kristo <kristo@xxxxxxxxxx>;
> > Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> > Catalin Marinas <catalin.marinas@xxxxxxx>; Will Deacon
> > <will@xxxxxxxxxx>; Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>; Geert
> > Uytterhoeven <geert+renesas@xxxxxxxxx>; Arnd Bergmann
> > <arnd@xxxxxxxx>; Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>; Neil
> > Armstrong <neil.armstrong@xxxxxxxxxx>; Nícolas F . R . A . Prado
> > <nfraprado@xxxxxxxxxxxxx>; Marek Szyprowski
> > <m.szyprowski@xxxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Etheridge, Darren
> > <detheridge@xxxxxx>
> > Subject: Re: [PATCH v3 1/6] arm64: dts: ti: k3-j784s4: Add Wave5 Video
> > Encoder/Decoder Node
> >
> > On 1/31/24 3:26 PM, Brandon Brnich wrote:
> > > This patch adds support for the Wave521cl on the J784S4-evm.
> > >
> > > Signed-off-by: Brandon Brnich <b-brnich@xxxxxx>
> > > ---
> > >    arch/arm64/boot/dts/ti/k3-j784s4-evm.dts   |  8 ++++++++
> > >    arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 20 ++++++++++++++++++++
> > >    arch/arm64/boot/dts/ti/k3-j784s4.dtsi      |  2 ++
> > >    3 files changed, 30 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> > > b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> > > index f34b92acc56d..7d37c11b4df4 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> > > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
> > > @@ -784,6 +784,14 @@ &main_gpio0 {
> > >    	status = "okay";
> > >    };
> > >
> > > +&vpu0 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&vpu1 {
> > > +	status = "okay";
> > > +};
> > > +
> > >    &mcu_cpsw {
> > >    	status = "okay";
> > >    	pinctrl-names = "default";
> > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > index f2b720ed1e4f..8b2623ac8160 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > @@ -662,6 +662,26 @@ main_i2c6: i2c@2060000 {
> > >    		status = "disabled";
> > >    	};
> > >
> > > +	vpu0: video-codec@4210000 {
> > > +		compatible = "ti,j721s2-wave521c", "cnm,wave521c";
> > > +		reg = <0x00 0x4210000 0x00 0x10000>;
> > > +		interrupts = <GIC_SPI 182 IRQ_TYPE_LEVEL_HIGH>;
> > > +		clocks = <&k3_clks 241 2>;
> > > +		clock-names = "vcodec";
> > > +		power-domains = <&k3_pds 241 TI_SCI_PD_EXCLUSIVE>;
> > > +		status = "disabled";
> >
> > Why are these default disabled? I don't see anything missing that would
> > need to be added at the board level. You disable them just to re-enable them
> > in the next patch. Leave these default enabled.
>
> I thought that disabled by default was the standard for node in dtsi files, where
> they get specifically enabled in the particular dts file for the SoC.

Only disabled if there is missing information needed for the node to function
that can only be added at the board level, e.g. pinmux usually.

> In V4 I will remove the disabled by default. Should this apply to all platforms in
> the series?

Yes

Andrew

>   
> > > +	};
> > > +
> > > +	vpu1: video-codec@4220000 {
> > > +		compatible = "ti,j721s2-wave521c", "cnm,wave521c";
> > > +		reg = <0x00 0x4220000 0x00 0x10000>;
> > > +		interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>;
> > > +		clocks = <&k3_clks 242 2>;
> > > +		clock-names = "vcodec";
> > > +		power-domains = <&k3_pds 242 TI_SCI_PD_EXCLUSIVE>;
> > > +		status = "disabled";
> > > +	};
> > > +
> > >    	main_sdhci0: mmc@4f80000 {
> > >    		compatible = "ti,j721e-sdhci-8bit";
> > >    		reg = <0x00 0x04f80000 0x00 0x1000>, diff --git
> > > a/arch/arm64/boot/dts/ti/k3-j784s4.dtsi
> > > b/arch/arm64/boot/dts/ti/k3-j784s4.dtsi
> > > index 4398c3a463e1..93bb0cba1b48 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j784s4.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-j784s4.dtsi
> > > @@ -247,6 +247,8 @@ cbass_main: bus@100000 {
> > >    			 <0x00 0x30000000 0x00 0x30000000 0x00
> > 0x0c400000>, /* MAIN NAVSS */
> > >    			 <0x41 0x00000000 0x41 0x00000000 0x01
> > 0x00000000>, /* PCIe1 DAT1 */
> > >    			 <0x4e 0x20000000 0x4e 0x20000000 0x00
> > 0x00080000>, /* GPU */
> > > +			 <0x00 0x04210000 0x00 0x04210000 0x00
> > 0x00010000>, /* VPU0 */
> > > +			 <0x00 0x04220000 0x00 0x04220000 0x00
> > 0x00010000>, /* VPU1 */
> >
> > Add these in sorted by memory address order.
>
> Will do in V4 as well.
>
> > Andrew
> >
> > >    			 /* MCUSS_WKUP Range */
> > >    			 <0x00 0x28380000 0x00 0x28380000 0x00
> > 0x03880000>,
>
> Brandon