Messages in this thread | | | Subject | Re: [PATCH V2 1/2] mmc: sdhci-msm: Add support to store supported vdd-io voltages | From | Vijay Viswanath <> | Date | Mon, 19 Mar 2018 18:02:01 +0530 |
| |
On 3/7/2018 9:42 PM, Doug Anderson wrote: > Hi, > > On Tue, Mar 6, 2018 at 11:13 PM, Vijay Viswanath > <vviswana@codeaurora.org> wrote: >> Hi Dough, Jeremy, >> >> >> On 3/3/2018 4:38 AM, Jeremy McNicoll wrote: >>> >>> On 2018-03-02 10:23 AM, Doug Anderson wrote: >>>> >>>> Hi, >>>> >>>> On Sun, Feb 11, 2018 at 10:01 PM, Vijay Viswanath >>>> <vviswana@codeaurora.org> wrote: >>>>> >>>>> During probe check whether the vdd-io regulator of sdhc platform device >>>>> can support 1.8V and 3V and store this information as a capability of >>>>> platform device. >>>>> >>>>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org> >>>>> --- >>>>> drivers/mmc/host/sdhci-msm.c | 38 >>>>> ++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 38 insertions(+) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>>>> index c283291..5c23e92 100644 >>>>> --- a/drivers/mmc/host/sdhci-msm.c >>>>> +++ b/drivers/mmc/host/sdhci-msm.c >>>>> @@ -23,6 +23,7 @@ >>>>> #include <linux/iopoll.h> >>>>> >>>>> #include "sdhci-pltfm.h" >>>>> +#include <linux/regulator/consumer.h> >>>> >>>> >>>> This is a strange sort order for this include file. Why is it after >>>> the local include? >>>> >>>> >>>>> #define CORE_MCI_VERSION 0x50 >>>>> #define CORE_VERSION_MAJOR_SHIFT 28 >>>>> @@ -81,6 +82,9 @@ >>>>> #define CORE_HC_SELECT_IN_HS400 (6 << 19) >>>>> #define CORE_HC_SELECT_IN_MASK (7 << 19) >>>>> >>>>> +#define CORE_3_0V_SUPPORT (1 << 25) >>>>> +#define CORE_1_8V_SUPPORT (1 << 26) >>>>> + >>>> >>>> >>>> Is there something magical about 25 and 26? This is a new caps field, >>>> so I'd have expected 0 and 1. >>>> >>>> >> >> Yes, these bits are the same corresponding to the capabilities in the >> Capabilities Register (offset 0x40). The bit positions become important when >> capabilities register doesn't show support to some voltages, but we can >> support those voltages. At that time, we will have to fake capabilities. The >> changes for those are currently not yet pushed up. >> >> >>>>> #define CORE_CSR_CDC_CTLR_CFG0 0x130 >>>>> #define CORE_SW_TRIG_FULL_CALIB BIT(16) >>>>> #define CORE_HW_AUTOCAL_ENA BIT(17) >>>>> @@ -148,6 +152,7 @@ struct sdhci_msm_host { >>>>> u32 curr_io_level; >>>>> wait_queue_head_t pwr_irq_wait; >>>>> bool pwr_irq_flag; >>>>> + u32 caps_0; >>>>> }; >>>>> >>>>> static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host >>>>> *host, >>>>> @@ -1313,6 +1318,35 @@ static void sdhci_msm_writeb(struct sdhci_host >>>>> *host, u8 val, int reg) >>>>> sdhci_msm_check_power_status(host, req_type); >>>>> } >>>>> >>>>> +static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host >>>>> *msm_host) >>>>> +{ >>>>> + struct mmc_host *mmc = msm_host->mmc; >>>>> + struct regulator *supply = mmc->supply.vqmmc; >>>>> + int i, count; >>>>> + u32 caps = 0, vdd_uV; >>>>> + >>>>> + if (!IS_ERR(mmc->supply.vqmmc)) { >>>>> + count = regulator_count_voltages(supply); >>>>> + if (count < 0) >>>>> + return count; >>>>> + for (i = 0; i < count; i++) { >>>>> + vdd_uV = regulator_list_voltage(supply, i); >>>>> + if (vdd_uV <= 0) >>>>> + continue; >>>>> + if (vdd_uV > 2700000) >>>>> + caps |= CORE_3_0V_SUPPORT; >>>>> + if (vdd_uV < 1950000) >>>>> + caps |= CORE_1_8V_SUPPORT; >>>>> + } >>>> >>>> >>>> Shouldn't you be using regulator_is_supported_voltage() rather than >>>> open coding? Also: I've never personally worked on a device where it >>>> was used, but there is definitely a concept floating about of a >>>> voltage level of 1.2V. Maybe should copy the ranges from >>>> mmc_regulator_set_vqmmc()? >>>> >>>> >> >> regulator_is_supported_voltage() checks for a range and it also uses >> regulator_list_voltage() internally. regulator_list_voltage() is also an >> exported API for use by drivers AFAIK. Please correct if it is not. > > Sure, regulator_list_voltage() is valid to call. I'm not saying that > your code is wrong or violates abstractions, just that it's > essentially re-implementing regulator_is_supported_voltage() for very > little gain. Calling regulator_is_supported_voltage() is better > because: > > 1. In theory, it should generate less code. Sure, it might loop twice > with the current implementation of regulator_is_supported_voltage(), > but for a non-time-critical section like this smaller code is likely > better than faster code (decreases kernel size / uses up less cache > space, etc). > > 2. If regulator_is_supported_voltage() is ever improved to be more > efficient you'll get that improvement automatically. If someone > happened to source vqmmc from a PWM regulator, for instance, trying to > enumerate all voltages like this would be a disaster. > > 3. Code will be simpler to understand. > > You can replace your whole loop with: > > if (regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000, 1950000)) > caps |= CORE_1_8V_SUPPORT > if (regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000, 3600000)) > caps |= CORE_3_0V_SUPPORT > > >>>> Also: seems like you should have some way to deal with "caps" ending >>>> up w/ no bits set. IIRC you can have a regulator that can be enabled >>>> / disabled but doesn't list a voltage, so if someone messed up their >>>> device tree you could end up in this case. Should you print a >>>> warning? ...or treat it as if we support "3.0V"? ...or ? I guess it >>>> depends on how do you want patch #2 to behave in that case. >>> >>> >>> Both, initialize it to sane value and print something. This way at >>> least you have a good chance of booting and not hard hanging and you >>> are given a reasonable message indicating what needs to be fixed. >>> >>> -jeremy >>>
Its good to add a warning, but initializing it to some value might not be appropriate. It will be better to leave it blank and if caps doesn't have any of 1.8V/3V, better to not enable IO_PAD_PWR_SWITCH_EN.
>>>> >>>> >>>>> + } >>>> >>>> >>>> How should things behave if vqmmc is an error? In that case is it >>>> important to not set "CORE_IO_PAD_PWR_SWITCH_EN" in patch set #2? >>>> ...or should you set "CORE_IO_PAD_PWR_SWITCH_EN" but then make sure >>>> you don't set "CORE_IO_PAD_PWR_SWITCH"? >>>> >>>> >> >> Thanks for the suggestion. If the regulators exit and doesn't list the >> voltages, then I believe initialization itself will not happen. We will not >> have any available ocr and in sdhci_setup_host it should fail. >> But these enhancements can be incorporated. Since this patch is already >> acknowledged, I will incorporate these changes in a subsequent patch. > > It's already acknowledged? I saw that your RFC was acknowledged by > Adrian Hunter but then you didn't include that tag in the posting of > v2, so I assumed for some reason it no longer applied. If you're > thinking that Ulf would be the one to apply this patch, he probably > doesn't know that it's Acked either. > > Perhaps Adrian or Ulf can give direction for how they see this patch proceeding. > >
Since I put up V2 anyway, I will include your suggestions and put V3. My mistake, I didn't notice the ACK was for RFC.
>>>>> + msm_host->caps_0 |= caps; >>>>> + pr_debug("%s: %s: supported caps: 0x%08x\n", mmc_hostname(mmc), >>>>> + __func__, caps); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> + >>>>> static const struct of_device_id sdhci_msm_dt_match[] = { >>>>> { .compatible = "qcom,sdhci-msm-v4" }, >>>>> {}, >>>>> @@ -1530,6 +1564,10 @@ static int sdhci_msm_probe(struct platform_device >>>>> *pdev) >>>>> ret = sdhci_add_host(host); >>>>> if (ret) >>>>> goto pm_runtime_disable; >>>>> + ret = sdhci_msm_set_regulator_caps(msm_host); >>>>> + if (ret) >>>>> + dev_err(&pdev->dev, "%s: Failed to set regulator caps: >>>>> %d\n", >>>>> + __func__, ret); >>>> >>>> >>>> Why do you need __func__ here? You're already using dev_err(), that >>>> gives an idea of where we are. >>>> >> >> dev_err() doesn't give information of where it is getting called. > > It gives you the driver and the error message should be unique to the > driver and easy to find. Including "__func__ in messages like this is > discouraged unless you are in a context where you somehow can't get > access to the device pointer. I suppose ultimately it's up the the > maintainer for individual cases but overall I've seen this to be a > consistently applied rule in the kernel. > > In any case, why would this particular print be special that it should > include __func__ but all others (in this file, or in dev_err in > general) shouldn't? > > >>>>> pm_runtime_mark_last_busy(&pdev->dev); >>>>> pm_runtime_put_autosuspend(&pdev->dev); >>>>> -- >>>>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation >>>>> Center, Inc. >>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a >>>>> Linux Foundation Collaborative Project. >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> Thanks, >> Vijay > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Thanks, Vijay
| |