Date:   Wed, 12 Jan 2022 13:08:50 -0500
From:   "Gabriel L. Somlo" <gsomlo@gmail.com>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     linux-kernel@vger.kernel.org, robh+dt@kernel.org,
        devicetree@vger.kernel.org, linux-mmc@vger.kernel.org,
        kgugala@antmicro.com, mholenko@antmicro.com, krakoczy@antmicro.com,
        mdudek@internships.antmicro.com, paulus@ozlabs.org, joel@jms.id.au,
        shorne@gmail.com, geert@linux-m68k.org,
        david.abdurachmanov@sifive.com, florent@enjoy-digital.fr,
        rdunlap@infradead.org, andy.shevchenko@gmail.com, hdanton@sina.com
Subject: Re: [PATCH v5 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Message-ID: <Yd8ZMtLINkLVNARv@glsvmlin.ini.cmu.edu>
References: <20211215130711.111186-1-gsomlo@gmail.com>
 <20211215130711.111186-4-gsomlo@gmail.com>
 <CAPDyKFqo5sZy8aVbOcfS_cxT9T5r214GKCL-FKRg_0P0yQJTFQ@mail.gmail.com>
 <YdOUbYpGFNyxz3iD@errol.ini.cmu.edu>
 <CAPDyKFohOHYu_bdXsAYvDmMLqnGUW=9pG+yJDwP5-db1B6F1Dw@mail.gmail.com>
 <Yd4JdBArPn9rBj5b@errol.ini.cmu.edu>
 <CAPDyKFq18AWsaWHcEkU6H1Sh4NsqRfUeQhbRz9MorGfnKzxHwQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPDyKFq18AWsaWHcEkU6H1Sh4NsqRfUeQhbRz9MorGfnKzxHwQ@mail.gmail.com>
Precedence: bulk

On Wed, Jan 12, 2022 at 11:24:34AM +0100, Ulf Hansson wrote:
> [...]
> 
> > > > > [...]
> > > > >
> > > > > > +
> > > > > > +       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. :-)

Sounds like a plan! I'll send out lucky v13 once I've had a chance to
test it on my FPGA, later this evening.

Thanks again,
--Gabriel