[PATCH 00/17] RFC for additional 'nvmem' plumbing

[PATCH 00/17] RFC for additional 'nvmem' plumbing

[Andrey Smirnov]

Hi everyone,

The main purpose of this patchset is propose additional Nvmem plubing
needed for that subsytem's inclusion in BB and solicit feedback about
it (patches ## 15 to 17) . Included in this set are also a number of
bugfixes and RFC-for-bugfixes for problems that were discoverd during
development of this patch series.

Proposed Nvmem additions are two new platform device drivers that
implement a nvmem interface on top of data embedded in DT as well as
nvmem interface that allows to combine existing nvmem-accesible bytes
into continuous blocks.

Below is an example of DT code that uses both drivers in order to
create a MAC address as a combination of bytes from DT and OCOTP
module:

	ro-blob {
		compatible = "barebox,nvmem-ro-of-blob";
		#address-cells = <1>;
		#size-cells = <1>;

		ro_blob: ro-blob-cell {
			reg = <0 5>;
			data = [aa bb cc dd ff];
		};
	};

	scatter-gather-blob {
		compatible = "barebox,nvmem-sg";
		#address-cells = <1>;
		#size-cells = <1>;

		mac_address: combined-bits {
			reg = <0 6>;
			layout = <&ro_blob 0 2
  			          &ro_blob 1 1
			          &fec_mac_address 1 1
			          &ro_blob 2 1
			          &fec_mac_address 2 1>;
		};
	};

	&ocotp {
	       #address-cells = <1>;
	       	#size-cells = <1>;

		barebox,provide-mac-address = <&fec 0x620>;

		fec_mac_address: mac_address@88 {
			reg = <0x88 0x8>;
		};
	};

This should result in 'mac_address' variable that when read returns
[aa bb bb xx cc yy], where 'xx' and 'yy' are bytes at offsets 1 and 2
of OCOTP's register 0x88.

Please bear in mind that the code in commits ## 16, 17 is very raw and
clunky (a lot of error checking/handling code is omitted), which was
done for the cause of expediency in order to start this discussion
sooner and abandon this path if this design doesn't make sense.

As usual, any feedback is appreciated.

Regards,
Andrey Smirnov


Andrey Smirnov (17):
  base: driver.c: Coalesce error checking code
  [RFC] at91: Make IS_ERR work for I/O pointers
  [RFC] base/driver.c: Remove dev_request_mem_region_err_null
  i2c-at91: Use IS_ERR instead of checking for NULL
  clk-imx6: Call clk_enable on mmdc_ch0_axi_podf
  fec_imx: Deallocate clocks when probe fails
  [RFC] base: Introduce dev_request_mem_resource
  fec_imx: Deallocate I/O resources if probe fails
  fec_imx: Free phy_reset GPIO if when probe fails
  fec_imx: Use FEC_ECNTRL_RESET instead of a magic number
  fec_imx: Impelemnt reset timeout
  fec_imx: Deallocate DMA buffers when probe fails
  fec_imx: Unregister MDIO when probe fails
  [RFC] net: eth: Always use DEVICE_ID_DYNAMIC
  [RFC] nvmem: Add of_nvmem_cell_from_cell_np
  [RFC] nvmem: Add nvmem-ro-of-blob driver
  [RFC] nvmem: Add nvmem-sg driver

 arch/arm/mach-at91/at91sam926x_time.c |   6 +-
 arch/arm/mach-imx/clk-imx6.c          |   1 +
 drivers/base/driver.c                 |  25 ++-
 drivers/i2c/busses/i2c-at91.c         |   4 +-
 drivers/misc/Makefile                 |   2 +
 drivers/misc/nvmem-ro-of-blob.c       | 196 +++++++++++++++++++++++
 drivers/misc/nvmem-sg.c               | 287 ++++++++++++++++++++++++++++++++++
 drivers/net/fec_imx.c                 |  60 +++++--
 drivers/nvmem/core.c                  |  57 ++++---
 drivers/pinctrl/pinctrl-at91.c        |   6 +-
 drivers/serial/atmel.c                |   6 +-
 include/driver.h                      |   8 +-
 include/linux/nvmem-consumer.h        |  10 ++
 net/eth.c                             |   8 +-
 scripts/include/linux/err.h           |  66 ++++++++
 15 files changed, 670 insertions(+), 72 deletions(-)
 create mode 100644 drivers/misc/nvmem-ro-of-blob.c
 create mode 100644 drivers/misc/nvmem-sg.c

-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 01/18] common: Add EPROBE_DEFER to strerror

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/misc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/misc.c b/common/misc.c
index 8b2417b..f0f0b80 100644
--- a/common/misc.c
+++ b/common/misc.c
@@ -65,6 +65,7 @@ const char *strerror(int errnum)
 	case	ENETUNREACH	: str = "Network is unreachable"; break;
 	case	ENETDOWN	: str = "Network is down"; break;
 	case	ETIMEDOUT	: str = "Connection timed out"; break;
+	case	EPROBE_DEFER	: str = "Requested probe deferral"; break;
 #if 0 /* These are probably not needed */
 	case	ENOTBLK		: str = "Block device required"; break;
 	case	EFBIG		: str = "File too large"; break;
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 02/18] base: driver.c: Coalesce error checking code

[Andrey Smirnov]

Both dev_get_mem_region and dev_get_mem_region_by_name do exactly the
same thing with struct resource they obtain. Move that code into a
dedicated helper function.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index a70fbb2..2ef7ca9 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -325,15 +325,18 @@ struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
 	return ERR_PTR(-ENOENT);
 }
 
+static void *__start_or_err(const struct resource *res)
+{
+	return IS_ERR(res) ? ERR_CAST(res) : IOMEM(res->start);
+}
+
 void *dev_get_mem_region(struct device_d *dev, int num)
 {
 	struct resource *res;
 
 	res = dev_get_resource(dev, IORESOURCE_MEM, num);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
 
-	return (void __force *)res->start;
+	return __start_or_err(res);
 }
 EXPORT_SYMBOL(dev_get_mem_region);
 
@@ -361,10 +364,8 @@ void *dev_get_mem_region_by_name(struct device_d *dev, const char *name)
 	struct resource *res;
 
 	res = dev_get_resource_by_name(dev, IORESOURCE_MEM, name);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
 
-	return (void __force *)res->start;
+	return __start_or_err(res);
 }
 EXPORT_SYMBOL(dev_get_mem_region_by_name);
 
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 03/18] [RFC] at91: Make IS_ERR work for I/O pointers

[Andrey Smirnov]

Having this functionality partially "broken" opens the door for subtle
bugs in peripheral drivers for AT91 platform since it not straight out
obvious that IS_ERR might return a false positive.

It also makes it much harder to judge the correctness of the driver
code, for example it is perfectly fine to use IS_ERR in at91-i2c.c since
I2C controller's register file is located at 0xFFFA_C000 (which it
doesn't but that's the subject for another patch), however one couldn't
help but wonder how the code it sam9_smc.c could possibly work given how
that module is located at 0xFFFF_EC00.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 scripts/include/linux/err.h | 66 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/scripts/include/linux/err.h b/scripts/include/linux/err.h
index bdc3dd8..f6ce0d0 100644
--- a/scripts/include/linux/err.h
+++ b/scripts/include/linux/err.h
@@ -29,6 +29,69 @@
  */
 #define MAX_ERRNO	4095
 
+#ifdef CONFIG_ARCH_AT91
+
+/*
+ * AT91 maps all of its peripherals' register files into last 256MB of
+ * address space. This means that if no appropriate action is taken
+ * ERR_PTR et al. would not work. We also don't have the luxury of
+ * guaranted to be enabled MMU, so remapping is not an option. Instead
+ * we do a little bit of additional arithmetic and "move" all of the
+ * error codes to the page right before AT91's peripheral memory starts
+ * (0xEFFF_0000 to 0xEFFF_FFFF)
+ */
+
+static inline bool IS_ERR_VALUE(unsigned long x)
+{
+	return x >= 0xEFFF0000 &&
+	       x <= 0xEFFFFFFF;
+}
+
+static inline void * __must_check ERR_PTR(long error_)
+{
+	/*
+	 * We need to remap all errnos from 0xFFFF_0000 - 0xFFFF_FFFF
+	 * to 0xEFFF_0000 - 0xEFFF_FFFF
+	 *
+	 * Given that
+	 *
+	 * errno_ == 0xFFFF_FFFF - (abs(errno_) - 1)
+	 *
+	 * and what we want it to be is
+	 *
+	 * errno_ == 0xEFFF_FFFF - (abs(errno_) - 1)
+	 *
+	 * desired remapping can be acheived by the following code:
+	 */
+	unsigned long e = error_;
+
+	BUG_ON(error_ < -MAX_ERRNO);
+	/*
+	 * Since we know that e is not going to be smaller then
+	 * 0xFFFF_0000 instead of substracting 0x1000_0000 from 'e' we
+	 * can just "convert" most significant nibble of the value to
+	 * 'e' using bitwise and
+	 */
+	e &= 0xEFFFFFFF;
+
+	return (void *) e;
+}
+
+static inline long __must_check PTR_ERR(__force const void *ptr)
+{
+	unsigned long e = (unsigned long) error;
+
+	/*
+	 * This is the "inverse transformation" corresponding to the
+	 * code above
+	 */
+	e |= 0xF0000000;
+
+	return (long) e;
+}
+
+#else
+
 #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 
 static inline void * __must_check ERR_PTR(long error_)
