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

[Sanjay R Mehta <sanmehta@amd.com>]

On 12/8/2020 6:17 PM, Krzysztof Kozlowski wrote:
> [CAUTION: External Email]
> 
> On Tue, Dec 08, 2020 at 06:37:41AM -0600, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <Sanju.Mehta@amd.com>
>>
>> Latest amdgpu card has USB Type-C interface. There is a Type-C controller
>> which can be accessed over I2C.
>>
>> This driver adds I2C bus driver to communicate with Type-C controller. I2C
>> client driver will be part of USB Type-C UCSI driver.
>>
>> Signed-off-by: Sanjay R Mehta <Sanju.Mehta@amd.com>
>> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.Shah@amd.com>
>> ---
>>
>> Changes in v2:
>>
>> - converted the code to use regmap, read_poll_timeout and made some cosmetic
>>   changes as suggested by Andy Shevchenko.
>> ---
>>  MAINTAINERS                          |   7 +
>>  drivers/i2c/busses/Kconfig           |   9 +
>>  drivers/i2c/busses/Makefile          |   1 +
>>  drivers/i2c/busses/i2c-amdgpu-navi.c | 345 +++++++++++++++++++++++++++
>>  4 files changed, 362 insertions(+)
>>  create mode 100644 drivers/i2c/busses/i2c-amdgpu-navi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 190c7fa2ea01..93894459a4c8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8119,6 +8119,13 @@ L:     linux-acpi@vger.kernel.org
>>  S:   Maintained
>>  F:   drivers/i2c/i2c-core-acpi.c
>>
>> +I2C CONTROLLER DRIVER FOR AMD GPU
>> +M:   Nehal Bakulchandra Shah <Nehal-Bakulchandra.Shah@amd.com>
>> +M:   Sanjay R Mehta <sanju.mehta@amd.com>
>> +L:   linux-i2c@vger.kernel.org
>> +S:   Maintained
>> +F:   drivers/i2c/busses/i2c-amdgpu-navi.*
>> +
>>  I2C CONTROLLER DRIVER FOR NVIDIA GPU
>>  M:   Ajay Gupta <ajayg@nvidia.com>
>>  L:   linux-i2c@vger.kernel.org
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 293e7a0760e7..0ff369c0f41f 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -88,6 +88,15 @@ config I2C_AMD_MP2
>>         This driver can also be built as modules.  If so, the modules will
>>         be called i2c-amd-mp2-pci and i2c-amd-mp2-plat.
>>
>> +config I2C_AMDGPU_NAVI
>> +     tristate "AMDGPU NAVI I2C controller"
>> +     depends on PCI
>> +     help
>> +       If you say yes to this option, support will be included for the
>> +       NAVI I2C controller which is used to communicate with the GPU's
>> +       Type-C controller. This driver can also be built as a module called
>> +       i2c-amdgpu-navi.
>> +
>>  config I2C_HIX5HD2
>>       tristate "Hix5hd2 high-speed I2C driver"
>>       depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 19aff0e45cb5..f599473a8ad9 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_I2C_ALI15X3)   += i2c-ali15x3.o
>>  obj-$(CONFIG_I2C_AMD756)     += i2c-amd756.o
>>  obj-$(CONFIG_I2C_AMD756_S4882)       += i2c-amd756-s4882.o
>>  obj-$(CONFIG_I2C_AMD8111)    += i2c-amd8111.o
>> +obj-$(CONFIG_I2C_AMDGPU_NAVI)        += i2c-amdgpu-navi.o
>>  obj-$(CONFIG_I2C_CHT_WC)     += i2c-cht-wc.o
>>  obj-$(CONFIG_I2C_I801)               += i2c-i801.o
>>  obj-$(CONFIG_I2C_ISCH)               += i2c-isch.o
>> diff --git a/drivers/i2c/busses/i2c-amdgpu-navi.c b/drivers/i2c/busses/i2c-amdgpu-navi.c
>> new file mode 100644
>> index 000000000000..3922b8aebc26
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-amdgpu-navi.c
>> @@ -0,0 +1,345 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +//
>> +// AMD I2C Controller Driver for Navi GPU's
>> +//
>> +// Copyright (c) 2020, Advanced Micro Devices, Inc.
>> +//
>> +// Authors:
>> +//   Nehal Bakulchandra Shah <Nehal-Bakulchandra.Shah@amd.com>
>> +//   Sanjay R Mehta <Sanju.Mehta@amd.com>
>> +
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/regmap.h>
>> +#include <asm/unaligned.h>
>> +#include "i2c-designware-core.h"
>> +
>> +#define AMD_UCSI_INTR_EN     0xD
>> +#define AMD_UCSI_INTR_REG    0x474
>> +#define AMD_MASTERCFG_MASK   GENMASK(15, 0)
>> +
>> +struct amdgpu_i2c_dev {
>> +     void __iomem            *regs;
>> +     struct regmap           *map;
>> +     struct device           *dev;
>> +     u32                     master_cfg;
>> +     u32                     slave_adr;
>> +     u32                     tx_fifo_depth;
>> +     u32                     rx_fifo_depth;
>> +     u16                     ss_hcnt;
>> +     u16                     ss_lcnt;
>> +     struct i2c_adapter      adapter;
>> +     struct i2c_board_info   *gpu_ccgx_ucsi;
>> +     struct i2c_client       *ccgx_client;
>> +};
>> +
>> +static int amdgpu_i2c_read(void *context, unsigned int reg, unsigned int *val)
>> +{
>> +     struct amdgpu_i2c_dev *i2cd = context;
>> +
>> +     *val = readl_relaxed(i2cd->regs + reg);
> 
> It's quite confusing calling it "i2c_read" function. What is more
> important - why do you need it? It's a simple MMIO on PCI, so why regmap
> MMIO cannot be used?
> 
Thanks Krzysztof.

I am new to using regmap based API's and had referred to designware code for this.
(https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-designware-common.c#L61)

Any specific API you recommend me to use or any driver you want me to refer will be helpful.

Thanks & Regards,
Sanjay

> Best regards,
> Krzysztof
>