ARM: tegra: Add PCIe support

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

	phys_addr_t start = 0;

	printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

Quoted 6 lines

	phys_addr_t start = 0;

	printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

The series,

Tested-by Stephen Warren <[email protected]>

Note that I did see the following printed a couple of times when I executed "run bootcmd_pxe":

pci_hose_bus_to_phys: invalid physical address

... but everything worked perfectly, so I guess we can track that down later.

Quoted 22 lines · Stephen Warren

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

Quoted 6 lines

	phys_addr_t start = 0;

	printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

The series,

Tested-by Stephen Warren <[email protected]>

Note that I did see the following printed a couple of times when I executed "run bootcmd_pxe":

Quoted 4 lines

pci_hose_bus_to_phys: invalid physical address

... but everything worked perfectly, so I guess we can track that down later.

Yes, it should definitely be tracked down. I don't see that message on my setup. I've seen it for example when noncached_alloc() fails and returns 0, but in that case everything shouldn't be working perfectly.

It would be helpful if that message showed what physical address was considered invalid.

Quoted 23 lines · Thierry Reding

Quoted 22 lines · Stephen Warren

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

Quoted 6 lines

	phys_addr_t start = 0;

	printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

The series,

Tested-by Stephen Warren <[email protected]>

Note that I did see the following printed a couple of times when I executed "run bootcmd_pxe":

Quoted 8 lines

Quoted 4 lines

pci_hose_bus_to_phys: invalid physical address

... but everything worked perfectly, so I guess we can track that down later.

Yes, it should definitely be tracked down. I don't see that message on my setup. I've seen it for example when noncached_alloc() fails and returns 0, but in that case everything shouldn't be working perfectly.

It looks like it happens when "dhcp" or "pxe get" attempt to download a file that doesn't exist on the server.

Quoted 2 lines

It would be helpful if that message showed what physical address was considered invalid.

The address is 0. I think this is because rtl_recv() is being called after rtl_halt(). rtl_halt() NULLs out all tpc->RxBufferRing[] entries, so calling rtl_recv() after that tries to convert address 0 to a PCI address. I haven't fully tracked this down, but it seems like a bug in the U-Boot networking or TFTP core, rather than anything to do with drivers for R8169 or the Tegra PCIe controller.

I'm not planning on tracking this down any further at the moment.

From: Thierry Reding <[email protected]>
Date: Thu, 28 Aug 2014 12:26:58 +0200
Subject: [PATCH] rtl8169: Defer network packet processing

When network protocol errors occur (such as a file not being found on a TFTP server), the processing done by the NetReceive() function will end up calling the driver's .halt() implementation. However, after that the device no longer has access to the memory buffers and will cause errors such as this in the rtl_recv() function when trying to hand descriptors back to the device:

pci_hose_bus_to_phys: invalid physical address

This can be fixed by deferring processing of network packets until the descriptors have been handed back. That way rtl_halt() tearing down network buffers is not going to prevent access to the buffers.

Reported-by Stephen Warren <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

        phys_addr_t start = 0;

        printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

Signed-off-by Thierry Reding <[email protected]>

---
 lib/vsprintf.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

lib/vsprintf.c +0 -0

index 7ec758e40fc5..044d5551bdd0 100644

lib/vsprintf.c +12 -3

        /*
         * Being a boot loader, we explicitly allow pointers to
         * (physical) address null.

@@ -530,6 +532,17 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 
 #ifdef CONFIG_CMD_NET
        switch (*fmt) {
+       case 'a':
+               flags |= SPECIAL | ZEROPAD;
+
+               switch (fmt[1]) {
+               case 'p':
+               default:
+                       field_width = sizeof(phys_addr_t) * 2 + 2;
+                       num = *(phys_addr_t *)ptr;
+                       break;
+               }
+               break;
        case 'm':
                flags |= SPECIAL;
                /* Fallthrough */

@@ -555,8 +568,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
                field_width = 2*sizeof(void *);
                flags |= ZEROPAD;
        }
