diff mbox series

[08/10] sun5i: add support for DIP detection to CHIP board

Message ID 20210218132938.2168-9-kory.maincent@bootlin.com
State Superseded
Delegated to: Tom Rini
Headers show
Series Add support for extension boards detection and DT overlays application | expand

Commit Message

Kory Maincent Feb. 18, 2021, 1:29 p.m. UTC
Add the extension_board_scan specific function to scan the information
of the EEPROM on one-wire and fill the extension struct.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 arch/arm/mach-sunxi/Kconfig |   2 +
 board/sunxi/Makefile        |   1 +
 board/sunxi/chip.c          | 104 ++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 board/sunxi/chip.c

Comments

Tom Rini Feb. 19, 2021, 4:54 p.m. UTC | #1
On Thu, Feb 18, 2021 at 02:29:36PM +0100, Kory Maincent wrote:

> Add the extension_board_scan specific function to scan the information
> of the EEPROM on one-wire and fill the extension struct.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  arch/arm/mach-sunxi/Kconfig |   2 +
>  board/sunxi/Makefile        |   1 +
>  board/sunxi/chip.c          | 104 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 board/sunxi/chip.c
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 37a4294d88..3bb8a08b34 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -6,10 +6,12 @@ choice
>  
>  config TARGET_CHIP
>  	bool "CHIP board"
> +	select SUPPORT_EXTENSION_SCAN
>  	select W1
>  	select W1_GPIO
>  	select W1_EEPROM
>  	select W1_EEPROM_DS24XXX
> +	imply CMD_EXTENSION

And then based on my comment in the previous patch, similar to
PINEPHONE_DT_SELECTION and other board-specific options, add a
CHIP_DIP_SCAN option or similar.
Andre Przywara Feb. 19, 2021, 5:29 p.m. UTC | #2
On Fri, 19 Feb 2021 11:54:54 -0500
Tom Rini <trini@konsulko.com> wrote:

Hi,

> On Thu, Feb 18, 2021 at 02:29:36PM +0100, Kory Maincent wrote:
> 
> > Add the extension_board_scan specific function to scan the information
> > of the EEPROM on one-wire and fill the extension struct.
> > 
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/Kconfig |   2 +
> >  board/sunxi/Makefile        |   1 +
> >  board/sunxi/chip.c          | 104 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 107 insertions(+)
> >  create mode 100644 board/sunxi/chip.c
> > 
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index 37a4294d88..3bb8a08b34 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -6,10 +6,12 @@ choice
> >  
> >  config TARGET_CHIP
> >  	bool "CHIP board"
> > +	select SUPPORT_EXTENSION_SCAN
> >  	select W1
> >  	select W1_GPIO
> >  	select W1_EEPROM
> >  	select W1_EEPROM_DS24XXX
> > +	imply CMD_EXTENSION  
> 
> And then based on my comment in the previous patch, similar to
> PINEPHONE_DT_SELECTION and other board-specific options, add a
> CHIP_DIP_SCAN option or similar.

Yes, indeed, thanks Tom.

The idea of making this extension scheme generic is great, it's just
that sunxi is using a different approach to board specific code here.
Normally we expect U-Boot-proper board specific code to be DT driven,
and where this does not really work, use those symbols that Tom pointed
to.

Thanks,
Andre
Kory Maincent Feb. 22, 2021, 2:23 p.m. UTC | #3
Hi Tom, Andre

Thanks for your reviews.

On Fri, 19 Feb 2021 17:29:26 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

> > 
> > And then based on my comment in the previous patch, similar to
> > PINEPHONE_DT_SELECTION and other board-specific options, add a
> > CHIP_DIP_SCAN option or similar.  
> 
> Yes, indeed, thanks Tom.
> 
> The idea of making this extension scheme generic is great, it's just
> that sunxi is using a different approach to board specific code here.
> Normally we expect U-Boot-proper board specific code to be DT driven,
> and where this does not really work, use those symbols that Tom pointed
> to.

For my extension_board_scan board specific function, would you prefer if I move
to callback like below instead of Kconfig?

if (of_machine_is_compatible("nextthing,chip"))
	extension_board_register_callback(chip_extension_board_scan);

Regards,
Köry
Andre Przywara Feb. 23, 2021, 1:30 p.m. UTC | #4
On Mon, 22 Feb 2021 15:23:08 +0100
Köry Maincent <kory.maincent@bootlin.com> wrote:

Hi Köry,

> Thanks for your reviews.
> 
> On Fri, 19 Feb 2021 17:29:26 +0000
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > > 
> > > And then based on my comment in the previous patch, similar to
> > > PINEPHONE_DT_SELECTION and other board-specific options, add a
> > > CHIP_DIP_SCAN option or similar.    
> > 
> > Yes, indeed, thanks Tom.
> > 
> > The idea of making this extension scheme generic is great, it's just
> > that sunxi is using a different approach to board specific code here.
> > Normally we expect U-Boot-proper board specific code to be DT driven,
> > and where this does not really work, use those symbols that Tom pointed
> > to.  
> 
> For my extension_board_scan board specific function, would you prefer if I move
> to callback like below instead of Kconfig?
> 
> if (of_machine_is_compatible("nextthing,chip"))
> 	extension_board_register_callback(chip_extension_board_scan);

Well, the CHIP Pro uses a different compatible string, so you would
need to check for that too.
I am not fully decided if checking for the machine compatible is the
right approach. The more traditional U-Boot way would be to define a
config symbol (as Tom already pointed out), that would also keep the
code out of other board builds.
We do this already with CONFIG_PINE64_DT_SELECTION and
CONFIG_PINEPHONE_DT_SELECTION.

