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 ?
