Received-SPF: pass (google.com: domain of linux-kernel+bounces-139551-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
To: Stephan Gerhold <stephan@gerhold.net>
CC: 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>,
        "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>
Subject: Re: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for SDHC2
 CD
Thread-Topic: [PATCH] arm64: dts: qcom: sa8155p-adp: use correct gpio for
 SDHC2 CD
Thread-Index: AQHai0zLM9/mnLFG7UOC+X9g3MAg9rFh+MwAgAAK5IA=
Date: Wed, 10 Apr 2024 21:57:02 +0000
Message-ID: <87cyqw6fvh.fsf@epam.com>
References: <20240410134022.732767-1-volodymyr_babchuk@epam.com>
 <Zhb53i8-pJhDMVZM@gerhold.net>
In-Reply-To: <Zhb53i8-pJhDMVZM@gerhold.net>
Accept-Language: en-US
Content-Language: en-US
user-agent: mu4e 1.10.7; emacs 29.3
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?iso-8859-1?Q?I1ukcloWMCWLQBXJVGNS/DhMwGSkgvfo0D8r/kSKhV+IGXF4JZ2Xzm9iTy?=
 =?iso-8859-1?Q?1Luu2w9pcc0zCi1buV/SGWeTlcn79BKxXLynfxxqKotZGUecUyjTqQqLXe?=
 =?iso-8859-1?Q?P+eiDkmGiTrkngLBxtsOodws1L/vBQAcOncE23wMXSgcJMy8pM4PRjJVqk?=
 =?iso-8859-1?Q?d3CsQscrRUDy/+6zrk+0w64C652EBMw/vlHw6XBp90XfmTuP9MTdPEovvO?=
 =?iso-8859-1?Q?+zT8BOlf2J52rrIN8fmTfJ+Zez7bOqgMaR43aFSSW1ZD1x5eOUNW2HCBCR?=
 =?iso-8859-1?Q?4HbFNtTzstPNwMN1aRzVtMR3kgDh+Zv0mD3o0tLZw2bkz50P1SuLrEUFVJ?=
 =?iso-8859-1?Q?rqP8VaInYBc3/BpzhE+fU+Ovy7KCN7yVyAm5y44QaCFvSDVQQ7xeaomCtd?=
 =?iso-8859-1?Q?nf5VnsX/RSBlXinz7ZRyeZVKm1fP/Jv5tIrz0YxZZsDEhgYDU9vdorRxKp?=
 =?iso-8859-1?Q?zs7U2fTOmVGYfNriA4EE3/pDvn4gJgjVmz3lqArtIdH3hJOmnWY6lO4Pge?=
 =?iso-8859-1?Q?jAa1UAKl2RcK8hXuxcdNd8cdyRZAjLvnvUOEU9NUTD6OB5Muzqe2/d0Qdl?=
 =?iso-8859-1?Q?LfMxM9lGs3lm+l08d+UtWjog7xgV23MgGWKXaenRHFXDVaKkEzxJLIprwS?=
 =?iso-8859-1?Q?WbrwXTm6s4BM6BqI/mPFBVWvt2XqPAkJQWUyDIr5OC1Woi0Rw9KuyaD/xT?=
 =?iso-8859-1?Q?AkWc1kzctrQkMO9ox4rPdIp6xqZmtpWqTPpisSHR7CjTuNNqurY2nxaPN5?=
 =?iso-8859-1?Q?OigF7GmztdW/BGnhL/9UmPyZqGN52vGzRgZoinMOX3sJF+3WSi/WQFwiP6?=
 =?iso-8859-1?Q?WuVp9acwDKcC7gCiHtq5Pkgm4UoLXQWdlPnK0k01a0BVdQh61Xylh8vHh/?=
 =?iso-8859-1?Q?L9CgfJxwJqqscnzmEqTsRguPGz0Z21Anlu1yCW0Uv9/malV3Pm0osZ2UP8?=
 =?iso-8859-1?Q?LD5OhfiWzeiEpDXnB9bNtDTrBzrW8TvFpxKrEeEq2/DwtPesFGJ630djaB?=
 =?iso-8859-1?Q?V5uK5o1RI76+gGTLzUPHI9HZmWc4nfCLbnnQ5bRPN2Cizvl7imbcJ824TZ?=
 =?iso-8859-1?Q?s2RuDRnUKjzX1XOFb+Z1+VAEPoBjc5xkHOJr80Oigmj6I0TpoToDpTCqCs?=
 =?iso-8859-1?Q?l5NLeOa7aNWqUF5ApZqE/C205KMb96noB4gZAdGY2vEZfZL8LuMhVdhroH?=
 =?iso-8859-1?Q?Dv8NqFYKZ1F2dwryDU9MpXllw/VIlgd7gG9feAe2Qlr3DHNm+GzcZIxIAx?=
 =?iso-8859-1?Q?hPASuQJgQwOPJLtLSTgy1E6vH4oDtG5yrNydWaDC4F5lzivor21Blc/blG?=
 =?iso-8859-1?Q?vAmGS+5JkVmJt/3ozF2KLsgOgmvZHHrOKV368COHyd0DbJTfj+tRKjsC+2?=
 =?iso-8859-1?Q?Mhtr3/MkQMcWmfhTpX4AgTlpqxCSp+sLgKmmkHuKme+nQvzQbZZYvl3OaU?=
 =?iso-8859-1?Q?CJNJpc4wu9a2AjlF/Tc0qDfgMiDBIvqycqXgaikS+QM0bjbNTZVbGuzM9p?=
 =?iso-8859-1?Q?pzC7K/sz89bingjFl2a4e24lwbKf3eG9he9VXytS9CdKMCBOI1I+2BZOtY?=
 =?iso-8859-1?Q?KVliHw7B8bSFpXH8aJ8DKNxeUTj8PbqaZcp5WEap/p05rgrFVpslsVcInz?=
 =?iso-8859-1?Q?WW/HWtpIQCoZ5vQrlbEMAlJiJxDJMmIt28ed6oj6ma6ehkWkhmsn3UcA?=
 =?iso-8859-1?Q?=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: epam.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: GV1PR03MB10456.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 54b39c72-77d2-4cc4-0364-08dc59a92742
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Apr 2024 21:57:02.2688
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b41b72d0-4e9f-4c26-8a69-f949f367c91d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Dw+g23g8Cpb7T5gorBbRzLbhWfomT4JX6KIJoMLB8LhtBjBJ1kBeZmXUQOEJ7Ra+FT3E+eEfYh8Zkej2FMwkvKsM8XoALGy19HqI615eF/I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR03MB6133
X-Proofpoint-GUID: 7FJrc-4gpmXggYIcIKk-X3iN1UugC4p_
X-Proofpoint-ORIG-GUID: 7FJrc-4gpmXggYIcIKk-X3iN1UugC4p_
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-10_06,2024-04-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0
 priorityscore=1501 bulkscore=0 malwarescore=0 adultscore=0 impostorscore=0
 mlxscore=0 mlxlogscore=999 clxscore=1011 phishscore=0 suspectscore=0
 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.19.0-2404010003 definitions=main-2404100160


Hi Stephan,

Stephan Gerhold <stephan@gerhold.net> writes:

> On Wed, Apr 10, 2024 at 01:41:30PM +0000, Volodymyr Babchuk wrote:
>> Card Detect pin for SHDC2 on SA8155P-ADP is connected to GPIO4 of
>> PMM8155AU_1, not to internal TLMM. This change fixes two issues at
>> once: not working ethernet (because GPIO4 is used for MAC TX) and SD
>> detection.
>>=20
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>=20
>> diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/=
dts/qcom/sa8155p-adp.dts
>> index 5e4287f8c8cd1..6b08ce246b78c 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8155p-adp.dts
>> @@ -384,7 +384,7 @@ &remoteproc_cdsp {
>>  &sdhc_2 {
>>  	status =3D "okay";
>> =20
>> -	cd-gpios =3D <&tlmm 4 GPIO_ACTIVE_LOW>;
>> +	cd-gpios =3D <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
>
> Good catch!

Yeah... It took time to understand why ethernet is not working
sometimes. It appeared that there were race between SDHC and MAC
initialization.

>
>>  	pinctrl-names =3D "default", "sleep";
>>  	pinctrl-0 =3D <&sdc2_on>;
>>  	pinctrl-1 =3D <&sdc2_off>;
>
> These two pinctrl configure "gpio96" for "sd-cd-pins". Yet another wrong
> GPIO? Should probably drop that and add proper pinctrl for the actual
> GPIO in the PMIC. Seems like Qualcomm configured the PMIC GPIO with
> pull-up downstream [1] (not sure if this is the right file). It might be
> redundant if there is an external pull-up on the board, but only the
> schematic could tell that for sure.

If I only had schematic for this board... gpio96 puzzles me as well. I
can understand where wrong GPIO4 come from. But gpio96 origin is
completely unclear. I have user manual for the board, it mention
functions of some GPIOs, but there is nothing about gpio96. Anyways, I
removed it from the DTS (see diff below) and I see no issues with SD card.

> FWIW: Looking closer at the broken commit, the regulator voltage setup
> of &sdhc_2 looks suspicious too. Typically one would want a 1.8V capable
> regulator for the vqmmc-supply to properly use ultra high-speed modes,
> but &vreg_l13c_2p96 seems to be configured with 2.504V-2.960V at the
> moment. On downstream it seems to be 1.8V-2.96V [2] (again, not sure if
> this is the right file). I would recommend re-checking this too and
> testing if UHS cards are detected correctly (if you have the board).

This is actually a good catch. I changed the voltage range to 1.8V-2.96V an=
d
now my card detects in UHS/SDR104 mode. Prior to this change it worked only
in HS mode.

> &vreg_l17a_2p96 has the same 2.504V-2.960V, but has 2.704V-2.960V
> downstream [3]. This is close at least, might be fine as-is (but I'm not
> convinced there is a good reason to differ there).
>

Well, I believe that downstream configuration is more correct, but I
can't prove it, so I'll leave it as is.

Diff for additional changes looks like this:

diff --git a/arch/arm64/boot/dts/qcom/sa8155p-adp.dts b/arch/arm64/boot/dts=
/qcom/sa8155p-adp.dts
index 6b08ce246b78c..b2a3496ff68ad 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 {
=20
                vreg_l13c_2p96: ldo13 {
                        regulator-name =3D "vreg_l13c_2p96";
-                       regulator-min-microvolt =3D <2504000>;
+                       regulator-min-microvolt =3D <1800000>;
                        regulator-max-microvolt =3D <2960000>;
                        regulator-initial-mode =3D <RPMH_REGULATOR_MODE_HPM=
>;
                };
@@ -386,8 +386,8 @@ &sdhc_2 {
=20
        cd-gpios =3D <&pmm8155au_1_gpios 4 GPIO_ACTIVE_LOW>;
        pinctrl-names =3D "default", "sleep";
-       pinctrl-0 =3D <&sdc2_on>;
-       pinctrl-1 =3D <&sdc2_off>;
+       pinctrl-0 =3D <&sdc2_on &pmm8155au_1_sdc2_on>;
+       pinctrl-1 =3D <&sdc2_off &pmm8155au_1_sdc2_off>;
        vqmmc-supply =3D <&vreg_l13c_2p96>; /* IO line power */
        vmmc-supply =3D <&vreg_l17a_2p96>;  /* Card power line */
        bus-width =3D <4>;
@@ -505,13 +505,6 @@ data-pins {
                        bias-pull-up;           /* pull up */
                        drive-strength =3D <16>;  /* 16 MA */
                };
-
-               sd-cd-pins {
-                       pins =3D "gpio96";
-                       function =3D "gpio";
-                       bias-pull-up;           /* pull up */
-                       drive-strength =3D <2>;   /* 2 MA */
-               };
        };
=20
        sdc2_off: sdc2-off-state {
        @@ -532,13 +525,6 @@ data-pins {
                        bias-pull-up;           /* pull up */
                        drive-strength =3D <2>;   /* 2 MA */
                };
-
-               sd-cd-pins {
-                       pins =3D "gpio96";
-                       function =3D "gpio";
-                       bias-pull-up;           /* pull up */
-                       drive-strength =3D <2>;   /* 2 MA */
-               };
        };
=20
        usb2phy_ac_en1_default: usb2phy-ac-en1-default-state {
@@ -604,3 +590,25 @@ phy-reset-pins {
                };
        };
 };
+
+&pmm8155au_1_gpios {
+       pmm8155au_1_sdc2_on: pmm8155au_1-sdc2-on-state {
+               sd-cd-pin {
+                       pins =3D "gpio4";
+                       function =3D "normal";
+                       input-enable;
+                       bias-pull-up;
+                       power-source =3D <0>;
+               };
+       };
+
+       pmm8155au_1_sdc2_off: pmm8155au_1-sdc2-off-state {
+               sd-cd-pin {
+                       pins =3D "gpio4";
+                       function =3D "normal";
+                       input-enable;
+                       bias-pull-up;
+                       power-source =3D <0>;
+               };
+       };
+};


I am planning to send v2 tomorrow.

--=20
WBR, Volodymyr=