* [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
@ 2021-03-20 18:17 Shaik Sajida Bhanu
2021-03-23 7:01 ` Stephen Boyd
2021-03-23 16:11 ` Doug Anderson
0 siblings, 2 replies; 22+ messages in thread
From: Shaik Sajida Bhanu @ 2021-03-20 18:17 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson, robh+dt
Cc: asutoshd, stummala, vbadigan, rampraka, sayalil, sartgarg,
rnayak, saiprakash.ranjan, sibis, cang, pragalla, nitirawa,
linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross,
bjorn.andersson, Shaik Sajida Bhanu
Add nodes for eMMC and SD card on sc7280.
Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
---
This change is depends on the below patch series:
https://lore.kernel.org/patchwork/project/lkml/list/?series=488871
https://lore.kernel.org/patchwork/project/lkml/list/?series=489530
https://lore.kernel.org/patchwork/project/lkml/list/?series=488429
Changes since V1:
- Moved SDHC nodes as suggested by Bjorn Andersson.
- Dropped "pinconf-" prefix as suggested by Bjorn Andersson.
- Removed extra newlines as suggested by Konrad Dybcio.
- Changed sd-cd pin to bias-pull-up in sdc2_off as suggested by
Veerabhadrarao Badiganti.
- Added bandwidth votes for eMMC and SD card.
---
arch/arm64/boot/dts/qcom/sc7280-idp.dts | 25 ++++
arch/arm64/boot/dts/qcom/sc7280.dtsi | 213 ++++++++++++++++++++++++++++++++
2 files changed, 238 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 54d2cb3..4105263 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -8,6 +8,7 @@
/dts-v1/;
#include "sc7280.dtsi"
+#include <dt-bindings/gpio/gpio.h>
/ {
model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
@@ -242,6 +243,30 @@
status = "okay";
};
+&sdhc_1 {
+ status = "okay";
+
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&sdc1_on>;
+ pinctrl-1 = <&sdc1_off>;
+
+ vmmc-supply = <&vreg_l7b_2p9>;
+ vqmmc-supply = <&vreg_l19b_1p8>;
+};
+
+&sdhc_2 {
+ status = "okay";
+
+ pinctrl-names = "default","sleep";
+ pinctrl-0 = <&sdc2_on>;
+ pinctrl-1 = <&sdc2_off>;
+
+ vmmc-supply = <&vreg_l9c_2p9>;
+ vqmmc-supply = <&vreg_l6c_2p9>;
+
+ cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
+};
+
/* PINCTRL - additions to nodes defined in sc7280.dtsi */
&qup_uart5_default {
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 8f6b569..69eb064 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -20,6 +20,11 @@
chosen { };
+ aliases {
+ mmc1 = &sdhc_1;
+ mmc2 = &sdhc_2;
+ };
+
clocks {
xo_board: xo-board {
compatible = "fixed-clock";
@@ -305,6 +310,64 @@
#power-domain-cells = <1>;
};
+ sdhc_1: sdhci@7c4000 {
+ compatible = "qcom,sdhci-msm-v5";
+ reg = <0 0x7c4000 0 0x1000>,
+ <0 0x7c5000 0 0x1000>;
+ reg-names = "hc", "cqhci";
+
+ iommus = <&apps_smmu 0xC0 0x0>;
+ interrupts = <GIC_SPI 652 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 656 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hc_irq", "pwr_irq";
+
+ clocks = <&gcc GCC_SDCC1_APPS_CLK>,
+ <&gcc GCC_SDCC1_AHB_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "core", "iface", "xo";
+ interconnects = <&aggre1_noc MASTER_SDCC_1 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_1 0>;
+ interconnect-names = "sdhc-ddr","cpu-sdhc";
+ power-domains = <&rpmhpd SC7280_CX>;
+ operating-points-v2 = <&sdhc1_opp_table>;
+
+ bus-width = <8>;
+ non-removable;
+ supports-cqe;
+ no-sd;
+ no-sdio;
+
+ max-frequency = <192000000>;
+
+ qcom,dll-config = <0x0007642c>;
+ qcom,ddr-config = <0x80040868>;
+
+ mmc-ddr-1_8v;
+ mmc-hs200-1_8v;
+ mmc-hs400-1_8v;
+ mmc-hs400-enhanced-strobe;
+
+ status = "disabled";
+
+ sdhc1_opp_table: sdhc1-opp-table {
+ compatible = "operating-points-v2";
+
+ opp-100000000 {
+ opp-hz = /bits/ 64 <100000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <1200000 76000>;
+ opp-avg-kBps = <1200000 50000>;
+ };
+
+ opp-384000000 {
+ opp-hz = /bits/ 64 <384000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <5400000 1600000>;
+ opp-avg-kBps = <6000000 300000>;
+ };
+ };
+ };
+
qupv3_id_0: geniqup@9c0000 {
compatible = "qcom,geni-se-qup";
reg = <0 0x009c0000 0 0x2000>;
@@ -328,6 +391,54 @@
};
};
+ sdhc_2: sdhci@8804000 {
+ compatible = "qcom,sdhci-msm-v5";
+ reg = <0 0x08804000 0 0x1000>;
+
+ iommus = <&apps_smmu 0x100 0x0>;
+ interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hc_irq", "pwr_irq";
+
+ clocks = <&gcc GCC_SDCC2_APPS_CLK>,
+ <&gcc GCC_SDCC2_AHB_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "core", "iface", "xo";
+ interconnects = <&aggre1_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_2 0>;
+ interconnect-names = "sdhc-ddr","cpu-sdhc";
+ power-domains = <&rpmhpd SC7280_CX>;
+ operating-points-v2 = <&sdhc2_opp_table>;
+
+ bus-width = <4>;
+
+ no-mmc;
+ no-sdio;
+
+ max-frequency = <202000000>;
+
+ qcom,dll-config = <0x0007642c>;
+
+ status = "disabled";
+
+ sdhc2_opp_table: sdhc2-opp-table {
+ compatible = "operating-points-v2";
+
+ opp-100000000 {
+ opp-hz =/bits/ 64 <100000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <1200000 76000>;
+ opp-avg-kBps = <1200000 50000>;
+ };
+ opp-202000000 {
+ opp-hz = /bits/ 64 <202000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <3500000 1200000>;
+ opp-avg-kBps = <5000000 100000>;
+ };
+ };
+ };
+
pdc: interrupt-controller@b220000 {
compatible = "qcom,sc7280-pdc", "qcom,pdc";
reg = <0 0x0b220000 0 0x30000>;
@@ -374,6 +485,108 @@
pins = "gpio46", "gpio47";
function = "qup13";
};
+
+ sdc1_on: sdc1-on {
+ clk {
+ pins = "sdc1_clk";
+ bias-disable;
+ drive-strength = <16>;
+ };
+
+ cmd {
+ pins = "sdc1_cmd";
+ bias-pull-up;
+ drive-strength = <10>;
+ };
+
+ data {
+ pins = "sdc1_data";
+ bias-pull-up;
+ drive-strength = <10>;
+ };
+
+ rclk {
+ pins = "sdc1_rclk";
+ bias-pull-down;
+ };
+ };
+
+ sdc1_off: sdc1-off {
+ clk {
+ pins = "sdc1_clk";
+ bias-disable;
+ drive-strength = <2>;
+ };
+
+ cmd {
+ pins = "sdc1_cmd";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+
+ data {
+ pins = "sdc1_data";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+
+ rclk {
+ pins = "sdc1_rclk";
+ bias-pull-down;
+ };
+ };
+
+ sdc2_on: sdc2-on {
+ clk {
+ pins = "sdc2_clk";
+ bias-disable;
+ drive-strength = <16>;
+ };
+
+ cmd {
+ pins = "sdc2_cmd";
+ bias-pull-up;
+ drive-strength = <10>;
+ };
+
+ data {
+ pins = "sdc2_data";
+ bias-pull-up;
+ drive-strength = <10>;
+ };
+
+ sd-cd {
+ pins = "gpio91";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+ };
+
+ sdc2_off: sdc2-off {
+ clk {
+ pins = "sdc2_clk";
+ bias-disable;
+ drive-strength = <2>;
+ };
+
+ cmd {
+ pins = "sdc2_cmd";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+
+ data {
+ pins = "sdc2_data";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+
+ sd-cd {
+ pins = "gpio91";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+ };
};
apps_smmu: iommu@15000000 {
--
2.7.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-20 18:17 [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card Shaik Sajida Bhanu
@ 2021-03-23 7:01 ` Stephen Boyd
2021-03-24 15:23 ` sbhanu
2021-03-23 16:11 ` Doug Anderson
1 sibling, 1 reply; 22+ messages in thread
From: Stephen Boyd @ 2021-03-23 7:01 UTC (permalink / raw)
To: Shaik Sajida Bhanu, adrian.hunter, robh+dt, ulf.hansson
Cc: asutoshd, stummala, vbadigan, rampraka, sayalil, sartgarg,
rnayak, saiprakash.ranjan, sibis, cang, pragalla, nitirawa,
linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross,
bjorn.andersson, Shaik Sajida Bhanu
Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 54d2cb3..4105263 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -8,6 +8,7 @@
> /dts-v1/;
>
> #include "sc7280.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
Please include this before sc7280.dtsi
>
> / {
> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
> @@ -242,6 +243,30 @@
> status = "okay";
> };
>
> +&sdhc_1 {
> + status = "okay";
> +
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&sdc1_on>;
> + pinctrl-1 = <&sdc1_off>;
> +
> + vmmc-supply = <&vreg_l7b_2p9>;
> + vqmmc-supply = <&vreg_l19b_1p8>;
> +};
> +
> +&sdhc_2 {
> + status = "okay";
> +
> + pinctrl-names = "default","sleep";
Please add a space after the comma ^
> + pinctrl-0 = <&sdc2_on>;
> + pinctrl-1 = <&sdc2_off>;
> +
> + vmmc-supply = <&vreg_l9c_2p9>;
> + vqmmc-supply = <&vreg_l6c_2p9>;
> +
> + cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
> +};
> +
> /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>
> &qup_uart5_default {
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 8f6b569..69eb064 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -20,6 +20,11 @@
>
> chosen { };
>
> + aliases {
> + mmc1 = &sdhc_1;
> + mmc2 = &sdhc_2;
> + };
> +
> clocks {
> xo_board: xo-board {
> compatible = "fixed-clock";
> @@ -305,6 +310,64 @@
> #power-domain-cells = <1>;
> };
>
> + sdhc_1: sdhci@7c4000 {
> + compatible = "qcom,sdhci-msm-v5";
> + reg = <0 0x7c4000 0 0x1000>,
Please add leading zeroes to the physical address, i.e. 0x007c4000
> + <0 0x7c5000 0 0x1000>;
> + reg-names = "hc", "cqhci";
> +
> + iommus = <&apps_smmu 0xC0 0x0>;
Lowercase hex please.
> + interrupts = <GIC_SPI 652 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 656 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
> + <&gcc GCC_SDCC1_AHB_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "core", "iface", "xo";
> + interconnects = <&aggre1_noc MASTER_SDCC_1 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_1 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + power-domains = <&rpmhpd SC7280_CX>;
> + operating-points-v2 = <&sdhc1_opp_table>;
> +
> + bus-width = <8>;
> + non-removable;
> + supports-cqe;
> + no-sd;
> + no-sdio;
> +
> + max-frequency = <192000000>;
Is this necessary?
> +
> + qcom,dll-config = <0x0007642c>;
> + qcom,ddr-config = <0x80040868>;
> +
> + mmc-ddr-1_8v;
> + mmc-hs200-1_8v;
> + mmc-hs400-1_8v;
> + mmc-hs400-enhanced-strobe;
> +
> + status = "disabled";
Can this be near the compatible string?
> +
> + sdhc1_opp_table: sdhc1-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz = /bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <1200000 76000>;
> + opp-avg-kBps = <1200000 50000>;
> + };
> +
> + opp-384000000 {
> + opp-hz = /bits/ 64 <384000000>;
> + required-opps = <&rpmhpd_opp_nom>;
> + opp-peak-kBps = <5400000 1600000>;
> + opp-avg-kBps = <6000000 300000>;
> + };
> + };
> + };
> +
> qupv3_id_0: geniqup@9c0000 {
> compatible = "qcom,geni-se-qup";
> reg = <0 0x009c0000 0 0x2000>;
> @@ -328,6 +391,54 @@
> };
> };
>
> + sdhc_2: sdhci@8804000 {
> + compatible = "qcom,sdhci-msm-v5";
> + reg = <0 0x08804000 0 0x1000>;
This has leading zeroes, great!
> +
> + iommus = <&apps_smmu 0x100 0x0>;
> + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC2_APPS_CLK>,
> + <&gcc GCC_SDCC2_AHB_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
Is this aligned properly?
> + clock-names = "core", "iface", "xo";
> + interconnects = <&aggre1_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_2 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + power-domains = <&rpmhpd SC7280_CX>;
> + operating-points-v2 = <&sdhc2_opp_table>;
> +
> + bus-width = <4>;
> +
> + no-mmc;
> + no-sdio;
> +
> + max-frequency = <202000000>;
Is this necessary?
> +
> + qcom,dll-config = <0x0007642c>;
> +
> + status = "disabled";
Move up near compatible?
> +
> + sdhc2_opp_table: sdhc2-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz =/bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <1200000 76000>;
> + opp-avg-kBps = <1200000 50000>;
> + };
> + opp-202000000 {
> + opp-hz = /bits/ 64 <202000000>;
> + required-opps = <&rpmhpd_opp_nom>;
> + opp-peak-kBps = <3500000 1200000>;
> + opp-avg-kBps = <5000000 100000>;
> + };
> + };
> + };
> +
> pdc: interrupt-controller@b220000 {
> compatible = "qcom,sc7280-pdc", "qcom,pdc";
> reg = <0 0x0b220000 0 0x30000>;
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-20 18:17 [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card Shaik Sajida Bhanu
2021-03-23 7:01 ` Stephen Boyd
@ 2021-03-23 16:11 ` Doug Anderson
2021-03-25 3:58 ` Veerabhadrarao Badiganti
1 sibling, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2021-03-23 16:11 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Asutosh Das,
Sahitya Tummala, Veerabhadrarao Badiganti, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
Hi,
On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
<sbhanu@codeaurora.org> wrote:
>
> Add nodes for eMMC and SD card on sc7280.
>
> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>
> ---
> This change is depends on the below patch series:
> https://lore.kernel.org/patchwork/project/lkml/list/?series=488871
> https://lore.kernel.org/patchwork/project/lkml/list/?series=489530
> https://lore.kernel.org/patchwork/project/lkml/list/?series=488429
>
> Changes since V1:
> - Moved SDHC nodes as suggested by Bjorn Andersson.
> - Dropped "pinconf-" prefix as suggested by Bjorn Andersson.
> - Removed extra newlines as suggested by Konrad Dybcio.
> - Changed sd-cd pin to bias-pull-up in sdc2_off as suggested by
> Veerabhadrarao Badiganti.
> - Added bandwidth votes for eMMC and SD card.
> ---
> arch/arm64/boot/dts/qcom/sc7280-idp.dts | 25 ++++
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 213 ++++++++++++++++++++++++++++++++
> 2 files changed, 238 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 54d2cb3..4105263 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -8,6 +8,7 @@
> /dts-v1/;
>
> #include "sc7280.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
>
> / {
> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
> @@ -242,6 +243,30 @@
> status = "okay";
> };
>
> +&sdhc_1 {
> + status = "okay";
When I apply your patch I find that your sort order is wrong. "s"
comes before "u" and after "q" in the alphabet so "sdhc_1" and
"sdhc_2" should sort _after "qupv3_id_0" and before "uart5"
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&sdc1_on>;
> + pinctrl-1 = <&sdc1_off>;
> +
> + vmmc-supply = <&vreg_l7b_2p9>;
> + vqmmc-supply = <&vreg_l19b_1p8>;
> +};
> +
> +&sdhc_2 {
> + status = "okay";
> +
> + pinctrl-names = "default","sleep";
> + pinctrl-0 = <&sdc2_on>;
> + pinctrl-1 = <&sdc2_off>;
> +
> + vmmc-supply = <&vreg_l9c_2p9>;
> + vqmmc-supply = <&vreg_l6c_2p9>;
> +
> + cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
Where is the pinctrl for the card detect? Oh, I see it's in
"sdc2_on". Probably would be good to break it out since this is
board-specific. See below.
> +};
> +
> /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>
> &qup_uart5_default {
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 8f6b569..69eb064 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -20,6 +20,11 @@
>
> chosen { };
>
> + aliases {
> + mmc1 = &sdhc_1;
> + mmc2 = &sdhc_2;
> + };
> +
> clocks {
> xo_board: xo-board {
> compatible = "fixed-clock";
> @@ -305,6 +310,64 @@
> #power-domain-cells = <1>;
> };
>
> + sdhc_1: sdhci@7c4000 {
> + compatible = "qcom,sdhci-msm-v5";
Please make the compatible:
compatible = "qcom,sc7280-sdhci", "qcom,sdhci-msm-v5";
...and add to the bindings. It should be a trivial bindings patch so
not too much trouble.
NOTE: even though the "qcom,sc7280-sdhci" should be in the bindings
and here you _shouldn't_ be adding any code for it. Just let the
fallback compatible string ("qcom,sdhci-msm-v5") do its magic. Adding
the sc7280 specific version is more of a "just in case we need it
later" type thing.
> + reg = <0 0x7c4000 0 0x1000>,
> + <0 0x7c5000 0 0x1000>;
> + reg-names = "hc", "cqhci";
> +
> + iommus = <&apps_smmu 0xC0 0x0>;
> + interrupts = <GIC_SPI 652 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 656 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
> + <&gcc GCC_SDCC1_AHB_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "core", "iface", "xo";
I'm curious: why is the "xo" clock needed here but not for sc7180?
> + interconnects = <&aggre1_noc MASTER_SDCC_1 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_1 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + power-domains = <&rpmhpd SC7280_CX>;
> + operating-points-v2 = <&sdhc1_opp_table>;
> +
> + bus-width = <8>;
> + non-removable;
This was actually a problem on sc7180 too, but you probably don't want
"non-removable" in the SoC file. Board files really should be adding
this. Though the SoC might be designed with the idea that this would
be used for a non-removable eMMC card I don't know why it wouldn't be
possible for someone to hook this up to an external slot and use a
GPIO somewhere as a card detect.
> + supports-cqe;
> + no-sd;
> + no-sdio;
Does the port really not support SD / SDIO, or are you adding these
two properties just because on your reference board it's not hooked up
to SD/SDIO? What exactly makes it impossible to use SD/SDIO on this
port?
> + max-frequency = <192000000>;
Why do you need to specify this?
> + qcom,dll-config = <0x0007642c>;
> + qcom,ddr-config = <0x80040868>;
These magic hex values really have no place being in dts which should
have things expressed at a higher level. ...but I guess that ship has
sailed and this is in the bindings so I guess we're stuck with them,
so I guess they're fine.
> + mmc-ddr-1_8v;
> + mmc-hs200-1_8v;
> + mmc-hs400-1_8v;
> + mmc-hs400-enhanced-strobe;
> +
> + status = "disabled";
> +
> + sdhc1_opp_table: sdhc1-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz = /bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <1200000 76000>;
> + opp-avg-kBps = <1200000 50000>;
Why are the kBps numbers so vastly different than the ones on sc7180
for the same OPP point. That implies:
a) sc7180 is wrong.
b) This patch is wrong.
c) The numbers are essentially random and don't really matter.
Can you identify which of a), b), or c) is correct, or propose an
alternate explanation of the difference?
> + };
> +
> + opp-384000000 {
> + opp-hz = /bits/ 64 <384000000>;
> + required-opps = <&rpmhpd_opp_nom>;
> + opp-peak-kBps = <5400000 1600000>;
> + opp-avg-kBps = <6000000 300000>;
These opp numbers are also quite different than sc7180
> + };
> + };
> + };
> +
> qupv3_id_0: geniqup@9c0000 {
> compatible = "qcom,geni-se-qup";
> reg = <0 0x009c0000 0 0x2000>;
> @@ -328,6 +391,54 @@
> };
> };
>
> + sdhc_2: sdhci@8804000 {
> + compatible = "qcom,sdhci-msm-v5";
> + reg = <0 0x08804000 0 0x1000>;
> +
> + iommus = <&apps_smmu 0x100 0x0>;
> + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC2_APPS_CLK>,
> + <&gcc GCC_SDCC2_AHB_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "core", "iface", "xo";
> + interconnects = <&aggre1_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_2 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + power-domains = <&rpmhpd SC7280_CX>;
> + operating-points-v2 = <&sdhc2_opp_table>;
> +
> + bus-width = <4>;
> +
> + no-mmc;
> + no-sdio;
Similar question to above: why exactly would mmc not work? Are you
saying that if someone hooked this up to a full sized SD card slot and
placed an MMC card into the slot that it wouldn't work? Similar
question about SDIO. If someone placed an external SDIO card into your
slot, would it not work?
> + max-frequency = <202000000>;
Not needed?
> +
> + qcom,dll-config = <0x0007642c>;
> +
> + status = "disabled";
> +
> + sdhc2_opp_table: sdhc2-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz =/bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <1200000 76000>;
> + opp-avg-kBps = <1200000 50000>;
> + };
> + opp-202000000 {
Blank line between the OPPs?
> + opp-hz = /bits/ 64 <202000000>;
> + required-opps = <&rpmhpd_opp_nom>;
> + opp-peak-kBps = <3500000 1200000>;
> + opp-avg-kBps = <5000000 100000>;
> + };
Similar questions about why the OPPs are so vastly different from sc7180.
> + };
> + };
> +
> pdc: interrupt-controller@b220000 {
> compatible = "qcom,sc7280-pdc", "qcom,pdc";
> reg = <0 0x0b220000 0 0x30000>;
> @@ -374,6 +485,108 @@
> pins = "gpio46", "gpio47";
> function = "qup13";
> };
> +
> + sdc1_on: sdc1-on {
> + clk {
> + pins = "sdc1_clk";
> + bias-disable;
> + drive-strength = <16>;
> + };
> +
> + cmd {
> + pins = "sdc1_cmd";
> + bias-pull-up;
> + drive-strength = <10>;
> + };
> +
> + data {
> + pins = "sdc1_data";
> + bias-pull-up;
> + drive-strength = <10>;
> + };
> +
> + rclk {
> + pins = "sdc1_rclk";
> + bias-pull-down;
> + };
* generally "bias" doesn't belong in the SoC file but instead should
be in the board file. Some boards might have external pulls (even if
the internal ones would work fine, hardware designers do weird things)
and thus might need to disable the internal ones (double pulls are not
great).
* generally drive-strength doesn't belong in the SoC file but should
be in the board file. Different boards with different layouts might
need different drive strengths, right?
If you remove those two things, I guess there's not actually much left
in the SoC dtsi file so I guess move these all to the board file? That
seems to be what we ended up with in "qrb5165-rb5.dts" / "sm8250.dtsi"
which is an example of a board using the new style of pinctrl for
devicetree.
> + };
> +
> + sdc1_off: sdc1-off {
> + clk {
> + pins = "sdc1_clk";
> + bias-disable;
> + drive-strength = <2>;
> + };
> +
> + cmd {
> + pins = "sdc1_cmd";
> + bias-pull-up;
> + drive-strength = <2>;
> + };
> +
> + data {
> + pins = "sdc1_data";
> + bias-pull-up;
> + drive-strength = <2>;
> + };
> +
> + rclk {
> + pins = "sdc1_rclk";
> + bias-pull-down;
> + };
> + };
No need for a sleep state for the rclk since it's the same as the
active state, right? NOTE: one way to handle this would be to define
one node per pingroup and thus do something like:
pinctrl-names = "default", "sleep";
pinctrl-0 = <&sdc1_clk>, <&sdc1_cmd>, <&sdc1_data>, <&sdc1_rclk>;
pinctrl-1 = <&sdc1_clk_sleep>, <&sdc1_cmd_sleep>, <&sdc1_data_sleep>,
<&sdc1_rclk>;
I do wish we could avoid having to duplicate the "bias" in every board
file. Hrm, I wonder if this could be made simpler by actually putting
the "sleep" states in the sc7180.dtsi file (not the board file) and
using "bias-bus-hold" to avoid it being board specific?
Thus (assuming it works), the total summary would be:
1. Board dts file fully defines "sdc1_clk", "sdc1_cmd", "sdc1_data",
"sdc1_rclk", specifying whatever bias and drive strength needed for
the board.
2. SoC dtsi fully defines "sdc1_clk_sleep", "sdc1_cmd_sleep",
"sdc1_data_sleep", "sdc1_rclk_sleep", specifying drive-strength of 2
(for outputs) and "bias-bus-hold" which is OK for all board.
> +
> + sdc2_on: sdc2-on {
> + clk {
> + pins = "sdc2_clk";
> + bias-disable;
> + drive-strength = <16>;
> + };
> +
> + cmd {
> + pins = "sdc2_cmd";
> + bias-pull-up;
> + drive-strength = <10>;
> + };
> +
> + data {
> + pins = "sdc2_data";
> + bias-pull-up;
> + drive-strength = <10>;
> + };
> +
> + sd-cd {
> + pins = "gpio91";
NOTE: even if we find some reason to keep some of the pinctrl in the
SoC dtsi file, the card detect almost certainly needs to move _fully_
to the board dts file. Different boards could use a different card
detect pin.
> + bias-pull-up;
> + drive-strength = <2>;
Drive strength isn't needed for input pins. Please remove.
> + };
> + };
> +
> + sdc2_off: sdc2-off {
> + clk {
> + pins = "sdc2_clk";
> + bias-disable;
> + drive-strength = <2>;
> + };
> +
> + cmd {
> + pins = "sdc2_cmd";
> + bias-pull-up;
> + drive-strength = <2>;
> + };
> +
> + data {
> + pins = "sdc2_data";
> + bias-pull-up;
> + drive-strength = <2>;
> + };
> +
> + sd-cd {
> + pins = "gpio91";
> + bias-pull-up;
> + drive-strength = <2>;
> + };
There's definitely no need for a separate sleep state for the CD line.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-23 7:01 ` Stephen Boyd
@ 2021-03-24 15:23 ` sbhanu
2021-03-24 15:57 ` Stephen Boyd
0 siblings, 1 reply; 22+ messages in thread
From: sbhanu @ 2021-03-24 15:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala,
vbadigan, rampraka, sayalil, sartgarg, rnayak, saiprakash.ranjan,
sibis, cang, pragalla, nitirawa, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, agross, bjorn.andersson
On 2021-03-23 12:31, Stephen Boyd wrote:
> Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index 54d2cb3..4105263 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -8,6 +8,7 @@
>> /dts-v1/;
>>
>> #include "sc7280.dtsi"
>> +#include <dt-bindings/gpio/gpio.h>
>
> Please include this before sc7280.dtsi
>
Sure
>>
>> / {
>> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
>> @@ -242,6 +243,30 @@
>> status = "okay";
>> };
>>
>> +&sdhc_1 {
>> + status = "okay";
>> +
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&sdc1_on>;
>> + pinctrl-1 = <&sdc1_off>;
>> +
>> + vmmc-supply = <&vreg_l7b_2p9>;
>> + vqmmc-supply = <&vreg_l19b_1p8>;
>> +};
>> +
>> +&sdhc_2 {
>> + status = "okay";
>> +
>> + pinctrl-names = "default","sleep";
>
> Please add a space after the comma ^
Sure
>
>> + pinctrl-0 = <&sdc2_on>;
>> + pinctrl-1 = <&sdc2_off>;
>> +
>> + vmmc-supply = <&vreg_l9c_2p9>;
>> + vqmmc-supply = <&vreg_l6c_2p9>;
>> +
>> + cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>> +};
>> +
>> /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>>
>> &qup_uart5_default {
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 8f6b569..69eb064 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -20,6 +20,11 @@
>>
>> chosen { };
>>
>> + aliases {
>> + mmc1 = &sdhc_1;
>> + mmc2 = &sdhc_2;
>> + };
>> +
>> clocks {
>> xo_board: xo-board {
>> compatible = "fixed-clock";
>> @@ -305,6 +310,64 @@
>> #power-domain-cells = <1>;
>> };
>>
>> + sdhc_1: sdhci@7c4000 {
>> + compatible = "qcom,sdhci-msm-v5";
>> + reg = <0 0x7c4000 0 0x1000>,
>
> Please add leading zeroes to the physical address, i.e. 0x007c4000
Sure
>
>> + <0 0x7c5000 0 0x1000>;
>> + reg-names = "hc", "cqhci";
>> +
>> + iommus = <&apps_smmu 0xC0 0x0>;
>
> Lowercase hex please.
Sure
>
>> + interrupts = <GIC_SPI 652
>> IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 656
>> IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "hc_irq", "pwr_irq";
>> +
>> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
>> + <&gcc GCC_SDCC1_AHB_CLK>,
>> + <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "core", "iface", "xo";
>> + interconnects = <&aggre1_noc MASTER_SDCC_1 0
>> &mc_virt SLAVE_EBI1 0>,
>> + <&gem_noc MASTER_APPSS_PROC 0
>> &cnoc2 SLAVE_SDCC_1 0>;
>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>> + power-domains = <&rpmhpd SC7280_CX>;
>> + operating-points-v2 = <&sdhc1_opp_table>;
>> +
>> + bus-width = <8>;
>> + non-removable;
>> + supports-cqe;
>> + no-sd;
>> + no-sdio;
>> +
>> + max-frequency = <192000000>;
>
> Is this necessary?
yes, to avoid lower speed modes running with high clock rates.
>
>> +
>> + qcom,dll-config = <0x0007642c>;
>> + qcom,ddr-config = <0x80040868>;
>> +
>> + mmc-ddr-1_8v;
>> + mmc-hs200-1_8v;
>> + mmc-hs400-1_8v;
>> + mmc-hs400-enhanced-strobe;
>> +
>> + status = "disabled";
>
> Can this be near the compatible string?
Sure
>
>> +
>> + sdhc1_opp_table: sdhc1-opp-table {
>> + compatible = "operating-points-v2";
>> +
>> + opp-100000000 {
>> + opp-hz = /bits/ 64
>> <100000000>;
>> + required-opps =
>> <&rpmhpd_opp_low_svs>;
>> + opp-peak-kBps = <1200000
>> 76000>;
>> + opp-avg-kBps = <1200000
>> 50000>;
>> + };
>> +
>> + opp-384000000 {
>> + opp-hz = /bits/ 64
>> <384000000>;
>> + required-opps =
>> <&rpmhpd_opp_nom>;
>> + opp-peak-kBps = <5400000
>> 1600000>;
>> + opp-avg-kBps = <6000000
>> 300000>;
>> + };
>> + };
>> + };
>> +
>> qupv3_id_0: geniqup@9c0000 {
>> compatible = "qcom,geni-se-qup";
>> reg = <0 0x009c0000 0 0x2000>;
>> @@ -328,6 +391,54 @@
>> };
>> };
>>
>> + sdhc_2: sdhci@8804000 {
>> + compatible = "qcom,sdhci-msm-v5";
>> + reg = <0 0x08804000 0 0x1000>;
>
> This has leading zeroes, great!
>
>> +
>> + iommus = <&apps_smmu 0x100 0x0>;
>> + interrupts = <GIC_SPI 207
>> IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 223
>> IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "hc_irq", "pwr_irq";
>> +
>> + clocks = <&gcc GCC_SDCC2_APPS_CLK>,
>> + <&gcc GCC_SDCC2_AHB_CLK>,
>> + <&rpmhcc RPMH_CXO_CLK>;
>
> Is this aligned properly?
yes
>
>> + clock-names = "core", "iface", "xo";
>> + interconnects = <&aggre1_noc MASTER_SDCC_2 0
>> &mc_virt SLAVE_EBI1 0>,
>> + <&gem_noc MASTER_APPSS_PROC 0
>> &cnoc2 SLAVE_SDCC_2 0>;
>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>> + power-domains = <&rpmhpd SC7280_CX>;
>> + operating-points-v2 = <&sdhc2_opp_table>;
>> +
>> + bus-width = <4>;
>> +
>> + no-mmc;
>> + no-sdio;
>> +
>> + max-frequency = <202000000>;
>
> Is this necessary?
yes, to avoid clock running more than required clock rate.
>
>> +
>> + qcom,dll-config = <0x0007642c>;
>> +
>> + status = "disabled";
>
> Move up near compatible?
sure
>
>> +
>> + sdhc2_opp_table: sdhc2-opp-table {
>> + compatible =
>> "operating-points-v2";
>> +
>> + opp-100000000 {
>> + opp-hz =/bits/ 64
>> <100000000>;
>> + required-opps =
>> <&rpmhpd_opp_low_svs>;
>> + opp-peak-kBps =
>> <1200000 76000>;
>> + opp-avg-kBps =
>> <1200000 50000>;
>> + };
>> + opp-202000000 {
>> + opp-hz = /bits/ 64
>> <202000000>;
>> + required-opps =
>> <&rpmhpd_opp_nom>;
>> + opp-peak-kBps =
>> <3500000 1200000>;
>> + opp-avg-kBps =
>> <5000000 100000>;
>> + };
>> + };
>> + };
>> +
>> pdc: interrupt-controller@b220000 {
>> compatible = "qcom,sc7280-pdc", "qcom,pdc";
>> reg = <0 0x0b220000 0 0x30000>;
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-24 15:23 ` sbhanu
@ 2021-03-24 15:57 ` Stephen Boyd
2021-03-24 16:28 ` Stephen Boyd
0 siblings, 1 reply; 22+ messages in thread
From: Stephen Boyd @ 2021-03-24 15:57 UTC (permalink / raw)
To: sbhanu
Cc: adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala,
vbadigan, rampraka, sayalil, sartgarg, rnayak, saiprakash.ranjan,
sibis, cang, pragalla, nitirawa, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, agross, bjorn.andersson
Quoting sbhanu@codeaurora.org (2021-03-24 08:23:55)
> On 2021-03-23 12:31, Stephen Boyd wrote:
> > Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
> >> +
> >> + bus-width = <8>;
> >> + non-removable;
> >> + supports-cqe;
> >> + no-sd;
> >> + no-sdio;
> >> +
> >> + max-frequency = <192000000>;
> >
> > Is this necessary?
> yes, to avoid lower speed modes running with high clock rates.
Is it part of the DT binding? I don't see any mention of it.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-24 15:57 ` Stephen Boyd
@ 2021-03-24 16:28 ` Stephen Boyd
2021-03-25 3:36 ` Veerabhadrarao Badiganti
0 siblings, 1 reply; 22+ messages in thread
From: Stephen Boyd @ 2021-03-24 16:28 UTC (permalink / raw)
To: sbhanu
Cc: adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala,
vbadigan, rampraka, sayalil, sartgarg, rnayak, saiprakash.ranjan,
sibis, cang, pragalla, nitirawa, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, agross, bjorn.andersson
Quoting Stephen Boyd (2021-03-24 08:57:33)
> Quoting sbhanu@codeaurora.org (2021-03-24 08:23:55)
> > On 2021-03-23 12:31, Stephen Boyd wrote:
> > > Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
> > >> +
> > >> + bus-width = <8>;
> > >> + non-removable;
> > >> + supports-cqe;
> > >> + no-sd;
> > >> + no-sdio;
> > >> +
> > >> + max-frequency = <192000000>;
> > >
> > > Is this necessary?
> > yes, to avoid lower speed modes running with high clock rates.
>
> Is it part of the DT binding? I don't see any mention of it.
Nevermind, found it in mmc-controller.yaml. But I think this is to work
around some problem with the clk driver picking lower speeds than
requested? That has been fixed on the clk driver side (see commit like
148ddaa89d4a "clk: qcom: gcc-sc7180: Use floor ops for the correct sdcc1
clk") so ideally this property can be omitted.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-24 16:28 ` Stephen Boyd
@ 2021-03-25 3:36 ` Veerabhadrarao Badiganti
2021-03-25 16:20 ` Doug Anderson
0 siblings, 1 reply; 22+ messages in thread
From: Veerabhadrarao Badiganti @ 2021-03-25 3:36 UTC (permalink / raw)
To: Stephen Boyd, sbhanu
Cc: adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala,
rampraka, sayalil, sartgarg, rnayak, saiprakash.ranjan, sibis,
cang, pragalla, nitirawa, linux-mmc, linux-kernel, linux-arm-msm,
devicetree, agross, bjorn.andersson
On 3/24/2021 9:58 PM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2021-03-24 08:57:33)
>> Quoting sbhanu@codeaurora.org (2021-03-24 08:23:55)
>>> On 2021-03-23 12:31, Stephen Boyd wrote:
>>>> Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
>>>>> +
>>>>> + bus-width = <8>;
>>>>> + non-removable;
>>>>> + supports-cqe;
>>>>> + no-sd;
>>>>> + no-sdio;
>>>>> +
>>>>> + max-frequency = <192000000>;
>>>> Is this necessary?
>>> yes, to avoid lower speed modes running with high clock rates.
>> Is it part of the DT binding? I don't see any mention of it.
> Nevermind, found it in mmc-controller.yaml. But I think this is to work
> around some problem with the clk driver picking lower speeds than
> requested? That has been fixed on the clk driver side (see commit like
> 148ddaa89d4a "clk: qcom: gcc-sc7180: Use floor ops for the correct sdcc1
> clk") so ideally this property can be omitted.
This is a good have dt node.
This will align clock requests between mmc core layer and sdhci-msm
platform driver. Say, for HS200/HS400 modes of eMMC, mmc-core layer
tries to set clock at 200Mhz, whereas sdhci-msm expects 192Mhz for
these modes. So we have to rely on clock driver floor/ceil values.
By having this property, mmc-core layer itself request for 192Mhz.
Same is for SD card SDR104 mode, core layer expects clock at 208Mhz
whereas sdhci-msm can max operate only at 202Mhz. By having this
property, core layer requests only for 202Mhz for SDR104 mode.
BTW, this helps only for max possible speed modes.
In case of lower-speed modes (for DDR52) we still need to rely on clock
floor rounding.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-23 16:11 ` Doug Anderson
@ 2021-03-25 3:58 ` Veerabhadrarao Badiganti
2021-03-25 16:17 ` Doug Anderson
2021-03-26 6:56 ` sbhanu
0 siblings, 2 replies; 22+ messages in thread
From: Veerabhadrarao Badiganti @ 2021-03-25 3:58 UTC (permalink / raw)
To: Doug Anderson, Shaik Sajida Bhanu
Cc: Adrian Hunter, Ulf Hansson, Rob Herring, Asutosh Das,
Sahitya Tummala, Ram Prakash Gupta, Sayali Lokhande, sartgarg,
Rajendra Nayak, Sai Prakash Ranjan, Sibi Sankar, cang, pragalla,
nitirawa, Linux MMC List, LKML, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
On 3/23/2021 9:41 PM, Doug Anderson wrote:
> Hi,
>
> On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
> <sbhanu@codeaurora.org> wrote:
>> Add nodes for eMMC and SD card on sc7280.
>>
>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>>
>> ---
>> This change is depends on the below patch series:
>> https://lore.kernel.org/patchwork/project/lkml/list/?series=488871
>> https://lore.kernel.org/patchwork/project/lkml/list/?series=489530
>> https://lore.kernel.org/patchwork/project/lkml/list/?series=488429
>>
>> Changes since V1:
>> - Moved SDHC nodes as suggested by Bjorn Andersson.
>> - Dropped "pinconf-" prefix as suggested by Bjorn Andersson.
>> - Removed extra newlines as suggested by Konrad Dybcio.
>> - Changed sd-cd pin to bias-pull-up in sdc2_off as suggested by
>> Veerabhadrarao Badiganti.
>> - Added bandwidth votes for eMMC and SD card.
>> ---
>> arch/arm64/boot/dts/qcom/sc7280-idp.dts | 25 ++++
>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 213 ++++++++++++++++++++++++++++++++
>> 2 files changed, 238 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index 54d2cb3..4105263 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -8,6 +8,7 @@
>> /dts-v1/;
>>
>> #include "sc7280.dtsi"
>> +#include <dt-bindings/gpio/gpio.h>
>>
>> / {
>> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
>> @@ -242,6 +243,30 @@
>> status = "okay";
>> };
>>
>> +&sdhc_1 {
>> + status = "okay";
> When I apply your patch I find that your sort order is wrong. "s"
> comes before "u" and after "q" in the alphabet so "sdhc_1" and
> "sdhc_2" should sort _after "qupv3_id_0" and before "uart5"
>
>
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&sdc1_on>;
>> + pinctrl-1 = <&sdc1_off>;
>> +
>> + vmmc-supply = <&vreg_l7b_2p9>;
>> + vqmmc-supply = <&vreg_l19b_1p8>;
>> +};
>> +
>> +&sdhc_2 {
>> + status = "okay";
>> +
>> + pinctrl-names = "default","sleep";
>> + pinctrl-0 = <&sdc2_on>;
>> + pinctrl-1 = <&sdc2_off>;
>> +
>> + vmmc-supply = <&vreg_l9c_2p9>;
>> + vqmmc-supply = <&vreg_l6c_2p9>;
>> +
>> + cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
> Where is the pinctrl for the card detect? Oh, I see it's in
> "sdc2_on". Probably would be good to break it out since this is
> board-specific. See below.
>
>
>> +};
>> +
>> /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>>
>> &qup_uart5_default {
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 8f6b569..69eb064 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -20,6 +20,11 @@
>>
>> chosen { };
>>
>> + aliases {
>> + mmc1 = &sdhc_1;
>> + mmc2 = &sdhc_2;
>> + };
>> +
>> clocks {
>> xo_board: xo-board {
>> compatible = "fixed-clock";
>> @@ -305,6 +310,64 @@
>> #power-domain-cells = <1>;
>> };
>>
>> + sdhc_1: sdhci@7c4000 {
>> + compatible = "qcom,sdhci-msm-v5";
> Please make the compatible:
> compatible = "qcom,sc7280-sdhci", "qcom,sdhci-msm-v5";
>
> ...and add to the bindings. It should be a trivial bindings patch so
> not too much trouble.
>
> NOTE: even though the "qcom,sc7280-sdhci" should be in the bindings
> and here you _shouldn't_ be adding any code for it. Just let the
> fallback compatible string ("qcom,sdhci-msm-v5") do its magic. Adding
> the sc7280 specific version is more of a "just in case we need it
> later" type thing.
>
>
>> + reg = <0 0x7c4000 0 0x1000>,
>> + <0 0x7c5000 0 0x1000>;
>> + reg-names = "hc", "cqhci";
>> +
>> + iommus = <&apps_smmu 0xC0 0x0>;
>> + interrupts = <GIC_SPI 652 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 656 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "hc_irq", "pwr_irq";
>> +
>> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
>> + <&gcc GCC_SDCC1_AHB_CLK>,
>> + <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "core", "iface", "xo";
> I'm curious: why is the "xo" clock needed here but not for sc7180?
Actually its needed even for sc7180. We are making use of this clock in
msm_init_cm_dll()
The default PoR value is also same as calculated value for
HS200/HS400/SDR104 modes.
But just not to rely on default register values we need this entry.
>
>> + interconnects = <&aggre1_noc MASTER_SDCC_1 0 &mc_virt SLAVE_EBI1 0>,
>> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_1 0>;
>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>> + power-domains = <&rpmhpd SC7280_CX>;
>> + operating-points-v2 = <&sdhc1_opp_table>;
>> +
>> + bus-width = <8>;
>> + non-removable;
> This was actually a problem on sc7180 too, but you probably don't want
> "non-removable" in the SoC file. Board files really should be adding
> this. Though the SoC might be designed with the idea that this would
> be used for a non-removable eMMC card I don't know why it wouldn't be
> possible for someone to hook this up to an external slot and use a
> GPIO somewhere as a card detect.
>
>
>> + supports-cqe;
>> + no-sd;
>> + no-sdio;
> Does the port really not support SD / SDIO, or are you adding these
> two properties just because on your reference board it's not hooked up
> to SD/SDIO? What exactly makes it impossible to use SD/SDIO on this
> port?
>
By having this, we can optimize emmc device scan time.
Driver wont issue SDIO & SDcards specific commands while
scanning the device.Its little optimization.
I think board specific dt is right place.
>> + max-frequency = <192000000>;
> Why do you need to specify this?
>
>
>> + qcom,dll-config = <0x0007642c>;
>> + qcom,ddr-config = <0x80040868>;
> These magic hex values really have no place being in dts which should
> have things expressed at a higher level. ...but I guess that ship has
> sailed and this is in the bindings so I guess we're stuck with them,
> so I guess they're fine.
>
>
>> + mmc-ddr-1_8v;
>> + mmc-hs200-1_8v;
>> + mmc-hs400-1_8v;
>> + mmc-hs400-enhanced-strobe;
>> +
>> + status = "disabled";
>> +
>> + sdhc1_opp_table: sdhc1-opp-table {
>> + compatible = "operating-points-v2";
>> +
>> + opp-100000000 {
>> + opp-hz = /bits/ 64 <100000000>;
>> + required-opps = <&rpmhpd_opp_low_svs>;
>> + opp-peak-kBps = <1200000 76000>;
>> + opp-avg-kBps = <1200000 50000>;
> Why are the kBps numbers so vastly different than the ones on sc7180
> for the same OPP point. That implies:
>
> a) sc7180 is wrong.
>
> b) This patch is wrong.
>
> c) The numbers are essentially random and don't really matter.
>
> Can you identify which of a), b), or c) is correct, or propose an
> alternate explanation of the difference?
>
>
>> + };
>> +
>> + opp-384000000 {
>> + opp-hz = /bits/ 64 <384000000>;
>> + required-opps = <&rpmhpd_opp_nom>;
>> + opp-peak-kBps = <5400000 1600000>;
>> + opp-avg-kBps = <6000000 300000>;
> These opp numbers are also quite different than sc7180
>
>
>> + };
>> + };
>> + };
>> +
>> qupv3_id_0: geniqup@9c0000 {
>> compatible = "qcom,geni-se-qup";
>> reg = <0 0x009c0000 0 0x2000>;
>> @@ -328,6 +391,54 @@
>> };
>> };
>>
>> + sdhc_2: sdhci@8804000 {
>> + compatible = "qcom,sdhci-msm-v5";
>> + reg = <0 0x08804000 0 0x1000>;
>> +
>> + iommus = <&apps_smmu 0x100 0x0>;
>> + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "hc_irq", "pwr_irq";
>> +
>> + clocks = <&gcc GCC_SDCC2_APPS_CLK>,
>> + <&gcc GCC_SDCC2_AHB_CLK>,
>> + <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "core", "iface", "xo";
>> + interconnects = <&aggre1_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
>> + <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_SDCC_2 0>;
>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>> + power-domains = <&rpmhpd SC7280_CX>;
>> + operating-points-v2 = <&sdhc2_opp_table>;
>> +
>> + bus-width = <4>;
>> +
>> + no-mmc;
>> + no-sdio;
> Similar question to above: why exactly would mmc not work? Are you
> saying that if someone hooked this up to a full sized SD card slot and
> placed an MMC card into the slot that it wouldn't work? Similar
> question about SDIO. If someone placed an external SDIO card into your
> slot, would it not work?
>
As mentioned above, its just to optimize SDcard scan time a little.
>> + max-frequency = <202000000>;
> Not needed?
>
>> +
>> + qcom,dll-config = <0x0007642c>;
>> +
>> + status = "disabled";
>> +
>> + sdhc2_opp_table: sdhc2-opp-table {
>> + compatible = "operating-points-v2";
>> +
>> + opp-100000000 {
>> + opp-hz =/bits/ 64 <100000000>;
>> + required-opps = <&rpmhpd_opp_low_svs>;
>> + opp-peak-kBps = <1200000 76000>;
>> + opp-avg-kBps = <1200000 50000>;
>> + };
>> + opp-202000000 {
> Blank line between the OPPs?
>
>> + opp-hz = /bits/ 64 <202000000>;
>> + required-opps = <&rpmhpd_opp_nom>;
>> + opp-peak-kBps = <3500000 1200000>;
>> + opp-avg-kBps = <5000000 100000>;
>> + };
> Similar questions about why the OPPs are so vastly different from sc7180.
>
>> + };
>> + };
>> +
>> pdc: interrupt-controller@b220000 {
>> compatible = "qcom,sc7280-pdc", "qcom,pdc";
>> reg = <0 0x0b220000 0 0x30000>;
>> @@ -374,6 +485,108 @@
>> pins = "gpio46", "gpio47";
>> function = "qup13";
>> };
>> +
>> + sdc1_on: sdc1-on {
>> + clk {
>> + pins = "sdc1_clk";
>> + bias-disable;
>> + drive-strength = <16>;
>> + };
>> +
>> + cmd {
>> + pins = "sdc1_cmd";
>> + bias-pull-up;
>> + drive-strength = <10>;
>> + };
>> +
>> + data {
>> + pins = "sdc1_data";
>> + bias-pull-up;
>> + drive-strength = <10>;
>> + };
>> +
>> + rclk {
>> + pins = "sdc1_rclk";
>> + bias-pull-down;
>> + };
> * generally "bias" doesn't belong in the SoC file but instead should
> be in the board file. Some boards might have external pulls (even if
> the internal ones would work fine, hardware designers do weird things)
> and thus might need to disable the internal ones (double pulls are not
> great).
>
> * generally drive-strength doesn't belong in the SoC file but should
> be in the board file. Different boards with different layouts might
> need different drive strengths, right?
>
> If you remove those two things, I guess there's not actually much left
> in the SoC dtsi file so I guess move these all to the board file? That
> seems to be what we ended up with in "qrb5165-rb5.dts" / "sm8250.dtsi"
> which is an example of a board using the new style of pinctrl for
> devicetree.
>
>
>> + };
>> +
>> + sdc1_off: sdc1-off {
>> + clk {
>> + pins = "sdc1_clk";
>> + bias-disable;
>> + drive-strength = <2>;
>> + };
>> +
>> + cmd {
>> + pins = "sdc1_cmd";
>> + bias-pull-up;
>> + drive-strength = <2>;
>> + };
>> +
>> + data {
>> + pins = "sdc1_data";
>> + bias-pull-up;
>> + drive-strength = <2>;
>> + };
>> +
>> + rclk {
>> + pins = "sdc1_rclk";
>> + bias-pull-down;
>> + };
>> + };
> No need for a sleep state for the rclk since it's the same as the
> active state, right? NOTE: one way to handle this would be to define
> one node per pingroup and thus do something like:
>
> pinctrl-names = "default", "sleep";
> pinctrl-0 = <&sdc1_clk>, <&sdc1_cmd>, <&sdc1_data>, <&sdc1_rclk>;
> pinctrl-1 = <&sdc1_clk_sleep>, <&sdc1_cmd_sleep>, <&sdc1_data_sleep>,
> <&sdc1_rclk>;
>
> I do wish we could avoid having to duplicate the "bias" in every board
> file. Hrm, I wonder if this could be made simpler by actually putting
> the "sleep" states in the sc7180.dtsi file (not the board file) and
> using "bias-bus-hold" to avoid it being board specific?
>
> Thus (assuming it works), the total summary would be:
>
> 1. Board dts file fully defines "sdc1_clk", "sdc1_cmd", "sdc1_data",
> "sdc1_rclk", specifying whatever bias and drive strength needed for
> the board.
>
> 2. SoC dtsi fully defines "sdc1_clk_sleep", "sdc1_cmd_sleep",
> "sdc1_data_sleep", "sdc1_rclk_sleep", specifying drive-strength of 2
> (for outputs) and "bias-bus-hold" which is OK for all board.
>
>
>> +
>> + sdc2_on: sdc2-on {
>> + clk {
>> + pins = "sdc2_clk";
>> + bias-disable;
>> + drive-strength = <16>;
>> + };
>> +
>> + cmd {
>> + pins = "sdc2_cmd";
>> + bias-pull-up;
>> + drive-strength = <10>;
>> + };
>> +
>> + data {
>> + pins = "sdc2_data";
>> + bias-pull-up;
>> + drive-strength = <10>;
>> + };
>> +
>> + sd-cd {
>> + pins = "gpio91";
> NOTE: even if we find some reason to keep some of the pinctrl in the
> SoC dtsi file, the card detect almost certainly needs to move _fully_
> to the board dts file. Different boards could use a different card
> detect pin.
>
>> + bias-pull-up;
>> + drive-strength = <2>;
> Drive strength isn't needed for input pins. Please remove.
>
>> + };
>> + };
>> +
>> + sdc2_off: sdc2-off {
>> + clk {
>> + pins = "sdc2_clk";
>> + bias-disable;
>> + drive-strength = <2>;
>> + };
>> +
>> + cmd {
>> + pins = "sdc2_cmd";
>> + bias-pull-up;
>> + drive-strength = <2>;
>> + };
>> +
>> + data {
>> + pins = "sdc2_data";
>> + bias-pull-up;
>> + drive-strength = <2>;
>> + };
>> +
>> + sd-cd {
>> + pins = "gpio91";
>> + bias-pull-up;
>> + drive-strength = <2>;
>> + };
> There's definitely no need for a separate sleep state for the CD line.
>
>
> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-25 3:58 ` Veerabhadrarao Badiganti
@ 2021-03-25 16:17 ` Doug Anderson
2021-04-01 9:58 ` sbhanu
2021-03-26 6:56 ` sbhanu
1 sibling, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2021-03-25 16:17 UTC (permalink / raw)
To: Veerabhadrarao Badiganti
Cc: Shaik Sajida Bhanu, Adrian Hunter, Ulf Hansson, Rob Herring,
Asutosh Das, Sahitya Tummala, Ram Prakash Gupta, Sayali Lokhande,
sartgarg, Rajendra Nayak, Sai Prakash Ranjan, Sibi Sankar, cang,
pragalla, nitirawa, Linux MMC List, LKML, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
Hi,
On Wed, Mar 24, 2021 at 8:59 PM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
> >> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
> >> + <&gcc GCC_SDCC1_AHB_CLK>,
> >> + <&rpmhcc RPMH_CXO_CLK>;
> >> + clock-names = "core", "iface", "xo";
> > I'm curious: why is the "xo" clock needed here but not for sc7180?
> Actually its needed even for sc7180. We are making use of this clock in
> msm_init_cm_dll()
> The default PoR value is also same as calculated value for
> HS200/HS400/SDR104 modes.
> But just not to rely on default register values we need this entry.
Can you post a patch for sc7180?
> >> + bus-width = <4>;
> >> +
> >> + no-mmc;
> >> + no-sdio;
> > Similar question to above: why exactly would mmc not work? Are you
> > saying that if someone hooked this up to a full sized SD card slot and
> > placed an MMC card into the slot that it wouldn't work? Similar
> > question about SDIO. If someone placed an external SDIO card into your
> > slot, would it not work?
> >
> As mentioned above, its just to optimize SDcard scan time a little.
OK. ...but while the eMMC one can make sense since the eMMC is
soldered down (but in the board dts file, not in the SoC dtsi file) I
think you should just remove these for SD card because:
1. Even if only a uSD slot is exposed it's still _possible_ for
someone to insert a card that uses MMC or SDIO signaling. If nothing
else I have a (probably non-compliant) adapter that plugs into a uSD
slot and provides a full sided SD slot. I could plug an MMC card or
SDIO card in.
2. Presumably the SD card scan time optimization is tiny.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-25 3:36 ` Veerabhadrarao Badiganti
@ 2021-03-25 16:20 ` Doug Anderson
0 siblings, 0 replies; 22+ messages in thread
From: Doug Anderson @ 2021-03-25 16:20 UTC (permalink / raw)
To: Veerabhadrarao Badiganti
Cc: Stephen Boyd, Shaik Sajida Bhanu, Adrian Hunter, Rob Herring,
Ulf Hansson, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
Hi,
On Wed, Mar 24, 2021 at 8:37 PM Veerabhadrarao Badiganti
<vbadigan@codeaurora.org> wrote:
>
>
> On 3/24/2021 9:58 PM, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2021-03-24 08:57:33)
> >> Quoting sbhanu@codeaurora.org (2021-03-24 08:23:55)
> >>> On 2021-03-23 12:31, Stephen Boyd wrote:
> >>>> Quoting Shaik Sajida Bhanu (2021-03-20 11:17:00)
> >>>>> +
> >>>>> + bus-width = <8>;
> >>>>> + non-removable;
> >>>>> + supports-cqe;
> >>>>> + no-sd;
> >>>>> + no-sdio;
> >>>>> +
> >>>>> + max-frequency = <192000000>;
> >>>> Is this necessary?
> >>> yes, to avoid lower speed modes running with high clock rates.
> >> Is it part of the DT binding? I don't see any mention of it.
> > Nevermind, found it in mmc-controller.yaml. But I think this is to work
> > around some problem with the clk driver picking lower speeds than
> > requested? That has been fixed on the clk driver side (see commit like
> > 148ddaa89d4a "clk: qcom: gcc-sc7180: Use floor ops for the correct sdcc1
> > clk") so ideally this property can be omitted.
> This is a good have dt node.
>
> This will align clock requests between mmc core layer and sdhci-msm
> platform driver. Say, for HS200/HS400 modes of eMMC, mmc-core layer
> tries to set clock at 200Mhz, whereas sdhci-msm expects 192Mhz for
> these modes. So we have to rely on clock driver floor/ceil values.
> By having this property, mmc-core layer itself request for 192Mhz.
>
> Same is for SD card SDR104 mode, core layer expects clock at 208Mhz
> whereas sdhci-msm can max operate only at 202Mhz. By having this
> property, core layer requests only for 202Mhz for SDR104 mode.
>
> BTW, this helps only for max possible speed modes.
> In case of lower-speed modes (for DDR52) we still need to rely on clock
> floor rounding.
Just let the clock driver figure it out and remove this from the
devicetree, please. As you said, the clock driver needs to understand
how to round rates anyway for the non-maximum requests. Putting the
information here just duplicates the data.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-25 3:58 ` Veerabhadrarao Badiganti
2021-03-25 16:17 ` Doug Anderson
@ 2021-03-26 6:56 ` sbhanu
2021-03-29 14:56 ` Doug Anderson
1 sibling, 1 reply; 22+ messages in thread
From: sbhanu @ 2021-03-26 6:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
On 2021-03-25 09:28, Veerabhadrarao Badiganti wrote:
> On 3/23/2021 9:41 PM, Doug Anderson wrote:
>> Hi,
>>
>> On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
>> <sbhanu@codeaurora.org> wrote:
>>> Add nodes for eMMC and SD card on sc7280.
>>>
>>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>>>
>>> ---
>>> This change is depends on the below patch series:
>>> https://lore.kernel.org/patchwork/project/lkml/list/?series=488871
>>> https://lore.kernel.org/patchwork/project/lkml/list/?series=489530
>>> https://lore.kernel.org/patchwork/project/lkml/list/?series=488429
>>>
>>> Changes since V1:
>>> - Moved SDHC nodes as suggested by Bjorn Andersson.
>>> - Dropped "pinconf-" prefix as suggested by Bjorn Andersson.
>>> - Removed extra newlines as suggested by Konrad Dybcio.
>>> - Changed sd-cd pin to bias-pull-up in sdc2_off as suggested
>>> by
>>> Veerabhadrarao Badiganti.
>>> - Added bandwidth votes for eMMC and SD card.
>>> ---
>>> arch/arm64/boot/dts/qcom/sc7280-idp.dts | 25 ++++
>>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 213
>>> ++++++++++++++++++++++++++++++++
>>> 2 files changed, 238 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> index 54d2cb3..4105263 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> @@ -8,6 +8,7 @@
>>> /dts-v1/;
>>>
>>> #include "sc7280.dtsi"
>>> +#include <dt-bindings/gpio/gpio.h>
>>>
>>> / {
>>> model = "Qualcomm Technologies, Inc. sc7280 IDP platform";
>>> @@ -242,6 +243,30 @@
>>> status = "okay";
>>> };
>>>
>>> +&sdhc_1 {
>>> + status = "okay";
>> When I apply your patch I find that your sort order is wrong. "s"
>> comes before "u" and after "q" in the alphabet so "sdhc_1" and
>> "sdhc_2" should sort _after "qupv3_id_0" and before "uart5"
>>
sure will change the order.
>>
>>> + pinctrl-names = "default", "sleep";
>>> + pinctrl-0 = <&sdc1_on>;
>>> + pinctrl-1 = <&sdc1_off>;
>>> +
>>> + vmmc-supply = <&vreg_l7b_2p9>;
>>> + vqmmc-supply = <&vreg_l19b_1p8>;
>>> +};
>>> +
>>> +&sdhc_2 {
>>> + status = "okay";
>>> +
>>> + pinctrl-names = "default","sleep";
>>> + pinctrl-0 = <&sdc2_on>;
>>> + pinctrl-1 = <&sdc2_off>;
>>> +
>>> + vmmc-supply = <&vreg_l9c_2p9>;
>>> + vqmmc-supply = <&vreg_l6c_2p9>;
>>> +
>>> + cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>> Where is the pinctrl for the card detect? Oh, I see it's in
>> "sdc2_on". Probably would be good to break it out since this is
>> board-specific. See below.
>>
okay
>>
>>> +};
>>> +
>>> /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>>>
>>> &qup_uart5_default {
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 8f6b569..69eb064 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -20,6 +20,11 @@
>>>
>>> chosen { };
>>>
>>> + aliases {
>>> + mmc1 = &sdhc_1;
>>> + mmc2 = &sdhc_2;
>>> + };
>>> +
>>> clocks {
>>> xo_board: xo-board {
>>> compatible = "fixed-clock";
>>> @@ -305,6 +310,64 @@
>>> #power-domain-cells = <1>;
>>> };
>>>
>>> + sdhc_1: sdhci@7c4000 {
>>> + compatible = "qcom,sdhci-msm-v5";
>> Please make the compatible:
>> compatible = "qcom,sc7280-sdhci", "qcom,sdhci-msm-v5";
>>
>> ...and add to the bindings. It should be a trivial bindings patch so
>> not too much trouble.
>>
>> NOTE: even though the "qcom,sc7280-sdhci" should be in the bindings
>> and here you _shouldn't_ be adding any code for it. Just let the
>> fallback compatible string ("qcom,sdhci-msm-v5") do its magic. Adding
>> the sc7280 specific version is more of a "just in case we need it
>> later" type thing.
>>
sure
>>
>>> + reg = <0 0x7c4000 0 0x1000>,
>>> + <0 0x7c5000 0 0x1000>;
>>> + reg-names = "hc", "cqhci";
>>> +
>>> + iommus = <&apps_smmu 0xC0 0x0>;
>>> + interrupts = <GIC_SPI 652
>>> IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 656
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> + interrupt-names = "hc_irq", "pwr_irq";
>>> +
>>> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
>>> + <&gcc GCC_SDCC1_AHB_CLK>,
>>> + <&rpmhcc RPMH_CXO_CLK>;
>>> + clock-names = "core", "iface", "xo";
>> I'm curious: why is the "xo" clock needed here but not for sc7180?
> Actually its needed even for sc7180. We are making use of this clock
> in msm_init_cm_dll()
> The default PoR value is also same as calculated value for
> HS200/HS400/SDR104 modes.
> But just not to rely on default register values we need this entry.
>
>>
>>> + interconnects = <&aggre1_noc MASTER_SDCC_1 0
>>> &mc_virt SLAVE_EBI1 0>,
>>> + <&gem_noc MASTER_APPSS_PROC 0
>>> &cnoc2 SLAVE_SDCC_1 0>;
>>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>>> + power-domains = <&rpmhpd SC7280_CX>;
>>> + operating-points-v2 = <&sdhc1_opp_table>;
>>> +
>>> + bus-width = <8>;
>>> + non-removable;
>> This was actually a problem on sc7180 too, but you probably don't want
>> "non-removable" in the SoC file. Board files really should be adding
>> this. Though the SoC might be designed with the idea that this would
>> be used for a non-removable eMMC card I don't know why it wouldn't be
>> possible for someone to hook this up to an external slot and use a
>> GPIO somewhere as a card detect.
>>
sure will move
>>
>>> + supports-cqe;
>>> + no-sd;
>>> + no-sdio;
>> Does the port really not support SD / SDIO, or are you adding these
>> two properties just because on your reference board it's not hooked up
>> to SD/SDIO? What exactly makes it impossible to use SD/SDIO on this
>> port?
>>
> By having this, we can optimize emmc device scan time.
> Driver wont issue SDIO & SDcards specific commands while
> scanning the device.Its little optimization.
> I think board specific dt is right place.
>
>
>>> + max-frequency = <192000000>;
>> Why do you need to specify this?
This helps to avoid lower speed modes running in high clock rate,
and As Veerabhadrarao Badiganti mentioned
This will align clock requests between mmc core layer and sdhci-msm
platform driver. Say, for HS200/HS400 modes of eMMC, mmc-core layer
tries to set clock at 200Mhz, whereas sdhci-msm expects 192Mhz for
these modes. So we have to rely on clock driver floor/ceil values.
By having this property, mmc-core layer itself request for 192Mhz.
Same is for SD card SDR104 mode, core layer expects clock at 208Mhz
whereas sdhci-msm can max operate only at 202Mhz. By having this
property, core layer requests only for 202Mhz for SDR104 mode.
BTW, this helps only for max possible speed modes.
In case of lower-speed modes (for DDR52) we still need to rely on clock
floor rounding.
>>
>>
>>> + qcom,dll-config = <0x0007642c>;
>>> + qcom,ddr-config = <0x80040868>;
>> These magic hex values really have no place being in dts which should
>> have things expressed at a higher level. ...but I guess that ship has
>> sailed and this is in the bindings so I guess we're stuck with them,
>> so I guess they're fine.
sure
>>
>>
>>> + mmc-ddr-1_8v;
>>> + mmc-hs200-1_8v;
>>> + mmc-hs400-1_8v;
>>> + mmc-hs400-enhanced-strobe;
>>> +
>>> + status = "disabled";
>>> +
>>> + sdhc1_opp_table: sdhc1-opp-table {
>>> + compatible = "operating-points-v2";
>>> +
>>> + opp-100000000 {
>>> + opp-hz = /bits/ 64
>>> <100000000>;
>>> + required-opps =
>>> <&rpmhpd_opp_low_svs>;
>>> + opp-peak-kBps = <1200000
>>> 76000>;
>>> + opp-avg-kBps = <1200000
>>> 50000>;
>> Why are the kBps numbers so vastly different than the ones on sc7180
>> for the same OPP point. That implies:
>>
>> a) sc7180 is wrong.
>>
>> b) This patch is wrong.
>>
>> c) The numbers are essentially random and don't really matter.
>>
>> Can you identify which of a), b), or c) is correct, or propose an
>> alternate explanation of the difference?
>>
We calculated bus votes values for both sc7180 and sc7280 with ICB tool,
above mentioned values we got for sc7280.
>>
>>> + };
>>> +
>>> + opp-384000000 {
>>> + opp-hz = /bits/ 64
>>> <384000000>;
>>> + required-opps =
>>> <&rpmhpd_opp_nom>;
>>> + opp-peak-kBps = <5400000
>>> 1600000>;
>>> + opp-avg-kBps = <6000000
>>> 300000>;
>> These opp numbers are also quite different than sc7180
>>
We calculated bus votes values for both sc7180 and sc7280 with ICB tool,
above mentioned values we got for sc7280.
>>
>>> + };
>>> + };
>>> + };
>>> +
>>> qupv3_id_0: geniqup@9c0000 {
>>> compatible = "qcom,geni-se-qup";
>>> reg = <0 0x009c0000 0 0x2000>;
>>> @@ -328,6 +391,54 @@
>>> };
>>> };
>>>
>>> + sdhc_2: sdhci@8804000 {
>>> + compatible = "qcom,sdhci-msm-v5";
>>> + reg = <0 0x08804000 0 0x1000>;
>>> +
>>> + iommus = <&apps_smmu 0x100 0x0>;
>>> + interrupts = <GIC_SPI 207
>>> IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 223
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> + interrupt-names = "hc_irq", "pwr_irq";
>>> +
>>> + clocks = <&gcc GCC_SDCC2_APPS_CLK>,
>>> + <&gcc GCC_SDCC2_AHB_CLK>,
>>> + <&rpmhcc RPMH_CXO_CLK>;
>>> + clock-names = "core", "iface", "xo";
>>> + interconnects = <&aggre1_noc MASTER_SDCC_2 0
>>> &mc_virt SLAVE_EBI1 0>,
>>> + <&gem_noc MASTER_APPSS_PROC 0
>>> &cnoc2 SLAVE_SDCC_2 0>;
>>> + interconnect-names = "sdhc-ddr","cpu-sdhc";
>>> + power-domains = <&rpmhpd SC7280_CX>;
>>> + operating-points-v2 = <&sdhc2_opp_table>;
>>> +
>>> + bus-width = <4>;
>>> +
>>> + no-mmc;
>>> + no-sdio;
>> Similar question to above: why exactly would mmc not work? Are you
>> saying that if someone hooked this up to a full sized SD card slot and
>> placed an MMC card into the slot that it wouldn't work? Similar
>> question about SDIO. If someone placed an external SDIO card into your
>> slot, would it not work?
>>
> As mentioned above, its just to optimize SDcard scan time a little.
>>> + max-frequency = <202000000>;
>> Not needed?
>>
yes it is needed same as above
>>> +
>>> + qcom,dll-config = <0x0007642c>;
>>> +
>>> + status = "disabled";
>>> +
>>> + sdhc2_opp_table: sdhc2-opp-table {
>>> + compatible =
>>> "operating-points-v2";
>>> +
>>> + opp-100000000 {
>>> + opp-hz =/bits/ 64
>>> <100000000>;
>>> + required-opps =
>>> <&rpmhpd_opp_low_svs>;
>>> + opp-peak-kBps =
>>> <1200000 76000>;
>>> + opp-avg-kBps =
>>> <1200000 50000>;
>>> + };
>>> + opp-202000000 {
>> Blank line between the OPPs?
sure
>>
>>> + opp-hz = /bits/ 64
>>> <202000000>;
>>> + required-opps =
>>> <&rpmhpd_opp_nom>;
>>> + opp-peak-kBps =
>>> <3500000 1200000>;
>>> + opp-avg-kBps =
>>> <5000000 100000>;
>>> + };
>> Similar questions about why the OPPs are so vastly different from
>> sc7180.
We calculated bus votes values for both sc7180 and sc7280 with ICB tool,
above mentioned values we got for sc7280.
>>
>>> + };
>>> + };
>>> +
>>> pdc: interrupt-controller@b220000 {
>>> compatible = "qcom,sc7280-pdc", "qcom,pdc";
>>> reg = <0 0x0b220000 0 0x30000>;
>>> @@ -374,6 +485,108 @@
>>> pins = "gpio46", "gpio47";
>>> function = "qup13";
>>> };
>>> +
>>> + sdc1_on: sdc1-on {
>>> + clk {
>>> + pins = "sdc1_clk";
>>> + bias-disable;
>>> + drive-strength = <16>;
>>> + };
>>> +
>>> + cmd {
>>> + pins = "sdc1_cmd";
>>> + bias-pull-up;
>>> + drive-strength = <10>;
>>> + };
>>> +
>>> + data {
>>> + pins = "sdc1_data";
>>> + bias-pull-up;
>>> + drive-strength = <10>;
>>> + };
>>> +
>>> + rclk {
>>> + pins = "sdc1_rclk";
>>> + bias-pull-down;
>>> + };
>> * generally "bias" doesn't belong in the SoC file but instead should
>> be in the board file. Some boards might have external pulls (even if
>> the internal ones would work fine, hardware designers do weird things)
>> and thus might need to disable the internal ones (double pulls are not
>> great).
>>
>> * generally drive-strength doesn't belong in the SoC file but should
>> be in the board file. Different boards with different layouts might
>> need different drive strengths, right?
>>
>> If you remove those two things, I guess there's not actually much left
>> in the SoC dtsi file so I guess move these all to the board file? That
>> seems to be what we ended up with in "qrb5165-rb5.dts" / "sm8250.dtsi"
>> which is an example of a board using the new style of pinctrl for
>> devicetree.
>>
sure
>>
>>> + };
>>> +
>>> + sdc1_off: sdc1-off {
>>> + clk {
>>> + pins = "sdc1_clk";
>>> + bias-disable;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + cmd {
>>> + pins = "sdc1_cmd";
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + data {
>>> + pins = "sdc1_data";
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + rclk {
>>> + pins = "sdc1_rclk";
>>> + bias-pull-down;
>>> + };
>>> + };
>> No need for a sleep state for the rclk since it's the same as the
>> active state, right? NOTE: one way to handle this would be to define
>> one node per pingroup and thus do something like:
>>
>> pinctrl-names = "default", "sleep";
>> pinctrl-0 = <&sdc1_clk>, <&sdc1_cmd>, <&sdc1_data>, <&sdc1_rclk>;
>> pinctrl-1 = <&sdc1_clk_sleep>, <&sdc1_cmd_sleep>, <&sdc1_data_sleep>,
>> <&sdc1_rclk>;
>>
>> I do wish we could avoid having to duplicate the "bias" in every board
>> file. Hrm, I wonder if this could be made simpler by actually putting
>> the "sleep" states in the sc7180.dtsi file (not the board file) and
>> using "bias-bus-hold" to avoid it being board specific?
>>
>> Thus (assuming it works), the total summary would be:
>>
>> 1. Board dts file fully defines "sdc1_clk", "sdc1_cmd", "sdc1_data",
>> "sdc1_rclk", specifying whatever bias and drive strength needed for
>> the board.
>>
>> 2. SoC dtsi fully defines "sdc1_clk_sleep", "sdc1_cmd_sleep",
>> "sdc1_data_sleep", "sdc1_rclk_sleep", specifying drive-strength of 2
>> (for outputs) and "bias-bus-hold" which is OK for all board.
>>
sure
>>
>>> +
>>> + sdc2_on: sdc2-on {
>>> + clk {
>>> + pins = "sdc2_clk";
>>> + bias-disable;
>>> + drive-strength = <16>;
>>> + };
>>> +
>>> + cmd {
>>> + pins = "sdc2_cmd";
>>> + bias-pull-up;
>>> + drive-strength = <10>;
>>> + };
>>> +
>>> + data {
>>> + pins = "sdc2_data";
>>> + bias-pull-up;
>>> + drive-strength = <10>;
>>> + };
>>> +
>>> + sd-cd {
>>> + pins = "gpio91";
>> NOTE: even if we find some reason to keep some of the pinctrl in the
>> SoC dtsi file, the card detect almost certainly needs to move _fully_
>> to the board dts file. Different boards could use a different card
>> detect pin.
>>
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>> Drive strength isn't needed for input pins. Please remove.
sure
>>
>>> + };
>>> + };
>>> +
>>> + sdc2_off: sdc2-off {
>>> + clk {
>>> + pins = "sdc2_clk";
>>> + bias-disable;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + cmd {
>>> + pins = "sdc2_cmd";
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + data {
>>> + pins = "sdc2_data";
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + sd-cd {
>>> + pins = "gpio91";
>>> + bias-pull-up;
>>> + drive-strength = <2>;
>>> + };
>> There's definitely no need for a separate sleep state for the CD line.
okay
>>
>>
>> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-26 6:56 ` sbhanu
@ 2021-03-29 14:56 ` Doug Anderson
2021-04-13 10:59 ` sbhanu
0 siblings, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2021-03-29 14:56 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
Hi,
On Thu, Mar 25, 2021 at 11:57 PM <sbhanu@codeaurora.org> wrote:
>
> >>> + max-frequency = <192000000>;
> >> Why do you need to specify this?
> This helps to avoid lower speed modes running in high clock rate,
> and As Veerabhadrarao Badiganti mentioned
Just to be clear, both Stephen and I agree that you should remove
"max-frequency" here (see previous discussion). Bjorn is, of course,
the file decision maker. However, unless he says "yeah, totally keep
it in" I'd suggest dropping it from the next version.
> >>> + required-opps =
> >>> <&rpmhpd_opp_low_svs>;
> >>> + opp-peak-kBps = <1200000
> >>> 76000>;
> >>> + opp-avg-kBps = <1200000
> >>> 50000>;
> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> for the same OPP point. That implies:
> >>
> >> a) sc7180 is wrong.
> >>
> >> b) This patch is wrong.
> >>
> >> c) The numbers are essentially random and don't really matter.
> >>
> >> Can you identify which of a), b), or c) is correct, or propose an
> >> alternate explanation of the difference?
> >>
>
> We calculated bus votes values for both sc7180 and sc7280 with ICB tool,
> above mentioned values we got for sc7280.
I don't know what an ICB tool is. Please clarify.
Also: just because a tool spits out numbers that doesn't mean it's
correct. Presumably the tool could be wrong or incorrectly configured.
We need to understand why these numbers are different.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-25 16:17 ` Doug Anderson
@ 2021-04-01 9:58 ` sbhanu
0 siblings, 0 replies; 22+ messages in thread
From: sbhanu @ 2021-04-01 9:58 UTC (permalink / raw)
To: Doug Anderson
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
On 2021-03-25 21:47, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 24, 2021 at 8:59 PM Veerabhadrarao Badiganti
> <vbadigan@codeaurora.org> wrote:
>>
>> >> + clocks = <&gcc GCC_SDCC1_APPS_CLK>,
>> >> + <&gcc GCC_SDCC1_AHB_CLK>,
>> >> + <&rpmhcc RPMH_CXO_CLK>;
>> >> + clock-names = "core", "iface", "xo";
>> > I'm curious: why is the "xo" clock needed here but not for sc7180?
>> Actually its needed even for sc7180. We are making use of this clock
>> in
>> msm_init_cm_dll()
>> The default PoR value is also same as calculated value for
>> HS200/HS400/SDR104 modes.
>> But just not to rely on default register values we need this entry.
>
> Can you post a patch for sc7180?
sure will post a patch for sc7180.
>
>
>> >> + bus-width = <4>;
>> >> +
>> >> + no-mmc;
>> >> + no-sdio;
>> > Similar question to above: why exactly would mmc not work? Are you
>> > saying that if someone hooked this up to a full sized SD card slot and
>> > placed an MMC card into the slot that it wouldn't work? Similar
>> > question about SDIO. If someone placed an external SDIO card into your
>> > slot, would it not work?
>> >
>> As mentioned above, its just to optimize SDcard scan time a little.
>
> OK. ...but while the eMMC one can make sense since the eMMC is
> soldered down (but in the board dts file, not in the SoC dtsi file) I
> think you should just remove these for SD card because:
>
> 1. Even if only a uSD slot is exposed it's still _possible_ for
> someone to insert a card that uses MMC or SDIO signaling. If nothing
> else I have a (probably non-compliant) adapter that plugs into a uSD
> slot and provides a full sided SD slot. I could plug an MMC card or
> SDIO card in.
>
> 2. Presumably the SD card scan time optimization is tiny.
>
sure will remove these for SD card and will move eMMC flags(no-sd and
no-sdio) to board dts file.
>
> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-03-29 14:56 ` Doug Anderson
@ 2021-04-13 10:59 ` sbhanu
2021-04-14 20:25 ` Doug Anderson
0 siblings, 1 reply; 22+ messages in thread
From: sbhanu @ 2021-04-13 10:59 UTC (permalink / raw)
To: Doug Anderson
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson
On 2021-03-29 20:26, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 25, 2021 at 11:57 PM <sbhanu@codeaurora.org> wrote:
>>
>> >>> + max-frequency = <192000000>;
>> >> Why do you need to specify this?
>> This helps to avoid lower speed modes running in high clock rate,
>> and As Veerabhadrarao Badiganti mentioned
>
> Just to be clear, both Stephen and I agree that you should remove
> "max-frequency" here (see previous discussion). Bjorn is, of course,
> the file decision maker. However, unless he says "yeah, totally keep
> it in" I'd suggest dropping it from the next version.
>
sure will drop in next version.
>
>> >>> + required-opps =
>> >>> <&rpmhpd_opp_low_svs>;
>> >>> + opp-peak-kBps = <1200000
>> >>> 76000>;
>> >>> + opp-avg-kBps = <1200000
>> >>> 50000>;
>> >> Why are the kBps numbers so vastly different than the ones on sc7180
>> >> for the same OPP point. That implies:
>> >>
>> >> a) sc7180 is wrong.
>> >>
>> >> b) This patch is wrong.
>> >>
>> >> c) The numbers are essentially random and don't really matter.
>> >>
>> >> Can you identify which of a), b), or c) is correct, or propose an
>> >> alternate explanation of the difference?
>> >>
>>
>> We calculated bus votes values for both sc7180 and sc7280 with ICB
>> tool,
>> above mentioned values we got for sc7280.
>
> I don't know what an ICB tool is. Please clarify.
>
> Also: just because a tool spits out numbers that doesn't mean it's
> correct. Presumably the tool could be wrong or incorrectly configured.
> We need to understand why these numbers are different.
>
we checked with ICB tool team on this they conformed as Rennell & Kodiak
are different chipsets,
we might see delta in ib/ab values due to delta in scaling factors.
> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-13 10:59 ` sbhanu
@ 2021-04-14 20:25 ` Doug Anderson
2021-04-16 9:52 ` Georgi Djakov
2021-04-20 17:20 ` sbhanu
0 siblings, 2 replies; 22+ messages in thread
From: Doug Anderson @ 2021-04-14 20:25 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Georgi Djakov
Hi,
On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>
> >> >>> + required-opps =
> >> >>> <&rpmhpd_opp_low_svs>;
> >> >>> + opp-peak-kBps = <1200000
> >> >>> 76000>;
> >> >>> + opp-avg-kBps = <1200000
> >> >>> 50000>;
> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> >> for the same OPP point. That implies:
> >> >>
> >> >> a) sc7180 is wrong.
> >> >>
> >> >> b) This patch is wrong.
> >> >>
> >> >> c) The numbers are essentially random and don't really matter.
> >> >>
> >> >> Can you identify which of a), b), or c) is correct, or propose an
> >> >> alternate explanation of the difference?
> >> >>
> >>
> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
> >> tool,
> >> above mentioned values we got for sc7280.
> >
> > I don't know what an ICB tool is. Please clarify.
> >
> > Also: just because a tool spits out numbers that doesn't mean it's
> > correct. Presumably the tool could be wrong or incorrectly configured.
> > We need to understand why these numbers are different.
> >
> we checked with ICB tool team on this they conformed as Rennell & Kodiak
> are different chipsets,
> we might see delta in ib/ab values due to delta in scaling factors.
...but these numbers are in kbps, aren't they? As I understand it
these aren't supposed to be random numbers spit out by a tool but are
supposed to be understandable by how much bandwidth an IP block (like
MMC) needs from the busses it's connected to. Since the MMC IP block
on sc7180 and sc7280 is roughly the same there shouldn't be a big
difference in numbers.
Something smells wrong.
Adding a few people who understand interconnects better than I do, though.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-14 20:25 ` Doug Anderson
@ 2021-04-16 9:52 ` Georgi Djakov
2021-04-20 17:20 ` sbhanu
1 sibling, 0 replies; 22+ messages in thread
From: Georgi Djakov @ 2021-04-16 9:52 UTC (permalink / raw)
To: Doug Anderson, Shaik Sajida Bhanu
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, okukatla
Hi,
On 14.04.21 23:25, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>>
>>>>>>> + required-opps =
>>>>>>> <&rpmhpd_opp_low_svs>;
>>>>>>> + opp-peak-kBps = <1200000
>>>>>>> 76000>;
>>>>>>> + opp-avg-kBps = <1200000
>>>>>>> 50000>;
>>>>>> Why are the kBps numbers so vastly different than the ones on sc7180
>>>>>> for the same OPP point. That implies:
>>>>>>
>>>>>> a) sc7180 is wrong.
>>>>>>
>>>>>> b) This patch is wrong.
>>>>>>
>>>>>> c) The numbers are essentially random and don't really matter.
>>>>>>
>>>>>> Can you identify which of a), b), or c) is correct, or propose an
>>>>>> alternate explanation of the difference?
>>>>>>
>>>>
>>>> We calculated bus votes values for both sc7180 and sc7280 with ICB
>>>> tool,
>>>> above mentioned values we got for sc7280.
>>>
>>> I don't know what an ICB tool is. Please clarify.
>>>
>>> Also: just because a tool spits out numbers that doesn't mean it's
>>> correct. Presumably the tool could be wrong or incorrectly configured.
>>> We need to understand why these numbers are different.
>>>
>> we checked with ICB tool team on this they conformed as Rennell & Kodiak
>> are different chipsets,
>> we might see delta in ib/ab values due to delta in scaling factors.
If the scaling factor is different, maybe this should be reflected
in the BCM data, where we have the following:
@vote_scale: scaling factor for vote_x and vote_y
This is 1000 by default, but maybe we should set it to some
different value for some of the BCMs?
I'm adding Odelu, who is more familiar with this platform.
>
> ...but these numbers are in kbps, aren't they? As I understand it
> these aren't supposed to be random numbers spit out by a tool but are
> supposed to be understandable by how much bandwidth an IP block (like
> MMC) needs from the busses it's connected to. Since the MMC IP block
> on sc7180 and sc7280 is roughly the same there shouldn't be a big
> difference in numbers.
>
> Something smells wrong.
>
> Adding a few people who understand interconnects better than I do, though.
>
Thanks!
Georgi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-14 20:25 ` Doug Anderson
2021-04-16 9:52 ` Georgi Djakov
@ 2021-04-20 17:20 ` sbhanu
2021-04-20 20:14 ` Doug Anderson
1 sibling, 1 reply; 22+ messages in thread
From: sbhanu @ 2021-04-20 17:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Georgi Djakov
On 2021-04-15 01:55, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>>
>> >> >>> + required-opps =
>> >> >>> <&rpmhpd_opp_low_svs>;
>> >> >>> + opp-peak-kBps = <1200000
>> >> >>> 76000>;
>> >> >>> + opp-avg-kBps = <1200000
>> >> >>> 50000>;
>> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
>> >> >> for the same OPP point. That implies:
>> >> >>
>> >> >> a) sc7180 is wrong.
>> >> >>
>> >> >> b) This patch is wrong.
>> >> >>
>> >> >> c) The numbers are essentially random and don't really matter.
>> >> >>
>> >> >> Can you identify which of a), b), or c) is correct, or propose an
>> >> >> alternate explanation of the difference?
>> >> >>
>> >>
>> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
>> >> tool,
>> >> above mentioned values we got for sc7280.
>> >
>> > I don't know what an ICB tool is. Please clarify.
>> >
>> > Also: just because a tool spits out numbers that doesn't mean it's
>> > correct. Presumably the tool could be wrong or incorrectly configured.
>> > We need to understand why these numbers are different.
>> >
>> we checked with ICB tool team on this they conformed as Rennell &
>> Kodiak
>> are different chipsets,
>> we might see delta in ib/ab values due to delta in scaling factors.
>
> ...but these numbers are in kbps, aren't they? As I understand it
> these aren't supposed to be random numbers spit out by a tool but are
> supposed to be understandable by how much bandwidth an IP block (like
> MMC) needs from the busses it's connected to. Since the MMC IP block
> on sc7180 and sc7280 is roughly the same there shouldn't be a big
> difference in numbers.
>
> Something smells wrong.
>
> Adding a few people who understand interconnects better than I do,
> though.
>
ICB team has re-checked the Rennell ICB tool and they confirmed that
some configs were wrong in Rennell ICB tool and they corrected it.With
the new updated Rennell ICB tool below are the values :
Rennell LC:(Sc7180)
opp-384000000 {
opp-hz = /bits/ 64 <384000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <5400000 490000>;
opp-avg-kBps = <6600000 300000>;
};
And now, these values are near to Kodaik LC values:
Kodaik LC:(SC7280)
opp-384000000 {
opp-hz = /bits/ 64 <384000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <5400000 399000>;
opp-avg-kBps = <6000000 300000>;
};
> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-20 17:20 ` sbhanu
@ 2021-04-20 20:14 ` Doug Anderson
2021-04-28 10:47 ` sbhanu
0 siblings, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2021-04-20 20:14 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Georgi Djakov,
Odelu Kukatla
Hi,
On Tue, Apr 20, 2021 at 10:21 AM <sbhanu@codeaurora.org> wrote:
>
> On 2021-04-15 01:55, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
> >>
> >> >> >>> + required-opps =
> >> >> >>> <&rpmhpd_opp_low_svs>;
> >> >> >>> + opp-peak-kBps = <1200000
> >> >> >>> 76000>;
> >> >> >>> + opp-avg-kBps = <1200000
> >> >> >>> 50000>;
> >> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> >> >> for the same OPP point. That implies:
> >> >> >>
> >> >> >> a) sc7180 is wrong.
> >> >> >>
> >> >> >> b) This patch is wrong.
> >> >> >>
> >> >> >> c) The numbers are essentially random and don't really matter.
> >> >> >>
> >> >> >> Can you identify which of a), b), or c) is correct, or propose an
> >> >> >> alternate explanation of the difference?
> >> >> >>
> >> >>
> >> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
> >> >> tool,
> >> >> above mentioned values we got for sc7280.
> >> >
> >> > I don't know what an ICB tool is. Please clarify.
> >> >
> >> > Also: just because a tool spits out numbers that doesn't mean it's
> >> > correct. Presumably the tool could be wrong or incorrectly configured.
> >> > We need to understand why these numbers are different.
> >> >
> >> we checked with ICB tool team on this they conformed as Rennell &
> >> Kodiak
> >> are different chipsets,
> >> we might see delta in ib/ab values due to delta in scaling factors.
> >
> > ...but these numbers are in kbps, aren't they? As I understand it
> > these aren't supposed to be random numbers spit out by a tool but are
> > supposed to be understandable by how much bandwidth an IP block (like
> > MMC) needs from the busses it's connected to. Since the MMC IP block
> > on sc7180 and sc7280 is roughly the same there shouldn't be a big
> > difference in numbers.
> >
> > Something smells wrong.
> >
> > Adding a few people who understand interconnects better than I do,
> > though.
> >
>
> ICB team has re-checked the Rennell ICB tool and they confirmed that
> some configs were wrong in Rennell ICB tool and they corrected it.With
> the new updated Rennell ICB tool below are the values :
>
>
> Rennell LC:(Sc7180)
>
> opp-384000000 {
> opp-hz = /bits/ 64 <384000000>;
> required-opps = <&rpmhpd_opp_nom>;
> opp-peak-kBps = <5400000 490000>;
> opp-avg-kBps = <6600000 300000>;
> };
>
>
> And now, these values are near to Kodaik LC values:
>
> Kodaik LC:(SC7280)
>
> opp-384000000 {
> opp-hz = /bits/ 64 <384000000>;
> required-opps = <&rpmhpd_opp_nom>;
> opp-peak-kBps = <5400000 399000>;
> opp-avg-kBps = <6000000 300000>;
> };
This still isn't making sense to me.
* sc7180 and sc7280 are running at the same speed. I'm glad the
numbers are closer now, but I would have thought they'd be exactly the
same.
* Aren't these supposed to be sensible? This is eMMC that does max
transfer rates of 400 megabytes / second to the external device. You
have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes /
second = 5400 megabytes / second. I can imagine there being some
overhead where an internal bus might need to be faster but that seems
excessive. This is 13.5x!
* I can't see how it can make sense that "average" values are higher
than "peak" values.
It still feels like there's a misconfiguration somewhere.
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-20 20:14 ` Doug Anderson
@ 2021-04-28 10:47 ` sbhanu
2021-04-28 15:13 ` Doug Anderson
0 siblings, 1 reply; 22+ messages in thread
From: sbhanu @ 2021-04-28 10:47 UTC (permalink / raw)
To: Doug Anderson
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Georgi Djakov,
Odelu Kukatla
On 2021-04-21 01:44, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 20, 2021 at 10:21 AM <sbhanu@codeaurora.org> wrote:
>>
>> On 2021-04-15 01:55, Doug Anderson wrote:
>> > Hi,
>> >
>> > On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>> >>
>> >> >> >>> + required-opps =
>> >> >> >>> <&rpmhpd_opp_low_svs>;
>> >> >> >>> + opp-peak-kBps = <1200000
>> >> >> >>> 76000>;
>> >> >> >>> + opp-avg-kBps = <1200000
>> >> >> >>> 50000>;
>> >> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
>> >> >> >> for the same OPP point. That implies:
>> >> >> >>
>> >> >> >> a) sc7180 is wrong.
>> >> >> >>
>> >> >> >> b) This patch is wrong.
>> >> >> >>
>> >> >> >> c) The numbers are essentially random and don't really matter.
>> >> >> >>
>> >> >> >> Can you identify which of a), b), or c) is correct, or propose an
>> >> >> >> alternate explanation of the difference?
>> >> >> >>
>> >> >>
>> >> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
>> >> >> tool,
>> >> >> above mentioned values we got for sc7280.
>> >> >
>> >> > I don't know what an ICB tool is. Please clarify.
>> >> >
>> >> > Also: just because a tool spits out numbers that doesn't mean it's
>> >> > correct. Presumably the tool could be wrong or incorrectly configured.
>> >> > We need to understand why these numbers are different.
>> >> >
>> >> we checked with ICB tool team on this they conformed as Rennell &
>> >> Kodiak
>> >> are different chipsets,
>> >> we might see delta in ib/ab values due to delta in scaling factors.
>> >
>> > ...but these numbers are in kbps, aren't they? As I understand it
>> > these aren't supposed to be random numbers spit out by a tool but are
>> > supposed to be understandable by how much bandwidth an IP block (like
>> > MMC) needs from the busses it's connected to. Since the MMC IP block
>> > on sc7180 and sc7280 is roughly the same there shouldn't be a big
>> > difference in numbers.
>> >
>> > Something smells wrong.
>> >
>> > Adding a few people who understand interconnects better than I do,
>> > though.
>> >
>>
>> ICB team has re-checked the Rennell ICB tool and they confirmed that
>> some configs were wrong in Rennell ICB tool and they corrected it.With
>> the new updated Rennell ICB tool below are the values :
>>
>>
>> Rennell LC:(Sc7180)
>>
>> opp-384000000 {
>> opp-hz = /bits/ 64 <384000000>;
>> required-opps = <&rpmhpd_opp_nom>;
>> opp-peak-kBps = <5400000 490000>;
>> opp-avg-kBps = <6600000 300000>;
>> };
>>
>>
>> And now, these values are near to Kodaik LC values:
>>
>> Kodaik LC:(SC7280)
>>
>> opp-384000000 {
>> opp-hz = /bits/ 64 <384000000>;
>> required-opps = <&rpmhpd_opp_nom>;
>> opp-peak-kBps = <5400000 399000>;
>> opp-avg-kBps = <6000000 300000>;
>> };
>
> This still isn't making sense to me.
>
> * sc7180 and sc7280 are running at the same speed. I'm glad the
> numbers are closer now, but I would have thought they'd be exactly the
> same.
>
> * Aren't these supposed to be sensible? This is eMMC that does max
> transfer rates of 400 megabytes / second to the external device. You
> have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes /
> second = 5400 megabytes / second. I can imagine there being some
> overhead where an internal bus might need to be faster but that seems
> excessive. This is 13.5x!
>
These numbers are not related to SDCC bandwidth, these are the values
needed for the NOC's to run in nominal voltage corners (internal to
hardware) and
thus it helps SDCC to run in nominal to get required through put
(384MBps).So above calculation mentioned by you is not applicable here.
> * I can't see how it can make sense that "average" values are higher
> than "peak" values.
Here actual peak = peak number * 2
actual average = average number
and this multiplication is taken care by ICC driver, so technically
actual peak is still high than average.
>
> It still feels like there's a misconfiguration somewhere.
>
> -Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-28 10:47 ` sbhanu
@ 2021-04-28 15:13 ` Doug Anderson
2021-04-29 20:44 ` Georgi Djakov
0 siblings, 1 reply; 22+ messages in thread
From: Doug Anderson @ 2021-04-28 15:13 UTC (permalink / raw)
To: Shaik Sajida Bhanu, Georgi Djakov
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Odelu Kukatla
Hi,
On Wed, Apr 28, 2021 at 3:47 AM <sbhanu@codeaurora.org> wrote:
>
> On 2021-04-21 01:44, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 20, 2021 at 10:21 AM <sbhanu@codeaurora.org> wrote:
> >>
> >> On 2021-04-15 01:55, Doug Anderson wrote:
> >> > Hi,
> >> >
> >> > On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
> >> >>
> >> >> >> >>> + required-opps =
> >> >> >> >>> <&rpmhpd_opp_low_svs>;
> >> >> >> >>> + opp-peak-kBps = <1200000
> >> >> >> >>> 76000>;
> >> >> >> >>> + opp-avg-kBps = <1200000
> >> >> >> >>> 50000>;
> >> >> >> >> Why are the kBps numbers so vastly different than the ones on sc7180
> >> >> >> >> for the same OPP point. That implies:
> >> >> >> >>
> >> >> >> >> a) sc7180 is wrong.
> >> >> >> >>
> >> >> >> >> b) This patch is wrong.
> >> >> >> >>
> >> >> >> >> c) The numbers are essentially random and don't really matter.
> >> >> >> >>
> >> >> >> >> Can you identify which of a), b), or c) is correct, or propose an
> >> >> >> >> alternate explanation of the difference?
> >> >> >> >>
> >> >> >>
> >> >> >> We calculated bus votes values for both sc7180 and sc7280 with ICB
> >> >> >> tool,
> >> >> >> above mentioned values we got for sc7280.
> >> >> >
> >> >> > I don't know what an ICB tool is. Please clarify.
> >> >> >
> >> >> > Also: just because a tool spits out numbers that doesn't mean it's
> >> >> > correct. Presumably the tool could be wrong or incorrectly configured.
> >> >> > We need to understand why these numbers are different.
> >> >> >
> >> >> we checked with ICB tool team on this they conformed as Rennell &
> >> >> Kodiak
> >> >> are different chipsets,
> >> >> we might see delta in ib/ab values due to delta in scaling factors.
> >> >
> >> > ...but these numbers are in kbps, aren't they? As I understand it
> >> > these aren't supposed to be random numbers spit out by a tool but are
> >> > supposed to be understandable by how much bandwidth an IP block (like
> >> > MMC) needs from the busses it's connected to. Since the MMC IP block
> >> > on sc7180 and sc7280 is roughly the same there shouldn't be a big
> >> > difference in numbers.
> >> >
> >> > Something smells wrong.
> >> >
> >> > Adding a few people who understand interconnects better than I do,
> >> > though.
> >> >
> >>
> >> ICB team has re-checked the Rennell ICB tool and they confirmed that
> >> some configs were wrong in Rennell ICB tool and they corrected it.With
> >> the new updated Rennell ICB tool below are the values :
> >>
> >>
> >> Rennell LC:(Sc7180)
> >>
> >> opp-384000000 {
> >> opp-hz = /bits/ 64 <384000000>;
> >> required-opps = <&rpmhpd_opp_nom>;
> >> opp-peak-kBps = <5400000 490000>;
> >> opp-avg-kBps = <6600000 300000>;
> >> };
> >>
> >>
> >> And now, these values are near to Kodaik LC values:
> >>
> >> Kodaik LC:(SC7280)
> >>
> >> opp-384000000 {
> >> opp-hz = /bits/ 64 <384000000>;
> >> required-opps = <&rpmhpd_opp_nom>;
> >> opp-peak-kBps = <5400000 399000>;
> >> opp-avg-kBps = <6000000 300000>;
> >> };
> >
> > This still isn't making sense to me.
> >
> > * sc7180 and sc7280 are running at the same speed. I'm glad the
> > numbers are closer now, but I would have thought they'd be exactly the
> > same.
> >
> > * Aren't these supposed to be sensible? This is eMMC that does max
> > transfer rates of 400 megabytes / second to the external device. You
> > have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes /
> > second = 5400 megabytes / second. I can imagine there being some
> > overhead where an internal bus might need to be faster but that seems
> > excessive. This is 13.5x!
> >
>
> These numbers are not related to SDCC bandwidth, these are the values
> needed for the NOC's to run in nominal voltage corners (internal to
> hardware) and
> thus it helps SDCC to run in nominal to get required through put
> (384MBps).So above calculation mentioned by you is not applicable here.
OK. I guess if everyone else understands this and it's just me that
doesn't then I won't stand in the way. In general, though, the device
tree is supposed to be describing the hardware in a way that makes
sense on its own. It's not a place to just dump in magic numbers.
These numbers must be somehow related to the transfer rate of the SD
card since otherwise they wouldn't scale up with faster card clocks.
Given that these numbers are expressed in "kBps" (since you're storing
them in a property that has "kBps" in the name), I would expect that
these numbers are expressing some type of bandwidth. I still haven't
really understood why you have to scale some bandwidth at over 10x the
card clock speed.
Said another way: you're saying that you need these numbers because
they make a whole bunch of math work out. I'm saying that these aren't
just supposed to be magic numbers. They're supposed to make sense on
their own and you should be able to describe to me how you arrived at
these numbers in a way that I could do the math on my own. Saying "we
plugged this into some program and it spit out these numbers" isn't
good enough.
> > * I can't see how it can make sense that "average" values are higher
> > than "peak" values.
>
>
> Here actual peak = peak number * 2
> actual average = average number
>
> and this multiplication is taken care by ICC driver, so technically
> actual peak is still high than average.
Sorry, but that is really counter-intuitive. Georgi: is that how this
is normally expected to work?
-Doug
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-28 15:13 ` Doug Anderson
@ 2021-04-29 20:44 ` Georgi Djakov
2021-06-01 9:58 ` sbhanu
0 siblings, 1 reply; 22+ messages in thread
From: Georgi Djakov @ 2021-04-29 20:44 UTC (permalink / raw)
To: Doug Anderson, Shaik Sajida Bhanu
Cc: Veerabhadrarao Badiganti, Adrian Hunter, Ulf Hansson,
Rob Herring, Asutosh Das, Sahitya Tummala, Ram Prakash Gupta,
Sayali Lokhande, sartgarg, Rajendra Nayak, Sai Prakash Ranjan,
Sibi Sankar, cang, pragalla, nitirawa, Linux MMC List, LKML,
linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Odelu Kukatla
On 28.04.21 18:13, Doug Anderson wrote:
> Hi,
>
> On Wed, Apr 28, 2021 at 3:47 AM <sbhanu@codeaurora.org> wrote:
>>
>> On 2021-04-21 01:44, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Apr 20, 2021 at 10:21 AM <sbhanu@codeaurora.org> wrote:
>>>>
>>>> On 2021-04-15 01:55, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>>>>>>
>>>>>>>>>>> + required-opps =
>>>>>>>>>>> <&rpmhpd_opp_low_svs>;
>>>>>>>>>>> + opp-peak-kBps = <1200000
>>>>>>>>>>> 76000>;
>>>>>>>>>>> + opp-avg-kBps = <1200000
>>>>>>>>>>> 50000>;
>>>>>>>>>> Why are the kBps numbers so vastly different than the ones on sc7180
>>>>>>>>>> for the same OPP point. That implies:
>>>>>>>>>>
>>>>>>>>>> a) sc7180 is wrong.
>>>>>>>>>>
>>>>>>>>>> b) This patch is wrong.
>>>>>>>>>>
>>>>>>>>>> c) The numbers are essentially random and don't really matter.
>>>>>>>>>>
>>>>>>>>>> Can you identify which of a), b), or c) is correct, or propose an
>>>>>>>>>> alternate explanation of the difference?
>>>>>>>>>>
>>>>>>>>
>>>>>>>> We calculated bus votes values for both sc7180 and sc7280 with ICB
>>>>>>>> tool,
>>>>>>>> above mentioned values we got for sc7280.
>>>>>>>
>>>>>>> I don't know what an ICB tool is. Please clarify.
>>>>>>>
>>>>>>> Also: just because a tool spits out numbers that doesn't mean it's
>>>>>>> correct. Presumably the tool could be wrong or incorrectly configured.
>>>>>>> We need to understand why these numbers are different.
>>>>>>>
>>>>>> we checked with ICB tool team on this they conformed as Rennell &
>>>>>> Kodiak
>>>>>> are different chipsets,
>>>>>> we might see delta in ib/ab values due to delta in scaling factors.
>>>>>
>>>>> ...but these numbers are in kbps, aren't they? As I understand it
>>>>> these aren't supposed to be random numbers spit out by a tool but are
>>>>> supposed to be understandable by how much bandwidth an IP block (like
>>>>> MMC) needs from the busses it's connected to. Since the MMC IP block
>>>>> on sc7180 and sc7280 is roughly the same there shouldn't be a big
>>>>> difference in numbers.
>>>>>
>>>>> Something smells wrong.
>>>>>
>>>>> Adding a few people who understand interconnects better than I do,
>>>>> though.
>>>>>
>>>>
>>>> ICB team has re-checked the Rennell ICB tool and they confirmed that
>>>> some configs were wrong in Rennell ICB tool and they corrected it.With
>>>> the new updated Rennell ICB tool below are the values :
>>>>
>>>>
>>>> Rennell LC:(Sc7180)
>>>>
>>>> opp-384000000 {
>>>> opp-hz = /bits/ 64 <384000000>;
>>>> required-opps = <&rpmhpd_opp_nom>;
>>>> opp-peak-kBps = <5400000 490000>;
>>>> opp-avg-kBps = <6600000 300000>;
>>>> };
>>>>
>>>>
>>>> And now, these values are near to Kodaik LC values:
>>>>
>>>> Kodaik LC:(SC7280)
>>>>
>>>> opp-384000000 {
>>>> opp-hz = /bits/ 64 <384000000>;
>>>> required-opps = <&rpmhpd_opp_nom>;
>>>> opp-peak-kBps = <5400000 399000>;
>>>> opp-avg-kBps = <6000000 300000>;
>>>> };
>>>
>>> This still isn't making sense to me.
>>>
>>> * sc7180 and sc7280 are running at the same speed. I'm glad the
>>> numbers are closer now, but I would have thought they'd be exactly the
>>> same.
>>>
>>> * Aren't these supposed to be sensible? This is eMMC that does max
>>> transfer rates of 400 megabytes / second to the external device. You
>>> have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes /
>>> second = 5400 megabytes / second. I can imagine there being some
>>> overhead where an internal bus might need to be faster but that seems
>>> excessive. This is 13.5x!
>>>
>>
>> These numbers are not related to SDCC bandwidth, these are the values
>> needed for the NOC's to run in nominal voltage corners (internal to
>> hardware) and
>> thus it helps SDCC to run in nominal to get required through put
>> (384MBps).So above calculation mentioned by you is not applicable here.
>
> OK. I guess if everyone else understands this and it's just me that
> doesn't then I won't stand in the way. In general, though, the device
> tree is supposed to be describing the hardware in a way that makes
> sense on its own. It's not a place to just dump in magic numbers.
> These numbers must be somehow related to the transfer rate of the SD
> card since otherwise they wouldn't scale up with faster card clocks.
> Given that these numbers are expressed in "kBps" (since you're storing
> them in a property that has "kBps" in the name), I would expect that
> these numbers are expressing some type of bandwidth. I still haven't
> really understood why you have to scale some bandwidth at over 10x the
> card clock speed.
>
> Said another way: you're saying that you need these numbers because
> they make a whole bunch of math work out. I'm saying that these aren't
> just supposed to be magic numbers. They're supposed to make sense on
> their own and you should be able to describe to me how you arrived at
> these numbers in a way that I could do the math on my own. Saying "we
> plugged this into some program and it spit out these numbers" isn't
> good enough.
Agree.
>>> * I can't see how it can make sense that "average" values are higher
>>> than "peak" values.
>>
>>
>> Here actual peak = peak number * 2
>> actual average = average number
>>
>> and this multiplication is taken care by ICC driver, so technically
>> actual peak is still high than average.
>
> Sorry, but that is really counter-intuitive. Georgi: is that how this
> is normally expected to work?
Average bandwidth being higher than peak does not make sense to me.
The numbers in DT should reflect the real bandwidth that is being
requested. If some links between nodes consist of multiple channels,
or there is anything specific to the topology or the hardware platform
(scaling factors, buswidth, etc), this should be handled in the
interconnect provider driver. The goal is to use bandwidth values and
not magic numbers.
Thanks,
Georgi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
2021-04-29 20:44 ` Georgi Djakov
@ 2021-06-01 9:58 ` sbhanu
0 siblings, 0 replies; 22+ messages in thread
From: sbhanu @ 2021-06-01 9:58 UTC (permalink / raw)
To: Georgi Djakov
Cc: Doug Anderson, Veerabhadrarao Badiganti, Adrian Hunter,
Ulf Hansson, Rob Herring, Asutosh Das, Sahitya Tummala,
Ram Prakash Gupta, Sayali Lokhande, sartgarg, Rajendra Nayak,
Sai Prakash Ranjan, Sibi Sankar, cang, pragalla, nitirawa,
Linux MMC List, LKML, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Andy Gross, Bjorn Andersson, Evan Green, Odelu Kukatla
On 2021-04-30 02:14, Georgi Djakov wrote:
> On 28.04.21 18:13, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, Apr 28, 2021 at 3:47 AM <sbhanu@codeaurora.org> wrote:
>>>
>>> On 2021-04-21 01:44, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Tue, Apr 20, 2021 at 10:21 AM <sbhanu@codeaurora.org> wrote:
>>>>>
>>>>> On 2021-04-15 01:55, Doug Anderson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Apr 13, 2021 at 3:59 AM <sbhanu@codeaurora.org> wrote:
>>>>>>>
>>>>>>>>>>>> + required-opps =
>>>>>>>>>>>> <&rpmhpd_opp_low_svs>;
>>>>>>>>>>>> + opp-peak-kBps =
>>>>>>>>>>>> <1200000
>>>>>>>>>>>> 76000>;
>>>>>>>>>>>> + opp-avg-kBps =
>>>>>>>>>>>> <1200000
>>>>>>>>>>>> 50000>;
>>>>>>>>>>> Why are the kBps numbers so vastly different than the ones on
>>>>>>>>>>> sc7180
>>>>>>>>>>> for the same OPP point. That implies:
>>>>>>>>>>>
>>>>>>>>>>> a) sc7180 is wrong.
>>>>>>>>>>>
>>>>>>>>>>> b) This patch is wrong.
>>>>>>>>>>>
>>>>>>>>>>> c) The numbers are essentially random and don't really
>>>>>>>>>>> matter.
>>>>>>>>>>>
>>>>>>>>>>> Can you identify which of a), b), or c) is correct, or
>>>>>>>>>>> propose an
>>>>>>>>>>> alternate explanation of the difference?
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We calculated bus votes values for both sc7180 and sc7280 with
>>>>>>>>> ICB
>>>>>>>>> tool,
>>>>>>>>> above mentioned values we got for sc7280.
>>>>>>>>
>>>>>>>> I don't know what an ICB tool is. Please clarify.
>>>>>>>>
>>>>>>>> Also: just because a tool spits out numbers that doesn't mean
>>>>>>>> it's
>>>>>>>> correct. Presumably the tool could be wrong or incorrectly
>>>>>>>> configured.
>>>>>>>> We need to understand why these numbers are different.
>>>>>>>>
>>>>>>> we checked with ICB tool team on this they conformed as Rennell &
>>>>>>> Kodiak
>>>>>>> are different chipsets,
>>>>>>> we might see delta in ib/ab values due to delta in scaling
>>>>>>> factors.
>>>>>>
>>>>>> ...but these numbers are in kbps, aren't they? As I understand it
>>>>>> these aren't supposed to be random numbers spit out by a tool but
>>>>>> are
>>>>>> supposed to be understandable by how much bandwidth an IP block
>>>>>> (like
>>>>>> MMC) needs from the busses it's connected to. Since the MMC IP
>>>>>> block
>>>>>> on sc7180 and sc7280 is roughly the same there shouldn't be a big
>>>>>> difference in numbers.
>>>>>>
>>>>>> Something smells wrong.
>>>>>>
>>>>>> Adding a few people who understand interconnects better than I do,
>>>>>> though.
>>>>>>
>>>>>
>>>>> ICB team has re-checked the Rennell ICB tool and they confirmed
>>>>> that
>>>>> some configs were wrong in Rennell ICB tool and they corrected
>>>>> it.With
>>>>> the new updated Rennell ICB tool below are the values :
>>>>>
>>>>>
>>>>> Rennell LC:(Sc7180)
>>>>>
>>>>> opp-384000000 {
>>>>> opp-hz = /bits/ 64 <384000000>;
>>>>> required-opps = <&rpmhpd_opp_nom>;
>>>>> opp-peak-kBps = <5400000 490000>;
>>>>> opp-avg-kBps = <6600000 300000>;
>>>>> };
>>>>>
>>>>>
>>>>> And now, these values are near to Kodaik LC values:
>>>>>
>>>>> Kodaik LC:(SC7280)
>>>>>
>>>>> opp-384000000 {
>>>>> opp-hz = /bits/ 64 <384000000>;
>>>>> required-opps = <&rpmhpd_opp_nom>;
>>>>> opp-peak-kBps = <5400000 399000>;
>>>>> opp-avg-kBps = <6000000 300000>;
>>>>> };
>>>>
>>>> This still isn't making sense to me.
>>>>
>>>> * sc7180 and sc7280 are running at the same speed. I'm glad the
>>>> numbers are closer now, but I would have thought they'd be exactly
>>>> the
>>>> same.
>>>>
>>>> * Aren't these supposed to be sensible? This is eMMC that does max
>>>> transfer rates of 400 megabytes / second to the external device. You
>>>> have bandwidths listed here of 5,400,000 kBps = 5,400,000 kilobytes
>>>> /
>>>> second = 5400 megabytes / second. I can imagine there being some
>>>> overhead where an internal bus might need to be faster but that
>>>> seems
>>>> excessive. This is 13.5x!
>>>>
>>>
>>> These numbers are not related to SDCC bandwidth, these are the values
>>> needed for the NOC's to run in nominal voltage corners (internal to
>>> hardware) and
>>> thus it helps SDCC to run in nominal to get required through put
>>> (384MBps).So above calculation mentioned by you is not applicable
>>> here.
>>
>> OK. I guess if everyone else understands this and it's just me that
>> doesn't then I won't stand in the way. In general, though, the device
>> tree is supposed to be describing the hardware in a way that makes
>> sense on its own. It's not a place to just dump in magic numbers.
>> These numbers must be somehow related to the transfer rate of the SD
>> card since otherwise they wouldn't scale up with faster card clocks.
>> Given that these numbers are expressed in "kBps" (since you're storing
>> them in a property that has "kBps" in the name), I would expect that
>> these numbers are expressing some type of bandwidth. I still haven't
>> really understood why you have to scale some bandwidth at over 10x the
>> card clock speed.
>>
>> Said another way: you're saying that you need these numbers because
>> they make a whole bunch of math work out. I'm saying that these aren't
>> just supposed to be magic numbers. They're supposed to make sense on
>> their own and you should be able to describe to me how you arrived at
>> these numbers in a way that I could do the math on my own. Saying "we
>> plugged this into some program and it spit out these numbers" isn't
>> good enough.
>
> Agree.
>
Peak bandwidth is an instantaneous bandwidth used as a floor vote to
take care
of latency (in this case for DDR). It is a mechanism to vote for floor
frequency to counter latency as opposed to an actual bandwidth
requirement.
So a client could say I need the clock to run @200 MHz and simply take
the
bus width times the frequency as the required peak bandwidth vote
(peak bandwidth vote = 200 * bus_width ) and vote for it.
So, we are passing peak bandwidth votes for DDR and CNOC for nom
frequencies
from device tree.
SDCC clocks are running at Nom. frequenices, which are power driven by
the cx rail.
The same cx rail will also power driven to DDR clocks too.
So, DDR clocks can also scale upto to Nom. frequenices without any extra
power drawing
by the cx rail. This will helps to get optimal performance.
So, on doing the math with DDR Nom. frequency (1.3GHz) and also
considering the Gerogi points
[If some links between nodes consist of multiple channels,
or there is anything specific to the topology or the hardware platform
(scaling factors, buswidth, etc), this should be handled in the
interconnect provider driver.] we will get values close to 5400000KBps.
same applies to CNOC config path Nom. frequency (403MHz), we will get
values close to 1600000KBps.
math used :
peak bandwidth = minimum DDR * effective width; //4bytes for DDR for
SC7280.
average bandwidth = is the actual throughput requirement.
By considering the above points, the new b/w values looks as below.
opp-384000000 {
opp-hz = /bits/ 64 <384000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <5400000 1600000>;
opp-avg-kBps = <390000 0>;
};
similarly for 100Mhz
opp-100000000 {
opp-hz = /bits/ 64 <100000000>;
required-opps = <&rpmhpd_opp_low_svs>;
opp-peak-kBps = <1800000 400000>;
opp-avg-kBps = <100000 0>;
};
>>>> * I can't see how it can make sense that "average" values are higher
>>>> than "peak" values.
>>>
>>>
>>> Here actual peak = peak number * 2
>>> actual average = average number
>>>
>>> and this multiplication is taken care by ICC driver, so technically
>>> actual peak is still high than average.
>>
>> Sorry, but that is really counter-intuitive. Georgi: is that how this
>> is normally expected to work?
>
> Average bandwidth being higher than peak does not make sense to me.
> The numbers in DT should reflect the real bandwidth that is being
> requested. If some links between nodes consist of multiple channels,
> or there is anything specific to the topology or the hardware platform
> (scaling factors, buswidth, etc), this should be handled in the
> interconnect provider driver. The goal is to use bandwidth values and
> not magic numbers.
>
> Thanks,
> Georgi
Sure, we will update this Average bandwidth vote values.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-06-01 9:58 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 18:17 [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card Shaik Sajida Bhanu
2021-03-23 7:01 ` Stephen Boyd
2021-03-24 15:23 ` sbhanu
2021-03-24 15:57 ` Stephen Boyd
2021-03-24 16:28 ` Stephen Boyd
2021-03-25 3:36 ` Veerabhadrarao Badiganti
2021-03-25 16:20 ` Doug Anderson
2021-03-23 16:11 ` Doug Anderson
2021-03-25 3:58 ` Veerabhadrarao Badiganti
2021-03-25 16:17 ` Doug Anderson
2021-04-01 9:58 ` sbhanu
2021-03-26 6:56 ` sbhanu
2021-03-29 14:56 ` Doug Anderson
2021-04-13 10:59 ` sbhanu
2021-04-14 20:25 ` Doug Anderson
2021-04-16 9:52 ` Georgi Djakov
2021-04-20 17:20 ` sbhanu
2021-04-20 20:14 ` Doug Anderson
2021-04-28 10:47 ` sbhanu
2021-04-28 15:13 ` Doug Anderson
2021-04-29 20:44 ` Georgi Djakov
2021-06-01 9:58 ` sbhanu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).