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