@@ -41,6 +104,9 @@ static inline long __must_check PTR_ERR(__force const void *ptr)
 	return (long) ptr;
 }
 
+#endif
+
+
 static inline bool __must_check IS_ERR(__force const void *ptr)
 {
 	return IS_ERR_VALUE((unsigned long)ptr);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/18] [RFC] at91: Make IS_ERR work for I/O pointers

[Sascha Hauer]

On Tue, Feb 16, 2016 at 05:29:04PM -0800, Andrey Smirnov wrote:
> Having this functionality partially "broken" opens the door for subtle
> bugs in peripheral drivers for AT91 platform since it not straight out
> obvious that IS_ERR might return a false positive.
> 
> It also makes it much harder to judge the correctness of the driver
> code, for example it is perfectly fine to use IS_ERR in at91-i2c.c since
> I2C controller's register file is located at 0xFFFA_C000 (which it
> doesn't but that's the subject for another patch), however one couldn't
> help but wonder how the code it sam9_smc.c could possibly work given how
> that module is located at 0xFFFF_EC00.

0x100000000 - MAX_ERRNO is still bigger than 0xFFFFEC00, so this should
work.

The workaround you suggest is sooo ugly, do we really need this? How
many places are really affected by false positives in IS_ERR_VALUE?

Another solution, although a job for someone familiar with coccinelle
would be to change the prototype of dev_request_mem_region to

int dev_request_mem_region(struct device_d *dev, int num, void __iomem **base);

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/18] [RFC] at91: Make IS_ERR work for I/O pointers

[Andrey Smirnov]

On Wed, Feb 17, 2016 at 12:34 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Feb 16, 2016 at 05:29:04PM -0800, Andrey Smirnov wrote:
>> Having this functionality partially "broken" opens the door for subtle
>> bugs in peripheral drivers for AT91 platform since it not straight out
>> obvious that IS_ERR might return a false positive.
>>
>> It also makes it much harder to judge the correctness of the driver
>> code, for example it is perfectly fine to use IS_ERR in at91-i2c.c since
>> I2C controller's register file is located at 0xFFFA_C000 (which it
>> doesn't but that's the subject for another patch), however one couldn't
>> help but wonder how the code it sam9_smc.c could possibly work given how
>> that module is located at 0xFFFF_EC00.
>
> 0x100000000 - MAX_ERRNO is still bigger than 0xFFFFEC00, so this should
> work.
>
> The workaround you suggest is sooo ugly, do we really need this? How
> many places are really affected by false positives in IS_ERR_VALUE?

Well, to be fair, it's not like IS_ERR is anything but a clever hack,
so code fixing it can't really get any prettier than the code it's
fixing. ;-)

So I did a bunch of datasheet comparison and it looks like here's what
we have available on AT91 in worst case:

- errnos in [-1; -320) should be fine, that chunk of memory has no
peripherals and is marked as "reserved".
- errnos in [-320; 336) clash with RTC block on SAM9's
- errnos in [-400; -inf)  is reserved for various peripheral blocks.

There are 15 peripheral block located withing the last memory page (I
haven't checked how many of them has BB drivers)

Current implementation has two potential problems: false positives for
IS_ERR in drivers for aforementioned peripherals and also the fact
that buggy code that doesn't check pointers via IS_ERR could
potentially write bogus values to registers of actual peripheral
blocks.

Another solution for this problem would be to compact errno's. It
looks like out of all errnos(in errno.h) BB is using, only
ERESTARTSYS, ERESTARTNOINTR, ERESTARTNOHAND, ENOIOCTLCMD,
EPROBE_DEFER, ENOTSUPP fall outside of [-1;-320) range. Do you think
we can move those and define MAX_ERRNO as 320 for AT91 as a fix?

>
> Another solution, although a job for someone familiar with coccinelle
> would be to change the prototype of dev_request_mem_region to
>
> int dev_request_mem_region(struct device_d *dev, int num, void __iomem **base);

On a slightly related subject, If you are open to changing
dev_request_mem_region's prototype can we do the in patch #8 of the
series instead of introducing additional function?

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 03/18] [RFC] at91: Make IS_ERR work for I/O pointers

[Sascha Hauer]

On Wed, Feb 17, 2016 at 12:39:00PM -0800, Andrey Smirnov wrote:
> On Wed, Feb 17, 2016 at 12:34 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Tue, Feb 16, 2016 at 05:29:04PM -0800, Andrey Smirnov wrote:
> >> Having this functionality partially "broken" opens the door for subtle
> >> bugs in peripheral drivers for AT91 platform since it not straight out
> >> obvious that IS_ERR might return a false positive.
> >>
> >> It also makes it much harder to judge the correctness of the driver
> >> code, for example it is perfectly fine to use IS_ERR in at91-i2c.c since
> >> I2C controller's register file is located at 0xFFFA_C000 (which it
> >> doesn't but that's the subject for another patch), however one couldn't
> >> help but wonder how the code it sam9_smc.c could possibly work given how
> >> that module is located at 0xFFFF_EC00.
> >
> > 0x100000000 - MAX_ERRNO is still bigger than 0xFFFFEC00, so this should
> > work.
> >
> > The workaround you suggest is sooo ugly, do we really need this? How
> > many places are really affected by false positives in IS_ERR_VALUE?
> 
> Well, to be fair, it's not like IS_ERR is anything but a clever hack,
> so code fixing it can't really get any prettier than the code it's
> fixing. ;-)
> 
> So I did a bunch of datasheet comparison and it looks like here's what
> we have available on AT91 in worst case:
> 
> - errnos in [-1; -320) should be fine, that chunk of memory has no
> peripherals and is marked as "reserved".
> - errnos in [-320; 336) clash with RTC block on SAM9's
> - errnos in [-400; -inf)  is reserved for various peripheral blocks.
> 
> There are 15 peripheral block located withing the last memory page (I
> haven't checked how many of them has BB drivers)
> 
> Current implementation has two potential problems: false positives for
> IS_ERR in drivers for aforementioned peripherals and also the fact
> that buggy code that doesn't check pointers via IS_ERR could
> potentially write bogus values to registers of actual peripheral
> blocks.
> 
> Another solution for this problem would be to compact errno's. It
> looks like out of all errnos(in errno.h) BB is using, only
> ERESTARTSYS, ERESTARTNOINTR, ERESTARTNOHAND, ENOIOCTLCMD,
> EPROBE_DEFER, ENOTSUPP fall outside of [-1;-320) range. Do you think
> we can move those and define MAX_ERRNO as 320 for AT91 as a fix?

I'm not very fond of changing error codes, this probably ends in a mess
at least when Linux gets new error codes and we want to use them for
barebox aswell.

I just jumped into the cold water and created my first coccinelle
patches. So far it catches enough occurences of dev_request_mem_region
that the rest can be converted by hand in reasonable time.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 04/18] [RFC] base/driver.c: Remove dev_request_mem_region_err_null

[Andrey Smirnov]

Having functional IS_ERR on AT91 means we no longer need that function

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-at91/at91sam926x_time.c |  6 +++---
 drivers/base/driver.c                 | 16 ----------------
 drivers/pinctrl/pinctrl-at91.c        |  6 +++---
 drivers/serial/atmel.c                |  6 +++---
 include/driver.h                      |  8 --------
 5 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index cc7ad2f..789e1ec 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -89,9 +89,9 @@ static int at91_pit_probe(struct device_d *dev)
 		return ret;
 	}
 
-	pit_base = dev_request_mem_region_err_null(dev, 0);
-	if (!pit_base)
-		return -ENOENT;
+	pit_base = dev_request_mem_region(dev, 0);
+	if (IS_ERR(pit_base))
+		return PTR_ERR(pit_base);
 
 	pit_rate = clk_get_rate(clk) / 16;
 
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 2ef7ca9..a3ca057 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -385,22 +385,6 @@ void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *n
 }
 EXPORT_SYMBOL(dev_request_mem_region_by_name);
 
-void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
-{
-	struct resource *res;
-
-	res = dev_get_resource(dev, IORESOURCE_MEM, num);
-	if (IS_ERR(res))
-		return NULL;
-
-	res = request_iomem_region(dev_name(dev), res->start, res->end);
-	if (IS_ERR(res))
-		return NULL;
-
-	return IOMEM(res->start);
-}
-EXPORT_SYMBOL(dev_request_mem_region_err_null);
-
 void __iomem *dev_request_mem_region(struct device_d *dev, int num)
 {
 	struct resource *res;
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index ebbc6f6..816a3f3 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -652,9 +652,9 @@ static int at91_gpio_probe(struct device_d *dev)
 	}
 
 	gpio_banks = max(gpio_banks, alias_idx + 1);
