Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality

[Ziji Hu <huziji@marvell.com>]

Hi Ulf,

On 2017/1/30 16:41, Ulf Hansson wrote:
> [...]
> 
>>>> + */
>>>> +static int xenon_child_node_of_parse(struct platform_device *pdev)
>>>> +{
>>>> +       struct device_node *np = pdev->dev.of_node;
>>>> +       struct sdhci_host *host = platform_get_drvdata(pdev);
>>>> +       struct mmc_host *mmc = host->mmc;
>>>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +       struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>> +       struct device_node *child;
>>>> +       int nr_child;
>>>> +
>>>> +       priv->init_card_type = XENON_CARD_TYPE_UNKNOWN;
>>>> +
>>>> +       nr_child = of_get_child_count(np);
>>>> +       if (!nr_child)
>>>> +               return 0;
>>>> +
>>>> +       for_each_child_of_node(np, child) {
>>>> +               if (of_device_is_compatible(child, "mmc-card")) {
>>>
>>> To avoid code from being duplicated, I would rather see the DT parsing
>>> of the child nodes for "mmc-card", to be done by the mmc core.
>>>
>>> As a matter of fact it is already being done, but perhaps we need to
>>> change that a bit as to fit your case.
>>>
>>> I suggest you have a look and see how to update this in the core,
>>> instead of doing it here in the host driver.
>>>
>>
>>     I understand your concern.
>>
>>     It seems that so far "mmc-card" is only used in our Xenon driver.
> 
> git grep "mmc-card" tells you more about where it's being parsed by
> the mmc core.
> 
>>     Besides, we set Xenon specific parameters and attributions when
>>     parsing "mmc-card" property.
> 
> I don't see any Xenon specific properties.
> 
> Instead I think it would make sense to update the generic
> interpretation of the binding for mmc-card, as you have done here.
> 

   OK. I will merge it into core layer.
 
   Our Xenon driver requires to determine whether current SDHC is
   for eMMC before card init. Thus I would like to implement a specific
   function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c
   to call it.
   But there are some detailed issues then.
   1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi.
      So mmc-card parse will still be duplicated.
      Shall we merge broken-hpi check into mmc-card parse?
      It might require a new flag to indicate broken-hpi attribution in
      mmc_host structure.

   2. Structure mmc_card is not ready while parsing mmc-card.
      Thus we are not able to indicate card type in mmc_card.
      As a result, our Xenon specific parse function still has to
      parse mmc-card again to check if eMMC card is in used.

   Could you please help suggest any better place in core layer to
   parse mmc-card?

   Thank you.

Best regards,
Hu Ziji

>>
>>     May I keep current implementation?
> 
> Rather not. Let's try to make the parsing of the child node for
> mmc-card generic.
> 
>>     In my very own opinion, moving it into core layer should be another
>>     independent patch.
> 
> Absolutely an independent patch. Whether it can be done as a part of
> mmc_of_parse() or whether we need yet another new mmc_of* API, we can
> discuss.
> 
> My point is that, I don't this to be specific for Xenon (unless there
> are specific reason, which I don't see here).
> 
>>     And it will also cost some more time. To be honest, it is difficult
>>     for me to bring up a generic core layer implementation to parse
>>     "mmc-card", since I'm not clear about other vendor's requirement.
> 
> Well, then you need to learn more about how the mmc core works. In
> this case, it shouldn't be too hard to implement.
> 
> [...]
> 
>>
>>>
>>>> +                                     MMC_CAP2_NO_SD |
>>>> +                                     MMC_CAP2_NO_SDIO;
>>>
>>> I think we need to update the DT documentation for mmc-card.
>>> Typically, we should explicitly state what kind of other existing mmc
>>> DT properties that will be automatically selected, when the mmc-card
>>> is specified.
>>>
>>> Can you please look into updating this DT doc as well.
>>>
>>
>>   Similar to above, may I keep it now and bring up another patch later
>>   to update mmc-card DT and parsing?
> 
> Please, no.
> 
> [...]
> 
> Kind regards
> Uffe
>