+ Linus (GPIO/pinctrl maintainer)
On Tue, 5 Jan 2021 at 15:07, Marek Vasut <marex@xxxxxxx> 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@xxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>
> Cc: Ludovic Barre <ludovic.barre@xxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> ---
> 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
