On Wed, 13 Jan 2021 at 12:34, Marek Vasut <marex@xxxxxxx> wrote:
>
> On 1/13/21 11:44 AM, Ulf Hansson wrote:
>
> [...]
>
> >> NOTE: I would prefer this solution over having a custom DT per SoM,
> >> since it reduces the amount of DT combinations.
> >> ---
> >> arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 32 ++++++++-
> >> drivers/mmc/host/mmci.c | 70 ++++++++++++++++++--
> >
> > Please avoid mixing DTS changes with changes to code in drivers.
>
> With RFC patch you likely want to see the whole picture, so I kept it in
> one patch.
>
> >> 2 files changed, 96 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
> >> index dc70ddd09e9d..a69cae19d92d 100644
> >> --- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
> >> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
> >> @@ -401,15 +401,45 @@ &rtc {
> >> status = "okay";
> >> };
> >>
> >> +&pinctrl {
> >> + sdmmc1_b4_init_pins_a: sdmmc1-init-b4-0 {
> >> + pins1 {
> >> + pinmux = <STM32_PINMUX('C', 8, AF12)>, /* SDMMC1_D0 */
> >> + <STM32_PINMUX('C', 9, AF12)>, /* SDMMC1_D1 */
> >> + <STM32_PINMUX('C', 10, AF12)>, /* SDMMC1_D2 */
> >> + <STM32_PINMUX('C', 11, AF12)>; /* SDMMC1_D3 */
> >> + slew-rate = <1>;
> >> + drive-push-pull;
> >> + bias-disable;
> >> + };
> >> + };
> >> +
> >> + sdmmc1_dir_init_pins_a: sdmmc1-init-dir-0 {
> >> + pins1 {
> >> + pinmux = <STM32_PINMUX('F', 2, AF11)>, /* SDMMC1_D0DIR */
> >> + <STM32_PINMUX('C', 7, AF8)>, /* SDMMC1_D123DIR */
> >> + <STM32_PINMUX('B', 9, AF11)>; /* SDMMC1_CDIR */
> >> + slew-rate = <1>;
> >> + drive-push-pull;
> >> + bias-pull-up;
> >> + };
> >> + };
> >> +};
> >> +
> >> &sdmmc1 {
> >> - pinctrl-names = "default", "opendrain", "sleep";
> >> + pinctrl-names = "default", "opendrain", "sleep", "init";
> >
> > Apologize for my ignorance, but my understanding of "init" vs
> > "default" is that "init" should be treated as the legacy name for the
> > pinstate. I would appreciate Linus' opinion on this.
>
> My understanding is that "init" is the way pins are configured before
> the driver reconfigures them to whatever the driver needs to configure
> them to. The "default" state is the normal operational state of the
> hardware, which often is the same as "init".
>
> [...]
>
> >> static int mmci_of_parse(struct device_node *np, struct mmc_host *mmc)
> >> {
> >> struct mmci_host *host = mmc_priv(mmc);
> >> @@ -1913,7 +1973,7 @@ static int mmci_of_parse(struct device_node *np, struct mmc_host *mmc)
> >> if (of_get_property(np, "st,neg-edge", NULL))
> >> host->clk_reg_add |= MCI_STM32_CLK_NEGEDGE;
> >> if (of_get_property(np, "st,use-ckin", NULL))
> >> - host->clk_reg_add |= MCI_STM32_CLK_SELCKIN;
> >> + mmci_probe_level_translator(mmc);
> >
> > I think you can make this change bit less invasive. Rather than having
> > to shuffle code around in ->probe(), I suggest you call
> > mmci_probe_level_translator() outside and after mmci_of_parse() has
> > been called.
> >
> > In this way, you can also provide mmci_probe_level_translator() with a
> > struct mmci_host *, rather than having to pick it up from
> > mmc_priv(mmc), if you see what I mean.
> >
> > Of course, this also means in mmci_probe_level_translator() you will
> > have to check if MCI_STM32_CLK_SELCKIN has been set, and if not then
> > do an early return.
>
> Testing the translator presence when checking whether its enabled in DT
> seems like the right place, but that's really just an implementation detail.
>
> I am more interested in knowing whether adding
> mmci_probe_level_translator() is even acceptable in the first place. Is it ?
Honestly, I don't know.
I think I need to defer that question to Linus Walleij. And of course,
it would be nice to get the opinion from Ludovic as well.
Kind regards
Uffe
