| Message ID | 20200710111140.29725-1-benchuanggli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support UHS-II for GL9755 | expand |
On 10/07/20 2:11 pm, Ben Chuang wrote: > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > VDD2 is used for powering UHS-II interface. > Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() > and sdhci_set_power_noreg() to handle VDD2. vdd2 is always 1.8 V and I suspect there may never be support for anything else, so we should start with 1.8 V only. Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use the existing ->set_power() callback > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/mmc/host/sdhci-omap.c | 2 +- > drivers/mmc/host/sdhci-pci-core.c | 4 +-- > drivers/mmc/host/sdhci-pxav3.c | 4 +-- > drivers/mmc/host/sdhci-xenon.c | 4 +-- > drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++------- > drivers/mmc/host/sdhci.h | 9 +++---- > 6 files changed, 43 insertions(+), 22 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > index 1ec74c2d5c17..1926585debe5 100644 > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -678,7 +678,7 @@ static void sdhci_omap_set_clock(struct sdhci_host *host, unsigned int clock) > } > > static void sdhci_omap_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > struct mmc_host *mmc = host->mmc; > > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c > index bb6802448b2f..40f5a24a8982 100644 > --- a/drivers/mmc/host/sdhci-pci-core.c > +++ b/drivers/mmc/host/sdhci-pci-core.c > @@ -629,12 +629,12 @@ static int bxt_get_cd(struct mmc_host *mmc) > #define SDHCI_INTEL_PWR_TIMEOUT_UDELAY 100 > > static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > int cntr; > u8 reg; > > - sdhci_set_power(host, mode, vdd); > + sdhci_set_power(host, mode, vdd, -1); > > if (mode == MMC_POWER_OFF) > return; > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index e55037ceda73..457e9425339a 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -298,12 +298,12 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > } > > static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > struct mmc_host *mmc = host->mmc; > u8 pwr = host->pwr; > > - sdhci_set_power_noreg(host, mode, vdd); > + sdhci_set_power_noreg(host, mode, vdd, -1); > > if (host->pwr == pwr) > return; > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > index 4703cd540c7f..2b0ebb91895a 100644 > --- a/drivers/mmc/host/sdhci-xenon.c > +++ b/drivers/mmc/host/sdhci-xenon.c > @@ -214,12 +214,12 @@ static void xenon_set_uhs_signaling(struct sdhci_host *host, > } > > static void xenon_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > struct mmc_host *mmc = host->mmc; > u8 pwr = host->pwr; > > - sdhci_set_power_noreg(host, mode, vdd); > + sdhci_set_power_noreg(host, mode, vdd, -1); > > if (host->pwr == pwr) > return; > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index d38d734ec83f..ca3d4a506e01 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2089,12 +2089,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > EXPORT_SYMBOL_GPL(sdhci_set_clock); > > static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > struct mmc_host *mmc = host->mmc; > > mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > + if (mmc->caps & MMC_CAP_UHS2 && !IS_ERR(mmc->supply.vmmc2)) > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc2, vdd2); > + > if (mode != MMC_POWER_OFF) > sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); > else > @@ -2102,7 +2105,7 @@ static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > } > > void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > u8 pwr = 0; > > @@ -2133,6 +2136,20 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > } > } > > + if (mode != MMC_POWER_OFF) { > + if (vdd2 != (unsigned short)-1) { > + switch (1 << vdd2) { > + case MMC_VDD2_165_195: > + pwr |= SDHCI_VDD2_POWER_180; > + break; > + default: > + WARN(1, "%s: Invalid vdd2 %#x\n", > + mmc_hostname(host->mmc), vdd2); > + break; > + } > + } > + } > + > if (host->pwr == pwr) > return; > > @@ -2159,7 +2176,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > pwr |= SDHCI_POWER_ON; > + if (vdd2 != (unsigned short)-1) > + pwr |= SDHCI_VDD2_POWER_ON; > + > + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL); > + mdelay(5); > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > + mdelay(5); > > if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) > sdhci_runtime_pm_bus_on(host); > @@ -2175,12 +2198,12 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > EXPORT_SYMBOL_GPL(sdhci_set_power_noreg); > > void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, unsigned short vdd2) > { > if (IS_ERR(host->mmc->supply.vmmc)) > - sdhci_set_power_noreg(host, mode, vdd); > + sdhci_set_power_noreg(host, mode, vdd, vdd2); > else > - sdhci_set_power_reg(host, mode, vdd); > + sdhci_set_power_reg(host, mode, vdd, vdd2); > } > EXPORT_SYMBOL_GPL(sdhci_set_power); > > @@ -2192,14 +2215,15 @@ EXPORT_SYMBOL_GPL(sdhci_set_power); > */ > void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, > unsigned char mode, > - unsigned short vdd) > + unsigned short vdd, > + unsigned short vdd2) > { > if (!IS_ERR(host->mmc->supply.vmmc)) { > struct mmc_host *mmc = host->mmc; > > mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > } > - sdhci_set_power_noreg(host, mode, vdd); > + sdhci_set_power_noreg(host, mode, vdd, -1); > } > EXPORT_SYMBOL_GPL(sdhci_set_power_and_bus_voltage); > > @@ -2377,9 +2401,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > } > > if (host->ops->set_power) > - host->ops->set_power(host, ios->power_mode, ios->vdd); > + host->ops->set_power(host, ios->power_mode, ios->vdd, -1); > else > - sdhci_set_power(host, ios->power_mode, ios->vdd); > + sdhci_set_power(host, ios->power_mode, ios->vdd, -1); > > /* 4.0 host support */ > if (host->version >= SDHCI_SPEC_400) { > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index ecf55394ea46..f6732f33f29f 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -689,7 +689,7 @@ struct sdhci_ops { > > void (*set_clock)(struct sdhci_host *host, unsigned int clock); > void (*set_power)(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd); > + unsigned short vdd, unsigned short vdd2); > > u32 (*irq)(struct sdhci_host *host, u32 intmask); > > @@ -852,13 +852,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, > unsigned int *actual_clock); > void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > void sdhci_enable_clk(struct sdhci_host *host, u16 clk); > -void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd); > void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, > unsigned char mode, > - unsigned short vdd); > -void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd); > + unsigned short vdd, > + unsigned short vdd2); > void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq); > int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq); > void sdhci_set_bus_width(struct sdhci_host *host, int width); >
Adrian, On Fri, Aug 21, 2020 at 05:11:18PM +0300, Adrian Hunter wrote: > On 10/07/20 2:11 pm, Ben Chuang wrote: > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > VDD2 is used for powering UHS-II interface. > > Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() > > and sdhci_set_power_noreg() to handle VDD2. > > vdd2 is always 1.8 V and I suspect there may never be support for anything > else, so we should start with 1.8 V only. What do you mean here? You don't want to add an extra argument, vdd2, to sdhci_set_power(). Correct? > Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use > the existing ->set_power() callback Again what do you expect here? Do you want to see any platform-specific mmc driver who supports UHS-II to implement its own call back like: void sdhci_foo_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd) { sdhci_set_power(host, mode,vdd); /* in case that sdhci_uhs2 module is not inserted */ if (!(mmc->caps & MMC_CAP_UHS2)) return; /* vdd2 specific operation */ if (IS_ERR_OR_NULL(host->mmc->supply.vmmc2)) sdhci_uhs2_set_power_noreg(host, mode); else sdhci_uhs2_set_power_reg(host, mode); /* maybe more platform-specific initialization */ } struct sdhci_ops sdhci_foo_ops = { .set_power = sdhci_foo_set_power, ... } Is this what you mean? (I'm not quite sure yet that sdhci_ush2_set_power_noreg() can be split off from sdhci_set_power_noreg().) -Takahiro Akashi } > > > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/mmc/host/sdhci-omap.c | 2 +- > > drivers/mmc/host/sdhci-pci-core.c | 4 +-- > > drivers/mmc/host/sdhci-pxav3.c | 4 +-- > > drivers/mmc/host/sdhci-xenon.c | 4 +-- > > drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++------- > > drivers/mmc/host/sdhci.h | 9 +++---- > > 6 files changed, 43 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > > index 1ec74c2d5c17..1926585debe5 100644 > > --- a/drivers/mmc/host/sdhci-omap.c > > +++ b/drivers/mmc/host/sdhci-omap.c > > @@ -678,7 +678,7 @@ static void sdhci_omap_set_clock(struct sdhci_host *host, unsigned int clock) > > } > > > > static void sdhci_omap_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > struct mmc_host *mmc = host->mmc; > > > > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c > > index bb6802448b2f..40f5a24a8982 100644 > > --- a/drivers/mmc/host/sdhci-pci-core.c > > +++ b/drivers/mmc/host/sdhci-pci-core.c > > @@ -629,12 +629,12 @@ static int bxt_get_cd(struct mmc_host *mmc) > > #define SDHCI_INTEL_PWR_TIMEOUT_UDELAY 100 > > > > static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > int cntr; > > u8 reg; > > > > - sdhci_set_power(host, mode, vdd); > > + sdhci_set_power(host, mode, vdd, -1); > > > > if (mode == MMC_POWER_OFF) > > return; > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > > index e55037ceda73..457e9425339a 100644 > > --- a/drivers/mmc/host/sdhci-pxav3.c > > +++ b/drivers/mmc/host/sdhci-pxav3.c > > @@ -298,12 +298,12 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > > } > > > > static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > struct mmc_host *mmc = host->mmc; > > u8 pwr = host->pwr; > > > > - sdhci_set_power_noreg(host, mode, vdd); > > + sdhci_set_power_noreg(host, mode, vdd, -1); > > > > if (host->pwr == pwr) > > return; > > diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c > > index 4703cd540c7f..2b0ebb91895a 100644 > > --- a/drivers/mmc/host/sdhci-xenon.c > > +++ b/drivers/mmc/host/sdhci-xenon.c > > @@ -214,12 +214,12 @@ static void xenon_set_uhs_signaling(struct sdhci_host *host, > > } > > > > static void xenon_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > struct mmc_host *mmc = host->mmc; > > u8 pwr = host->pwr; > > > > - sdhci_set_power_noreg(host, mode, vdd); > > + sdhci_set_power_noreg(host, mode, vdd, -1); > > > > if (host->pwr == pwr) > > return; > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index d38d734ec83f..ca3d4a506e01 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2089,12 +2089,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) > > EXPORT_SYMBOL_GPL(sdhci_set_clock); > > > > static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > struct mmc_host *mmc = host->mmc; > > > > mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > > > + if (mmc->caps & MMC_CAP_UHS2 && !IS_ERR(mmc->supply.vmmc2)) > > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc2, vdd2); > > + > > if (mode != MMC_POWER_OFF) > > sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); > > else > > @@ -2102,7 +2105,7 @@ static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > > } > > > > void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > u8 pwr = 0; > > > > @@ -2133,6 +2136,20 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > } > > } > > > > + if (mode != MMC_POWER_OFF) { > > + if (vdd2 != (unsigned short)-1) { > > + switch (1 << vdd2) { > > + case MMC_VDD2_165_195: > > + pwr |= SDHCI_VDD2_POWER_180; > > + break; > > + default: > > + WARN(1, "%s: Invalid vdd2 %#x\n", > > + mmc_hostname(host->mmc), vdd2); > > + break; > > + } > > + } > > + } > > + > > if (host->pwr == pwr) > > return; > > > > @@ -2159,7 +2176,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > > > pwr |= SDHCI_POWER_ON; > > + if (vdd2 != (unsigned short)-1) > > + pwr |= SDHCI_VDD2_POWER_ON; > > + > > + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL); > > + mdelay(5); > > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); > > + mdelay(5); > > > > if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) > > sdhci_runtime_pm_bus_on(host); > > @@ -2175,12 +2198,12 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > EXPORT_SYMBOL_GPL(sdhci_set_power_noreg); > > > > void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, unsigned short vdd2) > > { > > if (IS_ERR(host->mmc->supply.vmmc)) > > - sdhci_set_power_noreg(host, mode, vdd); > > + sdhci_set_power_noreg(host, mode, vdd, vdd2); > > else > > - sdhci_set_power_reg(host, mode, vdd); > > + sdhci_set_power_reg(host, mode, vdd, vdd2); > > } > > EXPORT_SYMBOL_GPL(sdhci_set_power); > > > > @@ -2192,14 +2215,15 @@ EXPORT_SYMBOL_GPL(sdhci_set_power); > > */ > > void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, > > unsigned char mode, > > - unsigned short vdd) > > + unsigned short vdd, > > + unsigned short vdd2) > > { > > if (!IS_ERR(host->mmc->supply.vmmc)) { > > struct mmc_host *mmc = host->mmc; > > > > mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > } > > - sdhci_set_power_noreg(host, mode, vdd); > > + sdhci_set_power_noreg(host, mode, vdd, -1); > > } > > EXPORT_SYMBOL_GPL(sdhci_set_power_and_bus_voltage); > > > > @@ -2377,9 +2401,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > } > > > > if (host->ops->set_power) > > - host->ops->set_power(host, ios->power_mode, ios->vdd); > > + host->ops->set_power(host, ios->power_mode, ios->vdd, -1); > > else > > - sdhci_set_power(host, ios->power_mode, ios->vdd); > > + sdhci_set_power(host, ios->power_mode, ios->vdd, -1); > > > > /* 4.0 host support */ > > if (host->version >= SDHCI_SPEC_400) { > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > > index ecf55394ea46..f6732f33f29f 100644 > > --- a/drivers/mmc/host/sdhci.h > > +++ b/drivers/mmc/host/sdhci.h > > @@ -689,7 +689,7 @@ struct sdhci_ops { > > > > void (*set_clock)(struct sdhci_host *host, unsigned int clock); > > void (*set_power)(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd); > > + unsigned short vdd, unsigned short vdd2); > > > > u32 (*irq)(struct sdhci_host *host, u32 intmask); > > > > @@ -852,13 +852,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, > > unsigned int *actual_clock); > > void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > > void sdhci_enable_clk(struct sdhci_host *host, u16 clk); > > -void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd); > > void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, > > unsigned char mode, > > - unsigned short vdd); > > -void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd); > > + unsigned short vdd, > > + unsigned short vdd2); > > void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq); > > int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq); > > void sdhci_set_bus_width(struct sdhci_host *host, int width); > > >
On 14/09/20 8:45 am, AKASHI Takahiro wrote: > Adrian, > > On Fri, Aug 21, 2020 at 05:11:18PM +0300, Adrian Hunter wrote: >> On 10/07/20 2:11 pm, Ben Chuang wrote: >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> >>> VDD2 is used for powering UHS-II interface. >>> Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() >>> and sdhci_set_power_noreg() to handle VDD2. >> >> vdd2 is always 1.8 V and I suspect there may never be support for anything >> else, so we should start with 1.8 V only. > > What do you mean here? > You don't want to add an extra argument, vdd2, to sdhci_set_power(). > Correct? Yes > >> Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use >> the existing ->set_power() callback > > Again what do you expect here? > > Do you want to see any platform-specific mmc driver who supports UHS-II > to implement its own call back like: Not exactly. I expect there to be a common implementation in sdhci-uhs2.c called sdhci_uhs2_set_power() for example, that drivers can use by setting their .set_power = sdhci_uhs2_set_power. If they need platform-specific code as well then their platform-specific code can call sdhci_uhs2_set_power() if desired. > > void sdhci_foo_set_power(struct sdhci_host *host, unsigned char mode, > unsigned short vdd) > { > sdhci_set_power(host, mode,vdd); > > /* in case that sdhci_uhs2 module is not inserted */ > if (!(mmc->caps & MMC_CAP_UHS2)) > return; > > /* vdd2 specific operation */ > if (IS_ERR_OR_NULL(host->mmc->supply.vmmc2)) > sdhci_uhs2_set_power_noreg(host, mode); > else > sdhci_uhs2_set_power_reg(host, mode); > > /* maybe more platform-specific initialization */ > } > > struct sdhci_ops sdhci_foo_ops = { > .set_power = sdhci_foo_set_power, > ... > } > > Is this what you mean? > (I'm not quite sure yet that sdhci_ush2_set_power_noreg() can be split off > from sdhci_set_power_noreg().) > > -Takahiro Akashi
Adrain, On Mon, Sep 14, 2020 at 09:36:02AM +0300, Adrian Hunter wrote: > On 14/09/20 8:45 am, AKASHI Takahiro wrote: > > Adrian, > > > > On Fri, Aug 21, 2020 at 05:11:18PM +0300, Adrian Hunter wrote: > >> On 10/07/20 2:11 pm, Ben Chuang wrote: > >>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> > >>> VDD2 is used for powering UHS-II interface. > >>> Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() > >>> and sdhci_set_power_noreg() to handle VDD2. > >> > >> vdd2 is always 1.8 V and I suspect there may never be support for anything > >> else, so we should start with 1.8 V only. > > > > What do you mean here? > > You don't want to add an extra argument, vdd2, to sdhci_set_power(). > > Correct? > > Yes > > > > >> Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use > >> the existing ->set_power() callback > > > > Again what do you expect here? > > > > Do you want to see any platform-specific mmc driver who supports UHS-II > > to implement its own call back like: > > Not exactly. I expect there to be a common implementation in sdhci-uhs2.c > called sdhci_uhs2_set_power() for example, that drivers can use by setting > their .set_power = sdhci_uhs2_set_power. If they need platform-specific > code as well then their platform-specific code can call > sdhci_uhs2_set_power() if desired. > > > > > void sdhci_foo_set_power(struct sdhci_host *host, unsigned char mode, > > unsigned short vdd) > > { > > sdhci_set_power(host, mode,vdd); > > > > /* in case that sdhci_uhs2 module is not inserted */ > > if (!(mmc->caps & MMC_CAP_UHS2)) > > return; > > > > /* vdd2 specific operation */ > > if (IS_ERR_OR_NULL(host->mmc->supply.vmmc2)) > > sdhci_uhs2_set_power_noreg(host, mode); > > else > > sdhci_uhs2_set_power_reg(host, mode); > > > > /* maybe more platform-specific initialization */ > > } > > > > struct sdhci_ops sdhci_foo_ops = { > > .set_power = sdhci_foo_set_power, > > ... > > } What do you think about this logic in general? (If necessary, read it replacing "foo" to "uhs2".) What I'm concerned about is SDHCI_POWER_CONTROL register. Vdd and vdd2 are controlled with corresponding bits in this register. It seems to be "natural" to me that vdd and vdd2 are enabled in a single function rather than putting them in separate ones. In particular, in the case of sdhci_set_power_noreg(), there exist a couple of "quirks" around writing the bits to SDHCI_POWER_CONTROL register. I don't know how we should handle them if we have a separate function, say, sdhci_uhs2_set_power_noreg(). Do you want to see a copy of the same logic in sdhci_uhs2_set_power_noreg()? -Takahiro Akashi > > > > Is this what you mean? > > (I'm not quite sure yet that sdhci_ush2_set_power_noreg() can be split off > > from sdhci_set_power_noreg().) > > > > -Takahiro Akashi
On 15/09/20 9:24 am, AKASHI Takahiro wrote: > Adrain, > > On Mon, Sep 14, 2020 at 09:36:02AM +0300, Adrian Hunter wrote: >> On 14/09/20 8:45 am, AKASHI Takahiro wrote: >>> Adrian, >>> >>> On Fri, Aug 21, 2020 at 05:11:18PM +0300, Adrian Hunter wrote: >>>> On 10/07/20 2:11 pm, Ben Chuang wrote: >>>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>>> >>>>> VDD2 is used for powering UHS-II interface. >>>>> Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() >>>>> and sdhci_set_power_noreg() to handle VDD2. >>>> >>>> vdd2 is always 1.8 V and I suspect there may never be support for anything >>>> else, so we should start with 1.8 V only. >>> >>> What do you mean here? >>> You don't want to add an extra argument, vdd2, to sdhci_set_power(). >>> Correct? >> >> Yes >> >>> >>>> Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use >>>> the existing ->set_power() callback >>> >>> Again what do you expect here? >>> >>> Do you want to see any platform-specific mmc driver who supports UHS-II >>> to implement its own call back like: >> >> Not exactly. I expect there to be a common implementation in sdhci-uhs2.c >> called sdhci_uhs2_set_power() for example, that drivers can use by setting >> their .set_power = sdhci_uhs2_set_power. If they need platform-specific >> code as well then their platform-specific code can call >> sdhci_uhs2_set_power() if desired. >> >>> >>> void sdhci_foo_set_power(struct sdhci_host *host, unsigned char mode, >>> unsigned short vdd) >>> { >>> sdhci_set_power(host, mode,vdd); >>> >>> /* in case that sdhci_uhs2 module is not inserted */ >>> if (!(mmc->caps & MMC_CAP_UHS2)) >>> return; >>> >>> /* vdd2 specific operation */ >>> if (IS_ERR_OR_NULL(host->mmc->supply.vmmc2)) >>> sdhci_uhs2_set_power_noreg(host, mode); >>> else >>> sdhci_uhs2_set_power_reg(host, mode); >>> >>> /* maybe more platform-specific initialization */ >>> } >>> >>> struct sdhci_ops sdhci_foo_ops = { >>> .set_power = sdhci_foo_set_power, >>> ... >>> } > > What do you think about this logic in general? > (If necessary, read it replacing "foo" to "uhs2".) > > What I'm concerned about is SDHCI_POWER_CONTROL register. > Vdd and vdd2 are controlled with corresponding bits in this register. > It seems to be "natural" to me that vdd and vdd2 are enabled > in a single function rather than putting them in separate ones. > > In particular, in the case of sdhci_set_power_noreg(), there exist a couple > of "quirks" around writing the bits to SDHCI_POWER_CONTROL register. We can treat UHS-II support as being for new hardware and therefore we don't necessarily need to support old quirks. Just make sure if a quirk is not being supported, to add a comment to that effect. > I don't know how we should handle them if we have a separate function, > say, sdhci_uhs2_set_power_noreg(). > Do you want to see a copy of the same logic in sdhci_uhs2_set_power_noreg()? I would probably consider making another function that non-UHS-II drivers do not need to care about e.g. existing drivers can keep using sdhci_set_power_noreg() and sdhci_uhs2 can call __sdhci_set_power_noreg() diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 592a55a34b58..ffe54f06fe38 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2013,8 +2013,8 @@ static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); } -void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) +void __sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, + unsigned short vdd, u8 vdd2) { u8 pwr = 0; @@ -2048,7 +2048,7 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, if (host->pwr == pwr) return; - host->pwr = pwr; + host->pwr = pwr | vdd2; if (pwr == 0) { sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); @@ -2085,6 +2085,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, mdelay(10); } } +EXPORT_SYMBOL_GPL(__sdhci_set_power_noreg); + +void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, + unsigned short vdd) +{ + __sdhci_set_power_noreg(host, mode, vdd, 0); +} EXPORT_SYMBOL_GPL(sdhci_set_power_noreg); void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > -Takahiro Akashi > > >>> >>> Is this what you mean? >>> (I'm not quite sure yet that sdhci_ush2_set_power_noreg() can be split off >>> from sdhci_set_power_noreg().) >>> >>> -Takahiro Akashi
Adrian, On Wed, Sep 16, 2020 at 09:42:28AM +0300, Adrian Hunter wrote: > On 15/09/20 9:24 am, AKASHI Takahiro wrote: > > Adrain, > > > > On Mon, Sep 14, 2020 at 09:36:02AM +0300, Adrian Hunter wrote: > >> On 14/09/20 8:45 am, AKASHI Takahiro wrote: > >>> Adrian, > >>> > >>> On Fri, Aug 21, 2020 at 05:11:18PM +0300, Adrian Hunter wrote: > >>>> On 10/07/20 2:11 pm, Ben Chuang wrote: > >>>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>>> > >>>>> VDD2 is used for powering UHS-II interface. > >>>>> Modify sdhci_set_power_and_bus_voltage(), sdhci_set_power_noreg() > >>>>> and sdhci_set_power_noreg() to handle VDD2. > >>>> > >>>> vdd2 is always 1.8 V and I suspect there may never be support for anything > >>>> else, so we should start with 1.8 V only. > >>> > >>> What do you mean here? > >>> You don't want to add an extra argument, vdd2, to sdhci_set_power(). > >>> Correct? > >> > >> Yes > >> > >>> > >>>> Also can we create uhs2_set_power_reg() and uhs2_set_power_noreg() and use > >>>> the existing ->set_power() callback > >>> > >>> Again what do you expect here? > >>> > >>> Do you want to see any platform-specific mmc driver who supports UHS-II > >>> to implement its own call back like: > >> > >> Not exactly. I expect there to be a common implementation in sdhci-uhs2.c > >> called sdhci_uhs2_set_power() for example, that drivers can use by setting > >> their .set_power = sdhci_uhs2_set_power. If they need platform-specific > >> code as well then their platform-specific code can call > >> sdhci_uhs2_set_power() if desired. > >> > >>> > >>> void sdhci_foo_set_power(struct sdhci_host *host, unsigned char mode, > >>> unsigned short vdd) > >>> { > >>> sdhci_set_power(host, mode,vdd); > >>> > >>> /* in case that sdhci_uhs2 module is not inserted */ > >>> if (!(mmc->caps & MMC_CAP_UHS2)) > >>> return; > >>> > >>> /* vdd2 specific operation */ > >>> if (IS_ERR_OR_NULL(host->mmc->supply.vmmc2)) > >>> sdhci_uhs2_set_power_noreg(host, mode); > >>> else > >>> sdhci_uhs2_set_power_reg(host, mode); > >>> > >>> /* maybe more platform-specific initialization */ > >>> } > >>> > >>> struct sdhci_ops sdhci_foo_ops = { > >>> .set_power = sdhci_foo_set_power, > >>> ... > >>> } > > > > What do you think about this logic in general? > > (If necessary, read it replacing "foo" to "uhs2".) > > > > What I'm concerned about is SDHCI_POWER_CONTROL register. > > Vdd and vdd2 are controlled with corresponding bits in this register. > > It seems to be "natural" to me that vdd and vdd2 are enabled > > in a single function rather than putting them in separate ones. > > > > In particular, in the case of sdhci_set_power_noreg(), there exist a couple > > of "quirks" around writing the bits to SDHCI_POWER_CONTROL register. > > We can treat UHS-II support as being for new hardware and therefore > we don't necessarily need to support old quirks. Just make sure if > a quirk is not being supported, to add a comment to that effect. > > > I don't know how we should handle them if we have a separate function, > > say, sdhci_uhs2_set_power_noreg(). > > Do you want to see a copy of the same logic in sdhci_uhs2_set_power_noreg()? > > I would probably consider making another function that non-UHS-II > drivers do not need to care about e.g. existing drivers can keep using > sdhci_set_power_noreg() and sdhci_uhs2 can call __sdhci_set_power_noreg() Well, but > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 592a55a34b58..ffe54f06fe38 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2013,8 +2013,8 @@ static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); > } > > -void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > +void __sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd, u8 vdd2) > { > u8 pwr = 0; > > @@ -2048,7 +2048,7 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > if (host->pwr == pwr) > return; > > - host->pwr = pwr; > + host->pwr = pwr | vdd2; (the line above is wrong, but anyway) we must also set if (vdd2) pwr |= SDHCI_VDD2_POWER_ON; As a result, this new function is the almost exact same as the corresponding one in our v3 patch, except its name. Now do you allow such a small piece of UHS-II specific code to be placed in sdhci.c? -Takahiro Akashi > if (pwr == 0) { > sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); > @@ -2085,6 +2085,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > mdelay(10); > } > } > +EXPORT_SYMBOL_GPL(__sdhci_set_power_noreg); > + > +void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd) > +{ > + __sdhci_set_power_noreg(host, mode, vdd, 0); > +} > EXPORT_SYMBOL_GPL(sdhci_set_power_noreg); > > void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > > > > -Takahiro Akashi > > > > > >>> > >>> Is this what you mean? > >>> (I'm not quite sure yet that sdhci_ush2_set_power_noreg() can be split off > >>> from sdhci_set_power_noreg().) > >>> > >>> -Takahiro Akashi >
diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c index 1ec74c2d5c17..1926585debe5 100644 --- a/drivers/mmc/host/sdhci-omap.c +++ b/drivers/mmc/host/sdhci-omap.c @@ -678,7 +678,7 @@ static void sdhci_omap_set_clock(struct sdhci_host *host, unsigned int clock) } static void sdhci_omap_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { struct mmc_host *mmc = host->mmc; diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c index bb6802448b2f..40f5a24a8982 100644 --- a/drivers/mmc/host/sdhci-pci-core.c +++ b/drivers/mmc/host/sdhci-pci-core.c @@ -629,12 +629,12 @@ static int bxt_get_cd(struct mmc_host *mmc) #define SDHCI_INTEL_PWR_TIMEOUT_UDELAY 100 static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { int cntr; u8 reg; - sdhci_set_power(host, mode, vdd); + sdhci_set_power(host, mode, vdd, -1); if (mode == MMC_POWER_OFF) return; diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index e55037ceda73..457e9425339a 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -298,12 +298,12 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) } static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { struct mmc_host *mmc = host->mmc; u8 pwr = host->pwr; - sdhci_set_power_noreg(host, mode, vdd); + sdhci_set_power_noreg(host, mode, vdd, -1); if (host->pwr == pwr) return; diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c index 4703cd540c7f..2b0ebb91895a 100644 --- a/drivers/mmc/host/sdhci-xenon.c +++ b/drivers/mmc/host/sdhci-xenon.c @@ -214,12 +214,12 @@ static void xenon_set_uhs_signaling(struct sdhci_host *host, } static void xenon_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { struct mmc_host *mmc = host->mmc; u8 pwr = host->pwr; - sdhci_set_power_noreg(host, mode, vdd); + sdhci_set_power_noreg(host, mode, vdd, -1); if (host->pwr == pwr) return; diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index d38d734ec83f..ca3d4a506e01 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2089,12 +2089,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock) EXPORT_SYMBOL_GPL(sdhci_set_clock); static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { struct mmc_host *mmc = host->mmc; mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); + if (mmc->caps & MMC_CAP_UHS2 && !IS_ERR(mmc->supply.vmmc2)) + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc2, vdd2); + if (mode != MMC_POWER_OFF) sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); else @@ -2102,7 +2105,7 @@ static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, } void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { u8 pwr = 0; @@ -2133,6 +2136,20 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, } } + if (mode != MMC_POWER_OFF) { + if (vdd2 != (unsigned short)-1) { + switch (1 << vdd2) { + case MMC_VDD2_165_195: + pwr |= SDHCI_VDD2_POWER_180; + break; + default: + WARN(1, "%s: Invalid vdd2 %#x\n", + mmc_hostname(host->mmc), vdd2); + break; + } + } + } + if (host->pwr == pwr) return; @@ -2159,7 +2176,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); pwr |= SDHCI_POWER_ON; + if (vdd2 != (unsigned short)-1) + pwr |= SDHCI_VDD2_POWER_ON; + + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL); + mdelay(5); sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL); + mdelay(5); if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) sdhci_runtime_pm_bus_on(host); @@ -2175,12 +2198,12 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, EXPORT_SYMBOL_GPL(sdhci_set_power_noreg); void sdhci_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, unsigned short vdd2) { if (IS_ERR(host->mmc->supply.vmmc)) - sdhci_set_power_noreg(host, mode, vdd); + sdhci_set_power_noreg(host, mode, vdd, vdd2); else - sdhci_set_power_reg(host, mode, vdd); + sdhci_set_power_reg(host, mode, vdd, vdd2); } EXPORT_SYMBOL_GPL(sdhci_set_power); @@ -2192,14 +2215,15 @@ EXPORT_SYMBOL_GPL(sdhci_set_power); */ void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) + unsigned short vdd, + unsigned short vdd2) { if (!IS_ERR(host->mmc->supply.vmmc)) { struct mmc_host *mmc = host->mmc; mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); } - sdhci_set_power_noreg(host, mode, vdd); + sdhci_set_power_noreg(host, mode, vdd, -1); } EXPORT_SYMBOL_GPL(sdhci_set_power_and_bus_voltage); @@ -2377,9 +2401,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) } if (host->ops->set_power) - host->ops->set_power(host, ios->power_mode, ios->vdd); + host->ops->set_power(host, ios->power_mode, ios->vdd, -1); else - sdhci_set_power(host, ios->power_mode, ios->vdd); + sdhci_set_power(host, ios->power_mode, ios->vdd, -1); /* 4.0 host support */ if (host->version >= SDHCI_SPEC_400) { diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index ecf55394ea46..f6732f33f29f 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -689,7 +689,7 @@ struct sdhci_ops { void (*set_clock)(struct sdhci_host *host, unsigned int clock); void (*set_power)(struct sdhci_host *host, unsigned char mode, - unsigned short vdd); + unsigned short vdd, unsigned short vdd2); u32 (*irq)(struct sdhci_host *host, u32 intmask); @@ -852,13 +852,10 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock, unsigned int *actual_clock); void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); void sdhci_enable_clk(struct sdhci_host *host, u16 clk); -void sdhci_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd); void sdhci_set_power_and_bus_voltage(struct sdhci_host *host, unsigned char mode, - unsigned short vdd); -void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, - unsigned short vdd); + unsigned short vdd, + unsigned short vdd2); void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq); int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq); void sdhci_set_bus_width(struct sdhci_host *host, int width);