Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hello Simon, On Fri, 21 Nov 2014 23:29:53 +0100, Simon Glass s...@chromium.org wrote: Do you mean you implemented the same (similar) behavior as the legacy I2C framework? Yes, we need the ability to scan the bus and find which devices are present. Side note: not sure this was touched on, and not sure what the implications are, but let me just mention that some SPLs need this ability too (this causes SPL to grow, which is probably the reason that some ARM boards failed to build once the i2c linker lists were added to the SPL lds file and until some other feature was removed). Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Simon, Heiko, On Thu, 20 Nov 2014 14:04:52 + Simon Glass s...@chromium.org wrote: Let's assume we want to write some data to a EEPROM chip connected to i2c bus. We generally send [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ... From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data. I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written. + return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer, + len); In this implementation, the offset_address is treated with addr and the *real* data is handled with buffer. It seems odd from the perspective of I2C protocol, I think. Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). Yes, the code of the write (for the address) in the .read() handler would the same as the .write handler. I thought it would not be a good idea to duplicate the write transaction code in every driver. If we can accept this change, we only need to implement write - restart - read code in i2c_read() in i2c-uclass.c. Each driver would be much simpler. That is the point of my suggestion. + } + debug(not found\n); + return i2c_bind_driver(bus, chip_addr, devp); +} If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why? The i2c_bind_driver() tries to create a generic chip. What is this generic chip? Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have reg property I could not understand at all what this code wants to do. This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it. As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only. Sorry, I could not parse you here. I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions? If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device. With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected. Do you mean you implemented the same (similar) behavior as the legacy I2C framework? Because the clock-frequency is set to 40 in the sandbox DTS, writing to I2C fails unless we change the I2C speed. Is this intentional? Personally, I like everything to work on the mail line. This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops. I see. Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Heiko, Simon, On Fri, 21 Nov 2014 08:24:18 +0100 Heiko Schocher h...@denx.de wrote: Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. Yes, but this changes the current U-Boot API ... I am hoping the translation code will be implemented in drivers/i2c/i2c-uclass.c, I think. We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). If we want to change it, we should do it now. My question is, what is the point? Will we really want 2 elements in the message array, do we Do we need more than 2 elements? But of course, if we go into this direction we should support n elements ... want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I Thats the point ... do we need all this stuff in U-Boot? feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely. Perhaps Heiko can share an opinion here? This implements that we must adapt each i2c driver a little bit more right? But I think, as we go with this approach more into the linux direction it sounds good to me (maybe we can directly use linux i2c drivers? ... that sounds good, and maybe should be a goal too?). I could not really say how many work this would be, but as we do this change step by step it is worth to go in this direction, as we can cleanup here and there (especially the eeprom driver) some suboptimal code ... Thinking about it ... maybe we start from scratch with i2c drivers for DM and try to use linux i2c drivers? Anyway, DM is a giant change. I think it is the best (and perhaps the last) opportunity to implement things correctly. Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Masahiro, On 21 November 2014 09:04, Masahiro Yamada yamad...@jp.panasonic.com wrote: Hi Simon, Heiko, On Thu, 20 Nov 2014 14:04:52 + Simon Glass s...@chromium.org wrote: Let's assume we want to write some data to a EEPROM chip connected to i2c bus. We generally send [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ... From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data. I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written. + return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer, + len); In this implementation, the offset_address is treated with addr and the *real* data is handled with buffer. It seems odd from the perspective of I2C protocol, I think. Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). Yes, the code of the write (for the address) in the .read() handler would the same as the .write handler. I thought it would not be a good idea to duplicate the write transaction code in every driver. If we can accept this change, we only need to implement write - restart - read code in i2c_read() in i2c-uclass.c. Each driver would be much simpler. That is the point of my suggestion. I'm not sure we can do that. Some drivers have to do special things to handle the restart and it would not necessarily be a good idea to move that logic to the uclass. + } + debug(not found\n); + return i2c_bind_driver(bus, chip_addr, devp); +} If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why? The i2c_bind_driver() tries to create a generic chip. What is this generic chip? Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have reg property I could not understand at all what this code wants to do. This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it. As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only. Sorry, I could not parse you here. I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions? If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device. With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected. Do you mean you implemented the same (similar) behavior as the legacy I2C framework? Yes, we need the ability to scan the bus and find which devices are present. Because the clock-frequency is set to 40 in the sandbox DTS, writing to I2C fails unless we change the I2C speed. Is this intentional? Personally, I like everything to work on the mail line. This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops. I see. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Masahiro, On 21 November 2014 09:18, Masahiro Yamada yamad...@jp.panasonic.com wrote: Hi Heiko, Simon, On Fri, 21 Nov 2014 08:24:18 +0100 Heiko Schocher h...@denx.de wrote: Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. Yes, but this changes the current U-Boot API ... I am hoping the translation code will be implemented in drivers/i2c/i2c-uclass.c, I think. That's right, no change to the U-Boot API. We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). If we want to change it, we should do it now. My question is, what is the point? Will we really want 2 elements in the message array, do we Do we need more than 2 elements? But of course, if we go into this direction we should support n elements ... want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I Thats the point ... do we need all this stuff in U-Boot? feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely. Perhaps Heiko can share an opinion here? This implements that we must adapt each i2c driver a little bit more right? But I think, as we go with this approach more into the linux direction it sounds good to me (maybe we can directly use linux i2c drivers? ... that sounds good, and maybe should be a goal too?). I could not really say how many work this would be, but as we do this change step by step it is worth to go in this direction, as we can cleanup here and there (especially the eeprom driver) some suboptimal code ... Thinking about it ... maybe we start from scratch with i2c drivers for DM and try to use linux i2c drivers? Anyway, DM is a giant change. I think it is the best (and perhaps the last) opportunity to implement things correctly. OK, any other opinions? I'm leaning towards going with Masahiro's idea. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
+ A few more people to cc Hi Masahiro, On 20 November 2014 06:05, Masahiro Yamada yamad...@jp.panasonic.com wrote: Hi Simon, On Wed, 19 Nov 2014 10:24:47 + Simon Glass s...@chromium.org wrote: BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver? I mean, can we put the offset address and data in the buffer? I'm not sure what you mean by this sorry. Let's assume we want to write some data to a EEPROM chip connected to i2c bus. We generally send [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ... From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data. I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written. + return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer, + len); In this implementation, the offset_address is treated with addr and the *real* data is handled with buffer. It seems odd from the perspective of I2C protocol, I think. Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). If we want to change it, we should do it now. My question is, what is the point? Will we really want 2 elements in the message array, do we want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely. Perhaps Heiko can share an opinion here? + } + debug(not found\n); + return i2c_bind_driver(bus, chip_addr, devp); +} If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why? The i2c_bind_driver() tries to create a generic chip. What is this generic chip? Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have reg property I could not understand at all what this code wants to do. This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it. As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only. Sorry, I could not parse you here. I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions? If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device. With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected. BTW, sandbox_i2c_read() is 400KHz tolerate: /* For testing, don't allow reading above 400KHz */ if (i2c-speed_hz 40 || alen != 1) return -EINVAL; but sandbox_i2c_write() only allows 100KHz: /* For testing, don't allow writing above 100KHz */ if (i2c-speed_hz 10 || alen != 1) return -EINVAL; Because the clock-frequency is set to 40 in the sandbox DTS, writing to I2C fails unless we change the I2C speed. Is this intentional? Personally, I like everything to work on the mail line. This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops. Regards, Simon
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hello Simon, Masahiro, Am 20.11.2014 15:04, schrieb Simon Glass: + A few more people to cc Hi Masahiro, On 20 November 2014 06:05, Masahiro Yamada yamad...@jp.panasonic.com wrote: Hi Simon, On Wed, 19 Nov 2014 10:24:47 + Simon Glass s...@chromium.org wrote: BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver? I mean, can we put the offset address and data in the buffer? I'm not sure what you mean by this sorry. Let's assume we want to write some data to a EEPROM chip connected to i2c bus. We generally send [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ... From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data. I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written. + return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer, + len); In this implementation, the offset_address is treated with addr and the *real* data is handled with buffer. It seems odd from the perspective of I2C protocol, I think. Yes. Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. Yes, but this changes the current U-Boot API ... We could certainly change this. I'm not sure that I have a strong opinion either way. I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements: a write (for the address) then a read (to get the data). If we want to change it, we should do it now. My question is, what is the point? Will we really want 2 elements in the message array, do we Do we need more than 2 elements? But of course, if we go into this direction we should support n elements ... want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I Thats the point ... do we need all this stuff in U-Boot? feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely. Perhaps Heiko can share an opinion here? This implements that we must adapt each i2c driver a little bit more right? But I think, as we go with this approach more into the linux direction it sounds good to me (maybe we can directly use linux i2c drivers? ... that sounds good, and maybe should be a goal too?). I could not really say how many work this would be, but as we do this change step by step it is worth to go in this direction, as we can cleanup here and there (especially the eeprom driver) some suboptimal code ... Thinking about it ... maybe we start from scratch with i2c drivers for DM and try to use linux i2c drivers? bye, Heiko + } + debug(not found\n); + return i2c_bind_driver(bus, chip_addr, devp); +} If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why? The i2c_bind_driver() tries to create a generic chip. What is this generic chip? Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have reg property I could not understand at all what this code wants to do. This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it. As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only. Sorry, I could not parse you here. I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions? If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device. With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Simon,
On Tue, 11 Nov 2014 10:46:24 -0700
Simon Glass s...@chromium.org wrote:
The uclass implements the same operations as the current I2C framework but
makes some changes to make it fit driver model better:
- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions
Signed-off-by: Simon Glass s...@chromium.org
---
Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number
[snip]
+
+int i2c_read(struct udevice *dev, uint addr, uint8_t *buffer, int len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-read)
+ return -ENOSYS;
+
+ return ops-read(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+ len);
+}
+
+int i2c_write(struct udevice *dev, uint addr, const uint8_t *buffer, int len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-write)
+ return -ENOSYS;
+
+ return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+ len);
+}
This seems inconsistent with struct dm_i2c_ops.
In this function, the address length(chip-addr_len) is passed to the forth
argument of ops-write().
You should compare it with struct dm_i2c_ops in include/i2c.h
It says that the third argument is alen.
BTW, address_offset within the chip and data are treated in the same way in I2C
bus.
Should we pass them separately to each driver?
I mean, can we put the offset address and data in the buffer?
+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+ struct udevice *dev;
+
+ debug(%s: Searching bus '%s' for address %02x: , __func__,
+ bus-name, chip_addr);
+ for (device_find_first_child(bus, dev); dev;
+ device_find_next_child(dev)) {
+ struct dm_i2c_chip store;
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ int ret;
+
+ if (!chip) {
+ chip = store;
+ i2c_chip_ofdata_to_platdata(gd-fdt_blob,
+ dev-of_offset, chip);
+ }
+ if (chip-chip_addr == chip_addr) {
+ ret = device_probe(dev);
+ debug(found, ret=%d\n, ret);
+ if (ret)
+ return ret;
+ *devp = dev;
+ return 0;
+ }
If chip is NULL, i2c_chip_ofdata_to_platdata() is called to create
struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.
If we use device_probe_child() instead of device_probe(), I think we can
re-use it.
I mean,
if (chip-chip_addr == chip_addr) {
ret = device_probe_child(dev, chip);
Perhaps, we can remove sandbox_i2c_child_pre_probe().
+ }
+ debug(not found\n);
+ return i2c_bind_driver(bus, chip_addr, devp);
+}
If no chip-device is found at the specified chip_addr,
the last line calls i2c_bind_driver(). Why?
The i2c_bind_driver() tries to create a generic chip.
What is this generic chip?
Besides, i2c_bind_driver() tries to probe the created generic chip,
but it always fails in i2c_chip_ofdata_to_platdata()
because the generic chip does not have reg property
I could not understand at all what this code wants to do.
+}
+
+int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+ int ret;
+
+ *devp = NULL;
+ if (!ops-probe)
+ return -ENODEV;
+
+ /* First probe that chip */
+ ret = ops-probe(bus, chip_addr);
+ debug(%s: bus='%s', address %02x, ret=%d\n, __func__, bus-name,
+ chip_addr, ret);
+ if (ret)
+ return ret;
Is the ops-probe responsible to probe child devices?
(At least, sandbox_i2c probes its children)
+ /* The chip was found, see if we have a driver, and probe it */
+ ret = i2c_get_chip(bus, chip_addr, devp);
+ debug(%s: i2c_get_chip: ret=%d\n, __func__, ret);
+ if (!ret || ret != -ENODEV)
+ return ret;
+
+ return i2c_bind_driver(bus, chip_addr, devp);
+}
If so, why do we need to call i2c_get_chip() and probe it again?
+int i2c_set_bus_speed(struct udevice
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Simon,
Let me correct my question.
(I should have read my mail three times before sending.)
On Wed, 19 Nov 2014 17:56:34 +0900
Masahiro Yamada yamad...@jp.panasonic.com wrote:
+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice
**devp)
+{
+ struct udevice *dev;
+
+ debug(%s: Searching bus '%s' for address %02x: , __func__,
+ bus-name, chip_addr);
+ for (device_find_first_child(bus, dev); dev;
+ device_find_next_child(dev)) {
+ struct dm_i2c_chip store;
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ int ret;
+
+ if (!chip) {
+ chip = store;
+ i2c_chip_ofdata_to_platdata(gd-fdt_blob,
+ dev-of_offset, chip);
+ }
+ if (chip-chip_addr == chip_addr) {
+ ret = device_probe(dev);
+ debug(found, ret=%d\n, ret);
+ if (ret)
+ return ret;
+ *devp = dev;
+ return 0;
+ }
If chip is NULL, i2c_chip_ofdata_to_platdata() is called to create
struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.
I mean:
but it *is* thrown away soon.
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Masahiro,
On 19 November 2014 08:56, Masahiro Yamada yamad...@jp.panasonic.com wrote:
Hi Simon,
On Tue, 11 Nov 2014 10:46:24 -0700
Simon Glass s...@chromium.org wrote:
The uclass implements the same operations as the current I2C framework but
makes some changes to make it fit driver model better:
- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions
Signed-off-by: Simon Glass s...@chromium.org
---
Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number
[snip]
+
+int i2c_read(struct udevice *dev, uint addr, uint8_t *buffer, int len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-read)
+ return -ENOSYS;
+
+ return ops-read(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+ len);
+}
+
+int i2c_write(struct udevice *dev, uint addr, const uint8_t *buffer, int
len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-write)
+ return -ENOSYS;
+
+ return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+ len);
+}
This seems inconsistent with struct dm_i2c_ops.
In this function, the address length(chip-addr_len) is passed to the forth
argument of ops-write().
You should compare it with struct dm_i2c_ops in include/i2c.h
It says that the third argument is alen.
Oh dear that's going to be very very confusing. I'll tidy this up.
BTW, address_offset within the chip and data are treated in the same way in
I2C bus.
Should we pass them separately to each driver?
I mean, can we put the offset address and data in the buffer?
I'm not sure what you mean by this sorry.
+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+ struct udevice *dev;
+
+ debug(%s: Searching bus '%s' for address %02x: , __func__,
+ bus-name, chip_addr);
+ for (device_find_first_child(bus, dev); dev;
+ device_find_next_child(dev)) {
+ struct dm_i2c_chip store;
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ int ret;
+
+ if (!chip) {
+ chip = store;
+ i2c_chip_ofdata_to_platdata(gd-fdt_blob,
+ dev-of_offset, chip);
+ }
+ if (chip-chip_addr == chip_addr) {
+ ret = device_probe(dev);
+ debug(found, ret=%d\n, ret);
+ if (ret)
+ return ret;
+ *devp = dev;
+ return 0;
+ }
If chip is NULL, i2c_chip_ofdata_to_platdata() is called to create
struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.
If we use device_probe_child() instead of device_probe(), I think we can
re-use it.
I mean,
if (chip-chip_addr == chip_addr) {
ret = device_probe_child(dev, chip);
Perhaps, we can remove sandbox_i2c_child_pre_probe().
Yes that sounds good but the only catch is that i2c drivers may want
to have extra information over and above struct dm_i2c_chip.
I'm not terribly happy with how this works (SPI is the same). We are
peeking to see the address of a device, then throwing that information
away.
For PCI (which I was looking at on a recent flight) it really can't be
made to work - in that case devices are configured before they are
probed, and it's really helpful to figure out the bus addresses at
'bind' time. So I'm looking at introducing a per-child platdata
structure. In that case we wouldn't need to throw the information
away, and better, it puts the driver back in control of this decoding.
+ }
+ debug(not found\n);
+ return i2c_bind_driver(bus, chip_addr, devp);
+}
If no chip-device is found at the specified chip_addr,
the last line calls i2c_bind_driver(). Why?
The i2c_bind_driver() tries to create a generic chip.
What is this generic chip?
Besides, i2c_bind_driver() tries to probe the created generic chip,
but it always fails in i2c_chip_ofdata_to_platdata()
because the generic chip does not have reg property
I could not understand at all what this code wants to do.
This actually creates the device. A generic I2C device is
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Simon, On Wed, 19 Nov 2014 10:24:47 + Simon Glass s...@chromium.org wrote: BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver? I mean, can we put the offset address and data in the buffer? I'm not sure what you mean by this sorry. Let's assume we want to write some data to a EEPROM chip connected to i2c bus. We generally send [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0 [byte 3] WData1 ... From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data. I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written. + return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer, + len); In this implementation, the offset_address is treated with addr and the *real* data is handled with buffer. It seems odd from the perspective of I2C protocol, I think. Likewise, when we read data from a EEPROM chip connected to i2c bus, We generally send/receive [byte 0] SLAVE_ADDR (7bit) + W flag [byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag [byte 3] RData 0 [byte 4] Rdata 1 [byte 1], [byte 3], [byte 4] are data written/read via I2C bus. In my view, each I2C driver should handle [byte 0] and [byte 1] in its .write operation and [byte 2], [byte3], [byte 4], in its .read operation. + } + debug(not found\n); + return i2c_bind_driver(bus, chip_addr, devp); +} If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why? The i2c_bind_driver() tries to create a generic chip. What is this generic chip? Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have reg property I could not understand at all what this code wants to do. This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it. As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only. Sorry, I could not parse you here. I2C is not a hot-plugged bus. I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions? BTW, sandbox_i2c_read() is 400KHz tolerate: /* For testing, don't allow reading above 400KHz */ if (i2c-speed_hz 40 || alen != 1) return -EINVAL; but sandbox_i2c_write() only allows 100KHz: /* For testing, don't allow writing above 100KHz */ if (i2c-speed_hz 10 || alen != 1) return -EINVAL; Because the clock-frequency is set to 40 in the sandbox DTS, writing to I2C fails unless we change the I2C speed. Is this intentional? Personally, I like everthing to work on the mail line. Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hi Simon,
On Tue, 11 Nov 2014 10:46:24 -0700
Simon Glass s...@chromium.org wrote:
The uclass implements the same operations as the current I2C framework but
makes some changes to make it fit driver model better:
- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions
Signed-off-by: Simon Glass s...@chromium.org
---
+static int i2c_post_probe(struct udevice *dev)
+{
+ struct dm_i2c_bus *i2c = dev-uclass_priv;
+
+ i2c-speed_hz = fdtdec_get_int(gd-fdt_blob, dev-of_offset,
+ clock-frequency, 10);
+
+ return i2c_set_bus_speed(dev, i2c-speed_hz);
+}
This code in drivers/i2c/i2c-uclass.c seems to highly depends on Device Tree.
I am not sure if I understood correctly, but
does this work on non Device Tree SoCs?
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hello Masahiro,
Am 18.11.2014 13:32, schrieb Masahiro Yamada:
Hi Simon,
On Tue, 11 Nov 2014 10:46:24 -0700
Simon Glass s...@chromium.org wrote:
The uclass implements the same operations as the current I2C framework but
makes some changes to make it fit driver model better:
- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions
Signed-off-by: Simon Glass s...@chromium.org
---
+static int i2c_post_probe(struct udevice *dev)
+{
+ struct dm_i2c_bus *i2c = dev-uclass_priv;
+
+ i2c-speed_hz = fdtdec_get_int(gd-fdt_blob, dev-of_offset,
+clock-frequency, 10);
+
+ return i2c_set_bus_speed(dev, i2c-speed_hz);
+}
This code in drivers/i2c/i2c-uclass.c seems to highly depends on Device Tree.
I am not sure if I understood correctly, but
does this work on non Device Tree SoCs?
No. Devie Model is currently (as I understand) useable with
Device Tree ...
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
On Tue, Nov 11, 2014 at 10:46:24AM -0700, Simon Glass wrote: The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better: - Remove the chip address from API calls - Remove the address length from API calls - Remove concept of 'current' I2C bus - Drop all existing init functions Signed-off-by: Simon Glass s...@chromium.org Reviewed-by: Tom Rini tr...@ti.com -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Hello Simon, Am 11.11.2014 18:46, schrieb Simon Glass: The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better: - Remove the chip address from API calls - Remove the address length from API calls - Remove concept of 'current' I2C bus - Drop all existing init functions Signed-off-by: Simon Glass s...@chromium.org --- Changes in v2: - Fix cihp typo - Implement generic I2C devices to allow 'i2c probe' on unknown devices - Return the probed device from i2c_probe() - Set the bus speed after the bus is probed - Add some debugging for generic I2C device binding - Add a 'deblock' method to recover an I2C bus stuck in mid-transaction - Add a helper function to find a chip on a particular bus number drivers/i2c/Makefile | 1 + drivers/i2c/i2c-uclass.c | 275 +++ include/config_fallbacks.h | 6 + include/dm/uclass-id.h | 2 + include/i2c.h | 288 + 5 files changed, 572 insertions(+) create mode 100644 drivers/i2c/i2c-uclass.c Thanks for your work. Acked-by: Heiko Schocher h...@denx.de bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
The uclass implements the same operations as the current I2C framework but
makes some changes to make it fit driver model better:
- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions
Signed-off-by: Simon Glass s...@chromium.org
---
Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number
drivers/i2c/Makefile | 1 +
drivers/i2c/i2c-uclass.c | 275 +++
include/config_fallbacks.h | 6 +
include/dm/uclass-id.h | 2 +
include/i2c.h | 288 +
5 files changed, 572 insertions(+)
create mode 100644 drivers/i2c/i2c-uclass.c
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index d067897..25f0f19 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -4,6 +4,7 @@
#
# SPDX-License-Identifier: GPL-2.0+
#
+obj-$(CONFIG_DM_I2C) += i2c-uclass.o
obj-$(CONFIG_BFIN_TWI_I2C) += bfin-twi_i2c.o
obj-$(CONFIG_I2C_MV) += mv_i2c.o
diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
new file mode 100644
index 000..9436dfa
--- /dev/null
+++ b/drivers/i2c/i2c-uclass.c
@@ -0,0 +1,275 @@
+/*
+ * Copyright (c) 2014 Google, Inc
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include common.h
+#include dm.h
+#include errno.h
+#include fdtdec.h
+#include i2c.h
+#include malloc.h
+#include dm/device-internal.h
+#include dm/lists.h
+#include dm/root.h
+
+DECLARE_GLOBAL_DATA_PTR;
+
+int i2c_read(struct udevice *dev, uint addr, uint8_t *buffer, int len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-read)
+ return -ENOSYS;
+
+ return ops-read(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+len);
+}
+
+int i2c_write(struct udevice *dev, uint addr, const uint8_t *buffer, int len)
+{
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ struct udevice *bus = dev_get_parent(dev);
+ struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+ if (!ops-write)
+ return -ENOSYS;
+
+ return ops-write(bus, chip-chip_addr, addr, chip-addr_len, buffer,
+ len);
+}
+
+static int i2c_bind_driver(struct udevice *bus, uint chip_addr,
+ struct udevice **devp)
+{
+ struct dm_i2c_chip *chip;
+ char name[30], *str;
+ struct udevice *dev;
+ int ret;
+
+ snprintf(name, sizeof(name), generic_%x, chip_addr);
+ str = strdup(name);
+ ret = device_bind_driver(bus, i2c_generic_drv, str, dev);
+ debug(%s: device_bind_driver: ret=%d\n, __func__, ret);
+ if (ret)
+ goto err_bind;
+
+ /* Tell the device what we know about it */
+ chip = calloc(1, sizeof(struct dm_i2c_chip));
+ if (!chip) {
+ ret = -ENOMEM;
+ goto err_mem;
+ }
+ chip-chip_addr = chip_addr;
+ chip-addr_len = 1; /* we assume */
+ ret = device_probe_child(dev, chip);
+ debug(%s: device_probe_child: ret=%d\n, __func__, ret);
+ free(chip);
+ if (ret)
+ goto err_probe;
+
+ *devp = dev;
+ return 0;
+
+err_probe:
+err_mem:
+ device_unbind(dev);
+err_bind:
+ free(str);
+ return ret;
+}
+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+ struct udevice *dev;
+
+ debug(%s: Searching bus '%s' for address %02x: , __func__,
+ bus-name, chip_addr);
+ for (device_find_first_child(bus, dev); dev;
+ device_find_next_child(dev)) {
+ struct dm_i2c_chip store;
+ struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+ int ret;
+
+ if (!chip) {
+ chip = store;
+ i2c_chip_ofdata_to_platdata(gd-fdt_blob,
+ dev-of_offset, chip);
+ }
+ if (chip-chip_addr == chip_addr) {
+ ret = device_probe(dev);
+ debug(found, ret=%d\n, ret);
+ if (ret)
+ return ret;
+ *devp = dev;
+ return 0;
+ }
+ }
+ debug(not found\n);
+ return i2c_bind_driver(bus, chip_addr, devp);
+}
+
+int i2c_get_chip_for_busnum(int busnum, int chip_addr, struct udevice **devp)
+{
+ struct udevice