-	at91_gpio->regbase = dev_request_mem_region_err_null(dev, 0);
-	if (!at91_gpio->regbase)
-		return -ENOENT;
+	at91_gpio->regbase = dev_request_mem_region(dev, 0);
+	if (IS_ERR(at91_gpio->regbase))
+		return PTR_ERR(at91_gpio->regbase);
 
 	at91_gpio->chip.ops = &at91_gpio_ops;
 	at91_gpio->chip.ngpio = MAX_NB_GPIO_PER_BANK;
diff --git a/drivers/serial/atmel.c b/drivers/serial/atmel.c
index 4e4624e..1f40692 100644
--- a/drivers/serial/atmel.c
+++ b/drivers/serial/atmel.c
@@ -398,9 +398,9 @@ static int atmel_serial_init_port(struct console_device *cdev)
 	struct device_d *dev = cdev->dev;
 	struct atmel_uart_port *uart = to_atmel_uart_port(cdev);
 
-	uart->base = dev_request_mem_region_err_null(dev, 0);
-	if (!uart->base)
-		return -ENOENT;
+	uart->base = dev_request_mem_region(dev, 0);
+	if (IS_ERR(uart->base))
+		return PTR_ERR(uart->base);
 
 	uart->clk = clk_get(dev, "usart");
 	clk_enable(uart->clk);
diff --git a/include/driver.h b/include/driver.h
index 31c6734..08adaf9 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -206,14 +206,6 @@ void *dev_get_mem_region(struct device_d *dev, int num);
  */
 void __iomem *dev_request_mem_region(struct device_d *dev, int num);
 
-/*
- * exlusively request register base 'num' for a device
- * will return NULL on error
- * only used on platform like at91 where the Ressource address collision with
- * PTR errno
- */
-void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num);
-
 struct device_d *device_alloc(const char *devname, int id);
 
 int device_add_resources(struct device_d *dev, const struct resource *res, int num);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 05/18] i2c-at91: Use IS_ERR instead of checking for NULL

[Andrey Smirnov]

Dev_request_mem_region doesn't return NULL to indicate error and IS_ERR
should be used in conjuction with it in order to detect failure.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/i2c/busses/i2c-at91.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 622c56d..6db7243 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -426,9 +426,9 @@ static int at91_twi_probe(struct device_d *dev)
 	i2c_at91->pdata = i2c_data;
 
 	i2c_at91->base = dev_request_mem_region(dev, 0);
-	if (!i2c_at91->base) {
+	if (IS_ERR(i2c_at91->base)) {
 		dev_err(dev, "could not get memory region\n");
-		rc = -ENODEV;
+		rc = PTR_ERR(i2c_at91->base);
 		goto out_free;
 	}
 
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 06/18] clk-imx6: Call clk_enable on mmdc_ch0_axi_podf

[Andrey Smirnov]

Call clk_enable on mmdc_ch0_axi_podf in order to properly increase
reference counters for all of the nodes in this particular clock
path. Otherwise it becomes possible for peripherals, located on other
branches stemming from "periph", to shut down the whole clock tree (up
to "pll2_bus") when they try to manage their own local clocks.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/clk-imx6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/clk-imx6.c b/arch/arm/mach-imx/clk-imx6.c
index 597e502..da8033e 100644
--- a/arch/arm/mach-imx/clk-imx6.c
+++ b/arch/arm/mach-imx/clk-imx6.c
@@ -501,6 +501,7 @@ static int imx6_ccm_probe(struct device_d *dev)
 	clk_data.clk_num = IMX6QDL_CLK_END;
 	of_clk_add_provider(dev->device_node, of_clk_src_onecell_get, &clk_data);
 
+	clk_enable(clks[IMX6QDL_CLK_MMDC_CH0_AXI_PODF]);
 	clk_enable(clks[IMX6QDL_CLK_PLL6_ENET]);
 	clk_enable(clks[IMX6QDL_CLK_SATA_REF_100M]);
 	clk_enable(clks[IMX6QDL_CLK_ENFC_PODF]);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 07/18] fec_imx: Deallocate clocks when probe fails

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 4465788..f413acd 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -710,7 +710,9 @@ static int fec_probe(struct device_d *dev)
 		goto err_free;
 	}
 
-	clk_enable(fec->clk);
+	ret = clk_enable(fec->clk);
+	if (ret < 0)
+		goto put_clk;
 
 	fec->regs = dev_request_mem_region(dev, 0);
 
@@ -720,11 +722,11 @@ static int fec_probe(struct device_d *dev)
 
 		ret = gpio_request(phy_reset, "phy-reset");
 		if (ret)
-			goto err_free;
+			goto disable_clk;
 
 		ret = gpio_direction_output(phy_reset, 0);
 		if (ret)
-			goto err_free;
+			goto disable_clk;
 
 		mdelay(msec);
 		gpio_set_value(phy_reset, 1);
@@ -764,7 +766,7 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	if (ret)
-		goto err_free;
+		goto disable_clk;
 
 	fec_init(edev);
 
@@ -784,6 +786,10 @@ static int fec_probe(struct device_d *dev)
 
 	return 0;
 
+disable_clk:
+	clk_disable(fec->clk);
+put_clk:
+	clk_put(fec->clk);
 err_free:
 	free(fec);
 	return ret;
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 08/18] [RFC] base: Introduce dev_request_mem_resource

[Andrey Smirnov]

Introduce dev_request_mem_resource as a version of
dev_request_mem_region that returns the corresponding 'struct resource'
and thus allows to perform proper resource deallocation. This is useful
for drivers that can experience deferred probling and need to be able to
be probed twice.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 12 ++++++++++++
 include/driver.h      |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index a3ca057..9e55061 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -385,6 +385,18 @@ void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *n
 }
 EXPORT_SYMBOL(dev_request_mem_region_by_name);
 
+struct resource *dev_request_mem_resource(struct device_d *dev, int num)
+{
+	struct resource *res;
+
+	res = dev_get_resource(dev, IORESOURCE_MEM, num);
+	if (IS_ERR(res))
+		return ERR_CAST(res);
+
+	return request_iomem_region(dev_name(dev), res->start, res->end);
+}
+EXPORT_SYMBOL(dev_request_mem_resource);
+
 void __iomem *dev_request_mem_region(struct device_d *dev, int num)
 {
 	struct resource *res;
diff --git a/include/driver.h b/include/driver.h
index 08adaf9..2e67223 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -206,6 +206,8 @@ void *dev_get_mem_region(struct device_d *dev, int num);
  */
 void __iomem *dev_request_mem_region(struct device_d *dev, int num);
 
+struct resource *dev_request_mem_resource(struct device_d *dev, int num);
+
 struct device_d *device_alloc(const char *devname, int id);
 
 int device_add_resources(struct device_d *dev, const struct resource *res, int num);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 09/18] fec_imx: Deallocate I/O resources if probe fails

[Andrey Smirnov]

Add a proper check for I/O memory resource allocation failure and
replace dev_request_mem_region with dev_request_mem_resource so it would
be possible to correctly deallocate device's I/O resources when probe
fails.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index f413acd..c059882 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -686,6 +686,7 @@ static int fec_probe(struct device_d *dev)
 	enum fec_type type;
 	int phy_reset;
 	u32 msec = 1;
+	struct resource *res;
 
 	ret = dev_get_drvdata(dev, (const void **)&type);
 	if (ret)
@@ -714,7 +715,12 @@ static int fec_probe(struct device_d *dev)
 	if (ret < 0)
 		goto put_clk;
 
-	fec->regs = dev_request_mem_region(dev, 0);
+	res = dev_request_mem_resource(dev, 0);
+	if (IS_ERR(res)) {
+		ret = PTR_ERR(res);
+		goto disable_clk;
+	}
+	fec->regs = IOMEM(res->start);
 
 	phy_reset = of_get_named_gpio(dev->device_node, "phy-reset-gpios", 0);
 	if (gpio_is_valid(phy_reset)) {
@@ -722,11 +728,11 @@ static int fec_probe(struct device_d *dev)
 
 		ret = gpio_request(phy_reset, "phy-reset");
 		if (ret)
-			goto disable_clk;
+			goto release_res;
 
 		ret = gpio_direction_output(phy_reset, 0);
 		if (ret)
-			goto disable_clk;
+			goto release_res;
 
 		mdelay(msec);
 		gpio_set_value(phy_reset, 1);
@@ -766,7 +772,7 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	if (ret)
-		goto disable_clk;
+		goto release_res;
 
 	fec_init(edev);
 
@@ -786,6 +792,8 @@ static int fec_probe(struct device_d *dev)
 
 	return 0;
 
+release_res:
+	release_region(res);
 disable_clk:
 	clk_disable(fec->clk);
 put_clk:
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 10/18] fec_imx: Free phy_reset GPIO if when probe fails

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index c059882..d132586 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -732,7 +732,7 @@ static int fec_probe(struct device_d *dev)
 
 		ret = gpio_direction_output(phy_reset, 0);
 		if (ret)