-       return number(buf, end, (unsigned long)ptr, 16, field_width,
-                     precision, flags);
+       return number(buf, end, num, 16, field_width, precision, flags);
 }
 
 static int vsnprintf_internal(char *buf, size_t size, const char *fmt,
--

2.0.4

Hi Thierry,

Quoted 10 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

        phys_addr_t start = 0;

Quoted 4 lines

        printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

Would it be better to use %#pa to get the 0x prefix so we have the option? Hex is the default in U-Boot.

Quoted 5 lines

Signed-off-by Thierry Reding <[email protected]>

---
 lib/vsprintf.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

lib/vsprintf.c static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, · line 522

Quoted 10 lines

lib/vsprintf.c +2 -0

index 7ec758e40fc5..044d5551bdd0 100644
@@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width,
 static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
                int field_width, int precision, int flags)
 {
+       u64 num = (uintptr_t)ptr;
+

Will this impact code size much? I suppose it is vsprintf() so it doesn't matter too much?

Quoted 13 lines · Simon Glass

Hi Thierry,

Quoted 10 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Provide a new modifier to vsprintf() to print phys_addr_t variables to avoid having to cast or #ifdef when printing them out. The %pa modifier is used for this purpose, so phys_addr_t variables need to be passed by reference, like so:

        phys_addr_t start = 0;

Quoted 7 lines

Quoted 4 lines

        printf("start: %pa\n", &start);

Depending on the size of phys_addr_t this will print out the address with 8 or 16 hexadecimal digits following a 0x prefix.

Would it be better to use %#pa to get the 0x prefix so we have the option? Hex is the default in U-Boot.

Yeah, that's very confusing since there's no way to tell apart a hexadecimal number from a decimal one since there's no decimal equivalent of the 0x prefix.

If you insist I can change that (it should be a simple matter of dropping the SPECIAL flag in the 'a' case).

Quoted 6 lines

Quoted 6 lines

Signed-off-by Thierry Reding <[email protected]>

---
 lib/vsprintf.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Quoted 13 lines

lib/vsprintf.c static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, · line 522

Quoted 10 lines

lib/vsprintf.c +2 -0

index 7ec758e40fc5..044d5551bdd0 100644
@@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width,
 static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
                int field_width, int precision, int flags)
 {
+       u64 num = (uintptr_t)ptr;
+

Will this impact code size much? I suppose it is vsprintf() so it doesn't matter too much?

Doing only this change and the corresponding change in the call to number() results in the exact sizes:

        $ size build/jetson-tk1.{before,after}/u-boot
           text    data     bss     dec     hex filename
         310434   10872  309184  630490   99eda build/jetson-tk1.before/u-boot
         310434   10872  309184  630490   99eda build/jetson-tk1.after/u-boot

Given that the num parameter of the number() function is already a u64 the compiler will presumably automatically handle the ptr argument as a 64-bit value.

Thierry

From: Thierry Reding <[email protected]>

Given a device tree node and a property name, the fdt_count_strings() function counts the number of strings found in the property value.

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding <[email protected]>

Given a device tree node and a property name, the new fdt_find_string() function will look up a given string in the string list contained in the property's value and return its index.

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding <[email protected]>

Given a device tree node, a property name and an index, the new function fdt_get_string_index() will return in an output argument a pointer to the index'th string in the property's value.

The fdt_get_string() is a shortcut for the above with the index being 0.

Signed-off-by Thierry Reding <[email protected]>

Applied to u-boot-fdt/next, thanks!

Hi Thierry,

Quoted 1 line · Simon Glass

Applied to u-boot-fdt/next, thanks!

Did you submit these patches to dtc upstream? I don't see them applied.

+Scott, Masahiro

Hi Thierry,

Quoted 2 lines · Simon Glass

Hi Thierry,

Quoted 4 lines · Simon Glass

Quoted 1 line

Applied to u-boot-fdt/next, thanks!

Did you submit these patches to dtc upstream? I don't see them applied.

Ping? Masahiro has now sent a series on top of your patches, so we need to sort this out.

Quoted 12 lines · Simon Glass

+Scott, Masahiro

Hi Thierry,

Quoted 6 lines · Simon Glass

Hi Thierry,

Quoted 1 line · Simon Glass

Applied to u-boot-fdt/next, thanks!

Did you submit these patches to dtc upstream? I don't see them applied.

Quoted 2 lines

Ping? Masahiro has now sent a series on top of your patches, so we need to sort this out.

I sent these to the DTC maintainers and the list, with you and Masahiro in Cc. They patches should be eqivalent to what's in U-Boot except for the node parameter that I renamed nodeoffset for consistency with libfdt and the testsuite bits.

Thierry

Hello Thierry,

Quoted 16 lines · Thierry Reding

Quoted 14 lines · Simon Glass

+Scott, Masahiro

Hi Thierry,

Quoted 6 lines · Simon Glass

Hi Thierry,

Quoted 1 line · Simon Glass

Applied to u-boot-fdt/next, thanks!

Did you submit these patches to dtc upstream? I don't see them applied.

Ping? Masahiro has now sent a series on top of your patches, so we need to sort this out.

Quoted 4 lines

I sent these to the DTC maintainers and the list, with you and Masahiro in Cc. They patches should be eqivalent to what's in U-Boot except for the node parameter that I renamed nodeoffset for consistency with libfdt and the testsuite bits.

Do you mean there are intrinsic incompatibilities between U-Boot and Linux that make it impossible to use the same name in both?

From: Thierry Reding <[email protected]>

Add the fdt_get_resource() and fdt_get_named_resource() functions which can be used to parse resources (memory regions) from an FDT. A helper to compute the size of a region is also provided.

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding<[email protected]>

i2c_bus_init() takes a bus number but relies on the currently selected bus to determine which adapter to initialize. Make the function use the bus passed in as parameter rather than the currently selected bus. While at it, keep a pointer to the specified bus to avoid having to look it up repeatedly.

Signed-off-by Thierry Reding<[email protected]>

---
  drivers/i2c/i2c_core.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

drivers/i2c/i2c_core.c +9 -6

index 18d6736601c1..cca455bc9c63 100644
@@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void)
   * Initializes one bus. Will initialize the parent adapter. No current bus
   * changes, no mux (if any) setup.
   */
-static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr)
+static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr)
  {
-	if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES)
+	struct i2c_adapter *adapter;
+
+	if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
  		return;
 
-	I2C_ADAP->init(I2C_ADAP, speed, slaveaddr);
+	adapter = i2c_get_adapter(I2C_ADAPTER(bus));
+	adapter->init(adapter, speed, slaveaddr);
 
  	if (gd->flags&  GD_FLG_RELOC) {
-		I2C_ADAP->init_done = 1;
-		I2C_ADAP->speed = speed;
-		I2C_ADAP->slaveaddr = slaveaddr;
+		adapter->init_done = 1;
+		adapter->speed = speed;
+		adapter->slaveaddr = slaveaddr;
  	}
  }
 

Hello Thierry,

Am 26.08.2014 17:33, schrieb Thierry Reding:

Quoted 8 lines

From: Thierry Reding<[email protected]>

i2c_bus_init() takes a bus number but relies on the currently selected bus to determine which adapter to initialize. Make the function use the bus passed in as parameter rather than the currently selected bus. While at it, keep a pointer to the specified bus to avoid having to look it up repeatedly.

Quoted 4 lines

Signed-off-by Thierry Reding<[email protected]>

---
 drivers/i2c/i2c_core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Why you could not use the current CONFIG_SYS_I2C API and init a bus with i2c_set_bus_num()?

Quoted 21 lines

drivers/i2c/i2c_core.c +6 -3

index 18d6736601c1..cca455bc9c63 100644
@@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void)
  * Initializes one bus. Will initialize the parent adapter. No current bus
  * changes, no mux (if any) setup.
  */
-static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr)
+static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr)
 {
-	if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES)
+	struct i2c_adapter *adapter;
+
+	if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
 		return;
 
-	I2C_ADAP->init(I2C_ADAP, speed, slaveaddr);
+	adapter = i2c_get_adapter(I2C_ADAPTER(bus));
+	adapter->init(adapter, speed, slaveaddr);
 

Quoted 10 lines

drivers/i2c/i2c_core.c +3 -3

 	if (gd->flags&  GD_FLG_RELOC) {
-		I2C_ADAP->init_done = 1;
-		I2C_ADAP->speed = speed;
-		I2C_ADAP->slaveaddr = slaveaddr;
+		adapter->init_done = 1;
+		adapter->speed = speed;
+		adapter->slaveaddr = slaveaddr;
 	}
 }
 

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Quoted 11 lines

Hello Thierry,

Am 26.08.2014 17:33, schrieb Thierry Reding:

Quoted 8 lines

From: Thierry Reding<[email protected]>

i2c_bus_init() takes a bus number but relies on the currently selected bus to determine which adapter to initialize. Make the function use the bus passed in as parameter rather than the currently selected bus. While at it, keep a pointer to the specified bus to avoid having to look it up repeatedly.

Quoted 7 lines

Quoted 4 lines

Signed-off-by Thierry Reding<[email protected]>

---
  drivers/i2c/i2c_core.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

Why you could not use the current CONFIG_SYS_I2C API and init a bus with i2c_set_bus_num()?

That's orthogonal to this patch. i2c_set_bus_num() will end up calling the i2c_init_bus() function, too. What this patch does is fix an issue where i2c_init_bus is completely ignoring the bus_no parameter (except for sanity checking) but instead relies on the gd->cur_i2c_bus (via I2C_ADAP) to initialize a bus. That's completely unexpected and making this consistent allows the function to be reused in a more generic way as done in subsequent patches.

Thierry

Quoted 35 lines

Quoted 21 lines

