[PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Peter Ujfalusi]

Add the nodes for McASP 0-2 and keep them disabled because several
required properties are not present as they are board specific.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 57 +++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
index 72d6496e88dd..cc6c2a81887a 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
@@ -446,4 +446,61 @@ usb0: usb@6000000 {
 			dr_mode = "otg";
 		};
 	};
+
+	mcasp0: mcasp@02b00000 {
+		compatible = "ti,am33xx-mcasp-audio";
+		reg = <0x0 0x02b00000 0x0 0x2000>,
+			<0x0 0x02b08000 0x0 0x1000>;
+		reg-names = "mpu","dat";
+		interrupts = <GIC_SPI 544 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 545 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "tx", "rx";
+
+		dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
+		dma-names = "tx", "rx";
+
+		clocks = <&k3_clks 174 40>;
+		clock-names = "fck";
+		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
+
+		status = "disabled";
+	};
+
+	mcasp1: mcasp@02b10000 {
+		compatible = "ti,am33xx-mcasp-audio";
+		reg = <0x0 0x02b10000 0x0 0x2000>,
+			<0x0 0x02b18000 0x0 0x1000>;
+		reg-names = "mpu","dat";
+		interrupts = <GIC_SPI 546 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 547 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "tx", "rx";
+
+		dmas = <&main_udmap 0xc401>, <&main_udmap 0x4401>;
+		dma-names = "tx", "rx";
+
+		clocks = <&k3_clks 175 40>;
+		clock-names = "fck";
+		power-domains = <&k3_pds 175 TI_SCI_PD_EXCLUSIVE>;
+
+		status = "disabled";
+	};
+
+	mcasp2: mcasp@02b20000 {
+		compatible = "ti,am33xx-mcasp-audio";
+		reg = <0x0 0x02b20000 0x0 0x2000>,
+			<0x0 0x02b28000 0x0 0x1000>;
+		reg-names = "mpu","dat";
+		interrupts = <GIC_SPI 548 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 549 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "tx", "rx";
+
+		dmas = <&main_udmap 0xc402>, <&main_udmap 0x4402>;
+		dma-names = "tx", "rx";
+
+		clocks = <&k3_clks 176 40>;
+		clock-names = "fck";
+		power-domains = <&k3_pds 176 TI_SCI_PD_EXCLUSIVE>;
+
+		status = "disabled";
+	};
 };
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Nishanth Menon]

On 10:48-20201005, Peter Ujfalusi wrote:
> Add the nodes for McASP 0-2 and keep them disabled because several
> required properties are not present as they are board specific.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 57 +++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> index 72d6496e88dd..cc6c2a81887a 100644
> --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> @@ -446,4 +446,61 @@ usb0: usb@6000000 {
>  			dr_mode = "otg";
>  		};
>  	};
> +
> +	mcasp0: mcasp@02b00000 {
> +		compatible = "ti,am33xx-mcasp-audio";
> +		reg = <0x0 0x02b00000 0x0 0x2000>,
> +			<0x0 0x02b08000 0x0 0x1000>;
> +		reg-names = "mpu","dat";
> +		interrupts = <GIC_SPI 544 IRQ_TYPE_LEVEL_HIGH>,
> +				<GIC_SPI 545 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "tx", "rx";
> +
> +		dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
> +		dma-names = "tx", "rx";
> +
> +		clocks = <&k3_clks 174 40>;
> +		clock-names = "fck";
> +		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
> +
> +		status = "disabled";

I see that there is inconsistent usage of "disabled" in our SoC.dts

Our generic rule has been set them to disabled in board.dtsi
McASP and DSS for existing SoC dts do not follow this.. which is a tad
confusing.. (considering that not even all uarts come out on every board
and every uart needs pinmux to function..)

Tero: Thoughts?

[...]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Peter Ujfalusi]

