| Message ID | 20240226100807.1095607-1-quic_varada@quicinc.com |
|---|---|
| Headers | show |
| Series | Add initial support for IPQ9574 based boards | expand |
HI Varada, On 26/02/2024 10:07, Varadarajan Narayanan wrote: > These patches introduce the initial support code needed > for the QTI IPQ9574 SoC and RDP433 board. Awesome! > > SoC : QTI IPQ9574 > RAM : 2GB DDR4 > Flash : eMMC 8GB > WiFi : 1 x 2.4GHz, 1 x 5GHz, 1 x 6GHz > > New to both patman and posting to U-Boot. Please let > me know if something is not correct, will try to > rectify to the best of my abilities. Patches came through just fine, thanks! I haven't used patman personally, but I have used the b4 tool, I'd recommend giving it a spin if you haven't yet. Your get_maintainers.pl options might need some tweaking too (the defaults from b4 seem to work better ime). I've spent some time going through your series and there's a lot to go over, so apologies if this gets a bit long. I'm planning to pick up this[1] patch series this week and I'm sorry to say it has some fairly major conflicts with your work here. My series makes some fairly significant changes to Qualcomm support in U-Boot (moving to upstream Linux DT and getting rid of most of the compile time board configuration among other things). The thing I'm mostly concerned with is the totally custom init code and linker script. I discovered this branch[2] which seems to have a fairly complete git history, but there's still a lot of unknowns. * What is the custom linker script for (which isn't handled by the default one)? * Why do you skip U-Boot relocation (GD_FLG_SKIP_RELOC)? * How is U-Boot expected to be flashed and consequently loaded on this platform? I get the impression that SBL1 jumps to it directly? Is it built into an XBL image like on the mobile/IoT platforms, or flashed to it's own partition, or to SPI flash? In general there is very little context around the code being added here, and as a result it's hard to understand what exactly is going on and why things have been done a certain way. I'll leave some more detailed comments in line, but I think initially I would just like to get a better understanding of the situation here, so answers to the questions above would be most helpful! Please also have a look at reworking this series on top of my patches linked at [1], if you have any issues or questions then please do get in touch with me via email, or better via the #u-boot IRC channel on OFTC, my nick there is calebccff. Regarding my series, the notable changes are in how mach-snapdragon is handled: the target specific memory maps have been dropped in favour of reading the memory map from DT. The sysmap headers are also gone, instead the defines are moved to the relevant driver (mostly clock/pinctrl). I think it would be sensible to (for example) introduce mach-snapdragon/smem.c for handling stuff like your env_get_location() implementation (it would provide smem_env_get_location() which would be called from env_get_location() in board.c). You'll probably also want to skip the carve_out_reserved_memory() call for now. [1]: https://lore.kernel.org/u-boot/20240226-b4-qcom-common-target-v5-0-10c8e078befb@linaro.org/ [2]: https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot/-/tree/NHSS.QSDK.12.4.5 > > Thanks > Varada Kind regards, > > > Varadarajan Narayanan (10): > mach-snapdragon: Add support for IPQ9574 > mach-snapdragon: ipq9574: Add SMEM defines needed for IPQ9574 > clk/qcom: Add support for clock registers in IPQ9574 > clk/qcom: Include IPQ9574 > mach-snapdragon: Update gd->ram_size in msm_fixup_memory > pinctrl: qcom: Add support for 'pull-down' > pinctrl: qcom: Include IPQ9574 > mmc: msm_sdhci: Handle different vendor cap register offsets > mmc: msm_sdhci: Reset clocks before reconfiguration > board: qualcomm: Add support for IPQ9574 RDP433 > > 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 ++++++++++ > arch/arm/mach-snapdragon/Kconfig | 15 + > arch/arm/mach-snapdragon/Makefile | 1 + > arch/arm/mach-snapdragon/dram.c | 10 +- > .../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 +++ > configs/ipq9574_mmc_defconfig | 118 ++ > drivers/clk/qcom/Kconfig | 8 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/clock-ipq9574.c | 1320 +++++++++++++++++ > drivers/clk/qcom/clock-qcom.c | 32 + > drivers/clk/qcom/clock-qcom.h | 8 + > drivers/mmc/msm_sdhci.c | 21 +- > drivers/pinctrl/qcom/Kconfig | 7 + > drivers/pinctrl/qcom/Makefile | 1 + > drivers/pinctrl/qcom/pinctrl-ipq9574.c | 77 + > drivers/pinctrl/qcom/pinctrl-qcom.c | 2 + > 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 + > include/smem.h | 78 + > 33 files changed, 4563 insertions(+), 5 deletions(-) > 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 configs/ipq9574_mmc_defconfig > 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 >
On Tue, Feb 27, 2024 at 02:37:25AM +0000, Caleb Connolly wrote: > HI Varada, > > On 26/02/2024 10:07, Varadarajan Narayanan wrote: > > These patches introduce the initial support code needed > > for the QTI IPQ9574 SoC and RDP433 board. > > Awesome! Thanks for the feedback. Will rework based on the comments given in this and the other patches and will post a new spin soon. -Varada > > > > SoC : QTI IPQ9574 > > RAM : 2GB DDR4 > > Flash : eMMC 8GB > > WiFi : 1 x 2.4GHz, 1 x 5GHz, 1 x 6GHz > > > > New to both patman and posting to U-Boot. Please let > > me know if something is not correct, will try to > > rectify to the best of my abilities. > > Patches came through just fine, thanks! I haven't used patman > personally, but I have used the b4 tool, I'd recommend giving it a spin > if you haven't yet. Your get_maintainers.pl options might need some > tweaking too (the defaults from b4 seem to work better ime). > > I've spent some time going through your series and there's a lot to go > over, so apologies if this gets a bit long. > > I'm planning to pick up this[1] patch series this week and I'm sorry to > say it has some fairly major conflicts with your work here. My series > makes some fairly significant changes to Qualcomm support in U-Boot > (moving to upstream Linux DT and getting rid of most of the compile time > board configuration among other things). > > The thing I'm mostly concerned with is the totally custom init code and > linker script. I discovered this branch[2] which seems to have a fairly > complete git history, but there's still a lot of unknowns. > > * What is the custom linker script for (which isn't handled by the > default one)? > * Why do you skip U-Boot relocation (GD_FLG_SKIP_RELOC)? > * How is U-Boot expected to be flashed and consequently loaded on this > platform? I get the impression that SBL1 jumps to it directly? Is it > built into an XBL image like on the mobile/IoT platforms, or flashed to > it's own partition, or to SPI flash? > > In general there is very little context around the code being added > here, and as a result it's hard to understand what exactly is going on > and why things have been done a certain way. > > I'll leave some more detailed comments in line, but I think initially I > would just like to get a better understanding of the situation here, so > answers to the questions above would be most helpful! > > Please also have a look at reworking this series on top of my patches > linked at [1], if you have any issues or questions then please do get in > touch with me via email, or better via the #u-boot IRC channel on OFTC, > my nick there is calebccff. > > Regarding my series, the notable changes are in how mach-snapdragon is > handled: the target specific memory maps have been dropped in favour of > reading the memory map from DT. The sysmap headers are also gone, > instead the defines are moved to the relevant driver (mostly clock/pinctrl). > > I think it would be sensible to (for example) introduce > mach-snapdragon/smem.c for handling stuff like your env_get_location() > implementation (it would provide smem_env_get_location() which would be > called from env_get_location() in board.c). > > You'll probably also want to skip the carve_out_reserved_memory() call > for now. > > [1]: > https://lore.kernel.org/u-boot/20240226-b4-qcom-common-target-v5-0-10c8e078befb@linaro.org/ > [2]: > https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot/-/tree/NHSS.QSDK.12.4.5 > > > > Thanks > > Varada > > Kind regards, > > > > > > Varadarajan Narayanan (10): > > mach-snapdragon: Add support for IPQ9574 > > mach-snapdragon: ipq9574: Add SMEM defines needed for IPQ9574 > > clk/qcom: Add support for clock registers in IPQ9574 > > clk/qcom: Include IPQ9574 > > mach-snapdragon: Update gd->ram_size in msm_fixup_memory > > pinctrl: qcom: Add support for 'pull-down' > > pinctrl: qcom: Include IPQ9574 > > mmc: msm_sdhci: Handle different vendor cap register offsets > > mmc: msm_sdhci: Reset clocks before reconfiguration > > board: qualcomm: Add support for IPQ9574 RDP433 > > > > 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 ++++++++++ > > arch/arm/mach-snapdragon/Kconfig | 15 + > > arch/arm/mach-snapdragon/Makefile | 1 + > > arch/arm/mach-snapdragon/dram.c | 10 +- > > .../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 +++ > > configs/ipq9574_mmc_defconfig | 118 ++ > > drivers/clk/qcom/Kconfig | 8 + > > drivers/clk/qcom/Makefile | 1 + > > drivers/clk/qcom/clock-ipq9574.c | 1320 +++++++++++++++++ > > drivers/clk/qcom/clock-qcom.c | 32 + > > drivers/clk/qcom/clock-qcom.h | 8 + > > drivers/mmc/msm_sdhci.c | 21 +- > > drivers/pinctrl/qcom/Kconfig | 7 + > > drivers/pinctrl/qcom/Makefile | 1 + > > drivers/pinctrl/qcom/pinctrl-ipq9574.c | 77 + > > drivers/pinctrl/qcom/pinctrl-qcom.c | 2 + > > 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 + > > include/smem.h | 78 + > > 33 files changed, 4563 insertions(+), 5 deletions(-) > > 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 configs/ipq9574_mmc_defconfig > > 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 > > > > -- > // Caleb (they/them)