Message-ID: <20200521182129.GB1331782@builder.lan>
Date:   Thu, 21 May 2020 11:21:29 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Veerabhadrarao Badiganti <vbadigan@...eaurora.org>
Cc:     adrian.hunter@...el.com, ulf.hansson@...aro.org,
        robh+dt@...nel.org, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org,
        Vijay Viswanath <vviswana@...eaurora.org>,
        Asutosh Das <asutoshd@...eaurora.org>,
        Andy Gross <agross@...nel.org>
Subject: Re: [PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

On Wed 20 May 04:16 PDT 2020, Veerabhadrarao Badiganti wrote:

> 
> Thanks Bjorn for the review. For major comments I'm responding.
> Other comments, i will take care of them in my next patch-set.
> 
> On 5/19/2020 1:27 AM, Bjorn Andersson wrote:
> > On Fri 15 May 04:18 PDT 2020, Veerabhadrarao Badiganti wrote:
[..]
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> > > +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> > > +			      struct mmc_host *mmc, int level)
> > > +{
> > > +	int load, ret;
> > > +
> > > +	if (IS_ERR(mmc->supply.vmmc))
> > > +		return 0;
> > > +
> > > +	if (msm_host->vmmc_load) {
> > > +		load = level ? msm_host->vmmc_load : 0;
> > > +		ret = regulator_set_load(mmc->supply.vmmc, load);
> > I started on the comment about regulator_set_load() that you can find
> > below...
> > 
> > > +		if (ret)
> > > +			goto out;
> > > +	}
> > > +
> > > +	ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> > ...but I don't see that mmc->ios.vdd necessarily is in sync with
> > "level". Or do you here simply set the load based on what the hardware
> > tell you and then orthogonally to that let the core enable/disable the
> > regulator?
> > 
> > Perhaps I'm just missing something obvious, but if not I believe this
> > warrants a comment describing that you're lowering the power level
> > regardless of the actual power being disabled.
> 
> ios.vdd will be in sync with level. Vdd will be either 0 or a valid voltage
> (3v).
> 
> This indirectly gets triggered/invoked through power-irq when driver writes
> 0
> or valid voltage to SDHCI_POWER_CONTROL register from
> sdhci_set_power_noreg().

Ok, thanks for explaining.

> > > +out:
> > > +	if (ret)
> > > +		pr_err("%s: vmmc set load/ocr failed: %d\n",
> > > +				mmc_hostname(mmc), ret);
> > Please use:
> > 	dev_err(mmc_dev(mmc), ...);
> > 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> > > +			      struct mmc_host *mmc, int level)
> > vqmmc_enabled is a bool and "level" sounds like an int with several
> > possible values. So please make level bool here as well, to make it
> > easer to read..
> > 
> > > +{
> > > +	int load, ret;
> > > +	struct mmc_ios ios;
> > > +
> > > +	if (IS_ERR(mmc->supply.vqmmc)			 ||
> > > +	    (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
> > > +	    (msm_host->vqmmc_enabled == level))
> > > +		return 0;
> > > +
> > > +	if (msm_host->vqmmc_load) {
> > > +		load = level ? msm_host->vqmmc_load : 0;
> > > +		ret = regulator_set_load(mmc->supply.vqmmc, load);
> > Since v5.0 the "load" of a regulator consumer is only taken into
> > consideration if the consumer is enabled. So given that you're toggling
> > the regulator below there's no need to change this here.
> > 
> > Just specify the "active load" at probe time.
> 
> For eMMC case, we don't disable this Vccq2 regulator by having always-on
> flag
> in the regulator node. Only for SDcard Vccq2 will be disabled.
> Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
> it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.
> 

You should still call regulator_enable()/regulator_disable() on your
consumer regulator in this driver. When you do this the regulator core
will conclude that the regulator_dev (i.e. the part that represents the
hardware) is marked always_on and will not enable/disable the regulator.

But it will still invoke _regulator_handle_consumer_enable() and
_regulator_handle_consumer_disable(), which will aggregate the "load" of
all client regulators and update the regulator's load.

So this will apply the load as you expect regardless of it being
supplied by a regulator marked as always_on.

> > 
> > > +		if (ret)
> > > +			goto out;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The IO voltage regulator may not always support a voltage close to
> > > +	 * vdd. Set IO voltage based on capability of the regulator.
> > > +	 */
> > Is this comment related to the if/else-if inside the conditional? If so
> > please move it one line down.
> > 
> > > +	if (level) {
> > > +		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> > > +			ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
> > > +		else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> > > +			ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
> > Please add a space here, to indicate that the if statement on the next
> > line is unrelated to the if/elseif above.
> > 
> > > +		if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> > > +			pr_debug("%s: %s: setting signal voltage: %d\n",
> > > +					mmc_hostname(mmc), __func__,
> > > +					ios.signal_voltage);
> > I strongly believe you should replace these debug prints with
> > tracepoints, throughout the driver.
> > 
> > > +			ret = mmc_regulator_set_vqmmc(mmc, &ios);
> > > +			if (ret < 0)
> > > +				goto out;
> > > +		}
> > > +		ret = regulator_enable(mmc->supply.vqmmc);
> > > +	} else {
> > > +		ret = regulator_disable(mmc->supply.vqmmc);
> > > +	}
> > Given that you don't need to regulator_set_load() this function is now
> > just one large if/else condition on a constant passed as an argument.
> > Please split it into sdhci_msm_enable_vqmmc() and
> > sdhci_msm_disable_vqmmc().
> 
> 
> Same response as above
> For eMMC case, we don't disable this Vccq2 regulator by having always-on
> flag
> in the regulator node. Only for SDcard Vccq2 will be disabled.
> Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
> it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.
> 
> > > +out:
> > > +	if (ret)
> > > +		pr_err("%s: vqmmc failed: %d\n", mmc_hostname(mmc), ret);
> > I think it would be useful to know if this error came from
> > mmc_regulator_set_vqmmc() or regulator_enable() - or
> > regulator_disable().
> > 
> > So please move this up and add some context in the error message, and
> > please use dev_err().
> > 
> > > +	else
> > > +		msm_host->vqmmc_enabled = level;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >   static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
> > >   {
> > >   	init_waitqueue_head(&msm_host->pwr_irq_wait);
> > > @@ -1401,8 +1478,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > >   {
> > >   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > >   	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > +	struct mmc_host *mmc = host->mmc;
> > >   	u32 irq_status, irq_ack = 0;
> > > -	int retry = 10;
> > > +	int retry = 10, ret = 0;
> > There's no need to initialize ret, in all occasions it's assigned before
> > being read.
> > 
> > >   	u32 pwr_state = 0, io_level = 0;
> > >   	u32 config;
> > >   	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> > > @@ -1438,14 +1516,35 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > >   	/* Handle BUS ON/OFF*/
> > >   	if (irq_status & CORE_PWRCTL_BUS_ON) {
> > > -		pwr_state = REQ_BUS_ON;
> > > -		io_level = REQ_IO_HIGH;
> > > -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 1);
> > > +		if (!ret)
> > > +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 1);
> > I find this quite complex to follow. Wouldn't it be cleaner to retain
> > the 4 checks on irq_status as they are and then before the writel of
> > irq_ack check pwr_state and io_level and call sdhci_msm_set_{vmmc,vqmmc}
> > accordingly?
> 
> I will see if i can update as you suggested.
> 
> > > +
> > > +		if (!ret) {
> > > +			pwr_state = REQ_BUS_ON;
> > > +			io_level = REQ_IO_HIGH;
> > > +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		} else {
> > > +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> > > +					mmc_hostname(mmc), ret, irq_status);
> > You already printed that this failed in sdhci_msm_set_{vmmc,vqmmc}, no
> > need to print again.
> > 
> > > +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> > > +			sdhci_msm_set_vmmc(msm_host, mmc, 0);
> > > +		}
> > >   	}
> > >   	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> > > -		pwr_state = REQ_BUS_OFF;
> > > -		io_level = REQ_IO_LOW;
> > > -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 0);
> > > +		if (!ret)
> > > +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 0);
> > > +
> > > +		if (!ret) {
> > > +			pwr_state = REQ_BUS_OFF;
> > > +			io_level = REQ_IO_LOW;
> > > +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		} else {
> > > +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> > > +					mmc_hostname(mmc), ret, irq_status);
> > > +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> > > +		}
> > >   	}
> > >   	/* Handle IO LOW/HIGH */
> > >   	if (irq_status & CORE_PWRCTL_IO_LOW) {
> > > @@ -1457,6 +1556,15 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > >   		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> > >   	}
> > > +	if (io_level && !IS_ERR(mmc->supply.vqmmc) && !pwr_state) {
> > > +		ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);
> > Didn't you already call this through sdhci_msm_set_vqmmc()?
> 
> No.sdhci_msm_set_vqmmc handles only vqmmc ON/OFF. While turning it ON it
> sets
> Vqmmc to possbile default IO level (1.8v or 3.0v).
> Where this is only to make IO lines high (3.0v) or Low (1.8v).

