diff mbox series

[v5,13/15] mmc: sdhci-cadence: Add AMD Pensando Elba SoC support

Message ID 20220613195658.5607-14-brad@pensando.io (mailing list archive)
State New, archived
Headers show
Series Support AMD Pensando Elba SoC | expand

Commit Message

Brad Larson June 13, 2022, 7:56 p.m. UTC 
From: Brad Larson <blarson@amd.com>

Add support for AMD Pensando Elba SoC which explicitly controls
byte-lane enables on writes.  Add priv_write_l() which is
used on Elba platforms for byte-lane control.

Select MMC_SDHCI_IO_ACCESSORS for MMC_SDHCI_CADENCE which
allows Elba SoC sdhci_elba_ops to overwrite the SDHCI
IO memory accessors.

Signed-off-by: Brad Larson <blarson@amd.com>
---
 drivers/mmc/host/Kconfig         |   1 +
 drivers/mmc/host/sdhci-cadence.c | 179 ++++++++++++++++++++++++++++---
 2 files changed, 166 insertions(+), 14 deletions(-)

Comments

Andy Shevchenko June 14, 2022, 11:19 a.m. UTC | #1 
On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
>
> From: Brad Larson <blarson@amd.com>
>
> Add support for AMD Pensando Elba SoC which explicitly controls
> byte-lane enables on writes.  Add priv_write_l() which is

enabling ?

> used on Elba platforms for byte-lane control.
>
> Select MMC_SDHCI_IO_ACCESSORS for MMC_SDHCI_CADENCE which
> allows Elba SoC sdhci_elba_ops to overwrite the SDHCI
> IO memory accessors.

...

> +       void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val,

priv_writel

> +                            void __iomem *reg);

And perhaps leave it on one line.

I also would swap parameters, so address goes first followed by value.

...

> +static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv,
> +                                         u32 val, void __iomem *reg)
> +{

> +       if (unlikely(priv->priv_write_l))

First of all, why if (unlikely())-else instead of if (likely())-else?

> +               priv->priv_write_l(priv, val, reg);
> +       else
> +               writel(val, reg);
> +}

Instead of branching each time you do I/O, make sure that callback is
always set and call it unconditionally. In this case you don't need to
have this callback, but maybe just a wrapper on `writel()`. As a
result you may split this to two patches in the first of which you
simply introduce a callback and a writel() wrapper which is assigned
unconditionally to all current chips. In the next you add a new chip
support.

...

> +       u32 m = (reg & 0x3);
> +       u32 msk = (0x3 << (m));
> +
> +       spin_lock_irqsave(&priv->wrlock, flags);
> +       writel(msk << 3, priv->ctl_addr);
> +       writew(val, host->ioaddr + reg);
> +       spin_unlock_irqrestore(&priv->wrlock, flags);

Too many 3:s as magic. Is it GENMASK() or something else? Perhaps it
needs a definition.

...

> +       u32 m = (reg & 0x3);
> +       u32 msk = (0x1 << (m));
> +
> +       spin_lock_irqsave(&priv->wrlock, flags);
> +       writel(msk << 3, priv->ctl_addr);
> +       writeb(val, host->ioaddr + reg);
> +       spin_unlock_irqrestore(&priv->wrlock, flags);

Ditto.

...

> +       writel(0x78, priv->ctl_addr);

Magic.

...

> +static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = {
> +       .pltfm_data = {
> +               .ops = &sdhci_cdns_ops,
> +       },
> +};
> +
> +

One blank line is enough.

...

> +       {
> +               .compatible = "amd,pensando-elba-sd4hc",
> +               .data = &sdhci_elba_drv_data

Leave a comma here.

>         },
Brad Larson July 3, 2022, 9:42 p.m. UTC | #2 
Hi Andy,

On Tue, Jun 14, 2022 at 4:19 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote:
>
> > Add support for AMD Pensando Elba SoC which explicitly controls
> > byte-lane enables on writes.  Add priv_write_l() which is
>
> enabling ?

Changed to enabling

> ...
>
> > +       void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val,
>
> priv_writel

Changed to priv_writel

>
> > +                            void __iomem *reg);
>
> And perhaps leave it on one line.
>
> I also would swap parameters, so address goes first followed by value.

