Re: [PATCH] i2c: add i2c bus driver for amd navi gpu

[Goswami, Sanket]

Hi Andy,

On 09-Mar-21 19:56, Andy Shevchenko wrote:
> [CAUTION: External Email]
> 
> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
> 
> i2c: -> i2c: designware:
> add i2c bus driver -> add a driver
> amd -> AMD
> gpu -> GPU
> 
> in the subject
> 
>> Latest AMDGPU NAVI cards have USB Type-C interface which can be accessed
>> over I2C.
> 
> I didn't get this. You mean that USB traffic over I²C? This sounds insane
> for a least. Maybe something else is there and description is not fully
> correct?
> 
>> The Type-C controller has integrated designware i2c which is
> 
> DesignWare
> 
>> exposed as a PCI device to the AMD platform.
>>
>> Also there exists couple of notable IP problems that are dealt as a
>> workaround:
>> - I2C transactions work on a polling mode as IP does not generate
>> interrupt.
>>
>> - I2C reads commands are sent twice to address the IP issues.
> 
> Please, read this article: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchris.beams.io%2Fposts%2Fgit-commit%2F&data=04%7C01%7CSanket.Goswami%40amd.com%7C70d1c562d0c04fc2b76508d8e3074c97%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508967775355658%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yLiOkOtqCwGQAJgpYast8cMfCY4I9R74ElWm9FDNFNQ%3D&reserved=0
> 
> ...
> 
>> +#define AMD_UCSI_INTR_EN     0xD
> 
> Why it's capitalized?
> 
> ...
> 
>>  #include "i2c-designware-core.h"
> 
> + Blank line
> 
>> +#define AMD_TIMEOUT_MSEC_MIN 10000
>> +#define AMD_TIMEOUT_MSEC_MAX 11000
> 
> Use unit suffix in the definitions.
> 
> ...
> 
>> +static void i2c_dw_configure_bus(struct dw_i2c_dev *i2cd)
> 
> Why all this is customized? Why FIFO can't be autodetected?
Addressed in v2.
> 
> ...
> 
>> +/* Initiate and continue master read/write transaction with polling
>> + * based transfer routine and write messages into the tx buffer.
>> + */
> 
> Wrong multi-line comment style.
> 
> ...
> 
>> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
> 
> Why do you need a custom function for that? Can it be generic and not AMD
> specific?
This function takes care of AMD Specific bus configuration for the transfer and
also addresses the IP issue of i2c transactions hence it is kept as custom.
> 
> ...
> 
>> +     /* Enable ucsi interrupt */
>> +     if (dev->flags & AMD_NON_INTR_MODE)
>> +             regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
> 
> This is looks like a hack. Why is it here?
In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
it is mandatory to set the right value in specific register
(offset:0x474) as per the hardware IP specification.
> 
> ...
> 
>> +     if (dev->flags & AMD_NON_INTR_MODE)
>> +             return amd_i2c_dw_master_xfer(adap, msgs, num);
> 
> Ditto.
Initiate I2C message transfer when AMD NAVI GPU card is enabled,
As it is polling based transfer mechanism, which does not support
interrupt based functionalities of existing DesignWare driver.

> ...
> 
>> +int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
>> +{
>> +     struct i2c_adapter *amdap = &amdev->adapter;
> 
>> +     int ret;
> 
> See below.
> 
>> +     if (!(amdev->flags & AMD_NON_INTR_MODE))
>> +             return -ENODEV;
> 
>> +     return i2c_add_numbered_adapter(amdap);
>> +
>> +     return ret;
> 
> What the second one does?
> 
>> +}
> 
> ...
> 
>> +     ret = amd_i2c_adap_quirk(dev);
>> +     if (ret != -ENODEV)
> 
> This is ugly. Can we run quirk if and only if we have an AMD chip?
> 
>> +             return ret;
> 
> ...
> 
>>  #define DRIVER_NAME "i2c-designware-pci"
>> +#define AMD_CLK_RATE 100000
> 
> Add units.
> 
> ...
> 
>> +/* NAVI-AMD HCNT/LCNT/SDA hold time */
>> +static struct dw_scl_sda_cfg navi_amd_config = {
>> +     .ss_hcnt = 0x1ae,
>> +     .ss_lcnt = 0x23a,
>> +     .sda_hold = 0x9,
>> +};
> 
> (1)
> 
> ...
> 
>> +static int i2c_dw_populate_client(struct dw_i2c_dev *i2cd)
>> +{
>> +     struct i2c_board_info   *i2c_dw_ccgx_ucsi;
>> +     struct i2c_client       *ccgx_client;
>> +
>> +     i2c_dw_ccgx_ucsi = devm_kzalloc(i2cd->dev, sizeof(*i2c_dw_ccgx_ucsi), GFP_KERNEL);
>> +     if (!i2c_dw_ccgx_ucsi)
>> +             return -ENOMEM;
>> +
>> +     strscpy(i2c_dw_ccgx_ucsi->type, "ccgx-ucsi", sizeof(i2c_dw_ccgx_ucsi->type));
>> +     i2c_dw_ccgx_ucsi->addr = 0x08;
>> +     i2c_dw_ccgx_ucsi->irq = i2cd->irq;
>> +
>> +     ccgx_client = i2c_new_client_device(&i2cd->adapter, i2c_dw_ccgx_ucsi);
>> +     if (!ccgx_client)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
> 
> This is the same as in nVidia GPU I²C driver. Can you do a preparatory changes
> to deduplicate this?
Addressed in v2.
> 
> Also why (1) and this can't be instantiated from ACPI / DT?
It is in line with the existing PCIe-based DesignWare driver,
A similar approach is used by the various vendors.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
Thanks,
Sanket