drivers/i2c/i2c_core.c +6 -3

index 18d6736601c1..cca455bc9c63 100644
@@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void)
   * Initializes one bus. Will initialize the parent adapter. No current bus
   * changes, no mux (if any) setup.
   */
-static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr)
+static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr)
  {
-	if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES)
+	struct i2c_adapter *adapter;
+
+	if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
  		return;
 
-	I2C_ADAP->init(I2C_ADAP, speed, slaveaddr);
+	adapter = i2c_get_adapter(I2C_ADAPTER(bus));
+	adapter->init(adapter, speed, slaveaddr);
 

drivers/i2c/i2c_core.c static int i2c_mux_disconnet_all(void) · line 233

Quoted 10 lines

drivers/i2c/i2c_core.c +3 -3

  	if (gd->flags&   GD_FLG_RELOC) {
-		I2C_ADAP->init_done = 1;
-		I2C_ADAP->speed = speed;
-		I2C_ADAP->slaveaddr = slaveaddr;
+		adapter->init_done = 1;
+		adapter->speed = speed;
+		adapter->slaveaddr = slaveaddr;
  	}
  }
 

--
DENX Software Engineering GmbH, MD: Wolfgang Denk& Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Drivers for I2C devices are supposed to embed a struct i2c_client within a driver-specific data structure and call i2c_client_init() on it, passing in a pointer to the parent I2C adapter and the slave address of the device.

Signed-off-by Thierry Reding<[email protected]>

---
  drivers/i2c/i2c_core.c | 53 ++++++++++++++++++++++++++++
  include/i2c.h          | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 149 insertions(+)

drivers/i2c/i2c_core.c +53 -0

index f6179a16244b..88c7af546b0b 100644
@@ -410,3 +410,56 @@ void __i2c_init(int speed, int slaveaddr)
  }
  void i2c_init(int speed, int slaveaddr)
  	__attribute__((weak, alias("__i2c_init")));
+
+struct i2c_adapter *i2c_adapter_get(unsigned int index)
+{
+	struct i2c_adapter *adapter = ll_entry_start(struct i2c_adapter, i2c);
+	unsigned int num = ll_entry_count(struct i2c_adapter, i2c);
+	unsigned int i;
+
+	if (index>= num)
+		return NULL;
+
+	for (i = 0; i<  index; i++)
+		adapter++;
+
+	i2c_adapter_init(adapter, adapter->speed, adapter->slaveaddr);
+	return adapter;
+}
+
+int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip,
+		     unsigned int address, size_t alen, void *buffer,
+		     size_t size)
+{
+	return adapter->read(adapter, chip, address, alen, buffer, size);
+}
+
+int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip,
+		      unsigned int address, size_t alen, void *buffer,
+		      size_t size)
+{
+	return adapter->write(adapter, chip, address, alen, buffer, size);
+}
+
+int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
+		    uint8_t address)
+{
+	client->adapter = adapter;
+	client->address = address;
+
+	return 0;
+}
+
+int i2c_client_read(struct i2c_client *client, unsigned int address,
+		    size_t alen, void *buffer, size_t size)
+{
+	return i2c_adapter_read(client->adapter, client->address, address,
+				alen, buffer, size);
+}
+
+int i2c_client_write(struct i2c_client *client, unsigned int address,
+		     size_t alen, void *buffer, size_t size)
+{
+	return i2c_adapter_write(client->adapter, client->address, address,
+				 alen, buffer, size);
+}

include/i2c.h +96 -0

index 1b4078ed62fe..8f73ba93c614 100644
@@ -55,6 +55,20 @@
  #define CONFIG_SYS_SPD_BUS_NUM		0
  #endif
 
+/**
+ * struct i2c_adapter - I2C adapter
+ * @init: initialize I2C adapter
+ * @probe: probe for a device on the I2C bus
+ * @read: read data from a device on the I2C bus
+ * @write: write data to a device on the I2C bus
+ * @set_bus_speed: configure the I2C bus speed
+ * @speed: current I2C bus speed
+ * @waitdelay:
+ * @slaveaddr: own address (when used as a slave)
+ * @init_done: flag to indicate whether or not the adapter has been initialized
+ * @hwadapnr: the hardware number of this adapter
+ * @name: name of this adapter
+ */
  struct i2c_adapter {
  	void		(*init)(struct i2c_adapter *adap, int speed,
  				int slaveaddr);

@@ -75,6 +89,16 @@ struct i2c_adapter {
  	char		*name;
  };
 
+/**
+ * struct i2c_client - I2C slave
+ * @adapter: I2C adapter providing the slave's parent bus
+ * address: address of the I2C slave on the parent bus
+ */
+struct i2c_client {
+	struct i2c_adapter *adapter;
+	unsigned int address;
+};
+
  #define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \
  		_set_speed, _speed, _slaveaddr, _hwadapnr, _name) \
  	{ \

@@ -271,6 +295,78 @@ unsigned int i2c_get_bus_speed(void);
   * Adjusts I2C pointers after U-Boot is relocated to DRAM
   */
  void i2c_reloc_fixup(void);
+
+/*
+ * i2c_adapter_get() - get the I2C adapter associated with a given index
+ * @index: index of the I2C adapter
+ */
+struct i2c_adapter *i2c_adapter_get(unsigned int index);
+
+/*
+ * i2c_adapter_read() - read data from an I2C slave at a given address
+ * @adapter: I2C adapter
+ * @chip: address of the I2C slave to read from
+ * @address: address within the I2C slave to read from
+ * @alen: length of address
+ * @buffer: buffer to receive data from I2C slave
+ * @size: number of bytes to read
+ */
+int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip,
+		     unsigned int address, size_t alen, void *buffer,
+		     size_t size);
+
+/*
+ * i2c_adapter_write() - write data to an I2C slave at a given address
+ * @adapter: I2C adapter
+ * @chip: address of the I2C slave to write to
+ * @address: address within the I2C slave to write to
+ * @alen: length of address
+ * @buffer: buffer containing the data to write
+ * @size: number of bytes to write
+ *
+ * Ideally the function would take a const void * buffer, but the underlying
+ * infrastructure doesn't properly propagate const and adding it here would
+ * cause a lot of build warnings.
+ */
+int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip,
+		      unsigned int address, size_t alen, void *buffer,
+		      size_t size);
+
+/*
+ * i2c_client_init() - initialize an I2C slave
+ * @client: I2C slave
+ * @adapter: parent I2C adapter
+ * @address: address of I2C slave
+ */
+int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
+		    uint8_t address);
+
+/*
+ * i2c_client_read() - read data from an I2C slave
+ * @client: I2C slave
+ * @address: address within the I2C slave to read from
+ * @alen: length of address
+ * @buffer: buffer to receive data from I2C slave
+ * @size: number of bytes to read
+ */
+int i2c_client_read(struct i2c_client *client, unsigned int address,
+		    size_t alen, void *buffer, size_t size);
+
+/*
+ * i2c_client_write() - write data to an I2C slave
+ * @client: I2C slave
+ * @address: address within the I2C slave to write to
+ * @alen: length of address
+ * @buffer: buffer containing the data to write
+ * @size: number of bytes to write
+ *
+ * Ideally the function would take a const void * buffer, but the underlying
+ * infrastructure doesn't properly propagate const and adding it here would
+ * cause a lot of build warnings.
+ */
+int i2c_client_write(struct i2c_client *client, unsigned int address,
+		     size_t alen, void *buffer, size_t size);
+
  #if defined(CONFIG_SYS_I2C_SOFT)
  void i2c_soft_init(void);
  void i2c_soft_active(void);

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

