Date:   Mon, 30 Jan 2017 23:12:10 +0800
From:   Ziji Hu <huziji@...vell.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Jimmy Xu <zmxu@...vell.com>,
        Jisheng Zhang <jszhang@...vell.com>,
        Nadav Haklai <nadavh@...vell.com>, Ryan Gao <ygao@...vell.com>,
        Doug Jones <dougj@...vell.com>, Victor Gu <xigu@...vell.com>,
        "Wei(SOCP) Liu" <liuw@...vell.com>,
        Wilson Ding <dingwei@...vell.com>,
        Yehuda Yitschak <yehuday@...vell.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Hanna Hawa <hannah@...vell.com>,
        Kostya Porotchkin <kostap@...vell.com>
Subject: Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core
 functionality

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
> 

Powered by blists - more mailing lists