[PATCH V2] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[PATCH V2] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

After Broadcom switched from MIPS to ARM for their home routers we need
to have NVRAM driver in some common place (not arch/mips/).
We were thinking about putting it in bus directory, however there are
two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
won't fit there neither.
This is why I would like to move this driver to the drivers/soc/

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Use drivers/soc/broadcom/ (instead of misc) and use -M for patch

I wasn't sure who to to/cc sending this patch. There isn't entry for
drivers/soc/ in MAINTAINERS. I picked e-mails from the commit
3a6e08218f36baa9c49282ad2fe0dfbf001d8f23
soc: Introduce drivers/soc place-holder for SOC specific drivers
---
 arch/mips/Kconfig                                            |  1 +
 arch/mips/bcm47xx/Makefile                                   |  2 +-
 arch/mips/bcm47xx/board.c                                    |  2 +-
 arch/mips/bcm47xx/setup.c                                    |  1 -
 arch/mips/bcm47xx/sprom.c                                    |  1 -
 arch/mips/bcm47xx/time.c                                     |  1 -
 arch/mips/include/asm/mach-bcm47xx/bcm47xx.h                 |  1 +
 drivers/bcma/driver_mips.c                                   |  2 +-
 drivers/net/ethernet/broadcom/b44.c                          |  2 +-
 drivers/net/ethernet/broadcom/bgmac.c                        |  2 +-
 drivers/soc/Kconfig                                          |  1 +
 drivers/soc/Makefile                                         |  1 +
 drivers/soc/broadcom/Kconfig                                 | 12 ++++++++++++
 drivers/soc/broadcom/Makefile                                |  1 +
 .../bcm47xx/nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c  |  4 +++-
 drivers/ssb/driver_chipcommon_pmu.c                          |  2 +-
 drivers/ssb/driver_mipscore.c                                |  2 +-
 .../asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h       |  3 ---
 18 files changed, 27 insertions(+), 14 deletions(-)
 create mode 100644 drivers/soc/broadcom/Kconfig
 create mode 100644 drivers/soc/broadcom/Makefile
 rename arch/mips/bcm47xx/nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c (98%)
 rename {arch/mips/include/asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h (84%)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 4a7e0c1..3d647e3 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -134,6 +134,7 @@ config BCM47XX
 	select USE_GENERIC_EARLY_PRINTK_8250
 	select GPIOLIB
 	select LEDS_GPIO_REGISTER
+	select BCM47XX_NVRAM
 	help
 	 Support for BCM47XX based boards
 
diff --git a/arch/mips/bcm47xx/Makefile b/arch/mips/bcm47xx/Makefile
index d58c51b..66bea4e 100644
--- a/arch/mips/bcm47xx/Makefile
+++ b/arch/mips/bcm47xx/Makefile
@@ -3,5 +3,5 @@
 # under Linux.
 #
 
-obj-y				+= irq.o nvram.o prom.o serial.o setup.o time.o sprom.o
+obj-y				+= irq.o prom.o serial.o setup.o time.o sprom.o
 obj-y				+= board.o buttons.o leds.o workarounds.o
diff --git a/arch/mips/bcm47xx/board.c b/arch/mips/bcm47xx/board.c
index b3ae068..6e85130 100644
--- a/arch/mips/bcm47xx/board.c
+++ b/arch/mips/bcm47xx/board.c
@@ -1,8 +1,8 @@
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/string.h>
+#include <bcm47xx.h>
 #include <bcm47xx_board.h>
-#include <bcm47xx_nvram.h>
 
 struct bcm47xx_board_type {
 	const enum bcm47xx_board board;
diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
index e43b504..b26c9c2 100644
--- a/arch/mips/bcm47xx/setup.c
+++ b/arch/mips/bcm47xx/setup.c
@@ -42,7 +42,6 @@
 #include <asm/reboot.h>
 #include <asm/time.h>
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <bcm47xx_board.h>
 
 union bcm47xx_bus bcm47xx_bus;
diff --git a/arch/mips/bcm47xx/sprom.c b/arch/mips/bcm47xx/sprom.c
index 2eff7fe..a077ed2 100644
--- a/arch/mips/bcm47xx/sprom.c
+++ b/arch/mips/bcm47xx/sprom.c
@@ -27,7 +27,6 @@
  */
 
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <linux/if_ether.h>
 #include <linux/etherdevice.h>
 
diff --git a/arch/mips/bcm47xx/time.c b/arch/mips/bcm47xx/time.c
index 2c85d92..5b46510 100644
--- a/arch/mips/bcm47xx/time.c
+++ b/arch/mips/bcm47xx/time.c
@@ -27,7 +27,6 @@
 #include <linux/ssb/ssb.h>
 #include <asm/time.h>
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <bcm47xx_board.h>
 
 void __init plat_time_init(void)
diff --git a/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h b/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
index 7527c1d..8ed77f6 100644
--- a/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
+++ b/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
@@ -22,6 +22,7 @@
 #include <linux/ssb/ssb.h>
 #include <linux/bcma/bcma.h>
 #include <linux/bcma/bcma_soc.h>
+#include <linux/bcm47xx_nvram.h>
 
 enum bcm47xx_bus_type {
 #ifdef CONFIG_BCM47XX_SSB
diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
index 8a653dc..15e278f 100644
--- a/drivers/bcma/driver_mips.c
+++ b/drivers/bcma/driver_mips.c
@@ -21,7 +21,7 @@
 #include <linux/serial_reg.h>
 #include <linux/time.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 enum bcma_boot_dev {
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 416620f..2095062 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -400,7 +400,7 @@ static void b44_set_flow_ctrl(struct b44 *bp, u32 local, u32 remote)
 }
 
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 static void b44_wap54g10_workaround(struct b44 *bp)
 {
 	char buf[20];
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 05c6af6..bdda57b 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -16,7 +16,7 @@
 #include <linux/phy.h>
 #include <linux/interrupt.h>
 #include <linux/dma-mapping.h>
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 
 static const struct bcma_device_id bgmac_bcma_tbl[] = {
 	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_4706_MAC_GBIT, BCMA_ANY_REV, BCMA_ANY_CLASS),
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 76d6bd4..6eee174 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -1,5 +1,6 @@
 menu "SOC (System On Chip) specific Drivers"
 
+source "drivers/soc/broadcom/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/ti/Kconfig"
 source "drivers/soc/versatile/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 063113d..c57e8f9 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the Linux Kernel SOC specific device drivers.
 #
 
+obj-y				+= broadcom/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-$(CONFIG_SOC_TI)		+= ti/
diff --git a/drivers/soc/broadcom/Kconfig b/drivers/soc/broadcom/Kconfig
new file mode 100644
index 0000000..4f1d498
--- /dev/null
+++ b/drivers/soc/broadcom/Kconfig
@@ -0,0 +1,12 @@
+#
+# Broadcom SoC drivers
+#
+
+config BCM47XX_NVRAM
+	bool "Broadcom NVRAM driver"
+	depends on BCM47XX || ARCH_BCM_5301X
+	help
+	  Broadcom home routers contain flash partition called "nvram" with all
+	  important hardware configuration as well as some minor user setup.
+	  It contains a text-like data representing name=value pairs.
+	  This driver provides an easy way to get value of requested parameter.
diff --git a/drivers/soc/broadcom/Makefile b/drivers/soc/broadcom/Makefile
new file mode 100644
index 0000000..f6816af
--- /dev/null
+++ b/drivers/soc/broadcom/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_BCM47XX_NVRAM)	+= bcm47xx_nvram.o
diff --git a/arch/mips/bcm47xx/nvram.c b/drivers/soc/broadcom/bcm47xx_nvram.c
similarity index 98%
rename from arch/mips/bcm47xx/nvram.c
rename to drivers/soc/broadcom/bcm47xx_nvram.c
index c5c381c..a4083d7 100644
--- a/arch/mips/bcm47xx/nvram.c
+++ b/drivers/soc/broadcom/bcm47xx_nvram.c
@@ -16,7 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/mtd/mtd.h>
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 
 #define NVRAM_MAGIC		0x48534C46	/* 'FLSH' */
 #define NVRAM_SPACE		0x8000
@@ -226,3 +226,5 @@ int bcm47xx_nvram_gpio_pin(const char *name)
 	return -ENOENT;
 }
 EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
+
+MODULE_LICENSE("GPLv2");
diff --git a/drivers/ssb/driver_chipcommon_pmu.c b/drivers/ssb/driver_chipcommon_pmu.c
index 1173a09..0942841 100644
--- a/drivers/ssb/driver_chipcommon_pmu.c
+++ b/drivers/ssb/driver_chipcommon_pmu.c
@@ -14,7 +14,7 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 #include "ssb_private.h"
diff --git a/drivers/ssb/driver_mipscore.c b/drivers/ssb/driver_mipscore.c
index 7b986f9..f87efef 100644
--- a/drivers/ssb/driver_mipscore.c
+++ b/drivers/ssb/driver_mipscore.c
@@ -16,7 +16,7 @@
 #include <linux/serial_reg.h>
 #include <linux/time.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 #include "ssb_private.h"
diff --git a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
similarity index 84%
rename from arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
rename to include/linux/bcm47xx_nvram.h
index ee59ffe..5ed6917 100644
--- a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
+++ b/include/linux/bcm47xx_nvram.h
@@ -1,7 +1,4 @@
 /*
- *  Copyright (C) 2005, Broadcom Corporation
- *  Copyright (C) 2006, Felix Fietkau <nbd@openwrt.org>
- *
  *  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
-- 
1.8.4.5

Re: [PATCH V2] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Hauke Mehrtens]

On 11/23/2014 10:50 AM, Rafał Miłecki wrote:
> After Broadcom switched from MIPS to ARM for their home routers we need
> to have NVRAM driver in some common place (not arch/mips/).
> We were thinking about putting it in bus directory, however there are
> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
> won't fit there neither.
> This is why I would like to move this driver to the drivers/soc/
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: Use drivers/soc/broadcom/ (instead of misc) and use -M for patch
> 
> I wasn't sure who to to/cc sending this patch. There isn't entry for
> drivers/soc/ in MAINTAINERS. I picked e-mails from the commit
> 3a6e08218f36baa9c49282ad2fe0dfbf001d8f23
> soc: Introduce drivers/soc place-holder for SOC specific drivers
> ---
>  arch/mips/Kconfig                                            |  1 +
>  arch/mips/bcm47xx/Makefile                                   |  2 +-
>  arch/mips/bcm47xx/board.c                                    |  2 +-
>  arch/mips/bcm47xx/setup.c                                    |  1 -
>  arch/mips/bcm47xx/sprom.c                                    |  1 -
>  arch/mips/bcm47xx/time.c                                     |  1 -
>  arch/mips/include/asm/mach-bcm47xx/bcm47xx.h                 |  1 +
>  drivers/bcma/driver_mips.c                                   |  2 +-
>  drivers/net/ethernet/broadcom/b44.c                          |  2 +-
>  drivers/net/ethernet/broadcom/bgmac.c                        |  2 +-
>  drivers/soc/Kconfig                                          |  1 +
>  drivers/soc/Makefile                                         |  1 +
>  drivers/soc/broadcom/Kconfig                                 | 12 ++++++++++++
>  drivers/soc/broadcom/Makefile                                |  1 +
>  .../bcm47xx/nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c  |  4 +++-
>  drivers/ssb/driver_chipcommon_pmu.c                          |  2 +-
>  drivers/ssb/driver_mipscore.c                                |  2 +-
>  .../asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h       |  3 ---
>  18 files changed, 27 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/soc/broadcom/Kconfig
>  create mode 100644 drivers/soc/broadcom/Makefile
>  rename arch/mips/bcm47xx/nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c (98%)
>  rename {arch/mips/include/asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h (84%)
> 

....

> diff --git a/drivers/soc/broadcom/Kconfig b/drivers/soc/broadcom/Kconfig
> new file mode 100644
> index 0000000..4f1d498
> --- /dev/null
> +++ b/drivers/soc/broadcom/Kconfig
> @@ -0,0 +1,12 @@
> +#
> +# Broadcom SoC drivers
> +#
> +
> +config BCM47XX_NVRAM
> +	bool "Broadcom NVRAM driver"
> +	depends on BCM47XX || ARCH_BCM_5301X
> +	help
> +	  Broadcom home routers contain flash partition called "nvram" with all
> +	  important hardware configuration as well as some minor user setup.
> +	  It contains a text-like data representing name=value pairs.
> +	  This driver provides an easy way to get value of requested parameter.

You could also explicitly add that this "driver" does not drive any
hardware. I think your text already says so, but it could be that
someone does not understand this.

> diff --git a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
> similarity index 84%
> rename from arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
> rename to include/linux/bcm47xx_nvram.h
> index ee59ffe..5ed6917 100644
> --- a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
> +++ b/include/linux/bcm47xx_nvram.h
> @@ -1,7 +1,4 @@
>  /*
> - *  Copyright (C) 2005, Broadcom Corporation
> - *  Copyright (C) 2006, Felix Fietkau <nbd@openwrt.org>
> - *

Any reason for removing these copyright statements? I think that nothing
in this file is copyrightable, but I am not a lawyer and would not
remove these lines.

>   *  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
> 

[PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

After Broadcom switched from MIPS to ARM for their home routers we need
to have NVRAM driver in some common place (not arch/mips/).
We were thinking about putting it in bus directory, however there are
two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
won't fit there neither.
This is why I would like to move this driver to the drivers/soc/

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Use drivers/soc/broadcom/ (instead of misc) and use -M for patch
V3: Extend description in Kconfig by saying what this drivers does
    Make calls to bcm47xx_nvram_* safe even with the driver disabled

Hauke: to address your question about .h copyrights.
1) I don't claim/have any copyrights to the
bcm47xx_nvram_init_from_mem function name
2) I don't think you claim/have any copyrights to the
bcm47xx_nvram_gpio_pin or bcm47xx_nvram_getenv function names
3) String "nvram_getenv" is not copyrightable
---
 arch/mips/Kconfig                                        |  1 +
 arch/mips/bcm47xx/Makefile                               |  2 +-
 arch/mips/bcm47xx/board.c                                |  2 +-
 arch/mips/bcm47xx/setup.c                                |  1 -
 arch/mips/bcm47xx/sprom.c                                |  1 -
 arch/mips/bcm47xx/time.c                                 |  1 -
 arch/mips/include/asm/mach-bcm47xx/bcm47xx.h             |  1 +
 drivers/bcma/driver_mips.c                               |  2 +-
 drivers/net/ethernet/broadcom/b44.c                      |  2 +-
 drivers/net/ethernet/broadcom/bgmac.c                    |  2 +-
 drivers/soc/Kconfig                                      |  1 +
 drivers/soc/Makefile                                     |  1 +
 drivers/soc/broadcom/Kconfig                             | 15 +++++++++++++++
 drivers/soc/broadcom/Makefile                            |  1 +
 .../nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c      |  4 +++-
 drivers/ssb/driver_chipcommon_pmu.c                      |  2 +-
 drivers/ssb/driver_mipscore.c                            |  2 +-
 .../asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h   | 16 +++++++++++++---
 18 files changed, 43 insertions(+), 14 deletions(-)
 create mode 100644 drivers/soc/broadcom/Kconfig
 create mode 100644 drivers/soc/broadcom/Makefile
 rename arch/mips/bcm47xx/nvram.c => drivers/soc/broadcom/bcm47xx_nvram.c (98%)
 rename {arch/mips/include/asm/mach-bcm47xx => include/linux}/bcm47xx_nvram.h (63%)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 4a7e0c1..3d647e3 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -134,6 +134,7 @@ config BCM47XX
 	select USE_GENERIC_EARLY_PRINTK_8250
 	select GPIOLIB
 	select LEDS_GPIO_REGISTER
+	select BCM47XX_NVRAM
 	help
 	 Support for BCM47XX based boards
 
diff --git a/arch/mips/bcm47xx/Makefile b/arch/mips/bcm47xx/Makefile
index d58c51b..66bea4e 100644
--- a/arch/mips/bcm47xx/Makefile
+++ b/arch/mips/bcm47xx/Makefile
@@ -3,5 +3,5 @@
 # under Linux.
 #
 
-obj-y				+= irq.o nvram.o prom.o serial.o setup.o time.o sprom.o
+obj-y				+= irq.o prom.o serial.o setup.o time.o sprom.o
 obj-y				+= board.o buttons.o leds.o workarounds.o
diff --git a/arch/mips/bcm47xx/board.c b/arch/mips/bcm47xx/board.c
index b3ae068..6e85130 100644
--- a/arch/mips/bcm47xx/board.c
+++ b/arch/mips/bcm47xx/board.c
@@ -1,8 +1,8 @@
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/string.h>
+#include <bcm47xx.h>
 #include <bcm47xx_board.h>
-#include <bcm47xx_nvram.h>
 
 struct bcm47xx_board_type {
 	const enum bcm47xx_board board;
diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
index e43b504..b26c9c2 100644
--- a/arch/mips/bcm47xx/setup.c
+++ b/arch/mips/bcm47xx/setup.c
@@ -42,7 +42,6 @@
 #include <asm/reboot.h>
 #include <asm/time.h>
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <bcm47xx_board.h>
 
 union bcm47xx_bus bcm47xx_bus;
diff --git a/arch/mips/bcm47xx/sprom.c b/arch/mips/bcm47xx/sprom.c
index 2eff7fe..a077ed2 100644
--- a/arch/mips/bcm47xx/sprom.c
+++ b/arch/mips/bcm47xx/sprom.c
@@ -27,7 +27,6 @@
  */
 
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <linux/if_ether.h>
 #include <linux/etherdevice.h>
 
diff --git a/arch/mips/bcm47xx/time.c b/arch/mips/bcm47xx/time.c
index 2c85d92..5b46510 100644
--- a/arch/mips/bcm47xx/time.c
+++ b/arch/mips/bcm47xx/time.c
@@ -27,7 +27,6 @@
 #include <linux/ssb/ssb.h>
 #include <asm/time.h>
 #include <bcm47xx.h>
-#include <bcm47xx_nvram.h>
 #include <bcm47xx_board.h>
 
 void __init plat_time_init(void)
diff --git a/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h b/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
index 7527c1d..8ed77f6 100644
--- a/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
+++ b/arch/mips/include/asm/mach-bcm47xx/bcm47xx.h
@@ -22,6 +22,7 @@
 #include <linux/ssb/ssb.h>
 #include <linux/bcma/bcma.h>
 #include <linux/bcma/bcma_soc.h>
+#include <linux/bcm47xx_nvram.h>
 
 enum bcm47xx_bus_type {
 #ifdef CONFIG_BCM47XX_SSB
diff --git a/drivers/bcma/driver_mips.c b/drivers/bcma/driver_mips.c
index 8a653dc..15e278f 100644
--- a/drivers/bcma/driver_mips.c
+++ b/drivers/bcma/driver_mips.c
@@ -21,7 +21,7 @@
 #include <linux/serial_reg.h>
 #include <linux/time.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 enum bcma_boot_dev {
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 416620f..2095062 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -400,7 +400,7 @@ static void b44_set_flow_ctrl(struct b44 *bp, u32 local, u32 remote)
 }
 
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 static void b44_wap54g10_workaround(struct b44 *bp)
 {
 	char buf[20];
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 05c6af6..bdda57b 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -16,7 +16,7 @@
 #include <linux/phy.h>
 #include <linux/interrupt.h>
 #include <linux/dma-mapping.h>
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 
 static const struct bcma_device_id bgmac_bcma_tbl[] = {
 	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_4706_MAC_GBIT, BCMA_ANY_REV, BCMA_ANY_CLASS),
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 76d6bd4..6eee174 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -1,5 +1,6 @@
 menu "SOC (System On Chip) specific Drivers"
 
+source "drivers/soc/broadcom/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/ti/Kconfig"
 source "drivers/soc/versatile/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 063113d..c57e8f9 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the Linux Kernel SOC specific device drivers.
 #
 
+obj-y				+= broadcom/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-$(CONFIG_SOC_TI)		+= ti/
diff --git a/drivers/soc/broadcom/Kconfig b/drivers/soc/broadcom/Kconfig
new file mode 100644
index 0000000..5fa2e32
--- /dev/null
+++ b/drivers/soc/broadcom/Kconfig
@@ -0,0 +1,15 @@
+#
+# Broadcom SoC drivers
+#
+
+config BCM47XX_NVRAM
+	bool "Broadcom NVRAM driver"
+	depends on BCM47XX || ARCH_BCM_5301X
+	help
+	  Broadcom home routers contain flash partition called "nvram" with all
+	  important hardware configuration as well as some minor user setup.
+	  NVRAM partition contains a text-like data representing name=value
+	  pairs.
+	  This driver provides an easy way to get value of requested parameter.
+	  It simply reads content of NVRAM and parses it. It doesn't control any
+	  hardware part itself.
diff --git a/drivers/soc/broadcom/Makefile b/drivers/soc/broadcom/Makefile
new file mode 100644
index 0000000..f6816af
--- /dev/null
+++ b/drivers/soc/broadcom/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_BCM47XX_NVRAM)	+= bcm47xx_nvram.o
diff --git a/arch/mips/bcm47xx/nvram.c b/drivers/soc/broadcom/bcm47xx_nvram.c
similarity index 98%
rename from arch/mips/bcm47xx/nvram.c
rename to drivers/soc/broadcom/bcm47xx_nvram.c
index c5c381c..a4083d7 100644
--- a/arch/mips/bcm47xx/nvram.c
+++ b/drivers/soc/broadcom/bcm47xx_nvram.c
@@ -16,7 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/mtd/mtd.h>
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 
 #define NVRAM_MAGIC		0x48534C46	/* 'FLSH' */
 #define NVRAM_SPACE		0x8000
@@ -226,3 +226,5 @@ int bcm47xx_nvram_gpio_pin(const char *name)
 	return -ENOENT;
 }
 EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