278 int i2c_set_bus_num(unsigned int bus)
279 {
280 int max;
281
282 if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
283 return 0;

And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes?

Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is an eeprom, on the other is a PMIC ... your code in patch "power: Add AMS AS3722 PMIC support" does access with your functions the PMIC ... what is, if between this accesses someone accesses the eeprom? If he switches the mux, you never switch back!

Your code did not check this!

Why is i2c_set_bus_num() such a problem?

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

doesn't run multithreaded this works. If you're really concerned about

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

this isn't going to work (in a multithreaded environment the switch to a different mux could happen between the call to i2c_set_bus_num() and the bus access).

In fact I think this would even have to be solved at the controller level if you want to make sure that client transactions are atomic.

Quoted 13 lines

This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

  278 int i2c_set_bus_num(unsigned int bus)
  279 {
  280         int max;
  281
  282         if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
  283                 return 0;

Quoted 2 lines

And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes?

That's complexity that users shouldn't have to worry about. They should

simply access an adapter and the adapter (or rather the core) should take care of setting up any muxes correctly.

Quoted 7 lines

Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is an eeprom, on the other is a PMIC ... your code in patch "power: Add AMS AS3722 PMIC support" does access with your functions the PMIC ... what is, if between this accesses someone accesses the eeprom? If he switches the mux, you never switch back!

Your code did not check this!

Like I said, a lot of code in U-Boot doesn't check this. And quite

frankly as long as this isn't handled in the core I don't think people will get it right.

Quoted 1 line

Why is i2c_set_bus_num() such a problem?

Because it's completely confusing. And it's exposing an implementation detail to users instead of handling it transparently in the core.

Hello Thierry,

Am 27.08.2014 08:21, schrieb Thierry Reding:

Quoted 14 lines · Heiko Schocher

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 6 lines

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

Yes, sadly. This has historical reasons ...

Quoted 1 line

doesn't run multithreaded this works. If you're really concerned about

Yes, U-Boot is singlethread only.

Quoted 3 lines

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!

But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write?

- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ...

I know, this is a big change and a lot of work ... thats the reason why we are not at this point ... nobody volunteered to go forward, and I did not found time to do it ...

Quoted 6 lines

this isn't going to work (in a multithreaded environment the switch to a different mux could happen between the call to i2c_set_bus_num() and the bus access).

In fact I think this would even have to be solved at the controller level if you want to make sure that client transactions are atomic.

As U-Boot is single threaded all i2c_read/writes are atomic.

Quoted 13 lines

Quoted 13 lines

This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

278 int i2c_set_bus_num(unsigned int bus)
279 {
280 int max;
281
282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))
283 return 0;

Quoted 4 lines

Quoted 2 lines

And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes?

That's complexity that users shouldn't have to worry about. They should

Exactly!

Quoted 2 lines

simply access an adapter and the adapter (or rather the core) should take care of setting up any muxes correctly.

Yes!

I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter maybe with m muxes, and each bus has exactly one way through the i2c muxes, see for an example the README:

http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349

So the only thing a User must know when he wants to use an i2c bus is his number. The switching to this i2c adapter, initializing it and maybe set i2c muxes does the i2c subsystem ...

Quoted 8 lines

Quoted 7 lines

Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is an eeprom, on the other is a PMIC ... your code in patch "power: Add AMS AS3722 PMIC support" does access with your functions the PMIC ... what is, if between this accesses someone accesses the eeprom? If he switches the mux, you never switch back!

Your code did not check this!

Quoted 1 line

Like I said, a lot of code in U-Boot doesn't check this. And quite

With using i2c_set_bus_num() you have not to check this! You only have to call i2c_set_bus_num() before calling i2c_read/write ... and yes, that would be nice, if we just pass the bus number to i2c_read/write() and drop the i2c_set_bus_num() call all over the code ...

Patches welcome!

Quoted 2 lines

frankly as long as this isn't handled in the core I don't think people will get it right.

Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c driver are converted to it, we can make i2c_set_bus_num() static, and add to the i2c API the bus number as a function parameter!

Quoted 4 lines

Quoted 1 line

Why is i2c_set_bus_num() such a problem?

Because it's completely confusing. And it's exposing an implementation detail to users instead of handling it transparently in the core.

Yes! Full Ack ... but I do not accept a new API for that! Please fix the i2c_read/write() functions!

Quoted 39 lines

Hello Thierry,

Am 27.08.2014 08:21, schrieb Thierry Reding:

Quoted 14 lines · Heiko Schocher

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 6 lines

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

Yes, sadly. This has historical reasons ...

Quoted 1 line

doesn't run multithreaded this works. If you're really concerned about

Yes, U-Boot is singlethread only.

Quoted 3 lines

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!

But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write?

Quoted 4 lines

- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ...

I don't think adding a bus number as parameter is useful. Why not just use the I2C adapter directly? That way we don't have to keep looking it up in an array every time.

Quoted 3 lines

I know, this is a big change and a lot of work ... thats the reason why we are not at this point ... nobody volunteered to go forward, and I did not found time to do it ...

I suppose that would be one possibility to do it. But I consider i2c_client more of a convenience around the lower-level i2c_read() and i2c_write(). The idea is that users set up an I2C client once and then refer to the client, which will automatically use the correct adapter and slave address rather than having that duplicated in every driver.

Quoted 4 lines

Quoted 4 lines

this isn't going to work (in a multithreaded environment the switch to a different mux could happen between the call to i2c_set_bus_num() and the bus access).

Quoted 4 lines

Quoted 2 lines

In fact I think this would even have to be solved at the controller level if you want to make sure that client transactions are atomic.

As U-Boot is single threaded all i2c_read/writes are atomic.

In which case you don't have to call i2c_set_bus_num() for every access, only whenever you don't know exactly where you're coming from. Functions that perform a sequence of accesses only need to set it once.

Also, if we directly talk to an adapter instead, then the bulk of what

i2c_set_bus_num() does isn't even required. It would require that

adapters are made aware of a hierarchy if there are muxes, but I think that's worthwhile to do in any case. Also if ever I2C muxing needs to gain device tree support having the muxes set up dynamically will be pretty much a prerequisite.

Quoted 32 lines

Quoted 13 lines

Quoted 13 lines

This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

  278 int i2c_set_bus_num(unsigned int bus)
  279 {
  280         int max;
  281
  282         if ((bus == I2C_BUS)&&   (I2C_ADAP->init_done>   0))
  283                 return 0;

Quoted 4 lines

Quoted 2 lines

And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes?

That's complexity that users shouldn't have to worry about. They should

Exactly!

Quoted 2 lines

simply access an adapter and the adapter (or rather the core) should take care of setting up any muxes correctly.

Yes!

I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter maybe with m muxes, and each bus has exactly one way through the i2c muxes, see for an example the README:

http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349

Quoted 3 lines

So the only thing a User must know when he wants to use an i2c bus is his number. The switching to this i2c adapter, initializing it and maybe set i2c muxes does the i2c subsystem ...

The above doesn't preclude an I2C adapter representing one of the ports of a mux. That way you can still talk to an adapter rather than having to refer to the bus by number. Adapter would become a little more abstract than it is now, since it would be simply an output that I2C

slaves are connected to (either a HW controller directly or a mux connected to a HW controller).

Quoted 15 lines

Quoted 9 lines

Quoted 7 lines

Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is an eeprom, on the other is a PMIC ... your code in patch "power: Add AMS AS3722 PMIC support" does access with your functions the PMIC ... what is, if between this accesses someone accesses the eeprom? If he switches the mux, you never switch back!

Your code did not check this!

Like I said, a lot of code in U-Boot doesn't check this. And quite

With using i2c_set_bus_num() you have not to check this! You only have to call i2c_set_bus_num() before calling i2c_read/write ... and yes, that would be nice, if we just pass the bus number to i2c_read/write() and drop the i2c_set_bus_num() call all over the code ...

Quoted 1 line

Patches welcome!

How about a slightly different proposal: introduce a new level of abstraction (like i2c_client) and start using it in new I2C slave drivers. At the same time existing drivers could be converted one at a time without having the big flag date when i2c_read() and i2c_write() are switched over all at once.

When that new level of abstraction is used, we can hide all the details behind that and the implementation no longer influences any of the drivers. Then we could transparently rework adapters and muxes to our heart's content without needing to update users of the high-level API.

Quoted 9 lines

Quoted 2 lines

frankly as long as this isn't handled in the core I don't think people will get it right.

Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c driver are converted to it, we can make i2c_set_bus_num() static, and add to the i2c API the bus number as a function parameter!

Quoted 2 lines

Quoted 1 line

Why is i2c_set_bus_num() such a problem?

Quoted 5 lines

Quoted 2 lines

Because it's completely confusing. And it's exposing an implementation detail to users instead of handling it transparently in the core.

Yes! Full Ack ... but I do not accept a new API for that! Please fix the i2c_read/write() functions!

Doing this kind of conversion is a nightmare. We'd be changing an API

that has around 600 occurrences in U-Boot, all of which need to be changed *at once* to avoid build breakage. At the same time we need to make sure any patches in development use the same API, which means that they can't even be build-tested until the the API has been changed.

Transitioning step by step is a lot less complicated.

Hello Thierry,

Am 27.08.2014 10:51, schrieb Thierry Reding:

Quoted 45 lines · Heiko Schocher

Quoted 43 lines

Hello Thierry,

Am 27.08.2014 08:21, schrieb Thierry Reding:

Quoted 14 lines · Heiko Schocher

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 6 lines

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

Yes, sadly. This has historical reasons ...

Quoted 1 line

doesn't run multithreaded this works. If you're really concerned about

Yes, U-Boot is singlethread only.

Quoted 3 lines

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!

But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write?

- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ...

Quoted 3 lines

I don't think adding a bus number as parameter is useful. Why not just use the I2C adapter directly? That way we don't have to keep looking it up in an array every time.

You again just talk from i2c_adapter ... why you ignore i2c muxes? A bus is not only an i2c adapter ...

Currently we have two "versions" of i2c_adapter:

In a system without muxes, you can say: i2c bus == i2c adapter
but in a system with muxes we have: i2c bus == i2c_bus_hose

i2c commands use also a "bus number" starting from 0 ... the bus number has historical reasons... we could change this ...

But if we introduce a new API, please with mux functionallity ... Hmm.. thinking about it ... if you want to introduce a new API, please start with using the DM for I2C!

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

this isn't going to work (in a multithreaded environment the switch to a different mux could happen between the call to i2c_set_bus_num() and the bus access).

In fact I think this would even have to be solved at the controller level if you want to make sure that client transactions are atomic.

Quoted 5 lines

Quoted 1 line

As U-Boot is single threaded all i2c_read/writes are atomic.

In which case you don't have to call i2c_set_bus_num() for every access, only whenever you don't know exactly where you're coming from. Functions that perform a sequence of accesses only need to set it once.

Yes ... which really is a pro for having i2c_set_bus_num() not static ...

Quoted 1 line

Also, if we directly talk to an adapter instead, then the bulk of what

You ignore again muxes ...

Quoted 1 line

i2c_set_bus_num() does isn't even required. It would require that

What does i2c_set_bus_num() ?

- first check, if current bus = new bus, and if it is initialized if so -> all is good, return!

Thats the main case ... is this so expensive? This checks should be always done (except we do a bulk of i2c accesses, yes). I think, this has also to be done somewhere with your approach ... or?

What is done in the case, we switch to another bus:

- If we have no muxes, save new bus for further use
- look if new bus is initialized, if not initialize it

All this is hidden from the user ... and actions, which must be done, whatever API you define ...

If I understand you correct, your big change is not using "int bus" instead introducing i2c_client ... and define some functions, which use this struct as parameter ...

Why not defining:

[1]:
int i2c_bus_read(int bus, uint8_t chip,
unsigned int address, size_t alen, void *buffer, size_t size)

{
    i2c_set_bus_num(bus);
    return i2c_read(chip, address, alen, buffer, size);
}

int i2c_bus_write(int bus, uint8_t chip,
unsigned int address, size_t alen, void *buffer, size_t size)

{
    i2c_set_bus_num(bus);
    return i2c_write(chip, address, alen, buffer, size);
}

and you never have to call something like i2c_init(), as i2c_set_bus_num() does this all for you! You only have to use in your driver i2c_bus_read/write ... without taking any attention ...

looking deeper in your approach:

drivers/i2c/i2c_core.c +8 -0

+int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
+            uint8_t address)
+{
+    client->adapter = adapter;
+    client->address = address;
+
+    return 0;
+}

Where is here called adapter->init() ? Nor you check, if the i2c adapter is intialized ... if more than one driver uses this function, each intializes the hw? Or currently none :-(

Quoted 37 lines

adapters are made aware of a hierarchy if there are muxes, but I think that's worthwhile to do in any case. Also if ever I2C muxing needs to gain device tree support having the muxes set up dynamically will be pretty much a prerequisite.

Quoted 32 lines

Quoted 13 lines

Quoted 13 lines

This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278

278 int i2c_set_bus_num(unsigned int bus)
279 {
280 int max;
281
282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))
283 return 0;

Quoted 4 lines

Quoted 2 lines

And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes?

That's complexity that users shouldn't have to worry about. They should

Exactly!

Quoted 2 lines

simply access an adapter and the adapter (or rather the core) should take care of setting up any muxes correctly.

Yes!

I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter maybe with m muxes, and each bus has exactly one way through the i2c muxes, see for an example the README:

http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349

Quoted 8 lines

Quoted 3 lines

So the only thing a User must know when he wants to use an i2c bus is his number. The switching to this i2c adapter, initializing it and maybe set i2c muxes does the i2c subsystem ...

The above doesn't preclude an I2C adapter representing one of the ports of a mux. That way you can still talk to an adapter rather than having to refer to the bus by number. Adapter would become a little more abstract than it is now, since it would be simply an output that I2C

Oh!

Quoted 2 lines

slaves are connected to (either a HW controller directly or a mux connected to a HW controller).

Thats sounds for me, that you should use DM ... I do not know, what your plans are!

Quoted 5 lines

When that new level of abstraction is used, we can hide all the details behind that and the implementation no longer influences any of the drivers. Then we could transparently rework adapters and muxes to our heart's content without needing to update users of the high-level API.

Ok, with some functions like in [1], maybe you introduce i2c_client, and use this instead "int bus" ...

Quoted 48 lines · Heiko Schocher

Hello Thierry,

Am 27.08.2014 10:51, schrieb Thierry Reding:

Quoted 45 lines · Heiko Schocher

Quoted 43 lines

Hello Thierry,

Am 27.08.2014 08:21, schrieb Thierry Reding:

Quoted 14 lines · Heiko Schocher

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 6 lines

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

Yes, sadly. This has historical reasons ...

Quoted 1 line

doesn't run multithreaded this works. If you're really concerned about

Yes, U-Boot is singlethread only.

Quoted 3 lines

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!

But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write?

- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ...

Quoted 6 lines

Quoted 3 lines

I don't think adding a bus number as parameter is useful. Why not just use the I2C adapter directly? That way we don't have to keep looking it up in an array every time.

You again just talk from i2c_adapter ... why you ignore i2c muxes? A bus is not only an i2c adapter ...

I know. I keep saying i2c_adapter because in the rough sketch that I have in mind for how this could work eventually, an mux is just another special kind of i2c_adapter.

Quoted 8 lines

Currently we have two "versions" of i2c_adapter:

In a system without muxes, you can say: i2c bus == i2c adapter
but in a system with muxes we have: i2c bus == i2c_bus_hose

i2c commands use also a "bus number" starting from 0 ... the bus number has historical reasons... we could change this ...

Quoted 3 lines

But if we introduce a new API, please with mux functionallity ... Hmm.. thinking about it ... if you want to introduce a new API, please start with using the DM for I2C!

I can look into it, but it sounds like a task way beyond what I have time for right now.

Hi Thierry,

Quoted 68 lines · Thierry Reding

Quoted 48 lines · Heiko Schocher

Hello Thierry,

Am 27.08.2014 10:51, schrieb Thierry Reding:

Quoted 45 lines · Heiko Schocher

Quoted 43 lines

Hello Thierry,

Am 27.08.2014 08:21, schrieb Thierry Reding:

Quoted 14 lines · Heiko Schocher

Quoted 13 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 3 lines

From: Thierry Reding<[email protected]>

This API operates on I2C adapters or I2C clients (a new type of object

which is a bad idea ...

Quoted 3 lines

that refers to a particular slave connected to an adapter). This is useful to avoid having to call i2c_set_bus_num() whenever a device is being accessed.

Quoted 6 lines

Quoted 3 lines

But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot

Yes, sadly. This has historical reasons ...

Quoted 1 line

doesn't run multithreaded this works. If you're really concerned about

Yes, U-Boot is singlethread only.

Quoted 3 lines

this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with

Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!

But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write?

- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ...

Quoted 6 lines

Quoted 3 lines

I don't think adding a bus number as parameter is useful. Why not just use the I2C adapter directly? That way we don't have to keep looking it up in an array every time.

You again just talk from i2c_adapter ... why you ignore i2c muxes? A bus is not only an i2c adapter ...

I know. I keep saying i2c_adapter because in the rough sketch that I have in mind for how this could work eventually, an mux is just another special kind of i2c_adapter.

Quoted 8 lines

Currently we have two "versions" of i2c_adapter:

In a system without muxes, you can say: i2c bus == i2c adapter
but in a system with muxes we have: i2c bus == i2c_bus_hose

i2c commands use also a "bus number" starting from 0 ... the bus number has historical reasons... we could change this ...

Quoted 7 lines

Quoted 3 lines

But if we introduce a new API, please with mux functionallity ... Hmm.. thinking about it ... if you want to introduce a new API, please start with using the DM for I2C!

I can look into it, but it sounds like a task way beyond what I have time for right now.

I can help if you are interested. I have patches for SPI at u-boot-dm.git (branch working) and would like to have I2C. They are sort-of similar so it might not be to hard to convert Tegra I2C over.

The concept of a 'current' bus is broken IMO. Maybe the command line should have this concept, and maintain a 'current hose' or whatever. But drives and other code should specific what they need with each transaction. With DM that will likely be automatic.

From: Thierry Reding <[email protected]>

This is useful to retrieve the U-Boot bus number of an I2C controller given a device tree node.

Signed-off-by Thierry Reding <[email protected]>

From: Thierry Reding<[email protected]>

The AS3722 provides a number of DC/DC converters and LDOs as well as 8 GPIOs.

Signed-off-by Thierry Reding<[email protected]>

---
  drivers/power/Makefile |   1 +
  drivers/power/as3722.c | 300 +++++++++++++++++++++++++++++++++++++++++++++++++
  include/fdtdec.h       |   1 +
  include/power/as3722.h |  27 +++++
  lib/fdtdec.c           |   1 +
  5 files changed, 330 insertions(+)

create mode 100644 drivers/power/as3722.c
create mode 100644 include/power/as3722.h

drivers/power/Makefile +1 -0

index dc64e4d32bff..b3097316e27e 100644
@@ -5,6 +5,7 @@
  # SPDX-License-Identifier:	GPL-2.0+
  #
 
+obj-$(CONFIG_AS3722_POWER)	+= as3722.o
  obj-$(CONFIG_AXP152_POWER)	+= axp152.o
  obj-$(CONFIG_AXP209_POWER)	+= axp209.o
  obj-$(CONFIG_EXYNOS_TMU)	+= exynos-tmu.o

drivers/power/as3722.c new file +263 -0

new file mode 100644
index 000000000000..59d1bf1b50b0
@@ -0,0 +1,300 @@
+/*
+ * Copyright (C) 2014 NVIDIA Corporation
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#define pr_fmt(fmt) "as3722: " fmt
+
+#include<common.h>
+#include<errno.h>
+#include<fdtdec.h>
+#include<i2c.h>
+
+#include<power/as3722.h>
+
+#define AS3722_SD_VOLTAGE(n) (0x00 + (n))
+#define AS3722_GPIO_CONTROL(n) (0x08 + (n))
+#define  AS3722_GPIO_CONTROL_MODE_OUTPUT_VDDH (1<<  0)
+#define  AS3722_GPIO_CONTROL_MODE_OUTPUT_VDDL (7<<  0)
+#define  AS3722_GPIO_CONTROL_INVERT (1<<  7)
+#define AS3722_LDO_VOLTAGE(n) (0x10 + (n))
+#define AS3722_GPIO_SIGNAL_OUT 0x20
+#define AS3722_SD_CONTROL 0x4d
+#define AS3722_LDO_CONTROL 0x4e
+#define AS3722_ASIC_ID1 0x90
+#define  AS3722_DEVICE_ID 0x0c
+#define AS3722_ASIC_ID2 0x91
+
+struct as3722 {
+	struct i2c_client client;
+	u8 address;
+};
+
+static struct as3722 as3722_pmic;
+
+static int as3722_read(struct as3722 *pmic, u8 reg, u8 *value)
+{
+	int err;
+
+	err = i2c_client_read(&pmic->client, reg, 1, value, 1);
+	if (err<  0)
+		return err;
+
+	return 0;
+}
+
+static int as3722_write(struct as3722 *pmic, u8 reg, u8 value)
+{
+	int err;
+
+	err = i2c_client_write(&pmic->client, reg, 1,&value, 1);
+	if (err<  0)
+		return err;
+
+	return 0;
+}
+
+static int as3722_read_id(struct as3722 *pmic, u8 *id, u8 *revision)
+{
+	int err;
+
+	err = as3722_read(pmic, AS3722_ASIC_ID1, id);
+	if (err) {
+		error("failed to read ID1 register: %d", err);
+		return err;
+	}
+
+	err = as3722_read(pmic, AS3722_ASIC_ID2, revision);
+	if (err) {
+		error("failed to read ID2 register: %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_sd_enable(struct as3722 *pmic, unsigned int sd)
+{
+	u8 value;
+	int err;
+
+	if (sd>  6)
+		return -EINVAL;
+
+	err = as3722_read(pmic, AS3722_SD_CONTROL,&value);
+	if (err) {
+		error("failed to read SD control register: %d", err);
+		return err;
+	}
+
+	value |= 1<<  sd;
+
+	err = as3722_write(pmic, AS3722_SD_CONTROL, value);
+	if (err<  0) {
+		error("failed to write SD control register: %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_sd_set_voltage(struct as3722 *pmic, unsigned int sd, u8 value)
+{
+	int err;
+
+	if (sd>  6)
+		return -EINVAL;
+
+	err = as3722_write(pmic, AS3722_SD_VOLTAGE(sd), value);
+	if (err<  0) {
+		error("failed to write SD%u voltage register: %d", sd, err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_ldo_enable(struct as3722 *pmic, unsigned int ldo)
+{
+	u8 value;
+	int err;
+
+	if (ldo>  11)
+		return -EINVAL;
+
+	err = as3722_read(pmic, AS3722_LDO_CONTROL,&value);
+	if (err) {
+		error("failed to read LDO control register: %d", err);
+		return err;
+	}
+
+	value |= 1<<  ldo;
+
+	err = as3722_write(pmic, AS3722_LDO_CONTROL, value);
+	if (err<  0) {
+		error("failed to write LDO control register: %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_ldo_set_voltage(struct as3722 *pmic, unsigned int ldo, u8 value)
+{
+	int err;
+
+	if (ldo>  11)
+		return -EINVAL;
+
+	err = as3722_write(pmic, AS3722_LDO_VOLTAGE(ldo), value);
+	if (err<  0) {
+		error("failed to write LDO%u voltage register: %d", ldo,
+		      err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_gpio_configure(struct as3722 *pmic, unsigned int gpio,
+			  unsigned long flags)
+{
+	u8 value = 0;
+	int err;
+
+	if (flags&  AS3722_GPIO_OUTPUT_VDDH)
+		value |= AS3722_GPIO_CONTROL_MODE_OUTPUT_VDDH;
+
+	if (flags&  AS3722_GPIO_INVERT)
+		value |= AS3722_GPIO_CONTROL_INVERT;
+
+	err = as3722_write(pmic, AS3722_GPIO_CONTROL(gpio), value);
+	if (err) {
+		error("failed to configure GPIO#%u: %d", gpio, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int as3722_gpio_set(struct as3722 *pmic, unsigned int gpio,
+			   unsigned int level)
+{
+	const char *l;
+	u8 value;
+	int err;
+
+	if (gpio>  7)
+		return -EINVAL;
+
+	err = as3722_read(pmic, AS3722_GPIO_SIGNAL_OUT,&value);
+	if (err<  0) {
+		error("failed to read GPIO signal out register: %d", err);
+		return err;
+	}
+
+	if (level == 0) {
+		value&= ~(1<<  gpio);
+		l = "low";
+	} else {
+		value |= 1<<  gpio;
+		l = "high";
+	}
+
+	err = as3722_write(pmic, AS3722_GPIO_SIGNAL_OUT, value);
+	if (err) {
+		error("failed to set GPIO#%u %s: %d", gpio, l, err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_gpio_direction_output(struct as3722 *pmic, unsigned int gpio,
+				 unsigned int level)
+{
+	u8 value;
+	int err;
+
+	if (gpio>  7)
+		return -EINVAL;
+
+	if (level == 0)
+		value = AS3722_GPIO_CONTROL_MODE_OUTPUT_VDDL;
+	else
+		value = AS3722_GPIO_CONTROL_MODE_OUTPUT_VDDH;
+
+	err = as3722_write(pmic, AS3722_GPIO_CONTROL(gpio), value);
+	if (err) {
+		error("failed to configure GPIO#%u as output: %d", gpio, err);
+		return err;
+	}
+
+	err = as3722_gpio_set(pmic, gpio, level);
+	if (err<  0) {
+		error("failed to set GPIO#%u high: %d", gpio, err);
+		return err;
+	}
+
+	return 0;
+}
+
+int as3722_init(struct as3722 **pmicp, const void *fdt)
+{
+	struct as3722 *pmic =&as3722_pmic;
+	int count, nodes[1], i;
+	int err;
+
+	count = fdtdec_find_aliases_for_id(fdt, NULL, COMPAT_AMS_AS3722,
+					   nodes, ARRAY_SIZE(nodes));
+	for (i = 0; i<  count; i++) {
+		int parent = fdt_parent_offset(fdt, nodes[i]), bus;
+		struct i2c_adapter *adapter;
+		fdt_addr_t address;
+		u8 id, revision;
+
+		bus = i2c_get_bus_num_fdt(parent);
+		if (bus<  0) {
+			error("invalid bus %d", bus);
+			continue;
+		}
+
+		address = fdtdec_get_addr(fdt, nodes[i], "reg");

drivers/power/as3722.c new file +27 -0

+
+		err = i2c_client_init(&pmic->client, adapter, address);
+		if (err<  0) {
+			error("failed to initialize I2C slave: %d", err);
+			continue;
+		}
+
+		err = as3722_read_id(pmic,&id,&revision);
+		if (err<  0) {
+			error("failed to read ID: %d", err);
+			continue;
+		}
+
+		if (id != AS3722_DEVICE_ID) {
+			error("unknown device");
+			continue;
+		}
+
+		debug("AS3722 revision %#x found on I2C bus %u, address %#x\n",
+		      revision, bus, address);
+
+		*pmicp = pmic;
+		return 0;
+	}
+
+	return -ENODEV;
+}

include/fdtdec.h +1 -0

index 5c669a5c8e03..3c38375397df 100644
@@ -115,6 +115,7 @@ enum fdt_compat_id {
  	COMPAT_SANDBOX_LCD_SDL,		/* Sandbox LCD emulation with SDL */
  	COMPAT_TI_TPS65090,		/* Texas Instrument TPS65090 */
  	COMPAT_NXP_PTN3460,		/* NXP PTN3460 DP/LVDS bridge */
+	COMPAT_AMS_AS3722,		/* AMS AS3722 PMIC */
 
  	COMPAT_COUNT,
  };

