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

From: Ulf Hansson
Date: Wed Jan 12 2022 - 05:25:16 EST


[...]

> > > > [...]
> > > >
> > > > > +
> > > > > + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > >
> > > > 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".
> >
> > If you provide a range (2.7-3.6V), that means that your hardware
> > supports the entire range, not just one single part of it.
>
> The "gateware" (open source migen/verilog at
> https://github.com/enjoy-digital/litesdcard)
> supports any value provided by the underlying FPGA dev board
> (typically 3.3v) -- by not attempting to manage it in any way.
>
> SD media presumably doesn't care as long as voltage is somewhere
> within 2.7-3.6V (at least that's how I read the spec, there's only
> one register value representing anything within that range).
>
> > >
> > > 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@12005000 {
> > > 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.
> >
> > At a quick glance, the above looks correct to me. Maybe there is
> > something wrong with the code in the driver instead?
>
> After some more hacking, I learned that:
>
> - an additional `regulator-name` line
> (e.g. `regulator-name = "vreg_mmc";`) is required
>
> - setting `regulator-always-on;` seems to help reduce attempts
> by the kernel to "manage" the regulator, but does not appear
> to be required
>
> In other words:
>
> ...
> vreg_mmc: vreg_mmc {
> compatible = "regulator-fixed";
> regulator-name = "vreg_mmc";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> regulator-always-on;
> };
> ...
>
> Additionally, CONFIG_REGULATOR=y and CONFIG_REGULATOR_FIXED_VOLTAGE=y
> *MUST* be enabled in the kernel's .config file, to prevent either
> litex_mmc_probe() from being deferred, or mmc_regulator_get_supply()
> from simply returning 0 without having set mmc->ocr_avail to anything
> at all!
>
> Presumably this would also mean either `select REGULATOR_FIXED_VOLTAGE`
> or `depends on REGULATOR_FIXED_VOLTAGE` in the mmc driver's Kconfig
> entry.

Yep, that's correct.

If you don't like to manage that dependency in the Kconfig, an option
is to check if mmc->ocr_avail is zero and if so, we could log a
message *and* assign mmc->ocr_avail a default value.

>
> Predictably, the "regulator-[min|max]-microvolt = <3300000>" setting
> gets us
>
> ocr_avail == MMC_VDD_32_33 | MMC_VDD_33_34
>
> > >
> > > 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!
> >
> > No, this isn't the way we want it to work. That's because it means
> > that we would lie to the card about what voltage range the HW actually
> > supports.
> >
> > It's better to let the DTS file give that information about the HW.
>
> I may be needlessly concerned, but it feels a bit weird to me to drag
> in CONFIG_REGULATOR_FIXED_VOLTAGE as an added dependency for what is
> ultimately a roundabout way of setting a constant... :)

The point is, it shouldn't really be a constant set by the driver,
because it would mean initialising a card under potentially wrong
conditions.

However, I am fine assigning it a default value as a fallback and best
effort, if it turns out that DT didn't provide us information about
what the HW is capable of.

>
> Thanks in advance for any additional clue!

Looks like there are two options, just pick one of them, then I am happy. :-)

Kind regards
Uffe