Received-SPF: pass (google.com: domain of linux-kernel+bounces-143159-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <dca6c1bf-4e7f-4177-bfe6-f43e0591b535@linaro.org>
Date: Fri, 12 Apr 2024 19:53:16 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] arm64: dts: qcom: sa8155p-adp: fix SDHC2 configuration
To: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Stephan Gerhold <stephan@gerhold.net>,
 "stable@vger.kernel.org" <stable@vger.kernel.org>,
 Bjorn Andersson <andersson@kernel.org>,
 Konrad Dybcio <konrad.dybcio@linaro.org>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Bhupesh Sharma <bhupesh.sharma@linaro.org>,
 "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
 "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20240410134022.732767-1-volodymyr_babchuk@epam.com>
 <20240411115506.1170360-1-volodymyr_babchuk@epam.com>
 <769a6a2a-2f38-42de-b3ce-8585b8b0a758@linaro.org> <87v84m4nah.fsf@epam.com>
Content-Language: en-US
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Autocrypt: addr=krzysztof.kozlowski@linaro.org; keydata=
 xsFNBFVDQq4BEAC6KeLOfFsAvFMBsrCrJ2bCalhPv5+KQF2PS2+iwZI8BpRZoV+Bd5kWvN79
 cFgcqTTuNHjAvxtUG8pQgGTHAObYs6xeYJtjUH0ZX6ndJ33FJYf5V3yXqqjcZ30FgHzJCFUu
 JMp7PSyMPzpUXfU12yfcRYVEMQrmplNZssmYhiTeVicuOOypWugZKVLGNm0IweVCaZ/DJDIH
 gNbpvVwjcKYrx85m9cBVEBUGaQP6AT7qlVCkrf50v8bofSIyVa2xmubbAwwFA1oxoOusjPIE
 J3iadrwpFvsZjF5uHAKS+7wHLoW9hVzOnLbX6ajk5Hf8Pb1m+VH/E8bPBNNYKkfTtypTDUCj
 NYcd27tjnXfG+SDs/EXNUAIRefCyvaRG7oRYF3Ec+2RgQDRnmmjCjoQNbFrJvJkFHlPeHaeS
 BosGY+XWKydnmsfY7SSnjAzLUGAFhLd/XDVpb1Een2XucPpKvt9ORF+48gy12FA5GduRLhQU
 vK4tU7ojoem/G23PcowM1CwPurC8sAVsQb9KmwTGh7rVz3ks3w/zfGBy3+WmLg++C2Wct6nM
 Pd8/6CBVjEWqD06/RjI2AnjIq5fSEH/BIfXXfC68nMp9BZoy3So4ZsbOlBmtAPvMYX6U8VwD
 TNeBxJu5Ex0Izf1NV9CzC3nNaFUYOY8KfN01X5SExAoVTr09ewARAQABzTRLcnp5c3p0b2Yg
 S296bG93c2tpIDxrcnp5c3p0b2Yua296bG93c2tpQGxpbmFyby5vcmc+wsGUBBMBCgA+FiEE
 m9B+DgxR+NWWd7dUG5NDfTtBYpsFAmI+BxMCGwMFCRRfreEFCwkIBwIGFQoJCAsCBBYCAwEC
 HgECF4AACgkQG5NDfTtBYptgbhAAjAGunRoOTduBeC7V6GGOQMYIT5n3OuDSzG1oZyM4kyvO
 XeodvvYv49/ng473E8ZFhXfrre+c1olbr1A8pnz9vKVQs9JGVa6wwr/6ddH7/yvcaCQnHRPK
 mnXyP2BViBlyDWQ71UC3N12YCoHE2cVmfrn4JeyK/gHCvcW3hUW4i5rMd5M5WZAeiJj3rvYh
 v8WMKDJOtZFXxwaYGbvFJNDdvdTHc2x2fGaWwmXMJn2xs1ZyFAeHQvrp49mS6PBQZzcx0XL5
 cU9ZjhzOZDn6Apv45/C/lUJvPc3lo/pr5cmlOvPq1AsP6/xRXsEFX/SdvdxJ8w9KtGaxdJuf
 rpzLQ8Ht+H0lY2On1duYhmro8WglOypHy+TusYrDEry2qDNlc/bApQKtd9uqyDZ+rx8bGxyY
 qBP6bvsQx5YACI4p8R0J43tSqWwJTP/R5oPRQW2O1Ye1DEcdeyzZfifrQz58aoZrVQq+innR
 aDwu8qDB5UgmMQ7cjDSeAQABdghq7pqrA4P8lkA7qTG+aw8Z21OoAyZdUNm8NWJoQy8m4nUP
 gmeeQPRc0vjp5JkYPgTqwf08cluqO6vQuYL2YmwVBIbO7cE7LNGkPDA3RYMu+zPY9UUi/ln5
 dcKuEStFZ5eqVyqVoZ9eu3RTCGIXAHe1NcfcMT9HT0DPp3+ieTxFx6RjY3kYTGLOwU0EVUNc
 NAEQAM2StBhJERQvgPcbCzjokShn0cRA4q2SvCOvOXD+0KapXMRFE+/PZeDyfv4dEKuCqeh0
 hihSHlaxTzg3TcqUu54w2xYskG8Fq5tg3gm4kh1Gvh1LijIXX99ABA8eHxOGmLPRIBkXHqJY
 oHtCvPc6sYKNM9xbp6I4yF56xVLmHGJ61KaWKf5KKWYgA9kfHufbja7qR0c6H79LIsiYqf92
 H1HNq1WlQpu/fh4/XAAaV1axHFt/dY/2kU05tLMj8GjeQDz1fHas7augL4argt4e+jum3Nwt
 yupodQBxncKAUbzwKcDrPqUFmfRbJ7ARw8491xQHZDsP82JRj4cOJX32sBg8nO2N5OsFJOcd
 5IE9v6qfllkZDAh1Rb1h6DFYq9dcdPAHl4zOj9EHq99/CpyccOh7SrtWDNFFknCmLpowhct9
 5ZnlavBrDbOV0W47gO33WkXMFI4il4y1+Bv89979rVYn8aBohEgET41SpyQz7fMkcaZU+ok/
 +HYjC/qfDxT7tjKXqBQEscVODaFicsUkjheOD4BfWEcVUqa+XdUEciwG/SgNyxBZepj41oVq
 FPSVE+Ni2tNrW/e16b8mgXNngHSnbsr6pAIXZH3qFW+4TKPMGZ2rZ6zITrMip+12jgw4mGjy
 5y06JZvA02rZT2k9aa7i9dUUFggaanI09jNGbRA/ABEBAAHCwXwEGAEKACYCGwwWIQSb0H4O
 DFH41ZZ3t1Qbk0N9O0FimwUCYDzvagUJFF+UtgAKCRAbk0N9O0Fim9JzD/0auoGtUu4mgnna
 oEEpQEOjgT7l9TVuO3Qa/SeH+E0m55y5Fjpp6ZToc481za3xAcxK/BtIX5Wn1mQ6+szfrJQ6
 59y2io437BeuWIRjQniSxHz1kgtFECiV30yHRgOoQlzUea7FgsnuWdstgfWi6LxstswEzxLZ
 Sj1EqpXYZE4uLjh6dW292sO+j4LEqPYr53hyV4I2LPmptPE9Rb9yCTAbSUlzgjiyyjuXhcwM
 qf3lzsm02y7Ooq+ERVKiJzlvLd9tSe4jRx6Z6LMXhB21fa5DGs/tHAcUF35hSJrvMJzPT/+u
 /oVmYDFZkbLlqs2XpWaVCo2jv8+iHxZZ9FL7F6AHFzqEFdqGnJQqmEApiRqH6b4jRBOgJ+cY
 qc+rJggwMQcJL9F+oDm3wX47nr6jIsEB5ZftdybIzpMZ5V9v45lUwmdnMrSzZVgC4jRGXzsU
 EViBQt2CopXtHtYfPAO5nAkIvKSNp3jmGxZw4aTc5xoAZBLo0OV+Ezo71pg3AYvq0a3/oGRG
 KQ06ztUMRrj8eVtpImjsWCd0bDWRaaR4vqhCHvAG9iWXZu4qh3ipie2Y0oSJygcZT7H3UZxq
 fyYKiqEmRuqsvv6dcbblD8ZLkz1EVZL6djImH5zc5x8qpVxlA0A0i23v5QvN00m6G9NFF0Le
 D2GYIS41Kv4Isx2dEFh+/Q==
In-Reply-To: <87v84m4nah.fsf@epam.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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 {
> &ethernet {
> &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 &ethernet 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