Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754557AbcK1LUz (ORCPT ); Mon, 28 Nov 2016 06:20:55 -0500 Received: from smtpoutz299.laposte.net ([178.22.154.199]:50621 "EHLO smtpoutz2.laposte.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753722AbcK1LUr (ORCPT ); Mon, 28 Nov 2016 06:20:47 -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> Cc: Linux ARM , LKML , Linus Walleij , Mason , P L Sai Krishna From: Sebastian Frias Message-ID: <66751ab5-59a4-ec30-07cd-44ca03694723@laposte.net> Date: Mon, 28 Nov 2016 12:20:42 +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: <2a949ade-edd7-4690-cd6a-434ae1e663dc@intel.com> 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: gggruggvucftvghtrhhoucdtuddrfeelfedrfeeigddvkecutefuodetggdotefrodftvfcurfhrohhf 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: 4075 Lines: 129 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? > > 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? 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. >>> >> >> >