include/power/as3722.h new file +27 -0

new file mode 100644
index 000000000000..2c2e45a44bab
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2014 NVIDIA Corporation
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __POWER_AS3722_H__
+#define __POWER_AS3722_H__
+
+#include<asm/types.h>
+
+#define AS3722_GPIO_OUTPUT_VDDH (1<<  0)
+#define AS3722_GPIO_INVERT (1<<  1)
+
+struct as3722;
+
+int as3722_init(struct as3722 **pmic, const void *fdt);
+int as3722_sd_enable(struct as3722 *pmic, unsigned int sd);
+int as3722_sd_set_voltage(struct as3722 *pmic, unsigned int sd, u8 value);
+int as3722_ldo_enable(struct as3722 *pmic, unsigned int ldo);
+int as3722_ldo_set_voltage(struct as3722 *pmic, unsigned int ldo, u8 value);
+int as3722_gpio_configure(struct as3722 *pmic, unsigned int gpio,
+			  unsigned long flags);
+int as3722_gpio_direction_output(struct as3722 *pmic, unsigned int gpio,
+				 unsigned int level);
+
+#endif /* __POWER_AS3722_H__ */

lib/fdtdec.c +1 -0

index fa5da0c0147d..cddd86c3186f 100644
@@ -70,6 +70,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
  	COMPAT(SANDBOX_LCD_SDL, "sandbox,lcd-sdl"),
  	COMPAT(TI_TPS65090, "ti,tps65090"),
  	COMPAT(COMPAT_NXP_PTN3460, "nxp,ptn3460"),
