[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d1801c1e-cd74-b79c-0280-8fb5dc377c91@intel.com>
Date: Mon, 28 Nov 2016 13:44:47 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Sebastian Frias <sf84@...oste.net>,
Michal Simek <michal.simek@...inx.com>,
Douglas Anderson <dianders@...omium.org>,
Sören Brinkmann <soren.brinkmann@...inx.com>,
Jerry Huang <Chang-Ming.Huang@...escale.com>,
Ulf Hansson <ulf.hansson@...aro.org>
Cc: Linux ARM <linux-arm-kernel@...ts.infradead.org>,
LKML <linux-kernel@...r.kernel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Mason <slash.tmp@...e.fr>, P L Sai Krishna <lakshmis@...inx.com>
Subject: Re: Adding a .platform_init callback to sdhci_arasan_ops
On 28/11/16 13:20, Sebastian Frias wrote:
> Hi Adrian,
>
> On 28/11/16 11:30, Adrian Hunter wrote:
>> On 28/11/16 09:32, Michal Simek wrote:
>>> +Sai for Xilinx perspective.
>>>
>>> On 25.11.2016 16:24, Sebastian Frias wrote:
>>>> Hi,
>>>>
>>>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>>>> "Hardware initialized registers"
>>>>
>>>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>>>> AHB Host Controller", revision 6.0 document)
>>>>
>>>> In some platforms those signals are connected to registers that need to
>>>> be programmed at some point for proper driver/HW initialisation.
>>>>
>>>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>>>> that is called from within 'sdhci_pltfm_init', and that seems a good
>>>> candidate for a place to program those registers (*).
>>>>
>>>> Do you agree?
>>
>> We already killed .platform_init
>
> I just saw that, yet it was the perfect place for the HW initialisation I'm
> talking about.
> Any way we can restore it?
It doesn't serve any purpose I am aware of.
>
>>
>> What is wrong with sdhci_arasan_probe()?
>
> Well, in 4.7 sdhci_arasan_probe() did not call of_match_device(), so I had
> put a call to it just before sdhci_pltfm_init(), something like:
>
> +static const struct of_device_id sdhci_arasan_of_match[] = {
> + {
> + .compatible = "arasan,sdhci-8.9a",
> + .data = &sdhci_arasan_ops,
> + },
> + {
> + .compatible = "arasan,sdhci-5.1",
> + .data = &sdhci_arasan_ops,
> + },
> + {
> + .compatible = "arasan,sdhci-4.9a",
> + .data = &sdhci_arasan_ops,
> + },
> + {
> + .compatible = "sigma,smp8734-sdio",
> + .data = &sdhci_arasan_tango4_ops,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>
> ...
>
> + const struct of_device_id *match;
> +
> + match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
> + if (match)
> + sdhci_arasan_pdata.ops = match->data;
>
> where 'sdhci_arasan_tango4_ops' contained a pointer to a .platform_init
> callback.
>
> However, as I stated earlier, an upstream commit:
>
> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
> Author: Douglas Anderson <dianders@...omium.org>
> Date: Mon Jun 20 10:56:47 2016 -0700
>
> mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>
> changed struct 'sdhci_arasan_of_match' to convey different data, which
> means that instead of having a generic way of accessing such data (such
> as 'of_match_device()' and ".data" field), one must also check for
> specific "compatible" strings to make sense of the ".data" field, such as
> "rockchip,rk3399-sdhci-5.1"
>
> With the current code:
> - there's no 'of_match_device()' before 'sdhci_pltfm_init()'
> - the sdhci_pltfm_init() call is made with a static 'sdhci_arasan_pdata'
> struct (so it cannot be made dependent on the "compatible" string).
> - since 'sdhci_arasan_pdata' is the same for all compatible devices, even
> for those that require special handling, more "compatible" matching code is
> required
> - leading to spread "compatible" matching code; IMHO it would be cleaner if
> the 'sdhci_arasan_probe()' code was generic, with just a generic "compatible"
> matching, which then proceeded with specific initialisation and generic
> initialisation.
>
> In a nutshell, IMHO it would be better if adding support for more SoCs only
> involved changing just 'sdhci_arasan_of_match' without the need to change
> 'sdhci_arasan_probe()'.
> That would clearly separate the generic and "SoC"-specific code, thus allowing
> better maintenance.
>
> Does that makes sense to you guys?
If you want to do that, then why not define your match data with your own
callbacks. e.g. something like
struct sdhci_arasan_of_data {
struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
void (*platform_init)(struct sdhci_arasan_data *sdhci_arasan);
};
struct sdhci_arasan_of_data *data;
data = match->data;
sdhci_arasan->soc_ctl_map = data->soc_ctl_map;
if (data->platform_init)
platform_init(sdhci_arasan);
>
> Best regards,
>
> Sebastian
>
>>
>>>>
>>>> Best regards,
>>>>
>>>> Sebastian
>>>>
>>>>
>>>> (*): This has been prototyped on 4.7 as working properly.
>>>> However, upstream commit:
>>>>
>>>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>>>> Author: Douglas Anderson <dianders@...omium.org>
>>>> Date: Mon Jun 20 10:56:47 2016 -0700
>>>>
>>>> mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>>>> ...
>>>>
>>>> could affect this solution because of the way the 'sdhci_arasan_of_match'
>>>> struct is used after that commit.
>>>>
>>>
>>>
>>
>
>
Powered by blists - more mailing lists