If you move both the regulator operations here (below the point where
you figure out pwr_state and io_level), wouldn't it be possible to avoid
the additional, nested, vqmmc voltage request?

> > > +		if (ret < 0)
> > > +			pr_err("%s: IO_level setting failed(%d). signal_voltage: %d, vdd: %d irq_status: 0x%08x\n",
> > > +					mmc_hostname(mmc), ret,
> > > +					mmc->ios.signal_voltage, mmc->ios.vdd,
> > > +					irq_status);
> > > +	}
> > > +
> > >   	/*
> > >   	 * The driver has to acknowledge the interrupt, switch voltages and
> > >   	 * report back if it succeded or not to this register. The voltage
> > > @@ -1833,6 +1941,91 @@ static void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
> > >   	sdhci_reset(host, mask);
> > >   }
> > > +static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
> > > +{
> > > +	int ret = 0;
> > No need to initialize ret, first use is an assignment.
> > 
> > > +	struct mmc_host *mmc = msm_host->mmc;
> > > +
> > > +	ret = mmc_regulator_get_supply(msm_host->mmc);
> > > +	if (ret)
> > > +		return ret;
> > > +	device_property_read_u32(&msm_host->pdev->dev,
> > > +			"vmmc-max-load-microamp",
> > > +			&msm_host->vmmc_load);
> > > +	device_property_read_u32(&msm_host->pdev->dev,
> > > +			"vqmmc-max-load-microamp",
> > > +			&msm_host->vqmmc_load);
> > These properties are not documented. Do they vary enough to mandate
> > being read from DT or could they simply be approximated by a define
> > instead?
> 
> I can use defines. But since these values are different for eMMC and SDcard
> I will have to maintain two sets and need to have logic during probe to
> identify SDcard or eMMC and use the assign the set accordingly.
> So we tought, getting from dt is simpler and clean.
> In case Rob didn't agree with dt entries, I will go with this approach.
> 

Sounds reasonable, let's see what Rob says.

> > > +
> > > +	sdhci_msm_set_regulator_caps(msm_host);
> > > +	mmc->ios.power_mode = MMC_POWER_UNDEFINED;
> > > +
> > > +	return 0;
> > > +
> > > +}
> > > +
> > > +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
> > > +				      struct mmc_ios *ios)
> > > +{
> > > +	struct sdhci_host *host = mmc_priv(mmc);
> > > +	u16 ctrl;
> > > +
> > > +	/*
> > > +	 * Signal Voltage Switching is only applicable for Host Controllers
> > > +	 * v3.00 and above.
> > > +	 */
> > > +	if (host->version < SDHCI_SPEC_300)
> > > +		return 0;
> > > +
> > > +	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > > +
> > > +	switch (ios->signal_voltage) {
> > > +	case MMC_SIGNAL_VOLTAGE_330:
> > > +		if (!(host->flags & SDHCI_SIGNALING_330))
> > > +			return -EINVAL;
> > > +		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
> > > +		ctrl &= ~SDHCI_CTRL_VDD_180;
> > > +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> > > +
> > > +		/* 3.3V regulator output should be stable within 5 ms */
> > What mechanism ensures that the readw won't return withing 5ms from the
> > writew above?
> 
> Thanks for pointing this. This delay got missed. I will add it in next
> patchset.

Nice, thanks.

Regards,
Bjorn

Powered by blists - more mailing lists