From: Marc Ferland <[email protected]>
The current ds_read_block function only supports block sizes up to
128 bytes, which is the depth of the 'data out' fifo on the ds2490.
Reading larger blocks will fail with a: -110 (ETIMEDOUT) from
usb_control_msg(). Example:
$ dd if=/sys/bus/w1/devices/43-000000478756/eeprom bs=256 count=1
yields to the following message from the kernel:
usb 5-1: Failed to write 1-wire data to ep0x2: err=-110.
I discovered this issue while implementing support for the ds28ec20
eeprom in the w1-2433 driver. This driver accepts reading blocks of
sizes up to the size of the entire memory (2560 bytes in the case of
the ds28ec20). Note that this issue _does not_ arises when the kernel
is configured with CONFIG_W1_SLAVE_DS2433_CRC enabled since in this
mode the driver reads one 32 byte block at a time (a single memory
page).
Also, from the ds2490 datasheet (2995.pdf, page 22, BLOCK I/O
command):
For a block write sequence the EP2 FIFO must be pre-filled with
data before command execution. Additionally, for block sizes
greater then the FIFO size, the FIFO content status must be
monitored by host SW so that additional data can be sent to the
FIFO when necessary. A similar EP3 FIFO content monitoring
requirement exists for block read sequences. During a block read
the number of bytes loaded into the EP3 FIFO must be monitored so
that the data can be read before the FIFO overflows.
Breaking the buffer in 128 bytes blocks and simply calling the
original code sequence has solved the issue for me.
Tested with a DS1490F usb<->one-wire adapter and both the DS28EC20 and
DS2433 eeprom memories.
Note: The v1 of this patch changed both the ds_read_block and
ds_write_block functions, but since I don't have any way to test the
'write' part with writes bigger than a page size (maximum accepted by
my eeprom), I preferred not to make any assumptions and I just
removed that part.
Signed-off-by: Marc Ferland <[email protected]>
---
drivers/w1/masters/ds2490.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 5f5b97e24700..b6e244992c15 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -98,6 +98,8 @@
#define ST_EPOF 0x80
/* Status transfer size, 16 bytes status, 16 byte result flags */
#define ST_SIZE 0x20
+/* 1-wire data i/o fifo size, 128 bytes */
+#define FIFO_SIZE 0x80
/* Result Register flags */
#define RR_DETECT 0xA5 /* New device detected */
@@ -614,14 +616,11 @@ static int ds_read_byte(struct ds_device *dev, u8 *byte)
return 0;
}
-static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
+static int __read_block(struct ds_device *dev, u8 *buf, int len)
{
struct ds_status st;
int err;
- if (len > 64*1024)
- return -E2BIG;
-
memset(buf, 0xFF, len);
err = ds_send_data(dev, buf, len);
@@ -640,6 +639,24 @@ static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
return err;
}
+static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
+{
+ int err, to_read, rem = len;
+
+ if (len > 64*1024)
+ return -E2BIG;
+
+ do {
+ to_read = rem <= FIFO_SIZE ? rem : FIFO_SIZE;
+ err = __read_block(dev, &buf[len - rem], to_read);
+ if (err < 0)
+ return err;
+ rem -= to_read;
+ } while (rem);
+
+ return err;
+}
+
static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
{
int err;
--
2.34.1
On 30/11/2023 14:52, [email protected] wrote:
> From: Marc Ferland <[email protected]>
>
> The current ds_read_block function only supports block sizes up to
> 128 bytes, which is the depth of the 'data out' fifo on the ds2490.
>
> Reading larger blocks will fail with a: -110 (ETIMEDOUT) from
> usb_control_msg(). Example:
>
> $ dd if=/sys/bus/w1/devices/43-000000478756/eeprom bs=256 count=1
>
> yields to the following message from the kernel:
>
> usb 5-1: Failed to write 1-wire data to ep0x2: err=-110.
>
> I discovered this issue while implementing support for the ds28ec20
> eeprom in the w1-2433 driver. This driver accepts reading blocks of
> sizes up to the size of the entire memory (2560 bytes in the case of
> the ds28ec20). Note that this issue _does not_ arises when the kernel
> is configured with CONFIG_W1_SLAVE_DS2433_CRC enabled since in this
> mode the driver reads one 32 byte block at a time (a single memory
> page).
>
> Also, from the ds2490 datasheet (2995.pdf, page 22, BLOCK I/O
> command):
>
> For a block write sequence the EP2 FIFO must be pre-filled with
> data before command execution. Additionally, for block sizes
> greater then the FIFO size, the FIFO content status must be
> monitored by host SW so that additional data can be sent to the
> FIFO when necessary. A similar EP3 FIFO content monitoring
> requirement exists for block read sequences. During a block read
> the number of bytes loaded into the EP3 FIFO must be monitored so
> that the data can be read before the FIFO overflows.
>
> Breaking the buffer in 128 bytes blocks and simply calling the
> original code sequence has solved the issue for me.
>
> Tested with a DS1490F usb<->one-wire adapter and both the DS28EC20 and
> DS2433 eeprom memories.
>
> Note: The v1 of this patch changed both the ds_read_block and
> ds_write_block functions, but since I don't have any way to test the
> 'write' part with writes bigger than a page size (maximum accepted by
> my eeprom), I preferred not to make any assumptions and I just
> removed that part.
Drop that paragraph, not really helping to understand the commit once
accepted.
>
> Signed-off-by: Marc Ferland <[email protected]>
> ---
> drivers/w1/masters/ds2490.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
> index 5f5b97e24700..b6e244992c15 100644
> --- a/drivers/w1/masters/ds2490.c
> +++ b/drivers/w1/masters/ds2490.c
> @@ -98,6 +98,8 @@
> #define ST_EPOF 0x80
> /* Status transfer size, 16 bytes status, 16 byte result flags */
> #define ST_SIZE 0x20
> +/* 1-wire data i/o fifo size, 128 bytes */
> +#define FIFO_SIZE 0x80
>
> /* Result Register flags */
> #define RR_DETECT 0xA5 /* New device detected */
> @@ -614,14 +616,11 @@ static int ds_read_byte(struct ds_device *dev, u8 *byte)
> return 0;
> }
>
> -static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
> +static int __read_block(struct ds_device *dev, u8 *buf, int len)
Drop __ and name the function descriptive. It's confusing to have two
times read_block(). Just name them according to what they really do.
Best regards,
Krzysztof