On 12/04/2024 17:24, Volodymyr Babchuk wrote: > > Hi Krzysztof, > > Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx >>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx> >>> >>> --- >>> >>> 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