-			goto release_res;
+			goto free_gpio;
 
 		mdelay(msec);
 		gpio_set_value(phy_reset, 1);
@@ -772,7 +772,7 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	if (ret)
-		goto release_res;
+		goto free_gpio;
 
 	fec_init(edev);
 
@@ -792,6 +792,9 @@ static int fec_probe(struct device_d *dev)
 
 	return 0;
 
+free_gpio:
+	if (gpio_is_valid(phy_reset))
+		gpio_free(phy_reset);
 release_res:
 	release_region(res);
 disable_clk:
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 11/18] fec_imx: Use FEC_ECNTRL_RESET instead of a magic number

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index d132586..46932fe 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -740,7 +740,7 @@ static int fec_probe(struct device_d *dev)
 
 	/* Reset chip. */
 	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
-	while(readl(fec->regs + FEC_ECNTRL) & 1) {
+	while(readl(fec->regs + FEC_ECNTRL) & FEC_ECNTRL_RESET) {
 		udelay(10);
 	}
 
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 12/18] fec_imx: Impelemnt reset timeout

[Andrey Smirnov]

Don't wait for more than one second for IP block to finish resetting. If
the block is dead it makes more sence to continue execution in hopes
that the rest of the processor is fine, rather than spin indefinetly
inside of the fec_probe function

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 46932fe..99bd179 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -687,6 +687,7 @@ static int fec_probe(struct device_d *dev)
 	int phy_reset;
 	u32 msec = 1;
 	struct resource *res;
+	u64 start;
 
 	ret = dev_get_drvdata(dev, (const void **)&type);
 	if (ret)
@@ -739,9 +740,14 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	/* Reset chip. */
+	start = get_time_ns();
 	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
 	while(readl(fec->regs + FEC_ECNTRL) & FEC_ECNTRL_RESET) {
 		udelay(10);
+		if (is_timeout(start, SECOND)) {
+			ret = -ETIMEDOUT;
+			goto free_gpio;
+		}
 	}
 
 	/*
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 12/18] fec_imx: Impelemnt reset timeout

[Sascha Hauer]

On Tue, Feb 16, 2016 at 05:29:13PM -0800, Andrey Smirnov wrote:
> Don't wait for more than one second for IP block to finish resetting. If
> the block is dead it makes more sence to continue execution in hopes
> that the rest of the processor is fine, rather than spin indefinetly
> inside of the fec_probe function
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/net/fec_imx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
> index 46932fe..99bd179 100644
> --- a/drivers/net/fec_imx.c
> +++ b/drivers/net/fec_imx.c
> @@ -687,6 +687,7 @@ static int fec_probe(struct device_d *dev)
>  	int phy_reset;
>  	u32 msec = 1;
>  	struct resource *res;
> +	u64 start;
>  
>  	ret = dev_get_drvdata(dev, (const void **)&type);
>  	if (ret)
> @@ -739,9 +740,14 @@ static int fec_probe(struct device_d *dev)
>  	}
>  
>  	/* Reset chip. */
> +	start = get_time_ns();
>  	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
>  	while(readl(fec->regs + FEC_ECNTRL) & FEC_ECNTRL_RESET) {
>  		udelay(10);

While at it you can drop the udelay here.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 13/18] fec_imx: Deallocate DMA buffers when probe fails

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 99bd179..0877a53 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -621,6 +621,12 @@ static int fec_alloc_receive_packets(struct fec_priv *fec, int count, int size)
 	return 0;
 }
 
+static void fec_free_receive_packets(struct fec_priv *fec, int count, int size)
+{
+	void *p = phys_to_virt(fec->rbd_base[0].data_pointer);
+	dma_free_coherent(p, 0, size * count);
+}
+
 #include <linux/nvmem-consumer.h>
 
 static int nvmem_test(struct device_d *dev)
@@ -754,8 +760,9 @@ static int fec_probe(struct device_d *dev)
 	 * reserve memory for both buffer descriptor chains at once
 	 * Datasheet forces the startaddress of each chain is 16 byte aligned
 	 */
-	base = dma_alloc_coherent((2 + FEC_RBD_NUM) *
-			sizeof(struct buffer_descriptor), DMA_ADDRESS_BROKEN);
+#define FEC_XBD_SIZE ((2 + FEC_RBD_NUM) * sizeof(struct buffer_descriptor))
+
+	base = dma_alloc_coherent(FEC_XBD_SIZE, DMA_ADDRESS_BROKEN);
 	fec->rbd_base = base;
 	base += FEC_RBD_NUM * sizeof(struct buffer_descriptor);
 	fec->tbd_base = base;
@@ -763,7 +770,9 @@ static int fec_probe(struct device_d *dev)
 	writel(virt_to_phys(fec->tbd_base), fec->regs + FEC_ETDSR);
 	writel(virt_to_phys(fec->rbd_base), fec->regs + FEC_ERDSR);
 
-	fec_alloc_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
+	ret = fec_alloc_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
+	if (ret < 0)
+		goto free_xbd;
 
 	if (dev->device_node) {
 		ret = fec_probe_dt(dev, fec);
@@ -778,7 +787,7 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	if (ret)
-		goto free_gpio;
+		goto free_receive_packets;
 
 	fec_init(edev);
 
@@ -798,6 +807,10 @@ static int fec_probe(struct device_d *dev)
 
 	return 0;
 
+free_receive_packets:
+	fec_free_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
+free_xbd:
+	dma_free_coherent(fec->rbd_base, 0, FEC_XBD_SIZE);
 free_gpio:
 	if (gpio_is_valid(phy_reset))
 		gpio_free(phy_reset);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 14/18] fec_imx: Unregister MDIO when probe fails

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 0877a53..27d816f 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -799,14 +799,16 @@ static int fec_probe(struct device_d *dev)
 
 	ret = mdiobus_register(&fec->miibus);
 	if (ret)
-		return ret;
+		goto free_receive_packets;
 
 	ret = eth_register(edev);
 	if (ret)
-		return ret;
+		goto unregister_mdio;
 
 	return 0;
 
+unregister_mdio:
+	mdiobus_unregister(&fec->miibus);
 free_receive_packets:
 	fec_free_receive_packets(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE);
 free_xbd:
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 15/18] [RFC] net: eth: Always use DEVICE_ID_DYNAMIC

[Andrey Smirnov]

Assigning device IDs based on device tree aliases doesn't work very well
on platforms that have both NICs that are a part of a platform with
assigned aliases and NICs with DEVICE_ID_DYNAMIC policy of ID
assignement due to the nature of the interfaces they are connected
via (PCIe, USB, etc.). Consider the following scenario:

A device with built-in Ethernet adapter aliased to "ethernet0" and a
Ethernet chip connected via PCIe. This gives us two possible probing
orders:
  1.
    - built-in adapter comes up first gets assigned id of 0 and device
      "dev0"
    - PCIe adapter comes up second gets assigned id of 1 and device
      "dev1"
    - everything is hunky-dory

  2.
    - built-in adapter comes up first but its probing gets deffered
    - PCIe adapter comes up second gets assigned id of 0 and device
      "dev0"
    - built-in adapter gets probed the second time, sucessfuly
      initializes itself but fails to register Ethernet device because
      "dev0" is already taken

This patch solves the problem by forcing all Ethernet adapters to
use dynamic ID allocation.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 net/eth.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index fb3f22f..d028f71 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -366,13 +366,7 @@ int eth_register(struct eth_device *edev)
 	if (edev->parent)
 		edev->dev.parent = edev->parent;
 
-	if (edev->dev.parent && edev->dev.parent->device_node) {
-		edev->dev.id = of_alias_get_id(edev->dev.parent->device_node, "ethernet");
-		if (edev->dev.id < 0)
-			edev->dev.id = DEVICE_ID_DYNAMIC;
-	} else {
-		edev->dev.id = DEVICE_ID_DYNAMIC;
-	}
+	edev->dev.id = DEVICE_ID_DYNAMIC;
 
 	ret = register_device(&edev->dev);
 	if (ret)
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 15/18] [RFC] net: eth: Always use DEVICE_ID_DYNAMIC

[Trent Piepho]

On Tue, 2016-02-16 at 17:29 -0800, Andrey Smirnov wrote:
> Assigning device IDs based on device tree aliases doesn't work very well
> on platforms that have both NICs that are a part of a platform with
> assigned aliases and NICs with DEVICE_ID_DYNAMIC policy of ID
> assignement due to the nature of the interfaces they are connected
> via (PCIe, USB, etc.). Consider the following scenario:
> 
> A device with built-in Ethernet adapter aliased to "ethernet0" and a
> Ethernet chip connected via PCIe. This gives us two possible probing
> orders:
>   1.
>     - built-in adapter comes up first gets assigned id of 0 and device
>       "dev0"
>     - PCIe adapter comes up second gets assigned id of 1 and device
>       "dev1"
>     - everything is hunky-dory
> 
>   2.
>     - built-in adapter comes up first but its probing gets deffered
>     - PCIe adapter comes up second gets assigned id of 0 and device
>       "dev0"
>     - built-in adapter gets probed the second time, sucessfuly
>       initializes itself but fails to register Ethernet device because
>       "dev0" is already taken
> 
> This patch solves the problem by forcing all Ethernet adapters to
> use dynamic ID allocation.

