Messages in this thread | | | From | "Sajida Bhanu (Temp) (QUIC)" <> | Subject | RE: [PATCH V2] mmc: sdhci-msm: Reset GCC_SDCC_BCR register for SDHC | Date | Wed, 16 Mar 2022 13:15:47 +0000 |
| |
Hi,
Thanks for the review.
Please find the inline comments.
Thanks, Sajida > -----Original Message----- > From: Philipp Zabel <p.zabel@pengutronix.de> > Sent: Tuesday, March 15, 2022 3:19 PM > To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@quicinc.com>; > adrian.hunter@intel.com; agross@kernel.org; bjorn.andersson@linaro.org; > ulf.hansson@linaro.org; chris@printf.net; gdjakov@mm-sol.com > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > kernel@vger.kernel.org; Asutosh Das (QUIC) <quic_asutoshd@quicinc.com>; > Ram Prakash Gupta (QUIC) <quic_rampraka@quicinc.com>; Pradeep > Pragallapati (QUIC) <quic_pragalla@quicinc.com>; Sarthak Garg (QUIC) > <quic_sartgarg@quicinc.com>; Nitin Rawat (QUIC) > <quic_nitirawa@quicinc.com>; Sayali Lokhande (QUIC) > <quic_sayalil@quicinc.com> > 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
| |