| Message ID | 20240226165321.91976-26-eajames@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | fsi: Interrupt support | expand |
Hi Eddie, > @@ -689,6 +692,20 @@ static int fsi_i2c_probe(struct device *dev) > mutex_init(&i2c->lock); > i2c->fsi = to_fsi_dev(dev); > INIT_LIST_HEAD(&i2c->ports); > + i2c->clock_div = I2C_DEFAULT_CLK_DIV; > + > + lbus = fsi_device_local_bus_frequency(i2c->fsi); > + if (lbus) { > + u32 clock = I2C_DEFAULT_CLK_RATE; I don't see the need for initialization. > + > + if (!device_property_read_u32(dev, "clock-frequency", &clock)) { > + if (!clock) > + clock = I2C_DEFAULT_CLK_RATE; > + } no need for brackets. > + > + // i2c clock rate = local bus clock / (4 * (i2c clock div + 1)) You forgot to remove this. Andi > + i2c->clock_div = (((lbus + (clock - 1)) / clock) / 4) - 1; > + } > > rc = fsi_i2c_dev_init(i2c); > if (rc) > -- > 2.39.3 >
On 4/15/24 17:11, Andi Shyti wrote: > Hi Eddie, > >> @@ -689,6 +692,20 @@ static int fsi_i2c_probe(struct device *dev) >> mutex_init(&i2c->lock); >> i2c->fsi = to_fsi_dev(dev); >> INIT_LIST_HEAD(&i2c->ports); >> + i2c->clock_div = I2C_DEFAULT_CLK_DIV; >> + >> + lbus = fsi_device_local_bus_frequency(i2c->fsi); >> + if (lbus) { >> + u32 clock = I2C_DEFAULT_CLK_RATE; > I don't see the need for initialization. Does device_property_read_u32 set clock if the property isn't found? If not, it needs to be initialized here. Or I can set it in an else statement from device_property_read_u32. > >> + >> + if (!device_property_read_u32(dev, "clock-frequency", &clock)) { >> + if (!clock) >> + clock = I2C_DEFAULT_CLK_RATE; >> + } > no need for brackets. Perhaps not, but checkpatch didn't complain, and I personally like brackets if multiple lines are included. > >> + >> + // i2c clock rate = local bus clock / (4 * (i2c clock div + 1)) > You forgot to remove this. I actually meant to leave that comment to explain how the clock rate is calculated, as the reverse calculation in the code is a little more confusing. > > Andi > >> + i2c->clock_div = (((lbus + (clock - 1)) / clock) / 4) - 1; >> + } >> >> rc = fsi_i2c_dev_init(i2c); >> if (rc) >> -- >> 2.39.3 >>
On Tue, Apr 16, 2024 at 01:09:04PM -0500, Eddie James wrote: > > On 4/15/24 17:11, Andi Shyti wrote: > > Hi Eddie, > > > > > @@ -689,6 +692,20 @@ static int fsi_i2c_probe(struct device *dev) > > > mutex_init(&i2c->lock); > > > i2c->fsi = to_fsi_dev(dev); > > > INIT_LIST_HEAD(&i2c->ports); > > > + i2c->clock_div = I2C_DEFAULT_CLK_DIV; > > > + > > > + lbus = fsi_device_local_bus_frequency(i2c->fsi); > > > + if (lbus) { > > > + u32 clock = I2C_DEFAULT_CLK_RATE; > > I don't see the need for initialization. > > > Does device_property_read_u32 set clock if the property isn't found? If not, > it needs to be initialized here. Or I can set it in an else statement from > device_property_read_u32. > > > > > > > + > > > + if (!device_property_read_u32(dev, "clock-frequency", &clock)) { > > > + if (!clock) > > > + clock = I2C_DEFAULT_CLK_RATE; > > > + } if (device_property_read_u32(dev, "clock-frequency", &clock) || !clock) clock = I2C_DEFAULT_CLK_RATE; > > > + > > > + // i2c clock rate = local bus clock / (4 * (i2c clock div + 1)) > > You forgot to remove this. > > > I actually meant to leave that comment to explain how the clock rate is > calculated, as the reverse calculation in the code is a little more > confusing. > Partially that is because you implemented DIV_ROUND_UP() manually. > > > > > Andi > > > > > + i2c->clock_div = (((lbus + (clock - 1)) / clock) / 4) - 1; = DIV_ROUND_UP(lbus, clock) / 4 - 1 Guenter
On 4/16/24 16:20, Guenter Roeck wrote: > On Tue, Apr 16, 2024 at 01:09:04PM -0500, Eddie James wrote: >> On 4/15/24 17:11, Andi Shyti wrote: >>> Hi Eddie, >>> >>>> @@ -689,6 +692,20 @@ static int fsi_i2c_probe(struct device *dev) >>>> mutex_init(&i2c->lock); >>>> i2c->fsi = to_fsi_dev(dev); >>>> INIT_LIST_HEAD(&i2c->ports); >>>> + i2c->clock_div = I2C_DEFAULT_CLK_DIV; >>>> + >>>> + lbus = fsi_device_local_bus_frequency(i2c->fsi); >>>> + if (lbus) { >>>> + u32 clock = I2C_DEFAULT_CLK_RATE; >>> I don't see the need for initialization. >> >> Does device_property_read_u32 set clock if the property isn't found? If not, >> it needs to be initialized here. Or I can set it in an else statement from >> device_property_read_u32. >> >> >>>> + >>>> + if (!device_property_read_u32(dev, "clock-frequency", &clock)) { >>>> + if (!clock) >>>> + clock = I2C_DEFAULT_CLK_RATE; >>>> + } > if (device_property_read_u32(dev, "clock-frequency", &clock) || !clock) > clock = I2C_DEFAULT_CLK_RATE; Nice one, thanks. > >>>> + >>>> + // i2c clock rate = local bus clock / (4 * (i2c clock div + 1)) >>> You forgot to remove this. >> >> I actually meant to leave that comment to explain how the clock rate is >> calculated, as the reverse calculation in the code is a little more >> confusing. >> > Partially that is because you implemented DIV_ROUND_UP() manually. Thanks Guenter, good point. Eddie >>> Andi >>> >>>> + i2c->clock_div = (((lbus + (clock - 1)) / clock) / 4) - 1; > = DIV_ROUND_UP(lbus, clock) / 4 - 1 > > Guenter
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c index 10332693edf0..eaecf156ac31 100644 --- a/drivers/i2c/busses/i2c-fsi.c +++ b/drivers/i2c/busses/i2c-fsi.c @@ -27,7 +27,8 @@ #define FSI_ENGID_I2C 0x7 -#define I2C_DEFAULT_CLK_DIV 6 +#define I2C_DEFAULT_CLK_DIV 7 +#define I2C_DEFAULT_CLK_RATE 400000 /* i2c registers */ #define I2C_FSI_FIFO 0x00 @@ -150,6 +151,7 @@ struct fsi_i2c_master { u8 fifo_size; struct list_head ports; struct mutex lock; + u32 clock_div; }; struct fsi_i2c_port { @@ -194,7 +196,7 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c) if (rc) return rc; - mode |= FIELD_PREP(I2C_MODE_CLKDIV, I2C_DEFAULT_CLK_DIV); + mode |= FIELD_PREP(I2C_MODE_CLKDIV, i2c->clock_div); rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode); if (rc) return rc; @@ -680,6 +682,7 @@ static int fsi_i2c_probe(struct device *dev) struct fsi_i2c_port *port; struct device_node *np; u32 port_no, ports, stat; + u32 lbus; int rc; i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); @@ -689,6 +692,20 @@ static int fsi_i2c_probe(struct device *dev) mutex_init(&i2c->lock); i2c->fsi = to_fsi_dev(dev); INIT_LIST_HEAD(&i2c->ports); + i2c->clock_div = I2C_DEFAULT_CLK_DIV; + + lbus = fsi_device_local_bus_frequency(i2c->fsi); + if (lbus) { + u32 clock = I2C_DEFAULT_CLK_RATE; + + if (!device_property_read_u32(dev, "clock-frequency", &clock)) { + if (!clock) + clock = I2C_DEFAULT_CLK_RATE; + } + + // i2c clock rate = local bus clock / (4 * (i2c clock div + 1)) + i2c->clock_div = (((lbus + (clock - 1)) / clock) / 4) - 1; + } rc = fsi_i2c_dev_init(i2c); if (rc)
Use the new FSI device local bus clock to calculate the proper i2c clock divder and look up an optional clock-frequency property from device tree. Change the default clock divider to 7 now that the default local bus clock divider has been reduced as well. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/i2c/busses/i2c-fsi.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)