A lot of systems depend on aliases/ethernet0 pointing to the proper
ethernet adapter.  How will they work with this?

How will a boot script know which ethernet device to use ethact on?

Suppose someone who doesn't know much about device trees and hasn't
looked at the SoC datasheet, which in my experience is about 80% of the
barebox userbase, comes up to you and says, "This port on the board is
labeled eth1, how do tell barebox to use it?"  Right now the answer is
"ethact eth1" but after this change, which basically eliminates the OF
alias system, the answer is????  I think it's going to be something that
said developer doesn't like very much.

Maybe a better solution would be to have dynamically allocated devices
not use IDs that have been "reserved" by the existence of an OF alias?
There wouldn't be some call to reserve an id, just the alias's existence
would be sufficient. Common code that allocates IDs should have all the
info it needs to check for an alias and then either use it or allocate
an id not already aliased.

While statically assigned ids are not nice in our dynamic discovery
software world of pointers, until one makes PCBs out of eInk displays
that dynamically update their silk screens, I think it's something we
still need to live with.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 15/18] [RFC] net: eth: Always use DEVICE_ID_DYNAMIC

[Andrey Smirnov]

On Tue, Feb 16, 2016 at 6:40 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Tue, 2016-02-16 at 17:29 -0800, Andrey Smirnov wrote:
>> Assigning device IDs based on device tree aliases doesn't work very well
>> on platforms that have both NICs that are a part of a platform with
>> assigned aliases and NICs with DEVICE_ID_DYNAMIC policy of ID
>> assignement due to the nature of the interfaces they are connected
>> via (PCIe, USB, etc.). Consider the following scenario:
>>
>> A device with built-in Ethernet adapter aliased to "ethernet0" and a
>> Ethernet chip connected via PCIe. This gives us two possible probing
>> orders:
>>   1.
>>     - built-in adapter comes up first gets assigned id of 0 and device
>>       "dev0"
>>     - PCIe adapter comes up second gets assigned id of 1 and device
>>       "dev1"
>>     - everything is hunky-dory
>>
>>   2.
>>     - built-in adapter comes up first but its probing gets deffered
>>     - PCIe adapter comes up second gets assigned id of 0 and device
>>       "dev0"
>>     - built-in adapter gets probed the second time, sucessfuly
>>       initializes itself but fails to register Ethernet device because
>>       "dev0" is already taken
>>
>> This patch solves the problem by forcing all Ethernet adapters to
>> use dynamic ID allocation.
>
> A lot of systems depend on aliases/ethernet0 pointing to the proper
> ethernet adapter.  How will they work with this?
>
> How will a boot script know which ethernet device to use ethact on?
>
> Suppose someone who doesn't know much about device trees and hasn't
> looked at the SoC datasheet, which in my experience is about 80% of the
> barebox userbase, comes up to you and says, "This port on the board is
> labeled eth1, how do tell barebox to use it?"  Right now the answer is
> "ethact eth1" but after this change, which basically eliminates the OF
> alias system, the answer is????  I think it's going to be something that
> said developer doesn't like very much.

I agree with your points and this commit isn't really a proper fix, I
put an RFC tag there to indicate that this patch was more of "Hey
guys, there's this problem, let's discuss how we solve it" rather than
"There's your problem, here's the fix".

>
> Maybe a better solution would be to have dynamically allocated devices
> not use IDs that have been "reserved" by the existence of an OF alias?
> There wouldn't be some call to reserve an id, just the alias's existence
> would be sufficient. Common code that allocates IDs should have all the
> info it needs to check for an alias and then either use it or allocate
> an id not already aliased.

It is a fix, but only partial. To play devil's advocate, imagine a
board that has SoC with two PCIe Ethernet chips (think i210) attached
to it. Even if you query for aliases you get back to the same problem
where your silkscreen labels are not providing any good info to inform
general user what to give to "ethact" (granted it is possible to
assigned a DT node to PCI device and thus probably possible to create
an aliases for such interfaces, but it is very clunky in practice not
many .dts files has any provisions like that)

I think the most sound fix for this problem would be to embed the
NIC's topology information into its name (not unlike Consistent
Network Device Naming in Linux), this way there should never be a
question which physical interface corresponds to which device in BB.
However I am not sure how feasible it is to implement something like
this in BB.

IMHO one simpler fix for this would be to treat aliases not just a
source of device id, but as a source of both device name and ID. This
way built in device would change from being "eth0" to "ethernet0" and
not clash with "eth0", however having both "ethernet0" and "eth0" on
the same system is less than ideal (an this still doesn't solve the
PCI/USB networking problem).

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 15/18] [RFC] net: eth: Always use DEVICE_ID_DYNAMIC

[Trent Piepho]

On Tue, 2016-02-16 at 20:16 -0800, Andrey Smirnov wrote:
> >> This patch solves the problem by forcing all Ethernet adapters to
> >> use dynamic ID allocation.
> >
> > A lot of systems depend on aliases/ethernet0 pointing to the proper
> > ethernet adapter.  How will they work with this?
> >
> > How will a boot script know which ethernet device to use ethact on?
> >
> > Suppose someone who doesn't know much about device trees and hasn't
> > looked at the SoC datasheet, which in my experience is about 80% of the
> > barebox userbase, comes up to you and says, "This port on the board is
> > labeled eth1, how do tell barebox to use it?"  Right now the answer is
> > "ethact eth1" but after this change, which basically eliminates the OF
> > alias system, the answer is????  I think it's going to be something that
> > said developer doesn't like very much.
> 
> I agree with your points and this commit isn't really a proper fix, I
> put an RFC tag there to indicate that this patch was more of "Hey
> guys, there's this problem, let's discuss how we solve it" rather than
> "There's your problem, here's the fix".
> 
> >
> > Maybe a better solution would be to have dynamically allocated devices
> > not use IDs that have been "reserved" by the existence of an OF alias?
> > There wouldn't be some call to reserve an id, just the alias's existence
> > would be sufficient. Common code that allocates IDs should have all the
> > info it needs to check for an alias and then either use it or allocate
> > an id not already aliased.
> 
> It is a fix, but only partial. To play devil's advocate, imagine a
> board that has SoC with two PCIe Ethernet chips (think i210) attached
> to it. Even if you query for aliases you get back to the same problem
> where your silkscreen labels are not providing any good info to inform
> general user what to give to "ethact" (granted it is possible to
> assigned a DT node to PCI device and thus probably possible to create
> an aliases for such interfaces, but it is very clunky in practice not
> many .dts files has any provisions like that)

Adding an alias for such a device seems reasonable to me.  My though
process goes something like this:

If the ports are a fixed part of the device, even if on a probe-able bus
like PCIe or USB, then we should be able to assign them a name.  They
are always there, always in the same place, always connected the same
way, so it's not impossible.

So we use what's constant, how the port is connected.  It will be a
certain device-function in a certain slot on a certain bus on a certain
pci controller.  And we make that the name because it's always the same.

But that name is huge and impossible to remember or type in.  So we
create a link, or alias, for it.  An easier to use name that points to
full name.

And thus we arrive at what is basically the OF alias system.


> I think the most sound fix for this problem would be to embed the
> NIC's topology information into its name (not unlike Consistent

Isn't that basically the same thing as the OF path of a device?

> Network Device Naming in Linux), this way there should never be a
> question which physical interface corresponds to which device in BB.
> However I am not sure how feasible it is to implement something like
> this in BB.
> 
> IMHO one simpler fix for this would be to treat aliases not just a
> source of device id, but as a source of both device name and ID. This
> way built in device would change from being "eth0" to "ethernet0" and
> not clash with "eth0", however having both "ethernet0" and "eth0" on
> the same system is less than ideal (an this still doesn't solve the
> PCI/USB networking problem).

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 15/18] [RFC] net: eth: Always use DEVICE_ID_DYNAMIC

[Andrey Smirnov]

