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-27e24fa17...@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@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/
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5
849D 1736 249D
