[PATCH 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[PATCH 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus on/off
require sdhc controller to be ready before progressing further. Once a
register write which affects IO lines is done, the driver should wait for
power irq from controller. Once the irq comes, the driver should acknowledge
the irq by writing to power control register. If the acknowledgement is not
given to controller, the controller may not complete the corresponding
register write action and this can mess up the controller if drivers proceeds
without power irq completing. 

Sahitya Tummala (2):
  mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
  mmc: sdhci-msm: Add support to wait for power irq

Subhash Jadavani (1):
  mmc: sdhci-msm: fix issue with power irq

Vijay Viswanath (2):
  mmc: sdhci-msm: Add ops to do sdhc register write
  defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

 arch/arm64/configs/defconfig |   1 +
 drivers/mmc/host/sdhci-msm.c | 233 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 229 insertions(+), 5 deletions(-)

-- 
 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 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus on/off
require sdhc controller to be ready before progressing further. Once a
register write which affects IO lines is done, the driver should wait for
power irq from controller. Once the irq comes, the driver should acknowledge
the irq by writing to power control register. If the acknowledgement is not
given to controller, the controller may not complete the corresponding
register write action and this can mess up the controller if drivers proceeds
without power irq completing. 

Sahitya Tummala (2):
  mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
  mmc: sdhci-msm: Add support to wait for power irq

Subhash Jadavani (1):
  mmc: sdhci-msm: fix issue with power irq

Vijay Viswanath (2):
  mmc: sdhci-msm: Add ops to do sdhc register write
  defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

 arch/arm64/configs/defconfig |   1 +
 drivers/mmc/host/sdhci-msm.c | 233 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 229 insertions(+), 5 deletions(-)

-- 
 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 1/5] mmc: sdhci-msm: fix issue with power irq

[Vijay Viswanath]

From: Subhash Jadavani <subhashj@codeaurora.org>

SDCC controller reset (SW_RST) during probe may trigger power irq if
previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
enable the power irq interrupt in GIC (by registering the interrupt
handler), we need to ensure that any pending power irq interrupt status
is acknowledged otherwise power irq interrupt handler would be fired
prematurely.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 9d601dc..0957199 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	u16 host_version, core_minor;
 	u32 core_version, config;
 	u8 core_major;
+	u32 irq_status, irq_ctl;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
@@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 			       CORE_VENDOR_SPEC_CAPABILITIES0);
 	}
 
+	/*
+	 * Power on reset state may trigger power irq if previous status of
+	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
+	 * interrupt in GIC, any pending power irq interrupt should be
+	 * acknowledged. Otherwise power irq interrupt handler would be
+	 * fired prematurely.
+	 */
+
+	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
+	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
+	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
+	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
+		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
+	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);
+	/*
+	 * Ensure that above writes are propogated before interrupt enablement
+	 * in GIC.
+	 */
+	mb();
+
 	/* Setup IRQ for handling power/voltage tasks with PMIC */
 	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
 	if (msm_host->pwr_irq < 0) {
@@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	/* Enable pwr irq interrupts */
+	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
+
 	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
 					sdhci_msm_pwr_irq, IRQF_ONESHOT,
 					dev_name(&pdev->dev), host);
-- 
 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 1/5] mmc: sdhci-msm: fix issue with power irq

[Vijay Viswanath]

From: Subhash Jadavani <subhashj@codeaurora.org>

SDCC controller reset (SW_RST) during probe may trigger power irq if
previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
enable the power irq interrupt in GIC (by registering the interrupt
handler), we need to ensure that any pending power irq interrupt status
is acknowledged otherwise power irq interrupt handler would be fired
prematurely.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 9d601dc..0957199 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	u16 host_version, core_minor;
 	u32 core_version, config;
 	u8 core_major;
+	u32 irq_status, irq_ctl;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
@@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 			       CORE_VENDOR_SPEC_CAPABILITIES0);
 	}
 
+	/*
+	 * Power on reset state may trigger power irq if previous status of
+	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
+	 * interrupt in GIC, any pending power irq interrupt should be
+	 * acknowledged. Otherwise power irq interrupt handler would be
+	 * fired prematurely.
+	 */
+
+	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
+	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
+	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
+	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
+		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
+	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);
+	/*
+	 * Ensure that above writes are propogated before interrupt enablement
+	 * in GIC.
+	 */
+	mb();
+
 	/* Setup IRQ for handling power/voltage tasks with PMIC */
 	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
 	if (msm_host->pwr_irq < 0) {
@@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	/* Enable pwr irq interrupts */
+	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
+
 	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
 					sdhci_msm_pwr_irq, IRQF_ONESHOT,
 					dev_name(&pdev->dev), host);
-- 
 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 1/5] mmc: sdhci-msm: fix issue with power irq

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> SDCC controller reset (SW_RST) during probe may trigger power irq if
> previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
> enable the power irq interrupt in GIC (by registering the interrupt
> handler), we need to ensure that any pending power irq interrupt status
> is acknowledged otherwise power irq interrupt handler would be fired
> prematurely.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 9d601dc..0957199 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	u16 host_version, core_minor;
>  	u32 core_version, config;
>  	u8 core_major;
> +	u32 irq_status, irq_ctl;
>  
>  	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>  	if (IS_ERR(host))
> @@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  			       CORE_VENDOR_SPEC_CAPABILITIES0);
>  	}
>  
> +	/*
> +	 * Power on reset state may trigger power irq if previous status of
> +	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
> +	 * interrupt in GIC, any pending power irq interrupt should be
> +	 * acknowledged. Otherwise power irq interrupt handler would be
> +	 * fired prematurely.
> +	 */
> +
> +	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> +	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
> +	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> +	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> +	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> +		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> +	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);

This looks a lot like sdhci_msm_voltage_switch().  Can clearing the
interrupt be a common function?

> +	/*
> +	 * Ensure that above writes are propogated before interrupt enablement
> +	 * in GIC.
> +	 */
> +	mb();
> +
>  	/* Setup IRQ for handling power/voltage tasks with PMIC */
>  	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>  	if (msm_host->pwr_irq < 0) {
> @@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	/* Enable pwr irq interrupts */
> +	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>  					sdhci_msm_pwr_irq, IRQF_ONESHOT,
>  					dev_name(&pdev->dev), host);
> 

[PATCH 1/5] mmc: sdhci-msm: fix issue with power irq

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> SDCC controller reset (SW_RST) during probe may trigger power irq if
> previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
> enable the power irq interrupt in GIC (by registering the interrupt
> handler), we need to ensure that any pending power irq interrupt status
> is acknowledged otherwise power irq interrupt handler would be fired
> prematurely.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 9d601dc..0957199 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	u16 host_version, core_minor;
>  	u32 core_version, config;
>  	u8 core_major;
> +	u32 irq_status, irq_ctl;
>  
>  	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>  	if (IS_ERR(host))
> @@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  			       CORE_VENDOR_SPEC_CAPABILITIES0);
>  	}
>  
> +	/*
> +	 * Power on reset state may trigger power irq if previous status of
> +	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
> +	 * interrupt in GIC, any pending power irq interrupt should be
> +	 * acknowledged. Otherwise power irq interrupt handler would be
> +	 * fired prematurely.
> +	 */
> +
> +	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> +	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
> +	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> +	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> +	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> +		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> +	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);

This looks a lot like sdhci_msm_voltage_switch().  Can clearing the
interrupt be a common function?

> +	/*
> +	 * Ensure that above writes are propogated before interrupt enablement
> +	 * in GIC.
> +	 */
> +	mb();
> +
>  	/* Setup IRQ for handling power/voltage tasks with PMIC */
>  	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>  	if (msm_host->pwr_irq < 0) {
> @@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	/* Enable pwr irq interrupts */
> +	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>  					sdhci_msm_pwr_irq, IRQF_ONESHOT,
>  					dev_name(&pdev->dev), host);
> 

Re: [PATCH 1/5] mmc: sdhci-msm: fix issue with power irq

[Vijay Viswanath]

On 8/24/2017 1:10 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>>
>> SDCC controller reset (SW_RST) during probe may trigger power irq if
>> previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
>> enable the power irq interrupt in GIC (by registering the interrupt
>> handler), we need to ensure that any pending power irq interrupt status
>> is acknowledged otherwise power irq interrupt handler would be fired
>> prematurely.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 9d601dc..0957199 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   	u16 host_version, core_minor;
>>   	u32 core_version, config;
>>   	u8 core_major;
>> +	u32 irq_status, irq_ctl;
>>   
>>   	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>>   	if (IS_ERR(host))
>> @@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   			       CORE_VENDOR_SPEC_CAPABILITIES0);
>>   	}
>>   
>> +	/*
>> +	 * Power on reset state may trigger power irq if previous status of
>> +	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
>> +	 * interrupt in GIC, any pending power irq interrupt should be
>> +	 * acknowledged. Otherwise power irq interrupt handler would be
>> +	 * fired prematurely.
>> +	 */
>> +
>> +	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>> +	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>> +	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
>> +	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
>> +	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
>> +		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
>> +	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);
> 
> This looks a lot like sdhci_msm_voltage_switch().  Can clearing the
> interrupt be a common function?
> 
This is better. Will change the patches to do it this way.
>> +	/*
>> +	 * Ensure that above writes are propogated before interrupt enablement
>> +	 * in GIC.
>> +	 */
>> +	mb();
>> +
>>   	/* Setup IRQ for handling power/voltage tasks with PMIC */
>>   	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>>   	if (msm_host->pwr_irq < 0) {
>> @@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   		goto clk_disable;
>>   	}
>>   
>> +	/* Enable pwr irq interrupts */
>> +	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>> +
>>   	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>>   					sdhci_msm_pwr_irq, IRQF_ONESHOT,
>>   					dev_name(&pdev->dev), host);
>>
> 

[PATCH 1/5] mmc: sdhci-msm: fix issue with power irq

[Vijay Viswanath]

