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

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

On 12/8/2020 6:32 PM, Sanjay R Mehta wrote:
> 
> 
> 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?
>>
Hi Krzysztof,

As suggested have made the changes in the code to use regmap MMIO and it works fine :).
will send v3 patch with this change.

Thanks,
Sanjay


> 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
>>