* [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits @ 2026-09-08 10:27 Shubham Patil 2026-09-10 18:34 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Shubham Patil @ 2026-09-08 10:27 UTC (permalink / raw) To: Alexandre Belloni, Frank Li Cc: linux-i3c, devicetree, linux-kernel, meaganlloyd, git, Shubham Patil The DW master rejects private SDR transfers larger than caps.datafifodepth with -EOPNOTSUPP. Targets often report MRL/MWL values larger than that FIFO, so the core stores limits the controller cannot meet. After a successful GETMRL/GETMWL, issue Direct SETMRL/SETMWL to the same target with lengths capped to the data FIFO (in bytes), then rewrite the GET payload so the core keeps the same values. Only update the GET buffer once SET is acked, so a failed SET does not leave the core and the target disagreeing. GETMRL is variable length: the optional third byte is max IBI payload and is only present if the target returned it. Clamp that IBI byte to the IBI queue depth from QUEUE_SIZE_CAPABILITY.IBI_BUF_SIZE (bits 19:16 at 0xe8, encoded as 2^(n+1) dwords). Rename the unused EXTENDED_CAPABILITY macro at 0xe8 to the databook name QUEUE_SIZE_CAPABILITY. Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com> --- drivers/i3c/master/dw-i3c-master.c | 149 ++++++++++++++++++++++++++++- drivers/i3c/master/dw-i3c-master.h | 1 + 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index 4563d8761ba0..51defcb57761 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -203,7 +203,13 @@ #define BUS_IDLE_TIMING 0xd8 #define I3C_VER_ID 0xe0 #define I3C_VER_TYPE 0xe4 -#define EXTENDED_CAPABILITY 0xe8 +#define QUEUE_SIZE_CAPABILITY 0xe8 +#define QUEUE_SIZE_CAPABILITY_IBI_BUF(x) (((x) & GENMASK(19, 16)) >> 16) +/* + * IBI_BUF_SIZE is encoded as 2^(field + 1) dwords: the smallest buffer is + * 2 dwords and each increment of the field doubles the depth. + */ +#define QUEUE_SIZE_IBI_BUF_MIN_DWORDS 2 #define SLAVE_CONFIG 0xec #define DYN_ADDR_LO_MASK GENMASK(4, 0) @@ -844,6 +850,130 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc) return ret; } +/* + * Cap the limits a target reported through GETMRL to what this controller can + * actually transfer, so the core never asks for a private read the data FIFO + * cannot hold. The optional IBI payload byte is capped to the IBI queue depth + * instead; since that byte is a u8, the IBI cap only ever applies to + * controllers whose IBI queue is smaller than 255 bytes. + * + * Direct SETMRL is optional, so a target may implement GETMRL and NACK the SET. + * Clamp the values handed back to the core either way: a failed SET only means + * the target keeps its own larger limit, which is harmless as long as the core + * stays within ours. + */ +static int dw_i3c_master_clamp_mrl(struct dw_i3c_master *master, + struct i3c_ccc_cmd *ccc) +{ + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); + u32 max_ibi_bytes = master->caps.ibififodepth * sizeof(u32); + u16 actual_len = ccc->dests[0].payload.actual_len; + struct i3c_ccc_cmd_dest set_dest = { }; + struct i3c_ccc_cmd set_cmd = { }; + struct i3c_ccc_mrl set_mrl; + struct i3c_ccc_mrl *mrl; + bool clamp_ibi = false; + bool clamp_read; + u8 ibi_len = 0; + u16 read_len; + int ret; + + /* Need at least the 2-byte max read length field to act on. */ + if (actual_len < 2) + return 0; + + mrl = ccc->dests[0].payload.data; + read_len = be16_to_cpu(mrl->read_len); + clamp_read = read_len > max_fifo_bytes; + + /* Optional third byte is valid only if the target returned it. */ + if (actual_len > 2) { + ibi_len = mrl->ibi_len; + clamp_ibi = max_ibi_bytes && ibi_len > max_ibi_bytes; + } + + if (!clamp_read && !clamp_ibi) + return 0; + + set_mrl.read_len = cpu_to_be16(clamp_read ? max_fifo_bytes : read_len); + if (actual_len > 2) + set_mrl.ibi_len = clamp_ibi ? max_ibi_bytes : ibi_len; + + set_dest.addr = ccc->dests[0].addr; + set_dest.payload.data = &set_mrl; + set_dest.payload.len = actual_len; + + set_cmd.rnw = 0; + set_cmd.id = I3C_CCC_SETMRL(false); + set_cmd.ndests = 1; + set_cmd.dests = &set_dest; + + ret = dw_i3c_ccc_set(master, &set_cmd); + if (ret) + dev_dbg(&master->base.dev, + "SETMRL not accepted by target: %d\n", ret); + + if (clamp_read) { + mrl->read_len = cpu_to_be16(max_fifo_bytes); + dev_dbg(&master->base.dev, + "clamped target MRL from %u to %u bytes (FIFO depth limit)\n", + read_len, max_fifo_bytes); + } + if (clamp_ibi) { + mrl->ibi_len = max_ibi_bytes; + dev_dbg(&master->base.dev, + "clamped target IBI len from %u to %u bytes (IBI buffer limit)\n", + ibi_len, max_ibi_bytes); + } + + return 0; +} + +/* Same contract as dw_i3c_master_clamp_mrl(), for the write direction. */ +static int dw_i3c_master_clamp_mwl(struct dw_i3c_master *master, + struct i3c_ccc_cmd *ccc) +{ + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); + struct i3c_ccc_cmd_dest set_dest = { }; + struct i3c_ccc_cmd set_cmd = { }; + struct i3c_ccc_mwl set_mwl; + struct i3c_ccc_mwl *mwl; + u16 write_len; + int ret; + + if (ccc->dests[0].payload.actual_len < 2) + return 0; + + mwl = ccc->dests[0].payload.data; + write_len = be16_to_cpu(mwl->len); + + if (write_len <= max_fifo_bytes) + return 0; + + set_mwl.len = cpu_to_be16(max_fifo_bytes); + + set_dest.addr = ccc->dests[0].addr; + set_dest.payload.data = &set_mwl; + set_dest.payload.len = sizeof(set_mwl); + + set_cmd.rnw = 0; + set_cmd.id = I3C_CCC_SETMWL(false); + set_cmd.ndests = 1; + set_cmd.dests = &set_dest; + + ret = dw_i3c_ccc_set(master, &set_cmd); + if (ret) + dev_dbg(&master->base.dev, + "SETMWL not accepted by target: %d\n", ret); + + mwl->len = cpu_to_be16(max_fifo_bytes); + dev_dbg(&master->base.dev, + "clamped target MWL from %u to %u bytes (FIFO depth limit)\n", + write_len, max_fifo_bytes); + + return 0; +} + static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, struct i3c_ccc_cmd *ccc) { @@ -866,6 +996,18 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, else ret = dw_i3c_ccc_set(master, ccc); + /* + * Clamp GETMRL/GETMWL responses to the data FIFO depth, and the + * optional GETMRL IBI byte to the IBI queue depth. The GET itself has + * already succeeded, so its result is never overridden here. + */ + if (!ret && ccc->rnw) { + if (ccc->id == I3C_CCC_GETMRL) + dw_i3c_master_clamp_mrl(master, ccc); + else if (ccc->id == I3C_CCC_GETMWL) + dw_i3c_master_clamp_mwl(master, ccc); + } + pm_runtime_put_autosuspend(master->dev); return ret; } @@ -1728,6 +1870,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master, ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL); master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret); + /* Read the IBI data buffer size advertised by the controller. */ + ret = readl(master->regs + QUEUE_SIZE_CAPABILITY); + master->caps.ibififodepth = QUEUE_SIZE_IBI_BUF_MIN_DWORDS << + QUEUE_SIZE_CAPABILITY_IBI_BUF(ret); + ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER); master->datstartaddr = ret; master->maxdevs = ret >> 16; diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h index 17ad817d1f8e..54c3912374c8 100644 --- a/drivers/i3c/master/dw-i3c-master.h +++ b/drivers/i3c/master/dw-i3c-master.h @@ -15,6 +15,7 @@ struct dw_i3c_master_caps { u8 cmdfifodepth; u8 datafifodepth; + u32 ibififodepth; }; struct dw_i3c_dat_entry { -- 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits 2026-09-08 10:27 [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits Shubham Patil @ 2026-09-10 18:34 ` Frank Li 2026-09-24 4:55 ` Patil, Shubham Sanjay 0 siblings, 1 reply; 4+ messages in thread From: Frank Li @ 2026-09-10 18:34 UTC (permalink / raw) To: Shubham Patil Cc: Alexandre Belloni, Frank Li, linux-i3c, devicetree, linux-kernel, meaganlloyd, git On Tue, Sep 08, 2026 at 03:57:24PM +0530, Shubham Patil wrote: > The DW master rejects private SDR transfers larger than > caps.datafifodepth with -EOPNOTSUPP. Targets often report MRL/MWL > values larger than that FIFO, so the core stores limits the controller > cannot meet. > > After a successful GETMRL/GETMWL, issue Direct SETMRL/SETMWL to the > same target with lengths capped to the data FIFO (in bytes), then > rewrite the GET payload so the core keeps the same values. Only update > the GET buffer once SET is acked, so a failed SET does not leave the > core and the target disagreeing. I think i3c device driver should know these information choose min value dring each xfer. even though you set devcie's MRL/MXL, device driver still issue a longer transfer. Frank > > GETMRL is variable length: the optional third byte is max IBI payload > and is only present if the target returned it. Clamp that IBI byte to > the IBI queue depth from QUEUE_SIZE_CAPABILITY.IBI_BUF_SIZE (bits 19:16 > at 0xe8, encoded as 2^(n+1) dwords). > > Rename the unused EXTENDED_CAPABILITY macro at 0xe8 to the databook > name QUEUE_SIZE_CAPABILITY. > > Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com> > --- > drivers/i3c/master/dw-i3c-master.c | 149 ++++++++++++++++++++++++++++- > drivers/i3c/master/dw-i3c-master.h | 1 + > 2 files changed, 149 insertions(+), 1 deletion(-) > > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c > index 4563d8761ba0..51defcb57761 100644 > --- a/drivers/i3c/master/dw-i3c-master.c > +++ b/drivers/i3c/master/dw-i3c-master.c > @@ -203,7 +203,13 @@ > #define BUS_IDLE_TIMING 0xd8 > #define I3C_VER_ID 0xe0 > #define I3C_VER_TYPE 0xe4 > -#define EXTENDED_CAPABILITY 0xe8 > +#define QUEUE_SIZE_CAPABILITY 0xe8 > +#define QUEUE_SIZE_CAPABILITY_IBI_BUF(x) (((x) & GENMASK(19, 16)) >> 16) > +/* > + * IBI_BUF_SIZE is encoded as 2^(field + 1) dwords: the smallest buffer is > + * 2 dwords and each increment of the field doubles the depth. > + */ > +#define QUEUE_SIZE_IBI_BUF_MIN_DWORDS 2 > #define SLAVE_CONFIG 0xec > > #define DYN_ADDR_LO_MASK GENMASK(4, 0) > @@ -844,6 +850,130 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc) > return ret; > } > > +/* > + * Cap the limits a target reported through GETMRL to what this controller can > + * actually transfer, so the core never asks for a private read the data FIFO > + * cannot hold. The optional IBI payload byte is capped to the IBI queue depth > + * instead; since that byte is a u8, the IBI cap only ever applies to > + * controllers whose IBI queue is smaller than 255 bytes. > + * > + * Direct SETMRL is optional, so a target may implement GETMRL and NACK the SET. > + * Clamp the values handed back to the core either way: a failed SET only means > + * the target keeps its own larger limit, which is harmless as long as the core > + * stays within ours. > + */ > +static int dw_i3c_master_clamp_mrl(struct dw_i3c_master *master, > + struct i3c_ccc_cmd *ccc) > +{ > + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); > + u32 max_ibi_bytes = master->caps.ibififodepth * sizeof(u32); > + u16 actual_len = ccc->dests[0].payload.actual_len; > + struct i3c_ccc_cmd_dest set_dest = { }; > + struct i3c_ccc_cmd set_cmd = { }; > + struct i3c_ccc_mrl set_mrl; > + struct i3c_ccc_mrl *mrl; > + bool clamp_ibi = false; > + bool clamp_read; > + u8 ibi_len = 0; > + u16 read_len; > + int ret; > + > + /* Need at least the 2-byte max read length field to act on. */ > + if (actual_len < 2) > + return 0; > + > + mrl = ccc->dests[0].payload.data; > + read_len = be16_to_cpu(mrl->read_len); > + clamp_read = read_len > max_fifo_bytes; > + > + /* Optional third byte is valid only if the target returned it. */ > + if (actual_len > 2) { > + ibi_len = mrl->ibi_len; > + clamp_ibi = max_ibi_bytes && ibi_len > max_ibi_bytes; > + } > + > + if (!clamp_read && !clamp_ibi) > + return 0; > + > + set_mrl.read_len = cpu_to_be16(clamp_read ? max_fifo_bytes : read_len); > + if (actual_len > 2) > + set_mrl.ibi_len = clamp_ibi ? max_ibi_bytes : ibi_len; > + > + set_dest.addr = ccc->dests[0].addr; > + set_dest.payload.data = &set_mrl; > + set_dest.payload.len = actual_len; > + > + set_cmd.rnw = 0; > + set_cmd.id = I3C_CCC_SETMRL(false); > + set_cmd.ndests = 1; > + set_cmd.dests = &set_dest; > + > + ret = dw_i3c_ccc_set(master, &set_cmd); > + if (ret) > + dev_dbg(&master->base.dev, > + "SETMRL not accepted by target: %d\n", ret); > + > + if (clamp_read) { > + mrl->read_len = cpu_to_be16(max_fifo_bytes); > + dev_dbg(&master->base.dev, > + "clamped target MRL from %u to %u bytes (FIFO depth limit)\n", > + read_len, max_fifo_bytes); > + } > + if (clamp_ibi) { > + mrl->ibi_len = max_ibi_bytes; > + dev_dbg(&master->base.dev, > + "clamped target IBI len from %u to %u bytes (IBI buffer limit)\n", > + ibi_len, max_ibi_bytes); > + } > + > + return 0; > +} > + > +/* Same contract as dw_i3c_master_clamp_mrl(), for the write direction. */ > +static int dw_i3c_master_clamp_mwl(struct dw_i3c_master *master, > + struct i3c_ccc_cmd *ccc) > +{ > + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); > + struct i3c_ccc_cmd_dest set_dest = { }; > + struct i3c_ccc_cmd set_cmd = { }; > + struct i3c_ccc_mwl set_mwl; > + struct i3c_ccc_mwl *mwl; > + u16 write_len; > + int ret; > + > + if (ccc->dests[0].payload.actual_len < 2) > + return 0; > + > + mwl = ccc->dests[0].payload.data; > + write_len = be16_to_cpu(mwl->len); > + > + if (write_len <= max_fifo_bytes) > + return 0; > + > + set_mwl.len = cpu_to_be16(max_fifo_bytes); > + > + set_dest.addr = ccc->dests[0].addr; > + set_dest.payload.data = &set_mwl; > + set_dest.payload.len = sizeof(set_mwl); > + > + set_cmd.rnw = 0; > + set_cmd.id = I3C_CCC_SETMWL(false); > + set_cmd.ndests = 1; > + set_cmd.dests = &set_dest; > + > + ret = dw_i3c_ccc_set(master, &set_cmd); > + if (ret) > + dev_dbg(&master->base.dev, > + "SETMWL not accepted by target: %d\n", ret); > + > + mwl->len = cpu_to_be16(max_fifo_bytes); > + dev_dbg(&master->base.dev, > + "clamped target MWL from %u to %u bytes (FIFO depth limit)\n", > + write_len, max_fifo_bytes); > + > + return 0; > +} > + > static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, > struct i3c_ccc_cmd *ccc) > { > @@ -866,6 +996,18 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, > else > ret = dw_i3c_ccc_set(master, ccc); > > + /* > + * Clamp GETMRL/GETMWL responses to the data FIFO depth, and the > + * optional GETMRL IBI byte to the IBI queue depth. The GET itself has > + * already succeeded, so its result is never overridden here. > + */ > + if (!ret && ccc->rnw) { > + if (ccc->id == I3C_CCC_GETMRL) > + dw_i3c_master_clamp_mrl(master, ccc); > + else if (ccc->id == I3C_CCC_GETMWL) > + dw_i3c_master_clamp_mwl(master, ccc); > + } > + > pm_runtime_put_autosuspend(master->dev); > return ret; > } > @@ -1728,6 +1870,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master, > ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL); > master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret); > > + /* Read the IBI data buffer size advertised by the controller. */ > + ret = readl(master->regs + QUEUE_SIZE_CAPABILITY); > + master->caps.ibififodepth = QUEUE_SIZE_IBI_BUF_MIN_DWORDS << > + QUEUE_SIZE_CAPABILITY_IBI_BUF(ret); > + > ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER); > master->datstartaddr = ret; > master->maxdevs = ret >> 16; > diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h > index 17ad817d1f8e..54c3912374c8 100644 > --- a/drivers/i3c/master/dw-i3c-master.h > +++ b/drivers/i3c/master/dw-i3c-master.h > @@ -15,6 +15,7 @@ > struct dw_i3c_master_caps { > u8 cmdfifodepth; > u8 datafifodepth; > + u32 ibififodepth; > }; > > struct dw_i3c_dat_entry { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits 2026-09-10 18:34 ` Frank Li @ 2026-09-24 4:55 ` Patil, Shubham Sanjay 2026-09-24 15:27 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Patil, Shubham Sanjay @ 2026-09-24 4:55 UTC (permalink / raw) To: Frank Li, Shubham Patil Cc: Alexandre Belloni, Frank Li, linux-i3c, devicetree, linux-kernel, meaganlloyd, git On 9/11/2026 12:04 AM, Frank Li wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Tue, Sep 08, 2026 at 03:57:24PM +0530, Shubham Patil wrote: >> The DW master rejects private SDR transfers larger than >> caps.datafifodepth with -EOPNOTSUPP. Targets often report MRL/MWL >> values larger than that FIFO, so the core stores limits the controller >> cannot meet. >> >> After a successful GETMRL/GETMWL, issue Direct SETMRL/SETMWL to the >> same target with lengths capped to the data FIFO (in bytes), then >> rewrite the GET payload so the core keeps the same values. Only update >> the GET buffer once SET is acked, so a failed SET does not leave the >> core and the target disagreeing. > > I think i3c device driver should know these information choose > min value dring each xfer. even though you set devcie's MRL/MXL, device > driver still issue a longer transfer. > > Frank Understood - I will drop the SETMRL/SETMWL and stop rewriting the GET payload, and instead expose the controller limit so the min is taken per transfer. Two questions on how you want that done: 1) Where should the min be taken? a) In the core: the controller driver sets max_read_len / max_write_len / max_ibi_len in struct i3c_master_controller, and the core caps i3c_device_info to min(target, controller) after GETMRL/GETMWL. Device drivers then use i3c_device_get_info() as-is and cannot forget. b) In each device driver: the core keeps reporting the raw target values, and drivers do the min themselves. 2) Either way, a driver that ignores these limits still gets -EOPNOTSUPP from dw_i3c_master_i3c_xfers() when the transfer does not fit the data FIFO. Should the driver keep returning that, or would you consider splitting an oversized private SDR transfer into FIFO-sized chunks in the controller driver? My understanding is no - splitting changes what the target sees on the bus - but I want to be sure before v2. Thanks, Shubham> >> >> GETMRL is variable length: the optional third byte is max IBI payload >> and is only present if the target returned it. Clamp that IBI byte to >> the IBI queue depth from QUEUE_SIZE_CAPABILITY.IBI_BUF_SIZE (bits 19:16 >> at 0xe8, encoded as 2^(n+1) dwords). >> >> Rename the unused EXTENDED_CAPABILITY macro at 0xe8 to the databook >> name QUEUE_SIZE_CAPABILITY. >> >> Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com> >> --- >> drivers/i3c/master/dw-i3c-master.c | 149 ++++++++++++++++++++++++++++- >> drivers/i3c/master/dw-i3c-master.h | 1 + >> 2 files changed, 149 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c >> index 4563d8761ba0..51defcb57761 100644 >> --- a/drivers/i3c/master/dw-i3c-master.c >> +++ b/drivers/i3c/master/dw-i3c-master.c >> @@ -203,7 +203,13 @@ >> #define BUS_IDLE_TIMING 0xd8 >> #define I3C_VER_ID 0xe0 >> #define I3C_VER_TYPE 0xe4 >> -#define EXTENDED_CAPABILITY 0xe8 >> +#define QUEUE_SIZE_CAPABILITY 0xe8 >> +#define QUEUE_SIZE_CAPABILITY_IBI_BUF(x) (((x) & GENMASK(19, 16)) >> 16) >> +/* >> + * IBI_BUF_SIZE is encoded as 2^(field + 1) dwords: the smallest buffer is >> + * 2 dwords and each increment of the field doubles the depth. >> + */ >> +#define QUEUE_SIZE_IBI_BUF_MIN_DWORDS 2 >> #define SLAVE_CONFIG 0xec >> >> #define DYN_ADDR_LO_MASK GENMASK(4, 0) >> @@ -844,6 +850,130 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc) >> return ret; >> } >> >> +/* >> + * Cap the limits a target reported through GETMRL to what this controller can >> + * actually transfer, so the core never asks for a private read the data FIFO >> + * cannot hold. The optional IBI payload byte is capped to the IBI queue depth >> + * instead; since that byte is a u8, the IBI cap only ever applies to >> + * controllers whose IBI queue is smaller than 255 bytes. >> + * >> + * Direct SETMRL is optional, so a target may implement GETMRL and NACK the SET. >> + * Clamp the values handed back to the core either way: a failed SET only means >> + * the target keeps its own larger limit, which is harmless as long as the core >> + * stays within ours. >> + */ >> +static int dw_i3c_master_clamp_mrl(struct dw_i3c_master *master, >> + struct i3c_ccc_cmd *ccc) >> +{ >> + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); >> + u32 max_ibi_bytes = master->caps.ibififodepth * sizeof(u32); >> + u16 actual_len = ccc->dests[0].payload.actual_len; >> + struct i3c_ccc_cmd_dest set_dest = { }; >> + struct i3c_ccc_cmd set_cmd = { }; >> + struct i3c_ccc_mrl set_mrl; >> + struct i3c_ccc_mrl *mrl; >> + bool clamp_ibi = false; >> + bool clamp_read; >> + u8 ibi_len = 0; >> + u16 read_len; >> + int ret; >> + >> + /* Need at least the 2-byte max read length field to act on. */ >> + if (actual_len < 2) >> + return 0; >> + >> + mrl = ccc->dests[0].payload.data; >> + read_len = be16_to_cpu(mrl->read_len); >> + clamp_read = read_len > max_fifo_bytes; >> + >> + /* Optional third byte is valid only if the target returned it. */ >> + if (actual_len > 2) { >> + ibi_len = mrl->ibi_len; >> + clamp_ibi = max_ibi_bytes && ibi_len > max_ibi_bytes; >> + } >> + >> + if (!clamp_read && !clamp_ibi) >> + return 0; >> + >> + set_mrl.read_len = cpu_to_be16(clamp_read ? max_fifo_bytes : read_len); >> + if (actual_len > 2) >> + set_mrl.ibi_len = clamp_ibi ? max_ibi_bytes : ibi_len; >> + >> + set_dest.addr = ccc->dests[0].addr; >> + set_dest.payload.data = &set_mrl; >> + set_dest.payload.len = actual_len; >> + >> + set_cmd.rnw = 0; >> + set_cmd.id = I3C_CCC_SETMRL(false); >> + set_cmd.ndests = 1; >> + set_cmd.dests = &set_dest; >> + >> + ret = dw_i3c_ccc_set(master, &set_cmd); >> + if (ret) >> + dev_dbg(&master->base.dev, >> + "SETMRL not accepted by target: %d\n", ret); >> + >> + if (clamp_read) { >> + mrl->read_len = cpu_to_be16(max_fifo_bytes); >> + dev_dbg(&master->base.dev, >> + "clamped target MRL from %u to %u bytes (FIFO depth limit)\n", >> + read_len, max_fifo_bytes); >> + } >> + if (clamp_ibi) { >> + mrl->ibi_len = max_ibi_bytes; >> + dev_dbg(&master->base.dev, >> + "clamped target IBI len from %u to %u bytes (IBI buffer limit)\n", >> + ibi_len, max_ibi_bytes); >> + } >> + >> + return 0; >> +} >> + >> +/* Same contract as dw_i3c_master_clamp_mrl(), for the write direction. */ >> +static int dw_i3c_master_clamp_mwl(struct dw_i3c_master *master, >> + struct i3c_ccc_cmd *ccc) >> +{ >> + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); >> + struct i3c_ccc_cmd_dest set_dest = { }; >> + struct i3c_ccc_cmd set_cmd = { }; >> + struct i3c_ccc_mwl set_mwl; >> + struct i3c_ccc_mwl *mwl; >> + u16 write_len; >> + int ret; >> + >> + if (ccc->dests[0].payload.actual_len < 2) >> + return 0; >> + >> + mwl = ccc->dests[0].payload.data; >> + write_len = be16_to_cpu(mwl->len); >> + >> + if (write_len <= max_fifo_bytes) >> + return 0; >> + >> + set_mwl.len = cpu_to_be16(max_fifo_bytes); >> + >> + set_dest.addr = ccc->dests[0].addr; >> + set_dest.payload.data = &set_mwl; >> + set_dest.payload.len = sizeof(set_mwl); >> + >> + set_cmd.rnw = 0; >> + set_cmd.id = I3C_CCC_SETMWL(false); >> + set_cmd.ndests = 1; >> + set_cmd.dests = &set_dest; >> + >> + ret = dw_i3c_ccc_set(master, &set_cmd); >> + if (ret) >> + dev_dbg(&master->base.dev, >> + "SETMWL not accepted by target: %d\n", ret); >> + >> + mwl->len = cpu_to_be16(max_fifo_bytes); >> + dev_dbg(&master->base.dev, >> + "clamped target MWL from %u to %u bytes (FIFO depth limit)\n", >> + write_len, max_fifo_bytes); >> + >> + return 0; >> +} >> + >> static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, >> struct i3c_ccc_cmd *ccc) >> { >> @@ -866,6 +996,18 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, >> else >> ret = dw_i3c_ccc_set(master, ccc); >> >> + /* >> + * Clamp GETMRL/GETMWL responses to the data FIFO depth, and the >> + * optional GETMRL IBI byte to the IBI queue depth. The GET itself has >> + * already succeeded, so its result is never overridden here. >> + */ >> + if (!ret && ccc->rnw) { >> + if (ccc->id == I3C_CCC_GETMRL) >> + dw_i3c_master_clamp_mrl(master, ccc); >> + else if (ccc->id == I3C_CCC_GETMWL) >> + dw_i3c_master_clamp_mwl(master, ccc); >> + } >> + >> pm_runtime_put_autosuspend(master->dev); >> return ret; >> } >> @@ -1728,6 +1870,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master, >> ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL); >> master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret); >> >> + /* Read the IBI data buffer size advertised by the controller. */ >> + ret = readl(master->regs + QUEUE_SIZE_CAPABILITY); >> + master->caps.ibififodepth = QUEUE_SIZE_IBI_BUF_MIN_DWORDS << >> + QUEUE_SIZE_CAPABILITY_IBI_BUF(ret); >> + >> ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER); >> master->datstartaddr = ret; >> master->maxdevs = ret >> 16; >> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h >> index 17ad817d1f8e..54c3912374c8 100644 >> --- a/drivers/i3c/master/dw-i3c-master.h >> +++ b/drivers/i3c/master/dw-i3c-master.h >> @@ -15,6 +15,7 @@ >> struct dw_i3c_master_caps { >> u8 cmdfifodepth; >> u8 datafifodepth; >> + u32 ibififodepth; >> }; >> >> struct dw_i3c_dat_entry { >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits 2026-09-24 4:55 ` Patil, Shubham Sanjay @ 2026-09-24 15:27 ` Frank Li 0 siblings, 0 replies; 4+ messages in thread From: Frank Li @ 2026-09-24 15:27 UTC (permalink / raw) To: Patil, Shubham Sanjay Cc: Shubham Patil, Alexandre Belloni, Frank Li, linux-i3c, devicetree, linux-kernel, meaganlloyd, git On Thu, Sep 24, 2026 at 10:25:44AM +0530, Patil, Shubham Sanjay wrote: > > > On 9/11/2026 12:04 AM, Frank Li wrote: > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > > > > On Tue, Sep 08, 2026 at 03:57:24PM +0530, Shubham Patil wrote: > > > The DW master rejects private SDR transfers larger than > > > caps.datafifodepth with -EOPNOTSUPP. Targets often report MRL/MWL > > > values larger than that FIFO, so the core stores limits the controller > > > cannot meet. > > > > > > After a successful GETMRL/GETMWL, issue Direct SETMRL/SETMWL to the > > > same target with lengths capped to the data FIFO (in bytes), then > > > rewrite the GET payload so the core keeps the same values. Only update > > > the GET buffer once SET is acked, so a failed SET does not leave the > > > core and the target disagreeing. > > > > I think i3c device driver should know these information choose > > min value dring each xfer. even though you set devcie's MRL/MXL, device > > driver still issue a longer transfer. > > > > Frank > > Understood - I will drop the SETMRL/SETMWL and stop rewriting the GET > payload, and instead expose the controller limit so the min is taken > per transfer. Two questions on how you want that done: > 1) Where should the min be taken? > a) In the core: the controller driver sets max_read_len / > max_write_len / max_ibi_len in struct i3c_master_controller, and > the core caps i3c_device_info to min(target, controller) after > GETMRL/GETMWL. Device drivers then use i3c_device_get_info() > as-is and cannot forget. > b) In each device driver: the core keeps reporting the raw target > values, and drivers do the min themselves. We can provide APIs for device driver to get whole data path required max_read/write_len. > 2) Either way, a driver that ignores these limits still gets > -EOPNOTSUPP from dw_i3c_master_i3c_xfers() when the transfer does > not fit the data FIFO. Should the driver keep returning that, or > would you consider splitting an oversized private SDR transfer into > FIFO-sized chunks in the controller driver? My understanding is > no - splitting changes what the target sees on the bus - but I > want to be sure before v2. the decision about split transfer should be decided by device drivers. Not all device treat two continue repeat START as continue write/read. Frank > > Thanks, > Shubham> > > > > > > GETMRL is variable length: the optional third byte is max IBI payload > > > and is only present if the target returned it. Clamp that IBI byte to > > > the IBI queue depth from QUEUE_SIZE_CAPABILITY.IBI_BUF_SIZE (bits 19:16 > > > at 0xe8, encoded as 2^(n+1) dwords). > > > > > > Rename the unused EXTENDED_CAPABILITY macro at 0xe8 to the databook > > > name QUEUE_SIZE_CAPABILITY. > > > > > > Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com> > > > --- > > > drivers/i3c/master/dw-i3c-master.c | 149 ++++++++++++++++++++++++++++- > > > drivers/i3c/master/dw-i3c-master.h | 1 + > > > 2 files changed, 149 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c > > > index 4563d8761ba0..51defcb57761 100644 > > > --- a/drivers/i3c/master/dw-i3c-master.c > > > +++ b/drivers/i3c/master/dw-i3c-master.c > > > @@ -203,7 +203,13 @@ > > > #define BUS_IDLE_TIMING 0xd8 > > > #define I3C_VER_ID 0xe0 > > > #define I3C_VER_TYPE 0xe4 > > > -#define EXTENDED_CAPABILITY 0xe8 > > > +#define QUEUE_SIZE_CAPABILITY 0xe8 > > > +#define QUEUE_SIZE_CAPABILITY_IBI_BUF(x) (((x) & GENMASK(19, 16)) >> 16) > > > +/* > > > + * IBI_BUF_SIZE is encoded as 2^(field + 1) dwords: the smallest buffer is > > > + * 2 dwords and each increment of the field doubles the depth. > > > + */ > > > +#define QUEUE_SIZE_IBI_BUF_MIN_DWORDS 2 > > > #define SLAVE_CONFIG 0xec > > > > > > #define DYN_ADDR_LO_MASK GENMASK(4, 0) > > > @@ -844,6 +850,130 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc) > > > return ret; > > > } > > > > > > +/* > > > + * Cap the limits a target reported through GETMRL to what this controller can > > > + * actually transfer, so the core never asks for a private read the data FIFO > > > + * cannot hold. The optional IBI payload byte is capped to the IBI queue depth > > > + * instead; since that byte is a u8, the IBI cap only ever applies to > > > + * controllers whose IBI queue is smaller than 255 bytes. > > > + * > > > + * Direct SETMRL is optional, so a target may implement GETMRL and NACK the SET. > > > + * Clamp the values handed back to the core either way: a failed SET only means > > > + * the target keeps its own larger limit, which is harmless as long as the core > > > + * stays within ours. > > > + */ > > > +static int dw_i3c_master_clamp_mrl(struct dw_i3c_master *master, > > > + struct i3c_ccc_cmd *ccc) > > > +{ > > > + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); > > > + u32 max_ibi_bytes = master->caps.ibififodepth * sizeof(u32); > > > + u16 actual_len = ccc->dests[0].payload.actual_len; > > > + struct i3c_ccc_cmd_dest set_dest = { }; > > > + struct i3c_ccc_cmd set_cmd = { }; > > > + struct i3c_ccc_mrl set_mrl; > > > + struct i3c_ccc_mrl *mrl; > > > + bool clamp_ibi = false; > > > + bool clamp_read; > > > + u8 ibi_len = 0; > > > + u16 read_len; > > > + int ret; > > > + > > > + /* Need at least the 2-byte max read length field to act on. */ > > > + if (actual_len < 2) > > > + return 0; > > > + > > > + mrl = ccc->dests[0].payload.data; > > > + read_len = be16_to_cpu(mrl->read_len); > > > + clamp_read = read_len > max_fifo_bytes; > > > + > > > + /* Optional third byte is valid only if the target returned it. */ > > > + if (actual_len > 2) { > > > + ibi_len = mrl->ibi_len; > > > + clamp_ibi = max_ibi_bytes && ibi_len > max_ibi_bytes; > > > + } > > > + > > > + if (!clamp_read && !clamp_ibi) > > > + return 0; > > > + > > > + set_mrl.read_len = cpu_to_be16(clamp_read ? max_fifo_bytes : read_len); > > > + if (actual_len > 2) > > > + set_mrl.ibi_len = clamp_ibi ? max_ibi_bytes : ibi_len; > > > + > > > + set_dest.addr = ccc->dests[0].addr; > > > + set_dest.payload.data = &set_mrl; > > > + set_dest.payload.len = actual_len; > > > + > > > + set_cmd.rnw = 0; > > > + set_cmd.id = I3C_CCC_SETMRL(false); > > > + set_cmd.ndests = 1; > > > + set_cmd.dests = &set_dest; > > > + > > > + ret = dw_i3c_ccc_set(master, &set_cmd); > > > + if (ret) > > > + dev_dbg(&master->base.dev, > > > + "SETMRL not accepted by target: %d\n", ret); > > > + > > > + if (clamp_read) { > > > + mrl->read_len = cpu_to_be16(max_fifo_bytes); > > > + dev_dbg(&master->base.dev, > > > + "clamped target MRL from %u to %u bytes (FIFO depth limit)\n", > > > + read_len, max_fifo_bytes); > > > + } > > > + if (clamp_ibi) { > > > + mrl->ibi_len = max_ibi_bytes; > > > + dev_dbg(&master->base.dev, > > > + "clamped target IBI len from %u to %u bytes (IBI buffer limit)\n", > > > + ibi_len, max_ibi_bytes); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +/* Same contract as dw_i3c_master_clamp_mrl(), for the write direction. */ > > > +static int dw_i3c_master_clamp_mwl(struct dw_i3c_master *master, > > > + struct i3c_ccc_cmd *ccc) > > > +{ > > > + u16 max_fifo_bytes = master->caps.datafifodepth * sizeof(u32); > > > + struct i3c_ccc_cmd_dest set_dest = { }; > > > + struct i3c_ccc_cmd set_cmd = { }; > > > + struct i3c_ccc_mwl set_mwl; > > > + struct i3c_ccc_mwl *mwl; > > > + u16 write_len; > > > + int ret; > > > + > > > + if (ccc->dests[0].payload.actual_len < 2) > > > + return 0; > > > + > > > + mwl = ccc->dests[0].payload.data; > > > + write_len = be16_to_cpu(mwl->len); > > > + > > > + if (write_len <= max_fifo_bytes) > > > + return 0; > > > + > > > + set_mwl.len = cpu_to_be16(max_fifo_bytes); > > > + > > > + set_dest.addr = ccc->dests[0].addr; > > > + set_dest.payload.data = &set_mwl; > > > + set_dest.payload.len = sizeof(set_mwl); > > > + > > > + set_cmd.rnw = 0; > > > + set_cmd.id = I3C_CCC_SETMWL(false); > > > + set_cmd.ndests = 1; > > > + set_cmd.dests = &set_dest; > > > + > > > + ret = dw_i3c_ccc_set(master, &set_cmd); > > > + if (ret) > > > + dev_dbg(&master->base.dev, > > > + "SETMWL not accepted by target: %d\n", ret); > > > + > > > + mwl->len = cpu_to_be16(max_fifo_bytes); > > > + dev_dbg(&master->base.dev, > > > + "clamped target MWL from %u to %u bytes (FIFO depth limit)\n", > > > + write_len, max_fifo_bytes); > > > + > > > + return 0; > > > +} > > > + > > > static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, > > > struct i3c_ccc_cmd *ccc) > > > { > > > @@ -866,6 +996,18 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, > > > else > > > ret = dw_i3c_ccc_set(master, ccc); > > > > > > + /* > > > + * Clamp GETMRL/GETMWL responses to the data FIFO depth, and the > > > + * optional GETMRL IBI byte to the IBI queue depth. The GET itself has > > > + * already succeeded, so its result is never overridden here. > > > + */ > > > + if (!ret && ccc->rnw) { > > > + if (ccc->id == I3C_CCC_GETMRL) > > > + dw_i3c_master_clamp_mrl(master, ccc); > > > + else if (ccc->id == I3C_CCC_GETMWL) > > > + dw_i3c_master_clamp_mwl(master, ccc); > > > + } > > > + > > > pm_runtime_put_autosuspend(master->dev); > > > return ret; > > > } > > > @@ -1728,6 +1870,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master, > > > ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL); > > > master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret); > > > > > > + /* Read the IBI data buffer size advertised by the controller. */ > > > + ret = readl(master->regs + QUEUE_SIZE_CAPABILITY); > > > + master->caps.ibififodepth = QUEUE_SIZE_IBI_BUF_MIN_DWORDS << > > > + QUEUE_SIZE_CAPABILITY_IBI_BUF(ret); > > > + > > > ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER); > > > master->datstartaddr = ret; > > > master->maxdevs = ret >> 16; > > > diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h > > > index 17ad817d1f8e..54c3912374c8 100644 > > > --- a/drivers/i3c/master/dw-i3c-master.h > > > +++ b/drivers/i3c/master/dw-i3c-master.h > > > @@ -15,6 +15,7 @@ > > > struct dw_i3c_master_caps { > > > u8 cmdfifodepth; > > > u8 datafifodepth; > > > + u32 ibififodepth; > > > }; > > > > > > struct dw_i3c_dat_entry { > > > -- > > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-24 15:27 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-08 10:27 [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits Shubham Patil 2026-09-10 18:34 ` Frank Li 2026-09-24 4:55 ` Patil, Shubham Sanjay 2026-09-24 15:27 ` Frank Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®