Re: [PATCH v2 4/8] arm64: dts: mediatek: cherry: Enable secondary SD/MMC controller

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 26/07/22 06:04, Chen-Yu Tsai ha scritto:
> On Mon, Jul 25, 2022 at 6:20 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 25/07/22 10:54, Chen-Yu Tsai ha scritto:
>>> On Thu, Jul 21, 2022 at 10:51 PM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>
>>>> As of now, all of the boards based on the cherry platform have a
>>>> usable secondary SD/MMC controller, usually for SD cards: enable
>>>> it to allow both booting from it and generally accessing external
>>>> storage.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    .../boot/dts/mediatek/mt8195-cherry.dtsi      | 62 +++++++++++++++++++
>>>>    1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
>>>> index 2853f7f76c90..8859957c7b27 100644
>>>> --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
>>>> @@ -17,6 +17,7 @@ aliases {
>>>>                   i2c5 = &i2c5;
>>>>                   i2c7 = &i2c7;
>>>>                   mmc0 = &mmc0;
>>>> +               mmc1 = &mmc1;
>>>>                   serial0 = &uart0;
>>>>           };
>>>>
>>>> @@ -227,6 +228,24 @@ &mmc0 {
>>>>           vqmmc-supply = <&mt6359_vufs_ldo_reg>;
>>>>    };
>>>>
>>>> +&mmc1 {
>>>> +       status = "okay";
>>>> +
>>>> +       bus-width = <4>;
>>>> +       cap-sd-highspeed;
>>>> +       cd-gpios = <&pio 54 GPIO_ACTIVE_LOW>;
>>>> +       max-frequency = <200000000>;
>>>> +       no-mmc;
>>>> +       no-sdio;
>>>> +       pinctrl-names = "default", "state_uhs";
>>>> +       pinctrl-0 = <&mmc1_pins_default>;
>>>> +       pinctrl-1 = <&mmc1_pins_uhs>;
>>>> +       sd-uhs-sdr50;
>>>> +       sd-uhs-sdr104;
>>>> +       vmmc-supply = <&mt_pmic_vmch_ldo_reg>;
>>>> +       vqmmc-supply = <&mt_pmic_vmc_ldo_reg>;
>>>> +};
>>>> +
>>>>    /* for CPU-L */
>>>>    &mt6359_vcore_buck_reg {
>>>>           regulator-always-on;
>>>> @@ -575,6 +594,49 @@ pins-rst {
>>>>                   };
>>>>           };
>>>>
>>>> +       mmc1_pins_default: mmc1-default-pins {
>>>> +               pins-cmd-dat {
>>>> +                       pinmux = <PINMUX_GPIO110__FUNC_MSDC1_CMD>,
>>>> +                                <PINMUX_GPIO112__FUNC_MSDC1_DAT0>,
>>>> +                                <PINMUX_GPIO113__FUNC_MSDC1_DAT1>,
>>>> +                                <PINMUX_GPIO114__FUNC_MSDC1_DAT2>,
>>>> +                                <PINMUX_GPIO115__FUNC_MSDC1_DAT3>;
>>>> +                       input-enable;
>>>> +                       drive-strength = <8>;
>>>> +                       bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
>>>> +               };
>>>> +
>>>> +               pins-clk {
>>>> +                       pinmux = <PINMUX_GPIO111__FUNC_MSDC1_CLK>;
>>>> +                       drive-strength = <8>;
>>>> +                       bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
>>>> +               };
>>>> +
>>>> +               pins-insert {
>>>> +                       pinmux = <PINMUX_GPIO54__FUNC_GPIO54>;
>>>> +                       bias-pull-up;
>>>> +               };
>>>> +       };
>>>> +
>>>> +       mmc1_pins_uhs: mmc1-uhs-pins {
>>>> +               pins-cmd-dat {
>>>> +                       pinmux = <PINMUX_GPIO110__FUNC_MSDC1_CMD>,
>>>> +                                <PINMUX_GPIO112__FUNC_MSDC1_DAT0>,
>>>> +                                <PINMUX_GPIO113__FUNC_MSDC1_DAT1>,
>>>> +                                <PINMUX_GPIO114__FUNC_MSDC1_DAT2>,
>>>> +                                <PINMUX_GPIO115__FUNC_MSDC1_DAT3>;
>>>> +                       input-enable;
>>>> +                       drive-strength = <8>;
>>>> +                       bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
>>>> +               };
>>>> +
>>>> +               pins-clk {
>>>> +                       pinmux = <PINMUX_GPIO111__FUNC_MSDC1_CLK>;
>>>> +                       drive-strength = <8>;
>>>> +                       bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
>>>> +               };
>>>
>>> I wonder if pins-insert should be duplicated here. And there's no
>>> difference between the standard and UHS pinconfigs. One would expect
>>> higher drive strength on the UHS set, if two sets were required.
>>> So maybe we should just have one set, and use that one for both
>>> the default and uhs states.
>>>
>>
>> I don't think that it would really make a lot of sense to duplicate the
>> insertion pin setup in the UHS-specific pinctrl set...
>>
>> Whenever you remove the uSD card, the controller goes back to default,
>> as the first steps in card initialization are always happening at low
>> speed and only after that we can switch to UHS speeds... so we do expect
>> that the first-ever state is always `default` (by spec!), which means
>> that we are also ensuring that the insertion pin setup is always done.
> 
> Right. What I wanted to say was that, besides the insertion pin, there's
> no difference between the default and uhs states here. So why have two
> copies instead of one that is referenced twice? (the uhs state is required
> by the binding).
> 

OH!!!! Right. Yes I definitely agree with you on that one.

Let's split the pins-insert in `mmc1_pins_det: mmc1-detect-pins` and add
a reference to it in the default pinctrl, that's a sensible option.

I'll do that for v3!

> ChenYu
> 
>> Cheers,
>> Angelo
>>
>>> Otherwise,
>>>
>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
>>> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
>>>
>>>> +       };
>>>> +
>>>>           nor_pins_default: nor-default-pins {
>>>>                   pins-ck-io {
>>>>                           pinmux = <PINMUX_GPIO142__FUNC_SPINOR_IO0>,
>>>> --
>>>> 2.35.1
>>>>
>>>>
>>


-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718