Which is the reverse of writel() parameter ordering which is value,
address.  Should I do this?

> ...
>
> > +static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv,
> > +                                         u32 val, void __iomem *reg)
> > +{
>
> > +       if (unlikely(priv->priv_write_l))
>
> First of all, why if (unlikely())-else instead of if (likely())-else?
>
> > +               priv->priv_write_l(priv, val, reg);
> > +       else
> > +               writel(val, reg);
> > +}

It was existing code and never looked at it.  This construct looks to
be widely used however this goes away with the two patch approach
below.

$ find . -name \*.c | xargs grep if | grep unlikely | wc
  18640

> Instead of branching each time you do I/O, make sure that callback is
> always set and call it unconditionally. In this case you don't need to
> have this callback, but maybe just a wrapper on `writel()`. As a
> result you may split this to two patches in the first of which you
> simply introduce a callback and a writel() wrapper which is assigned
> unconditionally to all current chips. In the next you add a new chip
> support.

Next version will separate into two patches as described

> ...
>
> > +       u32 m = (reg & 0x3);
> > +       u32 msk = (0x3 << (m));
> > +
> > +       spin_lock_irqsave(&priv->wrlock, flags);
> > +       writel(msk << 3, priv->ctl_addr);
> > +       writew(val, host->ioaddr + reg);
> > +       spin_unlock_irqrestore(&priv->wrlock, flags);
>
> Too many 3:s as magic. Is it GENMASK() or something else? Perhaps it
> needs a definition.

Definitely, changed this to be understandable by inspection.
GENMASK() for word and BIT() for byte makes this more clear.  The 3's
came from bits [6:3] are the byte-lane enables in the control reg
where the lower two bits of the address specify the byte(s) to enable.

/*  Elba control reg bits [6:3] are byte-lane enables */
#define ELBA_BYTE_ENABLE_MASK(x)        ((x) << 3)

elba_priv_write_l(...):
        writel(ELBA_BYTE_ENABLE_MASK(0xf), priv->ctl_addr);
        writel(val, reg);

elba_write_w(...):
        byte_enables = GENMASK(1, 0) << (reg & 0x3);
        writel(ELBA_BYTE_ENABLE_MASK(byte_enables), priv->ctl_addr);
        writew(val, host->ioaddr + reg);

> ...
>
> > +       u32 m = (reg & 0x3);
> > +       u32 msk = (0x1 << (m));
> > +
> > +       spin_lock_irqsave(&priv->wrlock, flags);
> > +       writel(msk << 3, priv->ctl_addr);
> > +       writeb(val, host->ioaddr + reg);
> > +       spin_unlock_irqrestore(&priv->wrlock, flags);
>
> Ditto.

elba_write_b(...):
        byte_enables = BIT(0) << (reg & 0x3);
        writel(ELBA_BYTE_ENABLE_MASK(byte_enables), priv->ctl_addr);
        writeb(val, host->ioaddr + reg);

> ...
>
> > +       writel(0x78, priv->ctl_addr);
>
> Magic.

writel(ELBA_BYTE_ENABLE_MASK(0xf), priv->ctl_addr);

> ...
>
> > +static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = {
> > +       .pltfm_data = {
> > +               .ops = &sdhci_cdns_ops,
> > +       },
> > +};
> > +
> > +
>
> One blank line is enough.

Removed extra blank line

> ...
>
> > +       {
> > +               .compatible = "amd,pensando-elba-sd4hc",
> > +               .data = &sdhci_elba_drv_data
>
> Leave a comma here.

Added comma

Regards,
Brad
diff mbox series

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d6144978e32d..d0a66a74532e 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -243,6 +243,7 @@  config MMC_SDHCI_CADENCE
 	tristate "SDHCI support for the Cadence SD/SDIO/eMMC controller"
 	depends on MMC_SDHCI_PLTFM
 	depends on OF
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the Cadence SD/SDIO/eMMC driver.
 
diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 6f2de54a5987..08253357535a 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"
 
@@ -66,7 +67,12 @@  struct sdhci_cdns_phy_param {
 
 struct sdhci_cdns_priv {
 	void __iomem *hrs_addr;
+	void __iomem *ctl_addr;	/* write control */
+	spinlock_t wrlock;	/* write lock */
 	bool enhanced_strobe;
+	void (*priv_write_l)(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[];
 };
@@ -76,6 +82,11 @@  struct sdhci_cdns_phy_cfg {
 	u8 addr;
 };
 
+struct sdhci_cdns_drv_data {
+	int (*init)(struct platform_device *pdev);
+	const struct sdhci_pltfm_data pltfm_data;
+};
+
 static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
 	{ "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS, },
 	{ "cdns,phy-input-delay-legacy", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
@@ -90,6 +101,15 @@  static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
 	{ "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
 };
 
+static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv,
+					  u32 val, void __iomem *reg)
+{
+	if (unlikely(priv->priv_write_l))
+		priv->priv_write_l(priv, val, reg);
+	else
+		writel(val, reg);
+}
+
 static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
 				    u8 addr, u8 data)
 {
@@ -104,17 +124,17 @@  static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
 
 	tmp = FIELD_PREP(SDHCI_CDNS_HRS04_WDATA, data) |
 	      FIELD_PREP(SDHCI_CDNS_HRS04_ADDR, addr);
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	tmp |= SDHCI_CDNS_HRS04_WR;
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	ret = readl_poll_timeout(reg, tmp, tmp & SDHCI_CDNS_HRS04_ACK, 0, 10);
 	if (ret)
 		return ret;
 
 	tmp &= ~SDHCI_CDNS_HRS04_WR;
-	writel(tmp, reg);
+	sdhci_cdns_priv_writel(priv, tmp, reg);
 
 	ret = readl_poll_timeout(reg, tmp, !(tmp & SDHCI_CDNS_HRS04_ACK),
 				 0, 10);
@@ -191,7 +211,7 @@  static void sdhci_cdns_set_emmc_mode(struct sdhci_cdns_priv *priv, u32 mode)
 	tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
 	tmp &= ~SDHCI_CDNS_HRS06_MODE;
 	tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_MODE, mode);
-	writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+	sdhci_cdns_priv_writel(priv, tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
 }
 
 static u32 sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv)
@@ -223,7 +243,7 @@  static int sdhci_cdns_set_tune_val(struct sdhci_host *host, unsigned int val)
 	 */
 	for (i = 0; i < 2; i++) {
 		tmp |= SDHCI_CDNS_HRS06_TUNE_UP;
-		writel(tmp, reg);
+		sdhci_cdns_priv_writel(priv, tmp, reg);
 
 		ret = readl_poll_timeout(reg, tmp,
 					 !(tmp & SDHCI_CDNS_HRS06_TUNE_UP),
@@ -309,6 +329,89 @@  static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_set_uhs_signaling(host, timing);
 }
 
+/*
+ * The Pensando Elba SoC explicitly controls byte-lane enables on writes
+ * which includes writes to the HRS registers.
+ */
+static void elba_priv_write_l(struct sdhci_cdns_priv *priv, u32 val,
+			      void __iomem *reg)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->wrlock, flags);
+	writel(0x78, priv->ctl_addr);
+	writel(val, reg);
+	spin_unlock_irqrestore(&priv->wrlock, flags);
+}
+
+static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
+{
+	elba_priv_write_l(sdhci_cdns_priv(host), val, host->ioaddr + reg);
+}
+
+static void elba_write_w(struct sdhci_host *host, u16 val, int reg)
+{
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	unsigned long flags;
+	u32 m = (reg & 0x3);
+	u32 msk = (0x3 << (m));
+
+	spin_lock_irqsave(&priv->wrlock, flags);
+	writel(msk << 3, priv->ctl_addr);
+	writew(val, host->ioaddr + reg);
+	spin_unlock_irqrestore(&priv->wrlock, flags);
+}
+
+static void elba_write_b(struct sdhci_host *host, u8 val, int reg)
+{
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	unsigned long flags;
+	u32 m = (reg & 0x3);
+	u32 msk = (0x1 << (m));
+
+	spin_lock_irqsave(&priv->wrlock, flags);
+	writel(msk << 3, priv->ctl_addr);
+	writeb(val, host->ioaddr + reg);
+	spin_unlock_irqrestore(&priv->wrlock, flags);
+}
+
+static const struct sdhci_ops sdhci_elba_ops = {
+	.write_l = elba_write_l,
+	.write_w = elba_write_w,
+	.write_b = elba_write_b,
+	.set_clock = sdhci_set_clock,
+	.get_timeout_clock = sdhci_cdns_get_timeout_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
+};
+
+static int elba_drv_init(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+	struct resource *iomem;
+	void __iomem *ioaddr;
+
+	host->mmc->caps |= (MMC_CAP_1_8V_DDR | MMC_CAP_8_BIT_DATA);
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!iomem)
+		return -ENOMEM;
+
+	/* Byte-lane control register */
+	ioaddr = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(ioaddr))
+		return PTR_ERR(ioaddr);
+
+	priv->ctl_addr = ioaddr;
+	priv->priv_write_l = elba_priv_write_l;
+	spin_lock_init(&priv->wrlock);
+	writel(0x78, priv->ctl_addr);
+
+	return 0;
+}
+
 static const struct sdhci_ops sdhci_cdns_ops = {
 	.set_clock = sdhci_set_clock,
 	.get_timeout_clock = sdhci_cdns_get_timeout_clock,
@@ -318,15 +421,27 @@  static const struct sdhci_ops sdhci_cdns_ops = {
 	.set_uhs_signaling = sdhci_cdns_set_uhs_signaling,
 };
 
-static const struct sdhci_pltfm_data sdhci_cdns_uniphier_pltfm_data = {
-	.ops = &sdhci_cdns_ops,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+static const struct sdhci_cdns_drv_data sdhci_cdns_uniphier_drv_data = {
+	.pltfm_data = {
+		.ops = &sdhci_cdns_ops,
+		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	},
 };
 
-static const struct sdhci_pltfm_data sdhci_cdns_pltfm_data = {
-	.ops = &sdhci_cdns_ops,
+static const struct sdhci_cdns_drv_data sdhci_elba_drv_data = {
+	.init = elba_drv_init,
+	.pltfm_data = {
+		.ops = &sdhci_elba_ops,
+	},
 };
 
+static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = {
+	.pltfm_data = {
+		.ops = &sdhci_cdns_ops,
+	},
+};
+
+
 static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
 					     struct mmc_ios *ios)
 {
@@ -347,10 +462,26 @@  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;
-	const struct sdhci_pltfm_data *data;
+	const struct sdhci_cdns_drv_data *data;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_cdns_priv *priv;
 	struct clk *clk;
@@ -369,10 +500,10 @@  static int sdhci_cdns_probe(struct platform_device *pdev)
 
 	data = of_device_get_match_data(dev);
 	if (!data)
-		data = &sdhci_cdns_pltfm_data;
+		data = &sdhci_cdns_drv_data;
 
 	nr_phy_params = sdhci_cdns_phy_param_count(dev->of_node);
-	host = sdhci_pltfm_init(pdev, data,
+	host = sdhci_pltfm_init(pdev, &data->pltfm_data,
 				struct_size(priv, phy_params, nr_phy_params));
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
@@ -389,6 +520,11 @@  static int sdhci_cdns_probe(struct platform_device *pdev)
 	host->ioaddr += SDHCI_CDNS_SRS_BASE;
 	host->mmc_host_ops.hs400_enhanced_strobe =
 				sdhci_cdns_hs400_enhanced_strobe;
+	if (data->init) {
+		ret = data->init(pdev);
+		if (ret)
+			goto free;
+	}
 	sdhci_enable_v4_mode(host);
 	__sdhci_read_caps(host, &version, NULL, NULL);
 
@@ -404,6 +540,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;
@@ -453,7 +600,11 @@  static const struct dev_pm_ops sdhci_cdns_pm_ops = {
 static const struct of_device_id sdhci_cdns_match[] = {
 	{
 		.compatible = "socionext,uniphier-sd4hc",
-		.data = &sdhci_cdns_uniphier_pltfm_data,
+		.data = &sdhci_cdns_uniphier_drv_data,
+	},
+	{
+		.compatible = "amd,pensando-elba-sd4hc",
+		.data = &sdhci_elba_drv_data
 	},
 	{ .compatible = "cdns,sd4hc" },
 	{ /* sentinel */ }