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
