[PATCH 1/3] firmware: add lpc18xx boot rom driver

Hi Joachim,

On 13.09.2016 22:51, Joachim Eastwood wrote:

Firmware driver for the boot ROM found on all NXP LPC18xx/43xx
devices. This driver makes it possible to retrieve device specific
information from either the ROM via API calls or from OTP memory.

The boot ROM contains several APIs for on-chip devices. Note that not
all APIs are available on all devices. The API to retrieve device
information and internal Flash programming (IAP) is only available on
devices with Flash. Flashless devices retrieve device information from
OTP memory. The CHIPID register in CREG (syscon) is used to check if
IAP is available.

For now this driver is only used to expose device information via a
'SoC device'. Linux API for the IAP and OTP will be added later. These
two APIs will be used by a Flash MTD driver and a OTP NVMEM driver to
program the memory.

Signed-off-by: Joachim Eastwood <redacted>

please feel free to add to the series my

  Tested-by: Vladimir Zapolskiy [off-list ref]
  Reviewed-by: Vladimir Zapolskiy [off-list ref]

I tested the change on a board powered by LPC4357.

Nevertheless I have some nitpicks for your consideration.

And the first one is that I'm not totally convinced that drivers/firmware
is the proper place for the driver, I tend to think that it might be
a good time to create drivers/soc/nxp/

---
 drivers/firmware/Kconfig            |  12 ++
 drivers/firmware/Makefile           |   1 +
 drivers/firmware/nxp_lpc_boot_rom.c | 411 ++++++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/firmware/nxp_lpc_boot_rom.c

[snip]

+/* LPC18xx/43xx CREG (syscon) defines */
+#define LPC18XX_CREG_CHIPID		0x200
+#define LPC18XX_FLASH_CHIPID0		0x4284e02b
+#define LPC18XX_FLASH_CHIPID1		0x7284e02b
+#define LPC18XX_FLASHLESS_CHIPID0	0x5284e02b
+#define LPC18XX_FLASHLESS_CHIPID1	0x6284e02b
+#define LPC43XX_FLASH_CHIPID0		0x4906002b
+#define LPC43XX_FLASH_CHIPID1		0x7906002b
+#define LPC43XX_FLASHLESS_CHIPID0	0x5906002b
+#define LPC43XX_FLASHLESS_CHIPID1	0x6906002b
+
+#define NXP_PART_LPC(_num, _id0, _id1, _sz0, _sz1)	\
+	{						\
+		.name = "LPC"#_num,			\
+		.id[0] = _id0, .id[1] = _id1,		\
+		.flash_size[0] = _sz0 * 1024,		\
+		.flash_size[1] = _sz1 * 1024,		\
+	}
+
+

checkpatch complains:

CHECK: Please don't use multiple blank lines
#145: FILE: drivers/firmware/nxp_lpc_boot_rom.c:75:
+
+

