Re: [RFC PATCH v3.1 16/27] mmc: sdhci-uhs2: add set_ios()
On 30/11/20 9:51 am, AKASHI Takahiro wrote:
> On Thu, Nov 26, 2020 at 10:17:11AM +0200, Adrian Hunter wrote:
>> On 6/11/20 4:27 am, AKASHI Takahiro wrote:
>>> This is a sdhci version of mmc's set_ios operation.
>>> It covers both UHS-I and UHS-II.
>>>
>>> Signed-off-by: Ben Chuang
>>> Signed-off-by: AKASHI Takahiro
>>> ---
>>> drivers/mmc/host/sdhci-uhs2.c | 100 ++
>>> drivers/mmc/host/sdhci-uhs2.h | 1 +
>>> drivers/mmc/host/sdhci.c | 40 +-
>>> drivers/mmc/host/sdhci.h | 2 +
>>> 4 files changed, 128 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-uhs2.c b/drivers/mmc/host/sdhci-uhs2.c
>>> index d9e98c097bfe..637464748cc4 100644
>>> --- a/drivers/mmc/host/sdhci-uhs2.c
>>> +++ b/drivers/mmc/host/sdhci-uhs2.c
>>> @@ -263,6 +263,74 @@ void sdhci_uhs2_set_timeout(struct sdhci_host *host,
>>> struct mmc_command *cmd)
>>> }
>>> EXPORT_SYMBOL_GPL(sdhci_uhs2_set_timeout);
>>>
>>> +/**
>>> + * sdhci_uhs2_clear_set_irqs - set Error Interrupt Status Enable register
>>> + * @host: SDHCI host
>>> + * @clear: bit-wise clear mask
>>> + * @set: bit-wise set mask
>>> + *
>>> + * Set/unset bits in UHS-II Error Interrupt Status Enable register
>>> + */
>>> +void sdhci_uhs2_clear_set_irqs(struct sdhci_host *host, u32 clear, u32 set)
>>> +{
>>> + u32 ier;
>>> +
>>> + ier = sdhci_readl(host, SDHCI_UHS2_ERR_INT_STATUS_EN);
>>> + ier &= ~clear;
>>> + ier |= set;
>>> + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_STATUS_EN);
>>> + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_SIG_EN);
>>> +}
>>> +EXPORT_SYMBOL_GPL(sdhci_uhs2_clear_set_irqs);
>>> +
>>> +static void __sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> + u8 cmd_res, dead_lock;
>>> + u16 ctrl_2;
>>> + unsigned long flags;
>>> +
>>> + /* FIXME: why lock? */
>>> + spin_lock_irqsave(>lock, flags);
>>> +
>>> + /* UHS2 Timeout Control */
>>> + sdhci_calc_timeout_uhs2(host, _res, _lock);
>>> +
>>> + /* change to use calculate value */
>>> + cmd_res |= dead_lock << SDHCI_UHS2_TIMER_CTRL_DEADLOCK_SHIFT;
>>> +
>>> + sdhci_uhs2_clear_set_irqs(host,
>>> + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
>>> + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT,
>>> + 0);
>>> + sdhci_writeb(host, cmd_res, SDHCI_UHS2_TIMER_CTRL);
>>> + sdhci_uhs2_clear_set_irqs(host, 0,
>>> + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
>>> + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT);
>>> +
>>> + /* UHS2 timing */
>>> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> + if (ios->timing == MMC_TIMING_UHS2)
>>> + ctrl_2 |= SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN;
>>> + else
>>> + ctrl_2 &= ~(SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN);
>>> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>> +
>>> + if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN))
>>> + sdhci_enable_preset_value(host, true);
>>> +
>>> + if (host->ops->set_power)
>>> + host->ops->set_power(host, ios->power_mode, ios->vdd);
>>> + else
>>> + sdhci_set_power(host, ios->power_mode, ios->vdd);
>>> + udelay(100);
>>> +
>>> + host->timing = ios->timing;
>>> + sdhci_set_clock(host, host->clock);
>>
>> sdhci_set_ios_common() already called ->set_clock() and ->set_power(), so I
>> am not really following what is going on here. Can you explain some more?
>
> To be frank, I don't know. The logic in Intel's (and/or Ben's?)
> original code does so.
> What I changed is to remove the code of setting (ios->vdd and) ios->vdd2,
> which is executed before calling set_power(), in __sdhci_uhs2_set_ios().
>
> So yes, effectively it may be of no use to call set_power() here.
Please try to rationalize it. Also set_ios() should not need the spin lock,
and that allows clock and power callbacks to sleep if needed.
>
> -Takahiro Akashi
>
>>> +
>>> + spin_unlock_irqrestore(>lock, flags);
>>> +}
>>> +
>>>
>>> /*\
>>> *
>>> *
>>> * MMC callbacks
>>> *
>>> @@ -286,6 +354,37 @@ static int
>>> sdhci_uhs2_start_signal_voltage_switch(struct mmc_host *mmc,
>>> return sdhci_start_signal_voltage_switch(mmc, ios);
>>> }
>>>
>>> +void sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + if (!(host->version >= SDHCI_SPEC_400) ||
>>> + !(host->mmc->flags & MMC_UHS2_SUPPORT &&
>>> + host->mmc->caps & MMC_CAP_UHS2)) {
>>> + sdhci_set_ios(mmc, ios);
>>> + return;
>>> + }
>>> +
>>> + if (ios->power_mode ==
Re: [RFC PATCH v3.1 16/27] mmc: sdhci-uhs2: add set_ios()
On Thu, Nov 26, 2020 at 10:17:11AM +0200, Adrian Hunter wrote:
> On 6/11/20 4:27 am, AKASHI Takahiro wrote:
> > This is a sdhci version of mmc's set_ios operation.
> > It covers both UHS-I and UHS-II.
> >
> > Signed-off-by: Ben Chuang
> > Signed-off-by: AKASHI Takahiro
> > ---
> > drivers/mmc/host/sdhci-uhs2.c | 100 ++
> > drivers/mmc/host/sdhci-uhs2.h | 1 +
> > drivers/mmc/host/sdhci.c | 40 +-
> > drivers/mmc/host/sdhci.h | 2 +
> > 4 files changed, 128 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-uhs2.c b/drivers/mmc/host/sdhci-uhs2.c
> > index d9e98c097bfe..637464748cc4 100644
> > --- a/drivers/mmc/host/sdhci-uhs2.c
> > +++ b/drivers/mmc/host/sdhci-uhs2.c
> > @@ -263,6 +263,74 @@ void sdhci_uhs2_set_timeout(struct sdhci_host *host,
> > struct mmc_command *cmd)
> > }
> > EXPORT_SYMBOL_GPL(sdhci_uhs2_set_timeout);
> >
> > +/**
> > + * sdhci_uhs2_clear_set_irqs - set Error Interrupt Status Enable register
> > + * @host: SDHCI host
> > + * @clear: bit-wise clear mask
> > + * @set: bit-wise set mask
> > + *
> > + * Set/unset bits in UHS-II Error Interrupt Status Enable register
> > + */
> > +void sdhci_uhs2_clear_set_irqs(struct sdhci_host *host, u32 clear, u32 set)
> > +{
> > + u32 ier;
> > +
> > + ier = sdhci_readl(host, SDHCI_UHS2_ERR_INT_STATUS_EN);
> > + ier &= ~clear;
> > + ier |= set;
> > + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_STATUS_EN);
> > + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_SIG_EN);
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_uhs2_clear_set_irqs);
> > +
> > +static void __sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > +{
> > + struct sdhci_host *host = mmc_priv(mmc);
> > + u8 cmd_res, dead_lock;
> > + u16 ctrl_2;
> > + unsigned long flags;
> > +
> > + /* FIXME: why lock? */
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + /* UHS2 Timeout Control */
> > + sdhci_calc_timeout_uhs2(host, _res, _lock);
> > +
> > + /* change to use calculate value */
> > + cmd_res |= dead_lock << SDHCI_UHS2_TIMER_CTRL_DEADLOCK_SHIFT;
> > +
> > + sdhci_uhs2_clear_set_irqs(host,
> > + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
> > + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT,
> > + 0);
> > + sdhci_writeb(host, cmd_res, SDHCI_UHS2_TIMER_CTRL);
> > + sdhci_uhs2_clear_set_irqs(host, 0,
> > + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
> > + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT);
> > +
> > + /* UHS2 timing */
> > + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > + if (ios->timing == MMC_TIMING_UHS2)
> > + ctrl_2 |= SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN;
> > + else
> > + ctrl_2 &= ~(SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN);
> > + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > +
> > + if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN))
> > + sdhci_enable_preset_value(host, true);
> > +
> > + if (host->ops->set_power)
> > + host->ops->set_power(host, ios->power_mode, ios->vdd);
> > + else
> > + sdhci_set_power(host, ios->power_mode, ios->vdd);
> > + udelay(100);
> > +
> > + host->timing = ios->timing;
> > + sdhci_set_clock(host, host->clock);
>
> sdhci_set_ios_common() already called ->set_clock() and ->set_power(), so I
> am not really following what is going on here. Can you explain some more?
To be frank, I don't know. The logic in Intel's (and/or Ben's?)
original code does so.
What I changed is to remove the code of setting (ios->vdd and) ios->vdd2,
which is executed before calling set_power(), in __sdhci_uhs2_set_ios().
So yes, effectively it may be of no use to call set_power() here.
-Takahiro Akashi
> > +
> > + spin_unlock_irqrestore(>lock, flags);
> > +}
> > +
> >
> > /*\
> > *
> > *
> > * MMC callbacks
> > *
> > @@ -286,6 +354,37 @@ static int
> > sdhci_uhs2_start_signal_voltage_switch(struct mmc_host *mmc,
> > return sdhci_start_signal_voltage_switch(mmc, ios);
> > }
> >
> > +void sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > +{
> > + struct sdhci_host *host = mmc_priv(mmc);
> > +
> > + if (!(host->version >= SDHCI_SPEC_400) ||
> > + !(host->mmc->flags & MMC_UHS2_SUPPORT &&
> > + host->mmc->caps & MMC_CAP_UHS2)) {
> > + sdhci_set_ios(mmc, ios);
> > + return;
> > + }
> > +
> > + if (ios->power_mode == MMC_POWER_UNDEFINED)
> > + return;
> > +
> > + if (host->flags & SDHCI_DEVICE_DEAD) {
> > + if (!IS_ERR(mmc->supply.vmmc) &&
> > + ios->power_mode == MMC_POWER_OFF)
> > +
Re: [RFC PATCH v3.1 16/27] mmc: sdhci-uhs2: add set_ios()
On 6/11/20 4:27 am, AKASHI Takahiro wrote:
> This is a sdhci version of mmc's set_ios operation.
> It covers both UHS-I and UHS-II.
>
> Signed-off-by: Ben Chuang
> Signed-off-by: AKASHI Takahiro
> ---
> drivers/mmc/host/sdhci-uhs2.c | 100 ++
> drivers/mmc/host/sdhci-uhs2.h | 1 +
> drivers/mmc/host/sdhci.c | 40 +-
> drivers/mmc/host/sdhci.h | 2 +
> 4 files changed, 128 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-uhs2.c b/drivers/mmc/host/sdhci-uhs2.c
> index d9e98c097bfe..637464748cc4 100644
> --- a/drivers/mmc/host/sdhci-uhs2.c
> +++ b/drivers/mmc/host/sdhci-uhs2.c
> @@ -263,6 +263,74 @@ void sdhci_uhs2_set_timeout(struct sdhci_host *host,
> struct mmc_command *cmd)
> }
> EXPORT_SYMBOL_GPL(sdhci_uhs2_set_timeout);
>
> +/**
> + * sdhci_uhs2_clear_set_irqs - set Error Interrupt Status Enable register
> + * @host:SDHCI host
> + * @clear: bit-wise clear mask
> + * @set: bit-wise set mask
> + *
> + * Set/unset bits in UHS-II Error Interrupt Status Enable register
> + */
> +void sdhci_uhs2_clear_set_irqs(struct sdhci_host *host, u32 clear, u32 set)
> +{
> + u32 ier;
> +
> + ier = sdhci_readl(host, SDHCI_UHS2_ERR_INT_STATUS_EN);
> + ier &= ~clear;
> + ier |= set;
> + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_STATUS_EN);
> + sdhci_writel(host, ier, SDHCI_UHS2_ERR_INT_SIG_EN);
> +}
> +EXPORT_SYMBOL_GPL(sdhci_uhs2_clear_set_irqs);
> +
> +static void __sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + u8 cmd_res, dead_lock;
> + u16 ctrl_2;
> + unsigned long flags;
> +
> + /* FIXME: why lock? */
> + spin_lock_irqsave(>lock, flags);
> +
> + /* UHS2 Timeout Control */
> + sdhci_calc_timeout_uhs2(host, _res, _lock);
> +
> + /* change to use calculate value */
> + cmd_res |= dead_lock << SDHCI_UHS2_TIMER_CTRL_DEADLOCK_SHIFT;
> +
> + sdhci_uhs2_clear_set_irqs(host,
> + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
> + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT,
> + 0);
> + sdhci_writeb(host, cmd_res, SDHCI_UHS2_TIMER_CTRL);
> + sdhci_uhs2_clear_set_irqs(host, 0,
> + SDHCI_UHS2_ERR_INT_STATUS_RES_TIMEOUT |
> + SDHCI_UHS2_ERR_INT_STATUS_DEADLOCK_TIMEOUT);
> +
> + /* UHS2 timing */
> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (ios->timing == MMC_TIMING_UHS2)
> + ctrl_2 |= SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN;
> + else
> + ctrl_2 &= ~(SDHCI_CTRL_UHS_2 | SDHCI_CTRL_UHS2_INTERFACE_EN);
> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +
> + if (!(host->quirks2 & SDHCI_QUIRK2_PRESET_VALUE_BROKEN))
> + sdhci_enable_preset_value(host, true);
> +
> + if (host->ops->set_power)
> + host->ops->set_power(host, ios->power_mode, ios->vdd);
> + else
> + sdhci_set_power(host, ios->power_mode, ios->vdd);
> + udelay(100);
> +
> + host->timing = ios->timing;
> + sdhci_set_clock(host, host->clock);
sdhci_set_ios_common() already called ->set_clock() and ->set_power(), so I
am not really following what is going on here. Can you explain some more?
> +
> + spin_unlock_irqrestore(>lock, flags);
> +}
> +
>
> /*\
> *
> *
> * MMC callbacks
> *
> @@ -286,6 +354,37 @@ static int sdhci_uhs2_start_signal_voltage_switch(struct
> mmc_host *mmc,
> return sdhci_start_signal_voltage_switch(mmc, ios);
> }
>
> +void sdhci_uhs2_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + if (!(host->version >= SDHCI_SPEC_400) ||
> + !(host->mmc->flags & MMC_UHS2_SUPPORT &&
> + host->mmc->caps & MMC_CAP_UHS2)) {
> + sdhci_set_ios(mmc, ios);
> + return;
> + }
> +
> + if (ios->power_mode == MMC_POWER_UNDEFINED)
> + return;
> +
> + if (host->flags & SDHCI_DEVICE_DEAD) {
> + if (!IS_ERR(mmc->supply.vmmc) &&
> + ios->power_mode == MMC_POWER_OFF)
> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
> + if (!IS_ERR_OR_NULL(mmc->supply.vmmc2) &&
> + ios->power_mode == MMC_POWER_OFF)
> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc2, 0);
> + return;
> + }
> +
> + /* FIXME: host->timing = ios->timing */
> +
> + sdhci_set_ios_common(mmc, ios);
> +
> + __sdhci_uhs2_set_ios(mmc, ios);
> +}
> +
>
> /*\
> *
