[v4, 0/6] eSDHC patches introduction

[v4, 0/6] eSDHC patches introduction

[Yangbo Lu]

This patchset is used to fix host version in eSDHC_HOSTVER for T4240-R1.0-R2.0.
To get the SoC version and revision, it's needed to add the GUTS driver to access
the global utilities registers.

So, the first three patches are to add the GUTS driver.
The following two patches are to enable GUTS driver support
to get SVR in eSDHC driver.
The last patch is to fix host version for T4240.

Yangbo Lu (6):
  soc: fsl: add GUTS driver for QorIQ platforms
  dt: move guts devicetree doc out of powerpc directory
  powerpc/fsl: move mpc85xx.h to include/linux
  mmc: sdhci-of-esdhc: get SVR from global utilities registers
  mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC
  mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

 .../bindings/{powerpc/fsl => soc}/guts.txt         |   3 +
 drivers/clk/clk-qoriq.c                            |   2 +-
 drivers/i2c/busses/i2c-mpc.c                       |   2 +-
 drivers/iommu/fsl_pamu.c                           |   2 +-
 drivers/mmc/host/Kconfig                           |   2 +
 drivers/mmc/host/sdhci-of-esdhc.c                  |  26 ++++-
 drivers/net/ethernet/freescale/gianfar.c           |   2 +-
 drivers/soc/Kconfig                                |   1 +
 drivers/soc/Makefile                               |   1 +
 drivers/soc/fsl/Kconfig                            |  19 ++++
 drivers/soc/fsl/Makefile                           |   4 +
 drivers/soc/fsl/guts.c                             | 112 +++++++++++++++++++++
 include/linux/fsl/guts.h                           | 103 ++++++++++---------
 .../include/asm/mpc85xx.h => include/linux/svr.h   |   4 +-
 14 files changed, 227 insertions(+), 56 deletions(-)
 rename Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt (90%)
 create mode 100644 drivers/soc/fsl/Kconfig
 create mode 100644 drivers/soc/fsl/Makefile
 create mode 100644 drivers/soc/fsl/guts.c
 rename arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h (97%)

-- 
2.1.0.27.g96db324

[v4, 1/6] soc: fsl: add GUTS driver for QorIQ platforms

[Yangbo Lu]

The global utilities block controls power management, I/O device
enabling, power-onreset(POR) configuration monitoring, alternate
function selection for multiplexed signals,and clock control.

This patch adds GUTS driver to manage and access global utilities
block.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- Added this patch
---
 drivers/soc/Kconfig      |   1 +
 drivers/soc/Makefile     |   1 +
 drivers/soc/fsl/Kconfig  |  19 ++++++++
 drivers/soc/fsl/Makefile |   4 ++
 drivers/soc/fsl/guts.c   | 112 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fsl/guts.h | 103 +++++++++++++++++++++++--------------------
 6 files changed, 192 insertions(+), 48 deletions(-)
 create mode 100644 drivers/soc/fsl/Kconfig
 create mode 100644 drivers/soc/fsl/Makefile
 create mode 100644 drivers/soc/fsl/guts.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 4e853ed..68b5b90 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -7,5 +7,6 @@ source "drivers/soc/rockchip/Kconfig"
 source "drivers/soc/sunxi/Kconfig"
 source "drivers/soc/ti/Kconfig"
 source "drivers/soc/versatile/Kconfig"
+source "drivers/soc/fsl/Kconfig"
 
 endmenu
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index f2ba2e9..2747e58 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_ARCH_SUNXI)	+= sunxi/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-$(CONFIG_SOC_TI)		+= ti/
 obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
+obj-$(CONFIG_SOC_FSL)		+= fsl/
diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
new file mode 100644
index 0000000..09966f0
--- /dev/null
+++ b/drivers/soc/fsl/Kconfig
@@ -0,0 +1,19 @@
+#
+# FSL SOC drivers
+#
+menuconfig SOC_FSL
+	bool "Freescale SOC drivers support"
+
+if SOC_FSL
+
+config FSL_GUTS
+	tristate "QorIQ Platforms GUTS Driver"
+	help
+	  Say y here to enable Freescale QorIQ platforms GUTS driver support.
+	  The global utilities block controls power management, I/O device
+	  enabling, power-onreset(POR) configuration monitoring, alternate
+	  function selection for multiplexed signals,and clock control.
+
+	  If unsure, say N.
+
+endif # SOC_FSL
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
new file mode 100644
index 0000000..51e5c73
--- /dev/null
+++ b/drivers/soc/fsl/Makefile
@@ -0,0 +1,4 @@
+#
+# Freescale SOC drivers
+#
+obj-$(CONFIG_FSL_GUTS)	+= guts.o
diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
new file mode 100644
index 0000000..68e3367
--- /dev/null
+++ b/drivers/soc/fsl/guts.c
@@ -0,0 +1,112 @@
+/*
+ * Freescale QorIQ Platforms GUTS Driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ *
+ * Author: Yangbo Lu <yangbo.lu@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/fsl/guts.h>
+
+/*
+ * Table for matching compatible strings, for device tree
+ * guts node, for Freescale QorIQ SOCs.
+ * "fsl,qoriq-device-config-2.0" corresponds to T4 & B4
+ * SOCs. For the older SOCs "fsl,qoriq-device-config-1.0"
+ * string would be used.
+ */
+static const struct of_device_id guts_device_ids[] = {
+	{ .compatible = "fsl,qoriq-device-config-1.0", },
+	{ .compatible = "fsl,qoriq-device-config-2.0", },
+	{}
+};
+
+struct ccsr_guts __iomem *guts_regmap(void)
+{
+	struct device_node *guts_node;
+	struct ccsr_guts __iomem *guts;
+
+	guts_node = of_find_matching_node(NULL, guts_device_ids);
+	if (!guts_node)
+		return NULL;
+
+	guts = of_iomap(guts_node, 0);
+	if (!guts)
+		return NULL;
+
+	of_node_put(guts_node);
+	return guts;
+}
+EXPORT_SYMBOL_GPL(guts_regmap);
+
+u8 guts_get_reg8(void __iomem *reg)
+{
+	u8 val;
+
+	val = ioread8(reg);
+	return val;
+}
+EXPORT_SYMBOL_GPL(guts_get_reg8);
+
+void guts_set_reg8(void __iomem *reg, u8 value)
+{
+	iowrite8(value, reg);
+}
+EXPORT_SYMBOL_GPL(guts_set_reg8);
+
+u32 guts_get_reg32(void __iomem *reg)
+{
+	struct device_node *guts_node;
+	u32 val;
+
+	guts_node = of_find_matching_node(NULL, guts_device_ids);
+	if (!guts_node)
+		return 0;
+
+	if (of_property_read_bool(guts_node, "little-endian"))
+		val = ioread32(reg);
+	else
+		val = ioread32be(reg);
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(guts_get_reg32);
+
+void guts_set_reg32(void __iomem *reg, u32 value)
+{
+	struct device_node *guts_node;
+
+	guts_node = of_find_matching_node(NULL, guts_device_ids);
+	if (!guts_node)
+		return;
+
+	if (of_property_read_bool(guts_node, "little-endian"))
+		iowrite32(value, reg);
+	else
+		iowrite32be(value, reg);
+}
+EXPORT_SYMBOL_GPL(guts_set_reg32);
+
+static int __init guts_drv_init(void)
+{
+	pr_info("guts: Freescale QorIQ Platforms GUTS Driver\n");
+	return 0;
+}
+module_init(guts_drv_init);
+
+static void __exit guts_drv_exit(void)
+{
+}
+module_exit(guts_drv_exit);
+
+MODULE_AUTHOR("Yangbo Lu <yangbo.lu@freescale.com>");
+MODULE_DESCRIPTION("Freescale QorIQ Platforms GUTS Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/fsl/guts.h b/include/linux/fsl/guts.h
index 84d971f..e3ae0af 100644
--- a/include/linux/fsl/guts.h
+++ b/include/linux/fsl/guts.h
@@ -29,83 +29,90 @@
  * #ifdefs.
  */
 struct ccsr_guts {
-	__be32	porpllsr;	/* 0x.0000 - POR PLL Ratio Status Register */
-	__be32	porbmsr;	/* 0x.0004 - POR Boot Mode Status Register */
-	__be32	porimpscr;	/* 0x.0008 - POR I/O Impedance Status and Control Register */
-	__be32	pordevsr;	/* 0x.000c - POR I/O Device Status Register */
-	__be32	pordbgmsr;	/* 0x.0010 - POR Debug Mode Status Register */
-	__be32	pordevsr2;	/* 0x.0014 - POR device status register 2 */
+	u32	porpllsr;	/* 0x.0000 - POR PLL Ratio Status Register */
+	u32	porbmsr;	/* 0x.0004 - POR Boot Mode Status Register */
+	u32	porimpscr;	/* 0x.0008 - POR I/O Impedance Status and Control Register */
+	u32	pordevsr;	/* 0x.000c - POR I/O Device Status Register */
+	u32	pordbgmsr;	/* 0x.0010 - POR Debug Mode Status Register */
+	u32	pordevsr2;	/* 0x.0014 - POR device status register 2 */
 	u8	res018[0x20 - 0x18];
-	__be32	porcir;		/* 0x.0020 - POR Configuration Information Register */
+	u32	porcir;		/* 0x.0020 - POR Configuration Information Register */
 	u8	res024[0x30 - 0x24];
-	__be32	gpiocr;		/* 0x.0030 - GPIO Control Register */
+	u32	gpiocr;		/* 0x.0030 - GPIO Control Register */
 	u8	res034[0x40 - 0x34];
-	__be32	gpoutdr;	/* 0x.0040 - General-Purpose Output Data Register */
+	u32	gpoutdr;	/* 0x.0040 - General-Purpose Output Data Register */
 	u8	res044[0x50 - 0x44];
-	__be32	gpindr;		/* 0x.0050 - General-Purpose Input Data Register */
+	u32	gpindr;		/* 0x.0050 - General-Purpose Input Data Register */
 	u8	res054[0x60 - 0x54];
-	__be32	pmuxcr;		/* 0x.0060 - Alternate Function Signal Multiplex Control */
-        __be32  pmuxcr2;	/* 0x.0064 - Alternate function signal multiplex control 2 */
-        __be32  dmuxcr;		/* 0x.0068 - DMA Mux Control Register */
+	u32	pmuxcr;		/* 0x.0060 - Alternate Function Signal Multiplex Control */
+	u32	pmuxcr2;	/* 0x.0064 - Alternate function signal multiplex control 2 */
+	u32	dmuxcr;		/* 0x.0068 - DMA Mux Control Register */
         u8	res06c[0x70 - 0x6c];
-	__be32	devdisr;	/* 0x.0070 - Device Disable Control */
+	u32	devdisr;	/* 0x.0070 - Device Disable Control */
 #define CCSR_GUTS_DEVDISR_TB1	0x00001000
 #define CCSR_GUTS_DEVDISR_TB0	0x00004000
-	__be32	devdisr2;	/* 0x.0074 - Device Disable Control 2 */
+	u32	devdisr2;	/* 0x.0074 - Device Disable Control 2 */
 	u8	res078[0x7c - 0x78];
-	__be32  pmjcr;		/* 0x.007c - 4 Power Management Jog Control Register */
-	__be32	powmgtcsr;	/* 0x.0080 - Power Management Status and Control Register */
-	__be32  pmrccr;		/* 0x.0084 - Power Management Reset Counter Configuration Register */
-	__be32  pmpdccr;	/* 0x.0088 - Power Management Power Down Counter Configuration Register */
-	__be32  pmcdr;		/* 0x.008c - 4Power management clock disable register */
-	__be32	mcpsumr;	/* 0x.0090 - Machine Check Summary Register */
-	__be32	rstrscr;	/* 0x.0094 - Reset Request Status and Control Register */
-	__be32  ectrstcr;	/* 0x.0098 - Exception reset control register */
-	__be32  autorstsr;	/* 0x.009c - Automatic reset status register */
-	__be32	pvr;		/* 0x.00a0 - Processor Version Register */
-	__be32	svr;		/* 0x.00a4 - System Version Register */
+	u32	pmjcr;		/* 0x.007c - 4 Power Management Jog Control Register */
+	u32	powmgtcsr;	/* 0x.0080 - Power Management Status and Control Register */
+	u32	pmrccr;		/* 0x.0084 - Power Management Reset Counter Configuration Register */
+	u32	pmpdccr;	/* 0x.0088 - Power Management Power Down Counter Configuration Register */
+	u32	pmcdr;		/* 0x.008c - 4Power management clock disable register */
+	u32	mcpsumr;	/* 0x.0090 - Machine Check Summary Register */
+	u32	rstrscr;	/* 0x.0094 - Reset Request Status and Control Register */
+	u32	ectrstcr;	/* 0x.0098 - Exception reset control register */
+	u32	autorstsr;	/* 0x.009c - Automatic reset status register */
+	u32	pvr;		/* 0x.00a0 - Processor Version Register */
+	u32	svr;		/* 0x.00a4 - System Version Register */
 	u8	res0a8[0xb0 - 0xa8];
-	__be32	rstcr;		/* 0x.00b0 - Reset Control Register */
+	u32	rstcr;		/* 0x.00b0 - Reset Control Register */
 	u8	res0b4[0xc0 - 0xb4];
-	__be32  iovselsr;	/* 0x.00c0 - I/O voltage select status register
+	u32	iovselsr;	/* 0x.00c0 - I/O voltage select status register
 					     Called 'elbcvselcr' on 86xx SOCs */
 	u8	res0c4[0x100 - 0xc4];
-	__be32	rcwsr[16];	/* 0x.0100 - Reset Control Word Status registers
+	u32	rcwsr[16];	/* 0x.0100 - Reset Control Word Status registers
 					     There are 16 registers */
 	u8	res140[0x224 - 0x140];
-	__be32  iodelay1;	/* 0x.0224 - IO delay control register 1 */
-	__be32  iodelay2;	/* 0x.0228 - IO delay control register 2 */
+	u32	iodelay1;	/* 0x.0224 - IO delay control register 1 */
+	u32	iodelay2;	/* 0x.0228 - IO delay control register 2 */
 	u8	res22c[0x604 - 0x22c];
-	__be32	pamubypenr; 	/* 0x.604 - PAMU bypass enable register */
+	u32	pamubypenr;	/* 0x.604 - PAMU bypass enable register */
 	u8	res608[0x800 - 0x608];
-	__be32	clkdvdr;	/* 0x.0800 - Clock Divide Register */
+	u32	clkdvdr;	/* 0x.0800 - Clock Divide Register */
 	u8	res804[0x900 - 0x804];
-	__be32	ircr;		/* 0x.0900 - Infrared Control Register */
+	u32	ircr;		/* 0x.0900 - Infrared Control Register */
 	u8	res904[0x908 - 0x904];
-	__be32	dmacr;		/* 0x.0908 - DMA Control Register */
+	u32	dmacr;		/* 0x.0908 - DMA Control Register */
 	u8	res90c[0x914 - 0x90c];
-	__be32	elbccr;		/* 0x.0914 - eLBC Control Register */
+	u32	elbccr;		/* 0x.0914 - eLBC Control Register */
 	u8	res918[0xb20 - 0x918];
-	__be32	ddr1clkdr;	/* 0x.0b20 - DDR1 Clock Disable Register */
-	__be32	ddr2clkdr;	/* 0x.0b24 - DDR2 Clock Disable Register */
-	__be32	ddrclkdr;	/* 0x.0b28 - DDR Clock Disable Register */
+	u32	ddr1clkdr;	/* 0x.0b20 - DDR1 Clock Disable Register */
+	u32	ddr2clkdr;	/* 0x.0b24 - DDR2 Clock Disable Register */
+	u32	ddrclkdr;	/* 0x.0b28 - DDR Clock Disable Register */
 	u8	resb2c[0xe00 - 0xb2c];
-	__be32	clkocr;		/* 0x.0e00 - Clock Out Select Register */
+	u32	clkocr;		/* 0x.0e00 - Clock Out Select Register */
 	u8	rese04[0xe10 - 0xe04];
-	__be32	ddrdllcr;	/* 0x.0e10 - DDR DLL Control Register */
+	u32	ddrdllcr;	/* 0x.0e10 - DDR DLL Control Register */
 	u8	rese14[0xe20 - 0xe14];
-	__be32	lbcdllcr;	/* 0x.0e20 - LBC DLL Control Register */
-	__be32  cpfor;		/* 0x.0e24 - L2 charge pump fuse override register */
+	u32	lbcdllcr;	/* 0x.0e20 - LBC DLL Control Register */
+	u32	cpfor;		/* 0x.0e24 - L2 charge pump fuse override register */
 	u8	rese28[0xf04 - 0xe28];
-	__be32	srds1cr0;	/* 0x.0f04 - SerDes1 Control Register 0 */
-	__be32	srds1cr1;	/* 0x.0f08 - SerDes1 Control Register 0 */
+	u32	srds1cr0;	/* 0x.0f04 - SerDes1 Control Register 0 */
+	u32	srds1cr1;	/* 0x.0f08 - SerDes1 Control Register 0 */
 	u8	resf0c[0xf2c - 0xf0c];
-	__be32  itcr;		/* 0x.0f2c - Internal transaction control register */
+	u32	itcr;		/* 0x.0f2c - Internal transaction control register */
 	u8	resf30[0xf40 - 0xf30];
-	__be32	srds2cr0;	/* 0x.0f40 - SerDes2 Control Register 0 */
-	__be32	srds2cr1;	/* 0x.0f44 - SerDes2 Control Register 0 */
+	u32	srds2cr0;	/* 0x.0f40 - SerDes2 Control Register 0 */
+	u32	srds2cr1;	/* 0x.0f44 - SerDes2 Control Register 0 */
 } __attribute__ ((packed));
 
+#ifdef CONFIG_FSL_GUTS
+extern struct ccsr_guts __iomem *guts_regmap(void);
+extern void guts_set_reg8(void __iomem *reg, u8 value);
+extern void guts_set_reg32(void __iomem *reg, u32 value);
+extern u8 guts_get_reg8(void __iomem *reg);
+extern u32 guts_get_reg32(void __iomem *reg);
+#endif
 
 /* Alternate function signal multiplex control */
 #define MPC85xx_PMUXCR_QE(x) (0x8000 >> (x))
-- 
2.1.0.27.g96db324

Re: [v4, 1/6] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Mon, 2015-12-14 at 12:24 +0800, Yangbo Lu wrote:
> The global utilities block controls power management, I/O device
> enabling, power-onreset(POR) configuration monitoring, alternate
> function selection for multiplexed signals,and clock control.
> 
> This patch adds GUTS driver to manage and access global utilities
> block.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> Changes for v4:
> 	- Added this patch
> ---
>  drivers/soc/Kconfig      |   1 +
>  drivers/soc/Makefile     |   1 +
>  drivers/soc/fsl/Kconfig  |  19 ++++++++
>  drivers/soc/fsl/Makefile |   4 ++
>  drivers/soc/fsl/guts.c   | 112
> +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fsl/guts.h | 103 +++++++++++++++++++++++--------------------
>  6 files changed, 192 insertions(+), 48 deletions(-)
>  create mode 100644 drivers/soc/fsl/Kconfig
>  create mode 100644 drivers/soc/fsl/Makefile
>  create mode 100644 drivers/soc/fsl/guts.c
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 4e853ed..68b5b90 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -7,5 +7,6 @@ source "drivers/soc/rockchip/Kconfig"
>  source "drivers/soc/sunxi/Kconfig"
>  source "drivers/soc/ti/Kconfig"
>  source "drivers/soc/versatile/Kconfig"
> +source "drivers/soc/fsl/Kconfig"
>  
>  endmenu
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index f2ba2e9..2747e58 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_ARCH_SUNXI)	+= sunxi/
>  obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
>  obj-$(CONFIG_SOC_TI)		+= ti/
>  obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
> +obj-$(CONFIG_SOC_FSL)		+= fsl/
> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
> new file mode 100644
> index 0000000..09966f0
> --- /dev/null
> +++ b/drivers/soc/fsl/Kconfig
> @@ -0,0 +1,19 @@
> +#
> +# FSL SOC drivers
> +#
> +menuconfig SOC_FSL
> +	bool "Freescale SOC drivers support"
> +
> +if SOC_FSL
> +
> +config FSL_GUTS
> +	tristate "QorIQ Platforms GUTS Driver"
> +	help
> +	  Say y here to enable Freescale QorIQ platforms GUTS driver
> support.
> +	  The global utilities block controls power management, I/O device
> +	  enabling, power-onreset(POR) configuration monitoring, alternate
> +	  function selection for multiplexed signals,and clock control.
> +
> +	  If unsure, say N.

This doesn't do anything user-visible (so far, at least) so it should just be
selected by the drivers that need it.

> +/*
> + * Table for matching compatible strings, for device tree
> + * guts node, for Freescale QorIQ SOCs.
> + * "fsl,qoriq-device-config-2.0" corresponds to T4 & B4
> + * SOCs. For the older SOCs "fsl,qoriq-device-config-1.0"
> + * string would be used.
> + */
> +static const struct of_device_id guts_device_ids[] = {
> +	{ .compatible = "fsl,qoriq-device-config-1.0", },
> +	{ .compatible = "fsl,qoriq-device-config-2.0", },
> +	{}
> +};

What about pre-corenet chips, with compatibles such as "fsl,p2020-guts"?  What
compatible gets used on Layerscape chips?

> +
> +struct ccsr_guts __iomem *guts_regmap(void)
> +{
> +	struct device_node *guts_node;
> +	struct ccsr_guts __iomem *guts;
> +
> +	guts_node = of_find_matching_node(NULL, guts_device_ids);
> +	if (!guts_node)
> +		return NULL;
> +
> +	guts = of_iomap(guts_node, 0);
> +	if (!guts)
> +		return NULL;
> +
> +	of_node_put(guts_node);
> +	return guts;
> +}
> +EXPORT_SYMBOL_GPL(guts_regmap);

This should not be exported.  This should be a normal driver that gets probed
and does its own init.  Callers to this driver should -EPROBE_DEFER if it's
not available yet, and this driver should probably register itself in a
subsys_initcall() or even arch_initcall() to reduce the likelihood of that
(especially if -EPROBE_DEFER would have resulted in deferring another driver
to a phase later than it wanted to init in).


> +
> +u8 guts_get_reg8(void __iomem *reg)
> +{
> +	u8 val;
> +
> +	val = ioread8(reg);
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(guts_get_reg8);
> +
> +void guts_set_reg8(void __iomem *reg, u8 value)
> +{
> +	iowrite8(value, reg);
> +}
> +EXPORT_SYMBOL_GPL(guts_set_reg8);
> +
> +u32 guts_get_reg32(void __iomem *reg)
> +{
> +	struct device_node *guts_node;
> +	u32 val;
> +
> +	guts_node = of_find_matching_node(NULL, guts_device_ids);
> +	if (!guts_node)
> +		return 0;
> +
> +	if (of_property_read_bool(guts_node, "little-endian"))
> +		val = ioread32(reg);
> +	else
> +		val = ioread32be(reg);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(guts_get_reg32);
> +
> +void guts_set_reg32(void __iomem *reg, u32 value)
> +{
> +	struct device_node *guts_node;
> +
> +	guts_node = of_find_matching_node(NULL, guts_device_ids);
> +	if (!guts_node)
> +		return;
> +
> +	if (of_property_read_bool(guts_node, "little-endian"))
> +		iowrite32(value, reg);
> +	else
> +		iowrite32be(value, reg);
> +}
> +EXPORT_SYMBOL_GPL(guts_set_reg32);

No.  Export fsl_guts_get_svr().

Also, read the little-endian property once at driver init, not on each access.

> +static int __init guts_drv_init(void)
> +{
> +	pr_info("guts: Freescale QorIQ Platforms GUTS Driver\n");
> +	return 0;
> +}
> +module_init(guts_drv_init);
> +
> +static void __exit guts_drv_exit(void)
> +{
> +}
> +module_exit(guts_drv_exit);

Get rid of the print, especially since it prints regardless of whether the
hardware is present.

> +
> +MODULE_AUTHOR("Yangbo Lu <yangbo.lu@freescale.com>");
> +MODULE_DESCRIPTION("Freescale QorIQ Platforms GUTS Driver");
> +MODULE_LICENSE("GPL v2");

The copyright header says "v2 or later" so MODULE_LICENSE should be just
"GPL".

-Scott

[v4, 2/6] dt: move guts devicetree doc out of powerpc directory

[Yangbo Lu]

Move guts devicetree doc to Documentation/devicetree/bindings/soc/
since it's used by not only PowerPC but also ARM. Add a specification
for 'little-endian' property.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- Added this patch
---
 Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt | 3 +++
 1 file changed, 3 insertions(+)
 rename Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt (90%)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt b/Documentation/devicetree/bindings/soc/guts.txt
similarity index 90%
rename from Documentation/devicetree/bindings/powerpc/fsl/guts.txt
rename to Documentation/devicetree/bindings/soc/guts.txt
index b71b203..6ae2269 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt
+++ b/Documentation/devicetree/bindings/soc/guts.txt
@@ -25,6 +25,9 @@ Recommended properties:
  - fsl,liodn-bits : Indicates the number of defined bits in the LIODN
    registers, for those SOCs that have a PAMU device.
 
+ - little-endian : Indicates that the global utilities block is little
+   endian mode. And it's big endian mode in default.
+
 Examples:
 	global-utilities@e0000 {	/* global utilities block */
 		compatible = "fsl,mpc8548-guts";
-- 
2.1.0.27.g96db324

Re: [v4, 2/6] dt: move guts devicetree doc out of powerpc directory

[Scott Wood]

On Mon, 2015-12-14 at 12:24 +0800, Yangbo Lu wrote:
> Move guts devicetree doc to Documentation/devicetree/bindings/soc/
> since it's used by not only PowerPC but also ARM. Add a specification
> for 'little-endian' property.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> Changes for v4:
> 	- Added this patch
> ---
>  Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt | 3 +++
>  1 file changed, 3 insertions(+)
>  rename Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt
> (90%)
> 
> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt
> b/Documentation/devicetree/bindings/soc/guts.txt
> similarity index 90%
> rename from Documentation/devicetree/bindings/powerpc/fsl/guts.txt
> rename to Documentation/devicetree/bindings/soc/guts.txt
> index b71b203..6ae2269 100644
> --- a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt
> +++ b/Documentation/devicetree/bindings/soc/guts.txt
> @@ -25,6 +25,9 @@ Recommended properties:
>   - fsl,liodn-bits : Indicates the number of defined bits in the LIODN
>     registers, for those SOCs that have a PAMU device.
>  
> + - little-endian : Indicates that the global utilities block is little
> +   endian mode. And it's big endian mode in default.
> +
>  Examples:
>  	global-utilities@e0000 {	/* global utilities block */
>  		compatible = "fsl,mpc8548-guts";

...bindings/soc/fsl/guts.txt

s/endian mode/endian/g

s/And it's big endian mode in default/The default is big endian/

-Scott

[v4, 3/6] powerpc/fsl: move mpc85xx.h to include/linux

[Yangbo Lu]

Move mpc85xx.h to include/linux and rename it to svr.h as
a common header file. It has been used for mpc85xx and it will
be used for ARM-based SoC as well.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- None
---
 drivers/clk/clk-qoriq.c                                   | 2 +-
 drivers/i2c/busses/i2c-mpc.c                              | 2 +-
 drivers/iommu/fsl_pamu.c                                  | 2 +-
 drivers/net/ethernet/freescale/gianfar.c                  | 2 +-
 arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)
 rename arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h (97%)

diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index 1ab0fb8..98c5ae0 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -1146,7 +1146,7 @@ bad_args:
 }
 
 #ifdef CONFIG_PPC
-#include <asm/mpc85xx.h>
+#include <linux/svr.h>
 
 static const u32 a4510_svrs[] __initconst = {
 	(SVR_P2040 << 8) | 0x10,	/* P2040 1.0 */
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 48ecffe..24b5bc7 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -27,9 +27,9 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
+#include <linux/svr.h>
 
 #include <asm/mpc52xx.h>
-#include <asm/mpc85xx.h>
 #include <sysdev/fsl_soc.h>
 
 #define DRV_NAME "mpc-i2c"
diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
index a34355f..b014b63 100644
--- a/drivers/iommu/fsl_pamu.c
+++ b/drivers/iommu/fsl_pamu.c
@@ -24,7 +24,7 @@
 #include <linux/interrupt.h>
 #include <linux/genalloc.h>
 
-#include <asm/mpc85xx.h>
+#include <linux/svr.h>
 
 /* define indexes for each operation mapping scenario */
 #define OMI_QMAN        0x00
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 3e6b9b4..9b2d641 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -90,7 +90,7 @@
 #include <asm/io.h>
 #ifdef CONFIG_PPC
 #include <asm/reg.h>
-#include <asm/mpc85xx.h>
+#include <linux/svr.h>
 #endif
 #include <asm/irq.h>
 #include <asm/uaccess.h>
diff --git a/arch/powerpc/include/asm/mpc85xx.h b/include/linux/svr.h
similarity index 97%
rename from arch/powerpc/include/asm/mpc85xx.h
rename to include/linux/svr.h
index 213f3a8..531b5a9 100644
--- a/arch/powerpc/include/asm/mpc85xx.h
+++ b/include/linux/svr.h
@@ -9,8 +9,8 @@
  * (at your option) any later version.
  */
 
-#ifndef __ASM_PPC_MPC85XX_H
-#define __ASM_PPC_MPC85XX_H
+#ifndef __SVR_H__
+#define __SVR_H__
 
 #define SVR_REV(svr)	((svr) & 0xFF)		/* SOC design resision */
 #define SVR_MAJ(svr)	(((svr) >>  4) & 0xF)	/* Major revision field*/
-- 
2.1.0.27.g96db324

Re: [v4, 3/6] powerpc/fsl: move mpc85xx.h to include/linux

[Scott Wood]

On Mon, 2015-12-14 at 12:24 +0800, Yangbo Lu wrote:
> Move mpc85xx.h to include/linux and rename it to svr.h as
> a common header file. It has been used for mpc85xx and it will
> be used for ARM-based SoC as well.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> Changes for v4:
> 	- None
> ---
>  drivers/clk/clk-qoriq.c                                   | 2 +-
>  drivers/i2c/busses/i2c-mpc.c                              | 2 +-
>  drivers/iommu/fsl_pamu.c                                  | 2 +-
>  drivers/net/ethernet/freescale/gianfar.c                  | 2 +-
>  arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h | 4 ++--
>  5 files changed, 6 insertions(+), 6 deletions(-)
>  rename arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h (97%)

include/linux/fsl/svr.h


> diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
> index 1ab0fb8..98c5ae0 100644
> --- a/drivers/clk/clk-qoriq.c
> +++ b/drivers/clk/clk-qoriq.c
> @@ -1146,7 +1146,7 @@ bad_args:
>  }
>  
>  #ifdef CONFIG_PPC
> -#include <asm/mpc85xx.h>
> +#include <linux/svr.h>

Now that it's not PPC-specific the #include can go to the top of the file with
the rest of them.

>  
>  static const u32 a4510_svrs[] __initconst = {
>  	(SVR_P2040 << 8) | 0x10,	/* P2040 1.0 */
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 48ecffe..24b5bc7 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -27,9 +27,9 @@
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/svr.h>
>  
>  #include <asm/mpc52xx.h>
> -#include <asm/mpc85xx.h>
>  #include <sysdev/fsl_soc.h>
>  
>  #define DRV_NAME "mpc-i2c"
> diff --git a/drivers/iommu/fsl_pamu.c b/drivers/iommu/fsl_pamu.c
> index a34355f..b014b63 100644
> --- a/drivers/iommu/fsl_pamu.c
> +++ b/drivers/iommu/fsl_pamu.c
> @@ -24,7 +24,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/genalloc.h>
>  
> -#include <asm/mpc85xx.h>
> +#include <linux/svr.h>

Move it with the other linux/ includes (i.e. remove the blank line before
this).

>  
>  /* define indexes for each operation mapping scenario */
>  #define OMI_QMAN        0x00
> diff --git a/drivers/net/ethernet/freescale/gianfar.c
> b/drivers/net/ethernet/freescale/gianfar.c
> index 3e6b9b4..9b2d641 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -90,7 +90,7 @@
>  #include <asm/io.h>
>  #ifdef CONFIG_PPC
>  #include <asm/reg.h>
> -#include <asm/mpc85xx.h>
> +#include <linux/svr.h>
>  #endif
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>

Same comment as clk-qoriq.c -- this can come out of the ifdef and go with the
other linux/ includes.

-Scott

[v4, 4/6] mmc: sdhci-of-esdhc: get SVR from global utilities registers

[Yangbo Lu]

Most of silicon errata about the eSDHC need to be identified through
the SoC version/revision. This patch makes the driver get these
information from SVR(system version register) of global utilities
registers.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- Got SVR through iomap instead of dts
Changes for v3:
	- Managed GUTS through syscon instead of iomap in eSDHC driver
Changes for v4:
	- Got SVR by GUTS driver instead of SYSCON
---
 drivers/mmc/host/sdhci-of-esdhc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 83b1226..9105888 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -18,6 +18,8 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/svr.h>
+#include <linux/fsl/guts.h>
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
@@ -28,6 +30,8 @@
 struct sdhci_esdhc {
 	u8 vendor_ver;
 	u8 spec_ver;
+	u32 soc_ver;
+	u8 soc_rev;
 };
 
 /**
@@ -566,18 +570,27 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_esdhc *esdhc;
+	struct ccsr_guts __iomem *guts;
+	u32 svr;
 	u16 host_ver;
 
 	pltfm_host = sdhci_priv(host);
 	esdhc = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_esdhc),
 			     GFP_KERNEL);
+	pltfm_host->priv = esdhc;
+
+	guts = guts_regmap();
+	if (guts) {
+		svr = guts_get_reg32(&guts->svr);
+		esdhc->soc_ver = SVR_SOC_VER(svr);
+		esdhc->soc_rev = SVR_REV(svr);
+	} else
+		dev_err(&pdev->dev, "unable to get SVR value!\n");
 
 	host_ver = sdhci_readw(host, SDHCI_HOST_VERSION);
 	esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
 			     SDHCI_VENDOR_VER_SHIFT;
 	esdhc->spec_ver = host_ver & SDHCI_SPEC_VER_MASK;
-
-	pltfm_host->priv = esdhc;
 }
 
 static int sdhci_esdhc_probe(struct platform_device *pdev)
-- 
2.1.0.27.g96db324

[v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

The sdhci-of-esdhc driver needs the GUTS driver support
to access the global utilities registers for SVR value.
So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- Added this patch
---
 drivers/mmc/host/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1526b8a..df57c14 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
 	depends on MMC_SDHCI_PLTFM
 	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
 	select MMC_SDHCI_IO_ACCESSORS
+	select SOC_FSL
+	select FSL_GUTS
 	help
 	  This selects the Freescale eSDHC controller support.
 
-- 
2.1.0.27.g96db324

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

On 14 December 2015 at 05:24, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> The sdhci-of-esdhc driver needs the GUTS driver support
> to access the global utilities registers for SVR value.
> So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
>         - None
> Changes for v3:
>         - None
> Changes for v4:
>         - Added this patch
> ---
>  drivers/mmc/host/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..df57c14 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>         depends on MMC_SDHCI_PLTFM
>         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>         select MMC_SDHCI_IO_ACCESSORS
> +       select SOC_FSL
> +       select FSL_GUTS

This is weird.

First, perhaps it would make sense to have stub functions for those
the FSL_GUTS driver provides via its API, thus the above wouldn't be
required at all. Of course this makes only sense if you think there
are/could be configurations for a cross SOC driver which don't need
the GUTS driver.

Second, even if you think the stubs above is a bad idea, I would from
the top-level Kconfig for your platform, add the needed "selects" as I
think it's where it belongs and then change this to "depends on"
instead.

Kind regards
Uffe

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Mon, 2015-12-14 at 14:08 +0100, Ulf Hansson wrote:
> On 14 December 2015 at 05:24, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> > The sdhci-of-esdhc driver needs the GUTS driver support
> > to access the global utilities registers for SVR value.
> > So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
> > 
> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> > ---
> > Changes for v2:
> >         - None
> > Changes for v3:
> >         - None
> > Changes for v4:
> >         - Added this patch
> > ---
> >  drivers/mmc/host/Kconfig | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 1526b8a..df57c14 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> >         depends on MMC_SDHCI_PLTFM
> >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> >         select MMC_SDHCI_IO_ACCESSORS
> > +       select SOC_FSL
> > +       select FSL_GUTS
> 
> This is weird.
> 
> First, perhaps it would make sense to have stub functions for those
> the FSL_GUTS driver provides via its API, thus the above wouldn't be
> required at all. Of course this makes only sense if you think there
> are/could be configurations for a cross SOC driver which don't need
> the GUTS driver.
> 
> Second, even if you think the stubs above is a bad idea, I would from
> the top-level Kconfig for your platform, add the needed "selects" as I
> think it's where it belongs and then change this to "depends on"
> instead.

Why is it weird for a driver to select another driver that it makes calls to? 
 Much easier to do it here than in all the platforms that use this driver.

And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
build but it will silently not be able to apply SVR-based workarounds.

-Scott

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

[...]
>> > --- a/drivers/mmc/host/Kconfig
>> > +++ b/drivers/mmc/host/Kconfig
>> > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>> >         depends on MMC_SDHCI_PLTFM
>> >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>> >         select MMC_SDHCI_IO_ACCESSORS
>> > +       select SOC_FSL
>> > +       select FSL_GUTS
>>
>> This is weird.
>>
>> First, perhaps it would make sense to have stub functions for those
>> the FSL_GUTS driver provides via its API, thus the above wouldn't be
>> required at all. Of course this makes only sense if you think there
>> are/could be configurations for a cross SOC driver which don't need
>> the GUTS driver.
>>
>> Second, even if you think the stubs above is a bad idea, I would from
>> the top-level Kconfig for your platform, add the needed "selects" as I
>> think it's where it belongs and then change this to "depends on"
>> instead.
>
> Why is it weird for a driver to select another driver that it makes calls to?
>  Much easier to do it here than in all the platforms that use this driver.

Because using "select" will not consider the dependencies for the new
selected Kconfig option. I can imagine that it might become a problem,
sooner or later.

So, "select" shall be used by care and in this case I think we can
cope fine with using "depends on".

>
> And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
> build but it will silently not be able to apply SVR-based workarounds.

It doesn't have to be "silent", the driver can return an error (and
print error messages) from its ->probe() method, if the calls to the
GUTS driver fails.

Anyway, I mentioned this idea only to understand the need for
*optional* GUTS supports. Perhaps there is a cross SOC drivers that
for some platforms depends on GUTS but on others it doesn't.

Maybe that isn't case then!?

Kind regards
Uffe

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
> [...]
> > > > --- a/drivers/mmc/host/Kconfig
> > > > +++ b/drivers/mmc/host/Kconfig
> > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> > > >         depends on MMC_SDHCI_PLTFM
> > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> > > >         select MMC_SDHCI_IO_ACCESSORS
> > > > +       select SOC_FSL
> > > > +       select FSL_GUTS
> > > 
> > > This is weird.
> > > 
> > > First, perhaps it would make sense to have stub functions for those
> > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
> > > required at all. Of course this makes only sense if you think there
> > > are/could be configurations for a cross SOC driver which don't need
> > > the GUTS driver.
> > > 
> > > Second, even if you think the stubs above is a bad idea, I would from
> > > the top-level Kconfig for your platform, add the needed "selects" as I
> > > think it's where it belongs and then change this to "depends on"
> > > instead.
> > 
> > Why is it weird for a driver to select another driver that it makes calls
> > to?
> >  Much easier to do it here than in all the platforms that use this driver.
> 
> Because using "select" will not consider the dependencies for the new
> selected Kconfig option. I can imagine that it might become a problem,
> sooner or later.

It's not a problem as long as the selected option's dependencies (if any) are
selected, or depended on by the selecting driver.  I wouldn't expect the
FSL_GUTS driver to depend on anything other than basic OF support.

> So, "select" shall be used by care and in this case I think we can
> cope fine with using "depends on".

What does select exist for if not situations like this?  What "care" is
missing?

-Scott

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

On 16 December 2015 at 23:48, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
>> [...]
>> > > > --- a/drivers/mmc/host/Kconfig
>> > > > +++ b/drivers/mmc/host/Kconfig
>> > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>> > > >         depends on MMC_SDHCI_PLTFM
>> > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>> > > >         select MMC_SDHCI_IO_ACCESSORS
>> > > > +       select SOC_FSL
>> > > > +       select FSL_GUTS
>> > >
>> > > This is weird.
>> > >
>> > > First, perhaps it would make sense to have stub functions for those
>> > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
>> > > required at all. Of course this makes only sense if you think there
>> > > are/could be configurations for a cross SOC driver which don't need
>> > > the GUTS driver.
>> > >
>> > > Second, even if you think the stubs above is a bad idea, I would from
>> > > the top-level Kconfig for your platform, add the needed "selects" as I
>> > > think it's where it belongs and then change this to "depends on"
>> > > instead.
>> >
>> > Why is it weird for a driver to select another driver that it makes calls
>> > to?
>> >  Much easier to do it here than in all the platforms that use this driver.
>>
>> Because using "select" will not consider the dependencies for the new
>> selected Kconfig option. I can imagine that it might become a problem,
>> sooner or later.
>
> It's not a problem as long as the selected option's dependencies (if any) are
> selected, or depended on by the selecting driver.  I wouldn't expect the
> FSL_GUTS driver to depend on anything other than basic OF support.
>
>> So, "select" shall be used by care and in this case I think we can
>> cope fine with using "depends on".
>
> What does select exist for if not situations like this?  What "care" is
> missing?
>

The GUTS driver is SoC specific driver. If/when dependencies are added
for that driver it will break the usage of the select for the MMC host
driver. No, thanks it's fragile!

To me, the best way to use select is either from top-level platform
Kconfig or where it's better isolated, perhaps within a subsystem.

Kind regards
Uffe

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Thu, 2015-12-17 at 12:25 +0100, Ulf Hansson wrote:
> On 16 December 2015 at 23:48, Scott Wood <scottwood@freescale.com> wrote:
> > On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
> > > [...]
> > > > > > --- a/drivers/mmc/host/Kconfig
> > > > > > +++ b/drivers/mmc/host/Kconfig
> > > > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> > > > > >         depends on MMC_SDHCI_PLTFM
> > > > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> > > > > >         select MMC_SDHCI_IO_ACCESSORS
> > > > > > +       select SOC_FSL
> > > > > > +       select FSL_GUTS
> > > > > 
> > > > > This is weird.
> > > > > 
> > > > > First, perhaps it would make sense to have stub functions for those
> > > > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
> > > > > required at all. Of course this makes only sense if you think there
> > > > > are/could be configurations for a cross SOC driver which don't need
> > > > > the GUTS driver.
> > > > > 
> > > > > Second, even if you think the stubs above is a bad idea, I would
> > > > > from
> > > > > the top-level Kconfig for your platform, add the needed "selects" as
> > > > > I
> > > > > think it's where it belongs and then change this to "depends on"
> > > > > instead.
> > > > 
> > > > Why is it weird for a driver to select another driver that it makes
> > > > calls
> > > > to?
> > > >  Much easier to do it here than in all the platforms that use this
> > > > driver.
> > > 
> > > Because using "select" will not consider the dependencies for the new
> > > selected Kconfig option. I can imagine that it might become a problem,
> > > sooner or later.
> > 
> > It's not a problem as long as the selected option's dependencies (if any)
> > are
> > selected, or depended on by the selecting driver.  I wouldn't expect the
> > FSL_GUTS driver to depend on anything other than basic OF support.
> > 
> > > So, "select" shall be used by care and in this case I think we can
> > > cope fine with using "depends on".
> > 
> > What does select exist for if not situations like this?  What "care" is
> > missing?
> > 
> 
> The GUTS driver is SoC specific driver. If/when dependencies are added
> for that driver it will break the usage of the select for the MMC host
> driver. No, thanks it's fragile!
> 
> To me, the best way to use select is either from top-level platform
> Kconfig or where it's better isolated, perhaps within a subsystem.

The guts driver is not going to depend on anything that the esdhc driver
shouldn't already be depending on (why does MMC_SDHCI_OF_ESDHC not currently
depend on OF?) and if the guts driver ever did grow a dependency that isn't
met but it is selected anyway, kconfig will warn.  I realize that growing a
guts dependency list in every user would be bad but I just don't see such
dependencies happening.

-Scott

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

[...]

>>
>> And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
>> build but it will silently not be able to apply SVR-based workarounds.
>
> It doesn't have to be "silent", the driver can return an error (and
> print error messages) from its ->probe() method, if the calls to the
> GUTS driver fails.
>
> Anyway, I mentioned this idea only to understand the need for
> *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> for some platforms depends on GUTS but on others it doesn't.
>
> Maybe that isn't case then!?

Can you please answer this question!?

According to the earlier versions of this patchset and from your
comments [1], it *do* seems like the GUTS driver may be optional and
thus stubs could address this.

Kind regards
Uffe

[1]
http://www.spinics.net/lists/linux-mmc/msg34412.html

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Thursday, December 17, 2015 7:31 PM
> To: Scott Wood
> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> [...]
> 
> >>
> >> And I think stubs for reading SVR is quite a bad idea.  It'll make
> >> the driver build but it will silently not be able to apply SVR-based
> workarounds.
> >
> > It doesn't have to be "silent", the driver can return an error (and
> > print error messages) from its ->probe() method, if the calls to the
> > GUTS driver fails.
> >
> > Anyway, I mentioned this idea only to understand the need for
> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > for some platforms depends on GUTS but on others it doesn't.
> >
> > Maybe that isn't case then!?
> 
> Can you please answer this question!?
> 
> According to the earlier versions of this patchset and from your comments
> [1], it *do* seems like the GUTS driver may be optional and thus stubs
> could address this.
> 
> Kind regards
> Uffe
> 
> [1]
> http://www.spinics.net/lists/linux-mmc/msg34412.html

[Lu Yangbo-B47093] Hi Scott and Uffe, 
In the earlier version, I'd like to use syscon support and only add 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
But I never thought this had caused so much discussion... :(
Could we reach an agreement about the 'optional' or not 'optional'?

Thank you so much!

 

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, December 17, 2015 7:31 PM
>> To: Scott Wood
>> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
>> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
>> MMC_SDHCI_OF_ESDHC
>>
>> [...]
>>
>> >>
>> >> And I think stubs for reading SVR is quite a bad idea.  It'll make
>> >> the driver build but it will silently not be able to apply SVR-based
>> workarounds.
>> >
>> > It doesn't have to be "silent", the driver can return an error (and
>> > print error messages) from its ->probe() method, if the calls to the
>> > GUTS driver fails.
>> >
>> > Anyway, I mentioned this idea only to understand the need for
>> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
>> > for some platforms depends on GUTS but on others it doesn't.
>> >
>> > Maybe that isn't case then!?
>>
>> Can you please answer this question!?
>>
>> According to the earlier versions of this patchset and from your comments
>> [1], it *do* seems like the GUTS driver may be optional and thus stubs
>> could address this.
>>
>> Kind regards
>> Uffe
>>
>> [1]
>> http://www.spinics.net/lists/linux-mmc/msg34412.html
>
> [Lu Yangbo-B47093] Hi Scott and Uffe,
> In the earlier version, I'd like to use syscon support and only add 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
> But I never thought this had caused so much discussion... :(

Sorry, I understand your frustration but that's life sometimes. :-)

To me, the syscon solution is more elegant...

> Could we reach an agreement about the 'optional' or not 'optional'?

...but I am fine with the current approach as well, as long as my
recent comments gets addressed. Let's make it optional.

Address Scott's and my review-comments, get other peoples ack for the
non-mmc parts, then I will happily pick up the patches.

Kind regards
Uffe

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > -----Original Message-----
> > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > Sent: Thursday, December 17, 2015 7:31 PM
> > > To: Scott Wood
> > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > MMC_SDHCI_OF_ESDHC
> > > 
> > > [...]
> > > 
> > > > > 
> > > > > And I think stubs for reading SVR is quite a bad idea.  It'll make
> > > > > the driver build but it will silently not be able to apply SVR-based
> > > workarounds.
> > > > 
> > > > It doesn't have to be "silent", the driver can return an error (and
> > > > print error messages) from its ->probe() method, if the calls to the
> > > > GUTS driver fails.
> > > > 
> > > > Anyway, I mentioned this idea only to understand the need for
> > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > > > for some platforms depends on GUTS but on others it doesn't.
> > > > 
> > > > Maybe that isn't case then!?
> > > 
> > > Can you please answer this question!?
> > > 
> > > According to the earlier versions of this patchset and from your
> > > comments
> > > [1], it *do* seems like the GUTS driver may be optional and thus stubs
> > > could address this.
> > > 
> > > Kind regards
> > > Uffe
> > > 
> > > [1]
> > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > 
> > [Lu Yangbo-B47093] Hi Scott and Uffe,
> > In the earlier version, I'd like to use syscon support and only add
> > 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
> > But I never thought this had caused so much discussion... :(
> 
> Sorry, I understand your frustration but that's life sometimes. :-)
> 
> To me, the syscon solution is more elegant...

The syscon patch was terrible.  It would have accessed a certain location in
any node labelled "syscon" whether it was guts or not, in addition to the
other complaints.

-Scott

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com]
> Sent: Tuesday, December 29, 2015 3:10 AM
> To: Ulf Hansson; Yangbo Lu
> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > > -----Original Message-----
> > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > Sent: Thursday, December 17, 2015 7:31 PM
> > > > To: Scott Wood
> > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > MMC_SDHCI_OF_ESDHC
> > > >
> > > > [...]
> > > >
> > > > > >
> > > > > > And I think stubs for reading SVR is quite a bad idea.  It'll
> > > > > > make the driver build but it will silently not be able to
> > > > > > apply SVR-based
> > > > workarounds.
> > > > >
> > > > > It doesn't have to be "silent", the driver can return an error
> > > > > (and print error messages) from its ->probe() method, if the
> > > > > calls to the GUTS driver fails.
> > > > >
> > > > > Anyway, I mentioned this idea only to understand the need for
> > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers
> > > > > that for some platforms depends on GUTS but on others it doesn't.
> > > > >
> > > > > Maybe that isn't case then!?
> > > >
> > > > Can you please answer this question!?
> > > >
> > > > According to the earlier versions of this patchset and from your
> > > > comments [1], it *do* seems like the GUTS driver may be optional
> > > > and thus stubs could address this.
> > > >
> > > > Kind regards
> > > > Uffe
> > > >
> > > > [1]
> > > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > >
> > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd
> > > like to use syscon support and only add 'syscon' compatible in the
> > > dts whose eSDHC needs to use it to get SVR.
> > > But I never thought this had caused so much discussion... :(
> >
> > Sorry, I understand your frustration but that's life sometimes. :-)
> >
> > To me, the syscon solution is more elegant...
> 
> The syscon patch was terrible.  It would have accessed a certain location
> in any node labelled "syscon" whether it was guts or not, in addition to
> the other complaints.
> 
> -Scott

[Lu Yangbo-B47093] As my understand, the syscon APIs would just check whether there is a 'syscon' compatible.
If no, the APIs return. We still could maintain a list of compatibles for guts if using syscon.
In my opinion, syscon and guts driver are just two method to get SVR.

I agree with Uffe, because I think syscon is really designed for this situation and many arm platforms are using it.
Of course, I still would like to try guts driver if you insist on it.

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Fri, 2016-01-08 at 06:17 +0000, Yangbo Lu wrote:
> > -----Original Message-----
> > From: Scott Wood
> > Sent: Friday, January 08, 2016 6:24 AM
> > To: Yangbo Lu; Scott Wood; Ulf Hansson
> > Cc: Lu Y.B.; linux-mmc; Xiaobo Xie; Li Leo
> > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > MMC_SDHCI_OF_ESDHC
> > 
> > On 01/06/2016 12:58 AM, Yangbo Lu wrote:
> > > > -----Original Message-----
> > > > From: Scott Wood [mailto:scottwood@freescale.com]
> > > > Sent: Tuesday, December 29, 2015 3:10 AM
> > > > To: Ulf Hansson; Yangbo Lu
> > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > MMC_SDHCI_OF_ESDHC
> > > > 
> > > > On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> > > > > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > > > > Sent: Thursday, December 17, 2015 7:31 PM
> > > > > > > To: Scott Wood
> > > > > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > > > > MMC_SDHCI_OF_ESDHC
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > And I think stubs for reading SVR is quite a bad idea. 
> > > > > > > > >  It'll
> > > > > > > > > make the driver build but it will silently not be able to
> > > > > > > > > apply
> > > > > > > > > SVR-based
> > > > > > > workarounds.
> > > > > > > > 
> > > > > > > > It doesn't have to be "silent", the driver can return an error
> > > > > > > > (and print error messages) from its ->probe() method, if the
> > > > > > > > calls to the GUTS driver fails.
> > > > > > > > 
> > > > > > > > Anyway, I mentioned this idea only to understand the need for
> > > > > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers
> > > > > > > > that for some platforms depends on GUTS but on others it
> > > > > > > > doesn't.
> > > > > > > > 
> > > > > > > > Maybe that isn't case then!?
> > > > > > > 
> > > > > > > Can you please answer this question!?
> > > > > > > 
> > > > > > > According to the earlier versions of this patchset and from your
> > > > > > > comments [1], it *do* seems like the GUTS driver may be optional
> > > > > > > and thus stubs could address this.
> > > > > > > 
> > > > > > > Kind regards
> > > > > > > Uffe
> > > > > > > 
> > > > > > > [1]
> > > > > > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > > > > > 
> > > > > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd
> > > > > > like to use syscon support and only add 'syscon' compatible in the
> > > > > > dts whose eSDHC needs to use it to get SVR.
> > > > > > But I never thought this had caused so much discussion... :(
> > > > > 
> > > > > Sorry, I understand your frustration but that's life sometimes. :-)
> > > > > 
> > > > > To me, the syscon solution is more elegant...
> > > > 
> > > > The syscon patch was terrible.  It would have accessed a certain
> > > > location in any node labelled "syscon" whether it was guts or not, in
> > > > addition to the other complaints.
> > > > 
> > > > -Scott
> > > 
> > > [Lu Yangbo-B47093] As my understand, the syscon APIs would just check
> > whether there is a 'syscon' compatible.
> > > If no, the APIs return. We still could maintain a list of compatibles
> > for guts if using syscon.
> > 
> > Where would that list of compatibles go?  Duplicated in every driver that
> > needs something from guts?  Why decentralize the implementation?
> > 
> > > In my opinion, syscon and guts driver are just two method to get SVR.
> > 
> > The guts driver could eventually handle more than just SVR, and it could
> > handle SVR better (e.g. on PPC fall back to reading the SPR if guts is
> > unavailable).
> > 
> > -Scott
> 
> [Lu Yangbo-B47093] Ok... It seems no proper place to maintain the guts list
> presently...
> I also have had a question about the guts driver.
> Could you suggest where to register the guts driver, to make it registered
> for all QorIQ platforms and avoid unavailability to call it?

In an initcall, just like other normal drivers.  Maybe subsys_initcall.

-Scott

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Yangbo Lu
> Sent: Wednesday, January 06, 2016 2:58 PM
> To: Scott Wood; Ulf Hansson
> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> Subject: RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> > -----Original Message-----
> > From: Scott Wood [mailto:scottwood@freescale.com]
> > Sent: Tuesday, December 29, 2015 3:10 AM
> > To: Ulf Hansson; Yangbo Lu
> > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > MMC_SDHCI_OF_ESDHC
> >
> > On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> > > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > > > -----Original Message-----
> > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > > Sent: Thursday, December 17, 2015 7:31 PM
> > > > > To: Scott Wood
> > > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > > MMC_SDHCI_OF_ESDHC
> > > > >
> > > > > [...]
> > > > >
> > > > > > >
> > > > > > > And I think stubs for reading SVR is quite a bad idea.
> > > > > > > It'll make the driver build but it will silently not be able
> > > > > > > to apply SVR-based
> > > > > workarounds.
> > > > > >
> > > > > > It doesn't have to be "silent", the driver can return an error
> > > > > > (and print error messages) from its ->probe() method, if the
> > > > > > calls to the GUTS driver fails.
> > > > > >
> > > > > > Anyway, I mentioned this idea only to understand the need for
> > > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers
> > > > > > that for some platforms depends on GUTS but on others it
> doesn't.
> > > > > >
> > > > > > Maybe that isn't case then!?
> > > > >
> > > > > Can you please answer this question!?
> > > > >
> > > > > According to the earlier versions of this patchset and from your
> > > > > comments [1], it *do* seems like the GUTS driver may be optional
> > > > > and thus stubs could address this.
> > > > >
> > > > > Kind regards
> > > > > Uffe
> > > > >
> > > > > [1]
> > > > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > > >
> > > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd
> > > > like to use syscon support and only add 'syscon' compatible in the
> > > > dts whose eSDHC needs to use it to get SVR.
> > > > But I never thought this had caused so much discussion... :(
> > >
> > > Sorry, I understand your frustration but that's life sometimes. :-)
> > >
> > > To me, the syscon solution is more elegant...
> >
> > The syscon patch was terrible.  It would have accessed a certain
> > location in any node labelled "syscon" whether it was guts or not, in
> > addition to the other complaints.
> >
> > -Scott
> 
> [Lu Yangbo-B47093] As my understand, the syscon APIs would just check
> whether there is a 'syscon' compatible.
> If no, the APIs return. We still could maintain a list of compatibles for
> guts if using syscon.

[Lu Yangbo-B47093] It's ok to use a compatible name that is not 'syscon' as the parameter of the APIs.
The node needs just to contain 'syscon'. 

> In my opinion, syscon and guts driver are just two method to get SVR.
> 
> I agree with Uffe, because I think syscon is really designed for this
> situation and many arm platforms are using it.
> Of course, I still would like to try guts driver if you insist on it.
> 
> 
> 
> 
> 

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Monday, December 28, 2015 8:10 PM
> To: Yangbo Lu
> Cc: Scott Wood; Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> >> Sent: Thursday, December 17, 2015 7:31 PM
> >> To: Scott Wood
> >> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> >> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> >> MMC_SDHCI_OF_ESDHC
> >>
> >> [...]
> >>
> >> >>
> >> >> And I think stubs for reading SVR is quite a bad idea.  It'll make
> >> >> the driver build but it will silently not be able to apply
> >> >> SVR-based
> >> workarounds.
> >> >
> >> > It doesn't have to be "silent", the driver can return an error (and
> >> > print error messages) from its ->probe() method, if the calls to
> >> > the GUTS driver fails.
> >> >
> >> > Anyway, I mentioned this idea only to understand the need for
> >> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> >> > for some platforms depends on GUTS but on others it doesn't.
> >> >
> >> > Maybe that isn't case then!?
> >>
> >> Can you please answer this question!?
> >>
> >> According to the earlier versions of this patchset and from your
> >> comments [1], it *do* seems like the GUTS driver may be optional and
> >> thus stubs could address this.
> >>
> >> Kind regards
> >> Uffe
> >>
> >> [1]
> >> http://www.spinics.net/lists/linux-mmc/msg34412.html
> >
> > [Lu Yangbo-B47093] Hi Scott and Uffe,
> > In the earlier version, I'd like to use syscon support and only add
> 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
> > But I never thought this had caused so much discussion... :(
> 
> Sorry, I understand your frustration but that's life sometimes. :-)
> 

[Lu Yangbo-B47093] Thank you so much for your understanding :)

> To me, the syscon solution is more elegant...
> 
> > Could we reach an agreement about the 'optional' or not 'optional'?
> 
> ...but I am fine with the current approach as well, as long as my recent
> comments gets addressed. Let's make it optional.
> 
> Address Scott's and my review-comments, get other peoples ack for the
> non-mmc parts, then I will happily pick up the patches.
> 

[Lu Yangbo-B47093] Thanks a lot. I would add other reviewers in next version.
 
> Kind regards
> Uffe

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Thu, 2015-12-17 at 12:30 +0100, Ulf Hansson wrote:
> [...]
> 
> > > 
> > > And I think stubs for reading SVR is quite a bad idea.  It'll make the
> > > driver
> > > build but it will silently not be able to apply SVR-based workarounds.
> > 
> > It doesn't have to be "silent", the driver can return an error (and
> > print error messages) from its ->probe() method, if the calls to the
> > GUTS driver fails.
> > 
> > Anyway, I mentioned this idea only to understand the need for
> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > for some platforms depends on GUTS but on others it doesn't.
> > 
> > Maybe that isn't case then!?
> 
> Can you please answer this question!?
> 
> According to the earlier versions of this patchset and from your
> comments [1], it *do* seems like the GUTS driver may be optional and
> thus stubs could address this.

I'd rather it not be optional at build time for the reason I explained above. 
 It would be too easy for users to accidentally not enable the guts driver and
miss the workaround.  Even if an error is printed it's easy to miss among all
the boot spam -- and if there's any legitimate reason to not have the driver
enabled then why would that be an "error"?  If the guts driver fails to bind
(e.g. running in a VM, or a platform the guts driver doesn't recognize) that's
another matter, but that should be handled by an error check in the guts
driver, not a build-time stub.

-Scott

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com]
> Sent: Tuesday, December 29, 2015 2:48 AM
> To: Ulf Hansson
> Cc: Yangbo Lu; linux-mmc; X.Xie@freescale.com; Leo li
> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> On Thu, 2015-12-17 at 12:30 +0100, Ulf Hansson wrote:
> > [...]
> >
> > > >
> > > > And I think stubs for reading SVR is quite a bad idea.  It'll make
> > > > the driver build but it will silently not be able to apply
> > > > SVR-based workarounds.
> > >
> > > It doesn't have to be "silent", the driver can return an error (and
> > > print error messages) from its ->probe() method, if the calls to the
> > > GUTS driver fails.
> > >
> > > Anyway, I mentioned this idea only to understand the need for
> > > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > > for some platforms depends on GUTS but on others it doesn't.
> > >
> > > Maybe that isn't case then!?
> >
> > Can you please answer this question!?
> >
> > According to the earlier versions of this patchset and from your
> > comments [1], it *do* seems like the GUTS driver may be optional and
> > thus stubs could address this.
> 
> I'd rather it not be optional at build time for the reason I explained
> above.
>  It would be too easy for users to accidentally not enable the guts
> driver and miss the workaround.  Even if an error is printed it's easy to
> miss among all the boot spam -- and if there's any legitimate reason to
> not have the driver enabled then why would that be an "error"?  If the
> guts driver fails to bind (e.g. running in a VM, or a platform the guts
> driver doesn't recognize) that's another matter, but that should be
> handled by an error check in the guts driver, not a build-time stub.
> 
> -Scott
> 

[Lu Yangbo-B47093] For the 'optional or not optional' problem, I think Scott's opinion is reasonable.
If we use guts driver, 'select' could avoid missing the workaround.
If we use syscon, select is better than 'depend on' since 'depend on' needs user self to find out the dependency.

Anyway, we should continue the discussion to come to an agreement finally.
I would very appreciate!

Thanks.

 

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Ulf Hansson]

> [Lu Yangbo-B47093] For the 'optional or not optional' problem, I think Scott's opinion is reasonable.
> If we use guts driver, 'select' could avoid missing the workaround.
> If we use syscon, select is better than 'depend on' since 'depend on' needs user self to find out the dependency.
>
> Anyway, we should continue the discussion to come to an agreement finally.
> I would very appreciate!

Please, go ahead and use the "select guts driver" option, as you both
seems to feel that strong about it.

Kind regards
Uffe

RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Yangbo Lu]

> -----Original Message-----
> From: Yangbo Lu
> Sent: Wednesday, January 06, 2016 3:18 PM
> To: Scott Wood; Ulf Hansson
> Cc: Yangbo Lu; linux-mmc; X.Xie@freescale.com; Leo li
> Subject: RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> MMC_SDHCI_OF_ESDHC
> 
> > -----Original Message-----
> > From: Scott Wood [mailto:scottwood@freescale.com]
> > Sent: Tuesday, December 29, 2015 2:48 AM
> > To: Ulf Hansson
> > Cc: Yangbo Lu; linux-mmc; X.Xie@freescale.com; Leo li
> > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > MMC_SDHCI_OF_ESDHC
> >
> > On Thu, 2015-12-17 at 12:30 +0100, Ulf Hansson wrote:
> > > [...]
> > >
> > > > >
> > > > > And I think stubs for reading SVR is quite a bad idea.  It'll
> > > > > make the driver build but it will silently not be able to apply
> > > > > SVR-based workarounds.
> > > >
> > > > It doesn't have to be "silent", the driver can return an error
> > > > (and print error messages) from its ->probe() method, if the calls
> > > > to the GUTS driver fails.
> > > >
> > > > Anyway, I mentioned this idea only to understand the need for
> > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers
> > > > that for some platforms depends on GUTS but on others it doesn't.
> > > >
> > > > Maybe that isn't case then!?
> > >
> > > Can you please answer this question!?
> > >
> > > According to the earlier versions of this patchset and from your
> > > comments [1], it *do* seems like the GUTS driver may be optional and
> > > thus stubs could address this.
> >
> > I'd rather it not be optional at build time for the reason I explained
> > above.
> >  It would be too easy for users to accidentally not enable the guts
> > driver and miss the workaround.  Even if an error is printed it's easy
> > to miss among all the boot spam -- and if there's any legitimate
> > reason to not have the driver enabled then why would that be an
> > "error"?  If the guts driver fails to bind (e.g. running in a VM, or a
> > platform the guts driver doesn't recognize) that's another matter, but
> > that should be handled by an error check in the guts driver, not a
> build-time stub.
> >
> > -Scott
> >
> 
> [Lu Yangbo-B47093] For the 'optional or not optional' problem, I think
> Scott's opinion is reasonable.
> If we use guts driver, 'select' could avoid missing the workaround.
> If we use syscon, select is better than 'depend on' since 'depend on'
> needs user self to find out the dependency.
> 
> Anyway, we should continue the discussion to come to an agreement finally.
> I would very appreciate!
> 
> Thanks.
> 
> 
[Lu Yangbo-B47093] Hi Uffe, do you have any comments? :)

Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

[Scott Wood]

On Mon, 2015-12-14 at 12:24 +0800, Yangbo Lu wrote:
> The sdhci-of-esdhc driver needs the GUTS driver support
> to access the global utilities registers for SVR value.
> So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> Changes for v4:
> 	- Added this patch
> ---
>  drivers/mmc/host/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..df57c14 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>  	depends on MMC_SDHCI_PLTFM
>  	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>  	select MMC_SDHCI_IO_ACCESSORS
> +	select SOC_FSL
> +	select FSL_GUTS
>  	help
>  	  This selects the Freescale eSDHC controller support.
>  

What is "SOC_FSL"?  If you mean "FSL_SOC", why are you selecting it?

-Scott

[v4, 6/6] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

[Yangbo Lu]

The eSDHC of T4240-R1.0-R2.0 has incorrect vender version and spec version.
Acturally, the right version numbers should be VVN=0x13 and SVN = 0x1.
So, fix it in driver to avoid that incorrect version numbers break down the
ADMA data transfer.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- Got SoC version/revision from struct sdhci_esdhc instead of global variables
Changes for v4
	- None
---
 drivers/mmc/host/sdhci-of-esdhc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 9105888..6ab94e7 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -77,6 +77,8 @@ static u32 esdhc_readl_fixup(struct sdhci_host *host,
 static u16 esdhc_readw_fixup(struct sdhci_host *host,
 				     int spec_reg, u32 value)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_esdhc *esdhc = pltfm_host->priv;
 	u16 ret;
 	int shift = (spec_reg & 0x2) * 8;
 
@@ -84,6 +86,13 @@ static u16 esdhc_readw_fixup(struct sdhci_host *host,
 		ret = value & 0xffff;
 	else
 		ret = (value >> shift) & 0xffff;
+
+	/* Workaround for T4240-R1.0-R2.0 eSDHC which has incorrect
+	 * vendor version and spec version information.
+	 */
+	if ((spec_reg == SDHCI_HOST_VERSION) &&
+	    (esdhc->soc_ver == SVR_T4240) && (esdhc->soc_rev <= 0x20))
+		ret = (VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200;
 	return ret;
 }
 
-- 
2.1.0.27.g96db324

Re: [v4, 0/6] eSDHC patches introduction

[Ulf Hansson]

On 14 December 2015 at 05:24, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> This patchset is used to fix host version in eSDHC_HOSTVER for T4240-R1.0-R2.0.
> To get the SoC version and revision, it's needed to add the GUTS driver to access
> the global utilities registers.
>
> So, the first three patches are to add the GUTS driver.
> The following two patches are to enable GUTS driver support
> to get SVR in eSDHC driver.
> The last patch is to fix host version for T4240.
>
> Yangbo Lu (6):
>   soc: fsl: add GUTS driver for QorIQ platforms
>   dt: move guts devicetree doc out of powerpc directory
>   powerpc/fsl: move mpc85xx.h to include/linux
>   mmc: sdhci-of-esdhc: get SVR from global utilities registers
>   mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC
>   mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
>
>  .../bindings/{powerpc/fsl => soc}/guts.txt         |   3 +
>  drivers/clk/clk-qoriq.c                            |   2 +-
>  drivers/i2c/busses/i2c-mpc.c                       |   2 +-
>  drivers/iommu/fsl_pamu.c                           |   2 +-
>  drivers/mmc/host/Kconfig                           |   2 +
>  drivers/mmc/host/sdhci-of-esdhc.c                  |  26 ++++-
>  drivers/net/ethernet/freescale/gianfar.c           |   2 +-
>  drivers/soc/Kconfig                                |   1 +
>  drivers/soc/Makefile                               |   1 +
>  drivers/soc/fsl/Kconfig                            |  19 ++++
>  drivers/soc/fsl/Makefile                           |   4 +
>  drivers/soc/fsl/guts.c                             | 112 +++++++++++++++++++++
>  include/linux/fsl/guts.h                           | 103 ++++++++++---------
>  .../include/asm/mpc85xx.h => include/linux/svr.h   |   4 +-
>  14 files changed, 227 insertions(+), 56 deletions(-)
>  rename Documentation/devicetree/bindings/{powerpc/fsl => soc}/guts.txt (90%)
>  create mode 100644 drivers/soc/fsl/Kconfig
>  create mode 100644 drivers/soc/fsl/Makefile
>  create mode 100644 drivers/soc/fsl/guts.c
>  rename arch/powerpc/include/asm/mpc85xx.h => include/linux/svr.h (97%)
>
> --
> 2.1.0.27.g96db324
>

I think you need some additional maintainers to be involved here.
There are more than mmc parts in this patchset.

Please re-post.

Kind regards
Uffe