diff mbox series

[v4,3/3] arm64: dts: ti: k3-am69-sk: Add phase tags marking

Message ID 20230811151644.3216621-4-a-nandan@ti.com
State New
Headers
Series arm64: dts: ti: k3-j784s4: Add phase tags marking |

Commit Message

Apurva Nandan Aug. 11, 2023, 3:16 p.m. UTC 
  bootph-all as phase tag was added to dt-schema
(dtschema/schemas/bootph.yaml) to cover U-Boot challenges with DT.
That's why add it also to Linux to be aligned with bootloader requirement.

wkup_uart0, wkup_i2c0, mcu_uart0, main_uart8, main_sdhci0 and main_sdhci1
are required for bootloader operation on TI K3 AM69-SK EVM. These IPs
along with pinmuxes need to be marked for all bootloader phases, hence add
bootph-all to these nodes in kernel dts.

Signed-off-by: Apurva Nandan <a-nandan@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am69-sk.dts | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Kumar, Udit Aug. 11, 2023, 5:35 p.m. UTC | #1 
Hi Apurva

On 8/11/2023 8:46 PM, Apurva Nandan wrote:
> bootph-all as phase tag was added to dt-schema
> (dtschema/schemas/bootph.yaml) to cover U-Boot challenges with DT.
> That's why add it also to Linux to be aligned with bootloader requirement.
>
> wkup_uart0, wkup_i2c0, mcu_uart0, main_uart8, main_sdhci0 and main_sdhci1
> are required for bootloader operation on TI K3 AM69-SK EVM. These IPs
> along with pinmuxes need to be marked for all bootloader phases, hence add
> bootph-all to these nodes in kernel dts.
>
> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> ---
> [...]
>   &wkup_uart0 {
> +	bootph-all;
>   	/* Firmware usage */
>   	status = "reserved";
>   	pinctrl-names = "default";

I am not sure, if you want to treat wkup_uart in same way as you are 
treating secure_proxy_mcu in patch 1 of this series.

IMO, where we are making this node status is okay, mark booth-all at 
that place only.

Otherwise for rest of series

LGTM


> @@ -249,6 +257,7 @@ &wkup_uart0 {
>   };
>   
>   &wkup_i2c0 {
> +	bootph-all;
>   	status = "okay";
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&wkup_i2c0_pins_default>;
> @@ -268,6 +277,7 @@ &wkup_gpio0 {
>   };
>   
>   &mcu_uart0 {
> +	bootph-all;
>   	status = "okay";
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&mcu_uart0_pins_default>;
> @@ -281,6 +291,7 @@ &mcu_i2c0 {
>   };
>   
>   &main_uart8 {
> +	bootph-all;
>   	status = "okay";
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&main_uart8_pins_default>;
> @@ -307,6 +318,7 @@ exp1: gpio@21 {
>   };
>   
>   &main_sdhci0 {
> +	bootph-all;
>   	/* eMMC */
>   	status = "okay";
>   	non-removable;
> @@ -315,6 +327,7 @@ &main_sdhci0 {
>   };
>   
>   &main_sdhci1 {
> +	bootph-all;
>   	/* SD card */
>   	status = "okay";
>   	pinctrl-0 = <&main_mmc1_pins_default>;
Nishanth Menon Aug. 11, 2023, 5:54 p.m. UTC | #2 
On 23:05-20230811, Kumar, Udit wrote:
> Hi Apurva
> 
> On 8/11/2023 8:46 PM, Apurva Nandan wrote:
> > bootph-all as phase tag was added to dt-schema
> > (dtschema/schemas/bootph.yaml) to cover U-Boot challenges with DT.
> > That's why add it also to Linux to be aligned with bootloader requirement.
> > 
> > wkup_uart0, wkup_i2c0, mcu_uart0, main_uart8, main_sdhci0 and main_sdhci1
> > are required for bootloader operation on TI K3 AM69-SK EVM. These IPs
> > along with pinmuxes need to be marked for all bootloader phases, hence add
> > bootph-all to these nodes in kernel dts.
> > 
> > Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> > ---
> > [...]
> >   &wkup_uart0 {
> > +	bootph-all;
> >   	/* Firmware usage */
> >   	status = "reserved";
> >   	pinctrl-names = "default";
> 
> I am not sure, if you want to treat wkup_uart in same way as you are
> treating secure_proxy_mcu in patch 1 of this series.

You should'nt. wkup_uart0 or what ever peripherals are specifically
board dependent. This patch does it the right way. I do have other
platforms on other K3 SoCs where the TIFS uart logs are actually
disabled.

> 
> IMO, where we are making this node status is okay, mark booth-all at that
> place only.
> 
> Otherwise for rest of series
> 
> LGTM

Do i take that as a Reviewed-by: for the series?
> 
> 
[...]
Kumar, Udit Aug. 11, 2023, 6:05 p.m. UTC | #3 
On 8/11/2023 11:24 PM, Nishanth Menon wrote:
> On 23:05-20230811, Kumar, Udit wrote:
>> Hi Apurva
>>
>> On 8/11/2023 8:46 PM, Apurva Nandan wrote:
>>> bootph-all as phase tag was added to dt-schema
>>> (dtschema/schemas/bootph.yaml) to cover U-Boot challenges with DT.
>>> That's why add it also to Linux to be aligned with bootloader requirement.
>>>
>>> wkup_uart0, wkup_i2c0, mcu_uart0, main_uart8, main_sdhci0 and main_sdhci1
>>> are required for bootloader operation on TI K3 AM69-SK EVM. These IPs
>>> along with pinmuxes need to be marked for all bootloader phases, hence add
>>> bootph-all to these nodes in kernel dts.
>>>
>>> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
>>> ---
>>> [...]
>>>    &wkup_uart0 {
>>> +	bootph-all;
>>>    	/* Firmware usage */
>>>    	status = "reserved";
>>>    	pinctrl-names = "default";
>> I am not sure, if you want to treat wkup_uart in same way as you are
>> treating secure_proxy_mcu in patch 1 of this series.
> You should'nt. wkup_uart0 or what ever peripherals are specifically
> board dependent. This patch does it the right way. I do have other
> platforms on other K3 SoCs where the TIFS uart logs are actually
> disabled.

Sorry, if i was not clear in my previous response.

This node is marked as reserved, adding bootph is not adding any value.

We can drop bootph from this node here.

>> IMO, where we are making this node status is okay, mark booth-all at that
>> place only.
>>
>> Otherwise for rest of series
>>
>> LGTM
> Do i take that as a Reviewed-by: for the series?


With above change,

Reviewed-by: Udit Kumar <u-kumar1@ti.com>

>>
> [...]
>
Nishanth Menon Aug. 11, 2023, 6:12 p.m. UTC | #4 
On 23:35-20230811, Kumar, Udit wrote:
> 
> On 8/11/2023 11:24 PM, Nishanth Menon wrote:
> > On 23:05-20230811, Kumar, Udit wrote:
> > > Hi Apurva
> > > 
> > > On 8/11/2023 8:46 PM, Apurva Nandan wrote:
> > > > bootph-all as phase tag was added to dt-schema
> > > > (dtschema/schemas/bootph.yaml) to cover U-Boot challenges with DT.
> > > > That's why add it also to Linux to be aligned with bootloader requirement.
> > > > 
> > > > wkup_uart0, wkup_i2c0, mcu_uart0, main_uart8, main_sdhci0 and main_sdhci1
> > > > are required for bootloader operation on TI K3 AM69-SK EVM. These IPs
> > > > along with pinmuxes need to be marked for all bootloader phases, hence add
> > > > bootph-all to these nodes in kernel dts.
> > > > 
> > > > Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> > > > ---
> > > > [...]
> > > >    &wkup_uart0 {
> > > > +	bootph-all;
> > > >    	/* Firmware usage */
> > > >    	status = "reserved";
> > > >    	pinctrl-names = "default";
> > > I am not sure, if you want to treat wkup_uart in same way as you are
> > > treating secure_proxy_mcu in patch 1 of this series.
> > You should'nt. wkup_uart0 or what ever peripherals are specifically
> > board dependent. This patch does it the right way. I do have other
> > platforms on other K3 SoCs where the TIFS uart logs are actually
> > disabled.
> 
> Sorry, if i was not clear in my previous response.
> 
> This node is marked as reserved, adding bootph is not adding any value.
> 
> We can drop bootph from this node here.


Aah - yes. makes sense. Thanks for catching this. reserved nodes dont
make sense to have bootph in kernel dts. zephyr or u-boot r5 view will
probably enable them, and they should call it out.

> 
> > > IMO, where we are making this node status is okay, mark booth-all at that
> > > place only.
> > > 
> > > Otherwise for rest of series
> > > 
> > > LGTM
> > Do i take that as a Reviewed-by: for the series?
> 
> 
> With above change,
> 
> Reviewed-by: Udit Kumar <u-kumar1@ti.com>
> 
> > > 
> > [...]
> >
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-am69-sk.dts b/arch/arm64/boot/dts/ti/k3-am69-sk.dts
index d282c2c633c1..2302d55c3fe7 100644
--- a/arch/arm64/boot/dts/ti/k3-am69-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am69-sk.dts
@@ -110,7 +110,9 @@  vdd_sd_dv: regulator-tlv71033 {
 };
 
 &main_pmx0 {
+	bootph-all;
 	main_uart8_pins_default: main-uart8-default-pins {
+		bootph-all;
 		pinctrl-single,pins = <
 			J784S4_IOPAD(0x0d0, PIN_INPUT, 11) /* (AP38) SPI0_CS1.UART8_RXD */
 			J784S4_IOPAD(0x0d4, PIN_OUTPUT, 11) /* (AN38) SPI0_CLK.UART8_TXD */
@@ -125,6 +127,7 @@  J784S4_IOPAD(0x0e4, PIN_INPUT_PULLUP, 0) /* (AP37) I2C0_SDA */
 	};
 
 	main_mmc1_pins_default: main-mmc1-default-pins {
+		bootph-all;
 		pinctrl-single,pins = <
 			J784S4_IOPAD(0x104, PIN_INPUT, 0) /* (AB38) MMC1_CLK */
 			J784S4_IOPAD(0x108, PIN_INPUT, 0) /* (AB36) MMC1_CMD */
@@ -164,7 +167,9 @@  J784S4_IOPAD(0x004, PIN_INPUT, 7) /* (AG36) MCAN12_TX.GPIO0_1 */
 };
 
 &wkup_pmx2 {
+	bootph-all;
 	wkup_uart0_pins_default: wkup-uart0-default-pins {
+		bootph-all;
 		pinctrl-single,pins = <
 			J721S2_WKUP_IOPAD(0x070, PIN_INPUT, 0) /* (L37) WKUP_GPIO0_6.WKUP_UART0_CTSn */
 			J721S2_WKUP_IOPAD(0x074, PIN_INPUT, 0) /* (L36) WKUP_GPIO0_7.WKUP_UART0_RTSn */
@@ -174,6 +179,7 @@  J721S2_WKUP_IOPAD(0x04c, PIN_INPUT, 0) /* (K34) WKUP_UART0_TXD */
 	};
 
 	wkup_i2c0_pins_default: wkup-i2c0-default-pins {
+		bootph-all;
 		pinctrl-single,pins = <
 			J721S2_WKUP_IOPAD(0x98, PIN_INPUT, 0) /* (N33) WKUP_I2C0_SCL */
 			J721S2_WKUP_IOPAD(0x9c, PIN_INPUT, 0) /* (N35) WKUP_I2C0_SDA */
@@ -181,6 +187,7 @@  J721S2_WKUP_IOPAD(0x9c, PIN_INPUT, 0) /* (N35) WKUP_I2C0_SDA */
 	};
 
 	mcu_uart0_pins_default: mcu-uart0-default-pins {
+		bootph-all;
 		pinctrl-single,pins = <
 			J784S4_WKUP_IOPAD(0x08c, PIN_INPUT, 0) /* (K38) WKUP_GPIO0_13.MCU_UART0_RXD */
 			J784S4_WKUP_IOPAD(0x088, PIN_OUTPUT, 0) /* (J37) WKUP_GPIO0_12.MCU_UART0_TXD */
@@ -242,6 +249,7 @@  J784S4_WKUP_IOPAD(0x0, PIN_INPUT, 7) /* (M33) WKUP_GPIO0_49 */
 };
 
 &wkup_uart0 {
+	bootph-all;
 	/* Firmware usage */
 	status = "reserved";
 	pinctrl-names = "default";
@@ -249,6 +257,7 @@  &wkup_uart0 {
 };
 
 &wkup_i2c0 {
+	bootph-all;
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&wkup_i2c0_pins_default>;
@@ -268,6 +277,7 @@  &wkup_gpio0 {
 };
 
 &mcu_uart0 {
+	bootph-all;
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&mcu_uart0_pins_default>;
@@ -281,6 +291,7 @@  &mcu_i2c0 {
 };
 
 &main_uart8 {
+	bootph-all;
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&main_uart8_pins_default>;
@@ -307,6 +318,7 @@  exp1: gpio@21 {
 };
 
 &main_sdhci0 {
+	bootph-all;
 	/* eMMC */
 	status = "okay";
 	non-removable;
@@ -315,6 +327,7 @@  &main_sdhci0 {
 };
 
 &main_sdhci1 {
+	bootph-all;
 	/* SD card */
 	status = "okay";
 	pinctrl-0 = <&main_mmc1_pins_default>;