[PATCH 1/7] mmc: msm_sdhci: Match clocks through "clocks" property
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
drivers/mmc/msm_sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c index 604f9c3ff99c..174435f01f68 100644 --- a/drivers/mmc/msm_sdhci.c +++ b/drivers/mmc/msm_sdhci.c @@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev) struct clk clk; int ret;
- ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2); + ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2); if (ret) return ret;
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
arch/arm/dts/qcom-ipq4019.dtsi | 2 +- drivers/serial/serial_msm.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi index 6edc69da6747..dee3159e5893 100644 --- a/arch/arm/dts/qcom-ipq4019.dtsi +++ b/arch/arm/dts/qcom-ipq4019.dtsi @@ -87,7 +87,7 @@ blsp1_uart1: serial@78af000 { compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm"; reg = <0x78af000 0x200>; - clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>; + clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>; bit-rate = <0xFF>; status = "disabled"; u-boot,dm-pre-reloc; diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index a22623c316ed..9c370cac323f 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev) struct clk clk; int ret;
- ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock", - clkd, 2); + ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2); if (ret) return ret;
On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio konrad.dybcio@linaro.org wrote:
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/dts/qcom-ipq4019.dtsi | 2 +- drivers/serial/serial_msm.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi index 6edc69da6747..dee3159e5893 100644 --- a/arch/arm/dts/qcom-ipq4019.dtsi +++ b/arch/arm/dts/qcom-ipq4019.dtsi @@ -87,7 +87,7 @@ blsp1_uart1: serial@78af000 { compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm"; reg = <0x78af000 0x200>;
clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
You have to update other platform dts files as well to avoid breaking them.
-Sumit
bit-rate = <0xFF>; status = "disabled"; u-boot,dm-pre-reloc;diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index a22623c316ed..9c370cac323f 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev) struct clk clk; int ret;
ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock",clkd, 2);
ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2); if (ret) return ret;-- 2.40.0
On 24.03.2023 07:00, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio konrad.dybcio@linaro.org wrote:
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/dts/qcom-ipq4019.dtsi | 2 +- drivers/serial/serial_msm.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi index 6edc69da6747..dee3159e5893 100644 --- a/arch/arm/dts/qcom-ipq4019.dtsi +++ b/arch/arm/dts/qcom-ipq4019.dtsi @@ -87,7 +87,7 @@ blsp1_uart1: serial@78af000 { compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm"; reg = <0x78af000 0x200>;
clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>;You have to update other platform dts files as well to avoid breaking them.
Right, my grep pattern didn't include dragonboard* (which I should probably update to include the SoC name too, anyway..).
Konrad
-Sumit
bit-rate = <0xFF>; status = "disabled"; u-boot,dm-pre-reloc;diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index a22623c316ed..9c370cac323f 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev) struct clk clk; int ret;
ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock",clkd, 2);
ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2); if (ret) return ret;-- 2.40.0
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
drivers/serial/serial_msm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index 9c370cac323f..59a2cf27aaf1 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = { .priv_auto = sizeof(struct msm_serial_data), .probe = msm_serial_probe, .ops = &msm_serial_ops, + .flags = DM_FLAG_PRE_RELOC, };
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
drivers/serial/serial_msm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index 9c370cac323f..59a2cf27aaf1 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = { .priv_auto = sizeof(struct msm_serial_data), .probe = msm_serial_probe, .ops = &msm_serial_ops,
.flags = DM_FLAG_PRE_RELOC,
"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
-Sumit
};
2.40.0
On 24.03.2023 07:04, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
drivers/serial/serial_msm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index 9c370cac323f..59a2cf27aaf1 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = { .priv_auto = sizeof(struct msm_serial_data), .probe = msm_serial_probe, .ops = &msm_serial_ops,
.flags = DM_FLAG_PRE_RELOC,"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation.
Yep, this way you can take a dtb you built with Linux and use u-boot with that!
So you need to get rid of redundant
"u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
Will do!
Konrad
-Sumit
};
2.40.0
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
arch/arm/mach-snapdragon/clock-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c index 0ac45dce9a92..d1af5d1fec7d 100644 --- a/arch/arm/mach-snapdragon/clock-snapdragon.c +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = { .ops = &msm_clk_ops, .priv_auto = sizeof(struct msm_clk_priv), .probe = msm_clk_probe, + .flags = DM_FLAG_PRE_RELOC, };
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/mach-snapdragon/clock-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c index 0ac45dce9a92..d1af5d1fec7d 100644 --- a/arch/arm/mach-snapdragon/clock-snapdragon.c +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = { .ops = &msm_clk_ops, .priv_auto = sizeof(struct msm_clk_priv), .probe = msm_clk_probe,
.flags = DM_FLAG_PRE_RELOC,
"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
-Sumit
};
2.40.0
On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/mach-snapdragon/clock-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c index 0ac45dce9a92..d1af5d1fec7d 100644 --- a/arch/arm/mach-snapdragon/clock-snapdragon.c +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = { .ops = &msm_clk_ops, .priv_auto = sizeof(struct msm_clk_priv), .probe = msm_clk_probe,
.flags = DM_FLAG_PRE_RELOC,"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
And in turn u-boot,dm-* are now bootph-* flags which can (should!) be part of the upstream dts file.
On Fri, 24 Mar 2023 at 20:54, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/mach-snapdragon/clock-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c index 0ac45dce9a92..d1af5d1fec7d 100644 --- a/arch/arm/mach-snapdragon/clock-snapdragon.c +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = { .ops = &msm_clk_ops, .priv_auto = sizeof(struct msm_clk_priv), .probe = msm_clk_probe,
.flags = DM_FLAG_PRE_RELOC,"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
And in turn u-boot,dm-* are now bootph-* flags which can (should!) be part of the upstream dts file.
That's good to know. Can you share a reference to the patch set adding support for bootph-* flags?
-Sumit
-- Tom
On Mon, Mar 27, 2023 at 11:07:06AM +0530, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 20:54, Tom Rini trini@konsulko.com wrote:
On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/mach-snapdragon/clock-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c index 0ac45dce9a92..d1af5d1fec7d 100644 --- a/arch/arm/mach-snapdragon/clock-snapdragon.c +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = { .ops = &msm_clk_ops, .priv_auto = sizeof(struct msm_clk_priv), .probe = msm_clk_probe,
.flags = DM_FLAG_PRE_RELOC,"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
And in turn u-boot,dm-* are now bootph-* flags which can (should!) be part of the upstream dts file.
That's good to know. Can you share a reference to the patch set adding support for bootph-* flags?
https://github.com/devicetree-org/dt-schema/commit/63bd8472dfc6951eae1c95b27...
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
arch/arm/mach-snapdragon/pinctrl-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c index 826dc5148661..9f261d70e4d3 100644 --- a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c +++ b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c @@ -163,4 +163,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { .ops = &msm_pinctrl_ops, .probe = msm_pinctrl_probe, .bind = msm_pinctrl_bind, + .flags = DM_FLAG_PRE_RELOC, };
On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio konrad.dybcio@linaro.org wrote:
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/mach-snapdragon/pinctrl-snapdragon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c index 826dc5148661..9f261d70e4d3 100644 --- a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c +++ b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c @@ -163,4 +163,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { .ops = &msm_pinctrl_ops, .probe = msm_pinctrl_probe, .bind = msm_pinctrl_bind,
.flags = DM_FLAG_PRE_RELOC,
"u-boot,dm-pre-reloc" serves the same purpose but this looks even better as we would like the serial driver to be enabled by default prior to relocation. So you need to get rid of redundant "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
-Sumit
};
2.40.0
The name "se" is used in upstream Linux device trees and has been for ages, long before this U-Boot-ism was introduced. Same goes for the existing compatible. Get rid of that.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
arch/arm/dts/sdm845.dtsi | 4 ++-- drivers/serial/serial_msm_geni.c | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi index 607af277f8be..92bdc82177d6 100644 --- a/arch/arm/dts/sdm845.dtsi +++ b/arch/arm/dts/sdm845.dtsi @@ -52,10 +52,10 @@ };
debug_uart: serial@a84000 { - compatible = "qcom,msm-geni-uart"; + compatible = "qcom,geni-debug-uart"; reg = <0xa84000 0x4000>; reg-names = "se_phys"; - clock-names = "se-clk"; + clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; pinctrl-names = "default"; pinctrl-0 = <&qup_uart9>; diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 3943ca43e49e..6c9371c4e69d 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate) struct clk *clk; int ret;
- clk = devm_clk_get(dev, "se-clk"); + clk = devm_clk_get(dev, "se"); if (!clk) return -EINVAL;
@@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev) }
static const struct udevice_id msm_serial_ids[] = { - {.compatible = "qcom,msm-geni-uart"}, {}}; + { .compatible = "qcom,geni-debug-uart" }, + { } +};
U_BOOT_DRIVER(serial_msm_geni) = { .name = "serial_msm_geni",
On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio konrad.dybcio@linaro.org wrote:
The name "se" is used in upstream Linux device trees and has been for ages, long before this U-Boot-ism was introduced. Same goes for the existing compatible. Get rid of that.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/dts/sdm845.dtsi | 4 ++-- drivers/serial/serial_msm_geni.c | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi index 607af277f8be..92bdc82177d6 100644 --- a/arch/arm/dts/sdm845.dtsi +++ b/arch/arm/dts/sdm845.dtsi @@ -52,10 +52,10 @@ };
debug_uart: serial@a84000 {
compatible = "qcom,msm-geni-uart";
compatible = "qcom,geni-debug-uart";
With this the custom u-boot DT bindings [1] becomes redundant, we should remove that as well.
[1] doc/device-tree-bindings/serial/msm-geni-serial.txt
-Sumit
reg = <0xa84000 0x4000>; reg-names = "se_phys";
clock-names = "se-clk";
clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; pinctrl-names = "default"; pinctrl-0 = <&qup_uart9>;diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 3943ca43e49e..6c9371c4e69d 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate) struct clk *clk; int ret;
clk = devm_clk_get(dev, "se-clk");
clk = devm_clk_get(dev, "se"); if (!clk) return -EINVAL;@@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev) }
static const struct udevice_id msm_serial_ids[] = {
{.compatible = "qcom,msm-geni-uart"}, {}};
{ .compatible = "qcom,geni-debug-uart" },{ }+};
U_BOOT_DRIVER(serial_msm_geni) = { .name = "serial_msm_geni", -- 2.40.0
On 24.03.2023 07:13, Sumit Garg wrote:
On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio konrad.dybcio@linaro.org wrote:
The name "se" is used in upstream Linux device trees and has been for ages, long before this U-Boot-ism was introduced. Same goes for the existing compatible. Get rid of that.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/dts/sdm845.dtsi | 4 ++-- drivers/serial/serial_msm_geni.c | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi index 607af277f8be..92bdc82177d6 100644 --- a/arch/arm/dts/sdm845.dtsi +++ b/arch/arm/dts/sdm845.dtsi @@ -52,10 +52,10 @@ };
debug_uart: serial@a84000 {
compatible = "qcom,msm-geni-uart";
compatible = "qcom,geni-debug-uart";With this the custom u-boot DT bindings [1] becomes redundant, we should remove that as well.
[1] doc/device-tree-bindings/serial/msm-geni-serial.txt
Ack!
Konrad
-Sumit
reg = <0xa84000 0x4000>; reg-names = "se_phys";
clock-names = "se-clk";
clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; pinctrl-names = "default"; pinctrl-0 = <&qup_uart9>;diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 3943ca43e49e..6c9371c4e69d 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate) struct clk *clk; int ret;
clk = devm_clk_get(dev, "se-clk");
clk = devm_clk_get(dev, "se"); if (!clk) return -EINVAL;@@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev) }
static const struct udevice_id msm_serial_ids[] = {
{.compatible = "qcom,msm-geni-uart"}, {}};
{ .compatible = "qcom,geni-debug-uart" },{ }+};
U_BOOT_DRIVER(serial_msm_geni) = { .name = "serial_msm_geni", -- 2.40.0
Mark's and Dzmitry's approaches come down to the same thing.. Let's unify them by first removing the static keyword from the common file to allow the variable to be reused, then renaming "reg0" to the more sensible fw_dtb_pointer coming from the Apple file and finally remove the mach-apple implementation of this very thing and enable the common approach in the respective defconfig.
Only build-tested.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org ---
arch/arm/lib/save_prev_bl_data.c | 14 +++++++------- arch/arm/mach-apple/Makefile | 1 - arch/arm/mach-apple/lowlevel_init.S | 17 ----------------- configs/apple_m1_defconfig | 1 + 4 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c index f7b23faf0d66..a127fec1f149 100644 --- a/arch/arm/lib/save_prev_bl_data.c +++ b/arch/arm/lib/save_prev_bl_data.c @@ -15,7 +15,7 @@ #include <asm/system.h> #include <asm/armv8/mmu.h>
-static ulong reg0 __section(".data"); +ulong fw_dtb_pointer __section(".data");
/** * Save x0 register value, assuming previous bootloader set it to @@ -23,7 +23,7 @@ static ulong reg0 __section(".data"); */ void save_boot_params(ulong r0) { - reg0 = r0; + fw_dtb_pointer = r0; save_boot_params_ret(); }
@@ -51,24 +51,24 @@ int save_prev_bl_data(void) int node; u64 initrd_start_prop;
- if (!is_addr_accessible((phys_addr_t)reg0)) + if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer)) return -ENODATA;
- fdt_blob = (struct fdt_header *)reg0; + fdt_blob = (struct fdt_header *)fw_dtb_pointer; if (!fdt_valid(&fdt_blob)) { - pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0); + pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer); return -ENODATA; }
if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR)) - env_set_addr("prevbl_fdt_addr", (void *)reg0); + env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer); if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR)) return 0;
node = fdt_path_offset(fdt_blob, "/chosen"); if (!node) { pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n", - __func__, reg0); + __func__, fw_dtb_pointer); return -ENODATA; } /* diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile index 50b465b9473f..a775d8866ad4 100644 --- a/arch/arm/mach-apple/Makefile +++ b/arch/arm/mach-apple/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0+
obj-y += board.o -obj-y += lowlevel_init.o obj-y += rtkit.o obj-$(CONFIG_NVME_APPLE) += sart.o diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S deleted file mode 100644 index e1c0d91cef2c..000000000000 --- a/arch/arm/mach-apple/lowlevel_init.S +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/* - * (C) Copyright 2021 Mark Kettenis kettenis@openbsd.org - */ - -.align 8 -.global fw_dtb_pointer -fw_dtb_pointer: - .quad 0 - -.global save_boot_params -save_boot_params: - /* Stash DTB pointer passed by m1n1 */ - adr x1, fw_dtb_pointer - str x0, [x1] - - b save_boot_params_ret diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig index b4ecf73cbc78..eb0addb741c5 100644 --- a/configs/apple_m1_defconfig +++ b/configs/apple_m1_defconfig @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y CONFIG_DEFAULT_DEVICE_TREE="t8103-j274" CONFIG_SYS_LOAD_ADDR=0x0 CONFIG_USE_PREBOOT=y +CONFIG_SAVE_PREV_BL_FDT_ADDR=y # CONFIG_DISPLAY_CPUINFO is not set CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_BOARD_LATE_INIT=y
From: Konrad Dybcio konrad.dybcio@linaro.org Date: Fri, 24 Mar 2023 01:40:40 +0100
Hi Konrad,
Mark's and Dzmitry's approaches come down to the same thing.. Let's unify them by first removing the static keyword from the common file to allow the variable to be reused, then renaming "reg0" to the more sensible fw_dtb_pointer coming from the Apple file and finally remove the mach-apple implementation of this very thing and enable the common approach in the respective defconfig.
Only build-tested.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/lib/save_prev_bl_data.c | 14 +++++++------- arch/arm/mach-apple/Makefile | 1 - arch/arm/mach-apple/lowlevel_init.S | 17 ----------------- configs/apple_m1_defconfig | 1 + 4 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c index f7b23faf0d66..a127fec1f149 100644 --- a/arch/arm/lib/save_prev_bl_data.c +++ b/arch/arm/lib/save_prev_bl_data.c @@ -15,7 +15,7 @@ #include <asm/system.h> #include <asm/armv8/mmu.h>
-static ulong reg0 __section(".data"); +ulong fw_dtb_pointer __section(".data");
/**
- Save x0 register value, assuming previous bootloader set it to
@@ -23,7 +23,7 @@ static ulong reg0 __section(".data"); */ void save_boot_params(ulong r0) {
- reg0 = r0;
- fw_dtb_pointer = r0; save_boot_params_ret();
I suppose this works, but it is a bit strange sice save_boot_params_ret is just a label that we're supposed to jump back to, not a function.
}
@@ -51,24 +51,24 @@ int save_prev_bl_data(void) int node; u64 initrd_start_prop;
- if (!is_addr_accessible((phys_addr_t)reg0))
- if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer)) return -ENODATA;
- fdt_blob = (struct fdt_header *)reg0;
- fdt_blob = (struct fdt_header *)fw_dtb_pointer; if (!fdt_valid(&fdt_blob)) {
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);return -ENODATA; }
if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
env_set_addr("prevbl_fdt_addr", (void *)reg0);
env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR)) return 0;
node = fdt_path_offset(fdt_blob, "/chosen"); if (!node) { pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
__func__, reg0);
__func__, fw_dtb_pointer);
And we have no use for these additional environment variables and I'd prefer not to add them.
diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile index 50b465b9473f..a775d8866ad4 100644 --- a/arch/arm/mach-apple/Makefile +++ b/arch/arm/mach-apple/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0+
obj-y += board.o -obj-y += lowlevel_init.o obj-y += rtkit.o obj-$(CONFIG_NVME_APPLE) += sart.o diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S deleted file mode 100644 index e1c0d91cef2c..000000000000 --- a/arch/arm/mach-apple/lowlevel_init.S +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*
- (C) Copyright 2021 Mark Kettenis kettenis@openbsd.org
- */
-.align 8 -.global fw_dtb_pointer -fw_dtb_pointer:
- .quad 0
-.global save_boot_params -save_boot_params:
- /* Stash DTB pointer passed by m1n1 */
- adr x1, fw_dtb_pointer
- str x0, [x1]
- b save_boot_params_ret
diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig index b4ecf73cbc78..eb0addb741c5 100644 --- a/configs/apple_m1_defconfig +++ b/configs/apple_m1_defconfig @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y CONFIG_DEFAULT_DEVICE_TREE="t8103-j274" CONFIG_SYS_LOAD_ADDR=0x0 CONFIG_USE_PREBOOT=y +CONFIG_SAVE_PREV_BL_FDT_ADDR=y # CONFIG_DISPLAY_CPUINFO is not set CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_BOARD_LATE_INIT=y -- 2.40.0
On 27.03.2023 21:31, Mark Kettenis wrote:
From: Konrad Dybcio konrad.dybcio@linaro.org Date: Fri, 24 Mar 2023 01:40:40 +0100
Hi Konrad,
Mark's and Dzmitry's approaches come down to the same thing.. Let's unify them by first removing the static keyword from the common file to allow the variable to be reused, then renaming "reg0" to the more sensible fw_dtb_pointer coming from the Apple file and finally remove the mach-apple implementation of this very thing and enable the common approach in the respective defconfig.
Only build-tested.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/lib/save_prev_bl_data.c | 14 +++++++------- arch/arm/mach-apple/Makefile | 1 - arch/arm/mach-apple/lowlevel_init.S | 17 ----------------- configs/apple_m1_defconfig | 1 + 4 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c index f7b23faf0d66..a127fec1f149 100644 --- a/arch/arm/lib/save_prev_bl_data.c +++ b/arch/arm/lib/save_prev_bl_data.c @@ -15,7 +15,7 @@ #include <asm/system.h> #include <asm/armv8/mmu.h>
-static ulong reg0 __section(".data"); +ulong fw_dtb_pointer __section(".data");
/**
- Save x0 register value, assuming previous bootloader set it to
@@ -23,7 +23,7 @@ static ulong reg0 __section(".data"); */ void save_boot_params(ulong r0) {
- reg0 = r0;
- fw_dtb_pointer = r0; save_boot_params_ret();
I suppose this works, but it is a bit strange sice save_boot_params_ret is just a label that we're supposed to jump back to, not a function.
}
@@ -51,24 +51,24 @@ int save_prev_bl_data(void) int node; u64 initrd_start_prop;
- if (!is_addr_accessible((phys_addr_t)reg0))
- if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer)) return -ENODATA;
- fdt_blob = (struct fdt_header *)reg0;
- fdt_blob = (struct fdt_header *)fw_dtb_pointer; if (!fdt_valid(&fdt_blob)) {
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);return -ENODATA; }
if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
env_set_addr("prevbl_fdt_addr", (void *)reg0);
env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR)) return 0;
node = fdt_path_offset(fdt_blob, "/chosen"); if (!node) { pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
__func__, reg0);
__func__, fw_dtb_pointer);And we have no use for these additional environment variables and I'd prefer not to add them.
Okay, let's forget about it then.
Would you (or anybody responsible, I don't know who picks things up) be willing to take this patch without the Apple part then, a.k.a. renaming r0 to fw_dtb_pointer?
Konrad
diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile index 50b465b9473f..a775d8866ad4 100644 --- a/arch/arm/mach-apple/Makefile +++ b/arch/arm/mach-apple/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0+
obj-y += board.o -obj-y += lowlevel_init.o obj-y += rtkit.o obj-$(CONFIG_NVME_APPLE) += sart.o diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S deleted file mode 100644 index e1c0d91cef2c..000000000000 --- a/arch/arm/mach-apple/lowlevel_init.S +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*
- (C) Copyright 2021 Mark Kettenis kettenis@openbsd.org
- */
-.align 8 -.global fw_dtb_pointer -fw_dtb_pointer:
- .quad 0
-.global save_boot_params -save_boot_params:
- /* Stash DTB pointer passed by m1n1 */
- adr x1, fw_dtb_pointer
- str x0, [x1]
- b save_boot_params_ret
diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig index b4ecf73cbc78..eb0addb741c5 100644 --- a/configs/apple_m1_defconfig +++ b/configs/apple_m1_defconfig @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y CONFIG_DEFAULT_DEVICE_TREE="t8103-j274" CONFIG_SYS_LOAD_ADDR=0x0 CONFIG_USE_PREBOOT=y +CONFIG_SAVE_PREV_BL_FDT_ADDR=y # CONFIG_DISPLAY_CPUINFO is not set CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_BOARD_LATE_INIT=y -- 2.40.0
On Tue, Mar 28, 2023 at 10:17:44AM +0200, Konrad Dybcio wrote:
On 27.03.2023 21:31, Mark Kettenis wrote:
From: Konrad Dybcio konrad.dybcio@linaro.org Date: Fri, 24 Mar 2023 01:40:40 +0100
Hi Konrad,
Mark's and Dzmitry's approaches come down to the same thing.. Let's unify them by first removing the static keyword from the common file to allow the variable to be reused, then renaming "reg0" to the more sensible fw_dtb_pointer coming from the Apple file and finally remove the mach-apple implementation of this very thing and enable the common approach in the respective defconfig.
Only build-tested.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
arch/arm/lib/save_prev_bl_data.c | 14 +++++++------- arch/arm/mach-apple/Makefile | 1 - arch/arm/mach-apple/lowlevel_init.S | 17 ----------------- configs/apple_m1_defconfig | 1 + 4 files changed, 8 insertions(+), 25 deletions(-) delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c index f7b23faf0d66..a127fec1f149 100644 --- a/arch/arm/lib/save_prev_bl_data.c +++ b/arch/arm/lib/save_prev_bl_data.c @@ -15,7 +15,7 @@ #include <asm/system.h> #include <asm/armv8/mmu.h>
-static ulong reg0 __section(".data"); +ulong fw_dtb_pointer __section(".data");
/**
- Save x0 register value, assuming previous bootloader set it to
@@ -23,7 +23,7 @@ static ulong reg0 __section(".data"); */ void save_boot_params(ulong r0) {
- reg0 = r0;
- fw_dtb_pointer = r0; save_boot_params_ret();
I suppose this works, but it is a bit strange sice save_boot_params_ret is just a label that we're supposed to jump back to, not a function.
I think maybe a problem is save_boot_params_ret is poorly defined, or too specifically defined as a point in the early boot code.
We're in a funny spot here since arch/arm/lib/save_prev_bl_data.c isn't exactly generic (it might be previous BL is little kernel specific? not 100% sure), as well. It would be good, now that everything is in Kconfig, to tighten the dependencies (this code is arm64 only, and initramfs without fdt option doesn't make sense, so you can cleanup the Makefile and caller logic) and preface is_addr_accessible with static as it's only called there.
}
@@ -51,24 +51,24 @@ int save_prev_bl_data(void) int node; u64 initrd_start_prop;
- if (!is_addr_accessible((phys_addr_t)reg0))
- if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer)) return -ENODATA;
- fdt_blob = (struct fdt_header *)reg0;
- fdt_blob = (struct fdt_header *)fw_dtb_pointer; if (!fdt_valid(&fdt_blob)) {
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);return -ENODATA; }
if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
env_set_addr("prevbl_fdt_addr", (void *)reg0);
env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR)) return 0;
node = fdt_path_offset(fdt_blob, "/chosen"); if (!node) { pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
__func__, reg0);
__func__, fw_dtb_pointer);And we have no use for these additional environment variables and I'd prefer not to add them.
Okay, let's forget about it then.
Would you (or anybody responsible, I don't know who picks things up) be willing to take this patch without the Apple part then, a.k.a. renaming r0 to fw_dtb_pointer?
Well, in the next iteration of this series just drop the apple m1 related changes.
Hi,
On Fri, 24 Mar 2023 at 07:26, Konrad Dybcio konrad.dybcio@linaro.org wrote:
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
drivers/mmc/msm_sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c index 604f9c3ff99c..174435f01f68 100644 --- a/drivers/mmc/msm_sdhci.c +++ b/drivers/mmc/msm_sdhci.c @@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev) struct clk clk; int ret;
ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2);
You have to update corresponding dts files as well to avoid breaking platforms.
-Sumit
if (ret) return ret;-- 2.40.0
Hi Konrad,
On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio konrad.dybcio@linaro.org wrote:
"clocks" is the standard property used in Linux, "clock" seems to be an U-Boot invention. Use the one that's more standardized.
Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org
drivers/mmc/msm_sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c index 604f9c3ff99c..174435f01f68 100644 --- a/drivers/mmc/msm_sdhci.c +++ b/drivers/mmc/msm_sdhci.c @@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev) struct clk clk; int ret;
ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2); if (ret) return ret;
This would break existing msm platforms like qcs404 and rb3. So, please send the changed dts for those platforms with the driver changes. We would also need to test the changes on those platforms first.
Same is valid for other driver patches which alter the dts properties without changing the actual dtsi/dts first.
Thanks, Bhupesh
participants (5)
-
Bhupesh Sharma -
Konrad Dybcio -
Mark Kettenis -
Sumit Garg -
Tom Rini