Re: [PATCH v2] i2c: designware: add a new bit check for IC_CON control

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 1/17/2023 8:38 PM, Andy Shevchenko wrote:
> On Tue, Jan 17, 2023 at 05:58:01PM +0530, Shyam Sundar S K wrote:
>> On some AMD platforms, based on the new designware datasheet,
>> BIOS sets the BIT(11) within the IC_CON register to advertise
>> the "bus clear feature capability".
>>
>> AMD/Designware datasheet says:
>>
>> Bit(11) BUS_CLEAR_FEATURE_CTRL. Read-write,Volatile. Reset: 0.
>> Description: In Master mode:
>> - 1'b1: Bus Clear Feature is enabled.
>> - 1'b0: Bus Clear Feature is Disabled.
>> In Slave mode, this register bit is not applicable.
>>
>> On AMD platform designs:
>> 1. BIOS programs the BUS_CLEAR_FEATURE_CTRL and enables the detection
>> of SCL/SDA stuck low.
>> 2. Whenever the stuck low is detected, the SMU FW shall do the bus
>> recovery procedure.
>>
>> Currently, the way in which the "master_cfg" is built in the driver, it
>> overrides the BUS_CLEAR_FEATURE_CTRL advertised by BIOS and the SMU FW
>> cannot initiate the bus recovery if the stuck low is detected.
>>
>> Hence add a check in i2c_dw_configure_master() that if the BIOS
>> advertises the bus clear feature, let driver not ignore it and
>> adapt accordingly.
> 
> ...
> 
>> +	ic_con = ioread32(dev->base + DW_IC_CON);
> 
> Any particular reason why regmap_read() can't be used?

Yes. init_regmap() happens at a later stage in dw_i2c_plat_probe() and
i2c_dw_configure() gets called first.

So dev->map will not be initialized to use regmap_read().

In order to use regmap_read() instead of ioread32() in this case, we
have to defer calling i2c_dw_configure()

Something like this.

--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -311,8 +311,6 @@ static int dw_i2c_plat_probe(struct platform_device
*pdev)
        if (ret)
                goto exit_reset;

-       i2c_dw_configure(dev);
-
        /* Optional interface clock */
        dev->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
        if (IS_ERR(dev->pclk)) {
@@ -378,6 +376,7 @@ static int dw_i2c_plat_probe(struct platform_device
*pdev)
        if (ret)
                goto exit_probe;

+       i2c_dw_configure(dev);
        return ret;

 exit_probe:


What are your thoughts? Should I send a patch to reorder the call flow?

Thanks,
Shyam