+
+MODULE_LICENSE("GPLv2");
diff --git a/drivers/ssb/driver_chipcommon_pmu.c b/drivers/ssb/driver_chipcommon_pmu.c
index 1173a09..0942841 100644
--- a/drivers/ssb/driver_chipcommon_pmu.c
+++ b/drivers/ssb/driver_chipcommon_pmu.c
@@ -14,7 +14,7 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 #include "ssb_private.h"
diff --git a/drivers/ssb/driver_mipscore.c b/drivers/ssb/driver_mipscore.c
index 7b986f9..f87efef 100644
--- a/drivers/ssb/driver_mipscore.c
+++ b/drivers/ssb/driver_mipscore.c
@@ -16,7 +16,7 @@
 #include <linux/serial_reg.h>
 #include <linux/time.h>
 #ifdef CONFIG_BCM47XX
-#include <bcm47xx_nvram.h>
+#include <linux/bcm47xx_nvram.h>
 #endif
 
 #include "ssb_private.h"
diff --git a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
similarity index 63%
rename from arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
rename to include/linux/bcm47xx_nvram.h
index ee59ffe..659dfb7 100644
--- a/arch/mips/include/asm/mach-bcm47xx/bcm47xx_nvram.h
+++ b/include/linux/bcm47xx_nvram.h
@@ -1,7 +1,4 @@
 /*
- *  Copyright (C) 2005, Broadcom Corporation
- *  Copyright (C) 2006, Felix Fietkau <nbd@openwrt.org>
- *
  *  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
@@ -14,8 +11,21 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 
+#ifdef CONFIG_BCM47XX_NVRAM
 int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
 int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
 int bcm47xx_nvram_gpio_pin(const char *name);
+#else
+static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim) {
+	return -ENOTSUPP;
+};
+static inline int bcm47xx_nvram_getenv(const char *name, char *val,
+				       size_t val_len) {
+	return -ENOTSUPP;
+};
+static inline int bcm47xx_nvram_gpio_pin(const char *name) {
+	return -ENOTSUPP;
+};
+#endif
 
 #endif /* __BCM47XX_NVRAM_H */
