Hi Jaehoon, On 21/01/21 10:40 am, Jaehoon Chung wrote: > Hi Aswath, > > On 1/21/21 1:13 PM, Aswath Govindraju wrote: >> Hi Jaehoon, >> >> On 21/01/21 4:26 am, Jaehoon Chung wrote: >>> Hi Aswath, >>> >>> On 1/19/21 9:35 PM, Aswath Govindraju wrote: >>>> Hi Jaehoon, >>>> >>>> On 05/11/20 4:03 am, Jaehoon Chung wrote: >>>>> On 11/5/20 4:05 AM, Faiz Abbas wrote: >>>>>> Jaehoon, >>>>>> >>>>>> On 21/10/20 5:08 pm, Jaehoon Chung wrote: >>>>>>> Hi Faiz, >>>>>>> >>>>>>> On 10/16/20 8:08 PM, Faiz Abbas wrote: >>>>>>>> Add a set_voltage() function which handles the switch from 3.3V to 1.8V >>>>>>>> for SD card UHS modes. >>>>>>>> >>>>>>>> Signed-off-by: Faiz Abbas <faiz_ab...@ti.com> >>>>>>>> --- >>>>>>>> drivers/mmc/sdhci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> include/sdhci.h | 1 + >>>>>>>> 2 files changed, 52 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c >>>>>>>> index 7673219fb3..a69f058191 100644 >>>>>>>> --- a/drivers/mmc/sdhci.c >>>>>>>> +++ b/drivers/mmc/sdhci.c >>>>>>>> @@ -20,6 +20,7 @@ >>>>>>>> #include <linux/delay.h> >>>>>>>> #include <linux/dma-mapping.h> >>>>>>>> #include <phys2bus.h> >>>>>>>> +#include <power/regulator.h> >>>>>>>> >>>>>>>> static void sdhci_reset(struct sdhci_host *host, u8 mask) >>>>>>>> { >>>>>>>> @@ -556,6 +557,56 @@ void sdhci_set_uhs_timing(struct sdhci_host *host) >>>>>>>> sdhci_writew(host, reg, SDHCI_HOST_CONTROL2); >>>>>>>> } >>>>>>>> >>>>>>>> +#if CONFIG_IS_ENABLED(MMC_IO_VOLTAGE) >>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) >>>>>>>> +{ >>>>>>>> + struct mmc *mmc = (struct mmc *)host->mmc; >>>>>>>> + u32 ctrl; >>>>>>>> + >>>>>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2); >>>>>>>> + >>>>>>>> + switch (mmc->signal_voltage) { >>>>>>>> + case MMC_SIGNAL_VOLTAGE_330: >>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>>>>>> + if (mmc->vqmmc_supply) { >>>>>>>> + regulator_set_enable(mmc->vqmmc_supply, false); >>>>>>>> + regulator_set_value(mmc->vqmmc_supply, 3300000); >>>>>>> >>>>>>> Doesn't need to consider about fail to set its value? >>>>>> >>>>>> You're right. I'll handle the failure case in v3. >>>>>> >>>>>>> >>>>>>>> + regulator_set_enable(mmc->vqmmc_supply, true); >>>>>>>> + } >>>>>>>> +#endif >>>>>>>> + mdelay(5); >>>>>>> >>>>>>> For what purpose about mdelay(5)? >>>>>> >>>>>> I'm following this from the kernel implementation here: >>>>>> https://protect2.fireeye.com/v1/url?k=8b617b16-d4fa420c-8b60f059-0cc47a6cba04-71d56f4128587abb&q=1&e=02f975f3-5191-4501-a554-a58145bd6ac1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fhost%2Fsdhci.c%23L2547 >>>>>> >>>>>> Not sure if this a part of the spec or not though. >>>>> >>>>> Thanks for sharing info. :) >>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> + if (IS_SD(mmc)) { >>>>>>>> + ctrl &= ~SDHCI_CTRL_VDD_180; >>>>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >>>>>>>> + } >>>>>>>> + break; >>>>>>>> + case MMC_SIGNAL_VOLTAGE_180: >>>>>>>> +#if CONFIG_IS_ENABLED(DM_REGULATOR) >>>>>>>> + if (mmc->vqmmc_supply) { >>>>>>>> + regulator_set_enable(mmc->vqmmc_supply, false); >>>>>>>> + regulator_set_value(mmc->vqmmc_supply, 1800000); >>>>>>>> + regulator_set_enable(mmc->vqmmc_supply, true); >>>>>>>> + } >>>>>>>> +#endif >>>>>>>> + if (IS_SD(mmc)) { >>>>>>>> + ctrl |= SDHCI_CTRL_VDD_180; >>>>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >>>>>>>> + } >>>>>>>> + break; >>>>>>>> + default: >>>>>>>> + /* No signal voltage switch required */ >>>>>>>> + return; >>>>>>>> + } >>>>>>>> +} >>>>>>>> +#else >>>>>>>> +static void sdhci_set_voltage(struct sdhci_host *host) { } >>>>>>>> +#endif >>>>>>>> +void sdhci_set_control_reg(struct sdhci_host *host) >>>>>>> >>>>>>> this function is called as callback function in sdhci_set_ios(). >>>>>>> it's strange... set_control_reg callback is for host specific control >>>>>>> register. >>>>>>> >>>>>>> I think that it doesn't need to assign to callback. >>>>>> >>>>>> This is the default set_control_reg() implementation which is defined >>>>>> in the sdhci spec. Any host that that wants default implementation >>>>>> case assign this as their callback. >>>>>> >>>>>> Hosts that have custom implementations can add in their own drivers. >>>>> >>>>> Yes..but when i have checked your code. It doesn't need to assign to >>>>> callback for your driver. >>>>> If sdhci_set_control_reg() is common function about all sdhci-xx driver, >>>>> then it can be just added in sdhci_set_ios(). >>>>>> callback is for sdhci-xx specific progress. >>>>> >>>>> In my opinion, >>>>> >>>>> static int sdhci_set_ios() >>>>> { >>>>> ... >>>>> sdhci_set_control_reg(); >>>>> if (host->ops && host->ops->set_control_reg) >>>>> host->ops->set_control_reg(); >>>>> >>>>> } >>>>> >>>>> if sdhci_set_control_reg() is not generic sequence, it has to be located >>>>> into am654_sdhci.c >>>>> It seems to add sdhci_set_ctonrol_reg() in sdhci.c for your am654_sdhci >>>>> driver. >>>>> >>>> >>>> >>>> As Faiz has mentioned sdhci_set_control_reg() added here is the default >>>> implementation according to the spec. One other driver apart from >>>> drivers/mmc/am654_sdhci.c that implements set_control_reg() similar to >>>> the above implementation is drivers/mmc/zynq_sdhci.c and there are some >>>> which implement it differently for example drivers/mmc/s5p_sdhci.c and >>>> drivers/mmc/sdhci-cadence.c. >>>> >>>> So, if a host driver wants use the default implementation then it can >>>> use the one implemented in the sdhci.c or it can implement its own >>>> implementation the way in which all the drivers have done so far. >>> >>> I understood what you said. :) >>> My mean is sdhci_set_control_reg() should be controlled a generic sdhci >>> spec. >>> It doesn't need to assign to callback function. >>> >>> The default implementation doesn't need to use callback function. >>> It will be used just in sdhci.c by default. callback function will be >>> additionally used for board specific. >>> (s5p_sdhci.c has some specific bits at control register. So i had added the >>> callback ops to control them in board specific driver.) >>> >> >> [1] >> >>> When i have checked its patchset again, am654_sdhci can be used the >>> sdhci_set_control_reg() without callback assigned. >>> >>> So it's same sequence the below.. >>> >>> sdhci_set_ios -> sdhci_set_contro_reg() in sdhci.c >>> sdhci_set_ios -> ops->set_control_reg() -> called set_control_reg() that is >>> assigned to callback. >>> >>> doesn't it? >>> >> >> Yes what you mentioned is correct assigning a callback is redundant in >> the case of am654_sdhci. The reason behind doing this is so that this >> patch would not break support for already existing drivers. If the >> implementation that you suggested below is followed then it would >> require follow up patches for all the existing drivers and some drivers >> might not be able to follow this implementation (reason is explained below). >> >> This way assigning a callback can also be seen in linux kernel mmc host >> drivers, >> >> For example, >> >> drivers/mmc/host/sdhci-pxav2.c >> drivers/mmc/host/sdhci-xenon.c >> drivers/mmc/host/sdhci-omap.c >> >> use function sdhci_pltfm_clk_get_max_clock() as a callback in their >> custom drivers which is a part of generic driver >> drivers/mmc/host/sdhci-pltfm.c. > > But i think that sdhci_pltfm_clk_get_max_clock() is not good example. > sdhci-pltfm is provided the generic code of platform side. > >> >> >>> Maybe, it can be changed as likes belows >>> >>> index baa935e0d5b0..b0e3ecc6314d 100644 >>> --- a/drivers/mmc/am654_sdhci.c >>> +++ b/drivers/mmc/am654_sdhci.c >>> @@ -295,7 +295,6 @@ static int am654_sdhci_deferred_probe(struct sdhci_host >>> *host) >>> const struct sdhci_ops am654_sdhci_ops = { >>> .deferred_probe = am654_sdhci_deferred_probe, >>> .set_ios_post = &am654_sdhci_set_ios_post, >>> - .set_control_reg = &am654_sdhci_set_control_reg, >>> }; >>> >>> const struct am654_driver_data am654_drv_data = { >>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c >>> index 06289343124e..9969a710755e 100644 >>> --- a/drivers/mmc/sdhci.c >>> +++ b/drivers/mmc/sdhci.c >>> @@ -509,6 +509,12 @@ void sdhci_set_uhs_timing(struct sdhci_host *host) >>> sdhci_writew(host, reg, SDHCI_HOST_CONTROL2); >>> } >>> >>> +void sdhci_set_control_reg(struct sdhci_host *host) >>> +{ >>> + sdhci_set_voltage(host); >>> + sdhci_set_uhs_timing(host); >>> +} >>> + >>> #ifdef CONFIG_DM_MMC >>> static int sdhci_set_ios(struct udevice *dev) >>> { >>> @@ -521,6 +527,9 @@ static int sdhci_set_ios(struct mmc *mmc) >>> struct sdhci_host *host = mmc->priv; >>> bool no_hispd_bit = false; >>> >>> + sdhci_set_control_reg(host); >> >> [2] >> >>> + >>> + /* If it needs to control the boards specific things, implement >>> callback */ >>> if (host->ops && host->ops->set_control_reg) >>> host->ops->set_control_reg(host); >>> >>> >>> Then other sdhci host drivers can also used the generic >>> sdhci_set_control_reg() according to SDHCI spec. >>> >> >> If [2] is added, then all the drivers should use it and not "can use it" >> which would be a constraint while implementing. > > Then it can be avoid with if-else? > At this point, you're right. I didn't know exactly, but it makes sense. > >> >>> Well, it's possible that i misunderstood something, if so that, let me >>> know, plz. >>> >> >> >> As you have mentioned at [1] the s5p_sdhci.c driver does not implement >> set_control_reg() in the generic way so it should not call >> sdhci_set_control_reg(host) at [2] right? > > Right. When i had added set_control_reg callback, u-boot didn't follow mush > rather than now about linux kernel. > And it didn't introduce some features at that time. > >> >> >> So, the main reason for going with the implementation in the patch >> rather than the one suggested by you is to not break support for the >> existing drivers. > > Thanks for kindly explanation in more detail! > > If possible to resend the patchset based-on latest, i will test with them. > (There are some conflicts.) >
I have sent a respin(v3) of this series. Thanks, Aswath