Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932589AbcK1N2X (ORCPT ); Mon, 28 Nov 2016 08:28:23 -0500 Received: from smtpoutz299.laposte.net ([178.22.154.199]:57430 "EHLO smtpoutz2.laposte.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932248AbcK1N2R (ORCPT ); Mon, 28 Nov 2016 08:28:17 -0500 Subject: Re: Adding a .platform_init callback to sdhci_arasan_ops To: Adrian Hunter , 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: Sebastian Frias Message-ID: Date: Mon, 28 Nov 2016 14:28:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-VR-SrcIP: 92.154.11.170 X-VR-FullState: 0 X-VR-Score: -100 X-VR-Cause-1: gggruggvucftvghtrhhoucdtuddrfeelfedrfeeigdehtdcutefuodetggdotefrodftvfcurfhrohhf X-VR-Cause-2: ihhlvgemucfntefrqffuvffgnecuuegrihhlohhuthemucehtddtnecusecvtfgvtghiphhivghnthhs X-VR-Cause-3: ucdlqddutddtmdenucfjughrpefuvfhfhffkffgfgggjtgfgsehtjeertddtfeejnecuhfhrohhmpefu X-VR-Cause-4: vggsrghsthhirghnucfhrhhirghsuceoshhfkeegsehlrghpohhsthgvrdhnvghtqeenucfkphepledv X-VR-Cause-5: rdduheegrdduuddrudejtdenucfrrghrrghmpehmohguvgepshhmthhpohhuthdphhgvlhhopegludej X-VR-Cause-6: vddrvdejrddtrddvudegngdpihhnvghtpeelvddrudehgedruddurddujedtpdhmrghilhhfrhhomhep X-VR-Cause-7: shhfkeegsehlrghpohhsthgvrdhnvghtpdhrtghpthhtoheprggurhhirghnrdhhuhhnthgvrhesihhn X-VR-Cause-8: thgvlhdrtghomh X-VR-AvState: No X-VR-State: 0 X-VR-State: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7933 Lines: 233 On 28/11/16 12:44, Adrian Hunter wrote: > 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. It would serve (for me) if it was there :-) > >> >>> >>> 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); Well, that adds a level in the hierarchy, but here is what it would look like: diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 410a55b..1cb3861 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend, sdhci_arasan_resume); -static const struct of_device_id sdhci_arasan_of_match[] = { - /* SoC-specific compatible strings w/ soc_ctl_map */ - { - .compatible = "rockchip,rk3399-sdhci-5.1", - .data = &rk3399_soc_ctl_map, - }, - - /* Generic compatible below here */ - { .compatible = "arasan,sdhci-8.9a" }, - { .compatible = "arasan,sdhci-5.1" }, - { .compatible = "arasan,sdhci-4.9a" }, - - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); - /** * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate * @@ -578,6 +562,53 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev) of_clk_del_provider(dev->of_node); } +static void sdhci_tango4_platform_init(struct sdhci_host *host) +{ + printk("%s\n", __func__); + + /* + pad_mode[2:0]=0 must be 0 + sel_sdio[3]=1 must be 1 for SDIO + inv_sdwp_pol[4]=0 if set inverts the SD write protect polarity + inv_sdcd_pol[5]=0 if set inverts the SD card present polarity + */ + sdhci_writel(host, 0x00000008, 0x100 + 0x0); +} + +struct sdhci_arasan_chip_specific_data { + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; + void (*platform_init)(struct sdhci_host *host); +}; + +static const struct sdhci_arasan_chip_specific_data sdhci_arasan_rockchip = { + .soc_ctl_map = &rk3399_soc_ctl_map, +}; + +static const struct sdhci_arasan_chip_specific_data sdhci_arasan_sigma = { + .platform_init = sdhci_tango4_platform_init, +}; + +static const struct of_device_id sdhci_arasan_of_match[] = { + /* SoC-specific compatible strings w/ soc_ctl_map */ + { + .compatible = "rockchip,rk3399-sdhci-5.1", + .data = &sdhci_arasan_rockchip, + }, + { + .compatible = "sigma,sdio-v1", + .data = &sdhci_arasan_sigma, + }, + + /* Generic compatible below here */ + { .compatible = "arasan,sdhci-8.9a" }, + { .compatible = "arasan,sdhci-5.1" }, + { .compatible = "arasan,sdhci-4.9a" }, + + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); + + static int sdhci_arasan_probe(struct platform_device *pdev) { int ret; @@ -587,6 +618,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev) struct sdhci_host *host; struct sdhci_pltfm_host *pltfm_host; struct sdhci_arasan_data *sdhci_arasan; + struct sdhci_arasan_chip_specific_data *sdhci_arasan_chip_specific; struct device_node *np = pdev->dev.of_node; host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, @@ -599,7 +631,11 @@ static int sdhci_arasan_probe(struct platform_device *pdev) sdhci_arasan->host = host; match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); - sdhci_arasan->soc_ctl_map = match->data; + sdhci_arasan_chip_specific = (struct sdhci_arasan_chip_specific_data *)match; + if (sdhci_arasan_chip_specific->soc_ctl_map) + sdhci_arasan->soc_ctl_map = sdhci_arasan_chip_specific->soc_ctl_map; + if (sdhci_arasan_chip_specific->platform_init) + sdhci_arasan_chip_specific->platform_init(host); node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0); if (node) { I will try to send another patch with what a different approach