Reset GCC_SDCC_BCR register before every fresh initilazation. This will
reset whole SDHC-msm controller, clears the previous power control
states and avoids, software reset timeout issues as below.
[ 5.458061][ T262] mmc1: Reset 0x1 never completed.
[ 5.462454][ T262] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
[ 5.469065][ T262] mmc1: sdhci: Sys addr: 0x00000000 | Version: 0x00007202
[ 5.475688][ T262] mmc1: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000
[ 5.482315][ T262] mmc1: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
[ 5.488927][ T262] mmc1: sdhci: Present: 0x01f800f0 | Host ctl: 0x00000000
[ 5.495539][ T262] mmc1: sdhci: Power: 0x00000000 | Blk gap: 0x00000000
[ 5.502162][ T262] mmc1: sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
[ 5.508768][ T262] mmc1: sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
[ 5.515381][ T262] mmc1: sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
[ 5.521996][ T262] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[ 5.528607][ T262] mmc1: sdhci: Caps: 0x362dc8b2 | Caps_1: 0x0000808f
[ 5.535227][ T262] mmc1: sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
[ 5.541841][ T262] mmc1: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000
[ 5.548454][ T262] mmc1: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
[ 5.555079][ T262] mmc1: sdhci: Host ctl2: 0x00000000
[ 5.559651][ T262] mmc1: sdhci_msm: ----------- VENDOR REGISTER DUMP-----------
[ 5.566621][ T262] mmc1: sdhci_msm: DLL sts: 0x00000000 | DLL cfg: 0x6000642c | DLL cfg2: 0x0020a000
[ 5.575465][ T262] mmc1: sdhci_msm: DLL cfg3: 0x00000000 | DLL usr ctl: 0x00010800 | DDR cfg: 0x80040873
[ 5.584658][ T262] mmc1: sdhci_msm: Vndr func: 0x00018a9c | Vndr func2 : 0xf88218a8 Vndr func3: 0x02626040
Fixes: 0eb0d9f4de34 ("mmc: sdhci-msm: Initial support for Qualcomm chipsets")
Signed-off-by: Shaik Sajida Bhanu <[email protected]>
---
drivers/mmc/host/sdhci-msm.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 50c71e0..0e5fb62 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -17,6 +17,7 @@
#include <linux/regulator/consumer.h>
#include <linux/interconnect.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/reset.h>
#include "sdhci-pltfm.h"
#include "cqhci.h"
@@ -2482,6 +2483,39 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
}
+static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
+{
+ struct reset_control *reset;
+ int ret = 0;
+
+ reset = reset_control_get_optional_exclusive(dev, NULL);
+ if (IS_ERR(reset))
+ return dev_err_probe(dev, PTR_ERR(reset),
+ "unable to acquire core_reset\n");
+
+ if (!reset)
+ return ret;
+
+ ret = reset_control_assert(reset);
+ if (ret)
+ return dev_err_probe(dev, ret, "core_reset assert failed\n");
+
+ /*
+ * The hardware requirement for delay between assert/deassert
+ * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
+ * ~125us (4/32768). To be on the safe side add 200us delay.
+ */
+ usleep_range(200, 210);
+
+ ret = reset_control_deassert(reset);
+ if (ret)
+ return dev_err_probe(dev, ret, "core_reset deassert failed\n");
+
+ usleep_range(200, 210);
+ reset_control_put(reset);
+
+ return ret;
+}
static int sdhci_msm_probe(struct platform_device *pdev)
{
@@ -2529,6 +2563,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
+ ret = sdhci_msm_gcc_reset(&pdev->dev, host);
+ if (ret)
+ goto pltfm_free;
+
/* Setup SDCC bus voter clock. */
msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
if (!IS_ERR(msm_host->bus_clk)) {
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Di, 2022-04-12 at 16:09 +0530, Shaik Sajida Bhanu wrote:
> Reset GCC_SDCC_BCR register before every fresh initilazation. This will
> reset whole SDHC-msm controller, clears the previous power control
> states and avoids, software reset timeout issues as below.
>
> [ 5.458061][ T262] mmc1: Reset 0x1 never completed.
> [ 5.462454][ T262] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
> [ 5.469065][ T262] mmc1: sdhci: Sys addr: 0x00000000 | Version: 0x00007202
> [ 5.475688][ T262] mmc1: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000
> [ 5.482315][ T262] mmc1: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
> [ 5.488927][ T262] mmc1: sdhci: Present: 0x01f800f0 | Host ctl: 0x00000000
> [ 5.495539][ T262] mmc1: sdhci: Power: 0x00000000 | Blk gap: 0x00000000
> [ 5.502162][ T262] mmc1: sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
> [ 5.508768][ T262] mmc1: sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
> [ 5.515381][ T262] mmc1: sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
> [ 5.521996][ T262] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [ 5.528607][ T262] mmc1: sdhci: Caps: 0x362dc8b2 | Caps_1: 0x0000808f
> [ 5.535227][ T262] mmc1: sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
> [ 5.541841][ T262] mmc1: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000
> [ 5.548454][ T262] mmc1: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
> [ 5.555079][ T262] mmc1: sdhci: Host ctl2: 0x00000000
> [ 5.559651][ T262] mmc1: sdhci_msm: ----------- VENDOR REGISTER DUMP-----------
> [ 5.566621][ T262] mmc1: sdhci_msm: DLL sts: 0x00000000 | DLL cfg: 0x6000642c | DLL cfg2: 0x0020a000
> [ 5.575465][ T262] mmc1: sdhci_msm: DLL cfg3: 0x00000000 | DLL usr ctl: 0x00010800 | DDR cfg: 0x80040873
> [ 5.584658][ T262] mmc1: sdhci_msm: Vndr func: 0x00018a9c | Vndr func2 : 0xf88218a8 Vndr func3: 0x02626040
>
> Fixes: 0eb0d9f4de34 ("mmc: sdhci-msm: Initial support for Qualcomm chipsets")
> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
> ---
> drivers/mmc/host/sdhci-msm.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 50c71e0..0e5fb62 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -17,6 +17,7 @@
> #include <linux/regulator/consumer.h>
> #include <linux/interconnect.h>
> #include <linux/pinctrl/consumer.h>
> +#include <linux/reset.h>
>
>
>
>
> #include "sdhci-pltfm.h"
> #include "cqhci.h"
> @@ -2482,6 +2483,39 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
> of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
> }
>
>
>
>
> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> +{
> + struct reset_control *reset;
> + int ret = 0;
No need to initialize ret.
> +
> + reset = reset_control_get_optional_exclusive(dev, NULL);
> + if (IS_ERR(reset))
> + return dev_err_probe(dev, PTR_ERR(reset),
> + "unable to acquire core_reset\n");
> +
> + if (!reset)
> + return ret;
> +
> + ret = reset_control_assert(reset);
> + if (ret)
> + return dev_err_probe(dev, ret, "core_reset assert failed\n");
Missing reset_control_put(reset) in the error path.
> +
> + /*
> + * The hardware requirement for delay between assert/deassert
> + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
> + * ~125us (4/32768). To be on the safe side add 200us delay.
> + */
> + usleep_range(200, 210);
> +
> + ret = reset_control_deassert(reset);
> + if (ret)
> + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
Same as above. Maybe make both ret = dev_err_probe() and goto ...
> +
> + usleep_range(200, 210);
... here.
> + reset_control_put(reset);
> +
> + return ret;
> +}
regards
Philipp
Hi,
Thanks for the review.
Please find the inline comments.
Thanks,
Sajida
On 4/12/2022 4:30 PM, Philipp Zabel wrote:
> On Di, 2022-04-12 at 16:09 +0530, Shaik Sajida Bhanu wrote:
>> Reset GCC_SDCC_BCR register before every fresh initilazation. This will
>> reset whole SDHC-msm controller, clears the previous power control
>> states and avoids, software reset timeout issues as below.
>>
>> [ 5.458061][ T262] mmc1: Reset 0x1 never completed.
>> [ 5.462454][ T262] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
>> [ 5.469065][ T262] mmc1: sdhci: Sys addr: 0x00000000 | Version: 0x00007202
>> [ 5.475688][ T262] mmc1: sdhci: Blk size: 0x00000000 | Blk cnt: 0x00000000
>> [ 5.482315][ T262] mmc1: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>> [ 5.488927][ T262] mmc1: sdhci: Present: 0x01f800f0 | Host ctl: 0x00000000
>> [ 5.495539][ T262] mmc1: sdhci: Power: 0x00000000 | Blk gap: 0x00000000
>> [ 5.502162][ T262] mmc1: sdhci: Wake-up: 0x00000000 | Clock: 0x00000003
>> [ 5.508768][ T262] mmc1: sdhci: Timeout: 0x00000000 | Int stat: 0x00000000
>> [ 5.515381][ T262] mmc1: sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>> [ 5.521996][ T262] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
>> [ 5.528607][ T262] mmc1: sdhci: Caps: 0x362dc8b2 | Caps_1: 0x0000808f
>> [ 5.535227][ T262] mmc1: sdhci: Cmd: 0x00000000 | Max curr: 0x00000000
>> [ 5.541841][ T262] mmc1: sdhci: Resp[0]: 0x00000000 | Resp[1]: 0x00000000
>> [ 5.548454][ T262] mmc1: sdhci: Resp[2]: 0x00000000 | Resp[3]: 0x00000000
>> [ 5.555079][ T262] mmc1: sdhci: Host ctl2: 0x00000000
>> [ 5.559651][ T262] mmc1: sdhci_msm: ----------- VENDOR REGISTER DUMP-----------
>> [ 5.566621][ T262] mmc1: sdhci_msm: DLL sts: 0x00000000 | DLL cfg: 0x6000642c | DLL cfg2: 0x0020a000
>> [ 5.575465][ T262] mmc1: sdhci_msm: DLL cfg3: 0x00000000 | DLL usr ctl: 0x00010800 | DDR cfg: 0x80040873
>> [ 5.584658][ T262] mmc1: sdhci_msm: Vndr func: 0x00018a9c | Vndr func2 : 0xf88218a8 Vndr func3: 0x02626040
>>
>> Fixes: 0eb0d9f4de34 ("mmc: sdhci-msm: Initial support for Qualcomm chipsets")
>> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 50c71e0..0e5fb62 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -17,6 +17,7 @@
>> #include <linux/regulator/consumer.h>
>> #include <linux/interconnect.h>
>> #include <linux/pinctrl/consumer.h>
>> +#include <linux/reset.h>
>>
>>
>>
>>
>> #include "sdhci-pltfm.h"
>> #include "cqhci.h"
>> @@ -2482,6 +2483,39 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
>> of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
>> }
>>
>>
>>
>>
>> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>> +{
>> + struct reset_control *reset;
>> + int ret = 0;
> No need to initialize ret.
>
>> +
>> + reset = reset_control_get_optional_exclusive(dev, NULL);
>> + if (IS_ERR(reset))
>> + return dev_err_probe(dev, PTR_ERR(reset),
>> + "unable to acquire core_reset\n");
>> +
>> + if (!reset)
>> + return ret;
Here we are returning ret directly if reset is NULL , so ret
initialization is required.
>> +
>> + ret = reset_control_assert(reset);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "core_reset assert failed\n");
> Missing reset_control_put(reset) in the error path.
Sure will add
>
>> +
>> + /*
>> + * The hardware requirement for delay between assert/deassert
>> + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
>> + * ~125us (4/32768). To be on the safe side add 200us delay.
>> + */
>> + usleep_range(200, 210);
>> +
>> + ret = reset_control_deassert(reset);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
> Same as above. Maybe make both ret = dev_err_probe() and goto ...
In both cases error message is different so I think goto not good idea here.
>
>> +
>> + usleep_range(200, 210);
> ... here.
>
>> + reset_control_put(reset);
>> +
>> + return ret;
>> +}
> regards
> Philipp
On Do, 2022-04-21 at 10:32 +0530, Sajida Bhanu (Temp) wrote:
> Hi,
>
> Thanks for the review.
>
> Please find the inline comments.
>
> Thanks,
>
> Sajida
>
> On 4/19/2022 12:52 PM, Philipp Zabel wrote:
> > Hi Sajida,
> >
> > On Di, 2022-04-19 at 11:46 +0530, Sajida Bhanu (Temp) wrote:
> > [...]
> > > > > +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> > > > > +{
> > > > > + struct reset_control *reset;
> > > > > + int ret = 0;
> > > > No need to initialize ret.
> > > >
> > > > > +
> > > > > + reset = reset_control_get_optional_exclusive(dev, NULL);
> > > > > + if (IS_ERR(reset))
> > > > > + return dev_err_probe(dev, PTR_ERR(reset),
> > > > > + "unable to acquire core_reset\n");
> > > > > +
> > > > > + if (!reset)
> > > > > + return ret;
> > > Here we are returning ret directly if reset is NULL , so ret
> > > initialization is required.
> > You are right. I would just "return 0;" here, but this is correct as
> > is.
> Ok
> > > > > +
> > > > > + ret = reset_control_assert(reset);
> > > > > + if (ret)
> > > > > + return dev_err_probe(dev, ret, "core_reset assert failed\n");
> > > > Missing reset_control_put(reset) in the error path.
> > > Sure will add
> > > > > +
> > > > > + /*
> > > > > + * The hardware requirement for delay between assert/deassert
> > > > > + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
> > > > > + * ~125us (4/32768). To be on the safe side add 200us delay.
> > > > > + */
> > > > > + usleep_range(200, 210);
> > > > > +
> > > > > + ret = reset_control_deassert(reset);
> > > > > + if (ret)
> > > > > + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
> > > > Same as above. Maybe make both ret = dev_err_probe() and goto ...
> > > In both cases error message is different so I think goto not good idea here.
> > You could goto after the error message. Either way is fine.
>
> Sorry didn't get this ..can you please help
I meant you could either use goto after the error messages:
+static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
+{
[...]
+ ret = reset_control_assert(reset);
+ if (ret) {
+ dev_err_probe(dev, ret, "core_reset assert failed\n");
+ goto out_reset_put;
+ }
[...]
+ ret = reset_control_deassert(reset);
+ if (ret) {
+ dev_err_probe(dev, ret, "core_reset deassert failed\n");
+ goto out_reset_put;
+ }
+
+ usleep_range(200, 210);
+
+out_reset_put:
+ reset_control_put(reset);
+
+ return ret;
+}
Or not use goto and copy the reset_control_put() into each error path:
+static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
+{
[...]
+ ret = reset_control_assert(reset);
+ if (ret) {
+ reset_control_put(reset);
+ return dev_err_probe(dev, ret, "core_reset assert failed\n");
+ }
[...]
+ ret = reset_control_deassert(reset);
+ if (ret) {
+ reset_control_put(reset);
+ return dev_err_probe(dev, ret, "core_reset deassert failed\n");
+ }
+
+ usleep_range(200, 210);
+ reset_control_put(reset);
+
+ return 0;
+}
regards
Philipp
>
Hi,
Thanks for the review.
Please find the inline comments.
Thanks,
Sajida
On 4/19/2022 12:52 PM, Philipp Zabel wrote:
> Hi Sajida,
>
> On Di, 2022-04-19 at 11:46 +0530, Sajida Bhanu (Temp) wrote:
> [...]
>>>> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>>> +{
>>>> + struct reset_control *reset;
>>>> + int ret = 0;
>>> No need to initialize ret.
>>>
>>>> +
>>>> + reset = reset_control_get_optional_exclusive(dev, NULL);
>>>> + if (IS_ERR(reset))
>>>> + return dev_err_probe(dev, PTR_ERR(reset),
>>>> + "unable to acquire core_reset\n");
>>>> +
>>>> + if (!reset)
>>>> + return ret;
>> Here we are returning ret directly if reset is NULL , so ret
>> initialization is required.
> You are right. I would just "return 0;" here, but this is correct as
> is.
Ok
>>>> +
>>>> + ret = reset_control_assert(reset);
>>>> + if (ret)
>>>> + return dev_err_probe(dev, ret, "core_reset assert failed\n");
>>> Missing reset_control_put(reset) in the error path.
>> Sure will add
>>>> +
>>>> + /*
>>>> + * The hardware requirement for delay between assert/deassert
>>>> + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
>>>> + * ~125us (4/32768). To be on the safe side add 200us delay.
>>>> + */
>>>> + usleep_range(200, 210);
>>>> +
>>>> + ret = reset_control_deassert(reset);
>>>> + if (ret)
>>>> + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
>>> Same as above. Maybe make both ret = dev_err_probe() and goto ...
>> In both cases error message is different so I think goto not good idea here.
> You could goto after the error message. Either way is fine.
Sorry didn't get this ..can you please help
>
> regards
> Philipp
Hi Sajida,
On Di, 2022-04-19 at 11:46 +0530, Sajida Bhanu (Temp) wrote:
[...]
> > > +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> > > +{
> > > + struct reset_control *reset;
> > > + int ret = 0;
> > No need to initialize ret.
> >
> > > +
> > > + reset = reset_control_get_optional_exclusive(dev, NULL);
> > > + if (IS_ERR(reset))
> > > + return dev_err_probe(dev, PTR_ERR(reset),
> > > + "unable to acquire core_reset\n");
> > > +
> > > + if (!reset)
> > > + return ret;
> Here we are returning ret directly if reset is NULL , so ret
> initialization is required.
You are right. I would just "return 0;" here, but this is correct as
is.
> > > +
> > > + ret = reset_control_assert(reset);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "core_reset assert failed\n");
> > Missing reset_control_put(reset) in the error path.
> Sure will add
> >
> > > +
> > > + /*
> > > + * The hardware requirement for delay between assert/deassert
> > > + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
> > > + * ~125us (4/32768). To be on the safe side add 200us delay.
> > > + */
> > > + usleep_range(200, 210);
> > > +
> > > + ret = reset_control_deassert(reset);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
> > Same as above. Maybe make both ret = dev_err_probe() and goto ...
> In both cases error message is different so I think goto not good idea here.
You could goto after the error message. Either way is fine.
regards
Philipp
Hi,
Thanks for the review.
Please find the inline comments.
Thanks,
Sajida
On 4/21/2022 3:26 PM, Philipp Zabel wrote:
> On Do, 2022-04-21 at 10:32 +0530, Sajida Bhanu (Temp) wrote:
>> Hi,
>>
>> Thanks for the review.
>>
>> Please find the inline comments.
>>
>> Thanks,
>>
>> Sajida
>>
>> On 4/19/2022 12:52 PM, Philipp Zabel wrote:
>>> Hi Sajida,
>>>
>>> On Di, 2022-04-19 at 11:46 +0530, Sajida Bhanu (Temp) wrote:
>>> [...]
>>>>>> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>>>>> +{
>>>>>> + struct reset_control *reset;
>>>>>> + int ret = 0;
>>>>> No need to initialize ret.
>>>>>
>>>>>> +
>>>>>> + reset = reset_control_get_optional_exclusive(dev, NULL);
>>>>>> + if (IS_ERR(reset))
>>>>>> + return dev_err_probe(dev, PTR_ERR(reset),
>>>>>> + "unable to acquire core_reset\n");
>>>>>> +
>>>>>> + if (!reset)
>>>>>> + return ret;
>>>> Here we are returning ret directly if reset is NULL , so ret
>>>> initialization is required.
>>> You are right. I would just "return 0;" here, but this is correct as
>>> is.
>> Ok
>>>>>> +
>>>>>> + ret = reset_control_assert(reset);
>>>>>> + if (ret)
>>>>>> + return dev_err_probe(dev, ret, "core_reset assert failed\n");
>>>>> Missing reset_control_put(reset) in the error path.
>>>> Sure will add
>>>>>> +
>>>>>> + /*
>>>>>> + * The hardware requirement for delay between assert/deassert
>>>>>> + * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
>>>>>> + * ~125us (4/32768). To be on the safe side add 200us delay.
>>>>>> + */
>>>>>> + usleep_range(200, 210);
>>>>>> +
>>>>>> + ret = reset_control_deassert(reset);
>>>>>> + if (ret)
>>>>>> + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
>>>>> Same as above. Maybe make both ret = dev_err_probe() and goto ...
>>>> In both cases error message is different so I think goto not good idea here.
>>> You could goto after the error message. Either way is fine.
>> Sorry didn't get this ..can you please help
> I meant you could either use goto after the error messages:
>
> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> +{
> [...]
> + ret = reset_control_assert(reset);
> + if (ret) {
> + dev_err_probe(dev, ret, "core_reset assert failed\n");
> + goto out_reset_put;
> + }
> [...]
> + ret = reset_control_deassert(reset);
> + if (ret) {
> + dev_err_probe(dev, ret, "core_reset deassert failed\n");
> + goto out_reset_put;
> + }
> +
> + usleep_range(200, 210);
> +
> +out_reset_put:
> + reset_control_put(reset);
> +
> + return ret;
> +}
>
> Or not use goto and copy the reset_control_put() into each error path:
>
> +static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> +{
> [...]
> + ret = reset_control_assert(reset);
> + if (ret) {
> + reset_control_put(reset);
> + return dev_err_probe(dev, ret, "core_reset assert failed\n");
> + }
> [...]
> + ret = reset_control_deassert(reset);
> + if (ret) {
> + reset_control_put(reset);
> + return dev_err_probe(dev, ret, "core_reset deassert failed\n");
> + }
> +
> + usleep_range(200, 210);
> + reset_control_put(reset);
> +
> + return 0;
> +}
>
> regards
> Philipp
Sure I will call reset_control_put() in each error path