LinuxLists.cc - [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

2018-05-22 06:43:08

Subject: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
PCIe interface. As Clock generation logic is implemented in MMCM module of
HAPS-DX platform, we have separate functions to control the MMCM to
generate required clocks with respect to speed mode. Also we have platform
specific set_power function to support different VDD of eMMC devices.

Signed-off-by: Prabu Thangamuthu <[email protected]>
---
MAINTAINERS | 7 ++
drivers/mmc/host/Makefile | 3 +-
drivers/mmc/host/sdhci-pci-core.c | 1 +
drivers/mmc/host/sdhci-pci-dwc-mshc.c | 146
++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci-pci-dwc-mshc.h | 37 +++++++++
drivers/mmc/host/sdhci-pci.h | 3 +
6 files changed, 196 insertions(+), 1 deletion(-)
create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.c
create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9051a9c..f1749c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12643,6 +12643,13 @@ S: Maintained
F: drivers/mmc/host/sdhci*
F: include/linux/mmc/sdhci*

+SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER
+M: Prabu Thangamuthu <[email protected]>
+M: Manjunath M B <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/mmc/host/sdhci-pci-dwc-mshc*
+
SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
M: Ben Dooks <[email protected]>
M: Jaehoon Chung <[email protected]>
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..6c0d3fb 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -11,7 +11,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o
obj-$(CONFIG_MMC_MXS) += mxs-mmc.o
obj-$(CONFIG_MMC_SDHCI) += sdhci.o
obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o
-sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
sdhci-pci-arasan.o
+sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
sdhci-pci-arasan.o \
+ sdhci-pci-dwc-mshc.o
obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o
obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o
obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o
diff --git a/drivers/mmc/host/sdhci-pci-core.c
b/drivers/mmc/host/sdhci-pci-core.c
index 77dd352..96b6963 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -1511,6 +1511,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
SDHCI_PCI_DEVICE(O2, SEABIRD0, o2),
SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
+ SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd),
/* Generic SD host controller */
{PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.c
b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
new file mode 100644
index 0000000..bca3db4
--- /dev/null
+++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SDHCI driver for Synopsys DWC_MSHC controller
+ *
+ * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * Authors:
+ * Prabu Thangamuthu <[email protected]>
+ * Manjunath M B <[email protected]>
+ *
+ */
+
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+
+#include "sdhci.h"
+#include "sdhci-pci.h"
+#include "sdhci-pci-dwc-mshc.h"
+
+/* Default emmc vdd is set to 3.3V */
+static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
+module_param(emmc_vdd, int, 0444);
+
+static void sdhci_snps_set_clock(struct sdhci_host *host, unsigned int
clock)
+{
+ u16 clk = 0;
+ u32 reg = 0;
+ u32 vendor_ptr = 0;
+
+ vendor_ptr = sdhci_readw(host, SDHCI_VENDOR_PTR_R);
+
+ /* Disable Software managed rx tuning */
+ reg = sdhci_readl(host, (SDHC_AT_CTRL_R + vendor_ptr));
+ reg &= ~SDHC_SW_TUNE_EN;
+ sdhci_writel(host, reg, (SDHC_AT_CTRL_R + vendor_ptr));
+
+ if (clock <= 52000000) {
+ sdhci_set_clock(host, clock);
+ } else {
+ /* Assert reset to MMCM */
+ reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
+ reg |= SDHC_CCLK_MMCM_RST;
+ sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
+
+ /* Configure MMCM*/
+ sdhci_writel(host, DIV_REG_100_MHZ, SDHC_MMCM_DIV_REG);
+ sdhci_writel(host, CLKFBOUT_100_MHZ,
+ SDHC_MMCM_CLKFBOUT);
+
+ /* De-assert reset to MMCM*/
+ reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
+ reg &= ~SDHC_CCLK_MMCM_RST;
+ sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
+
+ /* Enable clock */
+ clk = SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN |
+ SDHCI_CLOCK_CARD_EN;
+ sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+ }
+}
+
+static void sdhci_snps_set_power(struct sdhci_host *host, unsigned char
mode,
+ unsigned short vdd)
+{
+ u8 pwr = 0;
+ u16 ctrl;
+
+ if (mode != MMC_POWER_OFF) {
+ switch (1 << vdd) {
+ case MMC_VDD_165_195:
+ pwr = SDHCI_POWER_180;
+ break;
+ case MMC_VDD_29_30:
+ case MMC_VDD_30_31:
+ pwr = SDHCI_POWER_300;
+ break;
+ case MMC_VDD_32_33:
+ case MMC_VDD_33_34:
+ pwr = SDHCI_POWER_330;
+ break;
+ default:
+ WARN(1, "%s: Invalid vdd %#x\n",
+ mmc_hostname(host->mmc), vdd);
+ break;
+ }
+ }
+
+ if (host->pwr == pwr)
+ return;
+
+ host->pwr = pwr;
+
+ if (pwr == 0) {
+ sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+ } else {
+ sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
+ /*
+ * Enable it for eMMC phy cfg1 test with 1.8V mode
+ */
+ if (emmc_vdd == SDHC_EMMC_VDD_180V) {
+ pwr = SDHCI_POWER_180;
+ sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+
+ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ /*
+ * Enable 1.8V Signal Enable in the Host Control2
+ * register
+ */
+ ctrl |= SDHCI_CTRL_VDD_180;
+ sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+ }
+ pwr |= SDHCI_POWER_ON;
+
+ sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+ }
+}
+
+static int sdhci_snps_pci_probe_slot(struct sdhci_pci_slot *slot)
+{
+ struct sdhci_host *host = slot->host;
+
+ host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+ host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+
+ return 0;
+}
+
+/* Synopsys DWC MSHC Controller based on SDHCI-PCI */
+static const struct sdhci_ops sdhci_snps_ops = {
+ .set_clock = sdhci_snps_set_clock,
+ .set_power = sdhci_snps_set_power,
+ .enable_dma = sdhci_pci_enable_dma,
+ .set_bus_width = sdhci_set_bus_width,
+ .reset = sdhci_reset,
+ .set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+const struct sdhci_pci_fixes sdhci_snps = {
+ .probe_slot = sdhci_snps_pci_probe_slot,
+ .ops = &sdhci_snps_ops,
+};
+
+MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.h
b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
new file mode 100644
index 0000000..352bbfd
--- /dev/null
+++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * SDHCI driver for Synopsys DWC_MSHC controller
+ *
+ * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * Authors:
+ * Prabu Thangamuthu <[email protected]>
+ * Manjunath M B <[email protected]>
+ *
+ */
+
+#ifndef __SDHCI_PCI_DWC_MSHC_H__
+#define __SDHCI_PCI_DWC_MSHC_H__
+
+#define SDHCI_VENDOR_PTR_R 0xE8
+
+/* Module Parameters */
+#define SDHC_EMMC_VDD_330V 33
+#define SDHC_EMMC_VDD_180V 18
+
+/* Synopsys Vendor Specific Registers */
+#define SDHC_GPIO_OUT 0x34
+#define SDHC_AT_CTRL_R 0x40
+
+#define SDHC_SW_TUNE_EN 0x00000010
+
+/* MMCM DRP */
+#define SDHC_MMCM_DIV_REG 0x1020
+#define DIV_REG_100_MHZ 0x1145
+
+#define SDHC_MMCM_CLKFBOUT 0x1024
+#define CLKFBOUT_100_MHZ 0x0000
+
+#define SDHC_CCLK_MMCM_RST 0x00000001
+
+#endif
diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
index db9cb54..c91dcec 100644
--- a/drivers/mmc/host/sdhci-pci.h
+++ b/drivers/mmc/host/sdhci-pci.h
@@ -59,6 +59,7 @@
#define PCI_VENDOR_ID_ARASAN 0x16e6
#define PCI_DEVICE_ID_ARASAN_PHY_EMMC 0x0670

+#define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xC202
/*
* PCI device class and mask
*/
@@ -183,4 +184,6 @@ static inline void *sdhci_pci_priv(struct
sdhci_pci_slot *slot)

extern const struct sdhci_pci_fixes sdhci_arasan;

+extern const struct sdhci_pci_fixes sdhci_snps;
+
#endif /* __SDHCI_PCI_H */
--
1.9.1

2018-05-24 08:38:02

by Adrian Hunter

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

Hi

This patch is mangled.

On 22/05/18 09:42, Prabu Thangamuthu wrote:
> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
> PCIe interface. As Clock generation logic is implemented in MMCM module of
> HAPS-DX platform, we have separate functions to control the MMCM to
> generate required clocks with respect to speed mode. Also we have platform
> specific set_power function to support different VDD of eMMC devices.
>
> Signed-off-by: Prabu Thangamuthu <[email protected]>
> ---
> MAINTAINERS | 7 ++
> drivers/mmc/host/Makefile | 3 +-
> drivers/mmc/host/sdhci-pci-core.c | 1 +
> drivers/mmc/host/sdhci-pci-dwc-mshc.c | 146
> ++++++++++++++++++++++++++++++++++
> drivers/mmc/host/sdhci-pci-dwc-mshc.h | 37 +++++++++
> drivers/mmc/host/sdhci-pci.h | 3 +
> 6 files changed, 196 insertions(+), 1 deletion(-)
> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.c
> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9051a9c..f1749c4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12643,6 +12643,13 @@ S: Maintained
> F: drivers/mmc/host/sdhci*
> F: include/linux/mmc/sdhci*
>
> +SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER
> +M: Prabu Thangamuthu <[email protected]>
> +M: Manjunath M B <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/mmc/host/sdhci-pci-dwc-mshc*
> +
> SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
> M: Ben Dooks <[email protected]>
> M: Jaehoon Chung <[email protected]>
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 6aead24..6c0d3fb 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -11,7 +11,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o
> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o
> obj-$(CONFIG_MMC_SDHCI) += sdhci.o
> obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o
> -sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
> sdhci-pci-arasan.o
> +sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
> sdhci-pci-arasan.o \
> + sdhci-pci-dwc-mshc.o
> obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o
> obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o
> obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o
> diff --git a/drivers/mmc/host/sdhci-pci-core.c
> b/drivers/mmc/host/sdhci-pci-core.c
> index 77dd352..96b6963 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -1511,6 +1511,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
> SDHCI_PCI_DEVICE(O2, SEABIRD0, o2),
> SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
> SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
> + SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
> SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd),
> /* Generic SD host controller */
> {PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.c
> b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
> new file mode 100644
> index 0000000..bca3db4
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SDHCI driver for Synopsys DWC_MSHC controller
> + *
> + * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
> + *
> + * Authors:
> + * Prabu Thangamuthu <[email protected]>
> + * Manjunath M B <[email protected]>
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +
> +#include "sdhci.h"
> +#include "sdhci-pci.h"
> +#include "sdhci-pci-dwc-mshc.h"
> +
> +/* Default emmc vdd is set to 3.3V */
> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
> +module_param(emmc_vdd, int, 0444);

Why a module parameter?

> +
> +static void sdhci_snps_set_clock(struct sdhci_host *host, unsigned int
> clock)
> +{
> + u16 clk = 0;
> + u32 reg = 0;
> + u32 vendor_ptr = 0;
> +
> + vendor_ptr = sdhci_readw(host, SDHCI_VENDOR_PTR_R);
> +
> + /* Disable Software managed rx tuning */
> + reg = sdhci_readl(host, (SDHC_AT_CTRL_R + vendor_ptr));
> + reg &= ~SDHC_SW_TUNE_EN;
> + sdhci_writel(host, reg, (SDHC_AT_CTRL_R + vendor_ptr));
> +
> + if (clock <= 52000000) {
> + sdhci_set_clock(host, clock);
> + } else {
> + /* Assert reset to MMCM */
> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
> + reg |= SDHC_CCLK_MMCM_RST;
> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
> +
> + /* Configure MMCM*/

Space between MMCM and *

> + sdhci_writel(host, DIV_REG_100_MHZ, SDHC_MMCM_DIV_REG);
> + sdhci_writel(host, CLKFBOUT_100_MHZ,
> + SDHC_MMCM_CLKFBOUT);
> +
> + /* De-assert reset to MMCM*/

Space between MMCM and *

> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
> + reg &= ~SDHC_CCLK_MMCM_RST;
> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
> +
> + /* Enable clock */
> + clk = SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN |
> + SDHCI_CLOCK_CARD_EN;
> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> + }
> +}
> +
> +static void sdhci_snps_set_power(struct sdhci_host *host, unsigned char
> mode,
> + unsigned short vdd)
> +{
> + u8 pwr = 0;
> + u16 ctrl;
> +
> + if (mode != MMC_POWER_OFF) {
> + switch (1 << vdd) {
> + case MMC_VDD_165_195:
> + pwr = SDHCI_POWER_180;
> + break;
> + case MMC_VDD_29_30:
> + case MMC_VDD_30_31:
> + pwr = SDHCI_POWER_300;
> + break;
> + case MMC_VDD_32_33:
> + case MMC_VDD_33_34:
> + pwr = SDHCI_POWER_330;
> + break;
> + default:
> + WARN(1, "%s: Invalid vdd %#x\n",
> + mmc_hostname(host->mmc), vdd);
> + break;
> + }
> + }
> +
> + if (host->pwr == pwr)
> + return;
> +
> + host->pwr = pwr;
> +
> + if (pwr == 0) {
> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> + } else {
> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> +
> + /*
> + * Enable it for eMMC phy cfg1 test with 1.8V mode
> + */
> + if (emmc_vdd == SDHC_EMMC_VDD_180V) {
> + pwr = SDHCI_POWER_180;
> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + /*
> + * Enable 1.8V Signal Enable in the Host Control2
> + * register
> + */
> + ctrl |= SDHCI_CTRL_VDD_180;
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> + }
> + pwr |= SDHCI_POWER_ON;
> +
> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> + }
> +}
> +
> +static int sdhci_snps_pci_probe_slot(struct sdhci_pci_slot *slot)
> +{
> + struct sdhci_host *host = slot->host;
> +
> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);

Why read caps here?

> +
> + return 0;
> +}
> +
> +/* Synopsys DWC MSHC Controller based on SDHCI-PCI */
> +static const struct sdhci_ops sdhci_snps_ops = {
> + .set_clock = sdhci_snps_set_clock,
> + .set_power = sdhci_snps_set_power,
> + .enable_dma = sdhci_pci_enable_dma,
> + .set_bus_width = sdhci_set_bus_width,
> + .reset = sdhci_reset,
> + .set_uhs_signaling = sdhci_set_uhs_signaling,
> +};
> +
> +const struct sdhci_pci_fixes sdhci_snps = {
> + .probe_slot = sdhci_snps_pci_probe_slot,
> + .ops = &sdhci_snps_ops,
> +};
> +
> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.h
> b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
> new file mode 100644
> index 0000000..352bbfd
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.h

Please put the definitions into sdhci-pci-dwc-mshc.c and then this file is
not needed.

> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SDHCI driver for Synopsys DWC_MSHC controller
> + *
> + * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
> + *
> + * Authors:
> + * Prabu Thangamuthu <[email protected]>
> + * Manjunath M B <[email protected]>
> + *
> + */
> +
> +#ifndef __SDHCI_PCI_DWC_MSHC_H__
> +#define __SDHCI_PCI_DWC_MSHC_H__
> +
> +#define SDHCI_VENDOR_PTR_R 0xE8
> +
> +/* Module Parameters */
> +#define SDHC_EMMC_VDD_330V 33
> +#define SDHC_EMMC_VDD_180V 18
> +
> +/* Synopsys Vendor Specific Registers */
> +#define SDHC_GPIO_OUT 0x34
> +#define SDHC_AT_CTRL_R 0x40
> +
> +#define SDHC_SW_TUNE_EN 0x00000010
> +
> +/* MMCM DRP */
> +#define SDHC_MMCM_DIV_REG 0x1020
> +#define DIV_REG_100_MHZ 0x1145
> +
> +#define SDHC_MMCM_CLKFBOUT 0x1024
> +#define CLKFBOUT_100_MHZ 0x0000
> +
> +#define SDHC_CCLK_MMCM_RST 0x00000001
> +
> +#endif
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index db9cb54..c91dcec 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -59,6 +59,7 @@
> #define PCI_VENDOR_ID_ARASAN 0x16e6
> #define PCI_DEVICE_ID_ARASAN_PHY_EMMC 0x0670
>
> +#define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xC202
> /*
> * PCI device class and mask
> */
> @@ -183,4 +184,6 @@ static inline void *sdhci_pci_priv(struct
> sdhci_pci_slot *slot)
>
> extern const struct sdhci_pci_fixes sdhci_arasan;
>
> +extern const struct sdhci_pci_fixes sdhci_snps;
> +
> #endif /* __SDHCI_PCI_H */
>

2018-05-24 11:29:51

by Prabu Thangamuthu

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

Hi Adrian,

On 5/24/2018 2:06 PM, Adrian Hunter wrote:
> Hi
>
> This patch is mangled.
We will check it.
>
> On 22/05/18 09:42, Prabu Thangamuthu wrote:
>> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
>> PCIe interface. As Clock generation logic is implemented in MMCM module of
>> HAPS-DX platform, we have separate functions to control the MMCM to
>> generate required clocks with respect to speed mode. Also we have platform
>> specific set_power function to support different VDD of eMMC devices.
>>
>> Signed-off-by: Prabu Thangamuthu <[email protected]>
>> ---
>> MAINTAINERS | 7 ++
>> drivers/mmc/host/Makefile | 3 +-
>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>> drivers/mmc/host/sdhci-pci-dwc-mshc.c | 146
>> ++++++++++++++++++++++++++++++++++
>> drivers/mmc/host/sdhci-pci-dwc-mshc.h | 37 +++++++++
>> drivers/mmc/host/sdhci-pci.h | 3 +
>> 6 files changed, 196 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.c
>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9051a9c..f1749c4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12643,6 +12643,13 @@ S: Maintained
>> F: drivers/mmc/host/sdhci*
>> F: include/linux/mmc/sdhci*
>>
>> +SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER
>> +M: Prabu Thangamuthu <[email protected]>
>> +M: Manjunath M B <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/mmc/host/sdhci-pci-dwc-mshc*
>> +
>> SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
>> M: Ben Dooks <[email protected]>
>> M: Jaehoon Chung <[email protected]>
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 6aead24..6c0d3fb 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -11,7 +11,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o
>> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o
>> obj-$(CONFIG_MMC_SDHCI) += sdhci.o
>> obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o
>> -sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>> sdhci-pci-arasan.o
>> +sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>> sdhci-pci-arasan.o \
>> + sdhci-pci-dwc-mshc.o
>> obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o
>> obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o
>> obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c
>> b/drivers/mmc/host/sdhci-pci-core.c
>> index 77dd352..96b6963 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -1511,6 +1511,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>> SDHCI_PCI_DEVICE(O2, SEABIRD0, o2),
>> SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
>> SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
>> + SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
>> SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd),
>> /* Generic SD host controller */
>> {PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
>> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>> b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>> new file mode 100644
>> index 0000000..bca3db4
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>> @@ -0,0 +1,146 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SDHCI driver for Synopsys DWC_MSHC controller
>> + *
>> + * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
>> + *
>> + * Authors:
>> + * Prabu Thangamuthu <[email protected]>
>> + * Manjunath M B <[email protected]>
>> + *
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +
>> +#include "sdhci.h"
>> +#include "sdhci-pci.h"
>> +#include "sdhci-pci-dwc-mshc.h"
>> +
>> +/* Default emmc vdd is set to 3.3V */
>> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
>> +module_param(emmc_vdd, int, 0444);
> Why a module parameter?
Our platform has slots for eMMC devices which can supports both 1.8 V
and 3.3 V
operating modes. We want this module parameter to control the operating
voltage
of eMMC devices.
>> +
>> +static void sdhci_snps_set_clock(struct sdhci_host *host, unsigned int
>> clock)
>> +{
>> + u16 clk = 0;
>> + u32 reg = 0;
>> + u32 vendor_ptr = 0;
>> +
>> + vendor_ptr = sdhci_readw(host, SDHCI_VENDOR_PTR_R);
>> +
>> + /* Disable Software managed rx tuning */
>> + reg = sdhci_readl(host, (SDHC_AT_CTRL_R + vendor_ptr));
>> + reg &= ~SDHC_SW_TUNE_EN;
>> + sdhci_writel(host, reg, (SDHC_AT_CTRL_R + vendor_ptr));
>> +
>> + if (clock <= 52000000) {
>> + sdhci_set_clock(host, clock);
>> + } else {
>> + /* Assert reset to MMCM */
>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>> + reg |= SDHC_CCLK_MMCM_RST;
>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>> +
>> + /* Configure MMCM*/
> Space between MMCM and *
Okay.
>
>> + sdhci_writel(host, DIV_REG_100_MHZ, SDHC_MMCM_DIV_REG);
>> + sdhci_writel(host, CLKFBOUT_100_MHZ,
>> + SDHC_MMCM_CLKFBOUT);
>> +
>> + /* De-assert reset to MMCM*/
> Space between MMCM and *
Okay.
>
>
>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>> + reg &= ~SDHC_CCLK_MMCM_RST;
>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>> +
>> + /* Enable clock */
>> + clk = SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN |
>> + SDHCI_CLOCK_CARD_EN;
>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> + }
>> +}
>> +
>> +static void sdhci_snps_set_power(struct sdhci_host *host, unsigned char
>> mode,
>> + unsigned short vdd)
>> +{
>> + u8 pwr = 0;
>> + u16 ctrl;
>> +
>> + if (mode != MMC_POWER_OFF) {
>> + switch (1 << vdd) {
>> + case MMC_VDD_165_195:
>> + pwr = SDHCI_POWER_180;
>> + break;
>> + case MMC_VDD_29_30:
>> + case MMC_VDD_30_31:
>> + pwr = SDHCI_POWER_300;
>> + break;
>> + case MMC_VDD_32_33:
>> + case MMC_VDD_33_34:
>> + pwr = SDHCI_POWER_330;
>> + break;
>> + default:
>> + WARN(1, "%s: Invalid vdd %#x\n",
>> + mmc_hostname(host->mmc), vdd);
>> + break;
>> + }
>> + }
>> +
>> + if (host->pwr == pwr)
>> + return;
>> +
>> + host->pwr = pwr;
>> +
>> + if (pwr == 0) {
>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>> + } else {
>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>> +
>> + /*
>> + * Enable it for eMMC phy cfg1 test with 1.8V mode
>> + */
>> + if (emmc_vdd == SDHC_EMMC_VDD_180V) {
>> + pwr = SDHCI_POWER_180;
>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>> +
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> + /*
>> + * Enable 1.8V Signal Enable in the Host Control2
>> + * register
>> + */
>> + ctrl |= SDHCI_CTRL_VDD_180;
>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> + }
>> + pwr |= SDHCI_POWER_ON;
>> +
>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>> + }
>> +}
>> +
>> +static int sdhci_snps_pci_probe_slot(struct sdhci_pci_slot *slot)
>> +{
>> + struct sdhci_host *host = slot->host;
>> +
>> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
>> + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> Why read caps here?
We had it for logging purpose. we will remove it.
>
>> +
>> + return 0;
>> +}
>> +
>> +/* Synopsys DWC MSHC Controller based on SDHCI-PCI */
>> +static const struct sdhci_ops sdhci_snps_ops = {
>> + .set_clock = sdhci_snps_set_clock,
>> + .set_power = sdhci_snps_set_power,
>> + .enable_dma = sdhci_pci_enable_dma,
>> + .set_bus_width = sdhci_set_bus_width,
>> + .reset = sdhci_reset,
>> + .set_uhs_signaling = sdhci_set_uhs_signaling,
>> +};
>> +
>> +const struct sdhci_pci_fixes sdhci_snps = {
>> + .probe_slot = sdhci_snps_pci_probe_slot,
>> + .ops = &sdhci_snps_ops,
>> +};
>> +
>> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
>> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.h
>> b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
>> new file mode 100644
>> index 0000000..352bbfd
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
> Please put the definitions into sdhci-pci-dwc-mshc.c and then this file is
> not needed.
Okay.

Thanks for review comments.

Regards,
Prabu.

2018-05-24 12:14:26

by Adrian Hunter

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

On 24/05/18 14:28, Prabu Thangamuthu wrote:
> Hi Adrian,
>
> On 5/24/2018 2:06 PM, Adrian Hunter wrote:
>> Hi
>>
>> This patch is mangled.
> We will check it.
>>
>> On 22/05/18 09:42, Prabu Thangamuthu wrote:
>>> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
>>> PCIe interface. As Clock generation logic is implemented in MMCM module of
>>> HAPS-DX platform, we have separate functions to control the MMCM to
>>> generate required clocks with respect to speed mode. Also we have platform
>>> specific set_power function to support different VDD of eMMC devices.
>>>
>>> Signed-off-by: Prabu Thangamuthu <[email protected]>
>>> ---
>>> MAINTAINERS | 7 ++
>>> drivers/mmc/host/Makefile | 3 +-
>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>> drivers/mmc/host/sdhci-pci-dwc-mshc.c | 146
>>> ++++++++++++++++++++++++++++++++++
>>> drivers/mmc/host/sdhci-pci-dwc-mshc.h | 37 +++++++++
>>> drivers/mmc/host/sdhci-pci.h | 3 +
>>> 6 files changed, 196 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 9051a9c..f1749c4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -12643,6 +12643,13 @@ S: Maintained
>>> F: drivers/mmc/host/sdhci*
>>> F: include/linux/mmc/sdhci*
>>>
>>> +SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER
>>> +M: Prabu Thangamuthu <[email protected]>
>>> +M: Manjunath M B <[email protected]>
>>> +L: [email protected]
>>> +S: Maintained
>>> +F: drivers/mmc/host/sdhci-pci-dwc-mshc*
>>> +
>>> SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
>>> M: Ben Dooks <[email protected]>
>>> M: Jaehoon Chung <[email protected]>
>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>> index 6aead24..6c0d3fb 100644
>>> --- a/drivers/mmc/host/Makefile
>>> +++ b/drivers/mmc/host/Makefile
>>> @@ -11,7 +11,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o
>>> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o
>>> obj-$(CONFIG_MMC_SDHCI) += sdhci.o
>>> obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o
>>> -sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>>> sdhci-pci-arasan.o
>>> +sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>>> sdhci-pci-arasan.o \
>>> + sdhci-pci-dwc-mshc.o
>>> obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o
>>> obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o
>>> obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c
>>> b/drivers/mmc/host/sdhci-pci-core.c
>>> index 77dd352..96b6963 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -1511,6 +1511,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>>> SDHCI_PCI_DEVICE(O2, SEABIRD0, o2),
>>> SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
>>> SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
>>> + SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
>>> SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd),
>>> /* Generic SD host controller */
>>> {PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
>>> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>> b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>> new file mode 100644
>>> index 0000000..bca3db4
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>> @@ -0,0 +1,146 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * SDHCI driver for Synopsys DWC_MSHC controller
>>> + *
>>> + * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
>>> + *
>>> + * Authors:
>>> + * Prabu Thangamuthu <[email protected]>
>>> + * Manjunath M B <[email protected]>
>>> + *
>>> + */
>>> +
>>> +#include <linux/pci.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +
>>> +#include "sdhci.h"
>>> +#include "sdhci-pci.h"
>>> +#include "sdhci-pci-dwc-mshc.h"
>>> +
>>> +/* Default emmc vdd is set to 3.3V */
>>> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
>>> +module_param(emmc_vdd, int, 0444);
>> Why a module parameter?
> Our platform has slots for eMMC devices which can supports both 1.8 V
> and 3.3 V
> operating modes. We want this module parameter to control the operating
> voltage
> of eMMC devices.

So why not use firmware device properties?

>>> +
>>> +static void sdhci_snps_set_clock(struct sdhci_host *host, unsigned int
>>> clock)
>>> +{
>>> + u16 clk = 0;
>>> + u32 reg = 0;
>>> + u32 vendor_ptr = 0;
>>> +
>>> + vendor_ptr = sdhci_readw(host, SDHCI_VENDOR_PTR_R);
>>> +
>>> + /* Disable Software managed rx tuning */
>>> + reg = sdhci_readl(host, (SDHC_AT_CTRL_R + vendor_ptr));
>>> + reg &= ~SDHC_SW_TUNE_EN;
>>> + sdhci_writel(host, reg, (SDHC_AT_CTRL_R + vendor_ptr));
>>> +
>>> + if (clock <= 52000000) {
>>> + sdhci_set_clock(host, clock);
>>> + } else {
>>> + /* Assert reset to MMCM */
>>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>>> + reg |= SDHC_CCLK_MMCM_RST;
>>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>>> +
>>> + /* Configure MMCM*/
>> Space between MMCM and *
> Okay.
>>
>>> + sdhci_writel(host, DIV_REG_100_MHZ, SDHC_MMCM_DIV_REG);
>>> + sdhci_writel(host, CLKFBOUT_100_MHZ,
>>> + SDHC_MMCM_CLKFBOUT);
>>> +
>>> + /* De-assert reset to MMCM*/
>> Space between MMCM and *
> Okay.
>>
>>
>>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>>> + reg &= ~SDHC_CCLK_MMCM_RST;
>>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>>> +
>>> + /* Enable clock */
>>> + clk = SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN |
>>> + SDHCI_CLOCK_CARD_EN;
>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>> + }
>>> +}
>>> +
>>> +static void sdhci_snps_set_power(struct sdhci_host *host, unsigned char
>>> mode,
>>> + unsigned short vdd)
>>> +{
>>> + u8 pwr = 0;
>>> + u16 ctrl;
>>> +
>>> + if (mode != MMC_POWER_OFF) {
>>> + switch (1 << vdd) {
>>> + case MMC_VDD_165_195:
>>> + pwr = SDHCI_POWER_180;
>>> + break;
>>> + case MMC_VDD_29_30:
>>> + case MMC_VDD_30_31:
>>> + pwr = SDHCI_POWER_300;
>>> + break;
>>> + case MMC_VDD_32_33:
>>> + case MMC_VDD_33_34:
>>> + pwr = SDHCI_POWER_330;
>>> + break;
>>> + default:
>>> + WARN(1, "%s: Invalid vdd %#x\n",
>>> + mmc_hostname(host->mmc), vdd);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (host->pwr == pwr)
>>> + return;
>>> +
>>> + host->pwr = pwr;
>>> +
>>> + if (pwr == 0) {
>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>> + } else {
>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>> +
>>> + /*
>>> + * Enable it for eMMC phy cfg1 test with 1.8V mode
>>> + */
>>> + if (emmc_vdd == SDHC_EMMC_VDD_180V) {
>>> + pwr = SDHCI_POWER_180;
>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>>> +
>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> + /*
>>> + * Enable 1.8V Signal Enable in the Host Control2
>>> + * register
>>> + */
>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> + }
>>> + pwr |= SDHCI_POWER_ON;
>>> +
>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>>> + }
>>> +}
>>> +
>>> +static int sdhci_snps_pci_probe_slot(struct sdhci_pci_slot *slot)
>>> +{
>>> + struct sdhci_host *host = slot->host;
>>> +
>>> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
>>> + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>> Why read caps here?
> We had it for logging purpose. we will remove it.

To prevent 3.0V or 3.3V clear the corresponding capabilities bits. There
are DT bindings for overriding the capabilities.

To use only 1.8V signaling clear SDHCI_SIGNALING_330 from host->flags.

Then you shouldn't need sdhci_snps_set_power().

>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/* Synopsys DWC MSHC Controller based on SDHCI-PCI */
>>> +static const struct sdhci_ops sdhci_snps_ops = {
>>> + .set_clock = sdhci_snps_set_clock,
>>> + .set_power = sdhci_snps_set_power,
>>> + .enable_dma = sdhci_pci_enable_dma,
>>> + .set_bus_width = sdhci_set_bus_width,
>>> + .reset = sdhci_reset,
>>> + .set_uhs_signaling = sdhci_set_uhs_signaling,
>>> +};
>>> +
>>> +const struct sdhci_pci_fixes sdhci_snps = {
>>> + .probe_slot = sdhci_snps_pci_probe_slot,
>>> + .ops = &sdhci_snps_ops,
>>> +};
>>> +
>>> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
>>> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.h
>>> b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
>>> new file mode 100644
>>> index 0000000..352bbfd
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.h
>> Please put the definitions into sdhci-pci-dwc-mshc.c and then this file is
>> not needed.
> Okay.
>
> Thanks for review comments.
>
> Regards,
> Prabu.
>

2018-05-28 12:11:26

by Prabu Thangamuthu

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

Hi Adrian,

On 5/24/2018 5:43 PM, Adrian Hunter wrote:
> On 24/05/18 14:28, Prabu Thangamuthu wrote:
>> Hi Adrian,
>>
>> On 5/24/2018 2:06 PM, Adrian Hunter wrote:
>>> Hi
>>>
>>> This patch is mangled.
>> We will check it.
>>> On 22/05/18 09:42, Prabu Thangamuthu wrote:
>>>> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
>>>> PCIe interface. As Clock generation logic is implemented in MMCM module of
>>>> HAPS-DX platform, we have separate functions to control the MMCM to
>>>> generate required clocks with respect to speed mode. Also we have platform
>>>> specific set_power function to support different VDD of eMMC devices.
>>>>
>>>> Signed-off-by: Prabu Thangamuthu <[email protected]>
>>>> ---
>>>> MAINTAINERS | 7 ++
>>>> drivers/mmc/host/Makefile | 3 +-
>>>> drivers/mmc/host/sdhci-pci-core.c | 1 +
>>>> drivers/mmc/host/sdhci-pci-dwc-mshc.c | 146
>>>> ++++++++++++++++++++++++++++++++++
>>>> drivers/mmc/host/sdhci-pci-dwc-mshc.h | 37 +++++++++
>>>> drivers/mmc/host/sdhci-pci.h | 3 +
>>>> 6 files changed, 196 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>>> create mode 100644 drivers/mmc/host/sdhci-pci-dwc-mshc.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 9051a9c..f1749c4 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -12643,6 +12643,13 @@ S: Maintained
>>>> F: drivers/mmc/host/sdhci*
>>>> F: include/linux/mmc/sdhci*
>>>>
>>>> +SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER
>>>> +M: Prabu Thangamuthu <[email protected]>
>>>> +M: Manjunath M B <[email protected]>
>>>> +L: [email protected]
>>>> +S: Maintained
>>>> +F: drivers/mmc/host/sdhci-pci-dwc-mshc*
>>>> +
>>>> SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
>>>> M: Ben Dooks <[email protected]>
>>>> M: Jaehoon Chung <[email protected]>
>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>> index 6aead24..6c0d3fb 100644
>>>> --- a/drivers/mmc/host/Makefile
>>>> +++ b/drivers/mmc/host/Makefile
>>>> @@ -11,7 +11,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o
>>>> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o
>>>> obj-$(CONFIG_MMC_SDHCI) += sdhci.o
>>>> obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o
>>>> -sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>>>> sdhci-pci-arasan.o
>>>> +sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o
>>>> sdhci-pci-arasan.o \
>>>> + sdhci-pci-dwc-mshc.o
>>>> obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o
>>>> obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o
>>>> obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o
>>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c
>>>> b/drivers/mmc/host/sdhci-pci-core.c
>>>> index 77dd352..96b6963 100644
>>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>>> @@ -1511,6 +1511,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>>>> SDHCI_PCI_DEVICE(O2, SEABIRD0, o2),
>>>> SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
>>>> SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
>>>> + SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
>>>> SDHCI_PCI_DEVICE_CLASS(AMD, SYSTEM_SDHCI, PCI_CLASS_MASK, amd),
>>>> /* Generic SD host controller */
>>>> {PCI_DEVICE_CLASS(SYSTEM_SDHCI, PCI_CLASS_MASK)},
>>>> diff --git a/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>>> b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>>> new file mode 100644
>>>> index 0000000..bca3db4
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/host/sdhci-pci-dwc-mshc.c
>>>> @@ -0,0 +1,146 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * SDHCI driver for Synopsys DWC_MSHC controller
>>>> + *
>>>> + * Copyright (C) 2018 Synopsys, Inc. (http://www.synopsys.com)
>>>> + *
>>>> + * Authors:
>>>> + * Prabu Thangamuthu <[email protected]>
>>>> + * Manjunath M B <[email protected]>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/pci.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/moduleparam.h>
>>>> +
>>>> +#include "sdhci.h"
>>>> +#include "sdhci-pci.h"
>>>> +#include "sdhci-pci-dwc-mshc.h"
>>>> +
>>>> +/* Default emmc vdd is set to 3.3V */
>>>> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
>>>> +module_param(emmc_vdd, int, 0444);
>>> Why a module parameter?
>> Our platform has slots for eMMC devices which can supports both 1.8 V
>> and 3.3 V
>> operating modes. We want this module parameter to control the operating
>> voltage
>> of eMMC devices.
> So why not use firmware device properties?
We are assuming as you are referring EXT_CSD register of eMMC device to
understand 1.8V/3.3V support. But we need to fix the voltage before
enumeration of the eMMC device.
Please help us to understand more if you are not referring EXT_CSD register.
>
>>>> +
>>>> +static void sdhci_snps_set_clock(struct sdhci_host *host, unsigned int
>>>> clock)
>>>> +{
>>>> + u16 clk = 0;
>>>> + u32 reg = 0;
>>>> + u32 vendor_ptr = 0;
>>>> +
>>>> + vendor_ptr = sdhci_readw(host, SDHCI_VENDOR_PTR_R);
>>>> +
>>>> + /* Disable Software managed rx tuning */
>>>> + reg = sdhci_readl(host, (SDHC_AT_CTRL_R + vendor_ptr));
>>>> + reg &= ~SDHC_SW_TUNE_EN;
>>>> + sdhci_writel(host, reg, (SDHC_AT_CTRL_R + vendor_ptr));
>>>> +
>>>> + if (clock <= 52000000) {
>>>> + sdhci_set_clock(host, clock);
>>>> + } else {
>>>> + /* Assert reset to MMCM */
>>>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>>>> + reg |= SDHC_CCLK_MMCM_RST;
>>>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>>>> +
>>>> + /* Configure MMCM*/
>>> Space between MMCM and *
>> Okay.
>>>> + sdhci_writel(host, DIV_REG_100_MHZ, SDHC_MMCM_DIV_REG);
>>>> + sdhci_writel(host, CLKFBOUT_100_MHZ,
>>>> + SDHC_MMCM_CLKFBOUT);
>>>> +
>>>> + /* De-assert reset to MMCM*/
>>> Space between MMCM and *
>> Okay.
>>>
>>>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr));
>>>> + reg &= ~SDHC_CCLK_MMCM_RST;
>>>> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr));
>>>> +
>>>> + /* Enable clock */
>>>> + clk = SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN |
>>>> + SDHCI_CLOCK_CARD_EN;
>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>> + }
>>>> +}
>>>> +
>>>> +static void sdhci_snps_set_power(struct sdhci_host *host, unsigned char
>>>> mode,
>>>> + unsigned short vdd)
>>>> +{
>>>> + u8 pwr = 0;
>>>> + u16 ctrl;
>>>> +
>>>> + if (mode != MMC_POWER_OFF) {
>>>> + switch (1 << vdd) {
>>>> + case MMC_VDD_165_195:
>>>> + pwr = SDHCI_POWER_180;
>>>> + break;
>>>> + case MMC_VDD_29_30:
>>>> + case MMC_VDD_30_31:
>>>> + pwr = SDHCI_POWER_300;
>>>> + break;
>>>> + case MMC_VDD_32_33:
>>>> + case MMC_VDD_33_34:
>>>> + pwr = SDHCI_POWER_330;
>>>> + break;
>>>> + default:
>>>> + WARN(1, "%s: Invalid vdd %#x\n",
>>>> + mmc_hostname(host->mmc), vdd);
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + if (host->pwr == pwr)
>>>> + return;
>>>> +
>>>> + host->pwr = pwr;
>>>> +
>>>> + if (pwr == 0) {
>>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>>> + } else {
>>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>>> +
>>>> + /*
>>>> + * Enable it for eMMC phy cfg1 test with 1.8V mode
>>>> + */
>>>> + if (emmc_vdd == SDHC_EMMC_VDD_180V) {
>>>> + pwr = SDHCI_POWER_180;
>>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>>>> +
>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> + /*
>>>> + * Enable 1.8V Signal Enable in the Host Control2
>>>> + * register
>>>> + */
>>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> + }
>>>> + pwr |= SDHCI_POWER_ON;
>>>> +
>>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>>>> + }
>>>> +}
>>>> +
>>>> +static int sdhci_snps_pci_probe_slot(struct sdhci_pci_slot *slot)
>>>> +{
>>>> + struct sdhci_host *host = slot->host;
>>>> +
>>>> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
>>>> + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>> Why read caps here?
>> We had it for logging purpose. we will remove it.
> To prevent 3.0V or 3.3V clear the corresponding capabilities bits. There
> are DT bindings for overriding the capabilities.
>
> To use only 1.8V signaling clear SDHCI_SIGNALING_330 from host->flags.
>
> Then you shouldn't need sdhci_snps_set_power().
Agree your point. we will test with this approach and share the patches.

Thanks,
Prabu

2018-05-28 12:14:02

by Prabu Thangamuthu

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

2018-05-28 12:20:24

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

On Tue, May 22, 2018 at 9:42 AM, Prabu Thangamuthu <[email protected]> wrote:
> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
> PCIe interface. As Clock generation logic is implemented in MMCM module of
> HAPS-DX platform, we have separate functions to control the MMCM to
> generate required clocks with respect to speed mode. Also we have platform
> specific set_power function to support different VDD of eMMC devices.

> + * Authors:
> + * Prabu Thangamuthu <[email protected]>
> + * Manjunath M B <[email protected]>
> + *

Redundant last line in above excerpt.

> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>

> +#include <linux/moduleparam.h>

This one is effectively included by module.h.

> +/* Default emmc vdd is set to 3.3V */
> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
> +module_param(emmc_vdd, int, 0444);

This looks weird. Why do you need a module parameter?

> + u16 clk = 0;
> + u32 reg = 0;
> + u32 vendor_ptr = 0;

Why do above have assignments?

> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
This better to be near to the parameter declaration. Though, as I said
above, it feels wrong to have it in the first place.

> +/* Synopsys Vendor Specific Registers */
> +#define SDHC_GPIO_OUT 0x34
> +#define SDHC_AT_CTRL_R 0x40
> +
> +#define SDHC_SW_TUNE_EN 0x00000010
> +
> +/* MMCM DRP */
> +#define SDHC_MMCM_DIV_REG 0x1020
> +#define DIV_REG_100_MHZ 0x1145
> +
> +#define SDHC_MMCM_CLKFBOUT 0x1024
> +#define CLKFBOUT_100_MHZ 0x0000
> +
> +#define SDHC_CCLK_MMCM_RST 0x00000001

Do you need these all in the header file? Why?

> +#define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xC202

Absent blank line.
Broken style of the value.

Who knows what else wrong with this patch? Please, pay attention to
the details and check twice before submit.

--
With Best Regards,
Andy Shevchenko

2018-05-28 13:05:54

by Prabu Thangamuthu

[permalink] [raw]

Subject: Re: [PATCH 1/1] mmc: sdhci-pci-dwc-mshc: synopsys dwc mshc support

Hi Andy,

Thanks for pointing all. We will fix it.

Regards,
Prabu

On 5/28/2018 5:49 PM, Andy Shevchenko wrote:
> On Tue, May 22, 2018 at 9:42 AM, Prabu Thangamuthu <[email protected]> wrote:
>> To enable Synopsys DWC MSHC controller on HPAS-DX platform connected using
>> PCIe interface. As Clock generation logic is implemented in MMCM module of
>> HAPS-DX platform, we have separate functions to control the MMCM to
>> generate required clocks with respect to speed mode. Also we have platform
>> specific set_power function to support different VDD of eMMC devices.
>> + * Authors:
>> + * Prabu Thangamuthu <[email protected]>
>> + * Manjunath M B <[email protected]>
>> + *
> Redundant last line in above excerpt.
>
>> +#include <linux/pci.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
> This one is effectively included by module.h.
>
>> +/* Default emmc vdd is set to 3.3V */
>> +static unsigned int emmc_vdd = SDHC_EMMC_VDD_330V;
>> +module_param(emmc_vdd, int, 0444);
> This looks weird. Why do you need a module parameter?
>
>> + u16 clk = 0;
>> + u32 reg = 0;
>> + u32 vendor_ptr = 0;
> Why do above have assignments?
>
>> +MODULE_PARM_DESC(emmc_vdd, "VDD to configure eMMC device supply voltage");
> This better to be near to the parameter declaration. Though, as I said
> above, it feels wrong to have it in the first place.
>
>
>> +/* Synopsys Vendor Specific Registers */
>> +#define SDHC_GPIO_OUT 0x34
>> +#define SDHC_AT_CTRL_R 0x40
>> +
>> +#define SDHC_SW_TUNE_EN 0x00000010
>> +
>> +/* MMCM DRP */
>> +#define SDHC_MMCM_DIV_REG 0x1020
>> +#define DIV_REG_100_MHZ 0x1145
>> +
>> +#define SDHC_MMCM_CLKFBOUT 0x1024
>> +#define CLKFBOUT_100_MHZ 0x0000
>> +
>> +#define SDHC_CCLK_MMCM_RST 0x00000001
> Do you need these all in the header file? Why?
>
>
>> +#define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xC202
> Absent blank line.
> Broken style of the value.
>
> Who knows what else wrong with this patch? Please, pay attention to
> the details and check twice before submit.
>