| Message ID | 20210105140718.122752-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] mmc: mmci: Add support for probing bus voltage level translator | expand |
+ Linus (GPIO/pinctrl maintainer) On Tue, 5 Jan 2021 at 15:07, Marek Vasut <marex@denx.de> wrote: > > Add support for testing whether bus voltage level translator is present > and operational. This is useful on systems where the bus voltage level > translator is optional, as the translator can be auto-detected by the > driver and the feedback clock functionality can be disabled if it is > not present. > > This requires additional pinmux state, "init", where the CMD, CK, CKIN > lines are not configured, so they can be claimed as GPIOs early on in > probe(). The translator test sets CMD high to avoid interfering with a > card, and then verifies whether signal set on CK is detected on CKIN. > If the signal is detected, translator is present, otherwise the CKIN > feedback clock are disabled. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > Cc: Ludovic Barre <ludovic.barre@st.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-stm32@st-md-mailman.stormreply.com > --- > 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. > 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. I am wondering if it's really a good idea to support both states like this? > pinctrl-0 = <&sdmmc1_b4_pins_a &sdmmc1_dir_pins_a>; > pinctrl-1 = <&sdmmc1_b4_od_pins_a &sdmmc1_dir_pins_a>; > pinctrl-2 = <&sdmmc1_b4_sleep_pins_a &sdmmc1_dir_sleep_pins_a>; > + pinctrl-3 = <&sdmmc1_b4_init_pins_a &sdmmc1_dir_init_pins_a>; > cd-gpios = <&gpiog 1 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > disable-wp; > st,sig-dir; > st,neg-edge; > + st,use-ckin; > + st,cmd-gpios = <&gpiod 2 0>; > + st,ck-gpios = <&gpioc 12 0>; > + st,ckin-gpios = <&gpioe 4 0>; > bus-width = <4>; > vmmc-supply = <&vdd_sd>; > status = "okay"; > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index b5a41a7ce165..1bc674577ff9 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -36,6 +36,7 @@ > #include <linux/types.h> > #include <linux/pinctrl/consumer.h> > #include <linux/reset.h> > +#include <linux/gpio/consumer.h> > > #include <asm/div64.h> > #include <asm/io.h> > @@ -1888,6 +1889,65 @@ static struct mmc_host_ops mmci_ops = { > .start_signal_voltage_switch = mmci_sig_volt_switch, > }; > > +static void mmci_probe_level_translator(struct mmc_host *mmc) > +{ > + struct device *dev = mmc_dev(mmc); > + struct mmci_host *host = mmc_priv(mmc); > + struct gpio_desc *cmd_gpio; > + struct gpio_desc *ck_gpio; > + struct gpio_desc *ckin_gpio; > + int clk_hi, clk_lo; > + > + /* > + * Assume the level translator is present if st,use-ckin is set. > + * This is to cater for DTs which do not implement this test. > + */ > + host->clk_reg_add |= MCI_STM32_CLK_SELCKIN; > + > + cmd_gpio = gpiod_get(dev, "st,cmd", GPIOD_OUT_HIGH); > + if (IS_ERR(cmd_gpio)) > + goto exit_cmd; > + > + ck_gpio = gpiod_get(dev, "st,ck", GPIOD_OUT_HIGH); > + if (IS_ERR(ck_gpio)) > + goto exit_ck; > + > + ckin_gpio = gpiod_get(dev, "st,ckin", GPIOD_IN); > + if (IS_ERR(ckin_gpio)) > + goto exit_ckin; > + > + /* All GPIOs are valid, test whether level translator works */ > + > + /* Sample CKIN */ > + clk_hi = !!gpiod_get_value(ckin_gpio); > + > + /* Set CK low */ > + gpiod_set_value(ck_gpio, 0); > + > + /* Sample CKIN */ > + clk_lo = !!gpiod_get_value(ckin_gpio); > + > + /* Tristate all */ > + gpiod_direction_input(cmd_gpio); > + gpiod_direction_input(ck_gpio); > + > + /* Level translator is present if CK signal is propagated to CKIN */ > + if (!clk_hi || clk_lo) { > + host->clk_reg_add &= ~MCI_STM32_CLK_SELCKIN; > + dev_warn(dev, > + "Level translator inoperable, CK signal not detected on CKIN, disabling.\n"); > + } > + > + gpiod_put(ckin_gpio); > + > +exit_ckin: > + gpiod_put(ck_gpio); > +exit_ck: > + gpiod_put(cmd_gpio); > +exit_cmd: > + pinctrl_select_default_state(dev); > +} > + > 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. > > if (of_get_property(np, "mmc-cap-mmc-highspeed", NULL)) > mmc->caps |= MMC_CAP_MMC_HIGHSPEED; > @@ -1949,15 +2009,15 @@ static int mmci_probe(struct amba_device *dev, > if (!mmc) > return -ENOMEM; > > - ret = mmci_of_parse(np, mmc); > - if (ret) > - goto host_free; > - > host = mmc_priv(mmc); > host->mmc = mmc; > host->mmc_ops = &mmci_ops; > mmc->ops = &mmci_ops; > > + ret = mmci_of_parse(np, mmc); > + if (ret) > + goto host_free; > + > /* > * Some variant (STM32) doesn't have opendrain bit, nevertheless > * pins can be set accordingly using pinctrl > -- > 2.29.2 > Kind regards Uffe
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 ?
On Wed, 13 Jan 2021 at 12:34, Marek Vasut <marex@denx.de> 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
On 1/13/21 12:38 PM, Ulf Hansson wrote: [...] >>>> 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. Good, that's what I was hoping for too.
On 1/13/21 12:52 PM, Marek Vasut wrote: > On 1/13/21 12:38 PM, Ulf Hansson wrote: > [...] >>>>> 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. > > Good, that's what I was hoping for too. Hi, Ludovic is out of office this week. The feature of detecting a level translator seems to be quite generic, and not dedicated to MMCI driver or the ST dedicated code, and with new st,* properties. It may be in generic mmc code. But I'll let Linus comment about that. I also wonder if this HW detection should be done in kernel, or if it should be done in Bootloader. But it may be more complex, to add the st,use_ckin in kernel DT if bootloader detects this translator. Regards, Yann > _______________________________________________ > Linux-stm32 mailing list > Linux-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
On 1/13/21 3:21 PM, Yann GAUTIER wrote: > On 1/13/21 12:52 PM, Marek Vasut wrote: >> On 1/13/21 12:38 PM, Ulf Hansson wrote: >> [...] >>>>>> 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. >> >> Good, that's what I was hoping for too. > > Hi, > > Ludovic is out of office this week. > > The feature of detecting a level translator seems to be quite generic, > and not dedicated to MMCI driver or the ST dedicated code, and with new > st,* properties. It may be in generic mmc code. But I'll let Linus > comment about that. > > I also wonder if this HW detection should be done in kernel, or if it > should be done in Bootloader. But it may be more complex, to add the > st,use_ckin in kernel DT if bootloader detects this translator. Lets not attempt to hide inobvious functionality in bootloaders, the kernel should be independent of bootloader where possible. And here it is clearly and easily possible.
On 1/13/21 3:45 PM, Marek Vasut wrote: > On 1/13/21 3:21 PM, Yann GAUTIER wrote: >> On 1/13/21 12:52 PM, Marek Vasut wrote: >>> On 1/13/21 12:38 PM, Ulf Hansson wrote: >>> [...] >>>>>>> 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. >>> >>> Good, that's what I was hoping for too. >> >> Hi, >> >> Ludovic is out of office this week. >> >> The feature of detecting a level translator seems to be quite generic, >> and not dedicated to MMCI driver or the ST dedicated code, and with >> new st,* properties. It may be in generic mmc code. But I'll let Linus >> comment about that. >> >> I also wonder if this HW detection should be done in kernel, or if it >> should be done in Bootloader. But it may be more complex, to add the >> st,use_ckin in kernel DT if bootloader detects this translator. > > Lets not attempt to hide inobvious functionality in bootloaders, the > kernel should be independent of bootloader where possible. And here it > is clearly and easily possible. OK for this part, I understand it will be better not to hide this in bootloader. There is still the previous point for which Linus may help. Regards, Yann
Hi Marek, thanks for your patch! In general this patch is pretty much how I imagine I would have solved it myself. It's a really fringe situation but STM32 is pushing the envelope with this block so here we are. The pinmux core is definitely designed to handle stuff like this and I'm happy that it seems to work for you. On Tue, Jan 5, 2021 at 3:08 PM Marek Vasut <marex@denx.de> wrote: > NOTE: I would prefer this solution over having a custom DT per SoM, > since it reduces the amount of DT combinations. I don't see any problem with this approach. > &sdmmc1 { > - pinctrl-names = "default", "opendrain", "sleep"; > + pinctrl-names = "default", "opendrain", "sleep", "init"; > pinctrl-0 = <&sdmmc1_b4_pins_a &sdmmc1_dir_pins_a>; > pinctrl-1 = <&sdmmc1_b4_od_pins_a &sdmmc1_dir_pins_a>; > pinctrl-2 = <&sdmmc1_b4_sleep_pins_a &sdmmc1_dir_sleep_pins_a>; > + pinctrl-3 = <&sdmmc1_b4_init_pins_a &sdmmc1_dir_init_pins_a>; > cd-gpios = <&gpiog 1 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > disable-wp; > st,sig-dir; > st,neg-edge; > + st,use-ckin; > + st,cmd-gpios = <&gpiod 2 0>; > + st,ck-gpios = <&gpioc 12 0>; > + st,ckin-gpios = <&gpioe 4 0>; Fair enough, when submitting the final device tree, add som verbose comments as to what is going on here so people get it. I got reminded that the MMCI bindings are not converted to device tree so I spent some time on that. I will send out an RFC. > +static void mmci_probe_level_translator(struct mmc_host *mmc) This probing function looks good. > if (of_get_property(np, "st,use-ckin", NULL)) > - host->clk_reg_add |= MCI_STM32_CLK_SELCKIN; > + mmci_probe_level_translator(mmc); This activates the probing based on solely the existance of this device tree flag. It's not a problem in this patch but we should probably only look for some of these attributes if we determine that it's an STM32 platform block. Yours, Linus Walleij
On Wed, Jan 13, 2021 at 12:34 PM Marek Vasut <marex@denx.de> wrote: > 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". This is correct. "init" is optional and especially for situations like this one. Yours, Linus Walleij
On Thu, Jan 14, 2021 at 11:13 AM Yann GAUTIER <yann.gautier@foss.st.com> wrote: > On 1/13/21 3:45 PM, Marek Vasut wrote: > > On 1/13/21 3:21 PM, Yann GAUTIER wrote: > >> On 1/13/21 12:52 PM, Marek Vasut wrote: > >> I also wonder if this HW detection should be done in kernel, or if it > >> should be done in Bootloader. But it may be more complex, to add the > >> st,use_ckin in kernel DT if bootloader detects this translator. > > > > Lets not attempt to hide inobvious functionality in bootloaders, the > > kernel should be independent of bootloader where possible. And here it > > is clearly and easily possible. > > OK for this part, I understand it will be better not to hide this in > bootloader. We all agree. I am against bootloaderism, the tendency to toss all complex hardware detection over the wall and call it a bootloader problem. Let's proceed with Marek's solution. Yours, Linus Walleij
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"; pinctrl-0 = <&sdmmc1_b4_pins_a &sdmmc1_dir_pins_a>; pinctrl-1 = <&sdmmc1_b4_od_pins_a &sdmmc1_dir_pins_a>; pinctrl-2 = <&sdmmc1_b4_sleep_pins_a &sdmmc1_dir_sleep_pins_a>; + pinctrl-3 = <&sdmmc1_b4_init_pins_a &sdmmc1_dir_init_pins_a>; cd-gpios = <&gpiog 1 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; disable-wp; st,sig-dir; st,neg-edge; + st,use-ckin; + st,cmd-gpios = <&gpiod 2 0>; + st,ck-gpios = <&gpioc 12 0>; + st,ckin-gpios = <&gpioe 4 0>; bus-width = <4>; vmmc-supply = <&vdd_sd>; status = "okay"; diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index b5a41a7ce165..1bc674577ff9 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -36,6 +36,7 @@ #include <linux/types.h> #include <linux/pinctrl/consumer.h> #include <linux/reset.h> +#include <linux/gpio/consumer.h> #include <asm/div64.h> #include <asm/io.h> @@ -1888,6 +1889,65 @@ static struct mmc_host_ops mmci_ops = { .start_signal_voltage_switch = mmci_sig_volt_switch, }; +static void mmci_probe_level_translator(struct mmc_host *mmc) +{ + struct device *dev = mmc_dev(mmc); + struct mmci_host *host = mmc_priv(mmc); + struct gpio_desc *cmd_gpio; + struct gpio_desc *ck_gpio; + struct gpio_desc *ckin_gpio; + int clk_hi, clk_lo; + + /* + * Assume the level translator is present if st,use-ckin is set. + * This is to cater for DTs which do not implement this test. + */ + host->clk_reg_add |= MCI_STM32_CLK_SELCKIN; + + cmd_gpio = gpiod_get(dev, "st,cmd", GPIOD_OUT_HIGH); + if (IS_ERR(cmd_gpio)) + goto exit_cmd; + + ck_gpio = gpiod_get(dev, "st,ck", GPIOD_OUT_HIGH); + if (IS_ERR(ck_gpio)) + goto exit_ck; + + ckin_gpio = gpiod_get(dev, "st,ckin", GPIOD_IN); + if (IS_ERR(ckin_gpio)) + goto exit_ckin; + + /* All GPIOs are valid, test whether level translator works */ + + /* Sample CKIN */ + clk_hi = !!gpiod_get_value(ckin_gpio); + + /* Set CK low */ + gpiod_set_value(ck_gpio, 0); + + /* Sample CKIN */ + clk_lo = !!gpiod_get_value(ckin_gpio); + + /* Tristate all */ + gpiod_direction_input(cmd_gpio); + gpiod_direction_input(ck_gpio); + + /* Level translator is present if CK signal is propagated to CKIN */ + if (!clk_hi || clk_lo) { + host->clk_reg_add &= ~MCI_STM32_CLK_SELCKIN; + dev_warn(dev, + "Level translator inoperable, CK signal not detected on CKIN, disabling.\n"); + } + + gpiod_put(ckin_gpio); + +exit_ckin: + gpiod_put(ck_gpio); +exit_ck: + gpiod_put(cmd_gpio); +exit_cmd: + pinctrl_select_default_state(dev); +} + 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); if (of_get_property(np, "mmc-cap-mmc-highspeed", NULL)) mmc->caps |= MMC_CAP_MMC_HIGHSPEED; @@ -1949,15 +2009,15 @@ static int mmci_probe(struct amba_device *dev, if (!mmc) return -ENOMEM; - ret = mmci_of_parse(np, mmc); - if (ret) - goto host_free; - host = mmc_priv(mmc); host->mmc = mmc; host->mmc_ops = &mmci_ops; mmc->ops = &mmci_ops; + ret = mmci_of_parse(np, mmc); + if (ret) + goto host_free; + /* * Some variant (STM32) doesn't have opendrain bit, nevertheless * pins can be set accordingly using pinctrl
Add support for testing whether bus voltage level translator is present and operational. This is useful on systems where the bus voltage level translator is optional, as the translator can be auto-detected by the driver and the feedback clock functionality can be disabled if it is not present. This requires additional pinmux state, "init", where the CMD, CK, CKIN lines are not configured, so they can be claimed as GPIOs early on in probe(). The translator test sets CMD high to avoid interfering with a card, and then verifies whether signal set on CK is detected on CKIN. If the signal is detected, translator is present, otherwise the CKIN feedback clock are disabled. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@st.com> Cc: Ludovic Barre <ludovic.barre@st.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: linux-stm32@st-md-mailman.stormreply.com --- 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 ++++++++++++++++++-- 2 files changed, 96 insertions(+), 6 deletions(-)