* [v11, 0/8] Fix eSDHC host version register bug @ 2016-09-06 8:28 Yangbo Lu 2016-09-06 8:28 ` [v11, 1/8] dt: bindings: update Freescale DCFG compatible Yangbo Lu ` (7 more replies) 0 siblings, 8 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [v11, 1/8] dt: bindings: update Freescale DCFG compatible 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 8:28 ` [v11, 2/8] ARM64: dts: ls2080a: add device configuration node Yangbo Lu ` (6 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [v11, 2/8] ARM64: dts: ls2080a: add device configuration node 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu 2016-09-06 8:28 ` [v11, 1/8] dt: bindings: update Freescale DCFG compatible Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 8:28 ` [v11, 3/8] dt: bindings: move guts devicetree doc out of powerpc directory Yangbo Lu ` (5 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [v11, 3/8] dt: bindings: move guts devicetree doc out of powerpc directory 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu 2016-09-06 8:28 ` [v11, 1/8] dt: bindings: update Freescale DCFG compatible Yangbo Lu 2016-09-06 8:28 ` [v11, 2/8] ARM64: dts: ls2080a: add device configuration node Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 8:28 ` [v11, 4/8] powerpc/fsl: move mpc85xx.h to include/linux/fsl Yangbo Lu ` (4 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [v11, 4/8] powerpc/fsl: move mpc85xx.h to include/linux/fsl 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu ` (2 preceding siblings ...) 2016-09-06 8:28 ` [v11, 3/8] dt: bindings: move guts devicetree doc out of powerpc directory Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 8:28 ` [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Yangbo Lu ` (3 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu ` (3 preceding siblings ...) 2016-09-06 8:28 ` [v11, 4/8] powerpc/fsl: move mpc85xx.h to include/linux/fsl Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-09 3:47 ` Scott Wood 2016-09-06 8:28 ` [v11, 6/8] MAINTAINERS: add entry for Freescale SoC drivers Yangbo Lu ` (2 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-06 8:28 ` [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Yangbo Lu @ 2016-09-09 3:47 ` Scott Wood 2016-09-12 6:39 ` Y.B. Lu 0 siblings, 1 reply; 17+ messages in thread From: Scott Wood @ 2016-09-09 3:47 UTC (permalink / raw) To: Yangbo Lu, linux-mmc, ulf.hansson, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-09 3:47 ` Scott Wood @ 2016-09-12 6:39 ` Y.B. Lu 2016-09-12 23:25 ` Scott Wood 0 siblings, 1 reply; 17+ messages in thread From: Y.B. Lu @ 2016-09-12 6:39 UTC (permalink / raw) To: Scott Wood, linux-mmc, ulf.hansson, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-12 6:39 ` Y.B. Lu @ 2016-09-12 23:25 ` Scott Wood 2016-09-13 7:23 ` Y.B. Lu 0 siblings, 1 reply; 17+ messages in thread From: Scott Wood @ 2016-09-12 23:25 UTC (permalink / raw) To: Y.B. Lu, linux-mmc, ulf.hansson, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-12 23:25 ` Scott Wood @ 2016-09-13 7:23 ` Y.B. Lu 2016-09-13 22:24 ` Scott Wood 0 siblings, 1 reply; 17+ messages in thread From: Y.B. Lu @ 2016-09-13 7:23 UTC (permalink / raw) To: Scott Wood, linux-mmc, ulf.hansson, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, 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 > -----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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms 2016-09-13 7:23 ` Y.B. Lu @ 2016-09-13 22:24 ` Scott Wood 0 siblings, 0 replies; 17+ messages in thread From: Scott Wood @ 2016-09-13 22:24 UTC (permalink / raw) To: Y.B. Lu, linux-mmc, ulf.hansson, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [v11, 6/8] MAINTAINERS: add entry for Freescale SoC drivers 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu ` (4 preceding siblings ...) 2016-09-06 8:28 ` [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 8:28 ` [v11, 7/8] base: soc: introduce soc_device_match() interface Yangbo Lu 2016-09-06 8:28 ` [v11, 8/8] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Yangbo Lu 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [v11, 7/8] base: soc: introduce soc_device_match() interface 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu ` (5 preceding siblings ...) 2016-09-06 8:28 ` [v11, 6/8] MAINTAINERS: add entry for Freescale SoC drivers Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 2016-09-06 11:44 ` Ulf Hansson 2016-09-06 8:28 ` [v11, 8/8] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Yangbo Lu 7 siblings, 1 reply; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [v11, 7/8] base: soc: introduce soc_device_match() interface 2016-09-06 8:28 ` [v11, 7/8] base: soc: introduce soc_device_match() interface Yangbo Lu @ 2016-09-06 11:44 ` Ulf Hansson 2016-09-06 12:46 ` Arnd Bergmann 0 siblings, 1 reply; 17+ messages in thread From: Ulf Hansson @ 2016-09-06 11:44 UTC (permalink / raw) To: Yangbo Lu Cc: linux-mmc, Scott Wood, Arnd Bergmann, linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, Yang-Leo Li, Xiaobo Xie 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [v11, 7/8] base: soc: introduce soc_device_match() interface 2016-09-06 11:44 ` Ulf Hansson @ 2016-09-06 12:46 ` Arnd Bergmann 2016-09-07 4:10 ` Y.B. Lu 0 siblings, 1 reply; 17+ messages in thread From: Arnd Bergmann @ 2016-09-06 12:46 UTC (permalink / raw) To: Ulf Hansson Cc: Yangbo Lu, linux-mmc, Scott Wood, linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, Yang-Leo Li, Xiaobo Xie 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [v11, 7/8] base: soc: introduce soc_device_match() interface 2016-09-06 12:46 ` Arnd Bergmann @ 2016-09-07 4:10 ` Y.B. Lu 0 siblings, 0 replies; 17+ messages in thread From: Y.B. Lu @ 2016-09-07 4:10 UTC (permalink / raw) To: Arnd Bergmann, Ulf Hansson Cc: linux-mmc, Scott Wood, linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [v11, 8/8] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu ` (6 preceding siblings ...) 2016-09-06 8:28 ` [v11, 7/8] base: soc: introduce soc_device_match() interface Yangbo Lu @ 2016-09-06 8:28 ` Yangbo Lu 7 siblings, 0 replies; 17+ messages in thread From: Yangbo Lu @ 2016-09-06 8:28 UTC (permalink / raw) To: linux-mmc, ulf.hansson, Scott Wood, Arnd Bergmann Cc: linuxppc-dev, devicetree, linux-arm-kernel, linux-kernel, linux-clk, linux-i2c, iommu, netdev, Mark Rutland, Rob Herring, Russell King, Jochen Friedrich, Joerg Roedel, Claudiu Manoil, Bhupesh Sharma, Qiang Zhao, Kumar Gala, Santosh Shilimkar, leoyang.li, xiaobo.xie, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-09-13 22:24 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-06 8:28 [v11, 0/8] Fix eSDHC host version register bug Yangbo Lu 2016-09-06 8:28 ` [v11, 1/8] dt: bindings: update Freescale DCFG compatible Yangbo Lu 2016-09-06 8:28 ` [v11, 2/8] ARM64: dts: ls2080a: add device configuration node Yangbo Lu 2016-09-06 8:28 ` [v11, 3/8] dt: bindings: move guts devicetree doc out of powerpc directory Yangbo Lu 2016-09-06 8:28 ` [v11, 4/8] powerpc/fsl: move mpc85xx.h to include/linux/fsl Yangbo Lu 2016-09-06 8:28 ` [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Yangbo Lu 2016-09-09 3:47 ` Scott Wood 2016-09-12 6:39 ` Y.B. Lu 2016-09-12 23:25 ` Scott Wood 2016-09-13 7:23 ` Y.B. Lu 2016-09-13 22:24 ` Scott Wood 2016-09-06 8:28 ` [v11, 6/8] MAINTAINERS: add entry for Freescale SoC drivers Yangbo Lu 2016-09-06 8:28 ` [v11, 7/8] base: soc: introduce soc_device_match() interface Yangbo Lu 2016-09-06 11:44 ` Ulf Hansson 2016-09-06 12:46 ` Arnd Bergmann 2016-09-07 4:10 ` Y.B. Lu 2016-09-06 8:28 ` [v11, 8/8] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0 Yangbo Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).