Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
On 2018-09-11 18:53, Ajay Gupta wrote: > Hi Peter, > >> -Original Message- >> From: linux-i2c-ow...@vger.kernel.org >> On Behalf Of Peter Rosin >> Sent: Tuesday, September 11, 2018 1:55 AM >> To: Ajay Gupta ; w...@the-dreams.de; >> heikki.kroge...@linux.intel.com >> Cc: linux-usb@vger.kernel.org; linux-...@vger.kernel.org >> Subject: Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU >> >> [I seem to have lost my local copy of the mail I'm responding to, so I copied >> bits of it from an archive and broke threading in the process, sorry about >> that] > That’s why was wondering how you got "goto stop" in "if (msgs[i].flags & > I2C_M_RD) {" > Block at [1]. There is actually no stop when gpu_i2c_read() fails in > master_xfer() > > [1] https://marc.info/?l=linux-usb&m=153662481422174&w=2 No, the only message I lost was the one I quoted from. The "goto stop" line was me saying that "goto stop" would fix a bug caused by the unconditional "return status". I then continued saying that "goto stop" was not perfect etc. Cheers, Peter
RE: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Hi Peter, > -Original Message- > From: linux-i2c-ow...@vger.kernel.org > On Behalf Of Peter Rosin > Sent: Tuesday, September 11, 2018 1:55 AM > To: Ajay Gupta ; w...@the-dreams.de; > heikki.kroge...@linux.intel.com > Cc: linux-usb@vger.kernel.org; linux-...@vger.kernel.org > Subject: Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU > > [I seem to have lost my local copy of the mail I'm responding to, so I copied > bits of it from an archive and broke threading in the process, sorry about > that] That’s why was wondering how you got "goto stop" in "if (msgs[i].flags & I2C_M_RD) {" Block at [1]. There is actually no stop when gpu_i2c_read() fails in master_xfer() [1] https://marc.info/?l=linux-usb&m=153662481422174&w=2 > > On 2018-09-11 00:22, Ajay Gupta wrote: > >> Hmm, that goto stop is however not perfect. Ideally, you shouldn't > >> issue stop if i == 0 and gpu_i2c_read failed See above. There is no "stop" Thanks Ajay --nvpublic > > How can there be a read without rab write first? > > AFAIU, gpu_i2c_read() can get called only with i=1 onward. > > Well, you said that there could be other I2C devices on this I2C bus. I > thought > that meant that there could be other I2C client drivers using this I2C > adapter. > If so, then those drivers can issue I2C transfers where the first message is a > read. > > >> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) { > >> > +static struct i2c_board_info gpu_ccgx_ucsi = { > >> > +I2C_BOARD_INFO("ccgx-ucsi", 0x8), > >> > +}; > >> > >> Shouldn't this struct be dynamically allocated and attached to struct > >> gpu_i2c_dev so that you (maybe) could have more than one instance? > > Currently the structure is local variable. Will that not work in multi > > instance cases? > > Arrggh. NO! > > A static variable in function scope behaves very much the same as a static > variable in file scope. The only difference is that the visibility is > different. All > calls to the function gets the same gpu_ccgx_ucsi instance which means that > instances will clobber each other. > > You need to add a struct i2c_board_info (or a pointer to one) to struct > gpu_i2c_dev so that it can be dynamically allocated. > > Cheers, > Peter
Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
[I seem to have lost my local copy of the mail I'm responding to, so I copied bits of it from an archive and broke threading in the process, sorry about that] On 2018-09-11 00:22, Ajay Gupta wrote: >> Hmm, that goto stop is however not perfect. Ideally, >> you shouldn't issue stop if i == 0 and gpu_i2c_read failed > How can there be a read without rab write first? > AFAIU, gpu_i2c_read() can get called only with i=1 onward. Well, you said that there could be other I2C devices on this I2C bus. I thought that meant that there could be other I2C client drivers using this I2C adapter. If so, then those drivers can issue I2C transfers where the first message is a read. >> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) { >> > + static struct i2c_board_info gpu_ccgx_ucsi = { >> > + I2C_BOARD_INFO("ccgx-ucsi", 0x8), >> > + }; >> >> Shouldn't this struct be dynamically allocated and attached to struct >> gpu_i2c_dev so that you (maybe) could have more than one instance? > Currently the structure is local variable. Will that not work in multi > instance cases? Arrggh. NO! A static variable in function scope behaves very much the same as a static variable in file scope. The only difference is that the visibility is different. All calls to the function gets the same gpu_ccgx_ucsi instance which means that instances will clobber each other. You need to add a struct i2c_board_info (or a pointer to one) to struct gpu_i2c_dev so that it can be dynamically allocated. Cheers, Peter
RE: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Hi Peter, > > Latest NVIDIA GPU 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: Ajay Gupta > > Reviewed-by: Andy Shevchenko > > Reviewed-by: Heikki Krogerus > > --- > > Changes from v1 -> v2 > > None > > Changes from v2 -> v3 > > Fixed review comments from Andy and Thierry > > Rename i2c-gpu.c -> i2c-nvidia-gpu.c > > Changes from v3 -> v4 > > Fixed review comments from Andy > > Changes from v4 -> v5 > > Fixed review comments from Andy > > Changes from v5 -> v6 > > None > > Changes from v6 -> v7 -> v8 > > Fixed review comments from Peter > > - Add implicit STOP for last write message > > - Add i2c_adapter_quirks with max_read_len and > > I2C_AQ_COMB flags > > Changes from v8 -> v9 > > Fixed review comments from Peter > > - Drop do_start flag > > - Use i2c_8bit_addr_from_msg() > > Changes from v9 -> v10 > > Fixed review comments from Peter > > - Dropped I2C_FUNC_SMBUS_EMUL > > - Dropped local mutex > > Changes from v10 -> v11 > > Fixed review comments from Peter > > - Moved stop in master_xfer at end of message > > - Change i2c_read without STOP > > - Dropped I2C_AC_COMB* flags > > > > Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ > > MAINTAINERS | 7 + > > drivers/i2c/busses/Kconfig | 9 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-nvidia-gpu.c | 372 > > > 5 files changed, 407 insertions(+) > > create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu > > create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c > > > > diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu > > b/Documentation/i2c/busses/i2c-nvidia-gpu > > new file mode 100644 > > index 000..31884d2 > > --- /dev/null > > +++ b/Documentation/i2c/busses/i2c-nvidia-gpu > > @@ -0,0 +1,18 @@ > > +Kernel driver i2c-nvidia-gpu > > + > > +Datasheet: not publicly available. > > + > > +Authors: > > + Ajay Gupta > > + > > +Description > > +--- > > + > > +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA > > +Turing and later GPUs and it is used to communicate with Type-C controller > on GPUs. > > + > > +If your 'lspci -v' listing shows something like the following, > > + > > +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 > > +(rev a1) > > + > > +then this driver should support the I2C controller of your GPU. > > diff --git a/MAINTAINERS b/MAINTAINERS index d870cb5..b71b0b4 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6798,6 +6798,13 @@ L: linux-a...@vger.kernel.org > > S: Maintained > > F: drivers/i2c/i2c-core-acpi.c > > > > +I2C CONTROLLER DRIVER FOR NVIDIA GPU > > +M: Ajay Gupta > > +L: linux-...@vger.kernel.org > > +S: Maintained > > +F: Documentation/i2c/busses/i2c-nvidia-gpu > > +F: drivers/i2c/busses/i2c-nvidia-gpu.c > > + > > I2C MUXES > > M: Peter Rosin > > L: linux-...@vger.kernel.org > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 451d4ae..eed827b 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985 > > This driver can also be built as a module. If so, the module > > will be called i2c-nforce2-s4985. > > > > +config I2C_NVIDIA_GPU > > + tristate "NVIDIA GPU I2C controller" > > + depends on PCI > > + help > > + If you say yes to this option, support will be included for the > > + NVIDIA GPU 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-nvidia-gpu. > > + > > config I2C_SIS5595 > > tristate "SiS 5595" > > depends on PCI > > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > > index 18b26af..d499813 100644 > > --- a/drivers/i2c/busses/Makefile > > +++ b/drivers/i2c/busses/Makefile > > @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)+= i2c-sibyte.o > > obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o > > obj-$(CONFIG_SCx200_ACB) += scx200_acb.o > > obj-$(CONFIG_I2C_FSI) += i2c-fsi.o > > +obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o > > > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git > > a/drivers/i2c/busses/i2c-nvidia-gpu.c > > b/drivers/i2c/busses/i2c-nvidia-gpu.c > > new file mode 100644 > > index 000..e88424f > > --- /dev/null > > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c > > @@ -0,0 +1,372 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nvidia GPU I2C controller Driver > > + * > > + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved. > > + * Author: Ajay Gupta */ #include > > +#include #include #include
Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
On 2018-09-11 00:22, Ajay Gupta wrote: > Latest NVIDIA GPU 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: Ajay Gupta > Reviewed-by: Andy Shevchenko > Reviewed-by: Heikki Krogerus > --- > Changes from v1 -> v2 > None > Changes from v2 -> v3 > Fixed review comments from Andy and Thierry > Rename i2c-gpu.c -> i2c-nvidia-gpu.c > Changes from v3 -> v4 > Fixed review comments from Andy > Changes from v4 -> v5 > Fixed review comments from Andy > Changes from v5 -> v6 > None > Changes from v6 -> v7 -> v8 > Fixed review comments from Peter > - Add implicit STOP for last write message > - Add i2c_adapter_quirks with max_read_len and > I2C_AQ_COMB flags > Changes from v8 -> v9 > Fixed review comments from Peter > - Drop do_start flag > - Use i2c_8bit_addr_from_msg() > Changes from v9 -> v10 > Fixed review comments from Peter > - Dropped I2C_FUNC_SMBUS_EMUL > - Dropped local mutex > Changes from v10 -> v11 > Fixed review comments from Peter > - Moved stop in master_xfer at end of message > - Change i2c_read without STOP > - Dropped I2C_AC_COMB* flags > > Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ > MAINTAINERS | 7 + > drivers/i2c/busses/Kconfig | 9 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-nvidia-gpu.c | 372 > > 5 files changed, 407 insertions(+) > create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu > create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c > > diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu > b/Documentation/i2c/busses/i2c-nvidia-gpu > new file mode 100644 > index 000..31884d2 > --- /dev/null > +++ b/Documentation/i2c/busses/i2c-nvidia-gpu > @@ -0,0 +1,18 @@ > +Kernel driver i2c-nvidia-gpu > + > +Datasheet: not publicly available. > + > +Authors: > + Ajay Gupta > + > +Description > +--- > + > +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing > +and later GPUs and it is used to communicate with Type-C controller on GPUs. > + > +If your 'lspci -v' listing shows something like the following, > + > +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1) > + > +then this driver should support the I2C controller of your GPU. > diff --git a/MAINTAINERS b/MAINTAINERS > index d870cb5..b71b0b4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6798,6 +6798,13 @@ L: linux-a...@vger.kernel.org > S: Maintained > F: drivers/i2c/i2c-core-acpi.c > > +I2C CONTROLLER DRIVER FOR NVIDIA GPU > +M: Ajay Gupta > +L: linux-...@vger.kernel.org > +S: Maintained > +F: Documentation/i2c/busses/i2c-nvidia-gpu > +F: drivers/i2c/busses/i2c-nvidia-gpu.c > + > I2C MUXES > M: Peter Rosin > L: linux-...@vger.kernel.org > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 451d4ae..eed827b 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985 > This driver can also be built as a module. If so, the module > will be called i2c-nforce2-s4985. > > +config I2C_NVIDIA_GPU > + tristate "NVIDIA GPU I2C controller" > + depends on PCI > + help > + If you say yes to this option, support will be included for the > + NVIDIA GPU 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-nvidia-gpu. > + > config I2C_SIS5595 > tristate "SiS 5595" > depends on PCI > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 18b26af..d499813 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o > obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o > obj-$(CONFIG_SCx200_ACB) += scx200_acb.o > obj-$(CONFIG_I2C_FSI)+= i2c-fsi.o > +obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c > b/drivers/i2c/busses/i2c-nvidia-gpu.c > new file mode 100644 > index 000..e88424f > --- /dev/null > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c > @@ -0,0 +1,372 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Nvidia GPU I2C controller Driver > + * > + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved. > + * Author: Ajay Gupta > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/* I2C definitions */ > +#define I2C_MST_CNTL