| Message ID | 20220820195750.70861-18-brad@pensando.io |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
Hi Brad, On Sa, 2022-08-20 at 12:57 -0700, Brad Larson wrote: [...] > +static void sdhci_mmc_hw_reset(struct mmc_host *mmc) > +{ > + struct sdhci_host *host = mmc_priv(mmc); > + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); > + > + dev_info(mmc_dev(host->mmc), "emmc hardware reset\n"); > + > + reset_control_assert(priv->rst_hw); > + /* For eMMC, minimum is 1us but give it 9us for good measure */ > + udelay(9); At a glance, this seems excessive. Is there a reason 9 us is better than, say, 2 or 3? [...] > @@ -520,6 +538,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev) > if (ret) > goto free; > > > > > + if (host->mmc->caps & MMC_CAP_HW_RESET) { > + priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); This should be described in cdns,sdhci.yaml first. regards Philipp
On Sat, 20 Aug 2022 at 21:58, Brad Larson <brad@pensando.io> wrote: > > From: Brad Larson <blarson@amd.com> > > Add support for mmc hardware reset with a reset-controller > which would need to be enabled in the device tree with > a supporting driver. The default is disabled for all > existing designs. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > drivers/mmc/host/sdhci-cadence.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c > index c662c63d49fa..35d37b9aba63 100644 > --- a/drivers/mmc/host/sdhci-cadence.c > +++ b/drivers/mmc/host/sdhci-cadence.c > @@ -12,6 +12,7 @@ > #include <linux/mmc/mmc.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/reset.h> > > #include "sdhci-pltfm.h" > > @@ -70,6 +71,7 @@ struct sdhci_cdns_priv { > spinlock_t wrlock; /* write lock */ > bool enhanced_strobe; > void (*priv_writel)(struct sdhci_cdns_priv *priv, u32 val, void __iomem *reg); > + struct reset_control *rst_hw; > unsigned int nr_phy_params; > struct sdhci_cdns_phy_param phy_params[]; > }; > @@ -458,6 +460,22 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc, > SDHCI_CDNS_HRS06_MODE_MMC_HS400); > } > > +static void sdhci_mmc_hw_reset(struct mmc_host *mmc) Nitpick: Probably better to be consistent with the prefixes for function names. So, I suggest changing this to "sdhci_cdns_mmc_hw_reset". > +{ > + struct sdhci_host *host = mmc_priv(mmc); > + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); > + > + dev_info(mmc_dev(host->mmc), "emmc hardware reset\n"); Maybe it's sufficient with dev_dbg? > + > + reset_control_assert(priv->rst_hw); > + /* For eMMC, minimum is 1us but give it 9us for good measure */ > + udelay(9); > + > + reset_control_deassert(priv->rst_hw); > + /* For eMMC, minimum is 200us but give it 300us for good measure */ > + usleep_range(300, 1000); > +} > + > static int sdhci_cdns_probe(struct platform_device *pdev) > { > struct sdhci_host *host; > @@ -520,6 +538,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev) > if (ret) > goto free; > > + if (host->mmc->caps & MMC_CAP_HW_RESET) { > + priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); > + if (IS_ERR(priv->rst_hw)) { > + ret = PTR_ERR(priv->rst_hw); > + if (ret == -ENOENT) > + priv->rst_hw = NULL; > + } else { > + host->mmc_host_ops.card_hw_reset = sdhci_mmc_hw_reset; > + } > + } > + > ret = sdhci_add_host(host); > if (ret) > goto free; > -- Other than the comments above, I wonder about what merging strategy we should use for the series. I believe it looks fine for me to pick up the mmc related patches, thus we can apply patches on a per subsystem basis, right? Kind regards Uffe
On Mon, Aug 22, 2022 at 12:53:22PM +0200, Ulf Hansson wrote: > Other than the comments above, I wonder about what merging strategy we > should use for the series. I believe it looks fine for me to pick up > the mmc related patches, thus we can apply patches on a per subsystem > basis, right? Yes, if there's no relationship between the different subsystem components (which looks like the case?) they should probably just go separately - they can probably be submitted separately too.
On 8/22/22 12:03 AM, Philipp Zabel wrote: > Hi Brad, > > On Sa, 2022-08-20 at 12:57 -0700, Brad Larson wrote: > [...] >> +static void sdhci_mmc_hw_reset(struct mmc_host *mmc) >> +{ >> + struct sdhci_host *host = mmc_priv(mmc); >> + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); >> + >> + dev_info(mmc_dev(host->mmc), "emmc hardware reset\n"); >> + >> + reset_control_assert(priv->rst_hw); >> + /* For eMMC, minimum is 1us but give it 9us for good measure */ >> + udelay(9); > At a glance, this seems excessive. Is there a reason 9 us is better > than, say, 2 or 3? Yes, 3x the minimum should be fine. Changed to 3 usec. > [...] >> @@ -520,6 +538,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev) >> if (ret) >> goto free; >> >> >> >> >> + if (host->mmc->caps & MMC_CAP_HW_RESET) { >> + priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); > This should be described in cdns,sdhci.yaml first. Adding this to cdns,sdhci.yaml and running through schema checker. --- a/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml +++ b/Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml @@ -114,6 +114,16 @@ properties: minimum: 0 maximum: 0x7f + reset-names: + items: + - const: hw + + resets: + description: + optional. phandle to the system reset controller with line index + for mmc hw reset line if exists. + maxItems: 1 + required: - compatible Regards, Brad
On 8/22/22 3:53 AM, Ulf Hansson wrote: > On Sat, 20 Aug 2022 at 21:58, Brad Larson <brad@pensando.io> wrote: >> From: Brad Larson <blarson@amd.com> >> >> Add support for mmc hardware reset with a reset-controller >> which would need to be enabled in the device tree with >> a supporting driver. The default is disabled for all >> existing designs. >> >> Signed-off-by: Brad Larson <blarson@amd.com> >> --- >> drivers/mmc/host/sdhci-cadence.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c >> index c662c63d49fa..35d37b9aba63 100644 >> --- a/drivers/mmc/host/sdhci-cadence.c >> +++ b/drivers/mmc/host/sdhci-cadence.c >> @@ -12,6 +12,7 @@ >> #include <linux/mmc/mmc.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> +#include <linux/reset.h> >> >> #include "sdhci-pltfm.h" >> >> @@ -70,6 +71,7 @@ struct sdhci_cdns_priv { >> spinlock_t wrlock; /* write lock */ >> bool enhanced_strobe; >> void (*priv_writel)(struct sdhci_cdns_priv *priv, u32 val, void __iomem *reg); >> + struct reset_control *rst_hw; >> unsigned int nr_phy_params; >> struct sdhci_cdns_phy_param phy_params[]; >> }; >> @@ -458,6 +460,22 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc, >> SDHCI_CDNS_HRS06_MODE_MMC_HS400); >> } >> >> +static void sdhci_mmc_hw_reset(struct mmc_host *mmc) > Nitpick: Probably better to be consistent with the prefixes for > function names. So, I suggest changing this to > "sdhci_cdns_mmc_hw_reset". Changing to sdhci_cdns_mmc_hw_reset(). >> +{ >> + struct sdhci_host *host = mmc_priv(mmc); >> + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); >> + >> + dev_info(mmc_dev(host->mmc), "emmc hardware reset\n"); > Maybe it's sufficient with dev_dbg? Changing to dev_dbg(). >> + >> + reset_control_assert(priv->rst_hw); >> + /* For eMMC, minimum is 1us but give it 9us for good measure */ >> + udelay(9); >> + >> + reset_control_deassert(priv->rst_hw); >> + /* For eMMC, minimum is 200us but give it 300us for good measure */ >> + usleep_range(300, 1000); >> +} >> + >> static int sdhci_cdns_probe(struct platform_device *pdev) >> { >> struct sdhci_host *host; >> @@ -520,6 +538,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev) >> if (ret) >> goto free; >> >> + if (host->mmc->caps & MMC_CAP_HW_RESET) { >> + priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); >> + if (IS_ERR(priv->rst_hw)) { >> + ret = PTR_ERR(priv->rst_hw); >> + if (ret == -ENOENT) >> + priv->rst_hw = NULL; >> + } else { >> + host->mmc_host_ops.card_hw_reset = sdhci_mmc_hw_reset; >> + } >> + } >> + >> ret = sdhci_add_host(host); >> if (ret) >> goto free; >> -- > Other than the comments above, I wonder about what merging strategy we > should use for the series. I believe it looks fine for me to pick up > the mmc related patches, thus we can apply patches on a per subsystem > basis, right? Yes I think so and I'll be looking for guidance on this. Regards, Brad
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c index c662c63d49fa..35d37b9aba63 100644 --- a/drivers/mmc/host/sdhci-cadence.c +++ b/drivers/mmc/host/sdhci-cadence.c @@ -12,6 +12,7 @@ #include <linux/mmc/mmc.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/reset.h> #include "sdhci-pltfm.h" @@ -70,6 +71,7 @@ struct sdhci_cdns_priv { spinlock_t wrlock; /* write lock */ bool enhanced_strobe; void (*priv_writel)(struct sdhci_cdns_priv *priv, u32 val, void __iomem *reg); + struct reset_control *rst_hw; unsigned int nr_phy_params; struct sdhci_cdns_phy_param phy_params[]; }; @@ -458,6 +460,22 @@ static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc, SDHCI_CDNS_HRS06_MODE_MMC_HS400); } +static void sdhci_mmc_hw_reset(struct mmc_host *mmc) +{ + struct sdhci_host *host = mmc_priv(mmc); + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host); + + dev_info(mmc_dev(host->mmc), "emmc hardware reset\n"); + + reset_control_assert(priv->rst_hw); + /* For eMMC, minimum is 1us but give it 9us for good measure */ + udelay(9); + + reset_control_deassert(priv->rst_hw); + /* For eMMC, minimum is 200us but give it 300us for good measure */ + usleep_range(300, 1000); +} + static int sdhci_cdns_probe(struct platform_device *pdev) { struct sdhci_host *host; @@ -520,6 +538,17 @@ static int sdhci_cdns_probe(struct platform_device *pdev) if (ret) goto free; + if (host->mmc->caps & MMC_CAP_HW_RESET) { + priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); + if (IS_ERR(priv->rst_hw)) { + ret = PTR_ERR(priv->rst_hw); + if (ret == -ENOENT) + priv->rst_hw = NULL; + } else { + host->mmc_host_ops.card_hw_reset = sdhci_mmc_hw_reset; + } + } + ret = sdhci_add_host(host); if (ret) goto free;