linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V5 4/4] mmc: host: sdhci-pci: Add Genesys Logic GL975x support
@ 2019-08-20  2:07 Ben Chuang
  2019-08-21 12:29 ` Adrian Hunter
  0 siblings, 1 reply; 3+ messages in thread
From: Ben Chuang @ 2019-08-20  2:07 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, johnsonm, ben.chuang, Ben Chuang

From: Ben Chuang <ben.chuang@genesyslogic.com.tw>

Add support for the GL9750 and GL9755 chipsets.

The patches enable v4 mode and wait 5ms after set 1.8V signal enable for
GL9750/GL9755. It fixed the value of SDHCI_MAX_CURRENT register and uses
the vendor tuning flow for GL9750.

Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
Signed-off-by: Michael K Johnson <johnsonm@danlj.org>
---
 drivers/mmc/host/Makefile         |   2 +-
 drivers/mmc/host/sdhci-pci-core.c |   2 +
 drivers/mmc/host/sdhci-pci-gli.c  | 381 ++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci-pci.h      |   5 +
 4 files changed, 389 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/host/sdhci-pci-gli.c

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 73578718f119..661445415090 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -13,7 +13,7 @@ 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-dwc-mshc.o
+				   sdhci-pci-dwc-mshc.o sdhci-pci-gli.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 4154ee11b47d..e5835fbf73bc 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -1682,6 +1682,8 @@ static const struct pci_device_id pci_ids[] = {
 	SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
 	SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
 	SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
+	SDHCI_PCI_DEVICE(GLI, 9750, gl9750),
+	SDHCI_PCI_DEVICE(GLI, 9755, gl9755),
 	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-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
new file mode 100644
index 000000000000..99abb7830e62
--- /dev/null
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Genesys Logic, Inc.
+ *
+ * Authors: Ben Chuang <ben.chuang@genesyslogic.com.tw>
+ *
+ * Version: v0.9.0 (2019-08-08)
+ */
+
+#include <linux/pci.h>
+#include <linux/mmc/mmc.h>
+#include <linux/delay.h>
+#include "sdhci.h"
+#include "sdhci-pci.h"
+
+/*  Genesys Logic extra registers */
+#define SDHCI_GLI_9750_WT         0x800
+#define SDHCI_GLI_9750_DRIVING    0x860
+#define SDHCI_GLI_9750_PLL        0x864
+#define SDHCI_GLI_9750_SW_CTRL    0x874
+#define SDHCI_GLI_9750_MISC       0x878
+
+#define SDHCI_GLI_9750_TUNING_CONTROL		0x540
+#define SDHCI_GLI_9750_TUNING_PARAMETERS	0x544
+
+#define GLI_MAX_TUNING_LOOP 40
+
+/* Genesys Logic chipset */
+static void gli_set_9750(struct sdhci_host *host)
+{
+	u32 wt_value = 0;
+	u32 driving_value = 0;
+	u32 pll_value = 0;
+	u32 sw_ctrl_value = 0;
+	u32 misc_value = 0;
+	u32 parameter_value = 0;
+	u32 control_value = 0;
+
+	u16 ctrl2 = 0;
+
+	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
+	if ((wt_value & 0x1) == 0) {
+		wt_value |= 0x1;
+		sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
+	}
+
+	driving_value = sdhci_readl(host, SDHCI_GLI_9750_DRIVING);
+	pll_value = sdhci_readl(host, SDHCI_GLI_9750_PLL);
+	sw_ctrl_value = sdhci_readl(host, SDHCI_GLI_9750_SW_CTRL);
+	misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
+	parameter_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_PARAMETERS);
+	control_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_CONTROL);
+
+	driving_value &= ~(0x0C000FFF);
+	driving_value |= 0x0C000FFF;
+	sdhci_writel(host, driving_value, SDHCI_GLI_9750_DRIVING);
+
+	sw_ctrl_value |= 0xc0;
+	sdhci_writel(host, sw_ctrl_value, SDHCI_GLI_9750_SW_CTRL);
+
+	// reset the tuning flow after reinit and before starting tuning
+	pll_value |= 0x800000; // bit23-1
+	pll_value &= ~(0x00700000); // bit22:20-0
+
+	misc_value &= ~(0x8); // bit3-0
+	misc_value &= ~(0x4); // bit2-0
+
+	misc_value &= ~(0x70); // bit6:4-0
+	misc_value |= 0x50; // bit6:4-5
+
+	parameter_value &= ~(0x7); // bit2:0-0
+	parameter_value |= 0x1; // bit2:0-1
+
+	control_value &= ~(0x190000); // bit20:19-0, bit16-0
+	control_value |=   0x110000; // bit20:19-b10, bit16-1
+
+	sdhci_writel(host, pll_value, SDHCI_GLI_9750_PLL);
+	sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
+
+	// disable tuned clk
+	ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+	ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
+	sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
+
+	// 540 enable tuning parameters control
+	control_value |= 0x10;
+	sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
+
+	// write 544 tuning parameters
+	sdhci_writel(host, parameter_value, SDHCI_GLI_9750_TUNING_PARAMETERS);
+
+	// 540 disable tuning parameters control
+	control_value &= ~0x10;
+	sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
+
+	// clear tuned clk
+	ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+	ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
+	sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
+
+	udelay(1);
+	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
+	wt_value &= ~0x1;
+	sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
+}
+
+static void sdhci_gli_do_reset(struct sdhci_host *host, u8 mask)
+{
+	host->ops->reset(host, mask);
+
+	if (mask & SDHCI_RESET_ALL) {
+		if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
+			if (host->ops->enable_dma)
+				host->ops->enable_dma(host);
+		}
+
+		/* Resetting the controller clears many */
+		host->preset_enabled = false;
+	}
+}
+
+static void sdhci_gli_abort_tuning(struct sdhci_host *host, u32 opcode)
+{
+	sdhci_reset_tuning(host);
+
+	sdhci_gli_do_reset(host, SDHCI_RESET_CMD);
+	sdhci_gli_do_reset(host, SDHCI_RESET_DATA);
+
+	sdhci_end_tuning(host);
+
+	mmc_abort_tuning(host->mmc, opcode);
+}
+
+static void gli_set_9750_rx_inv(struct sdhci_host *host, bool b)
+{
+	u32 wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
+	u32 misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
+
+	if ((wt_value & 0x1) == 0) {
+		wt_value |= 0x1;
+		sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
+	}
+
+	misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
+	if (b) {
+		misc_value |= 0x8;
+		sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
+	} else {
+		misc_value &= ~0x8;
+		sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
+	}
+
+	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
+	wt_value &= ~0x1;
+	sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
+}
+
+static int __sdhci_execute_tuning_9750(struct sdhci_host *host, u32 opcode)
+{
+	int i;
+	int rx_inv = 0;
+
+	for (rx_inv = 0; rx_inv < 2; rx_inv++) {
+		if (rx_inv & 0x1)
+			gli_set_9750_rx_inv(host, true);
+		else
+			gli_set_9750_rx_inv(host, false);
+
+		sdhci_start_tuning(host);
+
+		for (i = 0; i < GLI_MAX_TUNING_LOOP; i++) {
+			u16 ctrl;
+
+			sdhci_send_tuning(host, opcode);
+
+			if (!host->tuning_done) {
+				if (rx_inv == 1) {
+					pr_info("%s: Tuning timeout, falling back to fixed sampling clock\n",
+						mmc_hostname(host->mmc));
+					sdhci_gli_abort_tuning(host, opcode);
+					return -ETIMEDOUT;
+				}
+				pr_info("%s: Tuning timeout, try next tuning\n",
+					mmc_hostname(host->mmc));
+				sdhci_gli_abort_tuning(host, opcode);
+				break;
+			}
+
+			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+			if (!(ctrl & SDHCI_CTRL_EXEC_TUNING)) {
+				if (ctrl & SDHCI_CTRL_TUNED_CLK)
+					return 0; /* Success! */
+				break;
+			}
+		}
+	}
+
+	pr_info("%s: Tuning failed, falling back to fixed sampling clock\n",
+		mmc_hostname(host->mmc));
+	sdhci_reset_tuning(host);
+	return -EAGAIN;
+}
+
+static int gl9750_execute_tuning(struct mmc_host *mmc, u32 opcode)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	int err = 0;
+	unsigned int tuning_count = 0;
+	bool hs400_tuning;
+
+	hs400_tuning = host->flags & SDHCI_HS400_TUNING;
+
+	if (host->tuning_mode == SDHCI_TUNING_MODE_1)
+		tuning_count = host->tuning_count;
+
+	/*
+	 * The Host Controller needs tuning in case of SDR104 and DDR50
+	 * mode, and for SDR50 mode when Use Tuning for SDR50 is set in
+	 * the Capabilities register.
+	 * If the Host Controller supports the HS200 mode then the
+	 * tuning function has to be executed.
+	 */
+	switch (host->timing) {
+	/* HS400 tuning is done in HS200 mode */
+	case MMC_TIMING_MMC_HS400:
+		err = -EINVAL;
+		goto out;
+
+	case MMC_TIMING_MMC_HS200:
+		/*
+		 * Periodic re-tuning for HS400 is not expected to be needed, so
+		 * disable it here.
+		 */
+		if (hs400_tuning)
+			tuning_count = 0;
+		break;
+
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_UHS_DDR50:
+		break;
+	case MMC_TIMING_UHS_SDR50:
+		if (host->flags & SDHCI_SDR50_NEEDS_TUNING)
+			break;
+		/* FALLTHROUGH */
+
+	default:
+		goto out;
+	}
+
+	if (host->ops->platform_execute_tuning) {
+		err = host->ops->platform_execute_tuning(host, opcode);
+		goto out;
+	}
+
+	host->mmc->retune_period = tuning_count;
+
+	if (host->tuning_delay < 0)
+		host->tuning_delay = opcode == MMC_SEND_TUNING_BLOCK;
+
+	gli_set_9750(host);
+	host->tuning_err = __sdhci_execute_tuning_9750(host, opcode);
+
+	sdhci_end_tuning(host);
+out:
+	host->flags &= ~SDHCI_HS400_TUNING;
+
+	return err;
+}
+
+static int gli_probe_slot_gl9750(struct sdhci_pci_slot *slot)
+{
+	struct sdhci_host *host = slot->host;
+	struct mmc_host_ops *ops = &host->mmc_host_ops;
+
+	slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
+	sdhci_enable_v4_mode(host);
+
+	ops->execute_tuning = gl9750_execute_tuning;
+
+	return 0;
+}
+
+static int gli_probe_slot_gl9755(struct sdhci_pci_slot *slot)
+{
+	struct sdhci_host *host = slot->host;
+
+	slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
+	sdhci_enable_v4_mode(host);
+
+	return 0;
+}
+
+static void sdhci_gli_voltage_switch(struct sdhci_host *host)
+{
+	usleep_range(5000, 5500);
+}
+
+static void sdhci_gli_pci_hw_reset(struct sdhci_host *host)
+{
+	struct sdhci_pci_slot *slot = sdhci_priv(host);
+
+	if (slot->hw_reset)
+		slot->hw_reset(host);
+}
+
+static void sdhci_gl9750_reset(struct sdhci_host *host, u8 mask)
+{
+	ktime_t timeout;
+
+	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
+
+	if (mask & SDHCI_RESET_ALL)
+		host->clock = 0;
+
+	/* Wait max 100 ms */
+	timeout = ktime_add_ms(ktime_get(), 100);
+
+	/* hw clears the bit when it's done */
+	while (1) {
+		bool timedout = ktime_after(ktime_get(), timeout);
+
+		if (!(sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask))
+			break;
+		if (timedout) {
+			pr_err("%s: Reset 0x%x never completed.\n",
+				mmc_hostname(host->mmc), (int)mask);
+			sdhci_dumpregs(host);
+			return;
+		}
+		udelay(10);
+	}
+	gli_set_9750(host);
+}
+
+static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
+{
+	u32 value;
+
+	value = readl(host->ioaddr + reg);
+	if (unlikely(reg == SDHCI_MAX_CURRENT)) {
+		if (!(value & 0xff))
+			value |= 0xc8;
+	}
+	return value;
+}
+
+static const struct sdhci_ops sdhci_gl9755_ops = {
+	.set_clock		= sdhci_set_clock,
+	.enable_dma		= sdhci_pci_enable_dma,
+	.set_bus_width		= sdhci_set_bus_width,
+	.reset			= sdhci_reset,
+	.set_uhs_signaling	= sdhci_set_uhs_signaling,
+	.hw_reset		= sdhci_gli_pci_hw_reset,
+	.voltage_switch		= sdhci_gli_voltage_switch,
+};
+
+const struct sdhci_pci_fixes sdhci_gl9755 = {
+	.quirks		= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
+	.quirks2	= SDHCI_QUIRK2_BROKEN_DDR50,
+	.probe_slot	= gli_probe_slot_gl9755,
+	.ops            = &sdhci_gl9755_ops,
+};
+
+static const struct sdhci_ops sdhci_gl9750_ops = {
+	.read_l                 = sdhci_gl9750_readl,
+	.set_clock		= sdhci_set_clock,
+	.enable_dma		= sdhci_pci_enable_dma,
+	.set_bus_width		= sdhci_set_bus_width,
+	.reset			= sdhci_gl9750_reset,
+	.set_uhs_signaling	= sdhci_set_uhs_signaling,
+	.hw_reset		= sdhci_gli_pci_hw_reset,
+	.voltage_switch		= sdhci_gli_voltage_switch,
+};
+
+const struct sdhci_pci_fixes sdhci_gl9750 = {
+	.quirks		= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
+	.quirks2	= SDHCI_QUIRK2_BROKEN_DDR50,
+	.probe_slot	= gli_probe_slot_gl9750,
+	.ops            = &sdhci_gl9750_ops,
+};
+
diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
index e5dc6e44c7a4..738ba5afcc20 100644
--- a/drivers/mmc/host/sdhci-pci.h
+++ b/drivers/mmc/host/sdhci-pci.h
@@ -65,6 +65,9 @@
 
 #define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xc202
 
+#define PCI_DEVICE_ID_GLI_9755		0x9755
+#define PCI_DEVICE_ID_GLI_9750		0x9750
+
 /*
  * PCI device class and mask
  */
@@ -185,5 +188,7 @@ int sdhci_pci_enable_dma(struct sdhci_host *host);
 extern const struct sdhci_pci_fixes sdhci_arasan;
 extern const struct sdhci_pci_fixes sdhci_snps;
 extern const struct sdhci_pci_fixes sdhci_o2;
+extern const struct sdhci_pci_fixes sdhci_gl9750;
+extern const struct sdhci_pci_fixes sdhci_gl9755;
 
 #endif /* __SDHCI_PCI_H */
-- 
2.22.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH V5 4/4] mmc: host: sdhci-pci: Add Genesys Logic GL975x support
  2019-08-20  2:07 [PATCH V5 4/4] mmc: host: sdhci-pci: Add Genesys Logic GL975x support Ben Chuang
@ 2019-08-21 12:29 ` Adrian Hunter
  2019-08-22  2:26   ` Ben Chuang
  0 siblings, 1 reply; 3+ messages in thread
From: Adrian Hunter @ 2019-08-21 12:29 UTC (permalink / raw)
  To: Ben Chuang, ulf.hansson; +Cc: linux-mmc, linux-kernel, johnsonm, ben.chuang

On 20/08/19 5:07 AM, Ben Chuang wrote:
> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> 
> Add support for the GL9750 and GL9755 chipsets.
> 
> The patches enable v4 mode and wait 5ms after set 1.8V signal enable for
> GL9750/GL9755. It fixed the value of SDHCI_MAX_CURRENT register and uses
> the vendor tuning flow for GL9750.
> 
> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
> Signed-off-by: Michael K Johnson <johnsonm@danlj.org>
> ---
>  drivers/mmc/host/Makefile         |   2 +-
>  drivers/mmc/host/sdhci-pci-core.c |   2 +
>  drivers/mmc/host/sdhci-pci-gli.c  | 381 ++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci-pci.h      |   5 +
>  4 files changed, 389 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mmc/host/sdhci-pci-gli.c
> 
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 73578718f119..661445415090 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -13,7 +13,7 @@ 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-dwc-mshc.o
> +				   sdhci-pci-dwc-mshc.o sdhci-pci-gli.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 4154ee11b47d..e5835fbf73bc 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -1682,6 +1682,8 @@ static const struct pci_device_id pci_ids[] = {
>  	SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
>  	SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
>  	SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
> +	SDHCI_PCI_DEVICE(GLI, 9750, gl9750),
> +	SDHCI_PCI_DEVICE(GLI, 9755, gl9755),
>  	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-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> new file mode 100644
> index 000000000000..99abb7830e62
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -0,0 +1,381 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Genesys Logic, Inc.
> + *
> + * Authors: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> + *
> + * Version: v0.9.0 (2019-08-08)
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/delay.h>
> +#include "sdhci.h"
> +#include "sdhci-pci.h"
> +
> +/*  Genesys Logic extra registers */
> +#define SDHCI_GLI_9750_WT         0x800
> +#define SDHCI_GLI_9750_DRIVING    0x860
> +#define SDHCI_GLI_9750_PLL        0x864
> +#define SDHCI_GLI_9750_SW_CTRL    0x874
> +#define SDHCI_GLI_9750_MISC       0x878
> +
> +#define SDHCI_GLI_9750_TUNING_CONTROL		0x540
> +#define SDHCI_GLI_9750_TUNING_PARAMETERS	0x544
> +
> +#define GLI_MAX_TUNING_LOOP 40
> +
> +/* Genesys Logic chipset */
> +static void gli_set_9750(struct sdhci_host *host)
> +{
> +	u32 wt_value = 0;
> +	u32 driving_value = 0;
> +	u32 pll_value = 0;
> +	u32 sw_ctrl_value = 0;
> +	u32 misc_value = 0;
> +	u32 parameter_value = 0;
> +	u32 control_value = 0;
> +
> +	u16 ctrl2 = 0;
> +
> +	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> +	if ((wt_value & 0x1) == 0) {
> +		wt_value |= 0x1;
> +		sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> +	}
> +
> +	driving_value = sdhci_readl(host, SDHCI_GLI_9750_DRIVING);
> +	pll_value = sdhci_readl(host, SDHCI_GLI_9750_PLL);
> +	sw_ctrl_value = sdhci_readl(host, SDHCI_GLI_9750_SW_CTRL);
> +	misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> +	parameter_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_PARAMETERS);
> +	control_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_CONTROL);
> +
> +	driving_value &= ~(0x0C000FFF);
> +	driving_value |= 0x0C000FFF;
> +	sdhci_writel(host, driving_value, SDHCI_GLI_9750_DRIVING);
> +
> +	sw_ctrl_value |= 0xc0;
> +	sdhci_writel(host, sw_ctrl_value, SDHCI_GLI_9750_SW_CTRL);
> +
> +	// reset the tuning flow after reinit and before starting tuning

For consistent style, please use C-style comments /* */ rather than //

> +	pll_value |= 0x800000; // bit23-1

Please define bit fields and look at using GENMASK(), BIT(), FIELD_GET(),
FIELD_PREP()

> +	pll_value &= ~(0x00700000); // bit22:20-0
> +
> +	misc_value &= ~(0x8); // bit3-0
> +	misc_value &= ~(0x4); // bit2-0
> +
> +	misc_value &= ~(0x70); // bit6:4-0
> +	misc_value |= 0x50; // bit6:4-5
> +
> +	parameter_value &= ~(0x7); // bit2:0-0
> +	parameter_value |= 0x1; // bit2:0-1
> +
> +	control_value &= ~(0x190000); // bit20:19-0, bit16-0
> +	control_value |=   0x110000; // bit20:19-b10, bit16-1
> +
> +	sdhci_writel(host, pll_value, SDHCI_GLI_9750_PLL);
> +	sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> +
> +	// disable tuned clk
> +	ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +	ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
> +	sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
> +
> +	// 540 enable tuning parameters control
> +	control_value |= 0x10;
> +	sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
> +
> +	// write 544 tuning parameters
> +	sdhci_writel(host, parameter_value, SDHCI_GLI_9750_TUNING_PARAMETERS);
> +
> +	// 540 disable tuning parameters control
> +	control_value &= ~0x10;
> +	sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
> +
> +	// clear tuned clk
> +	ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +	ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
> +	sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
> +
> +	udelay(1);
> +	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> +	wt_value &= ~0x1;
> +	sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> +}
> +
> +static void sdhci_gli_do_reset(struct sdhci_host *host, u8 mask)
> +{
> +	host->ops->reset(host, mask);
> +
> +	if (mask & SDHCI_RESET_ALL) {
> +		if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
> +			if (host->ops->enable_dma)
> +				host->ops->enable_dma(host);
> +		}
> +
> +		/* Resetting the controller clears many */
> +		host->preset_enabled = false;
> +	}
> +}
> +
> +static void sdhci_gli_abort_tuning(struct sdhci_host *host, u32 opcode)

Pleased export sdhci_abort_tuning() instead.

> +{
> +	sdhci_reset_tuning(host);
> +
> +	sdhci_gli_do_reset(host, SDHCI_RESET_CMD);
> +	sdhci_gli_do_reset(host, SDHCI_RESET_DATA);
> +
> +	sdhci_end_tuning(host);
> +
> +	mmc_abort_tuning(host->mmc, opcode);
> +}
> +
> +static void gli_set_9750_rx_inv(struct sdhci_host *host, bool b)
> +{
> +	u32 wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> +	u32 misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> +
> +	if ((wt_value & 0x1) == 0) {
> +		wt_value |= 0x1;
> +		sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> +	}
> +
> +	misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> +	if (b) {
> +		misc_value |= 0x8;
> +		sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> +	} else {
> +		misc_value &= ~0x8;
> +		sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> +	}
> +
> +	wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> +	wt_value &= ~0x1;
> +	sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> +}
> +
> +static int __sdhci_execute_tuning_9750(struct sdhci_host *host, u32 opcode)
> +{
> +	int i;
> +	int rx_inv = 0;
> +
> +	for (rx_inv = 0; rx_inv < 2; rx_inv++) {
> +		if (rx_inv & 0x1)
> +			gli_set_9750_rx_inv(host, true);
> +		else
> +			gli_set_9750_rx_inv(host, false);
> +
> +		sdhci_start_tuning(host);
> +
> +		for (i = 0; i < GLI_MAX_TUNING_LOOP; i++) {
> +			u16 ctrl;
> +
> +			sdhci_send_tuning(host, opcode);
> +
> +			if (!host->tuning_done) {
> +				if (rx_inv == 1) {
> +					pr_info("%s: Tuning timeout, falling back to fixed sampling clock\n",
> +						mmc_hostname(host->mmc));
> +					sdhci_gli_abort_tuning(host, opcode);
> +					return -ETIMEDOUT;
> +				}
> +				pr_info("%s: Tuning timeout, try next tuning\n",
> +					mmc_hostname(host->mmc));
> +				sdhci_gli_abort_tuning(host, opcode);
> +				break;
> +			}
> +
> +			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +			if (!(ctrl & SDHCI_CTRL_EXEC_TUNING)) {
> +				if (ctrl & SDHCI_CTRL_TUNED_CLK)
> +					return 0; /* Success! */
> +				break;
> +			}
> +		}
> +	}
> +
> +	pr_info("%s: Tuning failed, falling back to fixed sampling clock\n",
> +		mmc_hostname(host->mmc));
> +	sdhci_reset_tuning(host);
> +	return -EAGAIN;
> +}
> +
> +static int gl9750_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	int err = 0;
> +	unsigned int tuning_count = 0;
> +	bool hs400_tuning;
> +
> +	hs400_tuning = host->flags & SDHCI_HS400_TUNING;
> +
> +	if (host->tuning_mode == SDHCI_TUNING_MODE_1)
> +		tuning_count = host->tuning_count;
> +
> +	/*
> +	 * The Host Controller needs tuning in case of SDR104 and DDR50
> +	 * mode, and for SDR50 mode when Use Tuning for SDR50 is set in
> +	 * the Capabilities register.
> +	 * If the Host Controller supports the HS200 mode then the
> +	 * tuning function has to be executed.
> +	 */
> +	switch (host->timing) {
> +	/* HS400 tuning is done in HS200 mode */
> +	case MMC_TIMING_MMC_HS400:
> +		err = -EINVAL;
> +		goto out;
> +
> +	case MMC_TIMING_MMC_HS200:
> +		/*
> +		 * Periodic re-tuning for HS400 is not expected to be needed, so
> +		 * disable it here.
> +		 */
> +		if (hs400_tuning)
> +			tuning_count = 0;
> +		break;
> +
> +	case MMC_TIMING_UHS_SDR104:
> +	case MMC_TIMING_UHS_DDR50:
> +		break;
> +	case MMC_TIMING_UHS_SDR50:
> +		if (host->flags & SDHCI_SDR50_NEEDS_TUNING)
> +			break;
> +		/* FALLTHROUGH */
> +
> +	default:
> +		goto out;
> +	}
> +
> +	if (host->ops->platform_execute_tuning) {
> +		err = host->ops->platform_execute_tuning(host, opcode);
> +		goto out;
> +	}
> +
> +	host->mmc->retune_period = tuning_count;
> +
> +	if (host->tuning_delay < 0)
> +		host->tuning_delay = opcode == MMC_SEND_TUNING_BLOCK;
> +
> +	gli_set_9750(host);
> +	host->tuning_err = __sdhci_execute_tuning_9750(host, opcode);
> +
> +	sdhci_end_tuning(host);
> +out:
> +	host->flags &= ~SDHCI_HS400_TUNING;
> +
> +	return err;
> +}
> +
> +static int gli_probe_slot_gl9750(struct sdhci_pci_slot *slot)
> +{
> +	struct sdhci_host *host = slot->host;
> +	struct mmc_host_ops *ops = &host->mmc_host_ops;
> +
> +	slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
> +	sdhci_enable_v4_mode(host);
> +
> +	ops->execute_tuning = gl9750_execute_tuning;

It looks like there would be less duplicated code if you implemented
host->ops->platform_execute_tuning instead of mmc->ops->execute_tuning

> +
> +	return 0;
> +}
> +
> +static int gli_probe_slot_gl9755(struct sdhci_pci_slot *slot)
> +{
> +	struct sdhci_host *host = slot->host;
> +
> +	slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
> +	sdhci_enable_v4_mode(host);
> +
> +	return 0;
> +}
> +
> +static void sdhci_gli_voltage_switch(struct sdhci_host *host)
> +{
> +	usleep_range(5000, 5500);
> +}
> +
> +static void sdhci_gli_pci_hw_reset(struct sdhci_host *host)
> +{
> +	struct sdhci_pci_slot *slot = sdhci_priv(host);
> +
> +	if (slot->hw_reset)
> +		slot->hw_reset(host);
> +}
> +
> +static void sdhci_gl9750_reset(struct sdhci_host *host, u8 mask)
> +{
> +	ktime_t timeout;
> +
> +	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
> +
> +	if (mask & SDHCI_RESET_ALL)
> +		host->clock = 0;
> +
> +	/* Wait max 100 ms */
> +	timeout = ktime_add_ms(ktime_get(), 100);
> +
> +	/* hw clears the bit when it's done */
> +	while (1) {
> +		bool timedout = ktime_after(ktime_get(), timeout);
> +
> +		if (!(sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask))
> +			break;
> +		if (timedout) {
> +			pr_err("%s: Reset 0x%x never completed.\n",
> +				mmc_hostname(host->mmc), (int)mask);
> +			sdhci_dumpregs(host);
> +			return;
> +		}
> +		udelay(10);
> +	}

Please call sdhci_reset() instead of duplicating the code above.

> +	gli_set_9750(host);
> +}
> +
> +static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
> +{
> +	u32 value;
> +
> +	value = readl(host->ioaddr + reg);
> +	if (unlikely(reg == SDHCI_MAX_CURRENT)) {
> +		if (!(value & 0xff))
> +			value |= 0xc8;
> +	}
> +	return value;
> +}
> +
> +static const struct sdhci_ops sdhci_gl9755_ops = {
> +	.set_clock		= sdhci_set_clock,
> +	.enable_dma		= sdhci_pci_enable_dma,
> +	.set_bus_width		= sdhci_set_bus_width,
> +	.reset			= sdhci_reset,
> +	.set_uhs_signaling	= sdhci_set_uhs_signaling,
> +	.hw_reset		= sdhci_gli_pci_hw_reset,

You never set slot->hw_reset so it doesn't look like .hw_reset is needed

> +	.voltage_switch		= sdhci_gli_voltage_switch,
> +};
> +
> +const struct sdhci_pci_fixes sdhci_gl9755 = {
> +	.quirks		= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> +	.quirks2	= SDHCI_QUIRK2_BROKEN_DDR50,
> +	.probe_slot	= gli_probe_slot_gl9755,
> +	.ops            = &sdhci_gl9755_ops,
> +};
> +
> +static const struct sdhci_ops sdhci_gl9750_ops = {
> +	.read_l                 = sdhci_gl9750_readl,

You need CONFIG_MMC_SDHCI_IO_ACCESSORS for read_l i.e.

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 14d89a108edd..c3bd967d8a1a 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -94,6 +94,7 @@ config MMC_SDHCI_PCI
 	depends on MMC_SDHCI && PCI
 	select MMC_CQHCI
 	select IOSF_MBI if X86
+	select MMC_SDHCI_IO_ACCESSORS
 	help
 	  This selects the PCI Secure Digital Host Controller Interface.
 	  Most controllers found today are PCI devices.

> +	.set_clock		= sdhci_set_clock,
> +	.enable_dma		= sdhci_pci_enable_dma,
> +	.set_bus_width		= sdhci_set_bus_width,
> +	.reset			= sdhci_gl9750_reset,
> +	.set_uhs_signaling	= sdhci_set_uhs_signaling,
> +	.hw_reset		= sdhci_gli_pci_hw_reset,
> +	.voltage_switch		= sdhci_gli_voltage_switch,
> +};
> +
> +const struct sdhci_pci_fixes sdhci_gl9750 = {
> +	.quirks		= SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> +	.quirks2	= SDHCI_QUIRK2_BROKEN_DDR50,
> +	.probe_slot	= gli_probe_slot_gl9750,
> +	.ops            = &sdhci_gl9750_ops,
> +};
> +
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index e5dc6e44c7a4..738ba5afcc20 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -65,6 +65,9 @@
>  
>  #define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xc202
>  
> +#define PCI_DEVICE_ID_GLI_9755		0x9755
> +#define PCI_DEVICE_ID_GLI_9750		0x9750
> +
>  /*
>   * PCI device class and mask
>   */
> @@ -185,5 +188,7 @@ int sdhci_pci_enable_dma(struct sdhci_host *host);
>  extern const struct sdhci_pci_fixes sdhci_arasan;
>  extern const struct sdhci_pci_fixes sdhci_snps;
>  extern const struct sdhci_pci_fixes sdhci_o2;
> +extern const struct sdhci_pci_fixes sdhci_gl9750;
> +extern const struct sdhci_pci_fixes sdhci_gl9755;
>  
>  #endif /* __SDHCI_PCI_H */
> 


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH V5 4/4] mmc: host: sdhci-pci: Add Genesys Logic GL975x support
  2019-08-21 12:29 ` Adrian Hunter
@ 2019-08-22  2:26   ` Ben Chuang
  0 siblings, 0 replies; 3+ messages in thread
From: Ben Chuang @ 2019-08-22  2:26 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Ulf Hansson, linux-mmc, linux-kernel, Michael K. Johnson, Ben Chuang

Sorry to resend the email because of non-plain text issues.

On Wed, Aug 21, 2019 at 8:30 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 20/08/19 5:07 AM, Ben Chuang wrote:
> > From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >
> > Add support for the GL9750 and GL9755 chipsets.
> >
> > The patches enable v4 mode and wait 5ms after set 1.8V signal enable for
> > GL9750/GL9755. It fixed the value of SDHCI_MAX_CURRENT register and uses
> > the vendor tuning flow for GL9750.
> >
> > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > Co-developed-by: Michael K Johnson <johnsonm@danlj.org>
> > Signed-off-by: Michael K Johnson <johnsonm@danlj.org>
> > ---
> >  drivers/mmc/host/Makefile         |   2 +-
> >  drivers/mmc/host/sdhci-pci-core.c |   2 +
> >  drivers/mmc/host/sdhci-pci-gli.c  | 381 ++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci-pci.h      |   5 +
> >  4 files changed, 389 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mmc/host/sdhci-pci-gli.c
> >
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 73578718f119..661445415090 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -13,7 +13,7 @@ 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-dwc-mshc.o
> > +                                sdhci-pci-dwc-mshc.o sdhci-pci-gli.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 4154ee11b47d..e5835fbf73bc 100644
> > --- a/drivers/mmc/host/sdhci-pci-core.c
> > +++ b/drivers/mmc/host/sdhci-pci-core.c
> > @@ -1682,6 +1682,8 @@ static const struct pci_device_id pci_ids[] = {
> >       SDHCI_PCI_DEVICE(O2, SEABIRD1, o2),
> >       SDHCI_PCI_DEVICE(ARASAN, PHY_EMMC, arasan),
> >       SDHCI_PCI_DEVICE(SYNOPSYS, DWC_MSHC, snps),
> > +     SDHCI_PCI_DEVICE(GLI, 9750, gl9750),
> > +     SDHCI_PCI_DEVICE(GLI, 9755, gl9755),
> >       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-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > new file mode 100644
> > index 000000000000..99abb7830e62
> > --- /dev/null
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -0,0 +1,381 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Genesys Logic, Inc.
> > + *
> > + * Authors: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > + *
> > + * Version: v0.9.0 (2019-08-08)
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/delay.h>
> > +#include "sdhci.h"
> > +#include "sdhci-pci.h"
> > +
> > +/*  Genesys Logic extra registers */
> > +#define SDHCI_GLI_9750_WT         0x800
> > +#define SDHCI_GLI_9750_DRIVING    0x860
> > +#define SDHCI_GLI_9750_PLL        0x864
> > +#define SDHCI_GLI_9750_SW_CTRL    0x874
> > +#define SDHCI_GLI_9750_MISC       0x878
> > +
> > +#define SDHCI_GLI_9750_TUNING_CONTROL                0x540
> > +#define SDHCI_GLI_9750_TUNING_PARAMETERS     0x544
> > +
> > +#define GLI_MAX_TUNING_LOOP 40
> > +
> > +/* Genesys Logic chipset */
> > +static void gli_set_9750(struct sdhci_host *host)
> > +{
> > +     u32 wt_value = 0;
> > +     u32 driving_value = 0;
> > +     u32 pll_value = 0;
> > +     u32 sw_ctrl_value = 0;
> > +     u32 misc_value = 0;
> > +     u32 parameter_value = 0;
> > +     u32 control_value = 0;
> > +
> > +     u16 ctrl2 = 0;
> > +
> > +     wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> > +     if ((wt_value & 0x1) == 0) {
> > +             wt_value |= 0x1;
> > +             sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> > +     }
> > +
> > +     driving_value = sdhci_readl(host, SDHCI_GLI_9750_DRIVING);
> > +     pll_value = sdhci_readl(host, SDHCI_GLI_9750_PLL);
> > +     sw_ctrl_value = sdhci_readl(host, SDHCI_GLI_9750_SW_CTRL);
> > +     misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> > +     parameter_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_PARAMETERS);
> > +     control_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_CONTROL);
> > +
> > +     driving_value &= ~(0x0C000FFF);
> > +     driving_value |= 0x0C000FFF;
> > +     sdhci_writel(host, driving_value, SDHCI_GLI_9750_DRIVING);
> > +
> > +     sw_ctrl_value |= 0xc0;
> > +     sdhci_writel(host, sw_ctrl_value, SDHCI_GLI_9750_SW_CTRL);
> > +
> > +     // reset the tuning flow after reinit and before starting tuning
>
> For consistent style, please use C-style comments /* */ rather than //

Thanks. I will use C-style comments to replace // .

>
> > +     pll_value |= 0x800000; // bit23-1
>
> Please define bit fields and look at using GENMASK(), BIT(), FIELD_GET(),
> FIELD_PREP()
>

Got it. Thanks. I will define bit fields and use these pre-defined macro.

> > +     pll_value &= ~(0x00700000); // bit22:20-0
> > +
> > +     misc_value &= ~(0x8); // bit3-0
> > +     misc_value &= ~(0x4); // bit2-0
> > +
> > +     misc_value &= ~(0x70); // bit6:4-0
> > +     misc_value |= 0x50; // bit6:4-5
> > +
> > +     parameter_value &= ~(0x7); // bit2:0-0
> > +     parameter_value |= 0x1; // bit2:0-1
> > +
> > +     control_value &= ~(0x190000); // bit20:19-0, bit16-0
> > +     control_value |=   0x110000; // bit20:19-b10, bit16-1
> > +
> > +     sdhci_writel(host, pll_value, SDHCI_GLI_9750_PLL);
> > +     sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> > +
> > +     // disable tuned clk
> > +     ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > +     ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
> > +     sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
> > +
> > +     // 540 enable tuning parameters control
> > +     control_value |= 0x10;
> > +     sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
> > +
> > +     // write 544 tuning parameters
> > +     sdhci_writel(host, parameter_value, SDHCI_GLI_9750_TUNING_PARAMETERS);
> > +
> > +     // 540 disable tuning parameters control
> > +     control_value &= ~0x10;
> > +     sdhci_writel(host, control_value, SDHCI_GLI_9750_TUNING_CONTROL);
> > +
> > +     // clear tuned clk
> > +     ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > +     ctrl2 &= ~SDHCI_CTRL_TUNED_CLK;
> > +     sdhci_writew(host, ctrl2, SDHCI_HOST_CONTROL2);
> > +
> > +     udelay(1);
> > +     wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> > +     wt_value &= ~0x1;
> > +     sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> > +}
> > +
> > +static void sdhci_gli_do_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +     host->ops->reset(host, mask);
> > +
> > +     if (mask & SDHCI_RESET_ALL) {
> > +             if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
> > +                     if (host->ops->enable_dma)
> > +                             host->ops->enable_dma(host);
> > +             }
> > +
> > +             /* Resetting the controller clears many */
> > +             host->preset_enabled = false;
> > +     }
> > +}
> > +
> > +static void sdhci_gli_abort_tuning(struct sdhci_host *host, u32 opcode)
>
> Pleased export sdhci_abort_tuning() instead.
>

Got it, thanks. I will export it if I need to use it.

> > +{
> > +     sdhci_reset_tuning(host);
> > +
> > +     sdhci_gli_do_reset(host, SDHCI_RESET_CMD);
> > +     sdhci_gli_do_reset(host, SDHCI_RESET_DATA);
> > +
> > +     sdhci_end_tuning(host);
> > +
> > +     mmc_abort_tuning(host->mmc, opcode);
> > +}
> > +
> > +static void gli_set_9750_rx_inv(struct sdhci_host *host, bool b)
> > +{
> > +     u32 wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> > +     u32 misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> > +
> > +     if ((wt_value & 0x1) == 0) {
> > +             wt_value |= 0x1;
> > +             sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> > +     }
> > +
> > +     misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
> > +     if (b) {
> > +             misc_value |= 0x8;
> > +             sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> > +     } else {
> > +             misc_value &= ~0x8;
> > +             sdhci_writel(host, misc_value, SDHCI_GLI_9750_MISC);
> > +     }
> > +
> > +     wt_value = sdhci_readl(host, SDHCI_GLI_9750_WT);
> > +     wt_value &= ~0x1;
> > +     sdhci_writel(host, wt_value, SDHCI_GLI_9750_WT);
> > +}
> > +
> > +static int __sdhci_execute_tuning_9750(struct sdhci_host *host, u32 opcode)
> > +{
> > +     int i;
> > +     int rx_inv = 0;
> > +
> > +     for (rx_inv = 0; rx_inv < 2; rx_inv++) {
> > +             if (rx_inv & 0x1)
> > +                     gli_set_9750_rx_inv(host, true);
> > +             else
> > +                     gli_set_9750_rx_inv(host, false);
> > +
> > +             sdhci_start_tuning(host);
> > +
> > +             for (i = 0; i < GLI_MAX_TUNING_LOOP; i++) {
> > +                     u16 ctrl;
> > +
> > +                     sdhci_send_tuning(host, opcode);
> > +
> > +                     if (!host->tuning_done) {
> > +                             if (rx_inv == 1) {
> > +                                     pr_info("%s: Tuning timeout, falling back to fixed sampling clock\n",
> > +                                             mmc_hostname(host->mmc));
> > +                                     sdhci_gli_abort_tuning(host, opcode);
> > +                                     return -ETIMEDOUT;
> > +                             }
> > +                             pr_info("%s: Tuning timeout, try next tuning\n",
> > +                                     mmc_hostname(host->mmc));
> > +                             sdhci_gli_abort_tuning(host, opcode);
> > +                             break;
> > +                     }
> > +
> > +                     ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > +                     if (!(ctrl & SDHCI_CTRL_EXEC_TUNING)) {
> > +                             if (ctrl & SDHCI_CTRL_TUNED_CLK)
> > +                                     return 0; /* Success! */
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +
> > +     pr_info("%s: Tuning failed, falling back to fixed sampling clock\n",
> > +             mmc_hostname(host->mmc));
> > +     sdhci_reset_tuning(host);
> > +     return -EAGAIN;
> > +}
> > +
> > +static int gl9750_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > +{
> > +     struct sdhci_host *host = mmc_priv(mmc);
> > +     int err = 0;
> > +     unsigned int tuning_count = 0;
> > +     bool hs400_tuning;
> > +
> > +     hs400_tuning = host->flags & SDHCI_HS400_TUNING;
> > +
> > +     if (host->tuning_mode == SDHCI_TUNING_MODE_1)
> > +             tuning_count = host->tuning_count;
> > +
> > +     /*
> > +      * The Host Controller needs tuning in case of SDR104 and DDR50
> > +      * mode, and for SDR50 mode when Use Tuning for SDR50 is set in
> > +      * the Capabilities register.
> > +      * If the Host Controller supports the HS200 mode then the
> > +      * tuning function has to be executed.
> > +      */
> > +     switch (host->timing) {
> > +     /* HS400 tuning is done in HS200 mode */
> > +     case MMC_TIMING_MMC_HS400:
> > +             err = -EINVAL;
> > +             goto out;
> > +
> > +     case MMC_TIMING_MMC_HS200:
> > +             /*
> > +              * Periodic re-tuning for HS400 is not expected to be needed, so
> > +              * disable it here.
> > +              */
> > +             if (hs400_tuning)
> > +                     tuning_count = 0;
> > +             break;
> > +
> > +     case MMC_TIMING_UHS_SDR104:
> > +     case MMC_TIMING_UHS_DDR50:
> > +             break;
> > +     case MMC_TIMING_UHS_SDR50:
> > +             if (host->flags & SDHCI_SDR50_NEEDS_TUNING)
> > +                     break;
> > +             /* FALLTHROUGH */
> > +
> > +     default:
> > +             goto out;
> > +     }
> > +
> > +     if (host->ops->platform_execute_tuning) {
> > +             err = host->ops->platform_execute_tuning(host, opcode);
> > +             goto out;
> > +     }
> > +
> > +     host->mmc->retune_period = tuning_count;
> > +
> > +     if (host->tuning_delay < 0)
> > +             host->tuning_delay = opcode == MMC_SEND_TUNING_BLOCK;
> > +
> > +     gli_set_9750(host);
> > +     host->tuning_err = __sdhci_execute_tuning_9750(host, opcode);
> > +
> > +     sdhci_end_tuning(host);
> > +out:
> > +     host->flags &= ~SDHCI_HS400_TUNING;
> > +
> > +     return err;
> > +}
> > +
> > +static int gli_probe_slot_gl9750(struct sdhci_pci_slot *slot)
> > +{
> > +     struct sdhci_host *host = slot->host;
> > +     struct mmc_host_ops *ops = &host->mmc_host_ops;
> > +
> > +     slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
> > +     sdhci_enable_v4_mode(host);
> > +
> > +     ops->execute_tuning = gl9750_execute_tuning;
>
> It looks like there would be less duplicated code if you implemented
> host->ops->platform_execute_tuning instead of mmc->ops->execute_tuning

Thank you for comments.
I will study host->ops->platform_execute_tuning and use it to reduce
duplicated code.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int gli_probe_slot_gl9755(struct sdhci_pci_slot *slot)
> > +{
> > +     struct sdhci_host *host = slot->host;
> > +
> > +     slot->host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
> > +     sdhci_enable_v4_mode(host);
> > +
> > +     return 0;
> > +}
> > +
> > +static void sdhci_gli_voltage_switch(struct sdhci_host *host)
> > +{
> > +     usleep_range(5000, 5500);
> > +}
> > +
> > +static void sdhci_gli_pci_hw_reset(struct sdhci_host *host)
> > +{
> > +     struct sdhci_pci_slot *slot = sdhci_priv(host);
> > +
> > +     if (slot->hw_reset)
> > +             slot->hw_reset(host);
> > +}
> > +
> > +static void sdhci_gl9750_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +     ktime_t timeout;
> > +
> > +     sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
> > +
> > +     if (mask & SDHCI_RESET_ALL)
> > +             host->clock = 0;
> > +
> > +     /* Wait max 100 ms */
> > +     timeout = ktime_add_ms(ktime_get(), 100);
> > +
> > +     /* hw clears the bit when it's done */
> > +     while (1) {
> > +             bool timedout = ktime_after(ktime_get(), timeout);
> > +
> > +             if (!(sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask))
> > +                     break;
> > +             if (timedout) {
> > +                     pr_err("%s: Reset 0x%x never completed.\n",
> > +                             mmc_hostname(host->mmc), (int)mask);
> > +                     sdhci_dumpregs(host);
> > +                     return;
> > +             }
> > +             udelay(10);
> > +     }
>
> Please call sdhci_reset() instead of duplicating the code above.
>

Thanks, I will modify it.


> > +     gli_set_9750(host);
> > +}
> > +
> > +static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
> > +{
> > +     u32 value;
> > +
> > +     value = readl(host->ioaddr + reg);
> > +     if (unlikely(reg == SDHCI_MAX_CURRENT)) {
> > +             if (!(value & 0xff))
> > +                     value |= 0xc8;
> > +     }
> > +     return value;
> > +}
> > +
> > +static const struct sdhci_ops sdhci_gl9755_ops = {
> > +     .set_clock              = sdhci_set_clock,
> > +     .enable_dma             = sdhci_pci_enable_dma,
> > +     .set_bus_width          = sdhci_set_bus_width,
> > +     .reset                  = sdhci_reset,
> > +     .set_uhs_signaling      = sdhci_set_uhs_signaling,
> > +     .hw_reset               = sdhci_gli_pci_hw_reset,
>
> You never set slot->hw_reset so it doesn't look like .hw_reset is needed

I will removed it if testing is ok after removing .hw_reset.

>
> > +     .voltage_switch         = sdhci_gli_voltage_switch,
> > +};
> > +
> > +const struct sdhci_pci_fixes sdhci_gl9755 = {
> > +     .quirks         = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> > +     .quirks2        = SDHCI_QUIRK2_BROKEN_DDR50,
> > +     .probe_slot     = gli_probe_slot_gl9755,
> > +     .ops            = &sdhci_gl9755_ops,
> > +};
> > +
> > +static const struct sdhci_ops sdhci_gl9750_ops = {
> > +     .read_l                 = sdhci_gl9750_readl,
>
> You need CONFIG_MMC_SDHCI_IO_ACCESSORS for read_l i.e.

 I will add the condition define CONFIG_MMC_SDHCI_IO_ACCESSORS to the code.

>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 14d89a108edd..c3bd967d8a1a 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -94,6 +94,7 @@ config MMC_SDHCI_PCI
>         depends on MMC_SDHCI && PCI
>         select MMC_CQHCI
>         select IOSF_MBI if X86
> +       select MMC_SDHCI_IO_ACCESSORS
>         help
>           This selects the PCI Secure Digital Host Controller Interface.
>           Most controllers found today are PCI devices.
>
> > +     .set_clock              = sdhci_set_clock,
> > +     .enable_dma             = sdhci_pci_enable_dma,
> > +     .set_bus_width          = sdhci_set_bus_width,
> > +     .reset                  = sdhci_gl9750_reset,
> > +     .set_uhs_signaling      = sdhci_set_uhs_signaling,
> > +     .hw_reset               = sdhci_gli_pci_hw_reset,
> > +     .voltage_switch         = sdhci_gli_voltage_switch,
> > +};
> > +
> > +const struct sdhci_pci_fixes sdhci_gl9750 = {
> > +     .quirks         = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> > +     .quirks2        = SDHCI_QUIRK2_BROKEN_DDR50,
> > +     .probe_slot     = gli_probe_slot_gl9750,
> > +     .ops            = &sdhci_gl9750_ops,
> > +};
> > +
> > diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> > index e5dc6e44c7a4..738ba5afcc20 100644
> > --- a/drivers/mmc/host/sdhci-pci.h
> > +++ b/drivers/mmc/host/sdhci-pci.h
> > @@ -65,6 +65,9 @@
> >
> >  #define PCI_DEVICE_ID_SYNOPSYS_DWC_MSHC 0xc202
> >
> > +#define PCI_DEVICE_ID_GLI_9755               0x9755
> > +#define PCI_DEVICE_ID_GLI_9750               0x9750
> > +
> >  /*
> >   * PCI device class and mask
> >   */
> > @@ -185,5 +188,7 @@ int sdhci_pci_enable_dma(struct sdhci_host *host);
> >  extern const struct sdhci_pci_fixes sdhci_arasan;
> >  extern const struct sdhci_pci_fixes sdhci_snps;
> >  extern const struct sdhci_pci_fixes sdhci_o2;
> > +extern const struct sdhci_pci_fixes sdhci_gl9750;
> > +extern const struct sdhci_pci_fixes sdhci_gl9755;
> >
> >  #endif /* __SDHCI_PCI_H */
> >
>

Thank you for reviewing.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-08-22  2:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20  2:07 [PATCH V5 4/4] mmc: host: sdhci-pci: Add Genesys Logic GL975x support Ben Chuang
2019-08-21 12:29 ` Adrian Hunter
2019-08-22  2:26   ` Ben Chuang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).