On 05/10/2020 14.58, Nishanth Menon wrote:
> On 10:48-20201005, Peter Ujfalusi wrote:
>> Add the nodes for McASP 0-2 and keep them disabled because several
>> required properties are not present as they are board specific.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 57 +++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>> index 72d6496e88dd..cc6c2a81887a 100644
>> --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>> @@ -446,4 +446,61 @@ usb0: usb@6000000 {
>>  			dr_mode = "otg";
>>  		};
>>  	};
>> +x
>> +	mcasp0: mcasp@02b00000 {
>> +		compatible = "ti,am33xx-mcasp-audio";
>> +		reg = <0x0 0x02b00000 0x0 0x2000>,
>> +			<0x0 0x02b08000 0x0 0x1000>;
>> +		reg-names = "mpu","dat";
>> +		interrupts = <GIC_SPI 544 IRQ_TYPE_LEVEL_HIGH>,
>> +				<GIC_SPI 545 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-names = "tx", "rx";
>> +
>> +		dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
>> +		dma-names = "tx", "rx";
>> +
>> +		clocks = <&k3_clks 174 40>;
>> +		clock-names = "fck";
>> +		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
>> +
>> +		status = "disabled";
> 
> I see that there is inconsistent usage of "disabled" in our SoC.dts
> 
> Our generic rule has been set them to disabled in board.dtsi
> McASP and DSS for existing SoC dts do not follow this.. which is a tad
> confusing.. (considering that not even all uarts come out on every board
> and every uart needs pinmux to function..)

"keep them disabled because several required properties are not present
as they are board specific."

In board file the enabled mcasp must be updated with options that is
required for operation. Without those option the McASP can not be
initialized.

I think we have been revisiting the very same discussion every time we
have a new SoC with McASP...

> 
> Tero: Thoughts?
> 
> [...]
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Nishanth Menon]

On 15:02-20201005, Peter Ujfalusi wrote:
> 
> 
> On 05/10/2020 14.58, Nishanth Menon wrote:
> > On 10:48-20201005, Peter Ujfalusi wrote:
> >> Add the nodes for McASP 0-2 and keep them disabled because several
> >> required properties are not present as they are board specific.
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 57 +++++++++++++++++++++++
> >>  1 file changed, 57 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> >> index 72d6496e88dd..cc6c2a81887a 100644
> >> --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> >> +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
> >> @@ -446,4 +446,61 @@ usb0: usb@6000000 {
> >>  			dr_mode = "otg";
> >>  		};
> >>  	};
> >> +x
> >> +	mcasp0: mcasp@02b00000 {
> >> +		compatible = "ti,am33xx-mcasp-audio";
> >> +		reg = <0x0 0x02b00000 0x0 0x2000>,
> >> +			<0x0 0x02b08000 0x0 0x1000>;
> >> +		reg-names = "mpu","dat";
> >> +		interrupts = <GIC_SPI 544 IRQ_TYPE_LEVEL_HIGH>,
> >> +				<GIC_SPI 545 IRQ_TYPE_LEVEL_HIGH>;
> >> +		interrupt-names = "tx", "rx";
> >> +
> >> +		dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
> >> +		dma-names = "tx", "rx";
> >> +
> >> +		clocks = <&k3_clks 174 40>;
> >> +		clock-names = "fck";
> >> +		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
> >> +
> >> +		status = "disabled";
> > 
> > I see that there is inconsistent usage of "disabled" in our SoC.dts
> > 
> > Our generic rule has been set them to disabled in board.dtsi
> > McASP and DSS for existing SoC dts do not follow this.. which is a tad
> > confusing.. (considering that not even all uarts come out on every board
> > and every uart needs pinmux to function..)
> 
> "keep them disabled because several required properties are not present
> as they are board specific."
> 
> In board file the enabled mcasp must be updated with options that is
> required for operation. Without those option the McASP can not be
> initialized.
> 
> I think we have been revisiting the very same discussion every time we
> have a new SoC with McASP...
> 

Yep.. This doe'snt really follow the rest of the SoC definition. [1]
came to mind. The McASP discussion is a variation in the debate of the
same. I'd argue Serdes, or for that matter any IP that has a link to
outside-the-SoC world has the same discussion point.

[1] https://patchwork.kernel.org/patch/9304575/

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Peter Ujfalusi]

