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

[Chen-Yu Tsai]

On Mon, Jul 25, 2022 at 6:20 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >>   .../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).

ChenYu

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