-- 
1.8.4.5

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2913 bytes --]

Hi Rafał.

On Sun, 23 Nov 2014, Rafał Miłecki wrote:

> After Broadcom switched from MIPS to ARM for their home routers we need
> to have NVRAM driver in some common place (not arch/mips/).

So this is a driver for a chunk of NVRAM that's embedded in the SoC, 
hanging off an I/O bus?

Is this actually some kind of RAM, or is it flash, or something else?

> We were thinking about putting it in bus directory, however there are
> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
> won't fit there neither.
> This is why I would like to move this driver to the drivers/soc/

Can't speak for anyone else, but I personally consider drivers/soc to be 
primarily intended for so-called "integration" IP blocks.  Those are used 
for SoC control functions.

So low-level NVRAM drivers would probably go somewhere else.  Here are 
some likely possibilities, where it can live with others of its kind 
(assuming it is something similar to RAM):

1. the PC "CMOS memory" NVRAM driver appears to be under drivers/char, as 
drivers/char/nvram.c.  

2. there's a generic SRAM driver, drivers/misc/sram.c

3. while people have put some of the eFuse code under drivers/soc, in my 
opinion, the low-level fuse access code should really go under 
drivers/misc/fuse.

... 

Looking at arch/mips/bcm47xx/nvram.c: if the low-level NVRAM probe code 
were moved elsewhere, the higher-level NVRAM "interpretation" functions 
still remain: bcm47xx_nvram_getenv() and bcm47xx_nvram_gpio_pin().  Those 
seem to be intended to parse device configuration data, yes?  And this 
device configuration data is organized this way by platform software 
convention -- there's no hardware requirement to store data this way in 
the NVRAM, right?

