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

Marek Vasut
2022-09-28T12:59:37Z

On 9/28/22 09:35, Alexander Stein wrote:
> Hi Marek,

Hi,

> thanks for the update.

Thanks for the reviews.

> Am Dienstag, 27. September 2022, 23:44:14 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
>> V3: - Disable regcache, the driver does a couple of I2C writes on boot
>> and that is all, so it only adds complexity
>> - Set regmap max_register to RS9_REG_BCP which is the correct one
>> ---
>> 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 | 60 ++++++++++++++++++++++++++++++++--
>> 1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
>> index 4f5df1fc74b46..138aaab05fc8a 100644
>> --- a/drivers/clk/clk-renesas-pcie.c
>> +++ b/drivers/clk/clk-renesas-pcie.c
>> @@ -90,13 +90,61 @@ 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;
>
> I'm still in favor of removing I2C_M_RECV_LEN and setting len=2. This
> currently works only, because there is no read access between
> devm_regmap_init() call and regmap_write() to RS9_REG_BCP.
> Enabling cache later on again this will corrupt the stack.

The device does send you the amount of data in first byte of I2C READ,
so I think I2C_M_RECV_LEN is the right flag here.

I had a look into the above topic too before sending V3, and I think you
can use regcache_cache_bypass(..., true) right after devm_regmap_init()
and then again regcache_cache_bypass(..., false) close to the end of
probe() callback to deal with the problem you're describing.

Would that work ?