Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753358AbcLFNm2 (ORCPT ); Tue, 6 Dec 2016 08:42:28 -0500 Received: from smtpoutz300.laposte.net ([178.22.154.200]:36556 "EHLO smtp.laposte.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752827AbcLFNm0 (ORCPT ); Tue, 6 Dec 2016 08:42:26 -0500 Subject: Re: Adding a .platform_init callback to sdhci_arasan_ops To: Doug Anderson References: <982d633b-e9c4-0f10-052b-e324f094d0f5@xilinx.com> <2a949ade-edd7-4690-cd6a-434ae1e663dc@intel.com> <66751ab5-59a4-ec30-07cd-44ca03694723@laposte.net>

<9d63ea01-a5d1-603d-1ad4-6c6320022de8@laposte.net> Cc: Adrian Hunter , Michal Simek , =?UTF-8?Q?S=c3=b6ren_Brinkmann?= , Jerry Huang , Ulf Hansson , Linux ARM , LKML , Linus Walleij , Mason , P L Sai Krishna , Arnd Bergmann From: Sebastian Frias Message-ID: Date: Tue, 6 Dec 2016 14:42:21 +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: gggruggvucftvghtrhhoucdtuddrfeelfedrhedtgddvgecutefuodetggdotefrodftvfcurfhrohhf 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: shhfkeegsehlrghpohhsthgvrdhnvghtpdhrtghpthhtohepughirghnuggvrhhssegthhhrohhmihhu X-VR-Cause-8: mhdrohhrgh X-VR-AvState: No X-VR-State: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5760 Lines: 144 Hi, On 05/12/16 17:30, Doug Anderson wrote: > > AKA: you are setting up various "corecfg" stuff that's documented in > the generic Arasan docs. Others SDHCI-Arasan implementations might > want to set the same things, but those values may be stored elsewhere > for them. Exactly, that is what I'm trying to find out. To me, one good place to store this would be DT. > > So if _all_ Arasan implementations need these same values or need the > same logic to figure out these values, then you should do something > that's not chip-specific but something generic. > It depends on where this needs to be set, and why am I the first to need to set this up. > If you've got a specific weird quirk that's specific to your platform, > then you could do that in a chip-specific init. Yes, or in the set_ios you talked earlier. > > Presumably many of the above could just be hardcoded on some > implementations, so they might not be available in a memory-mapped > implementation... > Exactly. > >> which seems much easier to handle (and portable). >> >> At any rate, one thing to note from this is that many of these >> bits should probably be initialised based on DT, right? > > Probably, or by proving the voltage value of regulations. Note that I > think DT already gets parsed and sets up caps. I'm not really an > expert here and I'd let someone who actually knows / maintains SDMMC > comment. I know for sure that dw_mmc (which I'm way more familiar > with) does things very differently than sdhci (which I'm barely > familiar with). Thanks. Could somebody else comment please? Best regards, Sebastian > > >> For example, the DT has a "non-removable" property, which I think >> should be used to setup SLOT_TYPE_EMBEDDED (if the property is >> present) or SLOT_TYPE_REMOVABLE (if the property is not present) >> >> Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see >> more related properties: >> >> Optional properties: >> - bus-width: Number of data lines, can be <1>, <4>, or <8>. The default >> will be <1> if the property is absent. >> - wp-gpios: Specify GPIOs for write protection, see gpio binding >> - cd-inverted: when present, polarity on the CD line is inverted. See the note >> below for the case, when a GPIO is used for the CD line >> - wp-inverted: when present, polarity on the WP line is inverted. See the note >> below for the case, when a GPIO is used for the WP line >> - disable-wp: When set no physical WP line is present. This property should >> only be specified when the controller has a dedicated write-protect >> detection logic. If a GPIO is always used for the write-protect detection >> logic it is sufficient to not specify wp-gpios property in the absence of a WP >> line. >> - max-frequency: maximum operating clock frequency >> - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on >> this system, even if the controller claims it is. >> - cap-sd-highspeed: SD high-speed timing is supported >> - cap-mmc-highspeed: MMC high-speed timing is supported >> - sd-uhs-sdr12: SD UHS SDR12 speed is supported >> - sd-uhs-sdr25: SD UHS SDR25 speed is supported >> - sd-uhs-sdr50: SD UHS SDR50 speed is supported >> - sd-uhs-sdr104: SD UHS SDR104 speed is supported >> - sd-uhs-ddr50: SD UHS DDR50 speed is supported >> ... >> >> which makes me wonder, what is the recommended way of dealing with this? >> - Should I use properties on the DT? If so, then I need to add code to set >> up the register properly. >> - Should I hardcode these values use a minimal DT? If so, then I need an >> init function to put all this. >> - Should I hardcode stuff at u-Boot level? If so, nothing is required and >> should work without any modifications to the Arasan Linux driver. >> >> It appears that the Linux driver is expecting most of these fields to be >> hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence >> the lack of any "init" function so far. >> >>> >>> In your platform-specific init you're proposing you could set >>> tango_pad_mode to 0. That seems tango-specific. >>> >>> You'd want to hook into "set_ios" for setting sel_sdio or not. That's >>> important if anyone ever wants to plug in an external SDIO card to >>> your slot. This one good argument for putting this in >>> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a >>> compatibility matching every time set_ios is called. >> >> Thanks for the advice, I will look into that. >> >>> >>> I'd have to look more into the whole SD/WP polarity issue. There are >>> already so many levels of inversion for these signals in Linux that >>> it's confusing. It seems like you might just want to hardcode them to >>> "0" and let users use all the existing ways to invert things... You >>> could either put that hardcoding into your platform init code or (if >>> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so >>> that if anyone else needs to init similar signals then they can take >>> advantage of it. >> >> Yes, I think I will leave them to 0. >> >>> >>> -- >>> >>> One random side note is that as currently documented you need to >>> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you >>> aren't using. That seems stupid--not sure why I did that. It seems >>> better to clue off "width = 0" so that you could just freely not init >>> any fields you don't need. >> >> I see. >> So far I'm not really convinced about using "soc_ctl_map" because what I >> have so far is more portable, and can easily be put as is somewhere else >> (i.e.: in different flavors of bootloaders) > > Well, most of your parameters are generic corecfg parameters for > Asasan. Seems like they would fit into the map nicely... > > -Doug >