linux-kernel - Re: [PATCH v2 6/6] arm64: dts: qcom: msm8956-loire: Add SD Card Detect to SDC2 pin states

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <81783153-45a4-478d-a0d1-8ab401e9da58@collabora.com>
Date: Mon, 29 Jan 2024 12:05:07 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Marijn Suijten <marijn.suijten@...ainline.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, ~postmarketos/upstreaming@...ts.sr.ht,
 Luca Weiss <luca@...tu.xyz>, Adam Skladowski <a39.skl@...il.com>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Martin Botka <martin.botka@...ainline.org>,
 Jami Kettunen <jami.kettunen@...ainline.org>, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/6] arm64: dts: qcom: msm8956-loire: Add SD Card
 Detect to SDC2 pin states

Il 22/01/24 16:15, Marijn Suijten ha scritto:
> On 2024-01-22 15:59:37, AngeloGioacchino Del Regno wrote:
>> Il 22/01/24 14:49, Marijn Suijten ha scritto:
>>> On 2024-01-22 12:48:27, AngeloGioacchino Del Regno wrote:
>>>> Il 21/01/24 23:33, Marijn Suijten ha scritto:
>>>>> In addition to the SDC2 pins, set the SD Card Detect pin in a sane state
>>>>> to be used as an interrupt when an SD Card is slotted in or removed.
>>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
>>>>> ---
>>>>>     arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi | 17 +++++++++++++++++
>>>>>     1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> index b0b83edd3627..75412e37334c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> @@ -264,10 +264,27 @@ &sdhc_1 {
>>>>>     	status = "okay";
>>>>>     };
>>>>>     
>>>>> +&sdc2_off_state {
>>>>> +	sd-cd-pins {
>>>>> +		pins = "gpio100";
>>>>> +		function = "gpio";
>>>>> +		drive-strength = <2>;
>>>>> +		bias-disable;
>>>>> +	};
>>>>
>>>> Are you sure that you really don't want card detect during system suspend?
>>>
>>> Does it make a difference if the rest of pinctrl and the SDHCI controller are
>>> also turned off?
>>>
>>>> You could simply add a sdc2-cd-pins out of sdc2_{on,off}_state and add use it for
>>>> both default and sleep.
>>>
>>> This sounds close to what Konrad suggested by using a new block wit its own
>>> label rather than extending the existing state.
>>>
>>>> pinctrl-0 = <&sdc2_on_state>, <&sdc2_card_det_n>;
>>>> pinctrl-1 = <&sdc2_off_state>;
>>>
>>> You said both, but it's not in pinctrl-1 here?  (And might unselect bias-pull-up
>>> implicitly instead of explicitly selecting bias-disable via an off node?)
>>>
>>
>> I meant to add it to both, sorry.
>>
>> In any case, take the typo'ed example as a simplification of your first version :-)
> 
> Okay, I'll resend a version that creates a new pinctrl node and applies it to both cases.
> 
> Unfortunately I can no longer test and confirm that it makes a difference
> to have the card-detect IRQ always biased, even while the SDHCI controller
> is "asleep" or off, so I'll trust your word for it.  If I remember correctly
> downstream turns it off as well?

Marijn, I don't remember anything about a downstream kernel from 10 years ago..!

But yes, it's okay to do so - the worst case is that it won't wake up, but that
won't happen, as it's going to generate an interrupt and wake up the entire system
which includes the SDHCI controller.

Go on!

Cheers,
Angelo