[PATCH v1 2/4] arm: dts: k3-j721e: Sync with v6.5-rc1
Neha Malcom Francis
n-francis at ti.com
Thu Aug 31 10:23:42 CEST 2023
Hi Nishanth
On 28/08/23 22:59, Nishanth Menon wrote:
> On 17:01-20230828, Neha Malcom Francis wrote:
>> Sync k3-j721e DTS with kernel.org v6.5-rc1.
>> * pcie_epx nodes have been deleted, they are not needed [1]
>> * use mcu_timer0 instead of the redundant timer1. Also delete
>> clock and power domain properties since J721E follows legacy
>> boot flow where system firmware is not yet available to modify
>> clocks
>> * remove mcu_i2c0 as it is used for tps659413 PMIC in j721e-sk
>> for which support is not yet added.
>> * remove main_i2c2 pinmux for j721e-sk as it is connected to the
>> test automation header for which support is not there
>> * change mcu_secproxy to secure_proxy_mcu since linux has
>> defined these nodes
>> * hbmc nodes have been disabled as their support in kernel
>> itself is unfinished (dependent series not merged) [2]
>> * Drop mcu_cpsw node in U-Boot as it is no longer needed here
>> thanks to [3]
>
> Just use spaces.
>
> You do not need to provide changelog to all the u-boot changes in the commit
> message - use the diffstat for that. if you are referring to topics such
> as those use commit id style.
>
> [...]
>
>> diff --git a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
>> index 540c847eb3..6a95ed2a96 100644
>> --- a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
>> @@ -8,12 +8,10 @@
>>
>> / {
>> chosen {
>> - stdout-path = "serial2:115200n8";
>> - tick-timer = &timer1;
>> + tick-timer = &mcu_timer0;
>
> Looking at Manorit's series, I realized that tick-timer property is only
> needed for R5 dts.
>
>> };
>>
>> aliases {
>> - ethernet0 = &cpsw_port1;
>> spi0 = &ospi0;
>> spi1 = &ospi1;
> You don't need any of these. nor do you need i2c aliases etc. all those
> come from the board.dts.
>
>> remoteproc0 = &mcu_r5fss0_core0;
>
> remoteproc aliases is probably needed to remain sane - upstream kernel
> maintainer hasn't agreed[1] to pick up aliases so far.
>
>> @@ -34,52 +32,44 @@
>>
>> &cbass_main{
>
> watch for that space after nodename &cbass_main { instead of &cbass_main{
>
>> bootph-pre-ram;
> [...]
>
>> assigned-clock-parents = <&k3_clks 295 0>, <&k3_clks 295 9>;
>
> Why do we still have this for &wiz3_pll1_refclk and &wiz3_pll1_refclk
> &serdes0_pcie_link ?
>
> Also why are we dropping assigned-clocks and assigned-clock-parents in
> &serdes0? And these dont even have a bootph or u-boot, property - so
> what is the point in invoking them if they are'nt getting set in early
> stages of boot. if this is a valid clocking bug, fix should have gone to
> kernel - and document it as such.
>
> [...]
>
> I will comment about dr_mode and usb
>
> There is a bit of a debate ongoing on this.
> https://lore.kernel.org/u-boot/20230706-handle-otg-as-periph-v3-0-27e24fa17345@baylibre.com/
>
> But I don't see why we need to hold off for that resolution.
>
>> &main_mmc1_pins_default {
>> bootph-pre-ram;
>> };
>> @@ -169,8 +158,14 @@
>> bootph-pre-ram;
>> };
>>
>> +&wkup_uart0 {
>> + bootph-pre-ram;
>> + status = "okay";
>> +};
>> +
>> &wkup_i2c0 {
>> bootph-pre-ram;
>> + status = "okay";
>> };
>>
>> &main_i2c0 {
>> @@ -181,6 +176,10 @@
>> bootph-pre-ram;
>> };
>>
>> +&main_esm{
>
> Watch out for that space before {.
>
>> + bootph-pre-ram;
>> +};
>> +
>> &exp2 {
>> bootph-pre-ram;
>> };
>> @@ -194,6 +193,7 @@
>> };
>>
>> &hbmc {
>> + status = "disabled";
>> bootph-pre-ram;
>>
>> flash at 0,0 {
>
> Just drop the flash information etc in the node and mark the node as disabled.
>
>> @@ -268,7 +268,38 @@
>> assigned-clock-parents = <&wiz0_pll1_refclk>;
>> };
>>
>> -&serdes0_qsgmii_link {
>> - assigned-clocks = <&serdes0 CDNS_SIERRA_PLL_CMNLC1>;
>> - assigned-clock-parents = <&wiz0_pll1_refclk>;
>> +&main_crypto {
>> + dma-coherent;
>> +};
>
> Why?
>
>> +
>> +&rng {
>> + clocks = <&k3_clks 264 1>;
>> +};
>
> We don't even use rng in u-boot.
>
>> +
>> +&main_i2c2 {
>> + status = "okay";
>> +};
>> +
>> +&main_i2c4 {
>> + status = "okay";
>> +};
>> +
>> +&main_i2c5 {
>> + status = "okay";
>> +};
>
> You need all of these in u-boot? They dont even have bootph properties.
> nothing without bootph OR u-boot, properties should even be present in
> u-boot.dtsi
>
>> +
>> +&wkup_uart0 {
>> + status = "okay";
>> +};
>> +
>> +&mcu_i2c0 {
>> + status = "okay";
>> +};
>> +
>> +&mcu_i2c1 {
>> + status = "okay";
>> +};
>> +
>> +&dss {
>> + status = "disabled";
>> };
>
> Why not just leave things as is from kernel dts?
>
> [...]
>
>> diff --git a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
>> index 7bb5ce775c..da8cf7f37b 100644
>> --- a/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
>> +++ b/arch/arm/dts/k3-j721e-r5-common-proc-board.dts
>
> [...]
>>
>> &mcu_uart0 {
>> /delete-property/ power-domains;
>> /delete-property/ clocks;
>> /delete-property/ clock-names;
>
> NAK. no explanation why we have to delete these properties. and why not
> we add data to handle it if it is mandatory.
>
>> - pinctrl-names = "default";
>> - pinctrl-0 = <&mcu_uart0_pins_default>;
>> - status = "okay";
>> clock-frequency = <48000000>;
>> };
>>
>> -&main_uart0 {
>> - pinctrl-names = "default";
>> - pinctrl-0 = <&main_uart0_pins_default>;
>> - status = "okay";
>> - power-domains = <&k3_pds 146 TI_SCI_PD_SHARED>;
>> -};
>> -
>> &main_sdhci0 {
>> /delete-property/ power-domains;
>> /delete-property/ assigned-clocks;
>> /delete-property/ assigned-clock-parents;
>
> NAK. no explanation why we have to delete these properties. and why not
> we add data to handle it if it is mandatory.
>
>> clock-names = "clk_xin";
>> clocks = <&clk_200mhz>;
>
> NAK again. please explain why.
>
>> - ti,driver-strength-ohm = <50>;
>
> Will this even work?
>
>> - non-removable;
>> bus-width = <8>;
>> };
> [.. skipping the rest.. since pattern of comments seem to be repeated..]
>
> [1] https://lore.kernel.org/lkml/20230822215042.yjaqtwhuhls57pbu@glamour/T/
Thanks for all the review comments, will address these and post v2.
--
Thanking You
Neha Malcom Francis
More information about the U-Boot
mailing list