On 05/10/2020 15.08, Nishanth Menon wrote:
> On 15:02-20201005, Peter Ujfalusi wrote:
>>
>>
>> On 05/10/2020 14.58, Nishanth Menon wrote:
>>> On 10:48-20201005, Peter Ujfalusi wrote:
>>>> Add the nodes for McASP 0-2 and keep them disabled because several
>>>> required properties are not present as they are board specific.
>>>>
>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>> ---
>>>>  arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 57 +++++++++++++++++++++++
>>>>  1 file changed, 57 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>>>> index 72d6496e88dd..cc6c2a81887a 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
>>>> @@ -446,4 +446,61 @@ usb0: usb@6000000 {
>>>>  			dr_mode = "otg";
>>>>  		};
>>>>  	};
>>>> +x
>>>> +	mcasp0: mcasp@02b00000 {
>>>> +		compatible = "ti,am33xx-mcasp-audio";
>>>> +		reg = <0x0 0x02b00000 0x0 0x2000>,
>>>> +			<0x0 0x02b08000 0x0 0x1000>;
>>>> +		reg-names = "mpu","dat";
>>>> +		interrupts = <GIC_SPI 544 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				<GIC_SPI 545 IRQ_TYPE_LEVEL_HIGH>;
>>>> +		interrupt-names = "tx", "rx";
>>>> +
>>>> +		dmas = <&main_udmap 0xc400>, <&main_udmap 0x4400>;
>>>> +		dma-names = "tx", "rx";
>>>> +
>>>> +		clocks = <&k3_clks 174 40>;
>>>> +		clock-names = "fck";
>>>> +		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
>>>> +
>>>> +		status = "disabled";
>>>
>>> I see that there is inconsistent usage of "disabled" in our SoC.dts
>>>
>>> Our generic rule has been set them to disabled in board.dtsi
>>> McASP and DSS for existing SoC dts do not follow this.. which is a tad
>>> confusing.. (considering that not even all uarts come out on every board
>>> and every uart needs pinmux to function..)
>>
>> "keep them disabled because several required properties are not present
>> as they are board specific."
>>
>> In board file the enabled mcasp must be updated with options that is
>> required for operation. Without those option the McASP can not be
>> initialized.
>>
>> I think we have been revisiting the very same discussion every time we
>> have a new SoC with McASP...
>>
> 
> Yep.. This doe'snt really follow the rest of the SoC definition. [1]
> came to mind. The McASP discussion is a variation in the debate of the
> same.

Right, saying status = "okay" to a node which is missing required
properties (which can only be added by boards when the McASP is
connected up) does not sound a good solution.
How should the SW handle that? Fail the device probe and return with
-EINVAL or eat up the error and just probe with broken configuration.
Since the peripheral is not used, the broken configuration will not
cause much runtime errors as there will be no runtime use of the peripheral.

status of fail or fail-sss is not a good one either, their definition is:
"Indicates that the device is not operational. A serious error was
detected in the device, and it is unlikely to become operational without
repair."

The peripheral is fine, we are just trying to enable it without
providing the needed properties.

> I'd argue Serdes, or for that matter any IP that has a link to
> outside-the-SoC world has the same discussion point.

status = "disabled" is still the closest thing for everything which have
external dependencies. There is not much point to enable an i2c bus
without making sure that the signals are actually routed to the pins
where they supposed to go.

Or from other pow: a board design is not based on what is _not_
connected to outside world, but you actually _connect_ or _enable_
certain peripherals to external components, connectors.

> [1] https://patchwork.kernel.org/patch/9304575/
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Nishanth Menon]

Peter,