+struct nxp_lpc_part {
+	const char *name;
+	u16 flash_size[2];

flash_size[2] is set by NXP_PART_LPC() macro but unused, however
in my build environment (W=1 and sparse checks) I get tons of
legitimate compile time warnings:

drivers/firmware/nxp_lpc_boot_rom.c:93:9: warning: cast truncates bits from constant value (80000 becomes 0)
drivers/firmware/nxp_lpc_boot_rom.c:93:9: warning: cast truncates bits from constant value (80000 becomes 0)
drivers/firmware/nxp_lpc_boot_rom.c:94:9: warning: cast truncates bits from constant value (80000 becomes 0)
drivers/firmware/nxp_lpc_boot_rom.c:94:9: warning: cast truncates bits from constant value (80000 becomes 0)
....

drivers/firmware/nxp_lpc_boot_rom.c:93:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
   NXP_PART_LPC(1857,  0xf001d830, 0x00, 512, 512),
   ^
drivers/firmware/nxp_lpc_boot_rom.c:93:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
drivers/firmware/nxp_lpc_boot_rom.c:94:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
   NXP_PART_LPC(18S57, 0xf001d860, 0x00, 512, 512),
   ^
....

and so on.

Please consider either to store flash size in kB or change storage type to u32.

+	u32 id[2];
+};
+
+static const struct nxp_lpc_part nxp_lpc_parts[] = {
+	/* LPC18xx Flashless parts */
+	NXP_PART_LPC(1850,  0xf000d830, 0x00,   0,   0),
+	NXP_PART_LPC(18S50, 0xf000d860, 0x00,   0,   0),
+	NXP_PART_LPC(1830,  0xf000da30, 0x00,   0,   0),
+	NXP_PART_LPC(18S30, 0xf000da60, 0x00,   0,   0),
+	NXP_PART_LPC(1820,  0xf00adb3c, 0x00,   0,   0),
+	NXP_PART_LPC(18S20, 0xf00adb6c, 0x00,   0,   0),
+	NXP_PART_LPC(1810,  0xf00b5b3f, 0x00,   0,   0),
+	NXP_PART_LPC(18S10, 0xf00b5b6f, 0x00,   0,   0),
+	/* LPC18xx Flash parts */
+	NXP_PART_LPC(1857,  0xf001d830, 0x00, 512, 512),
+	NXP_PART_LPC(18S57, 0xf001d860, 0x00, 512, 512),
+	NXP_PART_LPC(1853,  0xf001d830, 0x44, 256, 256),
+	NXP_PART_LPC(1837,  0xf001da30, 0x00, 512, 512),
+	NXP_PART_LPC(18S37, 0xf001d860, 0x00, 512, 512),
+	NXP_PART_LPC(1833,  0xf001da30, 0x44, 256, 256),
+	NXP_PART_LPC(1827,  0xf001db3c, 0x00, 512, 512),
+	NXP_PART_LPC(1825,  0xf001db3c, 0x22, 384, 384),
+	NXP_PART_LPC(1823,  0xf00bdb3c, 0x44, 256, 256),
+	NXP_PART_LPC(1822,  0xf00bdb3c, 0x80, 512,   0),
+	NXP_PART_LPC(1817,  0xf001db3f, 0x00, 512, 512),
+	NXP_PART_LPC(1815,  0xf001db3f, 0x22, 384, 384),
+	NXP_PART_LPC(1813,  0xf00bdb3f, 0x44, 256, 256),
+	NXP_PART_LPC(1812,  0xf00bdb3f, 0x80, 512,   0),
+	/* LPC43xx Flashless parts */
+	NXP_PART_LPC(4370,  0x00000030, 0x00,   0,   0), /* LBGA256 */
+	NXP_PART_LPC(4370,  0x00000230, 0x00,   0,   0), /* TFBGA100 */
+	NXP_PART_LPC(43S70, 0x00000060, 0x00,   0,   0),
+	NXP_PART_LPC(4350,  0xa0000830, 0x00,   0,   0),
+	NXP_PART_LPC(43S50, 0xa0000860, 0x00,   0,   0),
+	NXP_PART_LPC(4330,  0xa0000a30, 0x00,   0,   0),
+	NXP_PART_LPC(43S30, 0xa0000a60, 0x00,   0,   0),
+	NXP_PART_LPC(4320,  0xa000cb3c, 0x00,   0,   0),
+	NXP_PART_LPC(43S20, 0xa000cb6c, 0x00,   0,   0),
+	NXP_PART_LPC(4310,  0xa00acb3f, 0x00,   0,   0),
+	/* LPC43xx parts with Flash */
+	NXP_PART_LPC(4367,  0x8001c030, 0x00, 512, 512),
+	NXP_PART_LPC(43S67, 0x8001c060, 0x00, 512, 512),
+	NXP_PART_LPC(4357,  0xa001c830, 0x00, 512, 512),
+	NXP_PART_LPC(43S57, 0xa001c860, 0x00, 512, 512), /* LBGA256 */
+	NXP_PART_LPC(43S57, 0xa001ca60, 0x00, 512, 512), /* LQFP208 */
+	NXP_PART_LPC(4353,  0xa001c830, 0x44, 256, 256),
+	NXP_PART_LPC(4337,  0xa001ca30, 0x00, 512, 512),
+	NXP_PART_LPC(43S37, 0xa001ca60, 0x00, 512, 512),
+	NXP_PART_LPC(4333,  0xa001ca30, 0x44, 256, 256),
+	NXP_PART_LPC(4327,  0xa001cb3c, 0x00, 512, 512),
+	NXP_PART_LPC(4325,  0xa001cb3c, 0x22, 384, 384),
+	NXP_PART_LPC(4323,  0xa00bcb3c, 0x44, 256, 256),
+	NXP_PART_LPC(4322,  0xa00bcb3c, 0x80, 512,   0),
+	NXP_PART_LPC(4317,  0xa001cb3f, 0x00, 512, 512),
+	NXP_PART_LPC(4315,  0xa001cb3f, 0x22, 384, 384),
+	NXP_PART_LPC(4313,  0xa00bcb3f, 0x44, 256, 256),
+	NXP_PART_LPC(4312,  0xa00bcb3f, 0x80, 512,   0),
+};
+
+struct iap_rom {
+	void (*entry)(u32 *, u32 *);
+};

Here it might be simpler to declare a typedef instead of a struct.

+struct nxp_rom_api {
+	struct device *dev;
+	void __iomem *rom;
+
+	bool has_iap;
+	struct iap_rom iap;
+	spinlock_t lock;
+
+	const struct nxp_lpc_part *part;
+	const char *partname;
+	u32 boot_version;
+
+	struct soc_device *soc_dev;
+	struct soc_device_attribute soc_dev_attr;
+};
+

--
With best wishes,
Vladimir