linux-kernel - Re: [PATCH v5 3/3] mmc: Add driver for LiteX's LiteSDCard interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdciJ2kVHzXCoq4r@errol.ini.cmu.edu>
Date:   Thu, 6 Jan 2022 12:08:55 -0500
From:   "Gabriel L. Somlo" <gsomlo@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        devicetree@...r.kernel.org, linux-mmc@...r.kernel.org,
        kgugala@...micro.com, mholenko@...micro.com, krakoczy@...micro.com,
        mdudek@...ernships.antmicro.com, paulus@...abs.org, joel@....id.au,
        shorne@...il.com, geert@...ux-m68k.org,
        david.abdurachmanov@...ive.com, florent@...oy-digital.fr,
        rdunlap@...radead.org
Subject: Re: [PATCH v5 3/3] mmc: Add driver for LiteX's LiteSDCard interface

On Mon, Jan 03, 2022 at 07:27:28PM -0500, Gabriel L. Somlo wrote:
> On Tue, Dec 28, 2021 at 05:15:25PM +0100, Ulf Hansson wrote:
> > I noticed that you use these hard coded values and don't really care
> > to manage voltage changes via ->set_ios().
> > 
> > Rather than doing it like this, I would prefer if you can hook up a
> > fixed vmmc regulator in the DTS. Then call mmc_regulator_get_supply()
> > to fetch it from here, which will let the mmc core create the
> > mmc->ocr_avail mask, based upon the voltage level the regulator
> > supports.
> > 
> > This becomes more generic and allows more flexibility for the platform
> > configuration.
> 
> The LiteSDCard "hardware" (i.e., *gateware*) does not allow modification
> or selection of voltage from the software side. When a CMD8 is issued,
> the "voltage supplied" bit pattern is expected to be '0001b', which per
> the spec means "2.7-3.6V". 
> 
> I tried adding this to the overall DTS:
> 
> 	vreg_mmc: vreg_mmc_3v {
> 		compatible = "regulator-fixed";
> 		regulator-min-microvolt = <3300000>;
> 		regulator-max-microvolt = <3300000>;
> 	};
> 
> and then added a reference to it to the LiteSDCard "mmc0" node in DTS,
> like so:
> 
> 	mmc0: mmc@...05000 {
> 		compatible = "litex,mmc";
> 		reg = <0x12005000 0x100>,
> 			<0x12003800 0x100>,
> 			<0x12003000 0x100>,
> 			<0x12004800 0x100>,
> 			<0x12004000 0x100>;
> 		reg-names = "phy", "core", "reader", "writer", "irq";
> 		clocks = <&sys_clk>;
> 		vmmc-supply = <&vreg_mmc>; /* <-------- HERE !!! */
> 		interrupt-parent = <&L1>;
> 		interrupts = <4>;
> 	};
> 
> Finally, I replaced the hardcoded setting of `mmc->ocr_avail` with a
> call to `mmc_regulator_get_supply(mmc)`. Now, I get a bunch of timeouts
> during attempts to send e.g., CMD8 and CMD55.
> (going for 3200000 and 3400000 for min- and max-microvolt, respectively,
>  -- or anything else in the allowed 2.7-3.6 range -- doesn't help either).
> 
> I might be doing something subtly wrong in the way I set things up
> above, but it feels a bit overengineered, and IMHO fragile.
> 
> OTOH, going all out and setting:
> 
>         /* allow for generic 2.7-3.6V range, no software tuning available */
>         mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
>                          MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
>                          MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
> 
> seems to work just fine... :) Please do let me know what you think!

I dug around `drivers/mmc/core/regulator.c` a bit more, and it turns
out `mmc_regulator_get_supply()` is allowed to return 0 even if not
all regulators have been found, "because they all are optional", and
I still need to write additional code to check if my regulator got
populated -- I assume that means checking if `mmc->ocr_avail` was set
to something useful, or whether it's still 0.

In my case, with the above-mentioned modifications in DTS, I still end
up with `mmc->ocr_avail == 0` after calling `mmc_regulator_get_supply()`,
which explains why the card doesnoesn't work correctly after being
probed.

Not quite sure what to do in that situation -- any ideas?

I still think it's a bit overkill to set up a dummy regulator in DTS
and probe for it when the "hardware" doesn't actually support
variable/configurable voltages or dynamic changes in voltage -- a
hard-coded constant somehow feels more appropriate, wouldn't you
agree?

IMHO, it makes more sense to define the entire generic/standard range
described in the SDCard specification (2.7-3.6V) as a constant, e.g.:

#define LITEX_MMC_OCR (MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 | \
                       MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 | \
                       MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36)

and then initialize `mmc->ocr_avail = LITEX_MMC_OCR` to that instead.
This is how they do it in drivers/mmc/host/au1xmmc.c, for instance.

I'm happy to learn more about why going the DTS-dummy-regulator
configurable route is better, so let me know what you think. I'm going
to send out v6 with the hard-coded constant version above soon, unless
I hear back from you before then. But we can always go another round
(i.e., v7) unless you agree with my argument -- please let me know
either way! :)

Thanks again,
--Gabriel