Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi,
On Tue, Apr 20, 2021 at 10:21 AM wrote:
>
> On 2021-04-15 01:55, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 13, 2021 at 3:59 AM wrote:
> >>
> >> >> >>> + required-opps =
> >> >> >>> <&rpmhpd_opp_low_svs>;
> >> >> >>> + opp-peak-kBps = <120
> >> >> >>> 76000>;
> >> >> >>> + opp-avg-kBps = <120
> >> >> >>> 5>;
> >> >> >> 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-38400 {
> opp-hz = /bits/ 64 <38400>;
> required-opps = <&rpmhpd_opp_nom>;
> opp-peak-kBps = <540 49>;
> opp-avg-kBps = <660 30>;
> };
>
>
> And now, these values are near to Kodaik LC values:
>
> Kodaik LC:(SC7280)
>
> opp-38400 {
> opp-hz = /bits/ 64 <38400>;
> required-opps = <&rpmhpd_opp_nom>;
> opp-peak-kBps = <540 399000>;
> opp-avg-kBps = <600 30>;
> };
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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
On 2021-04-15 01:55, Doug Anderson wrote:
Hi,
On Tue, Apr 13, 2021 at 3:59 AM wrote:
>> >>> + required-opps =
>> >>> <&rpmhpd_opp_low_svs>;
>> >>> + opp-peak-kBps = <120
>> >>> 76000>;
>> >>> + opp-avg-kBps = <120
>> >>> 5>;
>> >> 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-38400 {
opp-hz = /bits/ 64 <38400>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <540 49>;
opp-avg-kBps = <660 30>;
};
And now, these values are near to Kodaik LC values:
Kodaik LC:(SC7280)
opp-38400 {
opp-hz = /bits/ 64 <38400>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <540 399000>;
opp-avg-kBps = <600 30>;
};
-Doug
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi, On 14.04.21 23:25, Doug Anderson wrote: Hi, On Tue, Apr 13, 2021 at 3:59 AM wrote: + required-opps = <&rpmhpd_opp_low_svs>; + opp-peak-kBps = <120 76000>; + opp-avg-kBps = <120 5>; 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi, On Tue, Apr 13, 2021 at 3:59 AM wrote: > > >> >>> + required-opps = > >> >>> <&rpmhpd_opp_low_svs>; > >> >>> + opp-peak-kBps = <120 > >> >>> 76000>; > >> >>> + opp-avg-kBps = <120 > >> >>> 5>; > >> >> 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
On 2021-03-29 20:26, Doug Anderson wrote: Hi, On Thu, Mar 25, 2021 at 11:57 PM wrote: >>> + max-frequency = <19200>; >> 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 = <120 >>> 76000>; >>> + opp-avg-kBps = <120 >>> 5>; >> 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
On 2021-03-25 21:47, Doug Anderson wrote: Hi, On Wed, Mar 24, 2021 at 8:59 PM Veerabhadrarao Badiganti 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi, On Thu, Mar 25, 2021 at 11:57 PM wrote: > > >>> + max-frequency = <19200>; > >> 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 = <120 > >>> 76000>; > >>> + opp-avg-kBps = <120 > >>> 5>; > >> 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
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
wrote:
Add nodes for eMMC and SD card on sc7280.
Signed-off-by: Shaik Sajida Bhanu
---
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
/ {
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 = IRQ_TYPE_LEVEL_HIGH>,
+ 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>;
+
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi, On Wed, Mar 24, 2021 at 8:37 PM Veerabhadrarao Badiganti wrote: > > > On 3/24/2021 9:58 PM, Stephen Boyd wrote: > > Quoting Stephen Boyd (2021-03-24 08:57:33) > >> Quoting sbh...@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 = <19200>; > 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi, On Wed, Mar 24, 2021 at 8:59 PM Veerabhadrarao Badiganti 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
On 3/23/2021 9:41 PM, Doug Anderson wrote:
Hi,
On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
wrote:
Add nodes for eMMC and SD card on sc7280.
Signed-off-by: Shaik Sajida Bhanu
---
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
/ {
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 = ,
+ ;
+ 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
On 3/24/2021 9:58 PM, Stephen Boyd wrote: Quoting Stephen Boyd (2021-03-24 08:57:33) Quoting sbh...@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 = <19200>; 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.
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Quoting Stephen Boyd (2021-03-24 08:57:33) > Quoting sbh...@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 = <19200>; > > > > > > 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.
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Quoting sbh...@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 = <19200>; > > > > 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.
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
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
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 = IRQ_TYPE_LEVEL_HIGH>,
+ 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 = <19200>;
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-1 {
+ opp-hz = /bits/ 64
<1>;
+ required-opps =
<&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <120
76000>;
+ opp-avg-kBps = <120
5>;
+ };
+
+ opp-38400 {
+ opp-hz = /bits/ 64
<38400>;
+ required-opps =
<&rpmhpd_opp_nom>;
+ opp-peak-kBps = <540
160>;
+ opp-avg-kBps = <600
30>;
+ };
+ };
+ };
+
qupv3_id_0: geniqup@9c {
compatible = "qcom,geni-se-qup";
reg = <0 0x009c 0 0x2000>;
@@ -328,6 +391,54 @@
};
};
+ sdhc_2: sdhci@8804000 {
+ compatible = "qcom,sdhci-msm-v5";
+ reg
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Hi,
On Sat, Mar 20, 2021 at 11:18 AM Shaik Sajida Bhanu
wrote:
>
> Add nodes for eMMC and SD card on sc7280.
>
> Signed-off-by: Shaik Sajida Bhanu
>
> ---
> 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
>
> / {
> 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 = ,
> + ;
> + 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
Re: [PATCH V2] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
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
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 = ,
> + ;
> + 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 = <19200>;
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-1 {
> + opp-hz = /bits/ 64 <1>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <120 76000>;
> + opp-avg-kBps = <120 5>;
> + };
> +
> + opp-38400 {
> + opp-hz = /bits/ 64 <38400>;
> + required-opps = <&rpmhpd_opp_nom>;
> + opp-peak-kBps = <540 160>;
> + opp-avg-kBps = <600 30>;
> + };
> + };
> + };
> +
> qupv3_id_0: geniqup@9c {
> compatible = "qcom,geni-se-qup";
> reg = <0 0x009c 0 0x2000>;
> @@ -328,6 +391,54 @@
> };
> };
>
> + sdhc_2: sdhci@8804000 {
> +