On Thu, Feb 18, 2016 at 11:30 AM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Tue, 2016-02-16 at 20:16 -0800, Andrey Smirnov wrote:
>> >> This patch solves the problem by forcing all Ethernet adapters to
>> >> use dynamic ID allocation.
>> >
>> > A lot of systems depend on aliases/ethernet0 pointing to the proper
>> > ethernet adapter.  How will they work with this?
>> >
>> > How will a boot script know which ethernet device to use ethact on?
>> >
>> > Suppose someone who doesn't know much about device trees and hasn't
>> > looked at the SoC datasheet, which in my experience is about 80% of the
>> > barebox userbase, comes up to you and says, "This port on the board is
>> > labeled eth1, how do tell barebox to use it?"  Right now the answer is
>> > "ethact eth1" but after this change, which basically eliminates the OF
>> > alias system, the answer is????  I think it's going to be something that
>> > said developer doesn't like very much.
>>
>> I agree with your points and this commit isn't really a proper fix, I
>> put an RFC tag there to indicate that this patch was more of "Hey
>> guys, there's this problem, let's discuss how we solve it" rather than
>> "There's your problem, here's the fix".
>>
>> >
>> > Maybe a better solution would be to have dynamically allocated devices
>> > not use IDs that have been "reserved" by the existence of an OF alias?
>> > There wouldn't be some call to reserve an id, just the alias's existence
>> > would be sufficient. Common code that allocates IDs should have all the
>> > info it needs to check for an alias and then either use it or allocate
>> > an id not already aliased.
>>
>> It is a fix, but only partial. To play devil's advocate, imagine a
>> board that has SoC with two PCIe Ethernet chips (think i210) attached
>> to it. Even if you query for aliases you get back to the same problem
>> where your silkscreen labels are not providing any good info to inform
>> general user what to give to "ethact" (granted it is possible to
>> assigned a DT node to PCI device and thus probably possible to create
>> an aliases for such interfaces, but it is very clunky in practice not
>> many .dts files has any provisions like that)
>
> Adding an alias for such a device seems reasonable to me.  My though
> process goes something like this:
>
> If the ports are a fixed part of the device, even if on a probe-able bus
> like PCIe or USB, then we should be able to assign them a name.  They
> are always there, always in the same place, always connected the same
> way, so it's not impossible.
>
> So we use what's constant, how the port is connected.  It will be a
> certain device-function in a certain slot on a certain bus on a certain
> pci controller.  And we make that the name because it's always the same.
>
> But that name is huge and impossible to remember or type in.  So we
> create a link, or alias, for it.  An easier to use name that points to
> full name.

I don't necessarily agree with this point:

- IMHO, the amount if information needed to be encoded in the name to
cover 99% of the use-cases is not that impossible to type or remember,
compare e.g. "eth-pci-00-01", "eth-usb-00-01", "eth-spi-00-01" to
"eth0", "eth1"

- Even if we ignore the first point. IMHO, one's chances to remember
that "that one port to the left edge of the board" is "eth0" and "the
one to the right edge of the board" is "eth1" are the same as
remembering that former is "eth-pci-xx-yy" and latter is
"eth-spi-zz-zz", so the only way to make the process painless is to
have appropriate silkscreen and maybe labels on the case of the
device. At this point it doesn't make that much difference if it is
the "old" style name or a "new" one that's captured in those places.

>
> And thus we arrive at what is basically the OF alias system.
>

It's not that this can't "solve" this problem, but in my opinion (and
I could be completely wrong) the purpose of aliases in DT is to allow
flexibility in DT code and the fact it can be used to alter the
behavior of BB is somewhat of a serendipitous side-effect. What I
don't like about this solution is that it creates a dependency between
DT and BB's board's scripts that doesn't have to be there.

>
>> I think the most sound fix for this problem would be to embed the
>> NIC's topology information into its name (not unlike Consistent
>
> Isn't that basically the same thing as the OF path of a device?

Yes and no, it would be a rather abridged version of OF path that
omits a number of irrelevant details

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 16/18] [RFC] nvmem: Add of_nvmem_cell_from_cell_np

[Andrey Smirnov]

Add of_nvmem_cell_from_cell_np to allow creating 'struct nvmem_cell'
form a phandle of 'nvmem' cell.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/nvmem/core.c           | 57 ++++++++++++++++++++++++++----------------
 include/linux/nvmem-consumer.h | 10 ++++++++
 2 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 0214253..5f9b2ce 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -342,32 +342,16 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
 }
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
-/**
- * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
- *
- * @dev node: Device tree node that uses the nvmem cell
- * @id: nvmem cell name from nvmem-cell-names property.
- *
- * Return: Will be an ERR_PTR() on error or a valid pointer
- * to a struct nvmem_cell.  The nvmem_cell will be freed by the
- * nvmem_cell_put().
- */
-struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
-					    const char *name)
+
+
+struct nvmem_cell *
+of_nvmem_cell_from_cell_np(struct device_node *cell_np)
 {
-	struct device_node *cell_np, *nvmem_np;
+	struct device_node *nvmem_np;
 	struct nvmem_cell *cell;
 	struct nvmem_device *nvmem;
 	const __be32 *addr;
-	int rval, len, index;
-
-	index = of_property_match_string(np, "nvmem-cell-names", name);
-	if (index < 0)
-		return ERR_PTR(index);
-
-	cell_np = of_parse_phandle(np, "nvmem-cells", index);
-	if (!cell_np)
-		return ERR_PTR(-EINVAL);
+	int rval, len;
 
 	nvmem_np = of_get_parent(cell_np);
 	if (!nvmem_np)
@@ -429,6 +413,35 @@ err_mem:
 
 	return ERR_PTR(rval);
 }
+EXPORT_SYMBOL_GPL(of_nvmem_cell_from_cell_np);
+
+/**
+ * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
+ *
+ * @dev node: Device tree node that uses the nvmem cell
+ * @id: nvmem cell name from nvmem-cell-names property.
+ *
+ * Return: Will be an ERR_PTR() on error or a valid pointer
+ * to a struct nvmem_cell.  The nvmem_cell will be freed by the
+ * nvmem_cell_put().
+ */
+struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
+				     const char *name)
+{
+	struct device_node *cell_np;
+	int index;
+
+	index = of_property_match_string(np, "nvmem-cell-names", name);
+	if (index < 0)
+		return ERR_PTR(index);
+
+	cell_np = of_parse_phandle(np, "nvmem-cells", index);
+	if (!cell_np)
+		return ERR_PTR(-EINVAL);
+
+
+	return of_nvmem_cell_from_cell_np(cell_np);
+}
 EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
 #endif
 
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index cae6ec7..76d6138 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -136,11 +136,21 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
 #endif /* CONFIG_NVMEM */
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
+struct nvmem_cell *
+of_nvmem_cell_from_cell_np(struct device_node *cell_np);
+
 struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name);
 struct nvmem_device *of_nvmem_device_get(struct device_node *np,
 					 const char *name);
+
 #else
+struct nvmem_cell *
+of_nvmem_cell_from_cell_np(struct device_node *cell_np)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name)
 {
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 17/18] [RFC] nvmem: Add nvmem-ro-of-blob driver

[Andrey Smirnov]

Add a driver that implements provisions to allow creationg of fake nvmem
devices and cells whose data comes from device tree  blob.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/misc/Makefile           |   1 +
 drivers/misc/nvmem-ro-of-blob.c | 196 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 197 insertions(+)
 create mode 100644 drivers/misc/nvmem-ro-of-blob.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 94d2a4f..c7f56f8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_JTAG)		+= jtag.o
 obj-$(CONFIG_SRAM)		+= sram.o
 obj-$(CONFIG_STATE_DRV)		+= state.o
 obj-y += mac-address-map.o