On 13:38-20201007, Peter Ujfalusi wrote:
[...]
> >>>> +		status = "disabled";
> >>>
> >>> I see that there is inconsistent usage of "disabled" in our SoC.dts
> >>>
> >>> Our generic rule has been set them to disabled in board.dtsi
> >>> McASP and DSS for existing SoC dts do not follow this.. which is a tad
> >>> confusing.. (considering that not even all uarts come out on every board
> >>> and every uart needs pinmux to function..)
> >>
> >> "keep them disabled because several required properties are not present
> >> as they are board specific."
> >>
> >> In board file the enabled mcasp must be updated with options that is
> >> required for operation. Without those option the McASP can not be
> >> initialized.
> >>
> >> I think we have been revisiting the very same discussion every time we
> >> have a new SoC with McASP...
> >>
> > 
> > Yep.. This doe'snt really follow the rest of the SoC definition. [1]
> > came to mind. The McASP discussion is a variation in the debate of the
> > same.
> 
> Right, saying status = "okay" to a node which is missing required
> properties (which can only be added by boards when the McASP is
> connected up) does not sound a good solution.
> How should the SW handle that? Fail the device probe and return with
> -EINVAL or eat up the error and just probe with broken configuration.
> Since the peripheral is not used, the broken configuration will not
> cause much runtime errors as there will be no runtime use of the peripheral.
> 
> status of fail or fail-sss is not a good one either, their definition is:
> "Indicates that the device is not operational. A serious error was
> detected in the device, and it is unlikely to become operational without
> repair."
> 
> The peripheral is fine, we are just trying to enable it without
> providing the needed properties.
> 
> > I'd argue Serdes, or for that matter any IP that has a link to
> > outside-the-SoC world has the same discussion point.
> 
> status = "disabled" is still the closest thing for everything which have
> external dependencies. There is not much point to enable an i2c bus
> without making sure that the signals are actually routed to the pins
> where they supposed to go.
> 
> Or from other pow: a board design is not based on what is _not_
> connected to outside world, but you actually _connect_ or _enable_
> certain peripherals to external components, connectors.

OK, I will buy the argument that the current status thingy is a bit
overloaded and does'nt imply the correct state we need it to imply with
"fail-sss" either - I remember an argument for "fail-incomplete", but
that never happened anyways.

Lets add this argument to the commit message and repost after testing
on 5.10-rc1 please?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [201026 14:58]:
> On 13:38-20201007, Peter Ujfalusi wrote:
> [...]
> > >>>> +		status = "disabled";
> > >>>
> > >>> I see that there is inconsistent usage of "disabled" in our SoC.dts
> > >>>
> > >>> Our generic rule has been set them to disabled in board.dtsi
> > >>> McASP and DSS for existing SoC dts do not follow this.. which is a tad
> > >>> confusing.. (considering that not even all uarts come out on every board
> > >>> and every uart needs pinmux to function..)
> > >>
> > >> "keep them disabled because several required properties are not present
> > >> as they are board specific."
> > >>
> > >> In board file the enabled mcasp must be updated with options that is
> > >> required for operation. Without those option the McASP can not be
> > >> initialized.
> > >>
> > >> I think we have been revisiting the very same discussion every time we
> > >> have a new SoC with McASP...
> > >>
> > > 
> > > Yep.. This doe'snt really follow the rest of the SoC definition. [1]
> > > came to mind. The McASP discussion is a variation in the debate of the
> > > same.
> > 
> > Right, saying status = "okay" to a node which is missing required
> > properties (which can only be added by boards when the McASP is
> > connected up) does not sound a good solution.
> > How should the SW handle that? Fail the device probe and return with
> > -EINVAL or eat up the error and just probe with broken configuration.
> > Since the peripheral is not used, the broken configuration will not
> > cause much runtime errors as there will be no runtime use of the peripheral.
> > 
> > status of fail or fail-sss is not a good one either, their definition is:
> > "Indicates that the device is not operational. A serious error was
> > detected in the device, and it is unlikely to become operational without
> > repair."
> > 
> > The peripheral is fine, we are just trying to enable it without
> > providing the needed properties.
> > 
> > > I'd argue Serdes, or for that matter any IP that has a link to
> > > outside-the-SoC world has the same discussion point.
> > 
> > status = "disabled" is still the closest thing for everything which have
> > external dependencies. There is not much point to enable an i2c bus
> > without making sure that the signals are actually routed to the pins
> > where they supposed to go.

There is one reason to keep using the default status = "okay", for
i2c Linux can properly idle the device on boot without dependencies
to a certain boot loader version.

> > Or from other pow: a board design is not based on what is _not_
> > connected to outside world, but you actually _connect_ or _enable_
> > certain peripherals to external components, connectors.
> 
> OK, I will buy the argument that the current status thingy is a bit
> overloaded and does'nt imply the correct state we need it to imply with
> "fail-sss" either - I remember an argument for "fail-incomplete", but
> that never happened anyways.
> 
> Lets add this argument to the commit message and repost after testing
> on 5.10-rc1 please?

We should use status = "disabled" when the hardware is not accessible
to the SoC. That goes for things like secure world devices, and hardware
that relies on a missing external clock for example. For other things
we should just stick to the default which "okay" :)

Regards,

Tony

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Nishanth Menon]

Hi Tony,

   Thanks for joining the discussion.

On 15:07-20201027, Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [201026 14:58]:
> > On 13:38-20201007, Peter Ujfalusi wrote:
> > [...]
> > > >>>> +		status = "disabled";
> > > >>>
> > > >>> I see that there is inconsistent usage of "disabled" in our SoC.dts
> > > >>>
> > > >>> Our generic rule has been set them to disabled in board.dtsi
> > > >>> McASP and DSS for existing SoC dts do not follow this.. which is a tad
> > > >>> confusing.. (considering that not even all uarts come out on every board
> > > >>> and every uart needs pinmux to function..)
> > > >>
> > > >> "keep them disabled because several required properties are not present
> > > >> as they are board specific."
> > > >>
> > > >> In board file the enabled mcasp must be updated with options that is
> > > >> required for operation. Without those option the McASP can not be
> > > >> initialized.
> > > >>
> > > >> I think we have been revisiting the very same discussion every time we
> > > >> have a new SoC with McASP...
> > > >>
> > > > 
> > > > Yep.. This doe'snt really follow the rest of the SoC definition. [1]
> > > > came to mind. The McASP discussion is a variation in the debate of the
> > > > same.
> > > 
> > > Right, saying status = "okay" to a node which is missing required
> > > properties (which can only be added by boards when the McASP is
> > > connected up) does not sound a good solution.
> > > How should the SW handle that? Fail the device probe and return with
> > > -EINVAL or eat up the error and just probe with broken configuration.
> > > Since the peripheral is not used, the broken configuration will not
> > > cause much runtime errors as there will be no runtime use of the peripheral.
> > > 
> > > status of fail or fail-sss is not a good one either, their definition is:
> > > "Indicates that the device is not operational. A serious error was
> > > detected in the device, and it is unlikely to become operational without
> > > repair."
> > > 
> > > The peripheral is fine, we are just trying to enable it without
> > > providing the needed properties.
> > > 
> > > > I'd argue Serdes, or for that matter any IP that has a link to
> > > > outside-the-SoC world has the same discussion point.
> > > 
> > > status = "disabled" is still the closest thing for everything which have
> > > external dependencies. There is not much point to enable an i2c bus
> > > without making sure that the signals are actually routed to the pins
> > > where they supposed to go.
> 
> There is one reason to keep using the default status = "okay", for
> i2c Linux can properly idle the device on boot without dependencies
> to a certain boot loader version.

Yes, but in K3 architecture (and I suspect in many SCMI like
architectures involving heterogenous systems), Linux does'nt even own the
system power management of peripherals at all. In fact,
we dont even see all the SoC components. if a particular I2C is being
used by some other processor, then that node is disabled for linux.

So, I am wondering if the argument of "status = okay" helps Linux PM
idle unused devices holds good on architectures where System PM is not
owned by Linux. That (system level PM) is not really Linux's job anymore.

> > > Or from other pow: a board design is not based on what is _not_
> > > connected to outside world, but you actually _connect_ or _enable_
> > > certain peripherals to external components, connectors.
> > 
> > OK, I will buy the argument that the current status thingy is a bit
> > overloaded and does'nt imply the correct state we need it to imply with
> > "fail-sss" either - I remember an argument for "fail-incomplete", but
> > that never happened anyways.
> > 
> > Lets add this argument to the commit message and repost after testing
> > on 5.10-rc1 please?
> 
> We should use status = "disabled" when the hardware is not accessible
> to the SoC. That goes for things like secure world devices, and hardware

I'd argue 'not accessible to Linux instance' (considering how visibility
of peripherals are constrained further in VMs)

> that relies on a missing external clock for example. For other things
> we should just stick to the default which "okay" :)

ok, the debate here (and looks like the DSS series as well[1]) is
what do you do when the there are not enough properties for a valid
device node. Do you still do status="okay" and let the driver fail ?
The driver folks are arguing, they don't want to do that since it is an
incomplete description of the hardware..

[1]
https://lore.kernel.org/linux-arm-kernel/20201027104132.105485-2-tomi.valkeinen@ti.com/
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH] arm64: dts: ti: k3-j7200-main: Add McASP nodes

[Peter Ujfalusi]

On 27/10/2020 15.07, Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [201026 14:58]:
>> On 13:38-20201007, Peter Ujfalusi wrote:
>> [...]
>>>>>>> +		status = "disabled";
>>>>>>
>>>>>> I see that there is inconsistent usage of "disabled" in our SoC.dts
>>>>>>
>>>>>> Our generic rule has been set them to disabled in board.dtsi
>>>>>> McASP and DSS for existing SoC dts do not follow this.. which is a tad
>>>>>> confusing.. (considering that not even all uarts come out on every board
>>>>>> and every uart needs pinmux to function..)
>>>>>
>>>>> "keep them disabled because several required properties are not present
>>>>> as they are board specific."
>>>>>
>>>>> In board file the enabled mcasp must be updated with options that is
>>>>> required for operation. Without those option the McASP can not be
>>>>> initialized.
>>>>>
>>>>> I think we have been revisiting the very same discussion every time we
>>>>> have a new SoC with McASP...
>>>>>
>>>>
>>>> Yep.. This doe'snt really follow the rest of the SoC definition. [1]
>>>> came to mind. The McASP discussion is a variation in the debate of the
>>>> same.
>>>
>>> Right, saying status = "okay" to a node which is missing required
>>> properties (which can only be added by boards when the McASP is
>>> connected up) does not sound a good solution.
>>> How should the SW handle that? Fail the device probe and return with
>>> -EINVAL or eat up the error and just probe with broken configuration.
>>> Since the peripheral is not used, the broken configuration will not
>>> cause much runtime errors as there will be no runtime use of the peripheral.
>>>
>>> status of fail or fail-sss is not a good one either, their definition is:
>>> "Indicates that the device is not operational. A serious error was
>>> detected in the device, and it is unlikely to become operational without
>>> repair."
>>>
>>> The peripheral is fine, we are just trying to enable it without
>>> providing the needed properties.
>>>
>>>> I'd argue Serdes, or for that matter any IP that has a link to
>>>> outside-the-SoC world has the same discussion point.
>>>
>>> status = "disabled" is still the closest thing for everything which have
>>> external dependencies. There is not much point to enable an i2c bus
>>> without making sure that the signals are actually routed to the pins
>>> where they supposed to go.
> 
> There is one reason to keep using the default status = "okay", for
> i2c Linux can properly idle the device on boot without dependencies
> to a certain boot loader version.

Right, but I would expect board files to disable not used peripherals,
peripherals which is not connected up in the board. Including i2c buses.

>>> Or from other pow: a board design is not based on what is _not_
>>> connected to outside world, but you actually _connect_ or _enable_
>>> certain peripherals to external components, connectors.
>>
>> OK, I will buy the argument that the current status thingy is a bit
>> overloaded and does'nt imply the correct state we need it to imply with
>> "fail-sss" either - I remember an argument for "fail-incomplete", but
>> that never happened anyways.
>>
>> Lets add this argument to the commit message and repost after testing
>> on 5.10-rc1 please?
> 
> We should use status = "disabled" when the hardware is not accessible
> to the SoC. That goes for things like secure world devices, and hardware
> that relies on a missing external clock for example. For other things
> we should just stick to the default which "okay" :)

Even for peripherals which needs board specific properties and those
properties are mandatory for the IP to be operational and usable?

The McASP bindings demands several must be present properties
(tdm-slots, slot direction for example) and they are coming from the
board design.
Board files will disable all unused McASPs, they should. Imho the board
dts file represents the board design and the board design is to
_connect_ peripherals and not _disconnect_ them.
Why would a board file should be in charge to disable what is not
connected/used?

iow: what is the difference of a peripheral disabled in dtsi over a
peripheral disabled in board dts file?

> 
> Regards,
> 
> Tony
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki