[v11, 0/8] Fix eSDHC host version register bug

[v11, 0/8] Fix eSDHC host version register bug

[Yangbo Lu]

This patchset is used to fix a host version register bug in the T4240-R1.0-R2.0
eSDHC controller. To match the SoC version and revision, 10 previous version
patchsets had tried many methods but all of them were rejected by reviewers.
Such as
	- dts compatible method
	- syscon method
	- ifdef PPC method
	- GUTS driver getting SVR method
Anrd suggested a soc_device_match method in v10, and this is the only available
method left now. This v11 patchset introduces the soc_device_match interface in
soc driver.

The first six patches are to add the GUTS driver. This is used to register a
soc device which contain soc version and revision information.
The following two patches introduce the soc_device_match method in soc driver
and apply it on esdhc driver to fix this bug.

Yangbo Lu (8):
  dt: bindings: update Freescale DCFG compatible
  ARM64: dts: ls2080a: add device configuration node
  dt: bindings: move guts devicetree doc out of powerpc directory
  powerpc/fsl: move mpc85xx.h to include/linux/fsl
  soc: fsl: add GUTS driver for QorIQ platforms
  MAINTAINERS: add entry for Freescale SoC drivers
  base: soc: introduce soc_device_match() interface
  mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

 Documentation/devicetree/bindings/arm/fsl.txt      |   6 +-
 .../bindings/{powerpc => soc}/fsl/guts.txt         |   3 +
 MAINTAINERS                                        |  11 +-
 arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi     |   6 +
 arch/powerpc/kernel/cpu_setup_fsl_booke.S          |   2 +-
 arch/powerpc/sysdev/fsl_pci.c                      |   2 +-
 drivers/base/Kconfig                               |   1 +
 drivers/base/soc.c                                 |  61 +++
 drivers/clk/clk-qoriq.c                            |   3 +-
 drivers/i2c/busses/i2c-mpc.c                       |   2 +-
 drivers/iommu/fsl_pamu.c                           |   3 +-
 drivers/mmc/host/Kconfig                           |   1 +
 drivers/mmc/host/sdhci-of-esdhc.c                  |  20 +
 drivers/net/ethernet/freescale/gianfar.c           |   2 +-
 drivers/soc/Kconfig                                |   2 +-
 drivers/soc/fsl/Kconfig                            |  20 +
 drivers/soc/fsl/Makefile                           |   1 +
 drivers/soc/fsl/guts.c                             | 483 +++++++++++++++++++++
 include/linux/fsl/guts.h                           | 127 ++++--
 .../asm/mpc85xx.h => include/linux/fsl/svr.h       |   4 +-
 include/linux/sys_soc.h                            |   3 +
 21 files changed, 702 insertions(+), 61 deletions(-)
 rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)
 create mode 100644 drivers/soc/fsl/Kconfig
 create mode 100644 drivers/soc/fsl/guts.c
 rename arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h (97%)

-- 
2.1.0.27.g96db324

[v11, 1/8] dt: bindings: update Freescale DCFG compatible

[Yangbo Lu]

Update Freescale DCFG compatible with 'fsl,<chip>-dcfg' instead
of 'fsl,ls1021a-dcfg' to include more chips such as ls1021a,
ls1043a, and ls2080a.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Scott Wood <oss@buserror.net>
---
Changes for v8:
	- Added this patch
Changes for v9:
	- Added a list for the possible compatibles
Changes for v10:
	- None
Changes for v11:
	- Added 'Acked-by: Rob Herring'
	- Updated commit message by Scott
---
 Documentation/devicetree/bindings/arm/fsl.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
index dbbc095..713c1ae 100644
--- a/Documentation/devicetree/bindings/arm/fsl.txt
+++ b/Documentation/devicetree/bindings/arm/fsl.txt
@@ -119,7 +119,11 @@ Freescale DCFG
 configuration and status for the device. Such as setting the secondary
 core start address and release the secondary core from holdoff and startup.
   Required properties:
-  - compatible: should be "fsl,ls1021a-dcfg"
+  - compatible: should be "fsl,<chip>-dcfg"
+    Possible compatibles:
+	"fsl,ls1021a-dcfg"
+	"fsl,ls1043a-dcfg"
+	"fsl,ls2080a-dcfg"
   - reg : should contain base address and length of DCFG memory-mapped registers
 
 Example:
-- 
2.1.0.27.g96db324

[v11, 2/8] ARM64: dts: ls2080a: add device configuration node

[Yangbo Lu]

Add the dts node for device configuration unit that provides
general purpose configuration and status for the device.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Scott Wood <oss@buserror.net>
---
Changes for v5:
	- Added this patch
Changes for v6:
	- None
Changes for v7:
	- None
Changes for v8:
	- Added 'Acked-by: Scott Wood'
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- None
---
 arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
index 21023a3..39f7743 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
@@ -215,6 +215,12 @@
 			clocks = <&sysclk>;
 		};
 
+		dcfg: dcfg@1e00000 {
+			compatible = "fsl,ls2080a-dcfg", "syscon";
+			reg = <0x0 0x1e00000 0x0 0x10000>;
+			little-endian;
+		};
+
 		serial0: serial@21c0500 {
 			compatible = "fsl,ns16550", "ns16550a";
 			reg = <0x0 0x21c0500 0x0 0x100>;
-- 
2.1.0.27.g96db324

[v11, 3/8] dt: bindings: move guts devicetree doc out of powerpc directory

[Yangbo Lu]

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

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Scott Wood <oss@buserror.net>
---
Changes for v4:
	- Added this patch
Changes for v5:
	- Modified the description for little-endian property
Changes for v6:
	- None
Changes for v7:
	- None
Changes for v8:
	- Added 'Acked-by: Scott Wood'
	- Added 'Acked-by: Rob Herring'
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- None
---
 Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt | 3 +++
 1 file changed, 3 insertions(+)
 rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt b/Documentation/devicetree/bindings/soc/fsl/guts.txt
similarity index 91%
rename from Documentation/devicetree/bindings/powerpc/fsl/guts.txt
rename to Documentation/devicetree/bindings/soc/fsl/guts.txt
index b71b203..07adca9 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/guts.txt
+++ b/Documentation/devicetree/bindings/soc/fsl/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. The default is big endian.
+
 Examples:
 	global-utilities@e0000 {	/* global utilities block */
 		compatible = "fsl,mpc8548-guts";
-- 
2.1.0.27.g96db324

[v11, 4/8] powerpc/fsl: move mpc85xx.h to include/linux/fsl

[Yangbo Lu]

Move mpc85xx.h to include/linux/fsl and rename it to svr.h as a common
header file.  This SVR numberspace is used on some ARM chips as well as
PPC, and even to check for a PPC SVR multi-arch drivers would otherwise
need to ifdef the header inclusion and all references to the SVR symbols.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
Acked-by: Stephen Boyd <sboyd@codeaurora.org>
Acked-by: Joerg Roedel <jroedel@suse.de>
[scottwood: update description]
Signed-off-by: Scott Wood <oss@buserror.net>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- None
Changes for v5:
	- Changed to Move mpc85xx.h to include/linux/fsl/
	- Adjusted '#include <linux/fsl/svr.h>' position in file
Changes for v6:
	- None
Changes for v7:
	- Added 'Acked-by: Wolfram Sang' for I2C part
	- Also applied to arch/powerpc/kernel/cpu_setup_fsl_booke.S
Changes for v8:
	- Added 'Acked-by: Stephen Boyd' for clk part
	- Added 'Acked-by: Scott Wood'
	- Added 'Acked-by: Joerg Roedel' for iommu part
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- Updated description by Scott
---
 arch/powerpc/kernel/cpu_setup_fsl_booke.S                     | 2 +-
 arch/powerpc/sysdev/fsl_pci.c                                 | 2 +-
 drivers/clk/clk-qoriq.c                                       | 3 +--
 drivers/i2c/busses/i2c-mpc.c                                  | 2 +-
 drivers/iommu/fsl_pamu.c                                      | 3 +--
 drivers/net/ethernet/freescale/gianfar.c                      | 2 +-
 arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h | 4 ++--
 7 files changed, 8 insertions(+), 10 deletions(-)
 rename arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h (97%)

diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
index 462aed9..2b0284e 100644
--- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
+++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
@@ -13,13 +13,13 @@
  *
  */
 
+#include <linux/fsl/svr.h>
 #include <asm/page.h>
 #include <asm/processor.h>
 #include <asm/cputable.h>
 #include <asm/ppc_asm.h>
 #include <asm/mmu-book3e.h>
 #include <asm/asm-offsets.h>
-#include <asm/mpc85xx.h>
 
 _GLOBAL(__e500_icache_setup)
 	mfspr	r0, SPRN_L1CSR1
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 0ef9df4..0fd1895 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -22,6 +22,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/fsl/edac.h>
+#include <linux/fsl/svr.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/memblock.h>
@@ -37,7 +38,6 @@
 #include <asm/pci-bridge.h>
 #include <asm/ppc-pci.h>
 #include <asm/machdep.h>
-#include <asm/mpc85xx.h>
 #include <asm/disassemble.h>
 #include <asm/ppc-opcode.h>
 #include <sysdev/fsl_soc.h>
diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index 58566a17..4b6c438 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -13,6 +13,7 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/fsl/guts.h>
+#include <linux/fsl/svr.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -1149,8 +1150,6 @@ bad_args:
 }
 
 #ifdef CONFIG_PPC
-#include <asm/mpc85xx.h>
-
 static const u32 a4510_svrs[] __initconst = {
 	(SVR_P2040 << 8) | 0x10,	/* P2040 1.0 */
 	(SVR_P2040 << 8) | 0x11,	/* P2040 1.1 */
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 48ecffe..600704c 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/fsl/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..af8fb27 100644
--- a/drivers/iommu/fsl_pamu.c
+++ b/drivers/iommu/fsl_pamu.c
@@ -21,11 +21,10 @@
 #include "fsl_pamu.h"
 
 #include <linux/fsl/guts.h>
+#include <linux/fsl/svr.h>
 #include <linux/interrupt.h>
 #include <linux/genalloc.h>
 
-#include <asm/mpc85xx.h>
-
 /* define indexes for each operation mapping scenario */
 #define OMI_QMAN        0x00
 #define OMI_FMAN        0x01
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index d20935d..b8dac84 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -86,11 +86,11 @@
 #include <linux/udp.h>
 #include <linux/in.h>
 #include <linux/net_tstamp.h>
+#include <linux/fsl/svr.h>
 
 #include <asm/io.h>
 #ifdef CONFIG_PPC
 #include <asm/reg.h>
-#include <asm/mpc85xx.h>
 #endif
 #include <asm/irq.h>
 #include <asm/uaccess.h>
diff --git a/arch/powerpc/include/asm/mpc85xx.h b/include/linux/fsl/svr.h
similarity index 97%
rename from arch/powerpc/include/asm/mpc85xx.h
rename to include/linux/fsl/svr.h
index 213f3a8..8d13836 100644
--- a/arch/powerpc/include/asm/mpc85xx.h
+++ b/include/linux/fsl/svr.h
@@ -9,8 +9,8 @@
  * (at your option) any later version.
  */
 
-#ifndef __ASM_PPC_MPC85XX_H
-#define __ASM_PPC_MPC85XX_H
+#ifndef FSL_SVR_H
+#define FSL_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

[v11, 5/8] 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 a driver to manage and access global utilities block.
Initially only reading SVR and registering soc device are supported.
Other guts accesses, such as reading RCW, should eventually be moved
into this driver as well.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Signed-off-by: Scott Wood <oss@buserror.net>
---
Changes for v4:
	- Added this patch
Changes for v5:
	- Modified copyright info
	- Changed MODULE_LICENSE to GPL
	- Changed EXPORT_SYMBOL_GPL to EXPORT_SYMBOL
	- Made FSL_GUTS user-invisible
	- Added a complete compatible list for GUTS
	- Stored guts info in file-scope variable
	- Added mfspr() getting SVR
	- Redefined GUTS APIs
	- Called fsl_guts_init rather than using platform driver
	- Removed useless parentheses
	- Removed useless 'extern' key words
Changes for v6:
	- Made guts thread safe in fsl_guts_init
Changes for v7:
	- Removed 'ifdef' for function declaration in guts.h
Changes for v8:
	- Fixes lines longer than 80 characters checkpatch issue
	- Added 'Acked-by: Scott Wood'
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- Changed to platform driver
	- Registered soc device
---
 drivers/soc/Kconfig      |   2 +-
 drivers/soc/fsl/Kconfig  |  20 ++
 drivers/soc/fsl/Makefile |   1 +
 drivers/soc/fsl/guts.c   | 483 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fsl/guts.h | 127 ++++++++-----
 5 files changed, 584 insertions(+), 49 deletions(-)
 create mode 100644 drivers/soc/fsl/Kconfig
 create mode 100644 drivers/soc/fsl/guts.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index fe42a2f..f31bceb 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -1,7 +1,7 @@
 menu "SOC (System On Chip) specific Drivers"
 
 source "drivers/soc/bcm/Kconfig"
-source "drivers/soc/fsl/qe/Kconfig"
+source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/rockchip/Kconfig"
diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
new file mode 100644
index 0000000..439998d
--- /dev/null
+++ b/drivers/soc/fsl/Kconfig
@@ -0,0 +1,20 @@
+#
+# Freescale SOC drivers
+#
+
+source "drivers/soc/fsl/qe/Kconfig"
+
+config FSL_GUTS
+	bool "Freescale QorIQ GUTS driver"
+	select SOC_BUS
+	select GLOB
+	help
+	  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 driver is to manage and access global utilities block.
+	  Initially only reading SVR and registering soc device are supported.
+	  Other guts accesses, such as reading RCW, should eventually be moved
+	  into this driver as well.
+
+	  If you want GUTS driver support, you should say Y here.
diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
index 203307f..02afb7f 100644
--- a/drivers/soc/fsl/Makefile
+++ b/drivers/soc/fsl/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_QUICC_ENGINE)		+= qe/
 obj-$(CONFIG_CPM)			+= qe/
+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..93c39315
--- /dev/null
+++ b/drivers/soc/fsl/guts.c
@@ -0,0 +1,483 @@
+/*
+ * Freescale QorIQ Platforms GUTS Driver
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * 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/slab.h>
+#include <linux/module.h>
+#include <linux/of_fdt.h>
+#include <linux/sys_soc.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/glob.h>
+#include <linux/fsl/guts.h>
+#include <linux/fsl/svr.h>
+
+struct guts {
+	struct ccsr_guts __iomem *regs;
+	bool little_endian;
+};
+
+static struct guts *guts;
+static struct soc_device_attribute *soc_dev_attr;
+static struct soc_device *soc_dev;
+
+
+/* SoC attribute definition for QorIQ platform */
+static const struct soc_device_attribute qoriq_soc[] = {
+#ifdef CONFIG_PPC
+	/*
+	 * Power Architecture-based SoCs T Series
+	 */
+
+	/* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */
+	{ .soc_id	= "svr:0x85400010,name:T1024,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85480010,name:T1024E,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85440010,name:T1014,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x854C0010,name:T1014E,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85410010,name:T1023,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85490010,name:T1023E,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85450010,name:T1013,die:T1024",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x854D0010,name:T1013E,die:T1024",
+	  .revision	= "1.0",
+	},
+	/* SoC: T1040/T1020/T1042/T1022 Rev: 1.0/1.1 */
+	{ .soc_id	= "svr:0x85200010,name:T1040,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85280010,name:T1040E,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85210010,name:T1020,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85290010,name:T1020E,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85200210,name:T1042,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85280210,name:T1042E,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85210210,name:T1022,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85290210,name:T1022E,die:T1040",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85200011,name:T1040,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85280011,name:T1040E,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85210011,name:T1020,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85290011,name:T1020E,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85200211,name:T1042,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85280211,name:T1042E,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85210211,name:T1022,die:T1040",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85290211,name:T1022E,die:T1040",
+	  .revision	= "1.1",
+	},
+	/* SoC: T2080/T2081 Rev: 1.0/1.1 */
+	{ .soc_id	= "svr:0x85300010,name:T2080,die:T2080",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85380010,name:T2080E,die:T2080",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85310010,name:T2081,die:T2080",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85390010,name:T2081E,die:T2080",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x85300011,name:T2080,die:T2080",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85380011,name:T2080E,die:T2080",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85310011,name:T2081,die:T2080",
+	  .revision	= "1.1",
+	},
+	{ .soc_id	= "svr:0x85390011,name:T2081E,die:T2080",
+	  .revision	= "1.1",
+	},
+	/* SoC: T4240/T4160/T4080 Rev: 1.0/2.0 */
+	{ .soc_id	= "svr:0x82400010,name:T4240,die:T4240",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x82480010,name:T4240E,die:T4240",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x82410010,name:T4160,die:T4240",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x82490010,name:T4160E,die:T4240",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x82400020,name:T4240,die:T4240",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x82480020,name:T4240E,die:T4240",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x82410020,name:T4160,die:T4240",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x82490020,name:T4160E,die:T4240",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x82410220,name:T4080,die:T4240",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x82490220,name:T4080E,die:T4240",
+	  .revision	= "2.0",
+	},
+#endif /* CONFIG_PPC */
+#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_ARCH_LAYERSCAPE)
+	/*
+	 * ARM-based SoCs LS Series
+	 */
+
+	/* SoC: LS1021A/LS1020A/LS1022A Rev: 1.0/2.0 */
+	{ .soc_id	= "svr:0x87001110,name:LS1021A,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87081110,name:LS1021AE,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87001010,name:LS1020A,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87081010,name:LS1020AE,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87001210,name:LS1022A,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87081210,name:LS1022AE,die:LS1021A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87001120,name:LS1021A,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x87081120,name:LS1021AE,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x87001020,name:LS1020A,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x87081020,name:LS1020AE,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x87001220,name:LS1022A,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	{ .soc_id	= "svr:0x87081220,name:LS1022AE,die:LS1021A",
+	  .revision	= "2.0",
+	},
+	/* SoC: LS1046A/LS1026A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87070110,name:LS1046A,die:LS1046A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87070010,name:LS1046AE,die:LS1046A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87070910,name:LS1026A,die:LS1046A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87070810,name:LS1026AE,die:LS1046A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS1043A/LS1023A Rev: 1.0 Package: 21*21 */
+	{ .soc_id	= "svr:0x87920110,name:LS1043A,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920010,name:LS1043AE,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920910,name:LS1023A,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920810,name:LS1023AE,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS1043A/LS1023A Rev: 1.0 Package: 23*23 */
+	{ .soc_id	= "svr:0x87920310,name:LS1043A,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920210,name:LS1043AE,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920B10,name:LS1023A,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87920A10,name:LS1023AE,die:LS1043A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS1012A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87040110,name:LS1012A,die:LS1012A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87040010,name:LS1012AE,die:LS1012A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS2088A/LS2048A/LS2084A/LS2044A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87090110,name:LS2088A,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87090010,name:LS2088AE,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87092110,name:LS2048A,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87092010,name:LS2048AE,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87091110,name:LS2084A,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87091010,name:LS2084AE,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87093110,name:LS2044A,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87093010,name:LS2044AE,die:LS2088A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS2080A/LS2040A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87011010,name:LS2080AE,die:LS2080A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87013010,name:LS2040AE,die:LS2080A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS2085A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87010110,name:LS2085A,die:LS2085A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87010010,name:LS2085AE,die:LS2085A",
+	  .revision	= "1.0",
+	},
+	/* SoC: LS1088A/LS1048A/LS1084A/LS1044A Rev: 1.0 */
+	{ .soc_id	= "svr:0x87030110,name:LS1088A,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87030010,name:LS1088AE,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87032110,name:LS1048A,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87032010,name:LS1048AE,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87030310,name:LS1084A,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87030210,name:LS1084AE,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87032310,name:LS1044A,die:LS1088A",
+	  .revision	= "1.0",
+	},
+	{ .soc_id	= "svr:0x87032210,name:LS1044AE,die:LS1088A",
+	  .revision	= "1.0",
+	},
+#endif /* CONFIG_ARCH_MXC || CONFIG_ARCH_LAYERSCAPE */
+	{ },
+};
+
+static const struct soc_device_attribute *fsl_soc_device_match(
+	unsigned int svr, const struct soc_device_attribute *matches)
+{
+	char svr_match[50];
+	int n;
+
+	n = sprintf(svr_match, "*%08x*", svr);
+
+	do {
+		if (!matches->soc_id)
+			return NULL;
+		if (glob_match(svr_match, matches->soc_id))
+			break;
+	} while (matches++);
+
+	return matches;
+}
+
+unsigned int fsl_guts_get_svr(void)
+{
+	unsigned int svr = 0;
+
+	if (!guts || !guts->regs)
+		return svr;
+
+	if (guts->little_endian)
+		svr = ioread32(&guts->regs->svr);
+	else
+		svr = ioread32be(&guts->regs->svr);
+
+	return svr;
+}
+EXPORT_SYMBOL(fsl_guts_get_svr);
+
+static int fsl_guts_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	const struct soc_device_attribute *fsl_soc;
+	const char *machine;
+	unsigned int svr;
+	int ret = 0;
+
+	np = pdev->dev.of_node;
+
+	/* Initialize guts */
+	guts = kzalloc(sizeof(*guts), GFP_KERNEL);
+	if (!guts)
+		return -ENOMEM;
+
+	guts->little_endian = of_property_read_bool(np, "little-endian");
+
+	guts->regs = of_iomap(np, 0);
+	if (!guts->regs) {
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	/* Register soc device */
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr) {
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
+
+	machine = of_flat_dt_get_machine_name();
+	if (machine)
+		soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s", machine);
+
+	soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ");
+
+	svr = fsl_guts_get_svr();
+	fsl_soc = fsl_soc_device_match(svr, qoriq_soc);
+	if (fsl_soc) {
+		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
+						 fsl_soc->soc_id);
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s",
+						   fsl_soc->revision);
+	} else {
+		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x", svr);
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d.%d",
+						   SVR_MAJ(svr), SVR_MIN(svr));
+	}
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		ret = -ENODEV;
+		goto out;
+	} else {
+		pr_info("Detected: %s\n", soc_dev_attr->machine);
+		pr_info("Detected SoC family: %s\n", soc_dev_attr->family);
+		pr_info("Detected SoC ID: %s, revision: %s\n",
+			soc_dev_attr->soc_id, soc_dev_attr->revision);
+	}
+	return 0;
+out:
+	kfree(soc_dev_attr->machine);
+	kfree(soc_dev_attr->family);
+	kfree(soc_dev_attr->soc_id);
+	kfree(soc_dev_attr->revision);
+	kfree(soc_dev_attr);
+out_unmap:
+	iounmap(guts->regs);
+out_free:
+	kfree(guts);
+	return ret;
+}
+
+static int fsl_guts_remove(struct platform_device *dev)
+{
+	kfree(soc_dev_attr->machine);
+	kfree(soc_dev_attr->family);
+	kfree(soc_dev_attr->soc_id);
+	kfree(soc_dev_attr->revision);
+	kfree(soc_dev_attr);
+	soc_device_unregister(soc_dev);
+	iounmap(guts->regs);
+	kfree(guts);
+	return 0;
+}
+
+/*
+ * Table for matching compatible strings, for device tree
+ * guts node, for Freescale QorIQ SOCs.
+ */
+static const struct of_device_id fsl_guts_of_match[] = {
+	{ .compatible = "fsl,qoriq-device-config-1.0", },
+	{ .compatible = "fsl,qoriq-device-config-2.0", },
+	{ .compatible = "fsl,p1010-guts", },
+	{ .compatible = "fsl,p1020-guts", },
+	{ .compatible = "fsl,p1021-guts", },
+	{ .compatible = "fsl,p1022-guts", },
+	{ .compatible = "fsl,p1023-guts", },
+	{ .compatible = "fsl,p2020-guts", },
+	{ .compatible = "fsl,bsc9131-guts", },
+	{ .compatible = "fsl,bsc9132-guts", },
+	{ .compatible = "fsl,mpc8536-guts", },
+	{ .compatible = "fsl,mpc8544-guts", },
+	{ .compatible = "fsl,mpc8548-guts", },
+	{ .compatible = "fsl,mpc8568-guts", },
+	{ .compatible = "fsl,mpc8569-guts", },
+	{ .compatible = "fsl,mpc8572-guts", },
+	{ .compatible = "fsl,ls1021a-dcfg", },
+	{ .compatible = "fsl,ls1043a-dcfg", },
+	{ .compatible = "fsl,ls2080a-dcfg", },
+	{}
+};
+
+static struct platform_driver fsl_guts_driver = {
+	.driver = {
+		.name = "fsl-guts",
+		.of_match_table = fsl_guts_of_match,
+	},
+	.probe = fsl_guts_probe,
+	.remove = fsl_guts_remove,
+};
+
+module_platform_driver(fsl_guts_driver);
diff --git a/include/linux/fsl/guts.h b/include/linux/fsl/guts.h
index 649e917..c5e24cf 100644
--- a/include/linux/fsl/guts.h
+++ b/include/linux/fsl/guts.h
@@ -29,83 +29,114 @@
  * #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
+unsigned int fsl_guts_get_svr(void);
+#endif
 
 /* Alternate function signal multiplex control */
 #define MPC85xx_PMUXCR_QE(x) (0x8000 >> (x))
-- 
2.1.0.27.g96db324

Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Tue, 2016-09-06 at 16:28 +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 a driver to manage and access global utilities block.
> Initially only reading SVR and registering soc device are supported.
> Other guts accesses, such as reading RCW, should eventually be moved
> into this driver as well.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Scott Wood <oss@buserror.net>

Don't put my signoff on patches that I didn't put it on myself.  Definitely
don't put mine *after* yours on patches that were last modified by you.

If you want to mention that the soc_id encoding was my suggestion, then do so
explicitly.

> +/* SoC attribute definition for QorIQ platform */
> +static const struct soc_device_attribute qoriq_soc[] = {
> +#ifdef CONFIG_PPC
> +	/*
> +	 * Power Architecture-based SoCs T Series
> +	 */
> +
> +	/* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */
> +	{ .soc_id	= "svr:0x85400010,name:T1024,die:T1024",
> +	  .revision	= "1.0",
> +	},
> +	{ .soc_id	= "svr:0x85480010,name:T1024E,die:T1024",
> +	  .revision	= "1.0",
> +	},

Revision could be computed from the low 8 bits of SVR (just as you do for unknown SVRs).

We could move the die name into .family:

	{
		.soc_id = "svr:0x85490010,name:T1023E,",
		.family = "QorIQ T1024",
	}

I see you dropped svre (and the trailing comma), though I guess the vast
majority of potential users will be looking at .family.  In which case do we
even need name?  If we just make the soc_id be "svr:0xnnnnnnnn" then we could
shrink the table to an svr+mask that identifies each die.  I'd still want to
keep the "svr:" even if we're giving up on the general tagging system, to make
it clear what the number refers to, and to provide some defense against users
who match only against soc_id rather than soc_id+family.  Or we could go
further and format soc_id as "QorIQ SVR 0xnnnnnnnn" so that soc_id-only
matches are fully acceptable rather than just less dangerous.

> +static const struct soc_device_attribute *fsl_soc_device_match(
> +	unsigned int svr, const struct soc_device_attribute *matches)
> +{
> +	char svr_match[50];
> +	int n;
> +
> +	n = sprintf(svr_match, "*%08x*", svr);

n = sprintf(svr_match, "svr:0x%08x,*", svr);

(according to the current encoding)

> +
> +	do {
> +		if (!matches->soc_id)
> +			return NULL;
> +		if (glob_match(svr_match, matches->soc_id))
> +			break;
> +	} while (matches++);

Are you expecting "matches++" to ever evaluate as false?

> +	/* Register soc device */
> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}

Couldn't this be statically allocated?

> +
> +	machine = of_flat_dt_get_machine_name();
> +	if (machine)
> +		soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s",
> machine);
> +
> +	soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ");
> +
> +	svr = fsl_guts_get_svr();
> +	fsl_soc = fsl_soc_device_match(svr, qoriq_soc);
> +	if (fsl_soc) {
> +		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +						 fsl_soc->soc_id);

You can use kstrdup() if you're just copying the string as is.

> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s",
> +						   fsl_soc->revision);
> +	} else {
> +		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x",
> svr);

	kasprintf(GFP_KERNEL, "svr:0x%08x,", svr);


> +
> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev)) {
> +		ret = -ENODEV;

Why are you changing the error code?

> +		goto out;
> +	} else {

Unnecessary "else".

> +		pr_info("Detected: %s\n", soc_dev_attr->machine);

Machine: %s

> +		pr_info("Detected SoC family: %s\n", soc_dev_attr->family);
> +		pr_info("Detected SoC ID: %s, revision: %s\n",
> +			soc_dev_attr->soc_id, soc_dev_attr->revision);

s/Detected //g


> +	}
> +	return 0;
> +out:
> +	kfree(soc_dev_attr->machine);
> +	kfree(soc_dev_attr->family);
> +	kfree(soc_dev_attr->soc_id);
> +	kfree(soc_dev_attr->revision);
> +	kfree(soc_dev_attr);
> +out_unmap:
> +	iounmap(guts->regs);
> +out_free:
> +	kfree(guts);

devm

> +static int fsl_guts_remove(struct platform_device *dev)
> +{
> +	kfree(soc_dev_attr->machine);
> +	kfree(soc_dev_attr->family);
> +	kfree(soc_dev_attr->soc_id);
> +	kfree(soc_dev_attr->revision);
> +	kfree(soc_dev_attr);
> +	soc_device_unregister(soc_dev);
> +	iounmap(guts->regs);
> +	kfree(guts);
> +	return 0;
> +}

Don't free the memory before you unregister the device that uses it (moot if
you use devm).

>  
> +#ifdef CONFIG_FSL_GUTS
> +unsigned int fsl_guts_get_svr(void);
> +#endif

Don't ifdef prototypes (unless you're going to provide a stub alternative).

-Scott

RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Y.B. Lu]

Hi Scott,

Thanks for your review :)
See my comment inline.

> -----Original Message-----
> From: Scott Wood [mailto:oss@buserror.net]
> Sent: Friday, September 09, 2016 11:47 AM
> To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> Bergmann
> Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux-
> foundation.org; netdev@vger.kernel.org; Mark Rutland; Rob Herring;
> Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> 
> On Tue, 2016-09-06 at 16:28 +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 a driver to manage and access global utilities block.
> > Initially only reading SVR and registering soc device are supported.
> > Other guts accesses, such as reading RCW, should eventually be moved
> > into this driver as well.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > Signed-off-by: Scott Wood <oss@buserror.net>
> 
> Don't put my signoff on patches that I didn't put it on
> myself.  Definitely don't put mine *after* yours on patches that were
> last modified by you.
> 
> If you want to mention that the soc_id encoding was my suggestion, then
> do so explicitly.
> 

[Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
http://patchwork.ozlabs.org/patch/649211/

So, let me just change the order in next version ?
Signed-off-by: Scott Wood <oss@buserror.net>
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

> > +/* SoC attribute definition for QorIQ platform */ static const struct
> > +soc_device_attribute qoriq_soc[] = { #ifdef CONFIG_PPC
> > +	/*
> > +	 * Power Architecture-based SoCs T Series
> > +	 */
> > +
> > +	/* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */
> > +	{ .soc_id	= "svr:0x85400010,name:T1024,die:T1024",
> > +	  .revision	= "1.0",
> > +	},
> > +	{ .soc_id	= "svr:0x85480010,name:T1024E,die:T1024",
> > +	  .revision	= "1.0",
> > +	},
> 
> Revision could be computed from the low 8 bits of SVR (just as you do for
> unknown SVRs).
>
 
[Lu Yangbo-B47093] Yes, you're right. Will remove it here.

> We could move the die name into .family:
> 
> 	{
> 		.soc_id = "svr:0x85490010,name:T1023E,",
> 		.family = "QorIQ T1024",
> 	}
> 
> I see you dropped svre (and the trailing comma), though I guess the vast
> majority of potential users will be looking at .family.  In which case do
> we even need name?  If we just make the soc_id be "svr:0xnnnnnnnn" then
> we could shrink the table to an svr+mask that identifies each die.  I'd
> still want to keep the "svr:" even if we're giving up on the general
> tagging system, to make it clear what the number refers to, and to
> provide some defense against users who match only against soc_id rather
> than soc_id+family.  Or we could go further and format soc_id as "QorIQ
> SVR 0xnnnnnnnn" so that soc_id-only matches are fully acceptable rather
> than just less dangerous.

[Lu Yangbo-B47093] It's a good idea to move die into .family I think.
In my opinion, it's better to keep svr and name in soc_id just like your suggestion above.
> 	{
> 		.soc_id = "svr:0x85490010,name:T1023E,",
> 		.family = "QorIQ T1024",
> 	}
The user probably don’t like to learn the svr value. What they want is just to match the soc they use.
It's convenient to use name+rev for them to match a soc.

Regarding shrinking the table, I think it's hard to use svr+mask. Because I find many platforms use different masks.
We couldn’t know the mask according svr value.

> 
> > +static const struct soc_device_attribute *fsl_soc_device_match(
> > +	unsigned int svr, const struct soc_device_attribute *matches) {
> > +	char svr_match[50];
> > +	int n;
> > +
> > +	n = sprintf(svr_match, "*%08x*", svr);
> 
> n = sprintf(svr_match, "svr:0x%08x,*", svr);
> 
> (according to the current encoding)
> 

[Lu Yangbo-B47093] Ok. Will do that.

> > +
> > +	do {
> > +		if (!matches->soc_id)
> > +			return NULL;
> > +		if (glob_match(svr_match, matches->soc_id))
> > +			break;
> > +	} while (matches++);
> 
> Are you expecting "matches++" to ever evaluate as false?

[Lu Yangbo-B47093] Yes, this is used to match the soc we use in qoriq_soc array until getting true. 
We need to get the name and die information defined in array.

> 
> > +	/* Register soc device */
> > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > +	if (!soc_dev_attr) {
> > +		ret = -ENOMEM;
> > +		goto out_unmap;
> > +	}
> 
> Couldn't this be statically allocated?

[Lu Yangbo-B47093] Do you mean we define this struct statically ?

static struct soc_device_attribute soc_dev_attr;

> 
> > +
> > +	machine = of_flat_dt_get_machine_name();
> > +	if (machine)
> > +		soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s",
> > machine);
> > +
> > +	soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ");
> > +
> > +	svr = fsl_guts_get_svr();
> > +	fsl_soc = fsl_soc_device_match(svr, qoriq_soc);
> > +	if (fsl_soc) {
> > +		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> > +						 fsl_soc->soc_id);
> 
> You can use kstrdup() if you're just copying the string as is.

[Lu Yangbo-B47093] Ok. Will do that.

> 
> > +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s",
> > +						   fsl_soc->revision);
> > +	} else {
> > +		soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x",
> > svr);
> 
> 	kasprintf(GFP_KERNEL, "svr:0x%08x,", svr);

[Lu Yangbo-B47093] Sorry, will add that.

> 
> 
> > +
> > +	soc_dev = soc_device_register(soc_dev_attr);
> > +	if (IS_ERR(soc_dev)) {
> > +		ret = -ENODEV;
> 
> Why are you changing the error code?

[Lu Yangbo-B47093] What error code should we use ? :)

> 
> > +		goto out;
> > +	} else {
> 
> Unnecessary "else".

[Lu Yangbo-B47093] Oh.. Correct!

> 
> > +		pr_info("Detected: %s\n", soc_dev_attr->machine);
> 
> Machine: %s

[Lu Yangbo-B47093] Ok. Will do that.

> 
> > +		pr_info("Detected SoC family: %s\n", soc_dev_attr->family);
> > +		pr_info("Detected SoC ID: %s, revision: %s\n",
> > +			soc_dev_attr->soc_id, soc_dev_attr->revision);
> 
> s/Detected //g

[Lu Yangbo-B47093] Ok, will do that.

> 
> 
> > +	}
> > +	return 0;
> > +out:
> > +	kfree(soc_dev_attr->machine);
> > +	kfree(soc_dev_attr->family);
> > +	kfree(soc_dev_attr->soc_id);
> > +	kfree(soc_dev_attr->revision);
> > +	kfree(soc_dev_attr);
> > +out_unmap:
> > +	iounmap(guts->regs);
> > +out_free:
> > +	kfree(guts);
> 
> devm

[Lu Yangbo-B47093] What's the devm meaning here :)
 
> 
> > +static int fsl_guts_remove(struct platform_device *dev) {
> > +	kfree(soc_dev_attr->machine);
> > +	kfree(soc_dev_attr->family);
> > +	kfree(soc_dev_attr->soc_id);
> > +	kfree(soc_dev_attr->revision);
> > +	kfree(soc_dev_attr);
> > +	soc_device_unregister(soc_dev);
> > +	iounmap(guts->regs);
> > +	kfree(guts);
> > +	return 0;
> > +}
> 
> Don't free the memory before you unregister the device that uses it (moot
> if you use devm).

[Lu Yangbo-B47093] The soc.c driver mentions that.
Ensure soc_dev->attr is freed prior to calling soc_device_unregister.

> 
> >
> > +#ifdef CONFIG_FSL_GUTS
> > +unsigned int fsl_guts_get_svr(void);
> > +#endif
> 
> Don't ifdef prototypes (unless you're going to provide a stub
> alternative).

[Lu Yangbo-B47093] Ok, will remove ifdef.

> 
> -Scott

Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Mon, 2016-09-12 at 06:39 +0000, Y.B. Lu wrote:
> Hi Scott,
> 
> Thanks for your review :)
> See my comment inline.
> 
> > 
> > -----Original Message-----
> > From: Scott Wood [mailto:oss@buserror.net]
> > Sent: Friday, September 09, 2016 11:47 AM
> > To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> > Bergmann
> > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux-
> > foundation.org; netdev@vger.kernel.org; Mark Rutland; Rob Herring;
> > Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> > Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> > 
> > On Tue, 2016-09-06 at 16:28 +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 a driver to manage and access global utilities block.
> > > Initially only reading SVR and registering soc device are supported.
> > > Other guts accesses, such as reading RCW, should eventually be moved
> > > into this driver as well.
> > > 
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > Signed-off-by: Scott Wood <oss@buserror.net>
> > Don't put my signoff on patches that I didn't put it on
> > myself.  Definitely don't put mine *after* yours on patches that were
> > last modified by you.
> > 
> > If you want to mention that the soc_id encoding was my suggestion, then
> > do so explicitly.
> > 
> [Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
> http://patchwork.ozlabs.org/patch/649211/
> 
> So, let me just change the order in next version ?
> Signed-off-by: Scott Wood <oss@buserror.net>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

No.  This isn't my patch so my signoff shouldn't be on it.

> [Lu Yangbo-B47093] It's a good idea to move die into .family I think.
> In my opinion, it's better to keep svr and name in soc_id just like your
> suggestion above.
> > 
> > 	{
> > 		.soc_id = "svr:0x85490010,name:T1023E,",
> > 		.family = "QorIQ T1024",
> > 	}
> The user probably don’t like to learn the svr value. What they want is just
> to match the soc they use.
> It's convenient to use name+rev for them to match a soc.

What the user should want 99% of the time is to match the die (plus revision),
not the soc.

> Regarding shrinking the table, I think it's hard to use svr+mask. Because I
> find many platforms use different masks.
> We couldn’t know the mask according svr value.

The mask would be part of the table:

{
	{
		.die = "T1024",
		.svr = 0x85400000,
		.mask = 0xfff00000,
	},
	{
		.die = "T1040",
		.svr = 0x85200000,
		.mask = 0xfff00000,
	},
	{
		.die = "LS1088A",
		.svr = 0x87030000,
		.mask = 0xffff0000,
	},
	...
}

There's a small risk that we get the mask wrong and a different die is created
that matches an existing table, but it doesn't seem too likely, and can easily
be fixed with a kernel update if it happens.

BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E version
of LS2080A/LS2040A?

> > > +	do {
> > > +		if (!matches->soc_id)
> > > +			return NULL;
> > > +		if (glob_match(svr_match, matches->soc_id))
> > > +			break;
> > > +	} while (matches++);
> > Are you expecting "matches++" to ever evaluate as false?
> [Lu Yangbo-B47093] Yes, this is used to match the soc we use in qoriq_soc
> array until getting true. 
> We need to get the name and die information defined in array.

I'm not asking whether the glob_match will ever return true.  I'm saying that
"matches++" will never become NULL.

> > > +	/* Register soc device */
> > > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > +	if (!soc_dev_attr) {
> > > +		ret = -ENOMEM;
> > > +		goto out_unmap;
> > > +	}
> > Couldn't this be statically allocated?
> [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> 
> static struct soc_device_attribute soc_dev_attr;

Yes.

> > > +
> > > +	soc_dev = soc_device_register(soc_dev_attr);
> > > +	if (IS_ERR(soc_dev)) {
> > > +		ret = -ENODEV;
> > Why are you changing the error code?
> [Lu Yangbo-B47093] What error code should we use ? :)

ret = PTR_ERR(soc_dev);

+	}
> > > +	return 0;
> > > +out:
> > > +	kfree(soc_dev_attr->machine);
> > > +	kfree(soc_dev_attr->family);
> > > +	kfree(soc_dev_attr->soc_id);
> > > +	kfree(soc_dev_attr->revision);
> > > +	kfree(soc_dev_attr);
> > > +out_unmap:
> > > +	iounmap(guts->regs);
> > > +out_free:
> > > +	kfree(guts);
> > devm
> [Lu Yangbo-B47093] What's the devm meaning here :)

If you allocate these with devm_kzalloc(), devm_kasprintf(), devm_kstrdup(),
etc. then they will be freed automatically when the device is unbound.

>  
> > 
> > 
> > > 
> > > +static int fsl_guts_remove(struct platform_device *dev) {
> > > +	kfree(soc_dev_attr->machine);
> > > +	kfree(soc_dev_attr->family);
> > > +	kfree(soc_dev_attr->soc_id);
> > > +	kfree(soc_dev_attr->revision);
> > > +	kfree(soc_dev_attr);
> > > +	soc_device_unregister(soc_dev);
> > > +	iounmap(guts->regs);
> > > +	kfree(guts);
> > > +	return 0;
> > > +}
> > Don't free the memory before you unregister the device that uses it (moot
> > if you use devm).
> [Lu Yangbo-B47093] The soc.c driver mentions that.
> Ensure soc_dev->attr is freed prior to calling soc_device_unregister.

That comment is wrong.  Freeing the memory first creates a race condition that
could result in accessing freed memory, if something accesses the soc device
in parallel with unbinding.

-Scott

RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Y.B. Lu]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Scott Wood
> Sent: Tuesday, September 13, 2016 7:25 AM
> To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> Bergmann
> Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux-
> foundation.org; netdev@vger.kernel.org; Mark Rutland; Rob Herring;
> Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> 
> On Mon, 2016-09-12 at 06:39 +0000, Y.B. Lu wrote:
> > Hi Scott,
> >
> > Thanks for your review :)
> > See my comment inline.
> >
> > >
> > > -----Original Message-----
> > > From: Scott Wood [mailto:oss@buserror.net]
> > > Sent: Friday, September 09, 2016 11:47 AM
> > > To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> > > Bergmann
> > > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org;
> > > linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > linux- clk@vger.kernel.org; linux-i2c@vger.kernel.org;
> > > iommu@lists.linux- foundation.org; netdev@vger.kernel.org; Mark
> > > Rutland; Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel;
> > > Claudiu Manoil; Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh
> > > Shilimkar; Leo Li; X.B. Xie
> > > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ
> > > platforms
> > >
> > > On Tue, 2016-09-06 at 16:28 +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 a driver to manage and access global utilities
> block.
> > > > Initially only reading SVR and registering soc device are supported.
> > > > Other guts accesses, such as reading RCW, should eventually be
> > > > moved into this driver as well.
> > > >
> > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > Signed-off-by: Scott Wood <oss@buserror.net>
> > > Don't put my signoff on patches that I didn't put it on myself.
> > > Definitely don't put mine *after* yours on patches that were last
> > > modified by you.
> > >
> > > If you want to mention that the soc_id encoding was my suggestion,
> > > then do so explicitly.
> > >
> > [Lu Yangbo-B47093] I found your 'signoff' on this patch at below link.
> > http://patchwork.ozlabs.org/patch/649211/
> >
> > So, let me just change the order in next version ?
> > Signed-off-by: Scott Wood <oss@buserror.net>
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> 
> No.  This isn't my patch so my signoff shouldn't be on it.

[Lu Yangbo-B47093] Ok, will remove it.

> 
> > [Lu Yangbo-B47093] It's a good idea to move die into .family I think.
> > In my opinion, it's better to keep svr and name in soc_id just like
> > your suggestion above.
> > >
> > > 	{
> > > 		.soc_id = "svr:0x85490010,name:T1023E,",
> > > 		.family = "QorIQ T1024",
> > > 	}
> > The user probably don’t like to learn the svr value. What they want is
> > just to match the soc they use.
> > It's convenient to use name+rev for them to match a soc.
> 
> What the user should want 99% of the time is to match the die (plus
> revision), not the soc.
> 
> > Regarding shrinking the table, I think it's hard to use svr+mask.
> > Because I find many platforms use different masks.
> > We couldn’t know the mask according svr value.
> 
> The mask would be part of the table:
> 
> {
> 	{
> 		.die = "T1024",
> 		.svr = 0x85400000,
> 		.mask = 0xfff00000,
> 	},
> 	{
> 		.die = "T1040",
> 		.svr = 0x85200000,
> 		.mask = 0xfff00000,
> 	},
> 	{
> 		.die = "LS1088A",
> 		.svr = 0x87030000,
> 		.mask = 0xffff0000,
> 	},
> 	...
> }
> 
> There's a small risk that we get the mask wrong and a different die is
> created that matches an existing table, but it doesn't seem too likely,
> and can easily be fixed with a kernel update if it happens.
> 

[Lu Yangbo-B47093] You mean we will not define soc device attribute for each soc and we will define attribute for each die instead, right?
If so, when we want to match a specific soc we need to use its svr value in code. If it's acceptable, I can try in next version.

> BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E
> version of LS2080A/LS2040A?

[Lu Yangbo-B47093] I checked all the svr values in chip errata doc "Revision level to part marking cross-reference" table.
I found ls2080a and ls2085a were in two separate doc. And I didn’t find non-E version of LS2080A/LS2040A in chip errata doc.
Do you know is there any other doc we can confirm this?

> 
> > > > +	do {
> > > > +		if (!matches->soc_id)
> > > > +			return NULL;
> > > > +		if (glob_match(svr_match, matches->soc_id))
> > > > +			break;
> > > > +	} while (matches++);
> > > Are you expecting "matches++" to ever evaluate as false?
> > [Lu Yangbo-B47093] Yes, this is used to match the soc we use in
> > qoriq_soc array until getting true.
> > We need to get the name and die information defined in array.
> 
> I'm not asking whether the glob_match will ever return true.  I'm saying
> that "matches++" will never become NULL.

[Lu Yangbo-B47093] The matches++ will never become NULL while it will return NULL after matching for all the members in array.

> 
> > > > +	/* Register soc device */
> > > > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > > +	if (!soc_dev_attr) {
> > > > +		ret = -ENOMEM;
> > > > +		goto out_unmap;
> > > > +	}
> > > Couldn't this be statically allocated?
> > [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> >
> > static struct soc_device_attribute soc_dev_attr;
> 
> Yes.
> 

[Lu Yangbo-B47093] It's ok to define it statically. Is there any need to do that?

> > > > +
> > > > +	soc_dev = soc_device_register(soc_dev_attr);
> > > > +	if (IS_ERR(soc_dev)) {
> > > > +		ret = -ENODEV;
> > > Why are you changing the error code?
> > [Lu Yangbo-B47093] What error code should we use ? :)
> 
> ret = PTR_ERR(soc_dev);

[Lu Yangbo-B47093] Ok.. will do that.

> 
> +	}
> > > > +	return 0;
> > > > +out:
> > > > +	kfree(soc_dev_attr->machine);
> > > > +	kfree(soc_dev_attr->family);
> > > > +	kfree(soc_dev_attr->soc_id);
> > > > +	kfree(soc_dev_attr->revision);
> > > > +	kfree(soc_dev_attr);
> > > > +out_unmap:
> > > > +	iounmap(guts->regs);
> > > > +out_free:
> > > > +	kfree(guts);
> > > devm
> > [Lu Yangbo-B47093] What's the devm meaning here :)
> 
> If you allocate these with devm_kzalloc(), devm_kasprintf(),
> devm_kstrdup(), etc. then they will be freed automatically when the
> device is unbound.
> 
> >
> > >
> > >
> > > >
> > > > +static int fsl_guts_remove(struct platform_device *dev) {
> > > > +	kfree(soc_dev_attr->machine);
> > > > +	kfree(soc_dev_attr->family);
> > > > +	kfree(soc_dev_attr->soc_id);
> > > > +	kfree(soc_dev_attr->revision);
> > > > +	kfree(soc_dev_attr);
> > > > +	soc_device_unregister(soc_dev);
> > > > +	iounmap(guts->regs);
> > > > +	kfree(guts);
> > > > +	return 0;
> > > > +}
> > > Don't free the memory before you unregister the device that uses it
> > > (moot if you use devm).
> > [Lu Yangbo-B47093] The soc.c driver mentions that.
> > Ensure soc_dev->attr is freed prior to calling soc_device_unregister.
> 
> That comment is wrong.  Freeing the memory first creates a race condition
> that could result in accessing freed memory, if something accesses the
> soc device in parallel with unbinding.
> 

[Lu Yangbo-B47093] Ok, will unregister the device first. Thanks.

> -Scott
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms

[Scott Wood]

On Tue, 2016-09-13 at 07:23 +0000, Y.B. Lu wrote:
> > 


> > 
> > -----Original Message-----
> > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> > owner@vger.kernel.org] On Behalf Of Scott Wood
> > Sent: Tuesday, September 13, 2016 7:25 AM
> > To: Y.B. Lu; linux-mmc@vger.kernel.org; ulf.hansson@linaro.org; Arnd
> > Bergmann
> > Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > clk@vger.kernel.org; linux-i2c@vger.kernel.org; iommu@lists.linux-
> > foundation.org; netdev@vger.kernel.org; Mark Rutland; Rob Herring;
> > Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil; Bhupesh
> > Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B. Xie
> > Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms
> > 
> > BTW, aren't ls2080a and ls2085a the same die?  And is there no non-E
> > version of LS2080A/LS2040A?
> [Lu Yangbo-B47093] I checked all the svr values in chip errata doc "Revision
> level to part marking cross-reference" table.
> I found ls2080a and ls2085a were in two separate doc. And I didn’t find non-
> E version of LS2080A/LS2040A in chip errata doc.
> Do you know is there any other doc we can confirm this?

No.  Traditionally we've always had E and non-E versions of each chip, but I
have no knowledge of whether that has changed (I do note that the way that E-
status is indicated in SVR has changed).

But please label LS2080A and LS2085A as the same die (or provide strong
evidence that they are not).

> 
> > 
> > 
> > > 
> > > > > 
> > > > > +	do {
> > > > > +		if (!matches->soc_id)
> > > > > +			return NULL;
> > > > > +		if (glob_match(svr_match, matches->soc_id))
> > > > > +			break;
> > > > > +	} while (matches++);
> > > > Are you expecting "matches++" to ever evaluate as false?
> > > [Lu Yangbo-B47093] Yes, this is used to match the soc we use in
> > > qoriq_soc array until getting true.
> > > We need to get the name and die information defined in array.
> > I'm not asking whether the glob_match will ever return true.  I'm saying
> > that "matches++" will never become NULL.
> [Lu Yangbo-B47093] The matches++ will never become NULL while it will return
> NULL after matching for all the members in array.

"matches++" will never "return NULL".  It's just an incrementing address.  It
won't be null until you wrap around the address space, and even if the other
loop terminators never kicked in you'd crash long before that happens.

Please rewrite the loop as something like:

	while (matches->soc_id) {
		if (glob_match(...))
			return matches;

		matches++;
	}

	return NULL;


> > > > > +	/* Register soc device */
> > > > > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > > > +	if (!soc_dev_attr) {
> > > > > +		ret = -ENOMEM;
> > > > > +		goto out_unmap;
> > > > > +	}
> > > > Couldn't this be statically allocated?
> > > [Lu Yangbo-B47093] Do you mean we define this struct statically ?
> > > 
> > > static struct soc_device_attribute soc_dev_attr;
> > Yes.
> > 
> [Lu Yangbo-B47093] It's ok to define it statically. Is there any need to do
> that?

It's simpler.

-Scott

[v11, 6/8] MAINTAINERS: add entry for Freescale SoC drivers

[Yangbo Lu]

Add maintainer entry for Freescale SoC drivers including
the QE library and the GUTS driver now. Also add maintainer
for QE library.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Scott Wood <oss@buserror.net>
Acked-by: Qiang Zhao <qiang.zhao@nxp.com>
---
Changes for v8:
	- Added this patch
Changes for v9:
	- Added linux-arm mail list
	- Removed GUTS driver entry
Changes for v10:
	- Changed 'DRIVER' to 'DRIVERS'
	- Added 'Acked-by' of Scott and Qiang
Changes for v11:
	- None
---
 MAINTAINERS | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 71aa5da..3954c0c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4969,9 +4969,18 @@ F:	drivers/net/ethernet/freescale/fec_ptp.c
 F:	drivers/net/ethernet/freescale/fec.h
 F:	Documentation/devicetree/bindings/net/fsl-fec.txt
 
+FREESCALE SOC DRIVERS
+M:	Scott Wood <oss@buserror.net>
+L:	linuxppc-dev@lists.ozlabs.org
+L:	linux-arm-kernel@lists.infradead.org
+S:	Maintained
+F:	drivers/soc/fsl/
+F:	include/linux/fsl/
+
 FREESCALE QUICC ENGINE LIBRARY
+M:	Qiang Zhao <qiang.zhao@nxp.com>
 L:	linuxppc-dev@lists.ozlabs.org
-S:	Orphan
+S:	Maintained
 F:	drivers/soc/fsl/qe/
 F:	include/soc/fsl/*qe*.h
 F:	include/soc/fsl/*ucc*.h
-- 
2.1.0.27.g96db324

[v11, 7/8] base: soc: introduce soc_device_match() interface

[Yangbo Lu]

We keep running into cases where device drivers want to know the exact
version of the a SoC they are currently running on. In the past, this has
usually been done through a vendor specific API that can be called by a
driver, or by directly accessing some kind of version register that is
not part of the device itself but that belongs to a global register area
of the chip.

Common reasons for doing this include:

- A machine is not using devicetree or similar for passing data about
  on-chip devices, but just announces their presence using boot-time
  platform devices, and the machine code itself does not care about the
  revision.

- There is existing firmware or boot loaders with existing DT binaries
  with generic compatible strings that do not identify the particular
  revision of each device, but the driver knows which SoC revisions
  include which part.

- A prerelease version of a chip has some quirks and we are using the same
  version of the bootloader and the DT blob on both the prerelease and the
  final version. An update of the DT binding seems inappropriate because
  that would involve maintaining multiple copies of the dts and/or
  bootloader.

This patch introduces the soc_device_match() interface that is meant to
work like of_match_node() but instead of identifying the version of a
device, it identifies the SoC itself using a vendor-agnostic interface.

Unlike of_match_node(), we do not do an exact string compare but instead
use glob_match() to allow wildcards in strings.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v11:
	- Added this patch for soc match
---
 drivers/base/Kconfig    |  1 +
 drivers/base/soc.c      | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |  3 +++
 3 files changed, 65 insertions(+)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 98504ec..f1591ad2 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
 
 config SOC_BUS
 	bool
+	select GLOB
 
 source "drivers/base/regmap/Kconfig"
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 75b98aa..5c4e84a 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -14,6 +14,7 @@
 #include <linux/spinlock.h>
 #include <linux/sys_soc.h>
 #include <linux/err.h>
+#include <linux/glob.h>
 
 static DEFINE_IDA(soc_ida);
 
@@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void)
 	bus_unregister(&soc_bus_type);
 }
 module_exit(soc_bus_unregister);
+
+static int soc_device_match_one(struct device *dev, void *arg)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+	const struct soc_device_attribute *match = arg;
+
+	if (match->machine &&
+	    !glob_match(match->machine, soc_dev->attr->machine))
+		return 0;
+
+	if (match->family &&
+	    !glob_match(match->family, soc_dev->attr->family))
+		return 0;
+
+	if (match->revision &&
+	    !glob_match(match->revision, soc_dev->attr->revision))
+		return 0;
+
+	if (match->soc_id &&
+	    !glob_match(match->soc_id, soc_dev->attr->soc_id))
+		return 0;
+
+	return 1;
+}
+
+/*
+ * soc_device_match - identify the SoC in the machine
+ * @matches: zero-terminated array of possible matches
+ *
+ * returns the first matching entry of the argument array, or NULL
+ * if none of them match.
+ *
+ * This function is meant as a helper in place of of_match_node()
+ * in cases where either no device tree is available or the information
+ * in a device node is insufficient to identify a particular variant
+ * by its compatible strings or other properties. For new devices,
+ * the DT binding should always provide unique compatible strings
+ * that allow the use of of_match_node() instead.
+ *
+ * The calling function can use the .data entry of the
+ * soc_device_attribute to pass a structure or function pointer for
+ * each entry.
+ */
+const struct soc_device_attribute *soc_device_match(
+	const struct soc_device_attribute *matches)
+{
+	struct device *dev;
+	int ret;
+
+	for (ret = 0; ret == 0; matches++) {
+		if (!(matches->machine || matches->family ||
+		      matches->revision || matches->soc_id))
+			return NULL;
+		dev = NULL;
+		ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
+				       soc_device_match_one);
+	}
+	return matches;
+}
+EXPORT_SYMBOL_GPL(soc_device_match);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index 2739ccb..9f5eb06 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -13,6 +13,7 @@ struct soc_device_attribute {
 	const char *family;
 	const char *revision;
 	const char *soc_id;
+	const void *data;
 };
 
 /**
@@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
  */
 struct device *soc_device_to_device(struct soc_device *soc);
 
+const struct soc_device_attribute *soc_device_match(
+	const struct soc_device_attribute *matches);
 #endif /* __SOC_BUS_H */
-- 
2.1.0.27.g96db324

Re: [v11, 7/8] base: soc: introduce soc_device_match() interface

[Ulf Hansson]

On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> We keep running into cases where device drivers want to know the exact
> version of the a SoC they are currently running on. In the past, this has
> usually been done through a vendor specific API that can be called by a
> driver, or by directly accessing some kind of version register that is
> not part of the device itself but that belongs to a global register area
> of the chip.
>
> Common reasons for doing this include:
>
> - A machine is not using devicetree or similar for passing data about
>   on-chip devices, but just announces their presence using boot-time
>   platform devices, and the machine code itself does not care about the
>   revision.
>
> - There is existing firmware or boot loaders with existing DT binaries
>   with generic compatible strings that do not identify the particular
>   revision of each device, but the driver knows which SoC revisions
>   include which part.
>
> - A prerelease version of a chip has some quirks and we are using the same
>   version of the bootloader and the DT blob on both the prerelease and the
>   final version. An update of the DT binding seems inappropriate because
>   that would involve maintaining multiple copies of the dts and/or
>   bootloader.
>
> This patch introduces the soc_device_match() interface that is meant to
> work like of_match_node() but instead of identifying the version of a
> device, it identifies the SoC itself using a vendor-agnostic interface.
>
> Unlike of_match_node(), we do not do an exact string compare but instead
> use glob_match() to allow wildcards in strings.

Overall, this change make sense to me, although some minor comment below.

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v11:
>         - Added this patch for soc match
> ---
>  drivers/base/Kconfig    |  1 +
>  drivers/base/soc.c      | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |  3 +++
>  3 files changed, 65 insertions(+)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec..f1591ad2 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
>
>  config SOC_BUS
>         bool
> +       select GLOB
>
>  source "drivers/base/regmap/Kconfig"
>
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 75b98aa..5c4e84a 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -14,6 +14,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/sys_soc.h>
>  #include <linux/err.h>
> +#include <linux/glob.h>
>
>  static DEFINE_IDA(soc_ida);
>
> @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void)
>         bus_unregister(&soc_bus_type);
>  }
>  module_exit(soc_bus_unregister);
> +
> +static int soc_device_match_one(struct device *dev, void *arg)
> +{
> +       struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +       const struct soc_device_attribute *match = arg;
> +
> +       if (match->machine &&
> +           !glob_match(match->machine, soc_dev->attr->machine))
> +               return 0;
> +
> +       if (match->family &&
> +           !glob_match(match->family, soc_dev->attr->family))
> +               return 0;
> +
> +       if (match->revision &&
> +           !glob_match(match->revision, soc_dev->attr->revision))
> +               return 0;
> +
> +       if (match->soc_id &&
> +           !glob_match(match->soc_id, soc_dev->attr->soc_id))
> +               return 0;
> +
> +       return 1;
> +}
> +
> +/*
> + * soc_device_match - identify the SoC in the machine
> + * @matches: zero-terminated array of possible matches

Perhaps also express the constraint on the matching entries. As you
need at least one of the ->machine(), ->family(), ->revision() or
->soc_id() callbacks implemented, right!?

> + *
> + * returns the first matching entry of the argument array, or NULL
> + * if none of them match.
> + *
> + * This function is meant as a helper in place of of_match_node()
> + * in cases where either no device tree is available or the information
> + * in a device node is insufficient to identify a particular variant
> + * by its compatible strings or other properties. For new devices,
> + * the DT binding should always provide unique compatible strings
> + * that allow the use of of_match_node() instead.
> + *
> + * The calling function can use the .data entry of the
> + * soc_device_attribute to pass a structure or function pointer for
> + * each entry.

I don't get the use case behind this, could you elaborate?

Perhaps we should postpone adding the .data entry until we actually
see a need for it?

> + */
> +const struct soc_device_attribute *soc_device_match(
> +       const struct soc_device_attribute *matches)
> +{
> +       struct device *dev;
> +       int ret;
> +
> +       for (ret = 0; ret == 0; matches++) {

This loop looks a bit weird and unsafe.

1) Perhaps using a while loop makes this more readable?
2) As this is an exported API, I guess validation of the ->matches
pointer needs to be done before accessing it.

> +               if (!(matches->machine || matches->family ||
> +                     matches->revision || matches->soc_id))
> +                       return NULL;
> +               dev = NULL;

There's no need to use a struct device just to assign it to NULL.
Instead just provide the function below with NULL.

> +               ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> +                                      soc_device_match_one);
> +       }
> +       return matches;
> +}
> +EXPORT_SYMBOL_GPL(soc_device_match);
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> index 2739ccb..9f5eb06 100644
> --- a/include/linux/sys_soc.h
> +++ b/include/linux/sys_soc.h
> @@ -13,6 +13,7 @@ struct soc_device_attribute {
>         const char *family;
>         const char *revision;
>         const char *soc_id;
> +       const void *data;
>  };
>
>  /**
> @@ -34,4 +35,6 @@ void soc_device_unregister(struct soc_device *soc_dev);
>   */
>  struct device *soc_device_to_device(struct soc_device *soc);
>
> +const struct soc_device_attribute *soc_device_match(
> +       const struct soc_device_attribute *matches);
>  #endif /* __SOC_BUS_H */
> --
> 2.1.0.27.g96db324
>

Kind regards
Uffe

Re: [v11, 7/8] base: soc: introduce soc_device_match() interface

[Arnd Bergmann]

On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > We keep running into cases where device drivers want to know the exact
> > version of the a SoC they are currently running on. In the past, this has
> > usually been done through a vendor specific API that can be called by a
> > driver, or by directly accessing some kind of version register that is
> > not part of the device itself but that belongs to a global register area
> > of the chip.

Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to
preserve authorship. If you use "git send-email" or "git format-patch",
that should happen automatically if the author field is set right
(if not, use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"'
to fix it).