Unfortunately the way that we organize device data parsing code in the 
kernel is quite frankly ... anarchic.  In some kind of ideal world, we'd 
have a standard place for device data storage and parsing functions, and 
any combination of DT/ACPI/pdata/PCI/ISAPNP/whatever could be placed in 
use, depending on the software environment.  But instead we have 
directories like drivers/acpi and drivers/of and drivers/base/platform and 
drivers/pnp and drivers/platform/chrome.

So if the answer to the above two questions is "yes," meaning that this 
NVRAM is used to store device probing data by software convention, then it 
would seem to me that proposing some place like 
drivers/devicedata/bcm47xx-nvram is a better bet (where "bcm47xx-nvram" is 
just some kind of opaque token).  Looking at those two functions, it 
doesn't seem like there's really anything MIPS or Broadcom or NVRAM or 
even SoC-specific about key-value pair parsing and GPIO device data?  Or 
am I missing something?

Actually, considering that bcm47xx_nvram_gpio_pin() isn't even used, can 
we just drop it and save the hassle?  :-)


- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 24 November 2014 at 11:02, Paul Walmsley <paul@pwsan.com> wrote:
> Hi Rafał.
>
> On Sun, 23 Nov 2014, Rafał Miłecki wrote:
>
>> After Broadcom switched from MIPS to ARM for their home routers we need
>> to have NVRAM driver in some common place (not arch/mips/).
>
> So this is a driver for a chunk of NVRAM that's embedded in the SoC,
> hanging off an I/O bus?
>
> Is this actually some kind of RAM, or is it flash, or something else?

I think you missed my description (help section) in Kconfig file :) As
it says, NVRAM is a special partition on flash.


>> We were thinking about putting it in bus directory, however there are
>> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
>> won't fit there neither.
>> This is why I would like to move this driver to the drivers/soc/
>
> Can't speak for anyone else, but I personally consider drivers/soc to be
> primarily intended for so-called "integration" IP blocks.  Those are used
> for SoC control functions.
>
> So low-level NVRAM drivers would probably go somewhere else.  Here are
> some likely possibilities, where it can live with others of its kind
> (assuming it is something similar to RAM):
>
> 1. the PC "CMOS memory" NVRAM driver appears to be under drivers/char, as
> drivers/char/nvram.c.
>
> 2. there's a generic SRAM driver, drivers/misc/sram.c
>
> 3. while people have put some of the eFuse code under drivers/soc, in my
> opinion, the low-level fuse access code should really go under
> drivers/misc/fuse.

I don't think NVRAM can be treated as a standard char device. Also, in
my V1 I tried moving it to the drivers/misc/, but then drivers/soc/
was suggested as a better place, see Arnd's reply:
http://www.linux-mips.org/archives/linux-mips/2014-11/msg00238.html


> Looking at arch/mips/bcm47xx/nvram.c: if the low-level NVRAM probe code
> were moved elsewhere, the higher-level NVRAM "interpretation" functions
> still remain: bcm47xx_nvram_getenv() and bcm47xx_nvram_gpio_pin().  Those
> seem to be intended to parse device configuration data, yes?  And this
> device configuration data is organized this way by platform software
> convention -- there's no hardware requirement to store data this way in
> the NVRAM, right?

This bcm47xx_nvram driver has two ways of reading NVRAM content:
1) Using a standard MTD API to access "nvram" partition. In such case
flash driver is a low-level thing if you call it so.
2) Using memory-mapped flash access window. In such case there isn't
any extra low-level driver.

The "nvram" partition is required by the bootloader (CFE). It contains
some important info like CPU clock, RAM configuration, switch ports
layout. Then it's used by system drivers (like Ethernet driver bgmac)
to get info about some particular hardware parts (like PHY address,
MAC, etc.).


> So if the answer to the above two questions is "yes," meaning that this
> NVRAM is used to store device probing data by software convention, then it
> would seem to me that proposing some place like
> drivers/devicedata/bcm47xx-nvram is a better bet (where "bcm47xx-nvram" is
> just some kind of opaque token).  Looking at those two functions, it
> doesn't seem like there's really anything MIPS or Broadcom or NVRAM or
> even SoC-specific about key-value pair parsing and GPIO device data?  Or
> am I missing something?
>
> Actually, considering that bcm47xx_nvram_gpio_pin() isn't even used, can
> we just drop it and save the hassle?  :-)

We need this function to easily find GPIO responsible for "something".
For example during switch initialization we need to reset it toggling
GPIO. NVRAM contains info which GPIO should be toggled, e.g.:
gpio4=robo_reset
(robo means roboswitch)

