Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754656AbcK1LuI (ORCPT ); Mon, 28 Nov 2016 06:50:08 -0500 Received: from mga01.intel.com ([192.55.52.88]:6211 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754036AbcK1LuA (ORCPT ); Mon, 28 Nov 2016 06:50:00 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,563,1473145200"; d="scan'208";a="1091439588" Subject: Re: Adding a .platform_init callback to sdhci_arasan_ops To: Sebastian Frias , Michal Simek , Douglas Anderson , =?UTF-8?Q?S=c3=b6ren_Brinkmann?= , Jerry Huang , Ulf Hansson References: <982d633b-e9c4-0f10-052b-e324f094d0f5@xilinx.com> <2a949ade-edd7-4690-cd6a-434ae1e663dc@intel.com> <66751ab5-59a4-ec30-07cd-44ca03694723@laposte.net> Cc: Linux ARM , LKML , Linus Walleij , Mason , P L Sai Krishna From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: Date: Mon, 28 Nov 2016 13:44:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <66751ab5-59a4-ec30-07cd-44ca03694723@laposte.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4792 Lines: 151 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 > 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 >>>> 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. >>>> >>> >>> >> > >