mmc: sdhci-msm: Configuring IO_PAD support for sdhci-msm
Subsystems (3)
v4 cover mmc: sdhci-msm: Configuring IO_PAD support for sdhci-msm
Quoted 1 line
From the HPG:
In some platform, SDCC controller can be connected to either an eMMC device or an SD card. The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
For SD usage the default value of this signal is ‘0’, and SD driver changes it to ‘1’ as a part of voltage switching sequence.
For eMMC usage, SW should configure this signal to ‘1’ and supply 1.8v to PADs before starting any activity on the eMMC BUS.
To set this signal, write the following in theSDC1_SDCC_HC_VENDOR_SPECIFIC_FUNC register:
HC_IO_PAD_PWR_SWITCH: bit 16 HC_IO_PAD_PWR_SWITCH_EN: bit 15
Changes since v1:
Modified comments on io_pad related changes. Split some read+modify+write commands to multiple lines
Changes since v2:
IO_PAD_PWR_SWITCH_EN will be set only if we have info regarding what voltage is supported by the regulators.
Replaced regulator_list_voltage() API with regulator_is_supported_voltage().
Changes since v3:
Removed unnecessary prints and extra lines.
Krishna Konda (1):
mmc: sdhci-msm: support voltage pad switching
Vijay Viswanath (1):
mmc: sdhci-msm: Add support to store supported vdd-io voltages
drivers/mmc/host/sdhci-msm.c | 99 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 2 deletions(-)
Signature · 2 lines
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.
PATCH v4 1/2 mmc: sdhci-msm: Add support to store supported vdd-io voltages
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.
--- drivers/mmc/host/sdhci-msm.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-msm.c +34 -1
@@ -21,6 +21,7 @@#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/iopoll.h> +#include <linux/regulator/consumer.h> #include "sdhci-pltfm.h"
@@ -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) +#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,
@@ -1103,7 +1108,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)struct sdhci_msm_host *msm_host =sdhci_pltfm_priv (pltfm_host);u32 irq_status, irq_ack =0 ;int retry =10 ; -int pwr_state =0 , io_level =0 ; +u32 pwr_state =0 , io_level =0 ; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS );
@@ -1313,6 +1318,30 @@ 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; +u32 caps =0 ; + +if (!IS_ERR (mmc->supply.vqmmc)) { +if (regulator_is_supported_voltage (supply,1700000 ,1950000 )) + caps |=CORE_1_8V_SUPPORT ; +if (regulator_is_supported_voltage (supply,2700000 ,3600000 )) + caps |=CORE_3_0V_SUPPORT ; + +if (!caps) +pr_warn ("%s: %s: 1.8/3V not supported for vqmmc\n" , +mmc_hostname (mmc), __func__); + } + + 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 +1559,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,"Failed to set regulator caps: %d\n" , + ret);pm_runtime_mark_last_busy (&pdev->dev);pm_runtime_put_autosuspend (&pdev->dev);
Signature · 2 lines
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.
Re: [PATCH V4 1/2] mmc: sdhci-msm: Add support to store supported vdd-io voltages
Hi,
Quoted 8 lines · Vijay Viswanath
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 <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
Since I commented on v2, please copy me for this series going forward. Thanks.
Quoted 40 lines
drivers/mmc/host/sdhci-msm.c +6
-1@@ -21,6 +21,7 @@#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/iopoll.h> +#include <linux/regulator/consumer.h> #include "sdhci-pltfm.h" @@ -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) +#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,@@ -1103,7 +1108,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)struct sdhci_msm_host *msm_host =sdhci_pltfm_priv (pltfm_host);u32 irq_status, irq_ack =0 ;int retry =10 ; -int pwr_state =0 , io_level =0 ; +u32 pwr_state =0 , io_level =0 ; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS );
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1321
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +1
-0@@ -1313,6 +1318,30 @@ 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)
This function always returns 0. Make it void.
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+{ +struct mmc_host *mmc = msm_host->mmc; +struct regulator *supply = mmc->supply.vqmmc; +u32 caps =0 ;
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1335
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ +if (!IS_ERR (mmc->supply.vqmmc)) { +if (regulator_is_supported_voltage (supply,1700000 ,1950000 )) + caps |=CORE_1_8V_SUPPORT ; +if (regulator_is_supported_voltage (supply,2700000 ,3600000 )) + caps |=CORE_3_0V_SUPPORT ; + +if (!caps) +pr_warn ("%s: %s: 1.8/3V not supported for vqmmc\n" , +mmc_hostname (mmc), __func__);
Please remove __func__. You already have the unique thing to find the right driver (AKA mmc_hostname(mmc)) and the string itself should be enough from there.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1340
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+ } + + msm_host->caps_0 |= caps; +pr_debug ("%s: %s: supported caps: 0x%08x\n" ,mmc_hostname (mmc), + __func__, caps);
Same, no need for __func__.
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ +return 0 ; +} +static const struct of_device_id sdhci_msm_dt_match[] = { { .compatible ="qcom,sdhci-msm-v4" }, {},
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct platform_device *pdev) · line 1565
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +4
-0@@ -1530,6 +1559,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,"Failed to set regulator caps: %d\n" , + ret);
If you find some reason _not_ to make sdhci_msm_set_regulator_caps() return "void" as per above, you should actually do something about this error. You've used "dev_err" which makes me feel like you consider this a serious error. Presumably it should cause the probe to fail?
-Doug
Re: [PATCH V4 1/2] mmc: sdhci-msm: Add support to store supported vdd-io voltages
Quoted 8 lines · Doug Anderson
Hi,
Quoted 4 lines · Vijay Viswanath
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.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Vijay Viswanath <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)Since I commented on v2, please copy me for this series going forward. Thanks.
Will do. Sorry I missed.
Quoted 68 lines
Quoted 40 lines
drivers/mmc/host/sdhci-msm.c +6
-1@@ -21,6 +21,7 @@#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/iopoll.h> +#include <linux/regulator/consumer.h> #include "sdhci-pltfm.h" @@ -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) +#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,@@ -1103,7 +1108,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)struct sdhci_msm_host *msm_host =sdhci_pltfm_priv (pltfm_host);u32 irq_status, irq_ack =0 ;int retry =10 ; -int pwr_state =0 , io_level =0 ; +u32 pwr_state =0 , io_level =0 ; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS );drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1321
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +1
-0@@ -1313,6 +1318,30 @@ 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)This function always returns 0. Make it void.
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+{ +struct mmc_host *mmc = msm_host->mmc; +struct regulator *supply = mmc->supply.vqmmc; +u32 caps =0 ;drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1335
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ +if (!IS_ERR (mmc->supply.vqmmc)) { +if (regulator_is_supported_voltage (supply,1700000 ,1950000 )) + caps |=CORE_1_8V_SUPPORT ; +if (regulator_is_supported_voltage (supply,2700000 ,3600000 )) + caps |=CORE_3_0V_SUPPORT ; + +if (!caps) +pr_warn ("%s: %s: 1.8/3V not supported for vqmmc\n" , +mmc_hostname (mmc), __func__);Please remove
__func__. You already have the unique thing to find the right driver (AKAmmc_hostname(mmc)) and the string itself should be enough from there.
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg) · line 1340
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+ } + + msm_host->caps_0 |= caps; +pr_debug ("%s: %s: supported caps: 0x%08x\n" ,mmc_hostname (mmc), + __func__, caps);Same, no need for
__func__.
will remove all unnecessary __func__ references.
Quoted 22 lines
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ +return 0 ; +} +static const struct of_device_id sdhci_msm_dt_match[] = { { .compatible ="qcom,sdhci-msm-v4" }, {},drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct platform_device *pdev) · line 1565
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +4
-0@@ -1530,6 +1559,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,"Failed to set regulator caps: %d\n" , + ret);If you find some reason _not_ to make
sdhci_msm_set_regulator_caps()return "void" as per above, you should actually do something about this error. You've used "dev_err" which makes me feel like you consider this a serious error. Presumably it should cause the probe to fail?Quoted 1 line
Quoted 2 lines
-Doug
yeah, we don't need to print anything here as a warning is printed in sdhci_msm_set_regulator_caps() anyway.
PATCH v4 2/2 mmc: sdhci-msm: support voltage pad switching
In reply to Vijay ViswanathFrom: Krishna Konda <[email protected]>
The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
SDHC core on msm platforms should have IO_PAD_PWR_SWITCH bit set/unset based on actual voltage used for IO lines. So when power irq is triggered for io high or io low, the driver should check the voltages supported and set the pad accordingly.
--- drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-msm.c +62 -2
@@ -78,12 +78,15 @@#define CORE_HC_MCLK_SEL_DFLT (2 << 8)#define CORE_HC_MCLK_SEL_HS400 (3 << 8)#define CORE_HC_MCLK_SEL_MASK (3 << 8) +#define CORE_IO_PAD_PWR_SWITCH_EN (1 << 15) +#define CORE_IO_PAD_PWR_SWITCH (1 << 16)#define CORE_HC_SELECT_IN_EN BIT(18)#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) +#define CORE_VOLT_SUPPORT (CORE_3_0V_SUPPORT | CORE_1_8V_SUPPORT)#define CORE_CSR_CDC_CTLR_CFG0 0x130#define CORE_SW_TRIG_FULL_CALIB BIT(16)
@@ -1109,7 +1112,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)u32 irq_status, irq_ack =0 ;int retry =10 ;u32 pwr_state =0 , io_level =0 ; - +u32 config; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS ); irq_status &=INT_MASK ;
@@ -1166,6 +1169,45 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)*/ writel_relaxed (irq_ack, msm_host->core_mem +CORE_PWRCTL_CTL ); +/* +* If we don't have info regarding the voltage levels supported by +* regulators, don't change the IO PAD PWR SWITCH. +*/ +if (msm_host->caps_0 &CORE_VOLT_SUPPORT ) { +/* Ensure order between core_mem and hc_mem */ +mb (); +/* +* We should unset IO PAD PWR switch only if the register write +* can set IO lines high and the regulator also switches to 3 V. +* Else, we should keep the IO PAD PWR switch set. +* This is applicable to certain targets where eMMC vccq supply +* is only 1.8V. In such targets, even during REQ_IO_HIGH, the +* IO PAD PWR switch must be kept set to reflect actual +* regulator voltage. This way, during initialization of +* controllers with only 1.8V, we will set the IO PAD bit +* without waiting for a REQ_IO_LOW. +*/ + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + +if (((io_level &REQ_IO_HIGH ) && (msm_host->caps_0 & +CORE_3_0V_SUPPORT )) && + (config &CORE_IO_PAD_PWR_SWITCH )) { + config &= ~CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD register is in different memory space */ +mb (); + }else if (((io_level &REQ_IO_LOW ) || + (msm_host->caps_0 &CORE_1_8V_SUPPORT )) && + !(config &CORE_IO_PAD_PWR_SWITCH )) { + config |=CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD bit is in different memory space */ +mb (); + } + } +if (pwr_state) msm_host->curr_pwr_state = pwr_state;if (io_level)
@@ -1322,7 +1364,8 @@ 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; -u32 caps =0 ; +u32 caps =0 , config; +struct sdhci_host *host =mmc_priv (mmc);if (!IS_ERR (mmc->supply.vqmmc)) {if (regulator_is_supported_voltage (supply,1700000 ,1950000 ))
@@ -1335,6 +1378,23 @@ static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) mmc_hostname(mmc), __func__); } +if (caps) { +/* +* Set the PAD_PWR_SWITCH_EN bit so that the PAD_PWR_SWITCH +* bit can be used as required later on. +*/ +u32 io_level = msm_host->curr_io_level; + + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + config |=CORE_IO_PAD_PWR_SWITCH_EN ; + +if ((io_level &REQ_IO_HIGH ) && (caps &CORE_3_0V_SUPPORT )) + config &= ~CORE_IO_PAD_PWR_SWITCH ; +else if ((io_level &REQ_IO_LOW ) || (caps &CORE_1_8V_SUPPORT )) + config |=CORE_IO_PAD_PWR_SWITCH ; + +writel_relaxed (config, host->ioaddr +CORE_VENDOR_SPEC ); + } msm_host->caps_0 |= caps;pr_debug ("%s: %s: supported caps: 0x%08x\n" ,mmc_hostname (mmc), __func__, caps);
Signature · 2 lines
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.
Re: [PATCH V4 2/2] mmc: sdhci-msm: support voltage pad switching
Hi,
Quoted 48 lines · Vijay Viswanath
From: Krishna Konda <[email protected]>
The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
SDHC core on msm platforms should have
IO_PAD_PWR_SWITCHbit set/unset based on actual voltage used for IO lines. So when power irq is triggered for io high or io low, the driver should check the voltages supported and set the pad accordingly.Signed-off-by Krishna Konda <[email protected]>Signed-off-by Venkat Gopalakrishnan <[email protected]>Signed-off-by Vijay Viswanath <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +4
-1@@ -78,12 +78,15 @@#define CORE_HC_MCLK_SEL_DFLT (2 << 8)#define CORE_HC_MCLK_SEL_HS400 (3 << 8)#define CORE_HC_MCLK_SEL_MASK (3 << 8) +#define CORE_IO_PAD_PWR_SWITCH_EN (1 << 15) +#define CORE_IO_PAD_PWR_SWITCH (1 << 16)#define CORE_HC_SELECT_IN_EN BIT(18)#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) +#define CORE_VOLT_SUPPORT (CORE_3_0V_SUPPORT | CORE_1_8V_SUPPORT)#define CORE_CSR_CDC_CTLR_CFG0 0x130#define CORE_SW_TRIG_FULL_CALIB BIT(16)@@ -1109,7 +1112,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)u32 irq_status, irq_ack =0 ;int retry =10 ;u32 pwr_state =0 , io_level =0 ; - +u32 config; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS ); irq_status &=INT_MASK ;
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1178
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +7
-0@@ -1166,6 +1169,45 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)*/ writel_relaxed (irq_ack, msm_host->core_mem +CORE_PWRCTL_CTL ); +/* +* If we don't have info regarding the voltage levels supported by +* regulators, don't change the IO PAD PWR SWITCH. +*/ +if (msm_host->caps_0 &CORE_VOLT_SUPPORT ) { +/* Ensure order between core_mem and hc_mem */ +mb ();
Like in v2, I don't understand why you need a mb() before the read from CORE_VENDOR_SPEC. No reads or writes to the core_mem will affect the value you're reading here, so you need no barrier.
If you need a barrier before the _write_ to CORE_VENDOR_SPEC then add it below. Then in the case where the config doesn't change you have no barriers.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1189
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +11
-0+/* +* We should unset IO PAD PWR switch only if the register write +* can set IO lines high and the regulator also switches to 3 V. +* Else, we should keep the IO PAD PWR switch set. +* This is applicable to certain targets where eMMC vccq supply +* is only 1.8V. In such targets, even during REQ_IO_HIGH, the +* IO PAD PWR switch must be kept set to reflect actual +* regulator voltage. This way, during initialization of +* controllers with only 1.8V, we will set the IO PAD bit +* without waiting for a REQ_IO_LOW. +*/
For the above comment, what about just:
new_config = config
if (msm_host->caps_0 == CORE_1_8V_SUPPORT) {
new_config |= CORE_IO_PAD_PWR_SWITCH;
} else if (msm_host->caps_0 == CORE_3_3V_SUPPORT) {
new_config &= ~CORE_IO_PAD_PWR_SWITCH;
} else if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
if (io_level & REQ_IO_HIGH)
new_config &= ~CORE_IO_PAD_PWR_SWITCH;
else if (io_level & REQ_IO_LOW)
new_config |= CORE_IO_PAD_PWR_SWITCH;
}if (config != new_config) {
...
}
AKA: first check if it only supports one voltage and pick that one. Else if it supports both you can use the request. This might be more important if you get rid of the initial setting in sdhci_msm_set_regulator_caps() as I'm suggesting.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1199
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + +if (((io_level &REQ_IO_HIGH ) && (msm_host->caps_0 & +CORE_3_0V_SUPPORT )) && + (config &CORE_IO_PAD_PWR_SWITCH )) { + config &= ~CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD register is in different memory space */ +mb ();
Wow, for a driver that tries so hard to use "relaxed" versions of writes to avoid barriers you sure end up needing to sprinkle a lot of these around "just in case". :( ...this one seems extra fishy because:
* There are no more accesses after this one in this function.
* If you're worried about something that happens outside of the context of the IRQ needing this wb() then that's a silly concern. Presumably if they were doing anything that could race with you they'd have a lock and locking routines are implicit barriers.
* In the context of the IRQ itself the next call is sdhci_msm_complete_pwr_irq_wait(), which eventually calls wake_up. This has a locking primitive and thus an implicit barrier.
* There's a direct call of sdhci_msm_handle_pwr_irq() from probe, and it has a big fat mb(). I have a hard time believing that matters too because I'd bet "platform_get_irq_byname" has at least one lock in it.
IMHO these "_relaxed" calls are just not worth it except in _very_ targeted usage.
Quoted 29 lines
drivers/mmc/host/sdhci-msm.c +14
-1+ }else if (((io_level &REQ_IO_LOW ) || + (msm_host->caps_0 &CORE_1_8V_SUPPORT )) && + !(config &CORE_IO_PAD_PWR_SWITCH )) { + config |=CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD bit is in different memory space */ +mb (); + } + } +if (pwr_state) msm_host->curr_pwr_state = pwr_state;if (io_level)@@ -1322,7 +1364,8 @@ 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; -u32 caps =0 ; +u32 caps =0 , config; +struct sdhci_host *host =mmc_priv (mmc);if (!IS_ERR (mmc->supply.vqmmc)) {if (regulator_is_supported_voltage (supply,1700000 ,1950000 ))@@ -1335,6 +1378,23 @@ static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) mmc_hostname(mmc), __func__); } +if (caps) {
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1391
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+/* +* Set the PAD_PWR_SWITCH_EN bit so that the PAD_PWR_SWITCH +* bit can be used as required later on. +*/ +u32 io_level = msm_host->curr_io_level; + + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + config |=CORE_IO_PAD_PWR_SWITCH_EN ; + +if ((io_level &REQ_IO_HIGH ) && (caps &CORE_3_0V_SUPPORT ))
Slight nit that there's a tab character after "caps &". Please replace it with a space.
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1394
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ config &= ~CORE_IO_PAD_PWR_SWITCH ; + else if ((io_level & REQ_IO_LOW) || (caps & CORE_1_8V_SUPPORT)) + config |= CORE_IO_PAD_PWR_SWITCH;
Are you sure that's right? In English:
* If we requested high and we support high then set to high.
* else if we requested low __or__ we support low then set low.
Things that are weird above that:
* If we request low but don't support low, switch to low anyway.
* If we request high but only support low, switch to low anyway.
If nothing else seems like this would deserve a comment, but I'd be curious of the justification for that logic.
Also: seems like this is duplicated code between here and sdhci_msm_handle_pwr_irq(). Does it even need to be here? Can't you just move the call to sdhci_msm_set_regulator_caps() before the call to sdhci_msm_handle_pwr_irq() in probe? Then just let that first call to to sdhci_msm_handle_pwr_irq() do this work? In sdhci_msm_handle_pwr_irq() you can always just "OR" in CORE_IO_PAD_PWR_SWITCH_EN
-Doug
Re: [PATCH V4 2/2] mmc: sdhci-msm: support voltage pad switching
Quoted 87 lines · Doug Anderson
Hi,
Quoted 48 lines · Vijay Viswanath
From: Krishna Konda <[email protected]>
The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
SDHC core on msm platforms should have
IO_PAD_PWR_SWITCHbit set/unset based on actual voltage used for IO lines. So when power irq is triggered for io high or io low, the driver should check the voltages supported and set the pad accordingly.Signed-off-by Krishna Konda <[email protected]>Signed-off-by Venkat Gopalakrishnan <[email protected]>Signed-off-by Vijay Viswanath <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +4
-1@@ -78,12 +78,15 @@#define CORE_HC_MCLK_SEL_DFLT (2 << 8)#define CORE_HC_MCLK_SEL_HS400 (3 << 8)#define CORE_HC_MCLK_SEL_MASK (3 << 8) +#define CORE_IO_PAD_PWR_SWITCH_EN (1 << 15) +#define CORE_IO_PAD_PWR_SWITCH (1 << 16)#define CORE_HC_SELECT_IN_EN BIT(18)#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) +#define CORE_VOLT_SUPPORT (CORE_3_0V_SUPPORT | CORE_1_8V_SUPPORT)#define CORE_CSR_CDC_CTLR_CFG0 0x130#define CORE_SW_TRIG_FULL_CALIB BIT(16)@@ -1109,7 +1112,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)u32 irq_status, irq_ack =0 ;int retry =10 ;u32 pwr_state =0 , io_level =0 ; - +u32 config; irq_status =readl_relaxed (msm_host->core_mem +CORE_PWRCTL_STATUS ); irq_status &=INT_MASK ;drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1178
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +7
-0@@ -1166,6 +1169,45 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)*/ writel_relaxed (irq_ack, msm_host->core_mem +CORE_PWRCTL_CTL ); +/* +* If we don't have info regarding the voltage levels supported by +* regulators, don't change the IO PAD PWR SWITCH. +*/ +if (msm_host->caps_0 &CORE_VOLT_SUPPORT ) { +/* Ensure order between core_mem and hc_mem */ +mb ();Like in v2, I don't understand why you need a
mb()before the read fromCORE_VENDOR_SPEC. No reads or writes to thecore_memwill affect the value you're reading here, so you need no barrier.If you need a barrier before the _write_ to
CORE_VENDOR_SPECthen add it below. Then in the case where the config doesn't change you have no barriers.drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1189
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +11
-0+/* +* We should unset IO PAD PWR switch only if the register write +* can set IO lines high and the regulator also switches to 3 V. +* Else, we should keep the IO PAD PWR switch set. +* This is applicable to certain targets where eMMC vccq supply +* is only 1.8V. In such targets, even during REQ_IO_HIGH, the +* IO PAD PWR switch must be kept set to reflect actual +* regulator voltage. This way, during initialization of +* controllers with only 1.8V, we will set the IO PAD bit +* without waiting for a REQ_IO_LOW. +*/ For the above comment, what about just:
new_config= config
Quoted 10 lines
if (msm_host->caps_0 == CORE_1_8V_SUPPORT) {
new_config |= CORE_IO_PAD_PWR_SWITCH;
} else if (msm_host->caps_0 == CORE_3_3V_SUPPORT) {
new_config &= ~CORE_IO_PAD_PWR_SWITCH;
} else if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
if (io_level & REQ_IO_HIGH)
new_config &= ~CORE_IO_PAD_PWR_SWITCH;
else if (io_level & REQ_IO_LOW)
new_config |= CORE_IO_PAD_PWR_SWITCH;
}This looks a big mess of if/else. Does the above implementation have better performance compared to having two if/else with bit operations inside ? The latter looks much cleaner and faster.
If regulator only supports 3V and we get a io_low from BUS_OFF ( REQ_IO_LOW should never come if we don't support 1.8V), it is ok to set io pad.
Quoted 28 lines
if (config !=
new_config) {
...
}AKA: first check if it only supports one voltage and pick that one. Else if it supports both you can use the request. This might be more important if you get rid of the initial setting in
sdhci_msm_set_regulator_caps()as I'm suggesting.drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1199
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + +if (((io_level &REQ_IO_HIGH ) && (msm_host->caps_0 & +CORE_3_0V_SUPPORT )) && + (config &CORE_IO_PAD_PWR_SWITCH )) { + config &= ~CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD register is in different memory space */ +mb ();Wow, for a driver that tries so hard to use "relaxed" versions of writes to avoid barriers you sure end up needing to sprinkle a lot of these around "just in case". :( ...this one seems extra fishy because:
* There are no more accesses after this one in this function.
Quoted 9 lines
* If you're worried about something that happens outside of the context of the IRQ needing this
wb()then that's a silly concern. Presumably if they were doing anything that could race with you they'd have a lock and locking routines are implicit barriers.* In the context of the IRQ itself the next call is
sdhci_msm_complete_pwr_irq_wait(), which eventually callswake_up. This has a locking primitive and thus an implicit barrier.
Sorry, I didn't get implicit barrier in locking primitive.
In mmc_set_signal_voltage switch:
1. Send cmd 11.
2. Switch 1.8V in SDHCI_HOST_CONTROL2
3. Wait for pwr_irq wake_up().
4. pwr_irq context comes up & does register read/writes in core mem.
Updates IO PAD in HC mem.
5. pwr_irq calls wake_up.
6. mmc_set_signal_voltage_switch context does further register
read/writes which expects IO_PAD change within pwr_irq context is complete before step 6.
Can wake_up() ensure that any update to CORE_VENDOR_SPEC happens before any register writes in HC after the wake_up() ?
Quoted 37 lines
* There's a direct call of
sdhci_msm_handle_pwr_irq()from probe, and it has a big fatmb(). I have a hard time believing that matters too because I'd bet "platform_get_irq_byname" has at least one lock in it.IMHO these "
_relaxed" calls are just not worth it except in _very_ targeted usage.Quoted 28 lines
drivers/mmc/host/sdhci-msm.c +13
-1+ }else if (((io_level &REQ_IO_LOW ) || + (msm_host->caps_0 &CORE_1_8V_SUPPORT )) && + !(config &CORE_IO_PAD_PWR_SWITCH )) { + config |=CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD bit is in different memory space */ +mb (); + } + } +if (pwr_state) msm_host->curr_pwr_state = pwr_state;if (io_level)@@ -1322,7 +1364,8 @@ 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; -u32 caps =0 ; +u32 caps =0 , config; +struct sdhci_host *host =mmc_priv (mmc);if (!IS_ERR (mmc->supply.vqmmc)) {if (regulator_is_supported_voltage (supply,1700000 ,1950000 ))@@ -1335,6 +1378,23 @@ static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) mmc_hostname(mmc), __func__); }
Quoted 16 lines
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1391
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +11
-0+if (caps) { +/* +* Set the PAD_PWR_SWITCH_EN bit so that the PAD_PWR_SWITCH +* bit can be used as required later on. +*/ +u32 io_level = msm_host->curr_io_level; + + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + config |=CORE_IO_PAD_PWR_SWITCH_EN ; + +if ((io_level &REQ_IO_HIGH ) && (caps &CORE_3_0V_SUPPORT ))Slight nit that there's a tab character after "caps &". Please replace it with a space.
Will do
Quoted 28 lines
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1394
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ config &= ~CORE_IO_PAD_PWR_SWITCH ; + else if ((io_level & REQ_IO_LOW) || (caps & CORE_1_8V_SUPPORT)) + config |= CORE_IO_PAD_PWR_SWITCH;Are you sure that's right? In English:
* If we requested high and we support high then set to high.
* else if we requested low__or__we support low then set low.Things that are weird above that:
* If we request low but don't support low, switch to low anyway.
* If we request high but only support low, switch to low anyway.
If nothing else seems like this would deserve a comment, but I'd be curious of the justification for that logic.
Also: seems like this is duplicated code between here and
sdhci_msm_handle_pwr_irq(). Does it even need to be here? Can't you just move the call tosdhci_msm_set_regulator_caps()before the call tosdhci_msm_handle_pwr_irq()in probe? Then just let that first call to tosdhci_msm_handle_pwr_irq()do this work? Insdhci_msm_handle_pwr_irq()you can always just "OR" inCORE_IO_PAD_PWR_SWITCH_EN
Quoted 2 lines
-Doug
--
If we don't support 1.8V, then the only time io_low will happen is during BUS_OFF. For BUS_OFF, enabling IO_PAD_PWR_SWITCH is ok.
This logic is same as what is there in pwr_irq. Added the same stuff here because by the time we get regulator info from mmc layer, some power irqs would have already come and gone.
Quoted 4 lines
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 2/2] mmc: sdhci-msm: support voltage pad switching
Hi,
Quoted 115 lines · Vijay Viswanath
Quoted 100 lines · Doug Anderson
Hi,
Quoted 59 lines · Vijay Viswanath
From: Krishna Konda <[email protected]>
The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
SDHC core on msm platforms should have
IO_PAD_PWR_SWITCHbit set/unset based on actual voltage used for IO lines. So when power irq is triggered for io high or io low, the driver should check the voltages supported and set the pad accordingly.Signed-off-by Krishna Konda <[email protected]>Signed-off-by Venkat Gopalakrishnan <[email protected]>Signed-off-by Vijay Viswanath <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +4
-1@@ -78,12 +78,15 @@#define CORE_HC_MCLK_SEL_DFLT (2 << 8)#define CORE_HC_MCLK_SEL_HS400 (3 << 8)#define CORE_HC_MCLK_SEL_MASK (3 << 8) +#define CORE_IO_PAD_PWR_SWITCH_EN (1 << 15) +#define CORE_IO_PAD_PWR_SWITCH (1 << 16)#define CORE_HC_SELECT_IN_EN BIT(18)#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) +#define CORE_VOLT_SUPPORT (CORE_3_0V_SUPPORT | CORE_1_8V_SUPPORT)#define CORE_CSR_CDC_CTLR_CFG0 0x130#define CORE_SW_TRIG_FULL_CALIB BIT(16)@@ -1109,7 +1112,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) u32 irq_status, irq_ack = 0;int retry =10 ;u32 pwr_state =0 , io_level =0 ; - +u32 config; irq_status = readl_relaxed(msm_host->core_mem +CORE_PWRCTL_STATUS); irq_status &= INT_MASK;drivers/mmc/host/sdhci-msm.c +2
-0@@ -1166,6 +1169,45 @@ static void sdhci_msm_handle_pwr_irq(structsdhci_host *host, int irq) */ writel_relaxed (irq_ack, msm_host->core_mem +CORE_PWRCTL_CTL ); +/* +* If we don't have info regarding the voltage levels supported by
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1178
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+* regulators, don't change the IO PAD PWR SWITCH. +*/ +if (msm_host->caps_0 &CORE_VOLT_SUPPORT ) { +/* Ensure order between core_mem and hc_mem */ +mb ();Like in v2, I don't understand why you need a
mb()before the read fromCORE_VENDOR_SPEC. No reads or writes to thecore_memwill affect the value you're reading here, so you need no barrier.If you need a barrier before the _write_ to
CORE_VENDOR_SPECthen add it below. Then in the case where the config doesn't change you have no barriers.Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +2
-0+/* +* We should unset IO PAD PWR switch only if the register write
+ * can set IO lines high and the regulator also switches
to 3 V.drivers/mmc/host/sdhci-msm.c +2
-0+* Else, we should keep the IO PAD PWR switch set. +* This is applicable to certain targets where eMMC vccq supply
+ * is only 1.8V. In such targets, even duringREQ_IO_HIGH, thedrivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1189
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+* IO PAD PWR switch must be kept set to reflect actual +* regulator voltage. This way, during initialization of +* controllers with only 1.8V, we will set the IO PAD bit +* without waiting for a REQ_IO_LOW. +*/ For the above comment, what about just:
new_config= configQuoted 10 lines
if (msm_host->caps_0 == CORE_1_8V_SUPPORT) { new_config |= CORE_IO_PAD_PWR_SWITCH; } else if (msm_host->caps_0 == CORE_3_3V_SUPPORT) { new_config &= ~CORE_IO_PAD_PWR_SWITCH; } else if (msm_host->caps_0 & CORE_VOLT_SUPPORT) { if (io_level & REQ_IO_HIGH) new_config &= ~CORE_IO_PAD_PWR_SWITCH; else if (io_level & REQ_IO_LOW) new_config |= CORE_IO_PAD_PWR_SWITCH; }
Quoted 6 lines
This looks a big mess of if/else. Does the above implementation have better performance compared to having two if/else with bit operations inside ? The latter looks much cleaner and faster.
If regulator only supports 3V and we get a
io_lowfromBUS_OFF(REQ_IO_LOWshould never come if we don't support 1.8V), it is ok to set io pad.
Yeah, I think it's ugly no matter what. Personally I find the if/then/else easier to follow than the complicated conditions split across multiple lines. I seem to remember there was something that my version did differently than yours too (hence the "this might be more important if you get rid of the initial setting"), let's see if I can figure it out again.
Mine says:
- if it has exactly 1.8 or 3.3 support: set that.
- else if it supports both: set whatever is requested
- else (it support neither): do nothing
Yours says:
- if it supports high and requests high: set it high
- if it supports low and requests low: set it low
...so your code assumes that it was already set to the right thing if it only supports 1.8 or only support 3.3. Hence my comment about this being important if you get rid of the initial settings as I was suggesting.
Quoted 35 lines
Quoted 35 lines
if (config !=
new_config) {
...
}AKA: first check if it only supports one voltage and pick that one. Else if it supports both you can use the request. This might be more important if you get rid of the initial setting in
sdhci_msm_set_regulator_caps()as I'm suggesting.Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +9
-0+ config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + +if (((io_level &REQ_IO_HIGH ) && (msm_host->caps_0 & +CORE_3_0V_SUPPORT )) && + (config &CORE_IO_PAD_PWR_SWITCH )) { + config &= ~CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD register is in different memory space */
+mb();Wow, for a driver that tries so hard to use "relaxed" versions of writes to avoid barriers you sure end up needing to sprinkle a lot of these around "just in case". :( ...this one seems extra fishy because:
* There are no more accesses after this one in this function.
* If you're worried about something that happens outside of the context of the IRQ needing this
wb()then that's a silly concern. Presumably if they were doing anything that could race with you they'd have a lock and locking routines are implicit barriers.
Quoted 6 lines
Quoted 4 lines
* In the context of the IRQ itself the next call is
sdhci_msm_complete_pwr_irq_wait(), which eventually callswake_up. This has a locking primitive and thus an implicit barrier.Sorry, I didn't get implicit barrier in locking primitive.
My understanding is locking primitives (spin locks and mutexes) always act as a barrier. This is why most of the time you shouldn't need to think about barriers. As long as you've got things protected by spinlocks and/or mutexes then the ordering will work out right.
Specifically, see
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/memory-barriers.txt>
Quoted 2 lines
Some of the other functions in the linux kernel imply memory barriers, amongst which are locking and scheduling functions.
Said another way, if you have two routines:
routine_a() {
lock(driver_global_lock);
writel_relaxed(region_a);
unlock(driver_global_lock);
}
routine_b() {
lock(driver_global_lock);
writel_relaxed(region_b);
unlock(driver_global_lock);
}
If you call:
routine_a();
routine_b();
You can be assured that the write to region_a will take place before the write to region_b. As I understand it, no extra barriers are required. This is why most people don't need to worry about barriers--they just use locks. The only argument you have for barriers at all in this file is that you have two different memory regions that you've iomapped.
...basically if something you do causes both a lock() and unlock() you can be sure that your memory has been written by the time the unlock() finishes.
Quoted 11 lines
In
mmc_set_signal_voltageswitch:
1. Send cmd 11.
2. Switch 1.8V inSDHCI_HOST_CONTROL2
3. Wait forpwr_irqwake_up().
4.pwr_irqcontext comes up & does register read/writes in core mem.
Updates IO PAD in HC mem.
5.pwr_irqcallswake_up.
6.mmc_set_signal_voltage_switchcontext does further register
read/writes which expectsIO_PADchange withinpwr_irqcontext is complete before step 6.
Quoted 2 lines
Can
wake_up()ensure that any update toCORE_VENDOR_SPEChappens before any register writes in HC after thewake_up()?
I guess I will leave it to the wisdom of others if wake_up is guaranteed to always acquire and release a lock. Today it does and that means that any memory writes that happen before the call to wake_up() will finish by the time wake_up() finishes.
Of course, my overall advice remains to get rid of the "relaxed" usage everywhere except in places were you truly believe it's performance critical.
Quoted 99 lines
Quoted 50 lines
* There's a direct call of
sdhci_msm_handle_pwr_irq()from probe, and it has a big fatmb(). I have a hard time believing that matters too because I'd bet "platform_get_irq_byname" has at least one lock in it.IMHO these "
_relaxed" calls are just not worth it except in _very_ targeted usage.Quoted 41 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ }else if (((io_level &REQ_IO_LOW ) || + (msm_host->caps_0 & CORE_1_8V_SUPPORT))&&
drivers/mmc/host/sdhci-msm.c +11
-1+ !(config & CORE_IO_PAD_PWR_SWITCH)) { + config |=CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD bit is in different memory space */ +mb (); + } + } +if (pwr_state) msm_host->curr_pwr_state = pwr_state;if (io_level)@@ -1322,7 +1364,8 @@ 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; -u32 caps =0 ; +u32 caps =0 , config; +struct sdhci_host *host =mmc_priv (mmc);if (!IS_ERR (mmc->supply.vqmmc)) {if (regulator_is_supported_voltage(supply, 1700000,1950000))
drivers/mmc/host/sdhci-msm.c +3
-0@@ -1335,6 +1378,23 @@ static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) mmc_hostname(mmc), __func__); } +if (caps) { +/* +* Set the PAD_PWR_SWITCH_EN bit so that the
PAD_PWR_SWITCHdrivers/mmc/host/sdhci-msm.c +5
-0+* bit can be used as required later on. +*/ +u32 io_level = msm_host->curr_io_level; + + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC );Quoted 10 lines
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ config |=CORE_IO_PAD_PWR_SWITCH_EN ; + + if ((io_level & REQ_IO_HIGH) && (caps &
CORE_3_0V_SUPPORT))Slight nit that there's a tab character after "caps &". Please replace it with a space.
Will do
Quoted 31 lines
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ config &= ~CORE_IO_PAD_PWR_SWITCH ; + else if ((io_level & REQ_IO_LOW) || (caps &
CORE_1_8V_SUPPORT))
+ config |=CORE_IO_PAD_PWR_SWITCH;Are you sure that's right? In English:
* If we requested high and we support high then set to high.
* else if we requested low__or__we support low then set low.Things that are weird above that:
* If we request low but don't support low, switch to low anyway.
* If we request high but only support low, switch to low anyway.
If nothing else seems like this would deserve a comment, but I'd be curious of the justification for that logic.
Also: seems like this is duplicated code between here and
sdhci_msm_handle_pwr_irq(). Does it even need to be here? Can't you just move the call tosdhci_msm_set_regulator_caps()before the call tosdhci_msm_handle_pwr_irq()in probe? Then just let that first call to tosdhci_msm_handle_pwr_irq()do this work? Insdhci_msm_handle_pwr_irq()you can always just "OR" inCORE_IO_PAD_PWR_SWITCH_ENQuoted 2 lines
-Doug
--
Quoted 6 lines
If we don't support 1.8V, then the only time
io_lowwill happen is duringBUS_OFF. ForBUS_OFF, enablingIO_PAD_PWR_SWITCHis ok.This logic is same as what is there in
pwr_irq. Added the same stuff here because by the time we get regulator info from mmc layer, some power irqs would have already come and gone.
Did you try my suggestion of: "Can't you just move the call to sdhci_msm_set_regulator_caps() before the call to sdhci_msm_handle_pwr_irq() in probe? Then just let that first call to sdhci_msm_handle_pwr_irq() do this work?" or did I miss something there? Specifically that will make the caps_0 get set early. ...now when sdhci_msm_probe() manually calls sdhci_msm_handle_pwr_irq() caps_0 will be set and it can do all this work there. That should avoid this duplicated code.
-Doug
Re: [PATCH V4 2/2] mmc: sdhci-msm: support voltage pad switching
Quoted 132 lines · Doug Anderson
Hi,
Quoted 115 lines · Vijay Viswanath
Quoted 105 lines · Doug Anderson
Hi,
Quoted 59 lines · Vijay Viswanath
From: Krishna Konda <[email protected]>
The PADs for SD card are dual-voltage that support 3v/1.8v. Those PADs have a control signal (io_pad_pwr_switch/mode18 ) that indicates whether the PAD works in 3v or 1.8v.
SDHC core on msm platforms should have
IO_PAD_PWR_SWITCHbit set/unset based on actual voltage used for IO lines. So when power irq is triggered for io high or io low, the driver should check the voltages supported and set the pad accordingly.Signed-off-by Krishna Konda <[email protected]>Signed-off-by Venkat Gopalakrishnan <[email protected]>Signed-off-by Vijay Viswanath <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +4
-1@@ -78,12 +78,15 @@#define CORE_HC_MCLK_SEL_DFLT (2 << 8)#define CORE_HC_MCLK_SEL_HS400 (3 << 8)#define CORE_HC_MCLK_SEL_MASK (3 << 8) +#define CORE_IO_PAD_PWR_SWITCH_EN (1 << 15) +#define CORE_IO_PAD_PWR_SWITCH (1 << 16)#define CORE_HC_SELECT_IN_EN BIT(18)#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) +#define CORE_VOLT_SUPPORT (CORE_3_0V_SUPPORT | CORE_1_8V_SUPPORT)#define CORE_CSR_CDC_CTLR_CFG0 0x130#define CORE_SW_TRIG_FULL_CALIB BIT(16)@@ -1109,7 +1112,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) u32 irq_status, irq_ack = 0;int retry =10 ;u32 pwr_state =0 , io_level =0 ; - +u32 config; irq_status = readl_relaxed(msm_host->core_mem +CORE_PWRCTL_STATUS); irq_status &= INT_MASK;drivers/mmc/host/sdhci-msm.c +2
-0@@ -1166,6 +1169,45 @@ static void sdhci_msm_handle_pwr_irq(structsdhci_host *host, int irq) */ writel_relaxed (irq_ack, msm_host->core_mem +CORE_PWRCTL_CTL ); +/* +* If we don't have info regarding the voltage levels supported by
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1178
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+* regulators, don't change the IO PAD PWR SWITCH. +*/ +if (msm_host->caps_0 &CORE_VOLT_SUPPORT ) { +/* Ensure order between core_mem and hc_mem */ +mb ();Like in v2, I don't understand why you need a
mb()before the read fromCORE_VENDOR_SPEC. No reads or writes to thecore_memwill affect the value you're reading here, so you need no barrier.If you need a barrier before the _write_ to
CORE_VENDOR_SPECthen add it below. Then in the case where the config doesn't change you have no barriers.Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +2
-0+/* +* We should unset IO PAD PWR switch only if the register write
+ * can set IO lines high and the regulator also switches
to 3 V.drivers/mmc/host/sdhci-msm.c +2
-0+* Else, we should keep the IO PAD PWR switch set. +* This is applicable to certain targets where eMMC vccq supply
+ * is only 1.8V. In such targets, even duringREQ_IO_HIGH, thedrivers/mmc/host/sdhci-msm.c static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq) · line 1189
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+* IO PAD PWR switch must be kept set to reflect actual +* regulator voltage. This way, during initialization of +* controllers with only 1.8V, we will set the IO PAD bit +* without waiting for a REQ_IO_LOW. +*/ For the above comment, what about just:
new_config= configif (msm_host->caps_0 == CORE_1_8V_SUPPORT) { new_config |= CORE_IO_PAD_PWR_SWITCH; } else if (msm_host->caps_0 == CORE_3_3V_SUPPORT) { new_config &= ~CORE_IO_PAD_PWR_SWITCH; } else if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {Quoted 5 lines
if (
io_level&REQ_IO_HIGH)
new_config&= ~CORE_IO_PAD_PWR_SWITCH;
else if (io_level&REQ_IO_LOW)
new_config|=CORE_IO_PAD_PWR_SWITCH;
}Quoted 6 lines
This looks a big mess of if/else. Does the above implementation have better performance compared to having two if/else with bit operations inside ? The latter looks much cleaner and faster.
If regulator only supports 3V and we get a
io_lowfromBUS_OFF(REQ_IO_LOWshould never come if we don't support 1.8V), it is ok to set io pad.Yeah, I think it's ugly no matter what. Personally I find the if/then/else easier to follow than the complicated conditions split across multiple lines. I seem to remember there was something that my version did differently than yours too (hence the "this might be more important if you get rid of the initial setting"), let's see if I can figure it out again.
Quoted 5 lines
Mine says:
- if it has exactly 1.8 or 3.3 support: set that.
- else if it supports both: set whatever is requested
- else (it support neither): do nothing
Similar logic I am also using, with 1 exception:
if REQ_IO_LOW and only 3.3V support, set it for 1.8V .
Quoted 4 lines
Yours says:
- if it supports high and requests high: set it high
- if it supports low and requests low: set it low
2 addition:
- if it supports high only and requests low: set it low (this case comes when BUS_OFF. At that time, its ok to set it low)
- if it supports low only and requests high: set it low
Quoted 6 lines
...so your code assumes that it was already set to the right thing if it only supports 1.8 or only support 3.3. Hence my comment about this being important if you get rid of the initial settings as I was suggesting.
It doesn't assume about previous setting. Just checks what the io_pad_switch value and and what it should be.
Both of out logic works fine. But I feel the one in the patch is neater and less lines. Maybe because I am used to it.
Quoted 97 lines
Quoted 35 lines
Quoted 35 lines
if (config != new_config) { ... }AKA: first check if it only supports one voltage and pick that one. Else if it supports both you can use the request. This might be more important if you get rid of the initial setting in
sdhci_msm_set_regulator_caps()as I'm suggesting.Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +9
-0+ config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC ); + +if (((io_level &REQ_IO_HIGH ) && (msm_host->caps_0 & +CORE_3_0V_SUPPORT )) && + (config &CORE_IO_PAD_PWR_SWITCH )) { + config &= ~CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD register is in different memory space */
+mb();Wow, for a driver that tries so hard to use "relaxed" versions of writes to avoid barriers you sure end up needing to sprinkle a lot of these around "just in case". :( ...this one seems extra fishy because:
* There are no more accesses after this one in this function.
* If you're worried about something that happens outside of the context of the IRQ needing this
wb()then that's a silly concern. Presumably if they were doing anything that could race with you they'd have a lock and locking routines are implicit barriers.Quoted 6 lines
Quoted 4 lines
* In the context of the IRQ itself the next call is
sdhci_msm_complete_pwr_irq_wait(), which eventually callswake_up. This has a locking primitive and thus an implicit barrier.Sorry, I didn't get implicit barrier in locking primitive.
My understanding is locking primitives (spin locks and mutexes) always act as a barrier. This is why most of the time you shouldn't need to think about barriers. As long as you've got things protected by spinlocks and/or mutexes then the ordering will work out right.
Specifically, see
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/memory-barriers.txt>Quoted 2 lines
Some of the other functions in the linux kernel imply memory barriers, amongst which are locking and scheduling functions.
Said another way, if you have two routines:
routine_a() { lock(driver_global_lock); writel_relaxed(region_a); unlock(driver_global_lock); }routine_b() { lock(driver_global_lock); writel_relaxed(region_b); unlock(driver_global_lock); }If you call:
routine_a(); routine_b();You can be assured that the write to
region_awill take place before the write toregion_b. As I understand it, no extra barriers are required. This is why most people don't need to worry about barriers--they just use locks. The only argument you have for barriers at all in this file is that you have two different memory regions that you've iomapped....basically if something you do causes both a
lock()andunlock()you can be sure that your memory has been written by the time theunlock()finishes.Quoted 11 lines
In
mmc_set_signal_voltageswitch:
1. Send cmd 11.
2. Switch 1.8V inSDHCI_HOST_CONTROL2
3. Wait forpwr_irqwake_up().
4.pwr_irqcontext comes up & does register read/writes in core mem.
Updates IO PAD in HC mem.
5.pwr_irqcallswake_up.
6.mmc_set_signal_voltage_switchcontext does further register
read/writes which expectsIO_PADchange withinpwr_irqcontext is complete before step 6.Quoted 2 lines
Can
wake_up()ensure that any update toCORE_VENDOR_SPEChappens before any register writes in HC after thewake_up()?
Quoted 10 lines
I guess I will leave it to the wisdom of others if
wake_upis guaranteed to always acquire and release a lock. Today it does and that means that any memory writes that happen before the call towake_up()will finish by the timewake_up()finishes.Of course, my overall advice remains to get rid of the "relaxed" usage everywhere except in places were you truly believe it's performance critical.
From the memory barrier link:
A write memory barrier is implied by wake_up() and co. if and only if they wake something up. The barrier occurs before the task state is cleared, and so sits between the STORE to indicate the event and the STORE to set TASK_RUNNING:
_________________
So, as you said, an mb() is not required after the write to IO PAD in HC mem. Will remove it. If the irq thread is not waking anything up, that means there is no state change.
Quoted 102 lines
Quoted 102 lines
Quoted 50 lines
* There's a direct call of
sdhci_msm_handle_pwr_irq()from probe, and it has a big fatmb(). I have a hard time believing that matters too because I'd bet "platform_get_irq_byname" has at least one lock in it.IMHO these "
_relaxed" calls are just not worth it except in _very_ targeted usage.Quoted 41 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ }else if (((io_level &REQ_IO_LOW ) || + (msm_host->caps_0 & CORE_1_8V_SUPPORT))&&
drivers/mmc/host/sdhci-msm.c +11
-1+ !(config & CORE_IO_PAD_PWR_SWITCH)) { + config |=CORE_IO_PAD_PWR_SWITCH ; +writel_relaxed (config, + host->ioaddr +CORE_VENDOR_SPEC ); +/* IO PAD bit is in different memory space */ +mb (); + } + } +if (pwr_state) msm_host->curr_pwr_state = pwr_state;if (io_level)@@ -1322,7 +1364,8 @@ 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; -u32 caps =0 ; +u32 caps =0 , config; +struct sdhci_host *host =mmc_priv (mmc);if (!IS_ERR (mmc->supply.vqmmc)) {if (regulator_is_supported_voltage(supply, 1700000,1950000))
drivers/mmc/host/sdhci-msm.c +3
-0@@ -1335,6 +1378,23 @@ static int sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) mmc_hostname(mmc), __func__); } +if (caps) { +/* +* Set the PAD_PWR_SWITCH_EN bit so that the
PAD_PWR_SWITCHdrivers/mmc/host/sdhci-msm.c +5
-0+* bit can be used as required later on. +*/ +u32 io_level = msm_host->curr_io_level; + + config =readl_relaxed (host->ioaddr +CORE_VENDOR_SPEC );Quoted 10 lines
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ config |=CORE_IO_PAD_PWR_SWITCH_EN ; + + if ((io_level & REQ_IO_HIGH) && (caps &
CORE_3_0V_SUPPORT))Slight nit that there's a tab character after "caps &". Please replace it with a space.
Will do
Quoted 31 lines
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ config &= ~CORE_IO_PAD_PWR_SWITCH ; + else if ((io_level & REQ_IO_LOW) || (caps &
CORE_1_8V_SUPPORT))
+ config |=CORE_IO_PAD_PWR_SWITCH;Are you sure that's right? In English:
* If we requested high and we support high then set to high.
* else if we requested low__or__we support low then set low.Things that are weird above that:
* If we request low but don't support low, switch to low anyway.
* If we request high but only support low, switch to low anyway.
If nothing else seems like this would deserve a comment, but I'd be curious of the justification for that logic.
Also: seems like this is duplicated code between here and
sdhci_msm_handle_pwr_irq(). Does it even need to be here? Can't you just move the call tosdhci_msm_set_regulator_caps()before the call tosdhci_msm_handle_pwr_irq()in probe? Then just let that first call to tosdhci_msm_handle_pwr_irq()do this work? Insdhci_msm_handle_pwr_irq()you can always just "OR" inCORE_IO_PAD_PWR_SWITCH_ENQuoted 2 lines
-Doug
--If we don't support 1.8V, then the only time
io_lowwill happen is duringBUS_OFF. ForBUS_OFF, enablingIO_PAD_PWR_SWITCHis ok.
Quoted 7 lines
Quoted 3 lines
This logic is same as what is there in
pwr_irq. Added the same stuff here because by the time we get regulator info from mmc layer, some power irqs would have already come and gone.Did you try my suggestion of: "Can't you just move the call to
sdhci_msm_set_regulator_caps()before the call tosdhci_msm_handle_pwr_irq()in probe? Then just let that first call to
At this point, the regulator information wouldn't be available in mmc_host. We are reading regulator information from mmc layer which comes after the first power irq check. So we need to call sdhci_msm_set_regulator_caps() after sdhci_add_host() is complete, so that we don't have to wait for the next power_irq to come to complete the io pad settings.
Quoted 12 lines
sdhci_msm_handle_pwr_irq()do this work?" or did I miss something there? Specifically that will make thecaps_0get set early. ...now whensdhci_msm_probe()manually callssdhci_msm_handle_pwr_irq()caps_0will be set and it can do all this work there. That should avoid this duplicated code.-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html