+	COMPAT(AMS_AS3722, "ams,as3722"),
  };
 
  const char *fdtdec_get_compatible(enum fdt_compat_id id)

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

Quoted 27 lines

drivers/power/as3722.c new file +27 -0

+int as3722_init(struct as3722 **pmicp, const void *fdt)
+{
+	struct as3722 *pmic =&as3722_pmic;
+	int count, nodes[1], i;
+	int err;
+
+	count = fdtdec_find_aliases_for_id(fdt, NULL, COMPAT_AMS_AS3722,
+					   nodes, ARRAY_SIZE(nodes));
+	for (i = 0; i<  count; i++) {
+		int parent = fdt_parent_offset(fdt, nodes[i]), bus;
+		struct i2c_adapter *adapter;
+		fdt_addr_t address;
+		u8 id, revision;
+
+		bus = i2c_get_bus_num_fdt(parent);
+		if (bus<  0) {
+			error("invalid bus %d", bus);
+			continue;
+		}
+
+		address = fdtdec_get_addr(fdt, nodes[i], "reg");
+		if (address == FDT_ADDR_T_NONE) {
+			error("slave address not found");
+			continue;
+		}
+
+		adapter = i2c_adapter_get(bus);

drivers/power/as3722.c line 273

Quoted 4 lines

drivers/power/as3722.c new file +4 -0

+		if (!adapter) {
+			error("I2C adapter for bus %d not found", bus);
+			continue;
+		}

Why is i2c_set_bus_num() not enough for you? Why introducing a new API? If another uses this PMIC behind a i2c mux, this driver maybe not work correctly.

Quoted 3 lines

Hello Thierry,

Am 26.08.2014 17:34, schrieb Thierry Reding:

[...]

Quoted 27 lines

Quoted 27 lines

drivers/power/as3722.c new file +27 -0

+int as3722_init(struct as3722 **pmicp, const void *fdt)
+{
+	struct as3722 *pmic =&as3722_pmic;
+	int count, nodes[1], i;
+	int err;
+
+	count = fdtdec_find_aliases_for_id(fdt, NULL, COMPAT_AMS_AS3722,
+					   nodes, ARRAY_SIZE(nodes));
+	for (i = 0; i<   count; i++) {
+		int parent = fdt_parent_offset(fdt, nodes[i]), bus;
+		struct i2c_adapter *adapter;
+		fdt_addr_t address;
+		u8 id, revision;
+
+		bus = i2c_get_bus_num_fdt(parent);
+		if (bus<   0) {
+			error("invalid bus %d", bus);
+			continue;
+		}
+
+		address = fdtdec_get_addr(fdt, nodes[i], "reg");
+		if (address == FDT_ADDR_T_NONE) {
+			error("slave address not found");
+			continue;
+		}
+
+		adapter = i2c_adapter_get(bus);

Quoted 8 lines

drivers/power/as3722.c line 273

Quoted 4 lines

drivers/power/as3722.c new file +4 -0

+		if (!adapter) {
+			error("I2C adapter for bus %d not found", bus);
+			continue;
+		}

Why is i2c_set_bus_num() not enough for you? Why introducing a new API? If another uses this PMIC behind a i2c mux, this driver maybe not work correctly.

As I explained earlier, I think requiring users to manually micro-manage the I2C busses is a bad idea. It's very tedious and error prone. It does

also mean that every patch needs to be carefully checked that the I2C API is used correctly. The intent of the i2c_client API was to introduce a higher-level object that people can use and that does the bulk of the work behind the scenes rather than duplicating it all over the place. It

is true that perhaps it currently doesn't work well with I2C muxes. But the advantage is that when it does, it does so for everybody that uses it.