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

[Sajida Bhanu (Temp) (QUIC)]

Hi,

Thanks for the review.

Please find the inline comments.

Thanks,
Sajida
> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Tuesday, March 15, 2022 3:19 PM
> To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@xxxxxxxxxxx>;
> adrian.hunter@xxxxxxxxx; agross@xxxxxxxxxx; bjorn.andersson@xxxxxxxxxx;
> ulf.hansson@xxxxxxxxxx; chris@xxxxxxxxxx; gdjakov@xxxxxxxxxx
> Cc: linux-mmc@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Asutosh Das (QUIC) <quic_asutoshd@xxxxxxxxxxx>;
> Ram Prakash Gupta (QUIC) <quic_rampraka@xxxxxxxxxxx>; Pradeep
> Pragallapati (QUIC) <quic_pragalla@xxxxxxxxxxx>; Sarthak Garg (QUIC)
> <quic_sartgarg@xxxxxxxxxxx>; Nitin Rawat (QUIC)
> <quic_nitirawa@xxxxxxxxxxx>; Sayali Lokhande (QUIC)
> <quic_sayalil@xxxxxxxxxxx>
> Subject: Re: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for
> SDHC
> 
> 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?
Followed existing clients...
No its not added in the dt-binbing,  will add dt-binding patch in patch version.
> 
> > +       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.
> 
Ok
> [...]
> > @@ -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.
>
 Sure, we will discuss this proposal internally and update.
> regards
> Philipp