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

[Krzysztof Kozlowski <krzk@kernel.org>]

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?

Best regards,
Krzysztof