[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.