bcm47xx_nvram_gpio_pin is needed by out-of-tree "b53" driver that some
tried to push upstream, but was rejected because of API.

I'll also send a patch to USB host driver soon that will require
bcm47xx_nvram_gpio_pin.

-- 
Rafał

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7056 bytes --]

Hi Rafał,

On Mon, 24 Nov 2014, Rafał Miłecki wrote:

> On 24 November 2014 at 11:02, Paul Walmsley <paul@pwsan.com> wrote:
> > Hi Rafał.
> >
> > On Sun, 23 Nov 2014, Rafał Miłecki wrote:
> >
> >> After Broadcom switched from MIPS to ARM for their home routers we need
> >> to have NVRAM driver in some common place (not arch/mips/).
> >
> > So this is a driver for a chunk of NVRAM that's embedded in the SoC,
> > hanging off an I/O bus?
> >
> > Is this actually some kind of RAM, or is it flash, or something else?
> 
> I think you missed my description (help section) in Kconfig file :)

That's correct.  I only looked at the patch description in the E-mail, and 
the code in arch/mips/bcm47xx/nvram.c.

> As it says, NVRAM is a special partition on flash.

OK, thanks for the clarification.

> >> We were thinking about putting it in bus directory, however there are
> >> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this
> >> won't fit there neither.
> >> This is why I would like to move this driver to the drivers/soc/
> >
> > Can't speak for anyone else, but I personally consider drivers/soc to be
> > primarily intended for so-called "integration" IP blocks.  Those are used
> > for SoC control functions.
> >
> > So low-level NVRAM drivers would probably go somewhere else.  Here are
> > some likely possibilities, where it can live with others of its kind
> > (assuming it is something similar to RAM):
> >
> > 1. the PC "CMOS memory" NVRAM driver appears to be under drivers/char, as
> > drivers/char/nvram.c.
> >
> > 2. there's a generic SRAM driver, drivers/misc/sram.c
> >
> > 3. while people have put some of the eFuse code under drivers/soc, in my
> > opinion, the low-level fuse access code should really go under
> > drivers/misc/fuse.
> 
> I don't think NVRAM can be treated as a standard char device. Also, in
> my V1 I tried moving it to the drivers/misc/, but then drivers/soc/
> was suggested as a better place, see Arnd's reply:
> http://www.linux-mips.org/archives/linux-mips/2014-11/msg00238.html

Yeah.  It depends on who is going to merge the patch.  If you can persuade 
someone else to merge it in drivers/soc, then it doesn't really matter 
what I think.