Cheers,
Andre
Kory Maincent Feb. 23, 2021, 1:59 p.m. UTC | #5
Hello Andre,

On Tue, 23 Feb 2021 13:30:38 +0000
Andre Przywara <andre.przywara@arm.com> wrote:
y extension_board_scan board specific function, would you prefer if I
> > move to callback like below instead of Kconfig?
> > 
> > if (of_machine_is_compatible("nextthing,chip"))
> > 	extension_board_register_callback(chip_extension_board_scan);  
> 
> Well, the CHIP Pro uses a different compatible string, so you would
> need to check for that too.
> I am not fully decided if checking for the machine compatible is the
> right approach. The more traditional U-Boot way would be to define a
> config symbol (as Tom already pointed out), that would also keep the
> code out of other board builds.
> We do this already with CONFIG_PINE64_DT_SELECTION and
> CONFIG_PINEPHONE_DT_SELECTION.

Okay, I will move on to the traditional config symbol.

Thanks.

Köry
diff mbox series

Patch

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index 37a4294d88..3bb8a08b34 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -6,10 +6,12 @@  choice
 
 config TARGET_CHIP
 	bool "CHIP board"
+	select SUPPORT_EXTENSION_SCAN
 	select W1
 	select W1_GPIO
 	select W1_EEPROM
 	select W1_EEPROM_DS24XXX
+	imply CMD_EXTENSION
 
 endchoice
 
diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
index c4e13f8c38..e6dcc9fca5 100644
--- a/board/sunxi/Makefile
+++ b/board/sunxi/Makefile
@@ -11,3 +11,4 @@  obj-$(CONFIG_SUN7I_GMAC)	+= gmac.o
 obj-$(CONFIG_MACH_SUN4I)	+= dram_sun4i_auto.o
 obj-$(CONFIG_MACH_SUN5I)	+= dram_sun5i_auto.o
 obj-$(CONFIG_MACH_SUN7I)	+= dram_sun5i_auto.o
+obj-$(CONFIG_TARGET_CHIP)	+= chip.o
diff --git a/board/sunxi/chip.c b/board/sunxi/chip.c
new file mode 100644
index 0000000000..fc3d14e497
--- /dev/null
+++ b/board/sunxi/chip.c
@@ -0,0 +1,104 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2021
+ * Köry Maincent, Bootlin, <kory.maincent@bootlin.com>
+ * Based on initial code from Maxime Ripard
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <dm.h>
+#include <w1.h>
+#include <w1-eeprom.h>
+#include <dm/device-internal.h>
+
+#include <asm/arch/gpio.h>
+
+#include <extension_board.h>
+
+#ifdef CONFIG_CMD_EXTENSION
+
+#define for_each_1w_device(b, d) \
+	for (device_find_first_child(b, d); *d; device_find_next_child(d))
+
+#define dip_convert(field)						\
+	(								\
+		(sizeof(field) == 1) ? field :				\
+		(sizeof(field) == 2) ? be16_to_cpu(field) :		\
+		(sizeof(field) == 4) ? be32_to_cpu(field) :		\
+		-1							\
+	)
+
+#define DIP_MAGIC	0x50494843	/* CHIP */
+
+struct dip_w1_header {
+	u32     magic;                  /* CHIP */
+	u8      version;                /* spec version */
+	u32     vendor_id;
+	u16     product_id;
+	u8      product_version;
+	char    vendor_name[32];
+	char    product_name[32];
+	u8      rsvd[36];               /* rsvd for future spec versions */
+	u8      data[16];               /* user data, per-dip specific */
+} __packed;
+
+int extension_board_scan(struct list_head *extension_list)
+{
+	struct extension *dip;
+	struct dip_w1_header w1_header;
+	struct udevice *bus, *dev;
+	u32 vid;
+	u16 pid;
+	int ret;
+
+	int num_dip = 0;
+
+	sunxi_gpio_set_pull(SUNXI_GPD(2), SUNXI_GPIO_PULL_UP);
+
+	ret = w1_get_bus(0, &bus);
+	if (ret) {
+		printf("one wire interface not found\n");
+		return 0;
+	}
+
+	for_each_1w_device(bus, &dev) {
+		if (w1_get_device_family(dev) != W1_FAMILY_DS2431)
+			continue;
+
+		ret = device_probe(dev);
+		if (ret) {
+			printf("Couldn't probe device %s: error %d",
+			       dev->name, ret);
+			continue;
+		}
+
+		w1_eeprom_read_buf(dev, 0, (u8 *)&w1_header, sizeof(w1_header));
+
+		if (w1_header.magic != DIP_MAGIC)
+			continue;
+
+		vid = dip_convert(w1_header.vendor_id);
+		pid = dip_convert(w1_header.product_id);
+
+		printf("DIP: %s (0x%x) from %s (0x%x)\n",
+		       w1_header.product_name, pid,
+		       w1_header.vendor_name, vid);
+
+		dip = calloc(1, sizeof(struct extension));
+		if (!dip) {
+			printf("Error in memory allocation\n");
+			return num_dip;
+		}
+
+		snprintf(dip->overlay, sizeof(dip->overlay), "dip-%x-%x.dtbo",
+			 vid, pid);
+		strncpy(dip->name, w1_header.product_name, 32);
+		strncpy(dip->owner, w1_header.vendor_name, 32);
+		list_add_tail(&dip->list, extension_list);
+		num_dip++;
+	}
+	return num_dip;
+}
+
+#endif /* CONFIG_CMD_EXTENSION */