Re: [PATCH 01/10] mach-snapdragon: Add support for IPQ9574
Hi Krzysztof,
On Thu, 29 Feb 2024 at 22:50, Krzysztof Kozlowski wrote:
>
> On 26/02/2024 11:07, Varadarajan Narayanan wrote:
> > Signed-off-by: Varadarajan Narayanan
> > ---
> >
> > arch/arm/dts/Makefile |2 +
> > arch/arm/dts/ipq9574-default.dts | 167 +++
> > arch/arm/dts/ipq9574-rdp433-mht-phy.dts | 208 +++
> > arch/arm/dts/ipq9574.dtsi | 771 ++
> > .../include/mach/sysmap-ipq9574.h | 252
> > arch/arm/mach-snapdragon/init_ipq9574.c | 81 +
> > board/qualcomm/ipq9574/Kconfig| 15 +
> > board/qualcomm/ipq9574/Makefile |4 +
> > board/qualcomm/ipq9574/board_init.c | 326
> > board/qualcomm/ipq9574/ipq9574.c | 170 +++
> > board/qualcomm/ipq9574/ipq9574.h | 75 +
> > board/qualcomm/ipq9574/u-boot-x32.lds | 250
> > board/qualcomm/ipq9574/u-boot-x64.lds | 188 +++
> > drivers/clk/qcom/clock-ipq9574.c | 1320 +
> > drivers/pinctrl/qcom/pinctrl-ipq9574.c| 77 +
> > include/configs/ipq9574.h | 111 ++
> > include/dt-bindings/clock/gcc-ipq9574.h | 156 ++
> > include/dt-bindings/net/qti-ipqsoc.h | 20 +
> > include/dt-bindings/pinctrl/pinctrl-ipqsoc.h | 19 +
> > include/dt-bindings/reset/ipq9574-reset.h | 54 +
> > 20 files changed, 4266 insertions(+)
> > create mode 100644 arch/arm/dts/ipq9574-default.dts
> > create mode 100644 arch/arm/dts/ipq9574-rdp433-mht-phy.dts
> > create mode 100644 arch/arm/dts/ipq9574.dtsi
> > create mode 100644 arch/arm/mach-snapdragon/include/mach/sysmap-ipq9574.h
> > create mode 100644 arch/arm/mach-snapdragon/init_ipq9574.c
> > create mode 100644 board/qualcomm/ipq9574/Kconfig
> > create mode 100644 board/qualcomm/ipq9574/Makefile
> > create mode 100644 board/qualcomm/ipq9574/board_init.c
> > create mode 100644 board/qualcomm/ipq9574/ipq9574.c
> > create mode 100644 board/qualcomm/ipq9574/ipq9574.h
> > create mode 100644 board/qualcomm/ipq9574/u-boot-x32.lds
> > create mode 100644 board/qualcomm/ipq9574/u-boot-x64.lds
> > create mode 100644 drivers/clk/qcom/clock-ipq9574.c
> > create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq9574.c
> > create mode 100644 include/configs/ipq9574.h
> > create mode 100644 include/dt-bindings/clock/gcc-ipq9574.h
> > create mode 100644 include/dt-bindings/net/qti-ipqsoc.h
> > create mode 100644 include/dt-bindings/pinctrl/pinctrl-ipqsoc.h
> > create mode 100644 include/dt-bindings/reset/ipq9574-reset.h
> >
> > diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> > index d9725030d5..8931dfa2aa 100644
> > --- a/arch/arm/dts/Makefile
> > +++ b/arch/arm/dts/Makefile
> > @@ -1523,6 +1523,8 @@ dtb-$(CONFIG_ARCH_QEMU) += qemu-arm.dtb qemu-arm64.dtb
> > dtb-$(CONFIG_TARGET_CORSTONE1000) += corstone1000-mps3.dtb \
> > corstone1000-fvp.dtb
> >
> > +dtb-$(CONFIG_TARGET_IPQ9574) += ipq9574-rdp433-mht-phy.dtb
> > +
> > include $(srctree)/scripts/Makefile.dts
> >
> > targets += $(dtb-y)
> > diff --git a/arch/arm/dts/ipq9574-default.dts
> > b/arch/arm/dts/ipq9574-default.dts
> > new file mode 100644
> > index 00..501c9492df
> > --- /dev/null
> > +++ b/arch/arm/dts/ipq9574-default.dts
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights
> > reserved.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "ipq9574.dtsi"
> > +
> > +/ {
> > + config_name = "config-default";
> > +
> > + aliases {
> > + console = _uart2_console;
> > + uart2 = _uart3_additional;
> > + sdhci =
> > + };
> > +
> > + soc: soc {
> > + tlmm: pinctrl@100 {
> > +
> > + sdhci_pinmux: mmc {
> > + pinconfig;
> > + emmc_data {
>
> No, please use upstream DTS.
>
> You imported here a lot of vendor junk. There is no way this will pass
> any System Ready tests if you hand over this DTB to Linux. Plus really,
> that's ugly DTS to look at.
>
> I am not a maintainer of DTS in U-Boot, so up to the folks here, but I
> really recommend to NAK such DTS. It just re-adds all the issues we
> fixed in upstream kernel!
>
> I suggest using dts/upstream/qcom, but if you cannot then please import
> kernel DTS.
Yes, please. There's a lot of effort updating the dts file in u-boot
and using as much as we can verbatim from the upstream repos. Let's
not take any steps backwards
>
> Best regards,
> Krzysztof
>
Re: [PATCH 01/10] mach-snapdragon: Add support for IPQ9574
On 26/02/2024 11:07, Varadarajan Narayanan wrote:
> Signed-off-by: Varadarajan Narayanan
> ---
>
> arch/arm/dts/Makefile |2 +
> arch/arm/dts/ipq9574-default.dts | 167 +++
> arch/arm/dts/ipq9574-rdp433-mht-phy.dts | 208 +++
> arch/arm/dts/ipq9574.dtsi | 771 ++
> .../include/mach/sysmap-ipq9574.h | 252
> arch/arm/mach-snapdragon/init_ipq9574.c | 81 +
> board/qualcomm/ipq9574/Kconfig| 15 +
> board/qualcomm/ipq9574/Makefile |4 +
> board/qualcomm/ipq9574/board_init.c | 326
> board/qualcomm/ipq9574/ipq9574.c | 170 +++
> board/qualcomm/ipq9574/ipq9574.h | 75 +
> board/qualcomm/ipq9574/u-boot-x32.lds | 250
> board/qualcomm/ipq9574/u-boot-x64.lds | 188 +++
> drivers/clk/qcom/clock-ipq9574.c | 1320 +
> drivers/pinctrl/qcom/pinctrl-ipq9574.c| 77 +
> include/configs/ipq9574.h | 111 ++
> include/dt-bindings/clock/gcc-ipq9574.h | 156 ++
> include/dt-bindings/net/qti-ipqsoc.h | 20 +
> include/dt-bindings/pinctrl/pinctrl-ipqsoc.h | 19 +
> include/dt-bindings/reset/ipq9574-reset.h | 54 +
> 20 files changed, 4266 insertions(+)
> create mode 100644 arch/arm/dts/ipq9574-default.dts
> create mode 100644 arch/arm/dts/ipq9574-rdp433-mht-phy.dts
> create mode 100644 arch/arm/dts/ipq9574.dtsi
> create mode 100644 arch/arm/mach-snapdragon/include/mach/sysmap-ipq9574.h
> create mode 100644 arch/arm/mach-snapdragon/init_ipq9574.c
> create mode 100644 board/qualcomm/ipq9574/Kconfig
> create mode 100644 board/qualcomm/ipq9574/Makefile
> create mode 100644 board/qualcomm/ipq9574/board_init.c
> create mode 100644 board/qualcomm/ipq9574/ipq9574.c
> create mode 100644 board/qualcomm/ipq9574/ipq9574.h
> create mode 100644 board/qualcomm/ipq9574/u-boot-x32.lds
> create mode 100644 board/qualcomm/ipq9574/u-boot-x64.lds
> create mode 100644 drivers/clk/qcom/clock-ipq9574.c
> create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq9574.c
> create mode 100644 include/configs/ipq9574.h
> create mode 100644 include/dt-bindings/clock/gcc-ipq9574.h
> create mode 100644 include/dt-bindings/net/qti-ipqsoc.h
> create mode 100644 include/dt-bindings/pinctrl/pinctrl-ipqsoc.h
> create mode 100644 include/dt-bindings/reset/ipq9574-reset.h
>
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index d9725030d5..8931dfa2aa 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -1523,6 +1523,8 @@ dtb-$(CONFIG_ARCH_QEMU) += qemu-arm.dtb qemu-arm64.dtb
> dtb-$(CONFIG_TARGET_CORSTONE1000) += corstone1000-mps3.dtb \
> corstone1000-fvp.dtb
>
> +dtb-$(CONFIG_TARGET_IPQ9574) += ipq9574-rdp433-mht-phy.dtb
> +
> include $(srctree)/scripts/Makefile.dts
>
> targets += $(dtb-y)
> diff --git a/arch/arm/dts/ipq9574-default.dts
> b/arch/arm/dts/ipq9574-default.dts
> new file mode 100644
> index 00..501c9492df
> --- /dev/null
> +++ b/arch/arm/dts/ipq9574-default.dts
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include "ipq9574.dtsi"
> +
> +/ {
> + config_name = "config-default";
> +
> + aliases {
> + console = _uart2_console;
> + uart2 = _uart3_additional;
> + sdhci =
> + };
> +
> + soc: soc {
> + tlmm: pinctrl@100 {
> +
> + sdhci_pinmux: mmc {
> + pinconfig;
> + emmc_data {
No, please use upstream DTS.
You imported here a lot of vendor junk. There is no way this will pass
any System Ready tests if you hand over this DTB to Linux. Plus really,
that's ugly DTS to look at.
I am not a maintainer of DTS in U-Boot, so up to the folks here, but I
really recommend to NAK such DTS. It just re-adds all the issues we
fixed in upstream kernel!
I suggest using dts/upstream/qcom, but if you cannot then please import
kernel DTS.
Best regards,
Krzysztof
Re: [PATCH 01/10] mach-snapdragon: Add support for IPQ9574
On Mon, Feb 26, 2024 at 12:20:27PM +0200, Ilias Apalodimas wrote: > Hi, > > Please resend the patch with a proper description. I had given the description here with the 'Cover-letter:' tag and naively assumed it would be copied to cover letter. However, patman moved it to cover letter and this became a blank commit. Will fix it in the next spin. > Also, can you cc me on the linker script patches as well? > Why do you need a different linker script than the default? Sure. Thanks Varada > On Mon, 26 Feb 2024 at 12:09, Varadarajan Narayanan > wrote: > > > > Signed-off-by: Varadarajan Narayanan > > --- -- snip --
Re: [PATCH 01/10] mach-snapdragon: Add support for IPQ9574
Hi,
As Ilias said, please describe what this patch does and why, especially
things which are different to existing boards.
For the next revision, please split this patch up into something like
the following:
* clock driver
* pinctrl driver
* changes to mach-snapdragon
* board code / defconfig
* dt-bindings (which should be imported from Linux and mention the tag
they're taken from in the commit message)
* devicetree (also imported from Linux)
* U-Boot specific devicetree additions
This makes it much easier to review and generally individual components
and leaves us with a nicer git history at the end too.
Qualcomm platforms in U-Boot are switching over to using upstream
devicetree (from Linux), in the next revision you should also adopt
this. Both the SoC dtsi file and the board dts should be cut/paste from
Linux with no modifications. Any U-Boot specific additions must go in a
"ipq9574-rdp433-mht-phy-u-boot.dtsi" file, the build system will
automatically include it. If there are additional DT features that
aren't yet merged in upstream, those should be added in a separate
patch, ideally linking to an upstream patch series. This is part of a
wider effort to adopt upstream DT in U-Boot.
I've left some further comments and questions in-line below.
On 26/02/2024 10:07, Varadarajan Narayanan wrote:
> Signed-off-by: Varadarajan Narayanan
> ---
>
> arch/arm/dts/Makefile |2 +
> arch/arm/dts/ipq9574-default.dts | 167 +++
> arch/arm/dts/ipq9574-rdp433-mht-phy.dts | 208 +++
> arch/arm/dts/ipq9574.dtsi | 771 ++
> .../include/mach/sysmap-ipq9574.h | 252
> arch/arm/mach-snapdragon/init_ipq9574.c | 81 +
> board/qualcomm/ipq9574/Kconfig| 15 +
> board/qualcomm/ipq9574/Makefile |4 +
> board/qualcomm/ipq9574/board_init.c | 326
> board/qualcomm/ipq9574/ipq9574.c | 170 +++
> board/qualcomm/ipq9574/ipq9574.h | 75 +
> board/qualcomm/ipq9574/u-boot-x32.lds | 250
> board/qualcomm/ipq9574/u-boot-x64.lds | 188 +++
> drivers/clk/qcom/clock-ipq9574.c | 1320 +
> drivers/pinctrl/qcom/pinctrl-ipq9574.c| 77 +
> include/configs/ipq9574.h | 111 ++
> include/dt-bindings/clock/gcc-ipq9574.h | 156 ++
> include/dt-bindings/net/qti-ipqsoc.h | 20 +
> include/dt-bindings/pinctrl/pinctrl-ipqsoc.h | 19 +
> include/dt-bindings/reset/ipq9574-reset.h | 54 +
> 20 files changed, 4266 insertions(+)
> create mode 100644 arch/arm/dts/ipq9574-default.dts
> create mode 100644 arch/arm/dts/ipq9574-rdp433-mht-phy.dts
> create mode 100644 arch/arm/dts/ipq9574.dtsi
> create mode 100644 arch/arm/mach-snapdragon/include/mach/sysmap-ipq9574.h
> create mode 100644 arch/arm/mach-snapdragon/init_ipq9574.c
> create mode 100644 board/qualcomm/ipq9574/Kconfig
> create mode 100644 board/qualcomm/ipq9574/Makefile
> create mode 100644 board/qualcomm/ipq9574/board_init.c
> create mode 100644 board/qualcomm/ipq9574/ipq9574.c
> create mode 100644 board/qualcomm/ipq9574/ipq9574.h
> create mode 100644 board/qualcomm/ipq9574/u-boot-x32.lds
> create mode 100644 board/qualcomm/ipq9574/u-boot-x64.lds
> create mode 100644 drivers/clk/qcom/clock-ipq9574.c
> create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq9574.c
> create mode 100644 include/configs/ipq9574.h
> create mode 100644 include/dt-bindings/clock/gcc-ipq9574.h
> create mode 100644 include/dt-bindings/net/qti-ipqsoc.h
> create mode 100644 include/dt-bindings/pinctrl/pinctrl-ipqsoc.h
> create mode 100644 include/dt-bindings/reset/ipq9574-reset.h
>
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index d9725030d5..8931dfa2aa 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -1523,6 +1523,8 @@ dtb-$(CONFIG_ARCH_QEMU) += qemu-arm.dtb qemu-arm64.dtb
> dtb-$(CONFIG_TARGET_CORSTONE1000) += corstone1000-mps3.dtb \
> corstone1000-fvp.dtb
>
> +dtb-$(CONFIG_TARGET_IPQ9574) += ipq9574-rdp433-mht-phy.dtb
> +
> include $(srctree)/scripts/Makefile.dts
>
> targets += $(dtb-y)
> diff --git a/arch/arm/dts/ipq9574-default.dts
> b/arch/arm/dts/ipq9574-default.dts
> new file mode 100644
> index 00..501c9492df
> --- /dev/null
> +++ b/arch/arm/dts/ipq9574-default.dts
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/dts-v1/;
> +
> +#include "ipq9574.dtsi"
> +
> +/ {
> + config_name = "config-default";
> +
> + aliases {
> + console = _uart2_console;
> + uart2 = _uart3_additional;
> + sdhci =
> + };
> +
> + soc: soc {
> + tlmm: pinctrl@100 {
> +
> + sdhci_pinmux: mmc {
> + pinconfig;
> +
[PATCH 01/10] mach-snapdragon: Add support for IPQ9574
Signed-off-by: Varadarajan Narayanan
---
arch/arm/dts/Makefile |2 +
arch/arm/dts/ipq9574-default.dts | 167 +++
arch/arm/dts/ipq9574-rdp433-mht-phy.dts | 208 +++
arch/arm/dts/ipq9574.dtsi | 771 ++
.../include/mach/sysmap-ipq9574.h | 252
arch/arm/mach-snapdragon/init_ipq9574.c | 81 +
board/qualcomm/ipq9574/Kconfig| 15 +
board/qualcomm/ipq9574/Makefile |4 +
board/qualcomm/ipq9574/board_init.c | 326
board/qualcomm/ipq9574/ipq9574.c | 170 +++
board/qualcomm/ipq9574/ipq9574.h | 75 +
board/qualcomm/ipq9574/u-boot-x32.lds | 250
board/qualcomm/ipq9574/u-boot-x64.lds | 188 +++
drivers/clk/qcom/clock-ipq9574.c | 1320 +
drivers/pinctrl/qcom/pinctrl-ipq9574.c| 77 +
include/configs/ipq9574.h | 111 ++
include/dt-bindings/clock/gcc-ipq9574.h | 156 ++
include/dt-bindings/net/qti-ipqsoc.h | 20 +
include/dt-bindings/pinctrl/pinctrl-ipqsoc.h | 19 +
include/dt-bindings/reset/ipq9574-reset.h | 54 +
20 files changed, 4266 insertions(+)
create mode 100644 arch/arm/dts/ipq9574-default.dts
create mode 100644 arch/arm/dts/ipq9574-rdp433-mht-phy.dts
create mode 100644 arch/arm/dts/ipq9574.dtsi
create mode 100644 arch/arm/mach-snapdragon/include/mach/sysmap-ipq9574.h
create mode 100644 arch/arm/mach-snapdragon/init_ipq9574.c
create mode 100644 board/qualcomm/ipq9574/Kconfig
create mode 100644 board/qualcomm/ipq9574/Makefile
create mode 100644 board/qualcomm/ipq9574/board_init.c
create mode 100644 board/qualcomm/ipq9574/ipq9574.c
create mode 100644 board/qualcomm/ipq9574/ipq9574.h
create mode 100644 board/qualcomm/ipq9574/u-boot-x32.lds
create mode 100644 board/qualcomm/ipq9574/u-boot-x64.lds
create mode 100644 drivers/clk/qcom/clock-ipq9574.c
create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq9574.c
create mode 100644 include/configs/ipq9574.h
create mode 100644 include/dt-bindings/clock/gcc-ipq9574.h
create mode 100644 include/dt-bindings/net/qti-ipqsoc.h
create mode 100644 include/dt-bindings/pinctrl/pinctrl-ipqsoc.h
create mode 100644 include/dt-bindings/reset/ipq9574-reset.h
diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index d9725030d5..8931dfa2aa 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -1523,6 +1523,8 @@ dtb-$(CONFIG_ARCH_QEMU) += qemu-arm.dtb qemu-arm64.dtb
dtb-$(CONFIG_TARGET_CORSTONE1000) += corstone1000-mps3.dtb \
corstone1000-fvp.dtb
+dtb-$(CONFIG_TARGET_IPQ9574) += ipq9574-rdp433-mht-phy.dtb
+
include $(srctree)/scripts/Makefile.dts
targets += $(dtb-y)
diff --git a/arch/arm/dts/ipq9574-default.dts b/arch/arm/dts/ipq9574-default.dts
new file mode 100644
index 00..501c9492df
--- /dev/null
+++ b/arch/arm/dts/ipq9574-default.dts
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+/dts-v1/;
+
+#include "ipq9574.dtsi"
+
+/ {
+ config_name = "config-default";
+
+ aliases {
+ console = _uart2_console;
+ uart2 = _uart3_additional;
+ sdhci =
+ };
+
+ soc: soc {
+ tlmm: pinctrl@100 {
+
+ sdhci_pinmux: mmc {
+ pinconfig;
+ emmc_data {
+ pins = "GPIO_0", "GPIO_1", "GPIO_2",
+ "GPIO_3", "GPIO_6", "GPIO_7",
+ "GPIO_8", "GPIO_9";
+ function = "sdc";
+ drive-strength = ;
+ bias-pull-up;
+ };
+
+ emmc_cmd{
+ pins = "GPIO_4";
+ function = "sdc";
+ drive-strength = ;
+ bias-pull-up;
+ };
+
+ emmc_clk{
+ pins = "GPIO_5";
+ function = "sdc";
+ drive-strength = ;
+ bias-disable;
+ };
+
+ emmc_rclk{
+ pins = "GPIO_10";
+ function = "sdc";
+ drive-strength = ;
+ bias-pull-down;
+ };
+ };
+
+ qspi_pinmux: nand {
+
