Re: [PATCH v2] clk: rs9: Fix I2C accessors

From: Marek Vasut <marex@denx.de>
To: Alexander Stein <alexander.stein@ew.tq-group.com>,
	linux-clk@vger.kernel.org
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [PATCH v2] clk: rs9: Fix I2C accessors
Date: Tue, 27 Sep 2022 21:43:12 +0200	[thread overview]
Message-ID: <9a13e3ab-6541-7a24-e231-64faeac129c6@denx.de> (raw)
In-Reply-To: <4745081.GXAFRqVoOG@steina-w>

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 ?