| Message ID | 20220910232015.216329-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: rs9: Fix I2C accessors | expand |
Hi Marek, thanks for the patch. Am Sonntag, 11. September 2022, 01:20:15 CEST schrieb Marek Vasut: > Add custom I2C accessors to this driver, since the regular I2C regmap ones > do not generate the exact I2C transfers required by the chip. On I2C write, > it is mandatory to send transfer length first, on read the chip returns the > transfer length in first byte. Instead of always reading back 8 bytes, which > is the default and also the size of the entire register file, set BCP > register to 1 to read out 1 byte which is less wasteful. > > Fixes: 892e0ddea1aa6 ("clk: rs9: Add Renesas 9-series PCIe clock generator > driver") Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexander Stein <alexander.stein@ew.tq-group.com> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk-renesas-pcie.c | 53 +++++++++++++++++++++++++++++++++- > 1 file changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c > index 4f5df1fc74b46..711ba443f33b7 100644 > --- a/drivers/clk/clk-renesas-pcie.c > +++ b/drivers/clk/clk-renesas-pcie.c > @@ -90,6 +90,49 @@ static const struct regmap_access_table > rs9_writeable_table = { .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges), > }; > > +static int rs9_regmap_i2c_write(void *context, unsigned int reg, unsigned > int val) +{ > + struct i2c_client *i2c = context; > + const u8 data[3] = { reg, 1, val }; > + const int count = ARRAY_SIZE(data); > + int ret; > + > + ret = i2c_master_send(i2c, data, count); > + if (ret == count) > + return 0; > + else if (ret < 0) > + return ret; > + else > + return -EIO; > +} > + > +static int rs9_regmap_i2c_read(void *context, > + unsigned int reg, unsigned int *val) > +{ > + struct i2c_client *i2c = context; > + struct i2c_msg xfer[2]; > + u8 data[2]; > + int ret; > + > + xfer[0].addr = i2c->addr; > + xfer[0].flags = 0; > + xfer[0].len = 1; > + xfer[0].buf = (void *)® This does work only for little-endian machines, right? > + > + xfer[1].addr = i2c->addr; > + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; > + xfer[1].len = 1; > + xfer[1].buf = (void *)data; > + > + ret = i2c_transfer(i2c->adapter, xfer, 2); > + if (ret == 2) > + return 0; You are missing setting 'val' here. > + else if (ret < 0) > + return ret; > + else > + return -EIO; > +} > + > static const struct regmap_config rs9_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > @@ -97,6 +140,8 @@ static const struct regmap_config rs9_regmap_config = { > .max_register = 0x8, > .rd_table = &rs9_readable_table, > .wr_table = &rs9_writeable_table, > + .reg_write = rs9_regmap_i2c_write, > + .reg_read = rs9_regmap_i2c_read, > }; > > static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) > @@ -242,11 +287,17 @@ static int rs9_probe(struct i2c_client *client) > return ret; > } > > - rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config); > + rs9->regmap = devm_regmap_init(&client->dev, NULL, > + client, &rs9_regmap_config); > if (IS_ERR(rs9->regmap)) > return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap), > "Failed to allocate register map\n"); > > + /* Always read back 1 Byte via I2C */ > + ret = regmap_write(rs9->regmap, RS9_REG_BCP, 1); > + if (ret < 0) > + return ret; > + > /* Register clock */ > for (i = 0; i < rs9->chip_info->num_clks; i++) { > snprintf(name, 5, "DIF%d", i); For some reason this doesn't work with cache being enabled. No idea why this happens though. Best regards, Alexander
On 9/12/22 08:46, Alexander Stein wrote: Hi, [...] >> +static int rs9_regmap_i2c_read(void *context, >> + unsigned int reg, unsigned int *val) >> +{ >> + struct i2c_client *i2c = context; >> + struct i2c_msg xfer[2]; >> + u8 data[2]; >> + int ret; >> + >> + xfer[0].addr = i2c->addr; >> + xfer[0].flags = 0; >> + xfer[0].len = 1; >> + xfer[0].buf = (void *)® > > This does work only for little-endian machines, right? Ah, right, fixed in V2. >> + xfer[1].addr = i2c->addr; >> + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; >> + xfer[1].len = 1; >> + xfer[1].buf = (void *)data; >> + >> + ret = i2c_transfer(i2c->adapter, xfer, 2); >> + if (ret == 2) >> + return 0; > > You are missing setting 'val' here. Fixed in V2 >> + else if (ret < 0) >> + return ret; >> + else >> + return -EIO; >> +} >> + >> static const struct regmap_config rs9_regmap_config = { >> .reg_bits = 8, >> .val_bits = 8, >> @@ -97,6 +140,8 @@ static const struct regmap_config rs9_regmap_config = { >> .max_register = 0x8, >> .rd_table = &rs9_readable_table, >> .wr_table = &rs9_writeable_table, >> + .reg_write = rs9_regmap_i2c_write, >> + .reg_read = rs9_regmap_i2c_read, >> }; >> >> static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) >> @@ -242,11 +287,17 @@ static int rs9_probe(struct i2c_client *client) >> return ret; >> } >> >> - rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config); >> + rs9->regmap = devm_regmap_init(&client->dev, NULL, >> + client, > &rs9_regmap_config); >> if (IS_ERR(rs9->regmap)) >> return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap), >> "Failed to allocate register > map\n"); >> >> + /* Always read back 1 Byte via I2C */ >> + ret = regmap_write(rs9->regmap, RS9_REG_BCP, 1); >> + if (ret < 0) >> + return ret; >> + >> /* Register clock */ >> for (i = 0; i < rs9->chip_info->num_clks; i++) { >> snprintf(name, 5, "DIF%d", i); > > > For some reason this doesn't work with cache being enabled. No idea why this > happens though. What does this part mean , what does not work with which cache being enabled ?
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c index 4f5df1fc74b46..711ba443f33b7 100644 --- a/drivers/clk/clk-renesas-pcie.c +++ b/drivers/clk/clk-renesas-pcie.c @@ -90,6 +90,49 @@ static const struct regmap_access_table rs9_writeable_table = { .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges), }; +static int rs9_regmap_i2c_write(void *context, unsigned int reg, unsigned int val) +{ + struct i2c_client *i2c = context; + const u8 data[3] = { reg, 1, val }; + const int count = ARRAY_SIZE(data); + int ret; + + ret = i2c_master_send(i2c, data, count); + if (ret == count) + return 0; + else if (ret < 0) + return ret; + else + return -EIO; +} + +static int rs9_regmap_i2c_read(void *context, + unsigned int reg, unsigned int *val) +{ + struct i2c_client *i2c = context; + struct i2c_msg xfer[2]; + u8 data[2]; + int ret; + + xfer[0].addr = i2c->addr; + xfer[0].flags = 0; + xfer[0].len = 1; + xfer[0].buf = (void *)® + + xfer[1].addr = i2c->addr; + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; + xfer[1].len = 1; + xfer[1].buf = (void *)data; + + ret = i2c_transfer(i2c->adapter, xfer, 2); + if (ret == 2) + return 0; + else if (ret < 0) + return ret; + else + return -EIO; +} + static const struct regmap_config rs9_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -97,6 +140,8 @@ static const struct regmap_config rs9_regmap_config = { .max_register = 0x8, .rd_table = &rs9_readable_table, .wr_table = &rs9_writeable_table, + .reg_write = rs9_regmap_i2c_write, + .reg_read = rs9_regmap_i2c_read, }; static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) @@ -242,11 +287,17 @@ static int rs9_probe(struct i2c_client *client) return ret; } - rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config); + rs9->regmap = devm_regmap_init(&client->dev, NULL, + client, &rs9_regmap_config); if (IS_ERR(rs9->regmap)) return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap), "Failed to allocate register map\n"); + /* Always read back 1 Byte via I2C */ + ret = regmap_write(rs9->regmap, RS9_REG_BCP, 1); + if (ret < 0) + return ret; + /* Register clock */ for (i = 0; i < rs9->chip_info->num_clks; i++) { snprintf(name, 5, "DIF%d", i);
Add custom I2C accessors to this driver, since the regular I2C regmap ones do not generate the exact I2C transfers required by the chip. On I2C write, it is mandatory to send transfer length first, on read the chip returns the transfer length in first byte. Instead of always reading back 8 bytes, which is the default and also the size of the entire register file, set BCP register to 1 to read out 1 byte which is less wasteful. Fixes: 892e0ddea1aa6 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver") Reported-by: Alexander Stein <alexander.stein@ew.tq-group.com> Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk-renesas-pcie.c | 53 +++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)