On 8/24/2017 1:10 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>>
>> SDCC controller reset (SW_RST) during probe may trigger power irq if
>> previous status of PWRCTL was either BUS_ON or IO_HIGH_V. So before we
>> enable the power irq interrupt in GIC (by registering the interrupt
>> handler), we need to ensure that any pending power irq interrupt status
>> is acknowledged otherwise power irq interrupt handler would be fired
>> prematurely.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 9d601dc..0957199 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1128,6 +1128,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   	u16 host_version, core_minor;
>>   	u32 core_version, config;
>>   	u8 core_major;
>> +	u32 irq_status, irq_ctl;
>>   
>>   	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>>   	if (IS_ERR(host))
>> @@ -1250,6 +1251,28 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   			       CORE_VENDOR_SPEC_CAPABILITIES0);
>>   	}
>>   
>> +	/*
>> +	 * Power on reset state may trigger power irq if previous status of
>> +	 * PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
>> +	 * interrupt in GIC, any pending power irq interrupt should be
>> +	 * acknowledged. Otherwise power irq interrupt handler would be
>> +	 * fired prematurely.
>> +	 */
>> +
>> +	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>> +	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>> +	irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
>> +	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +		irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
>> +	if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
>> +		irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
>> +	writel_relaxed(irq_ctl,	msm_host->core_mem + CORE_PWRCTL_CTL);
> 
> This looks a lot like sdhci_msm_voltage_switch().  Can clearing the
> interrupt be a common function?
> 
This is better. Will change the patches to do it this way.
>> +	/*
>> +	 * Ensure that above writes are propogated before interrupt enablement
>> +	 * in GIC.
>> +	 */
>> +	mb();
>> +
>>   	/* Setup IRQ for handling power/voltage tasks with PMIC */
>>   	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>>   	if (msm_host->pwr_irq < 0) {
>> @@ -1259,6 +1282,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   		goto clk_disable;
>>   	}
>>   
>> +	/* Enable pwr irq interrupts */
>> +	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>> +
>>   	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>>   					sdhci_msm_pwr_irq, IRQF_ONESHOT,
>>   					dev_name(&pdev->dev), host);
>>
> 

[PATCH 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Vijay Viswanath]

From: Sahitya Tummala <stummala@codeaurora.org>

There is a rare scenario in HW, where the first clear pulse could
be lost when the actual reset and clear/read of status register
are happening at the same time. Fix this by retrying upto 10 times
to ensure the status register gets cleared. Otherwise, this will
lead to a spurious power IRQ which results in system instability.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 0957199..f3e0489 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
-static void sdhci_msm_voltage_switch(struct sdhci_host *host)
+static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
+			mmc_hostname(host->mmc),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
+}
+
+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);
 	u32 irq_status, irq_ack = 0;
+	int retry = 10;
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
 
 	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
 
+	/*
+	 * There is a rare HW scenario where the first clear pulse could be
+	 * lost when actual reset and clear/read of status register is
+	 * happening at a time. Hence, retry for at least 10 times to make
+	 * sure status register is cleared. Otherwise, this will result in
+	 * a spurious power IRQ resulting in system instability.
+	 */
+	while (irq_status & readl_relaxed(msm_host->core_mem +
+				CORE_PWRCTL_STATUS)) {
+		if (retry == 0) {
+			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
+					mmc_hostname(host->mmc), irq_status);
+			sdhci_msm_dump_pwr_ctrl_regs(host);
+			WARN_ON(1);
+		}
+		writel_relaxed(irq_status,
+				msm_host->core_mem + CORE_PWRCTL_CLEAR);
+		retry--;
+		udelay(10);
+	}
+
 	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
 	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
@@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
 	 * switches are handled by the sdhci core, so just report success.
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
+
+	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
+		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
+		irq_ack);
 }
 
 static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
 {
 	struct sdhci_host *host = (struct sdhci_host *)data;
 
-	sdhci_msm_voltage_switch(host);
+	sdhci_msm_handle_pwr_irq(host, irq);
 
 	return IRQ_HANDLED;
 }
@@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
-	.voltage_switch = sdhci_msm_voltage_switch,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
 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 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Vijay Viswanath]

From: Sahitya Tummala <stummala@codeaurora.org>

There is a rare scenario in HW, where the first clear pulse could
be lost when the actual reset and clear/read of status register
are happening at the same time. Fix this by retrying upto 10 times
to ensure the status register gets cleared. Otherwise, this will
lead to a spurious power IRQ which results in system instability.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 0957199..f3e0489 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
-static void sdhci_msm_voltage_switch(struct sdhci_host *host)
+static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
+			mmc_hostname(host->mmc),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
+			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
+}
+
+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);
 	u32 irq_status, irq_ack = 0;
+	int retry = 10;
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
 
 	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
 
+	/*
+	 * There is a rare HW scenario where the first clear pulse could be
+	 * lost when actual reset and clear/read of status register is
+	 * happening at a time. Hence, retry for at least 10 times to make
+	 * sure status register is cleared. Otherwise, this will result in
+	 * a spurious power IRQ resulting in system instability.
+	 */
+	while (irq_status & readl_relaxed(msm_host->core_mem +
+				CORE_PWRCTL_STATUS)) {
+		if (retry == 0) {
+			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
+					mmc_hostname(host->mmc), irq_status);
+			sdhci_msm_dump_pwr_ctrl_regs(host);
+			WARN_ON(1);
+		}
+		writel_relaxed(irq_status,
+				msm_host->core_mem + CORE_PWRCTL_CLEAR);
+		retry--;
+		udelay(10);
+	}
+
 	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
 	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
@@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
 	 * switches are handled by the sdhci core, so just report success.
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
+
+	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
+		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
+		irq_ack);
 }
 
 static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
 {
 	struct sdhci_host *host = (struct sdhci_host *)data;
 
-	sdhci_msm_voltage_switch(host);
+	sdhci_msm_handle_pwr_irq(host, irq);
 
 	return IRQ_HANDLED;
 }
@@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
-	.voltage_switch = sdhci_msm_voltage_switch,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
 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 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> There is a rare scenario in HW, where the first clear pulse could
> be lost when the actual reset and clear/read of status register
> are happening at the same time. Fix this by retrying upto 10 times
> to ensure the status register gets cleared. Otherwise, this will
> lead to a spurious power IRQ which results in system instability.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 0957199..f3e0489 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> -static void sdhci_msm_voltage_switch(struct sdhci_host *host)
> +static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
> +			mmc_hostname(host->mmc),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
> +}
> +
> +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);
>  	u32 irq_status, irq_ack = 0;
> +	int retry = 10;
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
>  
>  	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>  
> +	/*
> +	 * There is a rare HW scenario where the first clear pulse could be
> +	 * lost when actual reset and clear/read of status register is
> +	 * happening at a time. Hence, retry for at least 10 times to make
> +	 * sure status register is cleared. Otherwise, this will result in
> +	 * a spurious power IRQ resulting in system instability.
> +	 */
> +	while (irq_status & readl_relaxed(msm_host->core_mem +
> +				CORE_PWRCTL_STATUS)) {
> +		if (retry == 0) {
> +			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
> +					mmc_hostname(host->mmc), irq_status);
> +			sdhci_msm_dump_pwr_ctrl_regs(host);
> +			WARN_ON(1);

Is it your intention to loop forever here?

> +		}
> +		writel_relaxed(irq_status,
> +				msm_host->core_mem + CORE_PWRCTL_CLEAR);
> +		retry--;
> +		udelay(10);
> +	}
> +
>  	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>  	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> @@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>  	 * switches are handled by the sdhci core, so just report success.
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
> +
> +	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
> +		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
> +		irq_ack);
>  }
>  
>  static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>  {
>  	struct sdhci_host *host = (struct sdhci_host *)data;
>  
> -	sdhci_msm_voltage_switch(host);
> +	sdhci_msm_handle_pwr_irq(host, irq);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> -	.voltage_switch = sdhci_msm_voltage_switch,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> 

[PATCH 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> There is a rare scenario in HW, where the first clear pulse could
> be lost when the actual reset and clear/read of status register
> are happening at the same time. Fix this by retrying upto 10 times
> to ensure the status register gets cleared. Otherwise, this will
> lead to a spurious power IRQ which results in system instability.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 0957199..f3e0489 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> -static void sdhci_msm_voltage_switch(struct sdhci_host *host)
> +static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
> +			mmc_hostname(host->mmc),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
> +}
> +
> +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);
>  	u32 irq_status, irq_ack = 0;
> +	int retry = 10;
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
>  
>  	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>  
> +	/*
> +	 * There is a rare HW scenario where the first clear pulse could be
> +	 * lost when actual reset and clear/read of status register is
> +	 * happening at a time. Hence, retry for at least 10 times to make
> +	 * sure status register is cleared. Otherwise, this will result in
> +	 * a spurious power IRQ resulting in system instability.
> +	 */
> +	while (irq_status & readl_relaxed(msm_host->core_mem +
> +				CORE_PWRCTL_STATUS)) {
> +		if (retry == 0) {
> +			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
> +					mmc_hostname(host->mmc), irq_status);
> +			sdhci_msm_dump_pwr_ctrl_regs(host);
> +			WARN_ON(1);

Is it your intention to loop forever here?

> +		}
> +		writel_relaxed(irq_status,
> +				msm_host->core_mem + CORE_PWRCTL_CLEAR);
> +		retry--;
> +		udelay(10);
> +	}
> +
>  	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>  	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> @@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>  	 * switches are handled by the sdhci core, so just report success.
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
> +
> +	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
> +		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
> +		irq_ack);
>  }
>  
>  static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>  {
>  	struct sdhci_host *host = (struct sdhci_host *)data;
>  
> -	sdhci_msm_voltage_switch(host);
> +	sdhci_msm_handle_pwr_irq(host, irq);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> -	.voltage_switch = sdhci_msm_voltage_switch,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> 

Re: [PATCH 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Vijay Viswanath]

On 8/24/2017 1:12 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> There is a rare scenario in HW, where the first clear pulse could
>> be lost when the actual reset and clear/read of status register
>> are happening at the same time. Fix this by retrying upto 10 times
>> to ensure the status register gets cleared. Otherwise, this will
>> lead to a spurious power IRQ which results in system instability.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 0957199..f3e0489 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>   		sdhci_msm_hs400(host, &mmc->ios);
>>   }
>>   
>> -static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>> +static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
>> +			mmc_hostname(host->mmc),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
>> +}
>> +
>> +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);
>>   	u32 irq_status, irq_ack = 0;
>> +	int retry = 10;
>>   
>>   	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>>   	irq_status &= INT_MASK;
>>   
>>   	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>>   
>> +	/*
>> +	 * There is a rare HW scenario where the first clear pulse could be
>> +	 * lost when actual reset and clear/read of status register is
>> +	 * happening at a time. Hence, retry for at least 10 times to make
>> +	 * sure status register is cleared. Otherwise, this will result in
>> +	 * a spurious power IRQ resulting in system instability.
>> +	 */
>> +	while (irq_status & readl_relaxed(msm_host->core_mem +
>> +				CORE_PWRCTL_STATUS)) {
>> +		if (retry == 0) {
>> +			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
>> +					mmc_hostname(host->mmc), irq_status);
>> +			sdhci_msm_dump_pwr_ctrl_regs(host);
>> +			WARN_ON(1);
> 
> Is it your intention to loop forever here?
> 
A bad mistake from my side. Will add break here.
>> +		}
>> +		writel_relaxed(irq_status,
>> +				msm_host->core_mem + CORE_PWRCTL_CLEAR);
>> +		retry--;
>> +		udelay(10);
>> +	}
>> +
>>   	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>>   		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>>   	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
>> @@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>>   	 * switches are handled by the sdhci core, so just report success.
>>   	 */
>>   	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>> +
>> +	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>> +		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>> +		irq_ack);
>>   }
>>   
>>   static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>>   {
>>   	struct sdhci_host *host = (struct sdhci_host *)data;
>>   
>> -	sdhci_msm_voltage_switch(host);
>> +	sdhci_msm_handle_pwr_irq(host, irq);
>>   
>>   	return IRQ_HANDLED;
>>   }
>> @@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	.get_max_clock = sdhci_msm_get_max_clock,
>>   	.set_bus_width = sdhci_set_bus_width,
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>> -	.voltage_switch = sdhci_msm_voltage_switch,
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
> 
> --
> 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
> 