> > +
> > +/*
> > + * soc_device_match - identify the SoC in the machine
> > + * @matches: zero-terminated array of possible matches
> 
> Perhaps also express the constraint on the matching entries. As you
> need at least one of the ->machine(), ->family(), ->revision() or
> ->soc_id() callbacks implemented, right!?

They are not callbacks, just strings. Having an empty entry indicates
the end of the array, and this is not called.

> > + *
> > + * returns the first matching entry of the argument array, or NULL
> > + * if none of them match.
> > + *
> > + * This function is meant as a helper in place of of_match_node()
> > + * in cases where either no device tree is available or the information
> > + * in a device node is insufficient to identify a particular variant
> > + * by its compatible strings or other properties. For new devices,
> > + * the DT binding should always provide unique compatible strings
> > + * that allow the use of of_match_node() instead.
> > + *
> > + * The calling function can use the .data entry of the
> > + * soc_device_attribute to pass a structure or function pointer for
> > + * each entry.
> 
> I don't get the use case behind this, could you elaborate?
> 
> Perhaps we should postpone adding the .data entry until we actually
> see a need for it?

I think the interface is rather useless without a way to figure
out which entry you got. Almost all users of of_match_node()
actually use the returned ->data field, and I expect this to
be the same here.

> > + */
> > +const struct soc_device_attribute *soc_device_match(
> > +       const struct soc_device_attribute *matches)
> > +{
> > +       struct device *dev;
> > +       int ret;
> > +
> > +       for (ret = 0; ret == 0; matches++) {
> 
> This loop looks a bit weird and unsafe.

Ah, and I thought I was being clever ;-)

> 1) Perhaps using a while loop makes this more readable?
> 2) As this is an exported API, I guess validation of the ->matches
> pointer needs to be done before accessing it.

Sounds fine.

> > +               if (!(matches->machine || matches->family ||
> > +                     matches->revision || matches->soc_id))
> > +                       return NULL;
> > +               dev = NULL;
> 
> There's no need to use a struct device just to assign it to NULL.
> Instead just provide the function below with NULL.
>
> > +               ret = bus_for_each_dev(&soc_bus_type, dev, (void *)matches,
> > +                                      soc_device_match_one);


I don't remember what led to this, I think you are right, we should
just pass NULL as most other callers.

Thanks for the review.

	ARnd

RE: [v11, 7/8] base: soc: introduce soc_device_match() interface

[Y.B. Lu]

Hi Anrd and Uffe,

Thank you for your comment.
Please see my comment inline.



Best regards,
Yangbo Lu

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, September 06, 2016 8:46 PM
> To: Ulf Hansson
> Cc: Y.B. Lu; linux-mmc; Scott Wood; linuxppc-dev@lists.ozlabs.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-clk; linux-i2c@vger.kernel.org;
> iommu@lists.linux-foundation.org; netdev@vger.kernel.org; Mark Rutland;
> Rob Herring; Russell King; Jochen Friedrich; Joerg Roedel; Claudiu Manoil;
> Bhupesh Sharma; Qiang Zhao; Kumar Gala; Santosh Shilimkar; Leo Li; X.B.
> Xie
> Subject: Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
> 
> On Tuesday, September 6, 2016 1:44:23 PM CEST Ulf Hansson wrote:
> > On 6 September 2016 at 10:28, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > We keep running into cases where device drivers want to know the
> > > exact version of the a SoC they are currently running on. In the
> > > past, this has usually been done through a vendor specific API that
> > > can be called by a driver, or by directly accessing some kind of
> > > version register that is not part of the device itself but that
> > > belongs to a global register area of the chip.
> 
> Please add "From: Arnd Bergmann <arnd@arndb.de>" as the first line, to
> preserve authorship. If you use "git send-email" or "git format-patch",
> that should happen automatically if the author field is set right (if not,
> use 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"'
> to fix it).
> 

[Lu Yangbo-B47093] Oh, I'm sorry for my careless. Will correct it in next version.

> > > +
> > > +/*
> > > + * soc_device_match - identify the SoC in the machine
> > > + * @matches: zero-terminated array of possible matches
> >
> > Perhaps also express the constraint on the matching entries. As you
> > need at least one of the ->machine(), ->family(), ->revision() or
> > ->soc_id() callbacks implemented, right!?
> 
> They are not callbacks, just strings. Having an empty entry indicates the
> end of the array, and this is not called.
> 
> > > + *
> > > + * returns the first matching entry of the argument array, or NULL
> > > + * if none of them match.
> > > + *
> > > + * This function is meant as a helper in place of of_match_node()
> > > + * in cases where either no device tree is available or the
> > > + information
> > > + * in a device node is insufficient to identify a particular
> > > + variant
> > > + * by its compatible strings or other properties. For new devices,
> > > + * the DT binding should always provide unique compatible strings
> > > + * that allow the use of of_match_node() instead.
> > > + *
> > > + * The calling function can use the .data entry of the
> > > + * soc_device_attribute to pass a structure or function pointer for
> > > + * each entry.
> >
> > I don't get the use case behind this, could you elaborate?
> >
> > Perhaps we should postpone adding the .data entry until we actually
> > see a need for it?
> 
> I think the interface is rather useless without a way to figure out which
> entry you got. Almost all users of of_match_node() actually use the
> returned ->data field, and I expect this to be the same here.
> 
> > > + */
> > > +const struct soc_device_attribute *soc_device_match(
> > > +       const struct soc_device_attribute *matches) {
> > > +       struct device *dev;
> > > +       int ret;
> > > +
> > > +       for (ret = 0; ret == 0; matches++) {
> >
> > This loop looks a bit weird and unsafe.
> 
> Ah, and I thought I was being clever ;-)
> 
> > 1) Perhaps using a while loop makes this more readable?
> > 2) As this is an exported API, I guess validation of the ->matches
> > pointer needs to be done before accessing it.
> 
> Sounds fine.

[Lu Yangbo-B47093] Ok, Will change this according to Uffe. 
And actually there is issue with this for() when I verified it again this morning.
We will get matches++ rather than matches which is correct finally :)

> 
> > > +               if (!(matches->machine || matches->family ||
> > > +                     matches->revision || matches->soc_id))
> > > +                       return NULL;
> > > +               dev = NULL;
> >
> > There's no need to use a struct device just to assign it to NULL.
> > Instead just provide the function below with NULL.
> >
> > > +               ret = bus_for_each_dev(&soc_bus_type, dev, (void
> *)matches,
> > > +                                      soc_device_match_one);
> 
> 
> I don't remember what led to this, I think you are right, we should just
> pass NULL as most other callers.

[Lu Yangbo-B47093] Will correct it. Thanks. :)

> 
> Thanks for the review.
> 
> 	ARnd

[v11, 8/8] 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.
This patch adds the GUTS driver support for eSDHC driver to match SoC.
And fix host version to avoid that incorrect version numbers break down
the ADMA data transfer.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Scott Wood <oss@buserror.net>
---
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
Changes for v5:
	- Changed to get SVR through API fsl_guts_get_svr()
	- Combined patch 4, patch 5 and patch 6 into one
Changes for v6:
	- Added 'Acked-by: Ulf Hansson'
Changes for v7:
	- None
Changes for v8:
	- Added 'Acked-by: Scott Wood'
Changes for v9:
	- None
Changes for v10:
	- None
Changes for v11:
	- Changed to use soc_device_match
---
 drivers/mmc/host/Kconfig          |  1 +
 drivers/mmc/host/sdhci-of-esdhc.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5274f50..a1135a9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -144,6 +144,7 @@ config MMC_SDHCI_OF_ESDHC
 	depends on MMC_SDHCI_PLTFM
 	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
 	select MMC_SDHCI_IO_ACCESSORS
+	select FSL_GUTS
 	help
 	  This selects the Freescale eSDHC controller support.
 
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index fb71c86..0a31aa5 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/sys_soc.h>
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
@@ -28,6 +29,7 @@
 struct sdhci_esdhc {
 	u8 vendor_ver;
 	u8 spec_ver;
+	bool quirk_incorrect_hostver;
 };
 
 /**
@@ -73,6 +75,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 = sdhci_pltfm_priv(pltfm_host);
 	u16 ret;
 	int shift = (spec_reg & 0x2) * 8;
 
@@ -80,6 +84,12 @@ 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->quirk_incorrect_hostver))
+		ret = (VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200;
 	return ret;
 }
 
@@ -558,6 +568,12 @@ static const struct sdhci_pltfm_data sdhci_esdhc_le_pdata = {
 	.ops = &sdhci_esdhc_le_ops,
 };
 
+static struct soc_device_attribute soc_incorrect_hostver[] = {
+	{ .soc_id = "*name:T4240*", .revision = "1.0", },
+	{ .soc_id = "*name:T4240*", .revision = "2.0", },
+	{ },
+};
+
 static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host;
@@ -571,6 +587,10 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 	esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
 			     SDHCI_VENDOR_VER_SHIFT;
 	esdhc->spec_ver = host_ver & SDHCI_SPEC_VER_MASK;
+	if (soc_device_match(soc_incorrect_hostver))
+		esdhc->quirk_incorrect_hostver = true;
+	else
+		esdhc->quirk_incorrect_hostver = false;
 }
 
 static int sdhci_esdhc_probe(struct platform_device *pdev)
-- 
2.1.0.27.g96db324