Re: [PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support
On Tue, Mar 30, 2021 at 7:31 PM Ulf Hansson wrote:
>
> + Masahiro Yamada (main author of the driver)
>
> On Mon, 29 Mar 2021 at 03:59, Brad Larson wrote:
> >
> > Add support for Pensando Elba SoC which explicitly controls
> > byte-lane enables on writes. Refactor to allow platform
> > specific write ops.
> >
> > Signed-off-by: Brad Larson
> > ---
> > drivers/mmc/host/Kconfig | 15 +++
> > drivers/mmc/host/Makefile | 1 +
> > drivers/mmc/host/sdhci-cadence-elba.c | 137 ++
>
> By looking at the amount of code changes that seem to be needed to
> support the Pensando Elba variant, I don't think it's necessary to
> split this into a separate file.
>
> Unless Yamada-san has a different opinion, I would rather just stick
> with using sdhci-cadence.c.
I agree with Ulf.
BTW, this patch cannot probe
"pensando,elba-emmc"
when CONFIG_MMC_SDHCI_CADENCE_ELBA=m.
>
> > drivers/mmc/host/sdhci-cadence.c | 81 ---
> > drivers/mmc/host/sdhci-cadence.h | 68 +
> > 5 files changed, 260 insertions(+), 42 deletions(-)
> > create mode 100644 drivers/mmc/host/sdhci-cadence-elba.c
> > create mode 100644 drivers/mmc/host/sdhci-cadence.h
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index b236dfe2e879..65ea323c06f2 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -250,6 +250,21 @@ config MMC_SDHCI_CADENCE
> >
> > If unsure, say N.
> >
> > +config MMC_SDHCI_CADENCE_ELBA
> > + tristate "SDHCI support for the Pensando/Cadence SD/SDIO/eMMC
> > controller"
> > + depends on ARCH_PENSANDO_ELBA_SOC
> > + depends on MMC_SDHCI
> > + depends on OF
> > + depends on MMC_SDHCI_CADENCE
> > + depends on MMC_SDHCI_PLTFM
> > + select MMC_SDHCI_IO_ACCESSORS
>
> According to the comment above - then you should probably just extend
> the conditions for when building MMC_SDHCI_CADENCE, rather than having
> to add a new Kconfig for "*_ELBA".
>
> > + help
> > + This selects the Pensando/Cadence SD/SDIO/eMMC controller.
> > +
> > + If you have a controller with this interface, say Y or M here.
> > +
> > + If unsure, say N.
> > +
> > config MMC_SDHCI_CNS3XXX
> > tristate "SDHCI support on the Cavium Networks CNS3xxx SoC"
> > depends on ARCH_CNS3XXX || COMPILE_TEST
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 6df5c4774260..f2a6d50e64de 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -80,6 +80,7 @@ obj-$(CONFIG_MMC_REALTEK_USB) += rtsx_usb_sdmmc.o
> >
> > obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
> > obj-$(CONFIG_MMC_SDHCI_CADENCE)+= sdhci-cadence.o
> > +obj-$(CONFIG_MMC_SDHCI_CADENCE_ELBA) += sdhci-cadence-elba.o
> > obj-$(CONFIG_MMC_SDHCI_CNS3XXX)+= sdhci-cns3xxx.o
> > obj-$(CONFIG_MMC_SDHCI_ESDHC_MCF) += sdhci-esdhc-mcf.o
> > obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
> > diff --git a/drivers/mmc/host/sdhci-cadence-elba.c
> > b/drivers/mmc/host/sdhci-cadence-elba.c
> > new file mode 100644
> > index ..ec23f43de407
> > --- /dev/null
> > +++ b/drivers/mmc/host/sdhci-cadence-elba.c
> > @@ -0,0 +1,137 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2020 Pensando Systems, Inc.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "sdhci-pltfm.h"
> > +#include "sdhci-cadence.h"
> > +
> > +// delay regs address
>
> Please don't use "//" when adding comments, but instead "/* ... */".
> This applies to several more places of the patch.
>
> > +#define SDIO_REG_HRS4 0x10
This is the same as SDHCI_CDNS_HRS04
> > +#define REG_DELAY_HS 0x00
This is the same as SDHCI_CDNS_PHY_DLY_SD_HS
> > +#define REG_DELAY_DEFAULT 0x01
This is the same as SDHCI_CDNS_PHY_DLY_SD_DEFAULT
> > +#define REG_DELAY_UHSI_SDR50 0x04
This is the same as SDHCI_CDNS_PHY_DLY_UHS_SDR50
> > +#define REG_DELAY_UHSI_DDR50 0x05
This is the same as SDHCI_CDNS_PHY_DLY_UHS_DDR50
> > +
> > +static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
> > +{
> > + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&priv->wrlock, flags);
> > + writel(0x78, priv->ctl_addr);
> > + writel(val, host->ioaddr + reg);
> > + spin_unlock_irqrestore(&priv->wrlock, flags);
> > +}
> > +
> > +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, p
Re: [PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support
+ Masahiro Yamada (main author of the driver)
On Mon, 29 Mar 2021 at 03:59, Brad Larson wrote:
>
> Add support for Pensando Elba SoC which explicitly controls
> byte-lane enables on writes. Refactor to allow platform
> specific write ops.
>
> Signed-off-by: Brad Larson
> ---
> drivers/mmc/host/Kconfig | 15 +++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/sdhci-cadence-elba.c | 137 ++
By looking at the amount of code changes that seem to be needed to
support the Pensando Elba variant, I don't think it's necessary to
split this into a separate file.
Unless Yamada-san has a different opinion, I would rather just stick
with using sdhci-cadence.c.
> drivers/mmc/host/sdhci-cadence.c | 81 ---
> drivers/mmc/host/sdhci-cadence.h | 68 +
> 5 files changed, 260 insertions(+), 42 deletions(-)
> create mode 100644 drivers/mmc/host/sdhci-cadence-elba.c
> create mode 100644 drivers/mmc/host/sdhci-cadence.h
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index b236dfe2e879..65ea323c06f2 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -250,6 +250,21 @@ config MMC_SDHCI_CADENCE
>
> If unsure, say N.
>
> +config MMC_SDHCI_CADENCE_ELBA
> + tristate "SDHCI support for the Pensando/Cadence SD/SDIO/eMMC
> controller"
> + depends on ARCH_PENSANDO_ELBA_SOC
> + depends on MMC_SDHCI
> + depends on OF
> + depends on MMC_SDHCI_CADENCE
> + depends on MMC_SDHCI_PLTFM
> + select MMC_SDHCI_IO_ACCESSORS
According to the comment above - then you should probably just extend
the conditions for when building MMC_SDHCI_CADENCE, rather than having
to add a new Kconfig for "*_ELBA".
> + help
> + This selects the Pensando/Cadence SD/SDIO/eMMC controller.
> +
> + If you have a controller with this interface, say Y or M here.
> +
> + If unsure, say N.
> +
> config MMC_SDHCI_CNS3XXX
> tristate "SDHCI support on the Cavium Networks CNS3xxx SoC"
> depends on ARCH_CNS3XXX || COMPILE_TEST
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 6df5c4774260..f2a6d50e64de 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_MMC_REALTEK_USB) += rtsx_usb_sdmmc.o
>
> obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
> obj-$(CONFIG_MMC_SDHCI_CADENCE)+= sdhci-cadence.o
> +obj-$(CONFIG_MMC_SDHCI_CADENCE_ELBA) += sdhci-cadence-elba.o
> obj-$(CONFIG_MMC_SDHCI_CNS3XXX)+= sdhci-cns3xxx.o
> obj-$(CONFIG_MMC_SDHCI_ESDHC_MCF) += sdhci-esdhc-mcf.o
> obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
> diff --git a/drivers/mmc/host/sdhci-cadence-elba.c
> b/drivers/mmc/host/sdhci-cadence-elba.c
> new file mode 100644
> index ..ec23f43de407
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-cadence-elba.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Pensando Systems, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "sdhci-pltfm.h"
> +#include "sdhci-cadence.h"
> +
> +// delay regs address
Please don't use "//" when adding comments, but instead "/* ... */".
This applies to several more places of the patch.
> +#define SDIO_REG_HRS4 0x10
> +#define REG_DELAY_HS 0x00
> +#define REG_DELAY_DEFAULT 0x01
> +#define REG_DELAY_UHSI_SDR50 0x04
> +#define REG_DELAY_UHSI_DDR50 0x05
> +
> +static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
> +{
> + struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->wrlock, flags);
> + writel(0x78, priv->ctl_addr);
> + writel(val, host->ioaddr + reg);
> + spin_unlock_irqrestore(&priv->wrlock, flags);
> +}
> +
> +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 void elba_priv_write_l(struct sdhci_cdns_priv *priv,
> + u32 val, void __
[PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support
Add support for Pensando Elba SoC which explicitly controls
byte-lane enables on writes. Refactor to allow platform
specific write ops.
Signed-off-by: Brad Larson
---
drivers/mmc/host/Kconfig | 15 +++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-cadence-elba.c | 137 ++
drivers/mmc/host/sdhci-cadence.c | 81 ---
drivers/mmc/host/sdhci-cadence.h | 68 +
5 files changed, 260 insertions(+), 42 deletions(-)
create mode 100644 drivers/mmc/host/sdhci-cadence-elba.c
create mode 100644 drivers/mmc/host/sdhci-cadence.h
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index b236dfe2e879..65ea323c06f2 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -250,6 +250,21 @@ config MMC_SDHCI_CADENCE
If unsure, say N.
+config MMC_SDHCI_CADENCE_ELBA
+ tristate "SDHCI support for the Pensando/Cadence SD/SDIO/eMMC
controller"
+ depends on ARCH_PENSANDO_ELBA_SOC
+ depends on MMC_SDHCI
+ depends on OF
+ depends on MMC_SDHCI_CADENCE
+ depends on MMC_SDHCI_PLTFM
+ select MMC_SDHCI_IO_ACCESSORS
+ help
+ This selects the Pensando/Cadence SD/SDIO/eMMC controller.
+
+ If you have a controller with this interface, say Y or M here.
+
+ If unsure, say N.
+
config MMC_SDHCI_CNS3XXX
tristate "SDHCI support on the Cavium Networks CNS3xxx SoC"
depends on ARCH_CNS3XXX || COMPILE_TEST
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6df5c4774260..f2a6d50e64de 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_MMC_REALTEK_USB) += rtsx_usb_sdmmc.o
obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
obj-$(CONFIG_MMC_SDHCI_CADENCE)+= sdhci-cadence.o
+obj-$(CONFIG_MMC_SDHCI_CADENCE_ELBA) += sdhci-cadence-elba.o
obj-$(CONFIG_MMC_SDHCI_CNS3XXX)+= sdhci-cns3xxx.o
obj-$(CONFIG_MMC_SDHCI_ESDHC_MCF) += sdhci-esdhc-mcf.o
obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
diff --git a/drivers/mmc/host/sdhci-cadence-elba.c
b/drivers/mmc/host/sdhci-cadence-elba.c
new file mode 100644
index ..ec23f43de407
--- /dev/null
+++ b/drivers/mmc/host/sdhci-cadence-elba.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Pensando Systems, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sdhci-pltfm.h"
+#include "sdhci-cadence.h"
+
+// delay regs address
+#define SDIO_REG_HRS4 0x10
+#define REG_DELAY_HS 0x00
+#define REG_DELAY_DEFAULT 0x01
+#define REG_DELAY_UHSI_SDR50 0x04
+#define REG_DELAY_UHSI_DDR50 0x05
+
+static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
+{
+ struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->wrlock, flags);
+ writel(0x78, priv->ctl_addr);
+ writel(val, host->ioaddr + reg);
+ spin_unlock_irqrestore(&priv->wrlock, flags);
+}
+
+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 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 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 void sd4_set_dlyvr(struct sdhci_host *host,
+ unsigned char addr, unsigned char data)
+{
+ unsigned long dlyrv_reg;
+
+ dlyrv_reg = ((unsigned long)data << 8);
+ dlyrv_reg |= addr;
+
+ // set data and address
+ writel(dlyrv_reg, host->ioa
