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