To my mind, drivers/soc was intended to fill a very narrow and specific 
need: for low-level, hardware IP block drivers for so-called 'integration' 
IP blocks.  These IP blocks generally control some combination of clocks, 
external muxing (pin mux), internal muxing, reset, IO cells/bricks, debug 
observability, etc., and tend to be very specific to particular SoC 
families.  While it's certainly understandable that folks would 
like a place to move arbitrary code out of arch/*, drivers/soc was not 
intended to be that.

> > Looking at arch/mips/bcm47xx/nvram.c: if the low-level NVRAM probe code
> > were moved elsewhere, the higher-level NVRAM "interpretation" functions
> > still remain: bcm47xx_nvram_getenv() and bcm47xx_nvram_gpio_pin().  Those
> > seem to be intended to parse device configuration data, yes?  And this
> > device configuration data is organized this way by platform software
> > convention -- there's no hardware requirement to store data this way in
> > the NVRAM, right?
> 
> This bcm47xx_nvram driver has two ways of reading NVRAM content:
> 1) Using a standard MTD API to access "nvram" partition. In such case
> flash driver is a low-level thing if you call it so.

OK, so just to confirm, you are referring to these drivers:

drivers/mtd/nand/bcm47xxnflash/*
drivers/mtd/devices/bcm47xxsflash.c 
drivers/ssb/driver_mipscore.c (for pflash)
drivers/bcma/driver_mips.c (for pflash)

?  

Just out of curiosity, the nvram.c code seems to contain some code to work 
around cases where the flash size is larger than the MMIO aperture, and to 
truncate the copy.  Is there some way to program the flash controller to 
point the aperture at a different section of the flash?

> 2) Using memory-mapped flash access window. In such case there isn't 
> any extra low-level driver.

Could you point me at the software entity that configures the 
memory-mapped flash access window and programs the flash controller to use 
one of {p,n,s}flash?  Or is that done by some code external to the kernel, 
like the bootloader?

> The "nvram" partition is required by the bootloader (CFE). It contains
> some important info like CPU clock, RAM configuration, switch ports
> layout. Then it's used by system drivers (like Ethernet driver bgmac)
> to get info about some particular hardware parts (like PHY address,
> MAC, etc.).

So it's not used by the on-chip boot-ROM?  Sounds like it's just software 
convention, then?  In other words, if one used a different bootloader, 
like u-boot, and passed a DT or something like that to the kernel, this 
wouldn't be needed.  The presence and format of this flash is part of a 
specific hardware/software platform, external to the SoC hardware itself, 
that Broadcom suggests.  It's analogous to ARM ATAGS - 
arch/arm/kernel/atags_parse.c.  Does all this match your understanding?  
(I'm not advocating using a different bootloader or device data format - I 
think it's important to preserve compatibility with CPE - I am just trying 
to understand how this area of flash is used.)

> > So if the answer to the above two questions is "yes," meaning that this
> > NVRAM is used to store device probing data by software convention, then it
> > would seem to me that proposing some place like
> > drivers/devicedata/bcm47xx-nvram is a better bet (where "bcm47xx-nvram" is
> > just some kind of opaque token).  Looking at those two functions, it
> > doesn't seem like there's really anything MIPS or Broadcom or NVRAM or
> > even SoC-specific about key-value pair parsing and GPIO device data?  Or
> > am I missing something?
> >
> > Actually, considering that bcm47xx_nvram_gpio_pin() isn't even used, can
> > we just drop it and save the hassle?  :-)
> 
> We need this function to easily find GPIO responsible for "something".
> For example during switch initialization we need to reset it toggling
> GPIO. NVRAM contains info which GPIO should be toggled, e.g.:
> gpio4=robo_reset
> (robo means roboswitch)
> 
> bcm47xx_nvram_gpio_pin is needed by out-of-tree "b53" driver that some
> tried to push upstream, but was rejected because of API.
> 
> I'll also send a patch to USB host driver soon that will require
> bcm47xx_nvram_gpio_pin.

OK thanks for the context.

It sounds to me like this code is a combination of three 
pieces:

1. code that autoprobes the size of the "nvram" partition in the Broadcom 
platform flash, by reading various locations in the MMIO flash aperture, 
configured by some other system entity

2. code that shadow copies the device data from the MMIO flash aperture 
into system RAM

3. code that parses the CPE-generated device data and returns it to other 
drivers

Does that sound accurate?


- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 25 November 2014 at 18:50, Paul Walmsley <paul@pwsan.com> wrote:
>> I don't think NVRAM can be treated as a standard char device. Also, in
>> my V1 I tried moving it to the drivers/misc/, but then drivers/soc/
>> was suggested as a better place, see Arnd's reply:
>> http://www.linux-mips.org/archives/linux-mips/2014-11/msg00238.html
>
> Yeah.  It depends on who is going to merge the patch.  If you can persuade
> someone else to merge it in drivers/soc, then it doesn't really matter
> what I think.

I'm looking for the solution that will satisfy most ppl. I understand
your arguments against drivers/soc/, but on the other hand I have no
idea where else this driver could go.


>> > Looking at arch/mips/bcm47xx/nvram.c: if the low-level NVRAM probe code
>> > were moved elsewhere, the higher-level NVRAM "interpretation" functions
>> > still remain: bcm47xx_nvram_getenv() and bcm47xx_nvram_gpio_pin().  Those
>> > seem to be intended to parse device configuration data, yes?  And this
>> > device configuration data is organized this way by platform software
>> > convention -- there's no hardware requirement to store data this way in
>> > the NVRAM, right?
>>
>> This bcm47xx_nvram driver has two ways of reading NVRAM content:
>> 1) Using a standard MTD API to access "nvram" partition. In such case
>> flash driver is a low-level thing if you call it so.
>
> OK, so just to confirm, you are referring to these drivers:
>
> drivers/mtd/nand/bcm47xxnflash/*
> drivers/mtd/devices/bcm47xxsflash.c
> drivers/ssb/driver_mipscore.c (for pflash)
> drivers/bcma/driver_mips.c (for pflash)
>
> ?

Yes, these are drivers for Broadcom flash memories that implement
(more or less directly) MTD API. There is also a new SPI-NOR based
driver I'm trying to upstream (it was already sent, needs accepting
some more spi-nor changes first).


> Just out of curiosity, the nvram.c code seems to contain some code to work
> around cases where the flash size is larger than the MMIO aperture, and to
> truncate the copy.  Is there some way to program the flash controller to
> point the aperture at a different section of the flash?

Er, not really. We assume NVRAM content is not bigger than NVRAM_SPACE
(0x8000), but that's all. If NVRAM can be read using MMIO, then it's
always fully available.

There isn't any documented way of reprogramming flash content mapping
into memory.


>> 2) Using memory-mapped flash access window. In such case there isn't
>> any extra low-level driver.
>
> Could you point me at the software entity that configures the
> memory-mapped flash access window and programs the flash controller to use
> one of {p,n,s}flash?  Or is that done by some code external to the kernel,
> like the bootloader?

It's done by the CFE (bootloader). You can find CFE source in some
tarballs published by Asus in their GPL firmware packages. I never
really focused on CFE source, never tried to compiling it or
re-installing.


>> The "nvram" partition is required by the bootloader (CFE). It contains
>> some important info like CPU clock, RAM configuration, switch ports
>> layout. Then it's used by system drivers (like Ethernet driver bgmac)
>> to get info about some particular hardware parts (like PHY address,
>> MAC, etc.).
>
> So it's not used by the on-chip boot-ROM?  Sounds like it's just software
> convention, then?  In other words, if one used a different bootloader,
> like u-boot, and passed a DT or something like that to the kernel, this
> wouldn't be needed.  The presence and format of this flash is part of a
> specific hardware/software platform, external to the SoC hardware itself,
> that Broadcom suggests.  It's analogous to ARM ATAGS -
> arch/arm/kernel/atags_parse.c.  Does all this match your understanding?
> (I'm not advocating using a different bootloader or device data format - I
> think it's important to preserve compatibility with CPE - I am just trying
> to understand how this area of flash is used.)

I guess yes, you could probably use different bootloader and hardcode
all important settings into it (e.g. using DTB).

I guess DT is older than CFE, but Broadcom decided to invent own
solution called NVRAM anyway. This is a bit messy, because it actually
stores hardware details (CPU, RAM, switch) as well as user settings
(e.g. LEDs behavior). I can't say why Broadcom decided to implement it
this way.

We all would love to see Broadcom shipping devices with U-Boot and DT! ;)


>> > So if the answer to the above two questions is "yes," meaning that this
>> > NVRAM is used to store device probing data by software convention, then it
>> > would seem to me that proposing some place like
>> > drivers/devicedata/bcm47xx-nvram is a better bet (where "bcm47xx-nvram" is
>> > just some kind of opaque token).  Looking at those two functions, it
>> > doesn't seem like there's really anything MIPS or Broadcom or NVRAM or
>> > even SoC-specific about key-value pair parsing and GPIO device data?  Or
>> > am I missing something?
>> >
>> > Actually, considering that bcm47xx_nvram_gpio_pin() isn't even used, can
>> > we just drop it and save the hassle?  :-)
>>
>> We need this function to easily find GPIO responsible for "something".
>> For example during switch initialization we need to reset it toggling
>> GPIO. NVRAM contains info which GPIO should be toggled, e.g.:
>> gpio4=robo_reset
>> (robo means roboswitch)
>>
>> bcm47xx_nvram_gpio_pin is needed by out-of-tree "b53" driver that some
>> tried to push upstream, but was rejected because of API.
>>
>> I'll also send a patch to USB host driver soon that will require
>> bcm47xx_nvram_gpio_pin.
>
> OK thanks for the context.
>
> It sounds to me like this code is a combination of three
> pieces:
>
> 1. code that autoprobes the size of the "nvram" partition in the Broadcom
> platform flash, by reading various locations in the MMIO flash aperture,
> configured by some other system entity

That's right, on MIPS we simply detect flash type (drivers/ssb &
driver/bcma) and using that we init NVRAM passing memory offset where
the flash is mapped.


> 2. code that shadow copies the device data from the MMIO flash aperture
> into system RAM
>
> 3. code that parses the CPE-generated device data and returns it to other
> drivers
>
> Does that sound accurate?

Correct (s/CPE/CFE).

-- 
Rafał

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3537 bytes --]

+ lkml

Hi Rafał,

On Tue, 25 Nov 2014, Rafał Miłecki wrote:

> On 25 November 2014 at 18:50, Paul Walmsley <paul@pwsan.com> wrote:
> >> I don't think NVRAM can be treated as a standard char device. Also, in
> >> my V1 I tried moving it to the drivers/misc/, but then drivers/soc/
> >> was suggested as a better place, see Arnd's reply:
> >> http://www.linux-mips.org/archives/linux-mips/2014-11/msg00238.html
> >
> > Yeah.  It depends on who is going to merge the patch.  If you can persuade
> > someone else to merge it in drivers/soc, then it doesn't really matter
> > what I think.
> 
> I'm looking for the solution that will satisfy most ppl. 

Wow, that's a pretty high bar ;-)  Better IMHO to try for something that 
seems to make rational sense, then let others convert it into something 
that makes less sense afterwards, if necessary ;-)

> I understand your arguments against drivers/soc/, but on the other hand 
> I have no idea where else this driver could go.

After looking around the tree to find out where similar code is located, 
it looks like drivers/firmware is the right place.  These days, 
drivers/firmware is mainly used for drivers that parse EFI bootloader 
data, DMI data, that sort of thing.  Quite similar to the CFE-provided 
data that the bcm47xx-nvram code deals with.  So, by functional analogy, 
drivers/firmware appears to be the right place to stash this device 
data-probing code.

> I guess DT is older than CFE, but Broadcom decided to invent own
> solution called NVRAM anyway. This is a bit messy, because it actually
> stores hardware details (CPU, RAM, switch) as well as user settings
> (e.g. LEDs behavior). I can't say why Broadcom decided to implement it
> this way.

Yep, based on what the other drivers in drivers/firmware are used for, I 
think drivers/firmware is the right place for the CFE parsing code.

> > It sounds to me like this code is a combination of three
> > pieces:
> >
> > 1. code that autoprobes the size of the "nvram" partition in the Broadcom
> > platform flash, by reading various locations in the MMIO flash aperture,
> > configured by some other system entity
> 
> That's right, on MIPS we simply detect flash type (drivers/ssb &
> driver/bcma) and using that we init NVRAM passing memory offset where
> the flash is mapped.

OK.

So (as a side issue), I would suggest that when you move this code out of 
arch/mips, the MIPS-isms in it should be removed, like KSEG1ADDR(), etc., 
and replaced by the standard ioremap()-type approach.  After all, Broadcom 
could build CFE for ARM, and then we'd want to use this same code to parse 
the CFE-provided data.

Also I would suggest getting rid of the #ifdefs for the flash type, and 
probing it dynamically instead.  The flash setup code under drivers/ssb/ 
and drivers/bcma/ sets up platform_devices for the flash, right?  If so 
then it would be best if this code could run after the bus setup code, 
query the Linux device model for the type of platform flash in use, and 
then extract the appropriate address space to probe from that data.

> > 2. code that shadow copies the device data from the MMIO flash aperture
> > into system RAM
> >
> > 3. code that parses the CPE-generated device data and returns it to other
> > drivers
> >
> > Does that sound accurate?
> 
> Correct (s/CPE/CFE).

Thanks for the correction, I must have been dreaming of telecom 
equipment..

What do you think?  Does this sound reasonable?


- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 27 November 2014 at 20:56, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 25 Nov 2014, Rafał Miłecki wrote:
>> I understand your arguments against drivers/soc/, but on the other hand
>> I have no idea where else this driver could go.
>
> After looking around the tree to find out where similar code is located,
> it looks like drivers/firmware is the right place.  These days,
> drivers/firmware is mainly used for drivers that parse EFI bootloader
> data, DMI data, that sort of thing.  Quite similar to the CFE-provided
> data that the bcm47xx-nvram code deals with.  So, by functional analogy,
> drivers/firmware appears to be the right place to stash this device
> data-probing code.
>
>> I guess DT is older than CFE, but Broadcom decided to invent own
>> solution called NVRAM anyway. This is a bit messy, because it actually
>> stores hardware details (CPU, RAM, switch) as well as user settings
>> (e.g. LEDs behavior). I can't say why Broadcom decided to implement it
>> this way.
>
> Yep, based on what the other drivers in drivers/firmware are used for, I
> think drivers/firmware is the right place for the CFE parsing code.

The problem is I can't find MAINTAINER of the drivers/firmware/. Is
there someone responsible for that? Some mailing list maybe? Who could
give us an ACK to move bcm47xx_nvram there?


>> > It sounds to me like this code is a combination of three
>> > pieces:
>> >
>> > 1. code that autoprobes the size of the "nvram" partition in the Broadcom
>> > platform flash, by reading various locations in the MMIO flash aperture,
>> > configured by some other system entity
>>
>> That's right, on MIPS we simply detect flash type (drivers/ssb &
>> driver/bcma) and using that we init NVRAM passing memory offset where
>> the flash is mapped.
>
> OK.
>
> So (as a side issue), I would suggest that when you move this code out of
> arch/mips, the MIPS-isms in it should be removed, like KSEG1ADDR(), etc.,
> and replaced by the standard ioremap()-type approach.  After all, Broadcom
> could build CFE for ARM, and then we'd want to use this same code to parse
> the CFE-provided data.
>
> Also I would suggest getting rid of the #ifdefs for the flash type, and
> probing it dynamically instead.  The flash setup code under drivers/ssb/
> and drivers/bcma/ sets up platform_devices for the flash, right?  If so
> then it would be best if this code could run after the bus setup code,
> query the Linux device model for the type of platform flash in use, and
> then extract the appropriate address space to probe from that data.

I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
I wouldn't dare to move such a MIPS-focused driver to some common
place ;)

Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
think you'll like it much more. Hopefully you will even consider it
ready for moving to the drivers/firmware/ or whatever :)

-- 
Rafał

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5905 bytes --]

Hi Rafał,

On Thu, 27 Nov 2014, Rafał Miłecki wrote:

> On 27 November 2014 at 20:56, Paul Walmsley <paul@pwsan.com> wrote:
> > On Tue, 25 Nov 2014, Rafał Miłecki wrote:
> >> I understand your arguments against drivers/soc/, but on the other hand
> >> I have no idea where else this driver could go.
> >
> > After looking around the tree to find out where similar code is located,
> > it looks like drivers/firmware is the right place.  These days,
> > drivers/firmware is mainly used for drivers that parse EFI bootloader
> > data, DMI data, that sort of thing.  Quite similar to the CFE-provided
> > data that the bcm47xx-nvram code deals with.  So, by functional analogy,
> > drivers/firmware appears to be the right place to stash this device
> > data-probing code.
> >
> >> I guess DT is older than CFE, but Broadcom decided to invent own
> >> solution called NVRAM anyway. This is a bit messy, because it actually
> >> stores hardware details (CPU, RAM, switch) as well as user settings
> >> (e.g. LEDs behavior). I can't say why Broadcom decided to implement it
> >> this way.
> >
> > Yep, based on what the other drivers in drivers/firmware are used for, I
> > think drivers/firmware is the right place for the CFE parsing code.
> 
> The problem is I can't find MAINTAINER of the drivers/firmware/. Is
> there someone responsible for that? Some mailing list maybe? Who could
> give us an ACK to move bcm47xx_nvram there?

The list of folks who have committed patches that touch drivers/firmware 
is large.  I did this as a first-order approximation:

$ git log --format=fuller drivers/firmware/* | grep Commit: | sort -u
Commit:     Adrian Bunk <bunk@kernel.org>
Commit:     Adrian Bunk <bunk@stusta.de>
Commit:     Al Viro <viro@zeniv.linux.org.uk>
Commit:     Andi Kleen <andi@basil.nowhere.org>
Commit:     Benjamin Herrenschmidt <benh@kernel.crashing.org>
Commit:     David S. Miller <davem@davemloft.net>
Commit:     David Woodhouse <David.Woodhouse@intel.com>
Commit:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Commit:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Commit:     Greg Kroah-Hartman <gregkh@suse.de>
Commit:     H. Peter Anvin <hpa@linux.intel.com>
Commit:     H. Peter Anvin <hpa@zytor.com>
Commit:     Ingo Molnar <mingo@elte.hu>
Commit:     Ingo Molnar <mingo@kernel.org>
Commit:     James Bottomley <James.Bottomley@suse.de>
Commit:     James Bottomley <JBottomley@Parallels.com>
Commit:     Jean Delvare <khali@linux-fr.org>
Commit:     Jeff Garzik <jeff@garzik.org>
Commit:     Jeff Garzik <jgarzik@redhat.com>
Commit:     Jesse Barnes <jbarnes@virtuousgeek.org>
Commit:     Jiri Kosina <jkosina@suse.cz>
Commit:     Konrad Rzeszutek Wilk <konrad@kernel.org>
Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Commit:     Len Brown <len.brown@intel.com>
Commit:     Linus Torvalds <torvalds@g5.osdl.org>
Commit:     Linus Torvalds <torvalds@linux-foundation.org>
Commit:     Linus Torvalds <torvalds@ppc970.osdl.org>
Commit:     Linus Torvalds <torvalds@woody.linux-foundation.org>
Commit:     Linus Torvalds <torvalds@woody.osdl.org>
Commit:     Mark Brown <broonie@opensource.wolfsonmicro.com>
Commit:     Mark M. Hoffman <mhoffman@lightlink.com>
Commit:     Matt Fleming <matt.fleming@intel.com>
Commit:     Matthew Wilcox <willy@linux.intel.com>
Commit:     Paul Gortmaker <paul.gortmaker@windriver.com>
Commit:     Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Commit:     Russell King <rmk+kernel@arm.linux.org.uk>
Commit:     Tejun Heo <tj@kernel.org>
Commit:     Theodore Ts'o <tytso@mit.edu>
Commit:     Thomas Gleixner <tglx@linutronix.de>
Commit:     Tony Luck <tony.luck@intel.com>

If I were in your shoes, I would suggest either

1. asking Ralf to merge your patches that touch drivers/firmware, since 
he'll also presumably be merging the parts that touch arch/mips

or 

2. asking Greg KH to merge those patches

And of course it would not hurt to collect some Reviewed-By:s from other 
folks.  (See below...)

> >> > It sounds to me like this code is a combination of three
> >> > pieces:
> >> >
> >> > 1. code that autoprobes the size of the "nvram" partition in the Broadcom
> >> > platform flash, by reading various locations in the MMIO flash aperture,
> >> > configured by some other system entity
> >>
> >> That's right, on MIPS we simply detect flash type (drivers/ssb &
> >> driver/bcma) and using that we init NVRAM passing memory offset where
> >> the flash is mapped.
> >
> > OK.
> >
> > So (as a side issue), I would suggest that when you move this code out of
> > arch/mips, the MIPS-isms in it should be removed, like KSEG1ADDR(), etc.,
> > and replaced by the standard ioremap()-type approach.  After all, Broadcom
> > could build CFE for ARM, and then we'd want to use this same code to parse
> > the CFE-provided data.
> >
> > Also I would suggest getting rid of the #ifdefs for the flash type, and
> > probing it dynamically instead.  The flash setup code under drivers/ssb/
> > and drivers/bcma/ sets up platform_devices for the flash, right?  If so
> > then it would be best if this code could run after the bus setup code,
> > query the Linux device model for the type of platform flash in use, and
> > then extract the appropriate address space to probe from that data.
> 
> I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
> I wouldn't dare to move such a MIPS-focused driver to some common
> place ;)
> 
> Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
> think you'll like it much more. Hopefully you will even consider it
> ready for moving to the drivers/firmware/ or whatever :)

OK I will take a look at this, and will either send comments, or will 
send a Reviewed-By:.


Thanks for all of the good discussion on this :-)

- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Ralf Baechle]

On Fri, Nov 28, 2014 at 05:07:12PM +0000, Paul Walmsley wrote:

> If I were in your shoes, I would suggest either
> 
> 1. asking Ralf to merge your patches that touch drivers/firmware, since 
> he'll also presumably be merging the parts that touch arch/mips
> 
> or 
> 
> 2. asking Greg KH to merge those patches
> 
> And of course it would not hurt to collect some Reviewed-By:s from other 
> folks.  (See below...)

I surely can carry this patch.

  Ralf

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4179 bytes --]

Hello Rafał,

On Fri, 28 Nov 2014, Paul Walmsley wrote:

> On Thu, 27 Nov 2014, Rafał Miłecki wrote:
> 
> > I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
> > I wouldn't dare to move such a MIPS-focused driver to some common
> > place ;)
> > 
> > Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
> > think you'll like it much more. Hopefully you will even consider it
> > ready for moving to the drivers/firmware/ or whatever :)
> 
> OK I will take a look at this, and will either send comments, or will 
> send a Reviewed-By:.

I had the chance to take a brief look at this file, and you are right: I
like your newer version better than the older one :-)

It is too bad that it seems this flash area has to be accessed very early 
in boot.  That certainly makes it more difficult to write something 
particularly elegant.  It is also a pity that it is unknown how to verify 
that the flash MMIO window has been configured before reading from these 
areas of the address space.  But under the circumstances, calling 
bcm47xx_nvram_init_from_mem() with the appropriate addresses from the bus 
code during early init, as you did, seems rather reasonable.  I also like 
the code that you added to read the flash data from MTD later in boot.

Here are a few very minor comments that you are welcome to take or leave 
as you wish.

1. In nvram_find_and_copy(), the flash header copy loop uses:

	for (i = 0; i < sizeof(struct nvram_header); i += 4)
		*dst++ = *src++;

Consider using either __raw_readl() to read from the MMIO window, or 
possibly memcpy_fromio().  In theory, all MMIO accesses should use 
functions like these.


2. In nvram_find_and_copy(), the flash payload copy loop uses:

	for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
		*dst++ = le32_to_cpu(*src++);

Consider using readl() instead of the direct MMIO read & endianness 
conversion.  


3. In nvram_find_and_copy(), I don't believe that this is necessary:

memset(dst, 0x0, NVRAM_SPACE - i);

since nvram_buf[] is a file-static variable, and thus should have been
initialized to all zeroes.


4. As with #3 above, in nvram_init(), I don't believe that this is 
necessary:

memset(dst + bytes_read, 0x0, NVRAM_SPACE - bytes_read);


5. In bcm47xx_nvram_getenv(), this multiple assignment exists:

end[0] = end[1] = '\0';

Best to avoid multiple assignments, per Chapter 1 of 
Documentation/CodingStyle.  You might consider running checkpatch.pl on 
the file:

$ scripts/checkpatch.pl -f --strict arch/mips/bcm47xx/nvram.c
CHECK: No space is necessary after a cast
#101: FILE: arch/mips/bcm47xx/nvram.c:101:
+	src = (u32 *) header;

CHECK: No space is necessary after a cast
#102: FILE: arch/mips/bcm47xx/nvram.c:102:
+	dst = (u32 *) nvram_buf;

CHECK: multiple assignments should be avoided
#195: FILE: arch/mips/bcm47xx/nvram.c:195:
+	end[0] = end[1] = '\0';

CHECK: Alignment should match open parenthesis
#202: FILE: arch/mips/bcm47xx/nvram.c:202:
+		if ((eq - var) == strlen(name) &&
+			strncmp(var, name, (eq - var)) == 0) {


6. bcm47xx_nvram_getenv() calls strchr().  Perhaps it would be better to 
use strnchr(), in case the flash data is corrupted or in an invalid 
format?


7. There are a few magic numbers in this code, mostly in 
bcm47xx_nvram_gpio_pin().  It might be worth converting those to macros 
and documenting the expectations there in a comment above the macro.


8.  The way that bcm47xx_nvram_gpio_pin() calls bcm47xx_nvram_getenv() 
seems a bit inefficient.  It might be better to loop over all of the keys 
in the shadowed flash, looking for values that match "name".  Then if the 
key name matches "gpio" plus one or two digits, the code could just return 
the digits.  That way, only one pass is needed, rather than 32 (in the 
worst case).  Well, at least the reads should be coming from cached DRAM, 
rather than flash...

If you fix/address those (or correct my review ;-) ), then you're 
welcome to add my Reviewed-by: to a patch that moves this file out to 
drivers/firmware.


regards, 

- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 4 December 2014 at 07:43, Paul Walmsley <paul@pwsan.com> wrote:
> Hello Rafał,
>
> On Fri, 28 Nov 2014, Paul Walmsley wrote:
>
>> On Thu, 27 Nov 2014, Rafał Miłecki wrote:
>>
>> > I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
>> > I wouldn't dare to move such a MIPS-focused driver to some common
>> > place ;)
>> >
>> > Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
>> > think you'll like it much more. Hopefully you will even consider it
>> > ready for moving to the drivers/firmware/ or whatever :)
>>
>> OK I will take a look at this, and will either send comments, or will
>> send a Reviewed-By:.
>
> I had the chance to take a brief look at this file, and you are right: I
> like your newer version better than the older one :-)
>
> It is too bad that it seems this flash area has to be accessed very early
> in boot.  That certainly makes it more difficult to write something
> particularly elegant.  It is also a pity that it is unknown how to verify
> that the flash MMIO window has been configured before reading from these
> areas of the address space.  But under the circumstances, calling
> bcm47xx_nvram_init_from_mem() with the appropriate addresses from the bus
> code during early init, as you did, seems rather reasonable.  I also like
> the code that you added to read the flash data from MTD later in boot.
>
> Here are a few very minor comments that you are welcome to take or leave
> as you wish.

Thanks for your comments! I'll address them before (trying) moving
driver to the drivers/firmware/. Appreciate your review.