[PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Marek Vasut]

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(-)

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
-- 
2.29.2

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Ulf Hansson]

+ 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

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Marek Vasut]

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 ?

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Ulf Hansson]

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

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Marek Vasut]

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.

Re: [Linux-stm32] [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Yann GAUTIER]

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

Re: [Linux-stm32] [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Marek Vasut]

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.

Re: [Linux-stm32] [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Yann GAUTIER]

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

Re: [Linux-stm32] [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[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

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[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

Re: [PATCH] [RFC] mmc: mmci: Add support for probing bus voltage level translator

[Linus Walleij]

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