[PATCH 2/5] mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset

[Vijay Viswanath]

On 8/24/2017 1:12 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> There is a rare scenario in HW, where the first clear pulse could
>> be lost when the actual reset and clear/read of status register
>> are happening at the same time. Fix this by retrying upto 10 times
>> to ensure the status register gets cleared. Otherwise, this will
>> lead to a spurious power IRQ which results in system instability.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 0957199..f3e0489 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -995,17 +995,51 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>   		sdhci_msm_hs400(host, &mmc->ios);
>>   }
>>   
>> -static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>> +static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
>> +			mmc_hostname(host->mmc),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
>> +			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
>> +}
>> +
>> +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);
>>   	u32 irq_status, irq_ack = 0;
>> +	int retry = 10;
>>   
>>   	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>>   	irq_status &= INT_MASK;
>>   
>>   	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
>>   
>> +	/*
>> +	 * There is a rare HW scenario where the first clear pulse could be
>> +	 * lost when actual reset and clear/read of status register is
>> +	 * happening at a time. Hence, retry for at least 10 times to make
>> +	 * sure status register is cleared. Otherwise, this will result in
>> +	 * a spurious power IRQ resulting in system instability.
>> +	 */
>> +	while (irq_status & readl_relaxed(msm_host->core_mem +
>> +				CORE_PWRCTL_STATUS)) {
>> +		if (retry == 0) {
>> +			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
>> +					mmc_hostname(host->mmc), irq_status);
>> +			sdhci_msm_dump_pwr_ctrl_regs(host);
>> +			WARN_ON(1);
> 
> Is it your intention to loop forever here?
> 
A bad mistake from my side. Will add break here.
>> +		}
>> +		writel_relaxed(irq_status,
>> +				msm_host->core_mem + CORE_PWRCTL_CLEAR);
>> +		retry--;
>> +		udelay(10);
>> +	}
>> +
>>   	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>>   		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>>   	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
>> @@ -1017,13 +1051,17 @@ static void sdhci_msm_voltage_switch(struct sdhci_host *host)
>>   	 * switches are handled by the sdhci core, so just report success.
>>   	 */
>>   	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>> +
>> +	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>> +		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>> +		irq_ack);
>>   }
>>   
>>   static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>>   {
>>   	struct sdhci_host *host = (struct sdhci_host *)data;
>>   
>> -	sdhci_msm_voltage_switch(host);
>> +	sdhci_msm_handle_pwr_irq(host, irq);
>>   
>>   	return IRQ_HANDLED;
>>   }
>> @@ -1106,7 +1144,6 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	.get_max_clock = sdhci_msm_get_max_clock,
>>   	.set_bus_width = sdhci_set_bus_width,
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>> -	.voltage_switch = sdhci_msm_voltage_switch,
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Vijay Viswanath]

From: Sahitya Tummala <stummala@codeaurora.org>

Add support API which will check if power irq is expected to be
generated and wait for the power irq to come and complete if the irq is
expected.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index f3e0489..6d3b1fd 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -123,6 +123,10 @@
 #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
 
 #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
+
+/* Timeout value to avoid infinite waiting for pwr_irq */
+#define MSM_PWR_IRQ_TIMEOUT_MS 5000
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -138,6 +142,11 @@ struct sdhci_msm_host {
 	bool calibration_done;
 	u8 saved_tuning_phase;
 	bool use_cdclp533;
+	u32 curr_pwr_state;
+	u32 curr_io_level;
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+	struct completion pwr_irq_completion;
+#endif
 };
 
 static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
@@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	init_completion(&msm_host->pwr_irq_completion);
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	complete(&msm_host->pwr_irq_completion);
+}
+
+/*
+ * sdhci_msm_check_power_status API should be called when registers writes
+ * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
+ * To what state the register writes will change the IO lines should be passed
+ * as the argument req_type. This API will check whether the IO line's state
+ * is already the expected state and will wait for power irq only if
+ * power irq is expected to be trigerred based on the current IO line state
+ * and expected IO line state.
+ */
+static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	unsigned long flags;
+	bool done = false;
+
+	spin_lock_irqsave(&host->lock, flags);
+	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
+			mmc_hostname(host->mmc), __func__, req_type,
+			msm_host->curr_pwr_state, msm_host->curr_io_level);
+
+	/*
+	 * The IRQ for request type IO High/LOW will be generated when -
+	 * there is a state change in 1.8V enable bit (bit 3) of
+	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
+	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
+	 * to set it to 3.3V before card detection happens, the
+	 * IRQ doesn't get triggered as there is no state change in this bit.
+	 * The driver already handles this case by changing the IO voltage
+	 * level to high as part of controller power up sequence. Hence, check
+	 * for host->pwr to handle a case where IO voltage high request is
+	 * issued even before controller power up.
+	 */
+	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
+		pr_debug("%s: do not wait for power IRQ that never comes\n",
+				mmc_hostname(host->mmc));
+		spin_unlock_irqrestore(&host->lock, flags);
+		return;
+	}
+	if ((req_type & msm_host->curr_pwr_state) ||
+			(req_type & msm_host->curr_io_level))
+		done = true;
+	spin_unlock_irqrestore(&host->lock, flags);
+	/*
+	 * This is needed here to hanlde a case where IRQ gets
+	 * triggered even before this function is called so that
+	 * x->done counter of completion gets reset. Otherwise,
+	 * next call to wait_for_completion returns immediately
+	 * without actually waiting for the IRQ to be handled.
+	 */
+	if (done)
+		init_completion(&msm_host->pwr_irq_completion);
+	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
+				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
+		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
+				mmc_hostname(host->mmc), req_type);
+	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
+			__func__, req_type);
+}
+#else
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+#endif
+
 static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1013,6 +1106,9 @@ 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;
+	unsigned long flags;
+
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
@@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		udelay(10);
 	}
 
-	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+	/* 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;
+	}
+	if (irq_status & CORE_PWRCTL_BUS_OFF) {
+		pwr_state = REQ_BUS_OFF;
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
-	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
+	}
+	/* Handle IO LOW/HIGH */
+	if (irq_status & CORE_PWRCTL_IO_LOW) {
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
+	if (irq_status & CORE_PWRCTL_IO_HIGH) {
+		io_level = REQ_IO_HIGH;
+		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
 
 	/*
 	 * The driver has to acknowledge the interrupt, switch voltages and
@@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
 
+	spin_lock_irqsave(&host->lock, flags);
+	if (pwr_state)
+		msm_host->curr_pwr_state = pwr_state;
+	if (io_level)
+		msm_host->curr_io_level = io_level;
+	spin_unlock_irqrestore(&host->lock, flags);
+	sdhci_msm_complete_pwr_irq_completion(msm_host);
+
 	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
 		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
 		irq_ack);
@@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	sdhci_msm_init_pwr_irq_completion(msm_host);
 	/* Enable pwr irq interrupts */
 	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
 
-- 
 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 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Vijay Viswanath]

From: Sahitya Tummala <stummala@codeaurora.org>

Add support API which will check if power irq is expected to be
generated and wait for the power irq to come and complete if the irq is
expected.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 123 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index f3e0489..6d3b1fd 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -123,6 +123,10 @@
 #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
 
 #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
+
+/* Timeout value to avoid infinite waiting for pwr_irq */
+#define MSM_PWR_IRQ_TIMEOUT_MS 5000
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -138,6 +142,11 @@ struct sdhci_msm_host {
 	bool calibration_done;
 	u8 saved_tuning_phase;
 	bool use_cdclp533;
+	u32 curr_pwr_state;
+	u32 curr_io_level;
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+	struct completion pwr_irq_completion;
+#endif
 };
 
 static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
@@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	init_completion(&msm_host->pwr_irq_completion);
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+	complete(&msm_host->pwr_irq_completion);
+}
+
+/*
+ * sdhci_msm_check_power_status API should be called when registers writes
+ * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
+ * To what state the register writes will change the IO lines should be passed
+ * as the argument req_type. This API will check whether the IO line's state
+ * is already the expected state and will wait for power irq only if
+ * power irq is expected to be trigerred based on the current IO line state
+ * and expected IO line state.
+ */
+static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	unsigned long flags;
+	bool done = false;
+
+	spin_lock_irqsave(&host->lock, flags);
+	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
+			mmc_hostname(host->mmc), __func__, req_type,
+			msm_host->curr_pwr_state, msm_host->curr_io_level);
+
+	/*
+	 * The IRQ for request type IO High/LOW will be generated when -
+	 * there is a state change in 1.8V enable bit (bit 3) of
+	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
+	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
+	 * to set it to 3.3V before card detection happens, the
+	 * IRQ doesn't get triggered as there is no state change in this bit.
+	 * The driver already handles this case by changing the IO voltage
+	 * level to high as part of controller power up sequence. Hence, check
+	 * for host->pwr to handle a case where IO voltage high request is
+	 * issued even before controller power up.
+	 */
+	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
+		pr_debug("%s: do not wait for power IRQ that never comes\n",
+				mmc_hostname(host->mmc));
+		spin_unlock_irqrestore(&host->lock, flags);
+		return;
+	}
+	if ((req_type & msm_host->curr_pwr_state) ||
+			(req_type & msm_host->curr_io_level))
+		done = true;
+	spin_unlock_irqrestore(&host->lock, flags);
+	/*
+	 * This is needed here to hanlde a case where IRQ gets
+	 * triggered even before this function is called so that
+	 * x->done counter of completion gets reset. Otherwise,
+	 * next call to wait_for_completion returns immediately
+	 * without actually waiting for the IRQ to be handled.
+	 */
+	if (done)
+		init_completion(&msm_host->pwr_irq_completion);
+	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
+				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
+		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
+				mmc_hostname(host->mmc), req_type);
+	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
+			__func__, req_type);
+}
+#else
+static inline void sdhci_msm_init_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+
+static inline void sdhci_msm_complete_pwr_irq_completion(
+		struct sdhci_msm_host *msm_host)
+{
+}
+#endif
+
 static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1013,6 +1106,9 @@ 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;
+	unsigned long flags;
+
 
 	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
 	irq_status &= INT_MASK;
@@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		udelay(10);
 	}
 
-	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
+	/* 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;
+	}
+	if (irq_status & CORE_PWRCTL_BUS_OFF) {
+		pwr_state = REQ_BUS_OFF;
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
-	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
+	}
+	/* Handle IO LOW/HIGH */
+	if (irq_status & CORE_PWRCTL_IO_LOW) {
+		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
+	if (irq_status & CORE_PWRCTL_IO_HIGH) {
+		io_level = REQ_IO_HIGH;
+		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+	}
 
 	/*
 	 * The driver has to acknowledge the interrupt, switch voltages and
@@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	 */
 	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
 
+	spin_lock_irqsave(&host->lock, flags);
+	if (pwr_state)
+		msm_host->curr_pwr_state = pwr_state;
+	if (io_level)
+		msm_host->curr_io_level = io_level;
+	spin_unlock_irqrestore(&host->lock, flags);
+	sdhci_msm_complete_pwr_irq_completion(msm_host);
+
 	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
 		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
 		irq_ack);
@@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	sdhci_msm_init_pwr_irq_completion(msm_host);
 	/* Enable pwr irq interrupts */
 	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
 
-- 
 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 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> Add support API which will check if power irq is expected to be
> generated and wait for the power irq to come and complete if the irq is
> expected.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 123 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index f3e0489..6d3b1fd 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -123,6 +123,10 @@
>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>  
>  #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
> +
> +/* Timeout value to avoid infinite waiting for pwr_irq */
> +#define MSM_PWR_IRQ_TIMEOUT_MS 5000
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -138,6 +142,11 @@ struct sdhci_msm_host {
>  	bool calibration_done;
>  	u8 saved_tuning_phase;
>  	bool use_cdclp533;
> +	u32 curr_pwr_state;
> +	u32 curr_io_level;
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +	struct completion pwr_irq_completion;
> +#endif
>  };
>  
>  static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +static inline void sdhci_msm_init_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +	init_completion(&msm_host->pwr_irq_completion);
> +}
> +
> +static inline void sdhci_msm_complete_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +	complete(&msm_host->pwr_irq_completion);
> +}
> +
> +/*
> + * sdhci_msm_check_power_status API should be called when registers writes
> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> + * To what state the register writes will change the IO lines should be passed
> + * as the argument req_type. This API will check whether the IO line's state
> + * is already the expected state and will wait for power irq only if
> + * power irq is expected to be trigerred based on the current IO line state
> + * and expected IO line state.
> + */
> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	unsigned long flags;
> +	bool done = false;
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
> +			mmc_hostname(host->mmc), __func__, req_type,
> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
> +
> +	/*
> +	 * The IRQ for request type IO High/LOW will be generated when -
> +	 * there is a state change in 1.8V enable bit (bit 3) of
> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
> +	 * to set it to 3.3V before card detection happens, the
> +	 * IRQ doesn't get triggered as there is no state change in this bit.
> +	 * The driver already handles this case by changing the IO voltage
> +	 * level to high as part of controller power up sequence. Hence, check
> +	 * for host->pwr to handle a case where IO voltage high request is
> +	 * issued even before controller power up.
> +	 */
> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
> +		pr_debug("%s: do not wait for power IRQ that never comes\n",
> +				mmc_hostname(host->mmc));
> +		spin_unlock_irqrestore(&host->lock, flags);
> +		return;
> +	}
> +	if ((req_type & msm_host->curr_pwr_state) ||
> +			(req_type & msm_host->curr_io_level))
> +		done = true;
> +	spin_unlock_irqrestore(&host->lock, flags);
> +	/*
> +	 * This is needed here to hanlde a case where IRQ gets
> +	 * triggered even before this function is called so that
> +	 * x->done counter of completion gets reset. Otherwise,
> +	 * next call to wait_for_completion returns immediately
> +	 * without actually waiting for the IRQ to be handled.
> +	 */
> +	if (done)
> +		init_completion(&msm_host->pwr_irq_completion);
> +	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))

This all looks a bit more complicated and fragile than it needs to be.  You
are waiting for an event so you really want to be using
wait_event_timeout().  Reset the event condition before (will need a memory
barrier) writing the register and then just wait_event_timeout() to wait i.e.

Waiter:
	clear flag
	memory barrier
	write register
	wait_event_timeout(wq,flag is set,timeout)

Interrupt:
	set flag
	wake_up(&wq);

AFAICS you shouldn't need the spin lock at all.

> +		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
> +				mmc_hostname(host->mmc), req_type);
> +	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
> +			__func__, req_type);
> +}
> +#else
> +static inline void sdhci_msm_init_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +}
> +
> +static inline void sdhci_msm_complete_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +}
> +#endif
> +
>  static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1013,6 +1106,9 @@ 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;
> +	unsigned long flags;
> +
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
> @@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> -	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +	/* 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;
> +	}
> +	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> +		pwr_state = REQ_BUS_OFF;
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> -	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> +	}
> +	/* Handle IO LOW/HIGH */
> +	if (irq_status & CORE_PWRCTL_IO_LOW) {
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
> +	if (irq_status & CORE_PWRCTL_IO_HIGH) {
> +		io_level = REQ_IO_HIGH;
> +		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
>  
>  	/*
>  	 * The driver has to acknowledge the interrupt, switch voltages and
> @@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>  
> +	spin_lock_irqsave(&host->lock, flags);
> +	if (pwr_state)
> +		msm_host->curr_pwr_state = pwr_state;
> +	if (io_level)
> +		msm_host->curr_io_level = io_level;

Why separate curr_pwr_state and curr_io_level - the bits are separate
anyway.  Looks like this could just be:

	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);

	msm_host->pwr_irq_status = irq_status;

And as mentioned above, I don't think you need the spin lock.

> +	spin_unlock_irqrestore(&host->lock, flags);
> +	sdhci_msm_complete_pwr_irq_completion(msm_host);
> +
>  	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>  		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>  		irq_ack);
> @@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	sdhci_msm_init_pwr_irq_completion(msm_host);
>  	/* Enable pwr irq interrupts */
>  	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>  
> 

[PATCH 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> From: Sahitya Tummala <stummala@codeaurora.org>
> 
> Add support API which will check if power irq is expected to be
> generated and wait for the power irq to come and complete if the irq is
> expected.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 123 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index f3e0489..6d3b1fd 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -123,6 +123,10 @@
>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>  
>  #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
> +
> +/* Timeout value to avoid infinite waiting for pwr_irq */
> +#define MSM_PWR_IRQ_TIMEOUT_MS 5000
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -138,6 +142,11 @@ struct sdhci_msm_host {
>  	bool calibration_done;
>  	u8 saved_tuning_phase;
>  	bool use_cdclp533;
> +	u32 curr_pwr_state;
> +	u32 curr_io_level;
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +	struct completion pwr_irq_completion;
> +#endif
>  };
>  
>  static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +static inline void sdhci_msm_init_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +	init_completion(&msm_host->pwr_irq_completion);
> +}
> +
> +static inline void sdhci_msm_complete_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +	complete(&msm_host->pwr_irq_completion);
> +}
> +
> +/*
> + * sdhci_msm_check_power_status API should be called when registers writes
> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
> + * To what state the register writes will change the IO lines should be passed
> + * as the argument req_type. This API will check whether the IO line's state
> + * is already the expected state and will wait for power irq only if
> + * power irq is expected to be trigerred based on the current IO line state
> + * and expected IO line state.
> + */
> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	unsigned long flags;
> +	bool done = false;
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
> +			mmc_hostname(host->mmc), __func__, req_type,
> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
> +
> +	/*
> +	 * The IRQ for request type IO High/LOW will be generated when -
> +	 * there is a state change in 1.8V enable bit (bit 3) of
> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
> +	 * to set it to 3.3V before card detection happens, the
> +	 * IRQ doesn't get triggered as there is no state change in this bit.
> +	 * The driver already handles this case by changing the IO voltage
> +	 * level to high as part of controller power up sequence. Hence, check
> +	 * for host->pwr to handle a case where IO voltage high request is
> +	 * issued even before controller power up.
> +	 */
> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
> +		pr_debug("%s: do not wait for power IRQ that never comes\n",
> +				mmc_hostname(host->mmc));
> +		spin_unlock_irqrestore(&host->lock, flags);
> +		return;
> +	}
> +	if ((req_type & msm_host->curr_pwr_state) ||
> +			(req_type & msm_host->curr_io_level))
> +		done = true;
> +	spin_unlock_irqrestore(&host->lock, flags);
> +	/*
> +	 * This is needed here to hanlde a case where IRQ gets
> +	 * triggered even before this function is called so that
> +	 * x->done counter of completion gets reset. Otherwise,
> +	 * next call to wait_for_completion returns immediately
> +	 * without actually waiting for the IRQ to be handled.
> +	 */
> +	if (done)
> +		init_completion(&msm_host->pwr_irq_completion);
> +	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))

This all looks a bit more complicated and fragile than it needs to be.  You
are waiting for an event so you really want to be using
wait_event_timeout().  Reset the event condition before (will need a memory
barrier) writing the register and then just wait_event_timeout() to wait i.e.

Waiter:
	clear flag
	memory barrier
	write register
	wait_event_timeout(wq,flag is set,timeout)

Interrupt:
	set flag
	wake_up(&wq);

AFAICS you shouldn't need the spin lock at all.

> +		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
> +				mmc_hostname(host->mmc), req_type);
> +	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
> +			__func__, req_type);
> +}
> +#else
> +static inline void sdhci_msm_init_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +}
> +
> +static inline void sdhci_msm_complete_pwr_irq_completion(
> +		struct sdhci_msm_host *msm_host)
> +{
> +}
> +#endif
> +
>  static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1013,6 +1106,9 @@ 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;
> +	unsigned long flags;
> +
>  
>  	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>  	irq_status &= INT_MASK;
> @@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		udelay(10);
>  	}
>  
> -	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +	/* 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;
> +	}
> +	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> +		pwr_state = REQ_BUS_OFF;
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> -	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> +	}
> +	/* Handle IO LOW/HIGH */
> +	if (irq_status & CORE_PWRCTL_IO_LOW) {
> +		io_level = REQ_IO_LOW;
>  		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
> +	if (irq_status & CORE_PWRCTL_IO_HIGH) {
> +		io_level = REQ_IO_HIGH;
> +		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> +	}
>  
>  	/*
>  	 * The driver has to acknowledge the interrupt, switch voltages and
> @@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	 */
>  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>  
> +	spin_lock_irqsave(&host->lock, flags);
> +	if (pwr_state)
> +		msm_host->curr_pwr_state = pwr_state;
> +	if (io_level)
> +		msm_host->curr_io_level = io_level;

Why separate curr_pwr_state and curr_io_level - the bits are separate
anyway.  Looks like this could just be:

	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
  	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);

	msm_host->pwr_irq_status = irq_status;

And as mentioned above, I don't think you need the spin lock.

> +	spin_unlock_irqrestore(&host->lock, flags);
> +	sdhci_msm_complete_pwr_irq_completion(msm_host);
> +
>  	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>  		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>  		irq_ack);
> @@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	sdhci_msm_init_pwr_irq_completion(msm_host);
>  	/* Enable pwr irq interrupts */
>  	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>  
> 

Re: [PATCH 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Vijay Viswanath]

On 8/24/2017 3:35 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> Add support API which will check if power irq is expected to be
>> generated and wait for the power irq to come and complete if the irq is
>> expected.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 123 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index f3e0489..6d3b1fd 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -123,6 +123,10 @@
>>   #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>>   
>>   #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
>> +
>> +/* Timeout value to avoid infinite waiting for pwr_irq */
>> +#define MSM_PWR_IRQ_TIMEOUT_MS 5000
>> +
>>   struct sdhci_msm_host {
>>   	struct platform_device *pdev;
>>   	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -138,6 +142,11 @@ struct sdhci_msm_host {
>>   	bool calibration_done;
>>   	u8 saved_tuning_phase;
>>   	bool use_cdclp533;
>> +	u32 curr_pwr_state;
>> +	u32 curr_io_level;
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +	struct completion pwr_irq_completion;
>> +#endif
>>   };
>>   
>>   static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
>> @@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>   		sdhci_msm_hs400(host, &mmc->ios);
>>   }
>>   
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +static inline void sdhci_msm_init_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +	init_completion(&msm_host->pwr_irq_completion);
>> +}
>> +
>> +static inline void sdhci_msm_complete_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +	complete(&msm_host->pwr_irq_completion);
>> +}
>> +
>> +/*
>> + * sdhci_msm_check_power_status API should be called when registers writes
>> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
>> + * To what state the register writes will change the IO lines should be passed
>> + * as the argument req_type. This API will check whether the IO line's state
>> + * is already the expected state and will wait for power irq only if
>> + * power irq is expected to be trigerred based on the current IO line state
>> + * and expected IO line state.
>> + */
>> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	unsigned long flags;
>> +	bool done = false;
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
>> +			mmc_hostname(host->mmc), __func__, req_type,
>> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
>> +
>> +	/*
>> +	 * The IRQ for request type IO High/LOW will be generated when -
>> +	 * there is a state change in 1.8V enable bit (bit 3) of
>> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
>> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
>> +	 * to set it to 3.3V before card detection happens, the
>> +	 * IRQ doesn't get triggered as there is no state change in this bit.
>> +	 * The driver already handles this case by changing the IO voltage
>> +	 * level to high as part of controller power up sequence. Hence, check
>> +	 * for host->pwr to handle a case where IO voltage high request is
>> +	 * issued even before controller power up.
>> +	 */
>> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
>> +		pr_debug("%s: do not wait for power IRQ that never comes\n",
>> +				mmc_hostname(host->mmc));
>> +		spin_unlock_irqrestore(&host->lock, flags);
>> +		return;
>> +	}
>> +	if ((req_type & msm_host->curr_pwr_state) ||
>> +			(req_type & msm_host->curr_io_level))
>> +		done = true;
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +	/*
>> +	 * This is needed here to hanlde a case where IRQ gets
>> +	 * triggered even before this function is called so that
>> +	 * x->done counter of completion gets reset. Otherwise,
>> +	 * next call to wait_for_completion returns immediately
>> +	 * without actually waiting for the IRQ to be handled.
>> +	 */
>> +	if (done)
>> +		init_completion(&msm_host->pwr_irq_completion);
>> +	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
>> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> 
> This all looks a bit more complicated and fragile than it needs to be.  You
> are waiting for an event so you really want to be using
> wait_event_timeout().  Reset the event condition before (will need a memory
> barrier) writing the register and then just wait_event_timeout() to wait i.e.
> 
> Waiter:
> 	clear flag
> 	memory barrier
> 	write register
> 	wait_event_timeout(wq,flag is set,timeout)
> 
> Interrupt:
> 	set flag
> 	wake_up(&wq);
> 
> AFAICS you shouldn't need the spin lock at all.
>

Will do it this way. It looks cleaner and neat. Thanks!

>> +		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
>> +				mmc_hostname(host->mmc), req_type);
>> +	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
>> +			__func__, req_type);
>> +}
>> +#else
>> +static inline void sdhci_msm_init_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +}
>> +
>> +static inline void sdhci_msm_complete_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +}
>> +#endif
>> +
>>   static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1013,6 +1106,9 @@ 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;
>> +	unsigned long flags;
>> +
>>   
>>   	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>>   	irq_status &= INT_MASK;
>> @@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   		udelay(10);
>>   	}
>>   
>> -	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +	/* 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;
>> +	}
>> +	if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> +		pwr_state = REQ_BUS_OFF;
>> +		io_level = REQ_IO_LOW;
>>   		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> -	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
>> +	}
>> +	/* Handle IO LOW/HIGH */
>> +	if (irq_status & CORE_PWRCTL_IO_LOW) {
>> +		io_level = REQ_IO_LOW;
>>   		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> +	}
>> +	if (irq_status & CORE_PWRCTL_IO_HIGH) {
>> +		io_level = REQ_IO_HIGH;
>> +		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> +	}
>>   
>>   	/*
>>   	 * The driver has to acknowledge the interrupt, switch voltages and
>> @@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   	 */
>>   	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>>   
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	if (pwr_state)
>> +		msm_host->curr_pwr_state = pwr_state;
>> +	if (io_level)
>> +		msm_host->curr_io_level = io_level;
> 
> Why separate curr_pwr_state and curr_io_level - the bits are separate
> anyway.  Looks like this could just be:
> 
> 	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> 	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>    	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
> 
> 	msm_host->pwr_irq_status = irq_status;
> 
> And as mentioned above, I don't think you need the spin lock.
> 
I will remove the spinlock. Regarding why there are separate variable 
for BUS state and IO level,
During initialization, we get CORE_PWRCTL_BUS_ON interrupt from PWRCTL 
register. But the bit CORE_PWRCTL_IO_HIGH will not be set in the 
CORE_PWRCTL_STATUS register even though the io level will be high. So 
after the BUS is set on, if we do a register write to set IO as high, in 
the sdhci_msm_check_power_status register, we will think that there is a 
change in IO level and will wait for power irq . But the controller will 
not trigger any power irq as the IO level was already high.
So whenever we get the BUS_ON interrupt, we should store somewhere that 
the IO level is also HIGH.

We can do the above with a single variable instead of 2 variables used 
now, but it will make the code more complex. Whenever we have to change 
the pwr_irq_status in msm_host, we will have to clear either the 2 IO 
bits or the whole variable when we get power irq for IO level change or 
BUS on/off respectively.

>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +	sdhci_msm_complete_pwr_irq_completion(msm_host);
>> +
>>   	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>>   		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>>   		irq_ack);
>> @@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   		goto clk_disable;
>>   	}
>>   
>> +	sdhci_msm_init_pwr_irq_completion(msm_host);
>>   	/* Enable pwr irq interrupts */
>>   	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>>   
>>
> 

[PATCH 3/5] mmc: sdhci-msm: Add support to wait for power irq

[Vijay Viswanath]

On 8/24/2017 3:35 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> From: Sahitya Tummala <stummala@codeaurora.org>
>>
>> Add support API which will check if power irq is expected to be
>> generated and wait for the power irq to come and complete if the irq is
>> expected.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 125 ++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 123 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index f3e0489..6d3b1fd 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -123,6 +123,10 @@
>>   #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>>   
>>   #define MSM_MMC_AUTOSUSPEND_DELAY_MS	50
>> +
>> +/* Timeout value to avoid infinite waiting for pwr_irq */
>> +#define MSM_PWR_IRQ_TIMEOUT_MS 5000
>> +
>>   struct sdhci_msm_host {
>>   	struct platform_device *pdev;
>>   	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -138,6 +142,11 @@ struct sdhci_msm_host {
>>   	bool calibration_done;
>>   	u8 saved_tuning_phase;
>>   	bool use_cdclp533;
>> +	u32 curr_pwr_state;
>> +	u32 curr_io_level;
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +	struct completion pwr_irq_completion;
>> +#endif
>>   };
>>   
>>   static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
>> @@ -995,6 +1004,90 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>   		sdhci_msm_hs400(host, &mmc->ios);
>>   }
>>   
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +static inline void sdhci_msm_init_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +	init_completion(&msm_host->pwr_irq_completion);
>> +}
>> +
>> +static inline void sdhci_msm_complete_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +	complete(&msm_host->pwr_irq_completion);
>> +}
>> +
>> +/*
>> + * sdhci_msm_check_power_status API should be called when registers writes
>> + * which can toggle sdhci IO bus ON/OFF or change IO lines HIGH/LOW happens.
>> + * To what state the register writes will change the IO lines should be passed
>> + * as the argument req_type. This API will check whether the IO line's state
>> + * is already the expected state and will wait for power irq only if
>> + * power irq is expected to be trigerred based on the current IO line state
>> + * and expected IO line state.
>> + */
>> +static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	unsigned long flags;
>> +	bool done = false;
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
>> +			mmc_hostname(host->mmc), __func__, req_type,
>> +			msm_host->curr_pwr_state, msm_host->curr_io_level);
>> +
>> +	/*
>> +	 * The IRQ for request type IO High/LOW will be generated when -
>> +	 * there is a state change in 1.8V enable bit (bit 3) of
>> +	 * SDHCI_HOST_CONTROL2 register. The reset state of that bit is 0
>> +	 * which indicates 3.3V IO voltage. So, when MMC core layer tries
>> +	 * to set it to 3.3V before card detection happens, the
>> +	 * IRQ doesn't get triggered as there is no state change in this bit.
>> +	 * The driver already handles this case by changing the IO voltage
>> +	 * level to high as part of controller power up sequence. Hence, check
>> +	 * for host->pwr to handle a case where IO voltage high request is
>> +	 * issued even before controller power up.
>> +	 */
>> +	if ((req_type & REQ_IO_HIGH) && !host->pwr) {
>> +		pr_debug("%s: do not wait for power IRQ that never comes\n",
>> +				mmc_hostname(host->mmc));
>> +		spin_unlock_irqrestore(&host->lock, flags);
>> +		return;
>> +	}
>> +	if ((req_type & msm_host->curr_pwr_state) ||
>> +			(req_type & msm_host->curr_io_level))
>> +		done = true;
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +	/*
>> +	 * This is needed here to hanlde a case where IRQ gets
>> +	 * triggered even before this function is called so that
>> +	 * x->done counter of completion gets reset. Otherwise,
>> +	 * next call to wait_for_completion returns immediately
>> +	 * without actually waiting for the IRQ to be handled.
>> +	 */
>> +	if (done)
>> +		init_completion(&msm_host->pwr_irq_completion);
>> +	else if (!wait_for_completion_timeout(&msm_host->pwr_irq_completion,
>> +				msecs_to_jiffies(MSM_PWR_IRQ_TIMEOUT_MS)))
> 
> This all looks a bit more complicated and fragile than it needs to be.  You
> are waiting for an event so you really want to be using
> wait_event_timeout().  Reset the event condition before (will need a memory
> barrier) writing the register and then just wait_event_timeout() to wait i.e.
> 
> Waiter:
> 	clear flag
> 	memory barrier
> 	write register
> 	wait_event_timeout(wq,flag is set,timeout)
> 
> Interrupt:
> 	set flag
> 	wake_up(&wq);
> 
> AFAICS you shouldn't need the spin lock at all.
>

Will do it this way. It looks cleaner and neat. Thanks!

>> +		__WARN_printf("%s: request(%d) timed out waiting for pwr_irq\n",
>> +				mmc_hostname(host->mmc), req_type);
>> +	pr_debug("%s: %s: request %d done\n", mmc_hostname(host->mmc),
>> +			__func__, req_type);
>> +}
>> +#else
>> +static inline void sdhci_msm_init_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +}
>> +
>> +static inline void sdhci_msm_complete_pwr_irq_completion(
>> +		struct sdhci_msm_host *msm_host)
>> +{
>> +}
>> +#endif
>> +
>>   static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1013,6 +1106,9 @@ 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;
>> +	unsigned long flags;
>> +
>>   
>>   	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>>   	irq_status &= INT_MASK;
>> @@ -1040,10 +1136,26 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   		udelay(10);
>>   	}
>>   
>> -	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
>> +	/* 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;
>> +	}
>> +	if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> +		pwr_state = REQ_BUS_OFF;
>> +		io_level = REQ_IO_LOW;
>>   		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> -	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
>> +	}
>> +	/* Handle IO LOW/HIGH */
>> +	if (irq_status & CORE_PWRCTL_IO_LOW) {
>> +		io_level = REQ_IO_LOW;
>>   		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> +	}
>> +	if (irq_status & CORE_PWRCTL_IO_HIGH) {
>> +		io_level = REQ_IO_HIGH;
>> +		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> +	}
>>   
>>   	/*
>>   	 * The driver has to acknowledge the interrupt, switch voltages and
>> @@ -1052,6 +1164,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   	 */
>>   	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
>>   
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	if (pwr_state)
>> +		msm_host->curr_pwr_state = pwr_state;
>> +	if (io_level)
>> +		msm_host->curr_io_level = io_level;
> 
> Why separate curr_pwr_state and curr_io_level - the bits are separate
> anyway.  Looks like this could just be:
> 
> 	if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> 	if (irq_status & (CORE_PWRCTL_IO_LOW | CORE_PWRCTL_IO_HIGH))
> 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>    	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
> 
> 	msm_host->pwr_irq_status = irq_status;
> 
> And as mentioned above, I don't think you need the spin lock.
> 
I will remove the spinlock. Regarding why there are separate variable 
for BUS state and IO level,
During initialization, we get CORE_PWRCTL_BUS_ON interrupt from PWRCTL 
register. But the bit CORE_PWRCTL_IO_HIGH will not be set in the 
CORE_PWRCTL_STATUS register even though the io level will be high. So 
after the BUS is set on, if we do a register write to set IO as high, in 
the sdhci_msm_check_power_status register, we will think that there is a 
change in IO level and will wait for power irq . But the controller will 
not trigger any power irq as the IO level was already high.
So whenever we get the BUS_ON interrupt, we should store somewhere that 
the IO level is also HIGH.

We can do the above with a single variable instead of 2 variables used 
now, but it will make the code more complex. Whenever we have to change 
the pwr_irq_status in msm_host, we will have to clear either the 2 IO 
bits or the whole variable when we get power irq for IO level change or 
BUS on/off respectively.

>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +	sdhci_msm_complete_pwr_irq_completion(msm_host);
>> +
>>   	pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
>>   		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>>   		irq_ack);
>> @@ -1319,6 +1439,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   		goto clk_disable;
>>   	}
>>   
>> +	sdhci_msm_init_pwr_irq_completion(msm_host);
>>   	/* Enable pwr irq interrupts */
>>   	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
>>   
>>
> 

[PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus
on/off require controller to be ready before progressing further. When
the controller is ready, it will generate a power irq which needs to be
handled. The thread which initiated the register write should wait for
power irq to complete. This will be done through the new sdhc msm write
APIs which will check whether the particular write can trigger a power
irq and wait for it with a timeout if it is expected.
The SDHC core power control IRQ gets triggered when -
* There is a state change in power control bit (bit 0)
  of SDHCI_POWER_CONTROL register.
* There is a state change in 1.8V enable bit (bit 3) of
  SDHCI_HOST_CONTROL2 register.
* Bit 1 of SDHCI_SOFTWARE_RESET is set.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 6d3b1fd..6571880 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	__sdhci_msm_set_clock(host, clock);
 }
 
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
+{
+	u32 req_type = 0;
+
+	switch (reg) {
+	case SDHCI_HOST_CONTROL2:
+		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
+			REQ_IO_HIGH;
+		break;
+	case SDHCI_SOFTWARE_RESET:
+		if (host->pwr && (val & SDHCI_RESET_ALL))
+			req_type = REQ_BUS_OFF;
+		break;
+	case SDHCI_POWER_CONTROL:
+		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
+		break;
+	}
+
+	if (req_type)
+		sdhci_msm_check_power_status(host, req_type);
+}
+
+static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
+{
+		writew_relaxed(val, host->ioaddr + reg);
+		__sdhci_msm_check_write(host, val, reg);
+}
+
+static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+		writeb_relaxed(val, host->ioaddr + reg);
+		__sdhci_msm_check_write(host, val, reg);
+}
+#endif
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+	.write_w = sdhci_msm_writew,
+	.write_b = sdhci_msm_writeb,
+#endif
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
 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 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Vijay Viswanath]

Register writes which change voltage of IO lines or turn the IO bus
on/off require controller to be ready before progressing further. When
the controller is ready, it will generate a power irq which needs to be
handled. The thread which initiated the register write should wait for
power irq to complete. This will be done through the new sdhc msm write
APIs which will check whether the particular write can trigger a power
irq and wait for it with a timeout if it is expected.
The SDHC core power control IRQ gets triggered when -
* There is a state change in power control bit (bit 0)
  of SDHCI_POWER_CONTROL register.
* There is a state change in 1.8V enable bit (bit 3) of
  SDHCI_HOST_CONTROL2 register.
* Bit 1 of SDHCI_SOFTWARE_RESET is set.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 6d3b1fd..6571880 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	__sdhci_msm_set_clock(host, clock);
 }
 
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
+{
+	u32 req_type = 0;
+
+	switch (reg) {
+	case SDHCI_HOST_CONTROL2:
+		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
+			REQ_IO_HIGH;
+		break;
+	case SDHCI_SOFTWARE_RESET:
+		if (host->pwr && (val & SDHCI_RESET_ALL))
+			req_type = REQ_BUS_OFF;
+		break;
+	case SDHCI_POWER_CONTROL:
+		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
+		break;
+	}
+
+	if (req_type)
+		sdhci_msm_check_power_status(host, req_type);
+}
+
+static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
+{
+		writew_relaxed(val, host->ioaddr + reg);
+		__sdhci_msm_check_write(host, val, reg);
+}
+
+static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
+{
+		writeb_relaxed(val, host->ioaddr + reg);
+		__sdhci_msm_check_write(host, val, reg);
+}
+#endif
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
+#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
+	.write_w = sdhci_msm_writew,
+	.write_b = sdhci_msm_writeb,
+#endif
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
 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 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> Register writes which change voltage of IO lines or turn the IO bus
> on/off require controller to be ready before progressing further. When
> the controller is ready, it will generate a power irq which needs to be
> handled. The thread which initiated the register write should wait for
> power irq to complete. This will be done through the new sdhc msm write
> APIs which will check whether the particular write can trigger a power
> irq and wait for it with a timeout if it is expected.
> The SDHC core power control IRQ gets triggered when -
> * There is a state change in power control bit (bit 0)
>   of SDHCI_POWER_CONTROL register.
> * There is a state change in 1.8V enable bit (bit 3) of
>   SDHCI_HOST_CONTROL2 register.
> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 6d3b1fd..6571880 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
> +{
> +	u32 req_type = 0;
> +
> +	switch (reg) {
> +	case SDHCI_HOST_CONTROL2:
> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
> +			REQ_IO_HIGH;
> +		break;
> +	case SDHCI_SOFTWARE_RESET:
> +		if (host->pwr && (val & SDHCI_RESET_ALL))
> +			req_type = REQ_BUS_OFF;
> +		break;
> +	case SDHCI_POWER_CONTROL:
> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
> +		break;
> +	}
> +
> +	if (req_type)

So you are really relying on these register writes not being done in an
atomic context.  Since the spin lock was removed from sdhci_set_ios() that
seems to be true, but it would be good to add a comment here acknowledging
that you are depending on that.

> +		sdhci_msm_check_power_status(host, req_type);
> +}
> +
> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
> +{
> +		writew_relaxed(val, host->ioaddr + reg);
> +		__sdhci_msm_check_write(host, val, reg);
> +}
> +
> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
> +{
> +		writeb_relaxed(val, host->ioaddr + reg);
> +		__sdhci_msm_check_write(host, val, reg);
> +}
> +#endif
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>  	{ .compatible = "qcom,sdhci-msm-v4" },
>  	{},
> @@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +	.write_w = sdhci_msm_writew,
> +	.write_b = sdhci_msm_writeb,
> +#endif
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> 

[PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Adrian Hunter]

On 18/08/17 08:19, Vijay Viswanath wrote:
> Register writes which change voltage of IO lines or turn the IO bus
> on/off require controller to be ready before progressing further. When
> the controller is ready, it will generate a power irq which needs to be
> handled. The thread which initiated the register write should wait for
> power irq to complete. This will be done through the new sdhc msm write
> APIs which will check whether the particular write can trigger a power
> irq and wait for it with a timeout if it is expected.
> The SDHC core power control IRQ gets triggered when -
> * There is a state change in power control bit (bit 0)
>   of SDHCI_POWER_CONTROL register.
> * There is a state change in 1.8V enable bit (bit 3) of
>   SDHCI_HOST_CONTROL2 register.
> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 6d3b1fd..6571880 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
> +{
> +	u32 req_type = 0;
> +
> +	switch (reg) {
> +	case SDHCI_HOST_CONTROL2:
> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
> +			REQ_IO_HIGH;
> +		break;
> +	case SDHCI_SOFTWARE_RESET:
> +		if (host->pwr && (val & SDHCI_RESET_ALL))
> +			req_type = REQ_BUS_OFF;
> +		break;
> +	case SDHCI_POWER_CONTROL:
> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
> +		break;
> +	}
> +
> +	if (req_type)

So you are really relying on these register writes not being done in an
atomic context.  Since the spin lock was removed from sdhci_set_ios() that
seems to be true, but it would be good to add a comment here acknowledging
that you are depending on that.

> +		sdhci_msm_check_power_status(host, req_type);
> +}
> +
> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
> +{
> +		writew_relaxed(val, host->ioaddr + reg);
> +		__sdhci_msm_check_write(host, val, reg);
> +}
> +
> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
> +{
> +		writeb_relaxed(val, host->ioaddr + reg);
> +		__sdhci_msm_check_write(host, val, reg);
> +}
> +#endif
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>  	{ .compatible = "qcom,sdhci-msm-v4" },
>  	{},
> @@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> +	.write_w = sdhci_msm_writew,
> +	.write_b = sdhci_msm_writeb,
> +#endif
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> 

Re: [PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Vijay Viswanath]

On 8/24/2017 3:41 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> Register writes which change voltage of IO lines or turn the IO bus
>> on/off require controller to be ready before progressing further. When
>> the controller is ready, it will generate a power irq which needs to be
>> handled. The thread which initiated the register write should wait for
>> power irq to complete. This will be done through the new sdhc msm write
>> APIs which will check whether the particular write can trigger a power
>> irq and wait for it with a timeout if it is expected.
>> The SDHC core power control IRQ gets triggered when -
>> * There is a state change in power control bit (bit 0)
>>    of SDHCI_POWER_CONTROL register.
>> * There is a state change in 1.8V enable bit (bit 3) of
>>    SDHCI_HOST_CONTROL2 register.
>> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 6d3b1fd..6571880 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	__sdhci_msm_set_clock(host, clock);
>>   }
>>   
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +	u32 req_type = 0;
>> +
>> +	switch (reg) {
>> +	case SDHCI_HOST_CONTROL2:
>> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
>> +			REQ_IO_HIGH;
>> +		break;
>> +	case SDHCI_SOFTWARE_RESET:
>> +		if (host->pwr && (val & SDHCI_RESET_ALL))
>> +			req_type = REQ_BUS_OFF;
>> +		break;
>> +	case SDHCI_POWER_CONTROL:
>> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
>> +		break;
>> +	}
>> +
>> +	if (req_type)
> 
> So you are really relying on these register writes not being done in an
> atomic context.  Since the spin lock was removed from sdhci_set_ios() that
> seems to be true, but it would be good to add a comment here acknowledging
> that you are depending on that.
>

Will add the comments mentioning that this function can sleep and that 
it should not be called from atomic contexts.

>> +		sdhci_msm_check_power_status(host, req_type);
>> +}
>> +
>> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +		writew_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +
>> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
>> +{
>> +		writeb_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +#endif
>>   static const struct of_device_id sdhci_msm_dt_match[] = {
>>   	{ .compatible = "qcom,sdhci-msm-v4" },
>>   	{},
>> @@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	.get_max_clock = sdhci_msm_get_max_clock,
>>   	.set_bus_width = sdhci_set_bus_width,
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +	.write_w = sdhci_msm_writew,
>> +	.write_b = sdhci_msm_writeb,
>> +#endif
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
> 
> --
> 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
> 

[PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

[Vijay Viswanath]

On 8/24/2017 3:41 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> Register writes which change voltage of IO lines or turn the IO bus
>> on/off require controller to be ready before progressing further. When
>> the controller is ready, it will generate a power irq which needs to be
>> handled. The thread which initiated the register write should wait for
>> power irq to complete. This will be done through the new sdhc msm write
>> APIs which will check whether the particular write can trigger a power
>> irq and wait for it with a timeout if it is expected.
>> The SDHC core power control IRQ gets triggered when -
>> * There is a state change in power control bit (bit 0)
>>    of SDHCI_POWER_CONTROL register.
>> * There is a state change in 1.8V enable bit (bit 3) of
>>    SDHCI_HOST_CONTROL2 register.
>> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 6d3b1fd..6571880 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	__sdhci_msm_set_clock(host, clock);
>>   }
>>   
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +	u32 req_type = 0;
>> +
>> +	switch (reg) {
>> +	case SDHCI_HOST_CONTROL2:
>> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
>> +			REQ_IO_HIGH;
>> +		break;
>> +	case SDHCI_SOFTWARE_RESET:
>> +		if (host->pwr && (val & SDHCI_RESET_ALL))
>> +			req_type = REQ_BUS_OFF;
>> +		break;
>> +	case SDHCI_POWER_CONTROL:
>> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
>> +		break;
>> +	}
>> +
>> +	if (req_type)
> 
> So you are really relying on these register writes not being done in an
> atomic context.  Since the spin lock was removed from sdhci_set_ios() that
> seems to be true, but it would be good to add a comment here acknowledging
> that you are depending on that.
>

Will add the comments mentioning that this function can sleep and that 
it should not be called from atomic contexts.

>> +		sdhci_msm_check_power_status(host, req_type);
>> +}
>> +
>> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +		writew_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +
>> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
>> +{
>> +		writeb_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +#endif
>>   static const struct of_device_id sdhci_msm_dt_match[] = {
>>   	{ .compatible = "qcom,sdhci-msm-v4" },
>>   	{},
>> @@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	.get_max_clock = sdhci_msm_get_max_clock,
>>   	.set_bus_width = sdhci_set_bus_width,
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +	.write_w = sdhci_msm_writew,
>> +	.write_b = sdhci_msm_writeb,
>> +#endif
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
register read and write APIs, if registered, can be used.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 65cdd87..a3c93ed 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
 CONFIG_MMC_SDHCI_TEGRA=y
 CONFIG_MMC_MESON_GX=y
 CONFIG_MMC_SDHCI_MSM=y
+CONFIG_MMC_SDHCI_IO_ACCESSORS=y
 CONFIG_MMC_SPI=y
 CONFIG_MMC_SDHI=y
 CONFIG_MMC_DW=y
-- 
 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 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
register read and write APIs, if registered, can be used.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 65cdd87..a3c93ed 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
 CONFIG_MMC_SDHCI_TEGRA=y
 CONFIG_MMC_MESON_GX=y
 CONFIG_MMC_SDHCI_MSM=y
+CONFIG_MMC_SDHCI_IO_ACCESSORS=y
 CONFIG_MMC_SPI=y
 CONFIG_MMC_SDHI=y
 CONFIG_MMC_DW=y
-- 
 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 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
> register read and write APIs, if registered, can be used.
>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 65cdd87..a3c93ed 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>  CONFIG_MMC_SDHCI_TEGRA=y
>  CONFIG_MMC_MESON_GX=y
>  CONFIG_MMC_SDHCI_MSM=y
> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>  CONFIG_MMC_SPI=y
>  CONFIG_MMC_SDHI=y
>  CONFIG_MMC_DW=y

CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
SDHCI variants that needs it.

May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
drivers/mmc/host/Kconfig:
select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64

Kind regards
Uffe

[PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
> register read and write APIs, if registered, can be used.
>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 65cdd87..a3c93ed 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>  CONFIG_MMC_SDHCI_TEGRA=y
>  CONFIG_MMC_MESON_GX=y
>  CONFIG_MMC_SDHCI_MSM=y
> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>  CONFIG_MMC_SPI=y
>  CONFIG_MMC_SDHI=y
>  CONFIG_MMC_DW=y

CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
SDHCI variants that needs it.

May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
drivers/mmc/host/Kconfig:
select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64

Kind regards
Uffe

Re: [PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
> register read and write APIs, if registered, can be used.
>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 65cdd87..a3c93ed 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>  CONFIG_MMC_SDHCI_TEGRA=y
>  CONFIG_MMC_MESON_GX=y
>  CONFIG_MMC_SDHCI_MSM=y
> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>  CONFIG_MMC_SPI=y
>  CONFIG_MMC_SDHI=y
>  CONFIG_MMC_DW=y

CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
SDHCI variants that needs it.

May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
drivers/mmc/host/Kconfig:
select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64

Kind regards
Uffe

Re: [PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

On 8/22/2017 3:08 PM, Ulf Hansson wrote:
> On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
>> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
>> register read and write APIs, if registered, can be used.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   arch/arm64/configs/defconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index 65cdd87..a3c93ed 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>>   CONFIG_MMC_SDHCI_TEGRA=y
>>   CONFIG_MMC_MESON_GX=y
>>   CONFIG_MMC_SDHCI_MSM=y
>> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>>   CONFIG_MMC_SPI=y
>>   CONFIG_MMC_SDHI=y
>>   CONFIG_MMC_DW=y
> 
> CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
> SDHCI variants that needs it.
> 
> May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
> drivers/mmc/host/Kconfig:
> select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64
> 

Will do this way. Thanks!

> Kind regards
> Uffe
> --
> 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
> 

[PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

On 8/22/2017 3:08 PM, Ulf Hansson wrote:
> On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
>> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
>> register read and write APIs, if registered, can be used.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   arch/arm64/configs/defconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index 65cdd87..a3c93ed 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>>   CONFIG_MMC_SDHCI_TEGRA=y
>>   CONFIG_MMC_MESON_GX=y
>>   CONFIG_MMC_SDHCI_MSM=y
>> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>>   CONFIG_MMC_SPI=y
>>   CONFIG_MMC_SDHI=y
>>   CONFIG_MMC_DW=y
> 
> CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
> SDHCI variants that needs it.
> 
> May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
> drivers/mmc/host/Kconfig:
> select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64
> 

Will do this way. Thanks!

> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5] defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS

[Vijay Viswanath]

On 8/22/2017 3:08 PM, Ulf Hansson wrote:
> On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
>> Enable CONFIG_MMC_SDHCI_IO_ACCESSORS so that SDHC controller specific
>> register read and write APIs, if registered, can be used.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   arch/arm64/configs/defconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>> index 65cdd87..a3c93ed 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -398,6 +398,7 @@ CONFIG_MMC_SDHCI_CADENCE=y
>>   CONFIG_MMC_SDHCI_TEGRA=y
>>   CONFIG_MMC_MESON_GX=y
>>   CONFIG_MMC_SDHCI_MSM=y
>> +CONFIG_MMC_SDHCI_IO_ACCESSORS=y
>>   CONFIG_MMC_SPI=y
>>   CONFIG_MMC_SDHI=y
>>   CONFIG_MMC_DW=y
> 
> CONFIG_MMC_SDHCI_IO_ACCESSORS is intended to be selected by those
> SDHCI variants that needs it.
> 
> May therefore suggest you add a "select line" for MMC_SDHCI_MSM in
> drivers/mmc/host/Kconfig:
> select CONFIG_MMC_SDHCI_IO_ACCESSORS if ARM64
> 

Will do this way. Thanks!

> Kind regards
> Uffe
> --
> 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
> 

Re: [PATCH 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Register writes which change voltage of IO lines or turn the IO bus on/off
> require sdhc controller to be ready before progressing further. Once a
> register write which affects IO lines is done, the driver should wait for
> power irq from controller. Once the irq comes, the driver should acknowledge
> the irq by writing to power control register. If the acknowledgement is not
> given to controller, the controller may not complete the corresponding
> register write action and this can mess up the controller if drivers proceeds
> without power irq completing.
>
> Sahitya Tummala (2):
>   mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
>   mmc: sdhci-msm: Add support to wait for power irq
>
> Subhash Jadavani (1):
>   mmc: sdhci-msm: fix issue with power irq
>
> Vijay Viswanath (2):
>   mmc: sdhci-msm: Add ops to do sdhc register write
>   defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS
>
>  arch/arm64/configs/defconfig |   1 +
>  drivers/mmc/host/sdhci-msm.c | 233 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 229 insertions(+), 5 deletions(-)
>
> --
>  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.
>

Looks good to me, besides patch 5 which I provided comments for.

Unless Adrian objects, I intend to queue this within the next couple of days.

Kind regards
Uffe

[PATCH 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Register writes which change voltage of IO lines or turn the IO bus on/off
> require sdhc controller to be ready before progressing further. Once a
> register write which affects IO lines is done, the driver should wait for
> power irq from controller. Once the irq comes, the driver should acknowledge
> the irq by writing to power control register. If the acknowledgement is not
> given to controller, the controller may not complete the corresponding
> register write action and this can mess up the controller if drivers proceeds
> without power irq completing.
>
> Sahitya Tummala (2):
>   mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
>   mmc: sdhci-msm: Add support to wait for power irq
>
> Subhash Jadavani (1):
>   mmc: sdhci-msm: fix issue with power irq
>
> Vijay Viswanath (2):
>   mmc: sdhci-msm: Add ops to do sdhc register write
>   defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS
>
>  arch/arm64/configs/defconfig |   1 +
>  drivers/mmc/host/sdhci-msm.c | 233 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 229 insertions(+), 5 deletions(-)
>
> --
>  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.
>

Looks good to me, besides patch 5 which I provided comments for.

Unless Adrian objects, I intend to queue this within the next couple of days.

Kind regards
Uffe

Re: [PATCH 0/5] mmc: sdhci-msm: Corrections to implementation of power irq

[Ulf Hansson]

On 18 August 2017 at 07:19, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> Register writes which change voltage of IO lines or turn the IO bus on/off
> require sdhc controller to be ready before progressing further. Once a
> register write which affects IO lines is done, the driver should wait for
> power irq from controller. Once the irq comes, the driver should acknowledge
> the irq by writing to power control register. If the acknowledgement is not
> given to controller, the controller may not complete the corresponding
> register write action and this can mess up the controller if drivers proceeds
> without power irq completing.
>
> Sahitya Tummala (2):
>   mmc: sdhci-msm: Fix HW issue with power IRQ handling during reset
>   mmc: sdhci-msm: Add support to wait for power irq
>
> Subhash Jadavani (1):
>   mmc: sdhci-msm: fix issue with power irq
>
> Vijay Viswanath (2):
>   mmc: sdhci-msm: Add ops to do sdhc register write
>   defconfig: msm: Enable CONFIG_MMC_SDHCI_IO_ACCESSORS
>
>  arch/arm64/configs/defconfig |   1 +
>  drivers/mmc/host/sdhci-msm.c | 233 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 229 insertions(+), 5 deletions(-)
>
> --
>  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.
>

Looks good to me, besides patch 5 which I provided comments for.

Unless Adrian objects, I intend to queue this within the next couple of days.

Kind regards
Uffe