[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