| Message ID | 20220924164933.393649-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] clk: rs9: Fix I2C accessors | expand |
Hi Marek,
thanks for the update.
To answer your question regarding the cache: With this patch I get the
following:
$ cat /sys/kernel/debug/regmap/1-0068/registers
0: 00
1: 00
2: 00
3: 00
4: 00
5: 00
6: 00
7: 01
8: 00
Which is obviously wrong. Reason is that the cache is not allocated without a
known size. This can be fixed using this patch:
---8<---
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index 5b37d6a2e908..f1c185980466 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -140,7 +140,8 @@ static const struct regmap_config rs9_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.cache_type = REGCACHE_FLAT,
- .max_register = 0x8,
+ .max_register = 0x7,
+ .num_reg_defaults_raw = 0x8,
.rd_table = &rs9_readable_table,
.wr_table = &rs9_writeable_table,
.reg_write = rs9_regmap_i2c_write,
---8<---
Unfortunately now the cache is initialized before RS9_REG_BCP is set to 1,
resulting in the following panic:
[ 17.221637] Kernel panic - not syncing: stack-protector: Kernel stack is
corrupted in: rs9_regmap_i2c_read+0xb4/0xb4 [clk_renesas_pcie]
[ 16.862107] CPU: 3 PID: 277 Comm: systemd-udevd Not tainted 6.0.0-rc6-
next-20220923+ #764 d22d7e904fab3397adb372dbcb36af4e5b1f49bd
[ 16.862118] Hardware name: TQ-Systems GmbH i.MX8MM TQMa8MxML on MBa8Mx (DT)
[ 16.862123] Call trace:
[ 16.862125] dump_backtrace+0xd8/0x130
[ 16.862136] show_stack+0x14/0x40
[ 16.862141] dump_stack_lvl+0x88/0xb0
[ 16.862147] dump_stack+0x14/0x2c
[ 16.862152] panic+0x19c/0x394
[ 16.862160] __stack_chk_fail+0x24/0x30
[ 16.862167] rs9_get_common_config+0x0/0x19c [clk_renesas_pcie]
[ 16.862179] _regmap_read+0x74/0x164
[ 16.862188] regmap_read+0x48/0x70
[ 16.862193] regcache_hw_init+0x184/0x2d0
[ 16.862200] regcache_init+0x1d4/0x2c0
[ 16.862206] __regmap_init+0x864/0x1000
[ 16.862211] __devm_regmap_init+0x74/0xc0
[ 16.862217] rs9_probe+0x118/0x240 [clk_renesas_pcie]
This is caused by I2C_M_RECV_LEN for the rx i2c transfer. Upon cache
initialization the 1st byte received is still set to 8 in hardware. So 8 data
bytes + len are copied into rx buffer (which is actually only 2 bytes).
There is 2 ways to fix it: Set the rx buffer to the maximum receivable bytes
(8) or only read a fixed size of 2. As reg_read only supports reading 1
register, the latter one is enough.
Reading is fixed by the following patch.
---8<---
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index c320ce25c11b..5b37d6a2e908 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -122,8 +122,8 @@ static int rs9_regmap_i2c_read(void *context,
xfer[0].buf = (void *)&txdata;
xfer[1].addr = i2c->addr;
- xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN;
- xfer[1].len = 1;
+ xfer[1].flags = I2C_M_RD;
+ xfer[1].len = 2;
xfer[1].buf = (void *)rxdata;
ret = i2c_transfer(i2c->adapter, xfer, 2);
---8<---
Putting all together the regmap debug output is like this:
$ cat /sys/kernel/debug/regmap/1-0068/registers
0: ff
1: 06
2: ff
3: 5f
4: 00
5: 01
6: 04
7: 01
This is actually a 9FGV0441 using some queued patches on my side.
Best regards,
Alexander
Am Samstag, 24. September 2022, 18:49:33 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>
> ---
> V2: Fix endianness handling in rs9_regmap_i2c_read() i2c_transfer
> ---
> 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 | 56 +++++++++++++++++++++++++++++++++-
> 1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index 4f5df1fc74b46..c320ce25c11b6 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -90,6 +90,52 @@ 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 txdata = reg;
> + u8 rxdata[2];
> + int ret;
> +
> + xfer[0].addr = i2c->addr;
> + xfer[0].flags = 0;
> + xfer[0].len = 1;
> + xfer[0].buf = (void *)&txdata;
> +
> + xfer[1].addr = i2c->addr;
> + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN;
> + xfer[1].len = 1;
> + xfer[1].buf = (void *)rxdata;
> +
> + ret = i2c_transfer(i2c->adapter, xfer, 2);
> + if (ret < 0)
> + return ret;
> + if (ret != 2)
> + return -EIO;
> +
> + *val = rxdata[1];
> + return 0;
> +}
> +
> static const struct regmap_config rs9_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -97,6 +143,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 +290,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);
On 9/26/22 08:36, Alexander Stein wrote: > Hi Marek, Hi, > thanks for the update. > To answer your question regarding the cache: With this patch I get the > following: > $ cat /sys/kernel/debug/regmap/1-0068/registers > 0: 00 > 1: 00 > 2: 00 > 3: 00 > 4: 00 > 5: 00 > 6: 00 > 7: 01 > 8: 00 Ah, clear. Thanks for the detailed explanation. > Which is obviously wrong. Reason is that the cache is not allocated without a > known size. This can be fixed using this patch: > ---8<--- > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c > index 5b37d6a2e908..f1c185980466 100644 > --- a/drivers/clk/clk-renesas-pcie.c > +++ b/drivers/clk/clk-renesas-pcie.c > @@ -140,7 +140,8 @@ static const struct regmap_config rs9_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > .cache_type = REGCACHE_FLAT, > - .max_register = 0x8, > + .max_register = 0x7, > + .num_reg_defaults_raw = 0x8, > .rd_table = &rs9_readable_table, > .wr_table = &rs9_writeable_table, > .reg_write = rs9_regmap_i2c_write, > ---8<--- > > Unfortunately now the cache is initialized before RS9_REG_BCP is set to 1, > resulting in the following panic: > [ 17.221637] Kernel panic - not syncing: stack-protector: Kernel stack is > corrupted in: rs9_regmap_i2c_read+0xb4/0xb4 [clk_renesas_pcie] > [ 16.862107] CPU: 3 PID: 277 Comm: systemd-udevd Not tainted 6.0.0-rc6- > next-20220923+ #764 d22d7e904fab3397adb372dbcb36af4e5b1f49bd > [ 16.862118] Hardware name: TQ-Systems GmbH i.MX8MM TQMa8MxML on MBa8Mx (DT) > [ 16.862123] Call trace: > [ 16.862125] dump_backtrace+0xd8/0x130 > [ 16.862136] show_stack+0x14/0x40 > [ 16.862141] dump_stack_lvl+0x88/0xb0 > [ 16.862147] dump_stack+0x14/0x2c > [ 16.862152] panic+0x19c/0x394 > [ 16.862160] __stack_chk_fail+0x24/0x30 > [ 16.862167] rs9_get_common_config+0x0/0x19c [clk_renesas_pcie] > [ 16.862179] _regmap_read+0x74/0x164 > [ 16.862188] regmap_read+0x48/0x70 > [ 16.862193] regcache_hw_init+0x184/0x2d0 > [ 16.862200] regcache_init+0x1d4/0x2c0 > [ 16.862206] __regmap_init+0x864/0x1000 > [ 16.862211] __devm_regmap_init+0x74/0xc0 > [ 16.862217] rs9_probe+0x118/0x240 [clk_renesas_pcie] > > This is caused by I2C_M_RECV_LEN for the rx i2c transfer. Upon cache > initialization the 1st byte received is still set to 8 in hardware. So 8 data > bytes + len are copied into rx buffer (which is actually only 2 bytes). > There is 2 ways to fix it: Set the rx buffer to the maximum receivable bytes > (8) or only read a fixed size of 2. As reg_read only supports reading 1 > register, the latter one is enough. > Reading is fixed by the following patch. > ---8<--- > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c > index c320ce25c11b..5b37d6a2e908 100644 > --- a/drivers/clk/clk-renesas-pcie.c > +++ b/drivers/clk/clk-renesas-pcie.c > @@ -122,8 +122,8 @@ static int rs9_regmap_i2c_read(void *context, > xfer[0].buf = (void *)&txdata; > > xfer[1].addr = i2c->addr; > - xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; > - xfer[1].len = 1; > + xfer[1].flags = I2C_M_RD; > + xfer[1].len = 2; > xfer[1].buf = (void *)rxdata; > > ret = i2c_transfer(i2c->adapter, xfer, 2); > ---8<--- > > Putting all together the regmap debug output is like this: > $ cat /sys/kernel/debug/regmap/1-0068/registers > 0: ff > 1: 06 > 2: ff > 3: 5f > 4: 00 > 5: 01 > 6: 04 > 7: 01 What about option 3 -- disable the cache altogether ? I can imagine since the chip is configured with like 2-3 I2C writes on boot and then never again written to, that might be the simplest approach. > This is actually a 9FGV0441 using some queued patches on my side. Nice, do you plan to send a binding update for this one ?
Hi Marek, Am Dienstag, 27. September 2022, 21:43:12 CEST schrieb Marek Vasut: > On 9/26/22 08:36, Alexander Stein wrote: > > Hi Marek, > > Hi, > > > thanks for the update. > > To answer your question regarding the cache: With this patch I get the > > following: > > $ cat /sys/kernel/debug/regmap/1-0068/registers > > 0: 00 > > 1: 00 > > 2: 00 > > 3: 00 > > 4: 00 > > 5: 00 > > 6: 00 > > 7: 01 > > 8: 00 > > Ah, clear. Thanks for the detailed explanation. > > > Which is obviously wrong. Reason is that the cache is not allocated > > without a known size. This can be fixed using this patch: > > ---8<--- > > diff --git a/drivers/clk/clk-renesas-pcie.c > > b/drivers/clk/clk-renesas-pcie.c index 5b37d6a2e908..f1c185980466 100644 > > --- a/drivers/clk/clk-renesas-pcie.c > > +++ b/drivers/clk/clk-renesas-pcie.c > > @@ -140,7 +140,8 @@ static const struct regmap_config rs9_regmap_config = > > { > > > > .reg_bits = 8, > > .val_bits = 8, > > .cache_type = REGCACHE_FLAT, > > > > - .max_register = 0x8, > > + .max_register = 0x7, > > + .num_reg_defaults_raw = 0x8, > > > > .rd_table = &rs9_readable_table, > > .wr_table = &rs9_writeable_table, > > .reg_write = rs9_regmap_i2c_write, > > > > ---8<--- > > > > Unfortunately now the cache is initialized before RS9_REG_BCP is set to 1, > > resulting in the following panic: > > [ 17.221637] Kernel panic - not syncing: stack-protector: Kernel stack > > is > > corrupted in: rs9_regmap_i2c_read+0xb4/0xb4 [clk_renesas_pcie] > > [ 16.862107] CPU: 3 PID: 277 Comm: systemd-udevd Not tainted 6.0.0-rc6- > > next-20220923+ #764 d22d7e904fab3397adb372dbcb36af4e5b1f49bd > > [ 16.862118] Hardware name: TQ-Systems GmbH i.MX8MM TQMa8MxML on MBa8Mx > > (DT) [ 16.862123] Call trace: > > [ 16.862125] dump_backtrace+0xd8/0x130 > > [ 16.862136] show_stack+0x14/0x40 > > [ 16.862141] dump_stack_lvl+0x88/0xb0 > > [ 16.862147] dump_stack+0x14/0x2c > > [ 16.862152] panic+0x19c/0x394 > > [ 16.862160] __stack_chk_fail+0x24/0x30 > > [ 16.862167] rs9_get_common_config+0x0/0x19c [clk_renesas_pcie] > > [ 16.862179] _regmap_read+0x74/0x164 > > [ 16.862188] regmap_read+0x48/0x70 > > [ 16.862193] regcache_hw_init+0x184/0x2d0 > > [ 16.862200] regcache_init+0x1d4/0x2c0 > > [ 16.862206] __regmap_init+0x864/0x1000 > > [ 16.862211] __devm_regmap_init+0x74/0xc0 > > [ 16.862217] rs9_probe+0x118/0x240 [clk_renesas_pcie] > > > > This is caused by I2C_M_RECV_LEN for the rx i2c transfer. Upon cache > > initialization the 1st byte received is still set to 8 in hardware. So 8 > > data bytes + len are copied into rx buffer (which is actually only 2 > > bytes). There is 2 ways to fix it: Set the rx buffer to the maximum > > receivable bytes (8) or only read a fixed size of 2. As reg_read only > > supports reading 1 register, the latter one is enough. > > Reading is fixed by the following patch. > > ---8<--- > > diff --git a/drivers/clk/clk-renesas-pcie.c > > b/drivers/clk/clk-renesas-pcie.c index c320ce25c11b..5b37d6a2e908 100644 > > --- a/drivers/clk/clk-renesas-pcie.c > > +++ b/drivers/clk/clk-renesas-pcie.c > > @@ -122,8 +122,8 @@ static int rs9_regmap_i2c_read(void *context, > > > > xfer[0].buf = (void *)&txdata; > > > > xfer[1].addr = i2c->addr; > > > > - xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; > > - xfer[1].len = 1; > > + xfer[1].flags = I2C_M_RD; > > + xfer[1].len = 2; > > > > xfer[1].buf = (void *)rxdata; > > > > ret = i2c_transfer(i2c->adapter, xfer, 2); > > > > ---8<--- > > > > Putting all together the regmap debug output is like this: > > $ cat /sys/kernel/debug/regmap/1-0068/registers > > 0: ff > > 1: 06 > > 2: ff > > 3: 5f > > 4: 00 > > 5: 01 > > 6: 04 > > 7: 01 > > What about option 3 -- disable the cache altogether ? Sure this works as well. > I can imagine since the chip is configured with like 2-3 I2C writes on > boot and then never again written to, that might be the simplest approach. I'm thinking about disabling unused clock outputs later on, so dynamic bit flipping at runtime would be required. For this case cache usage seems reasonable. For now disabling is ok, IMHO. > > This is actually a 9FGV0441 using some queued patches on my side. > > Nice, do you plan to send a binding update for this one ? Sure, but as the DIF bit definition is device specific, I want to settle on general access first. Best regards, Alexander
On 9/28/22 09:32, Alexander Stein wrote: > Hi Marek, Hi, >>> Putting all together the regmap debug output is like this: >>> $ cat /sys/kernel/debug/regmap/1-0068/registers >>> 0: ff >>> 1: 06 >>> 2: ff >>> 3: 5f >>> 4: 00 >>> 5: 01 >>> 6: 04 >>> 7: 01 >> >> What about option 3 -- disable the cache altogether ? > > Sure this works as well. I sent a patch like that for now, let's revisit the cache topic in a separate patch. >> I can imagine since the chip is configured with like 2-3 I2C writes on >> boot and then never again written to, that might be the simplest approach. > > I'm thinking about disabling unused clock outputs later on, so dynamic bit > flipping at runtime would be required. For this case cache usage seems > reasonable. For now disabling is ok, IMHO. > >>> This is actually a 9FGV0441 using some queued patches on my side. >> >> Nice, do you plan to send a binding update for this one ? > > Sure, but as the DIF bit definition is device specific, I want to settle on > general access first. ACK
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c index 4f5df1fc74b46..c320ce25c11b6 100644 --- a/drivers/clk/clk-renesas-pcie.c +++ b/drivers/clk/clk-renesas-pcie.c @@ -90,6 +90,52 @@ 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 txdata = reg; + u8 rxdata[2]; + int ret; + + xfer[0].addr = i2c->addr; + xfer[0].flags = 0; + xfer[0].len = 1; + xfer[0].buf = (void *)&txdata; + + xfer[1].addr = i2c->addr; + xfer[1].flags = I2C_M_RD | I2C_M_RECV_LEN; + xfer[1].len = 1; + xfer[1].buf = (void *)rxdata; + + ret = i2c_transfer(i2c->adapter, xfer, 2); + if (ret < 0) + return ret; + if (ret != 2) + return -EIO; + + *val = rxdata[1]; + return 0; +} + static const struct regmap_config rs9_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -97,6 +143,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 +290,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> --- V2: Fix endianness handling in rs9_regmap_i2c_read() i2c_transfer --- 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 | 56 +++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)