+obj-y += nvmem-ro-of-blob.o
diff --git a/drivers/misc/nvmem-ro-of-blob.c b/drivers/misc/nvmem-ro-of-blob.c
new file mode 100644
index 0000000..8a9f091
--- /dev/null
+++ b/drivers/misc/nvmem-ro-of-blob.c
@@ -0,0 +1,196 @@
+#include <common.h>
+#include <driver.h>
+#include <init.h>
+#include <malloc.h>
+#include <of.h>
+#include <state.h>
+#include <io.h>
+#include <fcntl.h>
+#include <net.h>
+#include <regmap.h>
+
+#include <linux/err.h>
+#include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
+
+struct nvmem_ro_of_blob_cell {
+	struct list_head node;
+
+        unsigned int start;
+	unsigned int end;
+	unsigned int size;
+
+	u8 *data;
+};
+
+struct nvmem_ro_of_blob {
+	struct regmap *map;
+	struct nvmem_config config;
+	struct nvmem_device *nvmem;
+	struct list_head cells;
+};
+
+struct nvmem_ro_of_blob_cell *
+nvmem_ro_of_blob_reg_to_cell(struct nvmem_ro_of_blob *nrob,
+			     unsigned int reg)
+{
+	struct nvmem_ro_of_blob_cell *cell;
+
+	list_for_each_entry(cell, &nrob->cells, node) {
+		if (cell->start <= reg && reg <= cell->end)
+			return cell;
+	}
+
+	return NULL;
+}
+
+
+static int nvmem_ro_of_blob_reg_write(void *ctx, unsigned int reg,
+				      unsigned int val)
+{
+	return -ENOTSUPP;
+}
+
+static int nvmem_ro_of_blob_reg_read(void *ctx, unsigned int reg,
+				     unsigned int *val)
+{
+	struct nvmem_ro_of_blob *nrob = ctx;
+	struct nvmem_ro_of_blob_cell *cell =
+		nvmem_ro_of_blob_reg_to_cell(nrob, reg);
+
+	if (!cell)
+		return -ENOENT;
+
+	*val = cell->data[reg - cell->start];
+	return 0;
+}
+
+static const struct regmap_bus nvmem_ro_of_blob_regmap_bus = {
+	.reg_write = nvmem_ro_of_blob_reg_write,
+	.reg_read  = nvmem_ro_of_blob_reg_read,
+};
+
+static const struct regmap_config nvmem_ro_of_blob_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.reg_stride	= 1,
+	.max_register	= 0xbc,
+};
+
+static int nvmem_ro_of_blob_validate_dt(struct device_node *np)
+{
+	/* struct device_node *child; */
+	/* for_each_child_of_node(np, child) { */
+	/* 	const __be32 *addr; */
+	/* 	int len; */
+
+	/* 	addr = of_get_property(child, "reg", &len); */
+	/* 	if (!addr) */
+	/* 		return -EINVAL; */
+
+	/* } */
+
+	return 0;
+}
+
+static int nvmem_ro_of_blob_probe(struct device_d *dev)
+{
+	int ret;
+	struct list_head *p, *n;
+	struct device_node *np = dev->device_node;
+	struct device_node *child;
+
+	struct nvmem_ro_of_blob *nrob;
+	struct nvmem_ro_of_blob_cell *cell;
+
+	BUG_ON(!np);
+
+	if (!of_get_next_child(np, NULL)) {
+		dev_dbg(dev, "Device node doesn't specify any cells\n");
+		return -EINVAL;
+	}
+
+	ret = nvmem_ro_of_blob_validate_dt(np);
+	if (ret < 0) {
+		dev_dbg(dev, "Device tree validation failed\n");
+		return ret;
+	}
+
+	nrob = xzalloc(sizeof(*nrob));
+	INIT_LIST_HEAD(&nrob->cells);
+
+	for_each_child_of_node(np, child) {
+		struct property *pp;
+		const __be32 *addr;
+		int len;
+
+		addr = of_get_property(child, "reg", &len);
+		BUG_ON(!addr);
+		cell = xzalloc(sizeof(*cell));
+
+		cell->start = be32_to_cpup(addr++);
+		cell->size  = be32_to_cpup(addr);
+		cell->end   = cell->start + cell->size - 1;
+
+		pp = of_find_property(child, "data", NULL);
+		BUG_ON(!pp || pp->length != cell->size);
+		cell->data = pp->value;
+
+		list_add_tail(&cell->node, &nrob->cells);
+	}
+
+	nrob->map = regmap_init(dev,
+				&nvmem_ro_of_blob_regmap_bus,
+				nrob,
+				&nvmem_ro_of_blob_regmap_config);
+	if (IS_ERR(nrob->map)) {
+		dev_dbg(dev, "Failed to initilize regmap\n");
+		ret = PTR_ERR(nrob->map);
+		goto free_resources;
+	}
+
+	nrob->config.name = "nvmem-ro-of-blob";
+	nrob->config.read_only = true;
+	nrob->config.dev = dev;
+
+	nrob->nvmem = nvmem_register(&nrob->config, nrob->map);
+	if (IS_ERR(nrob->nvmem)) {
+		dev_dbg(dev, "Filed to register nvmem device\n");
+		ret = PTR_ERR(nrob->nvmem);
+		goto free_resources;
+	}
+
+	dev_dbg(dev, "probed\n");
+	return 0;
+
+free_resources:
+	if (!IS_ERR_OR_NULL(nrob->map))
+		regmap_exit(nrob->map);
+
+	list_for_each_safe(p, n, &nrob->cells) {
+		cell = list_entry(p,
+				  struct nvmem_ro_of_blob_cell,
+				  node);
+		list_del(p);
+		free(cell->data);
+		free(cell);
+	}
+	free(nrob);
+
+	return ret;
+}
+
+static __maybe_unused struct of_device_id nvmem_ro_of_blob_ids[] = {
+	{
+		.compatible = "barebox,nvmem-ro-of-blob",
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct driver_d nvmem_ro_of_blob_driver = {
+	.name = "nvmem-ro-of-blob",
+	.probe = nvmem_ro_of_blob_probe,
+	.of_compatible = DRV_OF_COMPAT(nvmem_ro_of_blob_ids),
+};
+device_platform_driver(nvmem_ro_of_blob_driver);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 17/18] [RFC] nvmem: Add nvmem-ro-of-blob driver

[Sascha Hauer]

On Tue, Feb 16, 2016 at 05:29:18PM -0800, Andrey Smirnov wrote:
> Add a driver that implements provisions to allow creationg of fake nvmem
> devices and cells whose data comes from device tree  blob.

This seems quite flexible. Care to create and post this for the kernel?
It would be much better if we can agree on a binding used by the kernel
aswell.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 17/18] [RFC] nvmem: Add nvmem-ro-of-blob driver

[Andrey Smirnov]

On Wed, Feb 17, 2016 at 12:52 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Feb 16, 2016 at 05:29:18PM -0800, Andrey Smirnov wrote:
>> Add a driver that implements provisions to allow creationg of fake nvmem
>> devices and cells whose data comes from device tree  blob.
>
> This seems quite flexible. Care to create and post this for the kernel?
> It would be much better if we can agree on a binding used by the kernel
> aswell.

Sure, sounds reasonable

>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 18/18] [RFC] nvmem: Add nvmem-sg driver

[Andrey Smirnov]

Add nvmem-sg driver which adds provisions to create fake 'nvmem' devices
whose cells are comprised of bits and pieces of other 'nvmem' cells in
a scatter-gather manner.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/misc/Makefile   |   1 +
 drivers/misc/nvmem-sg.c | 287 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 288 insertions(+)
 create mode 100644 drivers/misc/nvmem-sg.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c7f56f8..d048596 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_SRAM)		+= sram.o
 obj-$(CONFIG_STATE_DRV)		+= state.o
 obj-y += mac-address-map.o
 obj-y += nvmem-ro-of-blob.o
