On 5 June 2015 at 09:10, Lu Y.B. <yangbo.lu@xxxxxxxxxxxxx> wrote: > Thanks a lot, see my comments. > >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx] >> Sent: Thursday, June 04, 2015 7:36 PM >> To: Lu Yangbo-B47093 >> Cc: linux-mmc; Chris Ball >> Subject: Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for T4240 >> >> On 4 June 2015 at 11:56, Lu Y.B. <yangbo.lu@xxxxxxxxxxxxx> wrote: >> > Pls see my comments below. >> > >> >> -----Original Message----- >> >> From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx] >> >> Sent: Thursday, June 04, 2015 3:51 PM >> >> To: Lu Yangbo-B47093 >> >> Cc: linux-mmc; Chris Ball >> >> Subject: Re: [PATCH 2/2] mmc: esdhc: add quirk to support 3.3v for >> >> T4240 >> >> >> >> On 2 June 2015 at 09:09, Yangbo Lu <yangbo.lu@xxxxxxxxxxxxx> wrote: >> >> > Add SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33 for T4240 >> >> > >> >> > Signed-off-by: Yangbo Lu <yangbo.lu@xxxxxxxxxxxxx> >> >> > --- >> >> > drivers/mmc/host/sdhci-of-esdhc.c | 3 +++ >> >> > 1 file changed, 3 insertions(+) >> >> > >> >> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c >> >> b/drivers/mmc/host/sdhci-of-esdhc.c >> >> > index 22e9111..4f5fe42 100644 >> >> > --- a/drivers/mmc/host/sdhci-of-esdhc.c >> >> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c >> >> > @@ -369,6 +369,9 @@ static int sdhci_esdhc_probe(struct >> >> > platform_device >> >> *pdev) >> >> > host->quirks2 |= SDHCI_QUIRK2_BROKEN_HOST_CONTROL; >> >> > } >> >> > >> >> > + if (of_device_is_compatible(np, "fsl,t4240-esdhc")) >> >> > + host->quirks2 |= SDHCI_QUIRK2_CIRCUIT_SUPPORT_VS33; >> >> > + >> >> >> >> Instead of checking the compatible, could you check the "ocr_avail" >> >> mask which mmc_of_parse() create from the vmmc regulator? >> > >> > It could get ocr_mask value supporting 3.3v from dts, and get ocr_avail >> not supporting 3.3v from capability register. >> > But in sdhci.c, the ocr_avail will "&" the ocr_mask value, this makes >> 3.3v supporting bit cleaned. >> > >> > if (host->ocr_mask) >> > ocr_avail &= host->ocr_mask; >> >> I have to admit that this looks a bit odd... >> >> Anyway, as long as you don't specify the "host->ocr_mask" the above "if" >> will not change the ocr_avail mask. > > Actually the sdhci-of-esdhc driver would set host->ocr_mask when it probes. > mmc_of_parse_voltage(np, &host->ocr_mask); Okay, got it - thanks! So I tried to understand when the voltage-range binding should be used, but the DT documentation is quite poor for it. Anyway, I assumes the binding is there to describe internal HW characteristics of the what regulator levels the mmc controller can support. And the regulator levels are for the power to the card, *not* for the IO voltage. So, is this according to what you expects as well, especially since you were aiming to fix something for the IO voltage in patch1? > > But the "&" operation will make host->ocr_mask to have no use for other voltage supporting. > I think using "|" instead or just assigning host->ocr_mask to ocr_avail should be ok. > If so, there is no need to add this quirk. This is kind of a policy change for how to treat the configuration from DT. I have cc:ed Haijun Zhang, which invented the binding to see if we can get some feeback from him. The commit is: 6e9e318b304fd7373a0754805a76a02ddbc69a41 ("mmc: core: parse voltage from device-tree") > >> >> Looking a bit further up in the code in sdhci_add_host(), you will find >> that the ocr_avail mask is created by calling >> mmc_regulator_get_supply() (and not mmc_of_parse() as told you before). >> If there are external regulators, the ocr_avail mask will then override >> the values from SDHCI's caps register. > > But there is no external regulator on boards that using eSDHC... > Quoted from your response in patch1: "Although the controller only supports 1.8v, the hardware circuit has a level translator for supporting 3.3v". Again, that seems to concern the IO voltage, but if not - could it be modelled as regulator and thus used to fetch the ocr mask from? It's quite common that we use GPIO regulators in the mmc subsystem for this matter. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
