| Message ID | 20230216062040.497815-1-jk@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i3c: dw: Use configured rate and bus mode for clock configuration | expand |
Hi Jeremy, Please find my comments bellow. On 2/16/23 06:20, Jeremy Kerr wrote: > We may have a non-typical i3c rate configured; use this (instead of > the fixed I3C_BUS_TYP_I3C_SCL_RATE) when calculating timing > characteristics. We can also expand the SCL high time based on the > presence/absence of i2c devices. > > Also, since we now have bus->mode, use it to determine whether we se up > the BUS_FREE_TIMING register; rather than re-reading > DEV_CTRL_I2C_SLAVE_PRESENT from hardware. > > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > --- > drivers/i3c/master/dw-i3c-master.c | 44 ++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c > index 51a8608203de..d73d57362b3b 100644 > --- a/drivers/i3c/master/dw-i3c-master.c > +++ b/drivers/i3c/master/dw-i3c-master.c > @@ -515,7 +515,8 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr) > dw_i3c_master_start_xfer_locked(master); > } > > -static int dw_i3c_clk_cfg(struct dw_i3c_master *master) > +static int dw_i3c_clk_cfg(struct dw_i3c_master *master, unsigned long i3c_rate, > + bool pure) > { > unsigned long core_rate, core_period; > u32 scl_timing; > @@ -527,31 +528,45 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master) > > core_period = DIV_ROUND_UP(1000000000, core_rate); > > - hcnt = DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period) - 1; > - if (hcnt < SCL_I3C_TIMING_CNT_MIN) > - hcnt = SCL_I3C_TIMING_CNT_MIN; > + /* 50% duty cycle */ > + hcnt = DIV_ROUND_UP(core_rate, i3c_rate * 2); > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_TYP_I3C_SCL_RATE) - hcnt; > - if (lcnt < SCL_I3C_TIMING_CNT_MIN) > - lcnt = SCL_I3C_TIMING_CNT_MIN; > + /* In shared mode, we limit t_high, so that i3c SCL signalling is > + * rejected by the i2c devices' spike filter */ > + if (!pure) > + hcnt = min_t(u8, hcnt, > + DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period)) - 1; You are assuming that t_high may be lower than 40ns, right? by the spec the max speed is 12.9MHz, don't think you need this min_t() here. > + > + hcnt = max_t(u8, hcnt, SCL_I3C_TIMING_CNT_MIN); I would branch this part into: if(pure) hcnt= ; else hcnt= ; think this way is more readable. > + > + lcnt = DIV_ROUND_UP(core_rate, i3c_rate) - hcnt; > + lcnt = max_t(u8, lcnt, SCL_I3C_TIMING_CNT_MIN); > > scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); > writel(scl_timing, master->regs + SCL_I3C_PP_TIMING); > > - if (!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_I2C_SLAVE_PRESENT)) > + if (pure) > writel(BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING); > > - lcnt = DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period); > + /* open drain mode requires a minimum of OD_MIN_NS */ My comment here would say why we need a higher OD time rather the minimum. > + lcnt = max_t(u8, lcnt, DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period)); > scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); > writel(scl_timing, master->regs + SCL_I3C_OD_TIMING); > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt; > + /* Timings for lower SDRx rates where specified by device MXDS values; > + * we limit these to the global max rate provided, which also prevents > + * weird duty cycles */ I think checkpatch complains with this comment format. I would suggest to use like in the rest of kernel. Unfortunately, we need to address the cases in which SDRx frequency is higher than bus->scl_rate.i3c. > + lcnt = max_t(u8, lcnt, > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt); The lcnt within max_t() function comes from SCL_I3C_OD_TIMING, correct? Have you checked this value for bus->scl_rate.i3c = 12.5MHz? > scl_timing = SCL_EXT_LCNT_1(lcnt); > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt; > + lcnt = max_t(u8, lcnt, > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt); > scl_timing |= SCL_EXT_LCNT_2(lcnt); > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt; > + lcnt = max_t(u8, lcnt, > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt); > scl_timing |= SCL_EXT_LCNT_3(lcnt); > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt; > + lcnt = max_t(u8, lcnt, > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt); what about to use a for loop and only do lcnt calculation if bus->scl_rate.i3c > I3C_BUS_SDRx_SCL_RATE ? > scl_timing |= SCL_EXT_LCNT_4(lcnt); > writel(scl_timing, master->regs + SCL_EXT_LCNT_TIMING); > > @@ -605,7 +620,8 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m) > return ret; > fallthrough; > case I3C_BUS_MODE_PURE: > - ret = dw_i3c_clk_cfg(master); > + ret = dw_i3c_clk_cfg(master, bus->scl_rate.i3c, > + bus->mode == I3C_BUS_MODE_PURE); > if (ret) > return ret; > break; Best regards, Vitor Soares
Hi Vitor, > Please find my comments bellow. Thanks for taking a look, and for the review. Some replies inline: > > + /* 50% duty cycle */ > > + hcnt = DIV_ROUND_UP(core_rate, i3c_rate * 2); > > > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_TYP_I3C_SCL_RATE) - hcnt; > > - if (lcnt < SCL_I3C_TIMING_CNT_MIN) > > - lcnt = SCL_I3C_TIMING_CNT_MIN; > > + /* In shared mode, we limit t_high, so that i3c SCL signalling is > > + * rejected by the i2c devices' spike filter */ > > + if (!pure) > > + hcnt = min_t(u8, hcnt, > > + DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period)) - 1; > > You are assuming that t_high may be lower than 40ns, right? > by the spec the max speed is 12.9MHz, don't think you need this min_t() here. This is to handle the case where t_high is *longer* than 41ns - from the 50% duty cycle calculation above, when using a lower bus frequency (lower than about 12.2MHz). Then, for mixed-mode (where we need to be compatible with i2c devices on the bus), we reduce this to the 41ns max (and then hcnt gets increased to fit the intended frequency). This is to match the definitions in Table 86 of the I3C spec: the 41ns maximum is only specified for the mixed bus case. > > + > > + hcnt = max_t(u8, hcnt, SCL_I3C_TIMING_CNT_MIN); > > I would branch this part into: > > if(pure) > > hcnt= ; > > else > > hcnt= ; > > think this way is more readable. Yeah, I've been debating min_t()/max_t() vs. conditionals; the conditionals are more verbose, but likely easier to interpret. I'll re-do all of these limits as conditionals. > > + > > + lcnt = DIV_ROUND_UP(core_rate, i3c_rate) - hcnt; > > + lcnt = max_t(u8, lcnt, SCL_I3C_TIMING_CNT_MIN); > > > > scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); > > writel(scl_timing, master->regs + SCL_I3C_PP_TIMING); > > > > - if (!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_I2C_SLAVE_PRESENT)) > > + if (pure) > > writel(BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING); > > > > - lcnt = DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period); > > + /* open drain mode requires a minimum of OD_MIN_NS */ > > My comment here would say why we need a higher OD time rather the minimum. OK, sounds good. > > + lcnt = max_t(u8, lcnt, DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period)); > > scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); > > writel(scl_timing, master->regs + SCL_I3C_OD_TIMING); > > > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt; > > + /* Timings for lower SDRx rates where specified by device MXDS values; > > + * we limit these to the global max rate provided, which also prevents > > + * weird duty cycles */ > > I think checkpatch complains with this comment format. I would suggest > to use like in the rest of kernel. Yep, will change. > Unfortunately, we need to address the cases in which SDRx frequency is > higher than bus->scl_rate.i3c. I'm not sure if I'm interpreting your comment correctly, but that's essentially what this is doing: limiting the SDRx frequencies, so that none is higher than bus->scl_rate.i3c. For example, if scl_rate is set at 5MHz, we should end up with: * default: 5MHz * SDR1: 5MHz * SDR2: 5MHz * SDR3: 4MHz * SDR4: 2MHz Or do you mean that we should be generating timing configurations that allow SDRx to be greater than the specified scl_rate.i3c? That would seem to defeat the purpose of the scl_rate override - a device trying to limit the rate through GETMXDS would result in the rate *increasing* from the default. Or something else? > > + lcnt = max_t(u8, lcnt, > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt); > > The lcnt within max_t() function comes from SCL_I3C_OD_TIMING, > correct? Have you checked this value for bus->scl_rate.i3c = 12.5MHz? Good catch, I assume that this should be based on the PP value, will update. > > scl_timing = SCL_EXT_LCNT_1(lcnt); > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt; > > + lcnt = max_t(u8, lcnt, > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt); > > scl_timing |= SCL_EXT_LCNT_2(lcnt); > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt; > > + lcnt = max_t(u8, lcnt, > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt); > > scl_timing |= SCL_EXT_LCNT_3(lcnt); > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt; > > + lcnt = max_t(u8, lcnt, > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt); > > what about to use a for loop and only do lcnt calculation if > > bus->scl_rate.i3c > I3C_BUS_SDRx_SCL_RATE ? I have intended for this to be the same as the existing calculations, just applying the limit of the global scl_rate. We could restructure as a for-loop (which I'd suggest splitting as a separate change, so that the calculation changes are more obvious), but it's going to get a bit weird with the macro usage there. Cheers, Jeremy
Hi Vitor, > > > scl_timing = SCL_EXT_LCNT_1(lcnt); > > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt; > > > + lcnt = max_t(u8, lcnt, > > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt); > > > scl_timing |= SCL_EXT_LCNT_2(lcnt); > > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt; > > > + lcnt = max_t(u8, lcnt, > > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt); > > > scl_timing |= SCL_EXT_LCNT_3(lcnt); > > > - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt; > > > + lcnt = max_t(u8, lcnt, > > > + DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt); > > > > what about to use a for loop and only do lcnt calculation if > > > > bus->scl_rate.i3c > I3C_BUS_SDRx_SCL_RATE ? > > I have intended for this to be the same as the existing calculations, > just applying the limit of the global scl_rate. > > We could restructure as a for-loop (which I'd suggest splitting as a > separate change, so that the calculation changes are more obvious), > but it's going to get a bit weird with the macro usage there. Actually, a for-loop isn't too bad: static const struct { unsigned int freq; unsigned int shift; } sdrs[] = { { I3C_BUS_SDR1_SCL_RATE, 0 }, { I3C_BUS_SDR2_SCL_RATE, 8 }, { I3C_BUS_SDR3_SCL_RATE, 16 }, { I3C_BUS_SDR4_SCL_RATE, 24 }, }; static int dw_i3c_clk_cfg(struct dw_i3c_master *master, unsigned long i3c_rate, bool pure) { /* ... */ /* * Timings for lower SDRx rates where specified by device MXDS values; * we limit these to the global max rate provided, which also prevents * weird duty cycles */ scl_timing = 0; for (i = 0; i < ARRAY_SIZE(sdrs); i++) { tmp = DIV_ROUND_UP(core_rate, sdrs[i].freq) & 0xff; if (tmp < lcnt) tmp = lcnt; scl_timing |= tmp << sdrs[i].shift; } writel(scl_timing, master->regs + SCL_EXT_LCNT_TIMING); } Is this what you were intending? Cheers, Jeremy
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index 51a8608203de..d73d57362b3b 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -515,7 +515,8 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr) dw_i3c_master_start_xfer_locked(master); } -static int dw_i3c_clk_cfg(struct dw_i3c_master *master) +static int dw_i3c_clk_cfg(struct dw_i3c_master *master, unsigned long i3c_rate, + bool pure) { unsigned long core_rate, core_period; u32 scl_timing; @@ -527,31 +528,45 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master) core_period = DIV_ROUND_UP(1000000000, core_rate); - hcnt = DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period) - 1; - if (hcnt < SCL_I3C_TIMING_CNT_MIN) - hcnt = SCL_I3C_TIMING_CNT_MIN; + /* 50% duty cycle */ + hcnt = DIV_ROUND_UP(core_rate, i3c_rate * 2); - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_TYP_I3C_SCL_RATE) - hcnt; - if (lcnt < SCL_I3C_TIMING_CNT_MIN) - lcnt = SCL_I3C_TIMING_CNT_MIN; + /* In shared mode, we limit t_high, so that i3c SCL signalling is + * rejected by the i2c devices' spike filter */ + if (!pure) + hcnt = min_t(u8, hcnt, + DIV_ROUND_UP(I3C_BUS_THIGH_MAX_NS, core_period)) - 1; + + hcnt = max_t(u8, hcnt, SCL_I3C_TIMING_CNT_MIN); + + lcnt = DIV_ROUND_UP(core_rate, i3c_rate) - hcnt; + lcnt = max_t(u8, lcnt, SCL_I3C_TIMING_CNT_MIN); scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); writel(scl_timing, master->regs + SCL_I3C_PP_TIMING); - if (!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_I2C_SLAVE_PRESENT)) + if (pure) writel(BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING); - lcnt = DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period); + /* open drain mode requires a minimum of OD_MIN_NS */ + lcnt = max_t(u8, lcnt, DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period)); scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt); writel(scl_timing, master->regs + SCL_I3C_OD_TIMING); - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt; + /* Timings for lower SDRx rates where specified by device MXDS values; + * we limit these to the global max rate provided, which also prevents + * weird duty cycles */ + lcnt = max_t(u8, lcnt, + DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt); scl_timing = SCL_EXT_LCNT_1(lcnt); - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt; + lcnt = max_t(u8, lcnt, + DIV_ROUND_UP(core_rate, I3C_BUS_SDR2_SCL_RATE) - hcnt); scl_timing |= SCL_EXT_LCNT_2(lcnt); - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt; + lcnt = max_t(u8, lcnt, + DIV_ROUND_UP(core_rate, I3C_BUS_SDR3_SCL_RATE) - hcnt); scl_timing |= SCL_EXT_LCNT_3(lcnt); - lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt; + lcnt = max_t(u8, lcnt, + DIV_ROUND_UP(core_rate, I3C_BUS_SDR4_SCL_RATE) - hcnt); scl_timing |= SCL_EXT_LCNT_4(lcnt); writel(scl_timing, master->regs + SCL_EXT_LCNT_TIMING); @@ -605,7 +620,8 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m) return ret; fallthrough; case I3C_BUS_MODE_PURE: - ret = dw_i3c_clk_cfg(master); + ret = dw_i3c_clk_cfg(master, bus->scl_rate.i3c, + bus->mode == I3C_BUS_MODE_PURE); if (ret) return ret; break;
We may have a non-typical i3c rate configured; use this (instead of the fixed I3C_BUS_TYP_I3C_SCL_RATE) when calculating timing characteristics. We can also expand the SCL high time based on the presence/absence of i2c devices. Also, since we now have bus->mode, use it to determine whether we se up the BUS_FREE_TIMING register; rather than re-reading DEV_CTRL_I2C_SLAVE_PRESENT from hardware. Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- drivers/i3c/master/dw-i3c-master.c | 44 ++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 14 deletions(-)