+obj-y += nvmem-sg.o
diff --git a/drivers/misc/nvmem-sg.c b/drivers/misc/nvmem-sg.c
new file mode 100644
index 0000000..7fd5eb4
--- /dev/null
+++ b/drivers/misc/nvmem-sg.c
@@ -0,0 +1,287 @@
+#include <common.h>
+#include <driver.h>
+#include <init.h>
+#include <malloc.h>
+#include <of.h>
+#include <state.h>
+#include <io.h>
+#include <fcntl.h>
+#include <net.h>
+#include <regmap.h>
+
+#include <linux/err.h>
+#include <linux/nvmem-provider.h>
+#include <linux/nvmem-consumer.h>
+
+struct nvmem_sg {
+	struct regmap *map;
+	struct nvmem_config config;
+	struct nvmem_device *nvmem;
+
+	struct list_head cells;
+};
+
+struct nvmem_sg_item {
+	struct nvmem_cell *cell;
+
+	size_t idx;
+	size_t start;
+	size_t end;
+	size_t size;
+
+	struct list_head node;
+};
+
+struct nvmem_sg_cell {
+	struct nvmem_sg *nsg;
+
+	size_t start;
+	size_t end;
+	size_t size;
+
+	struct list_head layout;
+	struct list_head node;
+};
+
+struct nvmem_sg_item *nvmem_sg_find_item(struct nvmem_sg_cell *cell,
+					 unsigned int nr)
+{
+	struct nvmem_sg_item *item;
+	list_for_each_entry(item, &cell->layout, node) {
+		pr_info("[%d %d]\n", item->start, item->end);
+		if (item->start <= nr && nr <= item->end)
+			return item;
+	}
+
+	return NULL;
+}
+
+
+static int nvmem_sg_cell_read_byte(struct nvmem_sg_cell *cell,
+				   unsigned int nr,
+				   unsigned int *val)
+{
+	unsigned int size;
+	uint8_t *data;
+	struct nvmem_sg_item *item = nvmem_sg_find_item(cell, nr);
+
+	if (!item)
+		return -EINVAL;
+
+	data = nvmem_cell_read(item->cell, &size);
+	if (IS_ERR(data)) {
+		dev_dbg(cell->nsg->config.dev,
+			"Failed to read a cell\n");
+		return PTR_ERR(data);
+	}
+
+	*val = data[nr - item->start + item->idx];
+	pr_info("nr: %d %x\n", nr, *val);
+
+	free(data);
+	return 0;
+}
+
+struct nvmem_sg_cell *nvmem_sg_find_cell(struct nvmem_sg *nsg,
+					 unsigned int reg)
+{
+	struct nvmem_sg_cell *cell;
+
+	list_for_each_entry(cell, &nsg->cells, node)
+		if (cell->start <= reg && reg <= cell->end)
+			return cell;
+
+	return NULL;
+}
+
+static int nvmem_sg_reg_write(void *ctx, unsigned int reg,
+			      unsigned int val)
+{
+	return -ENOTSUPP;
+}
+
+static int nvmem_sg_reg_read(void *ctx, unsigned int reg,
+			     unsigned int *val)
+{
+	struct nvmem_sg *nsg = ctx;
+	struct nvmem_sg_cell *cell = nvmem_sg_find_cell(nsg, reg);
+
+	if (cell) {
+		const unsigned int nr = reg - cell->start;
+		return nvmem_sg_cell_read_byte(cell, nr, val);
+	}
+
+	pr_debug("No sg cell found for reg %x\n", reg);
+	return -ENOENT;
+}
+
+static const struct regmap_bus nvmem_sg_regmap_bus = {
+	.reg_write = nvmem_sg_reg_write,
+	.reg_read  = nvmem_sg_reg_read,
+};
+
+static const struct regmap_config nvmem_sg_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_register = 0xbc,
+};
+
+static int nvmem_sg_validate_dt(struct device_node *np)
+{
+	/* FIXME */
+	return 0;
+}
+
+static int nvmem_sg_cell_populate(struct nvmem_sg_cell *sg_cell,
+				  struct device_node *sg_cell_np)
+{
+	int i;
+	int ret;
+	int len;
+	size_t start;
+
+	struct nvmem_sg_item *item = NULL;
+	const __be32 *addr = of_get_property(sg_cell_np, "layout", &len);
+	const unsigned int cell_count = len / (3 * sizeof(__be32));
+
+	BUG_ON(!addr);
+	BUG_ON(!cell_count);
+
+	start = 0;
+	for (i = 0; i < cell_count; i++) {
+		struct device_node *cell_np;
+		uint32_t phandle;
+
+		item = xzalloc(sizeof(*item));
+
+		phandle = be32_to_cpup(addr++);
+		cell_np = of_find_node_by_phandle(phandle);
+		if (!cell_np) {
+			dev_dbg(sg_cell->nsg->config.dev,
+				"No device tree node corresponds to phandle\n");
+			ret = -ENOENT;
+			goto unwind;
+		}
+
+		item->cell = of_nvmem_cell_from_cell_np(cell_np);
+		if (IS_ERR(item->cell)) {
+			dev_dbg(sg_cell->nsg->config.dev,
+				"Failed to instantiate nvmem cell from "
+				"a device tree node\n");
+			ret = PTR_ERR(item->cell);
+			goto unwind;
+		}
+
+		item->start = start;
+		item->idx  = be32_to_cpup(addr++);
+		item->size = be32_to_cpup(addr++);
+		item->end  = item->start + item->size - 1;
+		start += item->size;
+
+		list_add_tail(&item->node, &sg_cell->layout);
+	}
+
+	return 0;
+
+unwind:
+	free(item);
+	/* FIXME */
+	BUG();
+
+	return ret;
+}
+
+static int nvmem_sg_probe(struct device_d *dev)
+{
+	int ret;
+	struct device_node *np = dev->device_node;
+	struct device_node *child;
+
+	struct nvmem_sg *nsg;
+	struct nvmem_sg_cell *cell;
+
+	BUG_ON(!np);
+
+	if (!of_get_next_child(np, NULL)) {
+		dev_dbg(dev, "Device node doesn't specify any cells\n");
+		return -EINVAL;
+	}
+
+	ret = nvmem_sg_validate_dt(np);
+	if (ret < 0) {
+		dev_dbg(dev, "Device validation failed\n");
+		return ret;
+	}
+
+	nsg = xzalloc(sizeof(*nsg));
+	INIT_LIST_HEAD(&nsg->cells);
+
+	for_each_child_of_node(np, child) {
+		const __be32 *addr;
+		int len;
+
+		addr = of_get_property(child, "reg", &len);
+		BUG_ON(!addr);
+
+		cell = xzalloc(sizeof(*cell));
+		INIT_LIST_HEAD(&cell->layout);
+		cell->nsg   = nsg;
+		cell->start = be32_to_cpup(addr++);
+		cell->size  = be32_to_cpup(addr);
+		cell->end   = cell->start + cell->size - 1;
+
+		ret = nvmem_sg_cell_populate(cell, child);
+		if (ret < 0) {
+			dev_dbg(dev,
+				"Failed to popluate sg-cell\n");
+			goto free_resources;
+		}
+
+		list_add_tail(&cell->node, &nsg->cells);
+	}
+
+	nsg->map = regmap_init(dev,
+			       &nvmem_sg_regmap_bus,
+			       nsg,
+			       &nvmem_sg_regmap_config);
+	if (IS_ERR(nsg->map)) {
+		dev_dbg(dev, "Failed to register 'nvmem' device\n");
+		ret = PTR_ERR(nsg->map);
+		goto free_resources;
+	}
+
+	nsg->config.name = "nvmem-sg";
+	nsg->config.read_only = true;
+	nsg->config.dev = dev;
+
+	nsg->nvmem = nvmem_register(&nsg->config, nsg->map);
+	if (IS_ERR(nsg->nvmem)) {
+		dev_dbg(dev, "Failed to register 'nvmem' device\n");
+		ret = PTR_ERR(nsg->nvmem);
+		goto free_resources;
+	}
+
+	dev_dbg(dev, "probed\n");
+	return 0;
+
+free_resources:
+	/* FIXME */
+	BUG();
+	return ret;
+}
+
+static __maybe_unused struct of_device_id nvmem_sg_ids[] = {
+	{
+		.compatible = "barebox,nvmem-sg",
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct driver_d nvmem_sg_driver = {
+	.name = "nvmem-sg",
+	.probe = nvmem_sg_probe,
+	.of_compatible = DRV_OF_COMPAT(nvmem_sg_ids),
+};
+device_platform_driver(nvmem_sg_driver);
-- 
2.5.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/17] RFC for additional 'nvmem' plumbing

[Trent Piepho]

On Tue, 2016-02-16 at 17:29 -0800, Andrey Smirnov wrote:
> Hi everyone,
> 
> The main purpose of this patchset is propose additional Nvmem plubing
> needed for that subsytem's inclusion in BB and solicit feedback about
> it (patches ## 15 to 17) . Included in this set are also a number of
> bugfixes and RFC-for-bugfixes for problems that were discoverd during
> development of this patch series.
> 
> Proposed Nvmem additions are two new platform device drivers that
> implement a nvmem interface on top of data embedded in DT as well as
> nvmem interface that allows to combine existing nvmem-accesible bytes
> into continuous blocks.
> 
> Below is an example of DT code that uses both drivers in order to
> create a MAC address as a combination of bytes from DT and OCOTP
> module:

So for a common case Sasha has pointed out, of the MAC in the opposite
byte order, one would write:

	scatter-gather-blob {
		compatible = "barebox,nvmem-sg";
		#address-cells = <1>;
		#size-cells = <1>;

		mac_address: combined-bits {
			reg = <0 6>;
			layout = <&fec_mac_address 5 1
			          &fec_mac_address 4 1
			          &fec_mac_address 3 1
			          &fec_mac_address 2 1
			          &fec_mac_address 1 1
			          &fec_mac_address 0 1>;
		};
	};

	&ocotp {
	        #address-cells = <1>;
	       	#size-cells = <1>;

		fec_mac_address: mac_address@88 {
			reg = <0x88 0x8>;
		};
	};

        &fec {
		?mac-address-location?? = <&mac_address>;
	};

I suppose that's a very powerful system, but it does seem a bit
cumbersome.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/17] RFC for additional 'nvmem' plumbing

[Andrey Smirnov]

On Tue, Feb 16, 2016 at 7:09 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Tue, 2016-02-16 at 17:29 -0800, Andrey Smirnov wrote:
>> Hi everyone,
>>
>> The main purpose of this patchset is propose additional Nvmem plubing
>> needed for that subsytem's inclusion in BB and solicit feedback about
>> it (patches ## 15 to 17) . Included in this set are also a number of
>> bugfixes and RFC-for-bugfixes for problems that were discoverd during
>> development of this patch series.
>>
>> Proposed Nvmem additions are two new platform device drivers that
>> implement a nvmem interface on top of data embedded in DT as well as
>> nvmem interface that allows to combine existing nvmem-accesible bytes
>> into continuous blocks.
>>
>> Below is an example of DT code that uses both drivers in order to
>> create a MAC address as a combination of bytes from DT and OCOTP
>> module:
>
> So for a common case Sasha has pointed out, of the MAC in the opposite
> byte order, one would write:
>
>         scatter-gather-blob {
>                 compatible = "barebox,nvmem-sg";
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>
>                 mac_address: combined-bits {
>                         reg = <0 6>;
>                         layout = <&fec_mac_address 5 1
>                                   &fec_mac_address 4 1
>                                   &fec_mac_address 3 1
>                                   &fec_mac_address 2 1
>                                   &fec_mac_address 1 1
>                                   &fec_mac_address 0 1>;
>                 };
>         };
>
>         &ocotp {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>
>                 fec_mac_address: mac_address@88 {
>                         reg = <0x88 0x8>;
>                 };
>         };
>
>         &fec {
>                 ?mac-address-location?? = <&mac_address>;
>         };
>

mac_address is still a nvmem cell, so the DT code for FEC would remain the same:

&fec {
    nvmem-cells = <&mac_address>;
    nvmem-cell-names = "mac-address";
}


> I suppose that's a very powerful system, but it does seem a bit
> cumbersome.

I agree, those bindings don't really make me glow with pride because
of their elegance, but unfortunately I wasn't able to come up with
anything better that would be as generic and not tied into a
particular use case of MAC address manipulation.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox