Re: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for SDHC

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Sajida,

On Do, 2022-03-10 at 21:10 +0530, Shaik Sajida Bhanu wrote:
[...]
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-
> msm.c
> index 50c71e0..cb33c9a 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
[...]
> @@ -2482,6 +2484,45 @@ 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 platform_device *pdev,
> +              struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       int ret = 0;
> +
> +       msm_host->core_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "core_reset");

I think the "_reset" part in the name is superfluous and this reset
control should be called "core". Is this documented in the sdhci-msm
device tree binding document?

> +       if (IS_ERR(msm_host->core_reset)) {
> +               ret = PTR_ERR(msm_host->core_reset);
> +               dev_err(&pdev->dev, "core_reset unavailable (%d)\n", ret);
> +               msm_host->core_reset = NULL;

As Bjorn pointed out, this error should be returned.
reset_control_get_optional returns NULL if the optional reset control
is not specified in the device tree, so we only land here if there's a
real error.

[...]
> @@ -2529,6 +2570,13 @@ static int sdhci_msm_probe(struct
> platform_device *pdev)
>  
>         msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>  
> +       ret = sdhci_msm_gcc_reset(pdev, host);
> +       if (ret) {
> +               dev_err(&pdev->dev, "core_reset assert/deassert failed (%d)\n",
> +                                       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)) {

I notice that this driver requests resources such as clocks and resets
and then immediately uses them, one by one. It would be better to
request all resources first, and only then start interacting with the
hardware. This is not an issue that can be fixed in this patch,
although maybe it could be prepared for it by separating the
reset_control_get from the _assert/deassert.

regards
Philipp