| Message ID | 20240411115506.1170360-1-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration | expand |
On 11/04/2024 13:55, Volodymyr Babchuk wrote: > There are multiple issues with SDHC2 configuration for SA8155P-ADP, > which prevent use of SDHC2 and causes issues with ethernet: > > - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of > PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC > TX. If sdhc driver probes after dwmac driver, it reconfigures > gpio4 and this breaks Ethernet MAC. > > - pinctrl configuration mentions gpio96 as CD pin. It seems it was > copied from some SM8150 example, because as mentioned above, > correct CD pin is gpio4 on PMM8155AU_1. > > - L13C voltage regulator limits minimal voltage to 2.504V, which > prevents use 1.8V to power SD card, which in turns does not allow > card to work in UHS mode. That's not really related. One issue, one commit. > > This patch fixes all the mentioned issues. > > Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card") > Cc: stable@vger.kernel.org > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > In v2: > - Added "Fixes:" tag > - CCed stable ML > - Fixed pinctrl configuration > - Extended voltage range for L13C voltage regulator > --- > arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 32 +++++++++++------------- > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > index 5e4287f8c8cd1..b9d56bda96759 100644 > --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 { > > vreg_l13c_2p96: ldo13 { > regulator-name = "vreg_l13c_2p96"; > - regulator-min-microvolt = <2504000>; > + regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <2960000>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > @@ -384,10 +384,10 @@ &remoteproc_cdsp { > &sdhc_2 { > status = "okay"; > > - cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>; > + cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>; > pinctrl-names = "default", "sleep"; > - pinctrl-0 = <&sdc2_on>; > - pinctrl-1 = <&sdc2_off>; > + pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>; > + pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>; > vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */ > vmmc-supply = <&vreg_l17a_2p96>; /* Card power line */ > bus-width = <4>; > @@ -505,13 +505,6 @@ data-pins { > bias-pull-up; /* pull up */ > drive-strength = <16>; /* 16 MA */ > }; > - > - sd-cd-pins { > - pins = "gpio96"; > - function = "gpio"; > - bias-pull-up; /* pull up */ > - drive-strength = <2>; /* 2 MA */ > - }; > }; > > sdc2_off: sdc2-off-state { > @@ -532,13 +525,6 @@ data-pins { > bias-pull-up; /* pull up */ > drive-strength = <2>; /* 2 MA */ > }; > - > - sd-cd-pins { > - pins = "gpio96"; > - function = "gpio"; > - bias-pull-up; /* pull up */ > - drive-strength = <2>; /* 2 MA */ > - }; > }; > > usb2phy_ac_en1_default: usb2phy-ac-en1-default-state { > @@ -604,3 +590,13 @@ phy-reset-pins { > }; > }; > }; > + > +&pmm8155au_1_gpios { > + pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd { No underscores in node names. Please also follow tlmm style of naming nodes. Also does not look like node is placed in alphabetical place. Where did you put it? Best regards, Krzysztof
On Thu, Apr 11, 2024 at 11:55:55AM +0000, Volodymyr Babchuk wrote: > There are multiple issues with SDHC2 configuration for SA8155P-ADP, > which prevent use of SDHC2 and causes issues with ethernet: > > - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of > PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC > TX. If sdhc driver probes after dwmac driver, it reconfigures > gpio4 and this breaks Ethernet MAC. > > - pinctrl configuration mentions gpio96 as CD pin. It seems it was > copied from some SM8150 example, because as mentioned above, > correct CD pin is gpio4 on PMM8155AU_1. > > - L13C voltage regulator limits minimal voltage to 2.504V, which > prevents use 1.8V to power SD card, which in turns does not allow > card to work in UHS mode. > > This patch fixes all the mentioned issues. > > Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card") > Cc: stable@vger.kernel.org > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > In v2: > - Added "Fixes:" tag > - CCed stable ML > - Fixed pinctrl configuration > - Extended voltage range for L13C voltage regulator > --- > arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 32 +++++++++++------------- > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > index 5e4287f8c8cd1..b9d56bda96759 100644 > --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts > @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 { > > vreg_l13c_2p96: ldo13 { > regulator-name = "vreg_l13c_2p96"; > - regulator-min-microvolt = <2504000>; > + regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <2960000>; > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > @@ -384,10 +384,10 @@ &remoteproc_cdsp { > &sdhc_2 { > status = "okay"; > > - cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>; > + cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>; > pinctrl-names = "default", "sleep"; > - pinctrl-0 = <&sdc2_on>; > - pinctrl-1 = <&sdc2_off>; > + pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>; > + pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>; > vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */ > vmmc-supply = <&vreg_l17a_2p96>; /* Card power line */ > bus-width = <4>; > @@ -505,13 +505,6 @@ data-pins { > bias-pull-up; /* pull up */ > drive-strength = <16>; /* 16 MA */ > }; > - > - sd-cd-pins { > - pins = "gpio96"; > - function = "gpio"; > - bias-pull-up; /* pull up */ > - drive-strength = <2>; /* 2 MA */ > - }; > }; > > sdc2_off: sdc2-off-state { > @@ -532,13 +525,6 @@ data-pins { > bias-pull-up; /* pull up */ > drive-strength = <2>; /* 2 MA */ > }; > - > - sd-cd-pins { > - pins = "gpio96"; > - function = "gpio"; > - bias-pull-up; /* pull up */ > - drive-strength = <2>; /* 2 MA */ > - }; > }; > > usb2phy_ac_en1_default: usb2phy-ac-en1-default-state { > @@ -604,3 +590,13 @@ phy-reset-pins { > }; > }; > }; > + > +&pmm8155au_1_gpios { > + pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd { > + pins = "gpio4"; > + function = "normal"; > + input-enable; > + bias-pull-up; > + power-source = <0>; Nitpick: There is one indentation level too much here (remove a tab). Barely worth mentioning, but I guess there will be a v3 to address Krzysztof's comments. :) Thanks, Stephan
Hi Krzysztof, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 11/04/2024 13:55, Volodymyr Babchuk wrote: >> There are multiple issues with SDHC2 configuration for SA8155P-ADP, >> which prevent use of SDHC2 and causes issues with ethernet: >> >> - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of >> PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC >> TX. If sdhc driver probes after dwmac driver, it reconfigures >> gpio4 and this breaks Ethernet MAC. >> >> - pinctrl configuration mentions gpio96 as CD pin. It seems it was >> copied from some SM8150 example, because as mentioned above, >> correct CD pin is gpio4 on PMM8155AU_1. >> >> - L13C voltage regulator limits minimal voltage to 2.504V, which >> prevents use 1.8V to power SD card, which in turns does not allow >> card to work in UHS mode. > > That's not really related. One issue, one commit. > >> >> This patch fixes all the mentioned issues. >> >> Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card") >> Cc: stable@vger.kernel.org >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> >> --- >> >> In v2: >> - Added "Fixes:" tag >> - CCed stable ML >> - Fixed pinctrl configuration >> - Extended voltage range for L13C voltage regulator >> --- >> arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 32 +++++++++++------------- >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >> index 5e4287f8c8cd1..b9d56bda96759 100644 >> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >> @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 { >> >> vreg_l13c_2p96: ldo13 { >> regulator-name = "vreg_l13c_2p96"; >> - regulator-min-microvolt = <2504000>; >> + regulator-min-microvolt = <1800000>; >> regulator-max-microvolt = <2960000>; >> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >> }; >> @@ -384,10 +384,10 @@ &remoteproc_cdsp { >> &sdhc_2 { >> status = "okay"; >> >> - cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>; >> + cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>; >> pinctrl-names = "default", "sleep"; >> - pinctrl-0 = <&sdc2_on>; >> - pinctrl-1 = <&sdc2_off>; >> + pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>; >> + pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>; >> vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */ >> vmmc-supply = <&vreg_l17a_2p96>; /* Card power line */ >> bus-width = <4>; >> @@ -505,13 +505,6 @@ data-pins { >> bias-pull-up; /* pull up */ >> drive-strength = <16>; /* 16 MA */ >> }; >> - >> - sd-cd-pins { >> - pins = "gpio96"; >> - function = "gpio"; >> - bias-pull-up; /* pull up */ >> - drive-strength = <2>; /* 2 MA */ >> - }; >> }; >> >> sdc2_off: sdc2-off-state { >> @@ -532,13 +525,6 @@ data-pins { >> bias-pull-up; /* pull up */ >> drive-strength = <2>; /* 2 MA */ >> }; >> - >> - sd-cd-pins { >> - pins = "gpio96"; >> - function = "gpio"; >> - bias-pull-up; /* pull up */ >> - drive-strength = <2>; /* 2 MA */ >> - }; >> }; >> >> usb2phy_ac_en1_default: usb2phy-ac-en1-default-state { >> @@ -604,3 +590,13 @@ phy-reset-pins { >> }; >> }; >> }; >> + >> +&pmm8155au_1_gpios { >> + pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd { > > No underscores in node names. Fill fix. > Please also follow tlmm style of naming nodes. Just to be on the same page, will "pmm8155au_1_sdc2_cd: sdc2-cd-pins" be fine? > Also does not look like node is placed in alphabetical place. Where did > you put it? I can't say that the file is sorted in the first place: # grep "^&" arch/arm64/boot/dts/qcom/sa8155p-adp.dts &apps_rsc { ðernet { &qupv3_id_1 { &remoteproc_adsp { &remoteproc_cdsp { &sdhc_2 { &uart2 { &uart9 { &ufs_mem_hc { &ufs_mem_phy { &usb_1 { &usb_1_dwc3 { &usb_1_hsphy { &usb_1_qmpphy { &usb_2 { &usb_2_dwc3 { &usb_2_hsphy { &usb_2_qmpphy { &pcie0 { &pcie0_phy { &pcie1_phy { &tlmm { &pmm8155au_1_gpios { So, I can put after &pci1 to have it grouped with other entries that start with p*, or I can put right after ðernet to make it appear in alphabetical order. It is your call.
On 12/04/2024 17:24, Volodymyr Babchuk wrote: > > Hi Krzysztof, > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > >> On 11/04/2024 13:55, Volodymyr Babchuk wrote: >>> There are multiple issues with SDHC2 configuration for SA8155P-ADP, >>> which prevent use of SDHC2 and causes issues with ethernet: >>> >>> - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of >>> PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC >>> TX. If sdhc driver probes after dwmac driver, it reconfigures >>> gpio4 and this breaks Ethernet MAC. >>> >>> - pinctrl configuration mentions gpio96 as CD pin. It seems it was >>> copied from some SM8150 example, because as mentioned above, >>> correct CD pin is gpio4 on PMM8155AU_1. >>> >>> - L13C voltage regulator limits minimal voltage to 2.504V, which >>> prevents use 1.8V to power SD card, which in turns does not allow >>> card to work in UHS mode. >> >> That's not really related. One issue, one commit. >> >>> >>> This patch fixes all the mentioned issues. >>> >>> Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>> >>> --- >>> >>> In v2: >>> - Added "Fixes:" tag >>> - CCed stable ML >>> - Fixed pinctrl configuration >>> - Extended voltage range for L13C voltage regulator >>> --- >>> arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 32 +++++++++++------------- >>> 1 file changed, 14 insertions(+), 18 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >>> index 5e4287f8c8cd1..b9d56bda96759 100644 >>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts >>> @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 { >>> >>> vreg_l13c_2p96: ldo13 { >>> regulator-name = "vreg_l13c_2p96"; >>> - regulator-min-microvolt = <2504000>; >>> + regulator-min-microvolt = <1800000>; >>> regulator-max-microvolt = <2960000>; >>> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; >>> }; >>> @@ -384,10 +384,10 @@ &remoteproc_cdsp { >>> &sdhc_2 { >>> status = "okay"; >>> >>> - cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>; >>> + cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>; >>> pinctrl-names = "default", "sleep"; >>> - pinctrl-0 = <&sdc2_on>; >>> - pinctrl-1 = <&sdc2_off>; >>> + pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>; >>> + pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>; >>> vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */ >>> vmmc-supply = <&vreg_l17a_2p96>; /* Card power line */ >>> bus-width = <4>; >>> @@ -505,13 +505,6 @@ data-pins { >>> bias-pull-up; /* pull up */ >>> drive-strength = <16>; /* 16 MA */ >>> }; >>> - >>> - sd-cd-pins { >>> - pins = "gpio96"; >>> - function = "gpio"; >>> - bias-pull-up; /* pull up */ >>> - drive-strength = <2>; /* 2 MA */ >>> - }; >>> }; >>> >>> sdc2_off: sdc2-off-state { >>> @@ -532,13 +525,6 @@ data-pins { >>> bias-pull-up; /* pull up */ >>> drive-strength = <2>; /* 2 MA */ >>> }; >>> - >>> - sd-cd-pins { >>> - pins = "gpio96"; >>> - function = "gpio"; >>> - bias-pull-up; /* pull up */ >>> - drive-strength = <2>; /* 2 MA */ >>> - }; >>> }; >>> >>> usb2phy_ac_en1_default: usb2phy-ac-en1-default-state { >>> @@ -604,3 +590,13 @@ phy-reset-pins { >>> }; >>> }; >>> }; >>> + >>> +&pmm8155au_1_gpios { >>> + pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd { >> >> No underscores in node names. > > Fill fix. > >> Please also follow tlmm style of naming nodes. > > Just to be on the same page, will "pmm8155au_1_sdc2_cd: sdc2-cd-pins" be fine? pins are for sublevel, so you want -state. Just like other pmic GPIOs. > >> Also does not look like node is placed in alphabetical place. Where did >> you put it? > > I can't say that the file is sorted in the first place: > > # grep "^&" arch/arm64/boot/dts/qcom/sa8155p-adp.dts > &apps_rsc { > ðernet { > &qupv3_id_1 { > &remoteproc_adsp { > &remoteproc_cdsp { > &sdhc_2 { > &uart2 { > &uart9 { > &ufs_mem_hc { > &ufs_mem_phy { > &usb_1 { > &usb_1_dwc3 { > &usb_1_hsphy { > &usb_1_qmpphy { > &usb_2 { > &usb_2_dwc3 { > &usb_2_hsphy { > &usb_2_qmpphy { Was sorted till here... > &pcie0 { > &pcie0_phy { > &pcie1_phy { > &tlmm { and here second sorting started... > &pmm8155au_1_gpios { and you started third. > > > So, I can put after &pci1 to have it grouped with other entries that > start with p*, or I can put right after ðernet to make it appear in > alphabetical order. It is your call. Please put it in the first group, so after ethernet. If this gets ever sorted, then at least one less move. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts index 5e4287f8c8cd1..b9d56bda96759 100644 --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts @@ -283,7 +283,7 @@ vreg_l12c_1p808: ldo12 { vreg_l13c_2p96: ldo13 { regulator-name = "vreg_l13c_2p96"; - regulator-min-microvolt = <2504000>; + regulator-min-microvolt = <1800000>; regulator-max-microvolt = <2960000>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; }; @@ -384,10 +384,10 @@ &remoteproc_cdsp { &sdhc_2 { status = "okay"; - cd-gpios = <&tlmm 4 GPIO_ACTIVE_LOW>; + cd-gpios = <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>; pinctrl-names = "default", "sleep"; - pinctrl-0 = <&sdc2_on>; - pinctrl-1 = <&sdc2_off>; + pinctrl-0 = <&sdc2_on &pmm8155au_1_sdc2_cd>; + pinctrl-1 = <&sdc2_off &pmm8155au_1_sdc2_cd>; vqmmc-supply = <&vreg_l13c_2p96>; /* IO line power */ vmmc-supply = <&vreg_l17a_2p96>; /* Card power line */ bus-width = <4>; @@ -505,13 +505,6 @@ data-pins { bias-pull-up; /* pull up */ drive-strength = <16>; /* 16 MA */ }; - - sd-cd-pins { - pins = "gpio96"; - function = "gpio"; - bias-pull-up; /* pull up */ - drive-strength = <2>; /* 2 MA */ - }; }; sdc2_off: sdc2-off-state { @@ -532,13 +525,6 @@ data-pins { bias-pull-up; /* pull up */ drive-strength = <2>; /* 2 MA */ }; - - sd-cd-pins { - pins = "gpio96"; - function = "gpio"; - bias-pull-up; /* pull up */ - drive-strength = <2>; /* 2 MA */ - }; }; usb2phy_ac_en1_default: usb2phy-ac-en1-default-state { @@ -604,3 +590,13 @@ phy-reset-pins { }; }; }; + +&pmm8155au_1_gpios { + pmm8155au_1_sdc2_cd: pmm8155au_1-sdc2-cd { + pins = "gpio4"; + function = "normal"; + input-enable; + bias-pull-up; + power-source = <0>; + }; +};
There are multiple issues with SDHC2 configuration for SA8155P-ADP, which prevent use of SDHC2 and causes issues with ethernet: - Card Detect pin for SHDC2 on SA8155P-ADP is connected to gpio4 of PMM8155AU_1, not to SoC itself. SoC's gpio4 is used for DWMAC TX. If sdhc driver probes after dwmac driver, it reconfigures gpio4 and this breaks Ethernet MAC. - pinctrl configuration mentions gpio96 as CD pin. It seems it was copied from some SM8150 example, because as mentioned above, correct CD pin is gpio4 on PMM8155AU_1. - L13C voltage regulator limits minimal voltage to 2.504V, which prevents use 1.8V to power SD card, which in turns does not allow card to work in UHS mode. This patch fixes all the mentioned issues. Fixes: 0deb2624e2d0 ("arm64: dts: qcom: sa8155p-adp: Add support for uSD card") Cc: stable@vger.kernel.org Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- In v2: - Added "Fixes:" tag - CCed stable ML - Fixed pinctrl configuration - Extended voltage range for L13C voltage regulator --- arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 32 +++++++++++------------- 1 file changed, 14 insertions(+), 18 deletions(-)