Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-10-25 23:55, Ajay Gupta wrote:
> Hi Heikki and Andy
> [...]
 Shouldn't you return -ETIMEDOUT if count == 0?
>>> Yes. Good catch. Does the below fix looks ok?
>>>
>>> do {
>>> status = ccg_write(uc, CCGX_RAB_INTR_REG, , 
>>> sizeof(data));
>>> if (status < 0)
>>> return status;
>>>
>>> usleep_range(1, 11000);
>>>
>>> status = ccg_read(uc, CCGX_RAB_INTR_REG, , 
>>> sizeof(data));
>>> if (status < 0)
>>> return status;
>>>
>>> if (!data)
>>> return 0;
>>> } while (data && count--);
>>
>> Doesn't that condition break out of the loop immediately?
> How? I didn't get your point? We want to break out when data is
> zero (interrupt status cleared).

The statement

if (!data)
return 0;

ensures that 'data' is non-zero when the loop continues, so checking
that 'data' is non-zero in the while loop test is pointless.

>>> Ah, I see, but why you not reorganize it to put this into do-while loop?
> We actually need to check data after reading it so will reorganize 
> accordingly.
> do {
>   read
>   check for data and break out if (!data)
>   write
>   sleep
> } while (--count);

Here, you have fixed the "issue" (but it doesn't match v14).

Cheers,
Peter

Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-10-25 10:17, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Oct 23, 2018 at 06:56:59PM +, Ajay Gupta wrote:
 +  /* i2c adapter (ccgx-ucsi) can read 4 byte max */
>>>
>>> By "i2c adapter" do you mean this Cypress CCGx controller, or the NVIDIA I2C
>>> host adapter?
>> It mean NVIDIA I2C host adapter with name "ccgx-ucsi"
>>  
 +  while (rem_len > 0) {
 +  msgs[1].buf = [len - rem_len];
 +  rlen = min_t(u16, rem_len, 4);
>>>
>>> I guess this is where you check for that 4 bytes.
>>>
>>> I'm guessing this limitation is for the NVIDIA I2C host adapter.
>> Correct
>>
>>> If that is the case than this driver really should not care about it.
>> I got your point but need to handle this case gracefully.
>>
>> I think best way to handle this is to add a runtime check to find 
>> I2C adapter's quirk and use quirks->max_read_len of the adapter.
>> How does below look?
>>
>> @@ -247,6 +247,7 @@ struct ucsi_ccg {
>>  static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>  {
>> struct i2c_client *client = uc->client;
>> +   const struct i2c_adapter_quirks *quirks = client->adapter->quirks;
>> unsigned char buf[2];
>> struct i2c_msg msgs[] = {
>> {
>> @@ -261,13 +262,16 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 
>> *data, u32 len)
>> .buf= data,
>> },
>> };
>> -   u32 rlen, rem_len = len;
>> +   u32 rlen, rem_len = len, max_read_len = len;
>> int status;
>>
>> -   /* i2c adapter (ccgx-ucsi) can read 4 byte max */
>> +   /* check any max_read_len limitation on i2c adapter */
>> +   if (quirks && quirks->max_read_len)
>> +   max_read_len = quirks->max_read_len;
>> +
>> while (rem_len > 0) {
>> msgs[1].buf = [len - rem_len];
>> -   rlen = min_t(u16, rem_len, 4);
>> +   rlen = min_t(u16, rem_len, max_read_len);
>> msgs[1].len = rlen;
>> put_unaligned_le16(rab, buf);
>> status = i2c_transfer(client->adapter, msgs, 
>> ARRAY_SIZE(msgs));
>>
>>> We most likely need to use this driver on other platforms as well
>>> where the I2C host is something else.
>> Correct and above solution would not impact other I2C host.
> 
> I still didn't understand why can't this just be taken care of in your
> I2C host driver? Why can't you just read 4 bytes at a time in your
> master_xfer hook until you have received as much as the message is
> asking, and only after that return?

The I2C host hardware *cannot* read more than 4 bytes in any one xfer
according to the HW designers. Seriously broken crap, that piece of
hardware... (If that assertion from the HW designers is indeed true?
I suspect that it can be made to work, but the docs are closed and I
don't have HW to experiment with, so it remains a suspicion...)

And the I2C host driver *cannot* be expected to know exactly how any
one client device needs to split xfers into many when the 4 byte limit
is getting in the way, and neither can the I2C core. Because I bet
there are devices where it's not even possible to split xfers while
keeping semantics equivalent...

So, every client driver will need to adjust to this quirk if they are
to operate with this worthless I2C host (or others with similar
limitations, if there are any?). Fortunately, most client drivers don't
read in bulk. Unfortunately, many do...

Cheers,
Peter

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-10-02 09:27, Wolfram Sang wrote:
> Hi,
> 
 We got confirmation from HW team about 4 byte read limitation. There
 has to be a STOP after every single read cycle. One read cycle
 supports maximum of
 4 byte burst. I will update the patches with a comment on this.
>>>
>>> Could it be that this is more an SMBus controller than an I2C controller?
>>> I haven't looked at the specs but maybe populating smbus_xfer instead of
>>> master_xfer is an option here?
>> I think its more of i2c controller and we do mention " .max_read_len = 4" in
>> " struct i2c_adapter_quirks". Do you still see something missing here?
> 
> Well, if there has to be STOP after a read, then you can't do a transfer
> containing "write-read-write", or? So, I wondered if this controller is
> of the type which can mainly do "write-then-read" transfers only (check
> I2C_AQ_COMB* quirk flags). And for some of those controller types, it
> might be even easier to drop generic I2C transfers and only handle the
> SMBUS calls.
> 
> I didn't check this driver closely yet, so I can't tell if/what it needs
> from the above. I wanted to give this input already, though.

I don't think SMBus is an option in this case since the intended client (Cypress
something in patch 2/2) does xfers that would need 16-bit commands and I think
they are always 8-bit in SMBus, no?

Cheers,
Peter

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-10-01 21:33, Ajay Gupta wrote:
> Hi Heikki and Peter,
> 
> Can we get the working set in while the issues is being debugged?

 I'm not the one making the decision, and I don't even know if this
 is going through the i2c or the usb tree? If it's going through the
 i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
 and if it's going through the usb tree, you need a tag from Wolfram
 on patch 1/2. As I said, I'm not involved with that part, I'm just
 reviewing these patches because I felt like it.

 The remaining issue that bothers me is the looping reads, and your
 email address reveals that you should be in a very good position to
 work out why they do not work, and fix it or let us know why they
 don't.
>> I am working with different teams to debug this and I think it may take some
>> time to know the root cause.
> We got confirmation from HW team about 4 byte read limitation. There has to
> be a STOP after every single read cycle. One read cycle supports maximum of
> 4 byte burst. I will update the patches with a comment on this.

Thanks for digging into this! And if the HW team says it's not possible, then
of course my objection falls flat. However, you only mention "read cycle", and
based on your defines (I2C_MST_CNTL_CYCLE_TRIGGER) that seems to be terminology
from the spec. Yet, you apparently do writes without triggering a cycle. Do
the HW team have anything to say about doing reads without triggering a "cycle"?

Cheers,
Peter

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-13 00:22, Ajay Gupta wrote:
> Hi Peter,
> Can we get the working set in while the issues is being debugged?

I'm not the one making the decision, and I don't even know if this
is going through the i2c or the usb tree? If it's going through the
i2c tree you need a tag from the usb people (Greg?) on patch 2/2,
and if it's going through the usb tree, you need a tag from Wolfram
on patch 1/2. As I said, I'm not involved with that part, I'm just
reviewing these patches because I felt like it.

The remaining issue that bothers me is the looping reads, and your
email address reveals that you should be in a very good position to
work out why they do not work, and fix it or let us know why they
don't. However, your responses indicate that you have given up. That
coupled with the fact that the datasheet is not publicly available
(but why, seems a little over-protective to think the interface to an
i2c controller is worth all that much) makes me think that perhaps
this little detail might never be fixed if it's not fixed now. Once
merged, there is no pressure on you to actually do anything about it,
and others are stuck in darkness without a spec.

> So I assume nothing more left to check on this for now.

Have you hooked up a scope to the I2C bus while trying the various
attempts to get looping reads working? What makes a difference?
Have you talked to the hardware people and described what you have
attempted? Maybe they know what is missing?

Can the datasheet be made public so that someone with more passion
can take a crack at it?

Cheers,
Peter

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-12 20:02, Ajay Gupta wrote:
> Hi Peter,
> 
>>> 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
>>> Changes from v11 -> v12
>>> Fixed review comments from Peter
>>> - Removed clearing of empty bits
>>> - Fix master_xfer for correct stop use
>>>
>>>  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 | 368
>> 
>>>  5 files changed, 403 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

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-11 19:45, 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
> Changes from v11 -> v12
>   Fixed review comments from Peter
>   - Removed clearing of empty bits
>   - Fix master_xfer for correct stop use
>   
>  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 | 368 
> 
>  5 files changed, 403 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
> d

Re: [PATCH v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-12 00:48, Ajay Gupta wrote:
>> If it doesn't work, maybe you can tweak something in gpu_i2c_read_byte to
>> make it work?
> I tried a few tweaks but it didn't work that way. I think it may take time to 
> get 
> this part working.
> 
> Can we update this part later when we have it working and let this patch 
> merged?

We could, but it feels like what I suggested should be *very* close. I'll write
a new response to v12 1/2 with a new attempt...

I've spent so much time looking at this, and this last limitation annoys me.

Cheers,
Peter

Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

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=153662481422174=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 v12 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-11 19:45, 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
> Changes from v11 -> v12
>   Fixed review comments from Peter
>   - Removed clearing of empty bits
>   - Fix master_xfer for correct stop use
>   
>  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 | 368 
> 
>  5 files changed, 403 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
> d

Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

[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 v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-11 06:30, Ajay Gupta wrote:
> Hi Peter,
> 
>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
>> +unsigned char buf1[USBC_MSG_OUT_SIZE];
>> +unsigned char buf2[USBC_CONTROL_SIZE];
>> +int status;
>> +u16 rab;
>> +
>> +memcpy(buf1, (u8 *)(uc->ppm.data) +
 USBC_MSG_OUT_OFFSET,
> sizeof(buf1));
>> +memcpy(buf2, (u8 *)(uc->ppm.data) +
 USBC_CONTROL_OFFSET,
>> +sizeof(buf2));
>
> Hmm, now that I see what this function does, instead of just
> seeing a bunch of magic numbers, I wonder why you make copies
> instead of feeding the correct section of the ppm.data buffer
> directly to ccg_write, like you do below for recv?
 Ok, will fix.
>>>
>>> Hmm, now that I see this again, it makes me wonder why you
>>> complained about copying the buffer to fix the misunderstanding of
>>> the i2c_transfer interface, when you already copy the buffer in
>>> the first
 place?
>> Copy is indeed not needed. I will fix it in next version.
>> We will have to do copy in ccg_write()if we try to combine two
>> write i2c_msg into one and I want to rather stay with two i2c_msg
>> to avoid
 copy.
>> Also master_xfer() will become tricky since rab write for read alsp
>> has to go
> first.
>
> You are stuck with the construction of the extended buffer. See my
> mail in the
> 1/2 thread.
>
>> +rab =
 CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
>> +status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
> USBC_VERSION_OFFSET,
>> +  USBC_VERSION_SIZE);
>
> E.g.
>   rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct
>> ucsi_data,
> version));
>   status = ccg_read(uc, rab, (u8 *)>ppm.data->version,
> sizeof(uc->ppm.data->version));
>
> Hmm, but this highlights that you are not doing any endian
> conversion of the fields in that struct as you read/write it.

> Do you need to in case you have an endian mismatch?
 Looks like don't need it. I have tested it and it works as is.
>>>
>>> Yeah, but have you tested the driver on a machine with the other
>>> byte-
 sex?
>> No, I think better to convert to desired endian.
>
> The device has a specific endianess. The host cpu has a specific 
> endianess.
> You transfer data byte-by-byte to/from a struct that appears to have
> multi- byte integers, e.g. the 16-bit version. You do not do any
> conversion that I see and you report that it works. So, there are
> two cases. Either
>
> 1. your host cpu and the device has the same endianess, and it all just
>works by accident
>
> or
>
> 2. whatever is consuming the ppm data does the endian conversion for
>> you
>on "the other side", and it all just works by design.
>
> I have no idea which it is since I know nothing about whatever
> handles the ppm data on the other side of that ucsi_register_ppm call. So,
>> I asked.
 UCSI specification requires the ppm data to be in little-endian format.

 Below is from the UCSI specification.
 "All multiple byte fields in this specification are interpreted as
 and moved over the bus in little-endian order, i.e., LSB to MSB unless
>> otherwise specified"

Taking another peek into the UCSI spec, and I do not find any mention of
any rab. So, I think the rab is out of scope for that spec. I.e. that the
rab falls into this bucket:

This specification does not define the method to use
(PCIe/ACPI/I2C/etc.) in order to interface with the PPM.
It is left to individual system manufacturers to determine 
what bus/protocol they use to expose the PPM.

What abbreviation is rab anyway? Register Address Block?

>>>
>>> Do we still need any conversion here? The ppm data is now directly fed
>>> for read and write both and rab should be in little endian as per macro.
>>> #define CCGX_RAB_UCSI_DATA_BLOCK(offset)(0xf000 | ((offset) & 0xff))
>>
>> What do you mean by "in little endian as per macro"?
>> Should not the non-offset 0xf0 byte of CCGX_RAB_UCSI_DATA_BLOCK 
>> be in the other byte of rab
> Shouldn't it be always in bits D[8:15] of rab so that it gets written to 
> buf[1] (2nd byte) in ccg_read/write() ?
> 
>> compared to e.g. the 0x06 byte of CCGX_RAB_INTR_REG?
>>
>> I assumed *all* CCGX_RAB_... defines to be in cpu-native endian. 
>> Are they not?
> How to know/confirm this?

I don't know what you want to have on the wire, but for the code as
written, a rab that is CCGX_RAB_UCSI_DATA_BLOCK(0x10) gets the bytes
0x10,0xf0 and for a rab that is CCGX_RAB_INTR_REG you get 0x06,0x00

What makes me a little worried is that the offset of the data-block

Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

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
> 

Re: [PATCH v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-10 23:53, Ajay Gupta wrote:
> Hi Peter
> 
 +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
 +  unsigned char buf1[USBC_MSG_OUT_SIZE];
 +  unsigned char buf2[USBC_CONTROL_SIZE];
 +  int status;
 +  u16 rab;
 +
 +  memcpy(buf1, (u8 *)(uc->ppm.data) +
>> USBC_MSG_OUT_OFFSET,
>>> sizeof(buf1));
 +  memcpy(buf2, (u8 *)(uc->ppm.data) +
>> USBC_CONTROL_OFFSET,
 +sizeof(buf2));
>>>
>>> Hmm, now that I see what this function does, instead of just
>>> seeing a bunch of magic numbers, I wonder why you make copies
>>> instead of feeding the correct section of the ppm.data buffer
>>> directly to ccg_write, like you do below for recv?
>> Ok, will fix.
>
> Hmm, now that I see this again, it makes me wonder why you
> complained about copying the buffer to fix the misunderstanding of
> the i2c_transfer interface, when you already copy the buffer in the first
>> place?
 Copy is indeed not needed. I will fix it in next version.
 We will have to do copy in ccg_write()if we try to combine two write
 i2c_msg into one and I want to rather stay with two i2c_msg to avoid
>> copy.
 Also master_xfer() will become tricky since rab write for read alsp
 has to go
>>> first.
>>>
>>> You are stuck with the construction of the extended buffer. See my
>>> mail in the
>>> 1/2 thread.
>>>
 +  rab =
>> CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
 +  status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
>>> USBC_VERSION_OFFSET,
 +USBC_VERSION_SIZE);
>>>
>>> E.g.
>>> rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
>>> version));
>>> status = ccg_read(uc, rab, (u8 *)>ppm.data->version,
>>>   sizeof(uc->ppm.data->version));
>>>
>>> Hmm, but this highlights that you are not doing any endian
>>> conversion of the fields in that struct as you read/write it.
>>
>>> Do you need to in case you have an endian mismatch?
>> Looks like don't need it. I have tested it and it works as is.
>
> Yeah, but have you tested the driver on a machine with the other byte-
>> sex?
 No, I think better to convert to desired endian.
>>>
>>> The device has a specific endianess. The host cpu has a specific endianess.
>>> You transfer data byte-by-byte to/from a struct that appears to have
>>> multi- byte integers, e.g. the 16-bit version. You do not do any
>>> conversion that I see and you report that it works. So, there are two
>>> cases. Either
>>>
>>> 1. your host cpu and the device has the same endianess, and it all just
>>>works by accident
>>>
>>> or
>>>
>>> 2. whatever is consuming the ppm data does the endian conversion for you
>>>on "the other side", and it all just works by design.
>>>
>>> I have no idea which it is since I know nothing about whatever handles
>>> the ppm data on the other side of that ucsi_register_ppm call. So, I asked.
>> UCSI specification requires the ppm data to be in little-endian format.
>>
>> Below is from the UCSI specification.
>> "All multiple byte fields in this specification are interpreted as and moved
>> over the bus in little-endian order, i.e., LSB to MSB unless otherwise 
>> specified"
> 
> Do we still need any conversion here? The ppm data is now directly fed for 
> read
> and write both and rab should be in little endian as per macro. 
> #define CCGX_RAB_UCSI_DATA_BLOCK(offset)(0xf000 | ((offset) & 0xff))

What do you mean by "in little endian as per macro"? Should not the non-offset
0xf0 byte of CCGX_RAB_UCSI_DATA_BLOCK be in the other byte of rab compared to
e.g. the 0x06 byte of CCGX_RAB_INTR_REG?

I assumed *all* CCGX_RAB_... defines to be in cpu-native endian. Are they not?

In case they are, I think that no further conversion is needed. You feed rab
to ccg_read/ccg_write as cpu-native, and ccg_read/ccg_write then converts to
little-endian (with put_unaligned_le16).

And it's actually crucial that rab is cpu-native when ccg_read performs
arithmetic on it, otherwise the result can turn out to be garbage...

Cheers,
Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 23:38, Ajay Gupta wrote:
>> My guess is that with the above, you can actually program the I2C_MST_ADDR
>> register inside the "if (Read)" branch and then remove the I2C_AQ_COMB |
>> I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.
> 
> We need I2C_MST_ADDR to be programmed even in a single write message.
> If we move it inside "if (Read)" branch then a single write message will not 
> get it.
> 
> How does the below look?
> 
> for (i = 0; i < num; i++) {
>   write msg[i].addr to I2C_MST_ADDR;
>   if (Read) {
>   read (without stop);
>   } else  {
>   start;
>   addr;
>   write;
>   }
>  }
>  stop;

Sure, that's perfectly fine. I just don't see why the chip needs that for
writes when the address has to be written manually anyway, but if it does
need it, then so it is (and if it doesn't need it, the penalty for the
extra register write is probably not all that significant...)

Cheers,
Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 22:04, Ajay Gupta wrote:
> Hi Peter,
> 
> + if (msgs[i].flags & I2C_M_RD) {
> + /* gpu_i2c_read has implicit start and stop */
> + status = gpu_i2c_read(i2cd, msgs[i].buf,
>> msgs[i].len);
> + if (status < 0)
> + return status;
> + } else {
> + /* start on first write message */
> + if (i == 0) {

 This "if (i == 0)" test is completely bogus to me. I fail to see
 why the meat of the block should not happen for both writes in a
 double-write transfer.

 If the second message is a write, you do not issue any start nor
 do you write out the address for the second message. You want to
 generate the following for a transfer consisting of 2 write-messages:

   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
  =

 (where "..." denotes further optional "Data [A]" instances)

 As is, the stuff underlined by equal signs are not generated, at
 least as I read the code.

 This is what I meant in my comment around this area for the v9 patch.
>>>
>>> Oh, I just realized, this probably means that the ccg_write
>>> function in patch
>>> 2/2 asks for the wrong thing. If this code actually works, the
>>> client driver should probably ask for a single-message transfer
>>> consisting of the 2-byte rab concatenated with the data buffer.
>>> And that actually makes sense, there is no reason to split the two
>>> (dependent) parts of the write into separate messages.
>> That would require to create new buffer and copy data for each
>> write request from UCSI core driver for sending UCSI command. This
>> doesn't look proper way of doing it.
>
> Well, that's the way master_xfer works, so you will just have to
> live with it if you do not want a stop in the middle.

 Bzzt, s/stop/restart/
>>> Obviously, a stop in middle will not work. For any read or write
>>> request,
>>
>> Yes, with "s/stop/restart/", I meant "replace stop with restart", and not 
>> "stop
>> followed by restart". That's pretty standard notation, but sorry if that was 
>> not
>> clear.
>>
>>> 2 byte rab write has to go first. We anyway have to separate rab part
>>> in read transactions so write should be okay to have separate rab
>>> write message.
>>
>> That is not at all certain. The two below transfers need not mean the same
>> thing at all.
>>
>> S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P S Addr Wr [A] rab1 [A] rab2 
>> [A] S
>> Addr Wr [A] Data [A] ... P
>>
>> In fact, I strongly suspect that for the latter case, the device will 
>> interpret the
>> first two Data bytes as rab and that you therefore have to make sure that the
>> first transfer (w/o restart) is what you generate in ccg_write.
>>
>>> All the client driver of this needs to create messages in pairs where
>>> first one will always be a rab write and second one either read or write.
>>> I can add a check for this condition in master_xfer if needed.
>>>
>>> Do you still see any issue with this?
>>
>> Yes, this is very much an issue. master_xfer has to start each message with a
>> start for the first message, and a restart for subsequent messages. And after
>> the final message there should be a stop. All starts/restarts should be
>> followed by the address and the read/write bit. Then data in the given
>> direction. The master_xfer function should never do any device specific
>> checks for device specific conditions. It simply should not care about which
>> client driver is currently using it, and instead just stick to implementing 
>> the
>> master_xfer api so that all client drivers know what to expect. No cludges.
> 
> I didn't get how read will happen. Read will also require a 2 byte rab write 
> and
> followed by read. 
> 
> Are you saying to have two messages for reads [rab + read]
> and combined (rab and write) single message for write?
> Then for read: rab is written first followed by reads and then actual 
> explicit stop?
> 
> for (i = 0; i < num; i++)
>   if (Read) {
>   read (without stop);
>   } else  {
>   start;
>   addr;
>   write;
>   }
> }
> stop;

Yes, exactly like that (just missing a { for the for loop). If you can do reads
without an implicit stop, that's excellent news!

My guess is that with the above, you can actually program the I2C_MST_ADDR
register inside the "if (Read)" branch and then remove the
I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.

Cheers,
Peter

> 
> Thanks
> Ajay
> 
> --
> nvpublic
> --
>>
>> Cheers,
>> Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 19:43, Ajay Gupta wrote:
> Hi Peter,
> 
>>> +
>>> +   if (msgs[i].flags & I2C_M_RD) {
>>> +   /* gpu_i2c_read has implicit start and stop */
>>> +   status = gpu_i2c_read(i2cd, msgs[i].buf, 
>>> msgs[i].len);
>>> +   if (status < 0)
>>> +   return status;
>>> +   } else {
>>> +   /* start on first write message */
>>> +   if (i == 0) {
>>
>> This "if (i == 0)" test is completely bogus to me. I fail to see
>> why the meat of the block should not happen for both writes in a
>> double-write transfer.
>>
>> If the second message is a write, you do not issue any start nor do
>> you write out the address for the second message. You want to
>> generate the following for a transfer consisting of 2 write-messages:
>>
>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>  =
>>
>> (where "..." denotes further optional "Data [A]" instances)
>>
>> As is, the stuff underlined by equal signs are not generated, at
>> least as I read the code.
>>
>> This is what I meant in my comment around this area for the v9 patch.
>
> Oh, I just realized, this probably means that the ccg_write function
> in patch
> 2/2 asks for the wrong thing. If this code actually works, the
> client driver should probably ask for a single-message transfer
> consisting of the 2-byte rab concatenated with the data buffer.
> And that actually makes sense, there is no reason to split the two
> (dependent) parts of the write into separate messages.
 That would require to create new buffer and copy data for each write
 request from UCSI core driver for sending UCSI command. This doesn't
 look proper way of doing it.
>>>
>>> Well, that's the way master_xfer works, so you will just have to live
>>> with it if you do not want a stop in the middle.
>>
>> Bzzt, s/stop/restart/
> Obviously, a stop in middle will not work. For any read or write request,

Yes, with "s/stop/restart/", I meant "replace stop with restart", and not
"stop followed by restart". That's pretty standard notation, but sorry if
that was not clear.

> 2 byte rab write has to go first. We anyway have to separate rab part
> in read transactions so write should be okay to have separate rab write
> message.

That is not at all certain. The two below transfers need not mean the same
thing at all.

S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P
S Addr Wr [A] rab1 [A] rab2 [A] S Addr Wr [A] Data [A] ... P

In fact, I strongly suspect that for the latter case, the device will
interpret the first two Data bytes as rab and that you therefore have to
make sure that the first transfer (w/o restart) is what you generate in
ccg_write.

> All the client driver of this needs to create messages in pairs where first
> one will always be a rab write and second one either read or write.
> I can add a check for this condition in master_xfer if needed.
> 
> Do you still see any issue with this?

Yes, this is very much an issue. master_xfer has to start each message with
a start for the first message, and a restart for subsequent messages. And
after the final message there should be a stop. All starts/restarts should
be followed by the address and the read/write bit. Then data in the given
direction. The master_xfer function should never do any device specific
checks for device specific conditions. It simply should not care about
which client driver is currently using it, and instead just stick to
implementing the master_xfer api so that all client drivers know what to
expect. No cludges.

Cheers,
Peter

Re: [PATCH v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-10 20:51, Ajay Gupta wrote:
> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
> + unsigned char buf1[USBC_MSG_OUT_SIZE];
> + unsigned char buf2[USBC_CONTROL_SIZE];
> + int status;
> + u16 rab;
> +
> + memcpy(buf1, (u8 *)(uc->ppm.data) + USBC_MSG_OUT_OFFSET,
 sizeof(buf1));
> + memcpy(buf2, (u8 *)(uc->ppm.data) + USBC_CONTROL_OFFSET,
> +sizeof(buf2));

 Hmm, now that I see what this function does, instead of just seeing a
 bunch of magic numbers, I wonder why you make copies instead of
 feeding the correct section of the ppm.data buffer directly to
 ccg_write, like you do below for recv?
>>> Ok, will fix.
>>
>> Hmm, now that I see this again, it makes me wonder why you complained
>> about copying the buffer to fix the misunderstanding of the i2c_transfer
>> interface, when you already copy the buffer in the first place?
> Copy is indeed not needed. I will fix it in next version. 
> We will have to do copy in ccg_write()if we try to combine two write i2c_msg
> into one and I want to rather stay with two i2c_msg to avoid copy. 
> Also master_xfer() will become tricky since rab write for read alsp has to go 
> first.

You are stuck with the construction of the extended buffer. See my mail
in the 1/2 thread.

> + rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
> + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
 USBC_VERSION_OFFSET,
> +   USBC_VERSION_SIZE);

 E.g.
rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
 version));
status = ccg_read(uc, rab, (u8 *)>ppm.data->version,
  sizeof(uc->ppm.data->version));

 Hmm, but this highlights that you are not doing any endian conversion
 of the fields in that struct as you read/write it.
>>>
 Do you need to in case you have an endian mismatch?
>>> Looks like don't need it. I have tested it and it works as is.
>>
>> Yeah, but have you tested the driver on a machine with the other byte-sex?
> No, I think better to convert to desired endian.

The device has a specific endianess. The host cpu has a specific endianess.
You transfer data byte-by-byte to/from a struct that appears to have
multi-byte integers, e.g. the 16-bit version. You do not do any conversion
that I see and you report that it works. So, there are two cases. Either

1. your host cpu and the device has the same endianess, and it all just
   works by accident

or

2. whatever is consuming the ppm data does the endian conversion for you
   on "the other side", and it all just works by design.

I have no idea which it is since I know nothing about whatever handles the
ppm data on the other side of that ucsi_register_ppm call. So, I asked.

Cheers,
Peter

Re: [PATCH v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-10 19:32, Ajay Gupta wrote:
>>> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> +   struct i2c_client *client = uc->client;
>>> +   unsigned char buf[2];
>>> +   struct i2c_msg msgs[] = {
>>> +   {
>>> +   .addr   = client->addr,
>>> +   .flags  = 0x0,
>>> +   .len= 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> +   .buf= buf,
>>> +   },
>>> +   {
>>> +   .addr   = client->addr,
>>> +   .flags  = I2C_M_RD,
>>> +   .buf= data,
>>> +   },
>>> +   };
>>> +   u32 rlen, rem_len = len;
>>> +   int status;
>>> +
>>
>> If your target I2C adapter had supported larger reads, this would have been a
>> single xfer instead of the loop, correct? I think this deserves a comment, 
>> and
>> perhaps e.g. the eeprom drivers should be examined to see how they handle
>> deficient I2C adapters (there is a module_param named io_limit in the at24
>> driver). Because it is a little bit sad to penalise all users just because 
>> you have
>> an adapter with limitations.
>> Or is this driver tied to that adapter?
>> Anyway, I'm satisfied with a comment, as I don't care all that much.
> I am thinking of moving the loop to the i2c driver. 

No, that will not work. The i2c driver cannot know how the splitting works,
since how to split transfers will typically be different for different drivers
needing large reads.

You will have to do the split here in this driver.

>>> +   while (rem_len > 0) {
>>> +   msgs[1].buf = [len - rem_len];
>>> +   rlen = min_t(u16, rem_len, 4);
>>> +   msgs[1].len = rlen;
>>> +   put_unaligned_le16(rab, buf);
>>> +   status = i2c_transfer(client->adapter, msgs,
>> ARRAY_SIZE(msgs));
>>> +   if (status < 0) {
>>> +   dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> +   return status;
>>> +   }
>>> +   rab += rlen;
>>> +   rem_len -= rlen;
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> +   struct i2c_client *client = uc->client;
>>> +   unsigned char buf[2];
>>> +   struct i2c_msg msgs[] = {
>>> +   {
>>> +   .addr   = client->addr,
>>> +   .flags  = 0x0,
>>> +   .len= 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> +   .buf= buf,
>>> +   },
>>> +   {
>>> +   .addr   = client->addr,
>>> +   .flags  = 0x0,
>>> +   .buf= data,
>>> +   .len= len,
>>> +   },
>>> +   };
>>> +   int status;
>>> +
>>> +   put_unaligned_le16(rab, buf);
>>> +   status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>>> +   if (status < 0) {
>>> +   dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> +   return status;
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
>>> +   struct device *dev = uc->dev;
>>> +   unsigned int count = 10;
>>> +   u8 data[64];
>>> +   int status;
>>> +
>>> +   status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
>> sizeof(data));
>>> +   if (status < 0)
>>> +   return status;
>>> +
>>> +   dev_dbg(dev, "Silicon id %2ph", data +
>> CCGX_I2C_RAB_READ_SILICON_ID);
>>> +   dev_dbg(dev, "FW1 version %8ph\n", data +
>> CCGX_I2C_RAB_FW1_VERSION);
>>> +   dev_dbg(dev, "FW2 version %8ph\n", data +
>> CCGX_I2C_RAB_FW2_VERSION);
>>> +
>>> +   data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
>>> +   status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> +   if (status < 0)
>>> +   return status;
>>> +
>>> +   data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
>>> +   status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> +   if (status < 0)
>>> +   return status;
>>> +
>>> +   /*
>>> +* Flush CCGx RESPONSE queue by acking interrupts
>>> +* - above ucsi control register write will push response
>>> +* which must be flushed
>>> +* - affects f/w update which reads response register
>>> +*/
>>> +   data[0] = 0xff;
>>> +   do {
>>> +   status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> +   if (status < 0)
>>> +   return status;
>>> +
>>> +   usleep_range(1, 11000);
>>> +
>>> +   status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> +   if (status < 0)
>>> +   return status;
>>> +   } while ((data[0] != 0x00) && count--);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
>>> +   unsigned char buf1[USBC_MSG_OUT_SIZE];
>>> +   unsigned char buf2[USBC_CONTROL_SIZE];
>>> +   int status;
>>> +   u16 rab;
>>> +
>>> +   memcpy(buf1, (u8 *)(uc->ppm.data) + USBC_MSG_OUT_OFFSET,
>> sizeof(buf1));
>>> +   memcpy(buf2, (u8 

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 19:28, Peter Rosin wrote:
> On 2018-09-10 18:08, Ajay Gupta wrote:
>> Hi Peter,
>>
>>>>> +
>>>>> + if (msgs[i].flags & I2C_M_RD) {
>>>>> + /* gpu_i2c_read has implicit start and stop */
>>>>> + status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>>>> + if (status < 0)
>>>>> + return status;
>>>>> + } else {
>>>>> + /* start on first write message */
>>>>> + if (i == 0) {
>>>>
>>>> This "if (i == 0)" test is completely bogus to me. I fail to see why
>>>> the meat of the block should not happen for both writes in a
>>>> double-write transfer.
>>>>
>>>> If the second message is a write, you do not issue any start nor do
>>>> you write out the address for the second message. You want to generate
>>>> the following for a transfer consisting of 2 write-messages:
>>>>
>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>>  =
>>>>
>>>> (where "..." denotes further optional "Data [A]" instances)
>>>>
>>>> As is, the stuff underlined by equal signs are not generated, at least
>>>> as I read the code.
>>>>
>>>> This is what I meant in my comment around this area for the v9 patch.
>>>
>>> Oh, I just realized, this probably means that the ccg_write function in 
>>> patch
>>> 2/2 asks for the wrong thing. If this code actually works, the client driver
>>> should probably ask for a single-message transfer consisting of the 2-byte 
>>> rab
>>> concatenated with the data buffer. 
>>> And that actually makes sense, there is no
>>> reason to split the two (dependent) parts of the write into separate 
>>> messages.
>> That would require to create new buffer and copy data for each write request
>> from UCSI core driver for sending UCSI command. This doesn't look proper way
>> of doing it.
> 
> Well, that's the way master_xfer works, so you will just have to live with it
> if you do not want a stop in the middle.

Bzzt, s/stop/restart/

Cheers,
Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 18:08, Ajay Gupta wrote:
> Hi Peter,
> 
 +
 +  if (msgs[i].flags & I2C_M_RD) {
 +  /* gpu_i2c_read has implicit start and stop */
 +  status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
 +  if (status < 0)
 +  return status;
 +  } else {
 +  /* start on first write message */
 +  if (i == 0) {
>>>
>>> This "if (i == 0)" test is completely bogus to me. I fail to see why
>>> the meat of the block should not happen for both writes in a
>>> double-write transfer.
>>>
>>> If the second message is a write, you do not issue any start nor do
>>> you write out the address for the second message. You want to generate
>>> the following for a transfer consisting of 2 write-messages:
>>>
>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>  =
>>>
>>> (where "..." denotes further optional "Data [A]" instances)
>>>
>>> As is, the stuff underlined by equal signs are not generated, at least
>>> as I read the code.
>>>
>>> This is what I meant in my comment around this area for the v9 patch.
>>
>> Oh, I just realized, this probably means that the ccg_write function in patch
>> 2/2 asks for the wrong thing. If this code actually works, the client driver
>> should probably ask for a single-message transfer consisting of the 2-byte 
>> rab
>> concatenated with the data buffer. 
>> And that actually makes sense, there is no
>> reason to split the two (dependent) parts of the write into separate 
>> messages.
> That would require to create new buffer and copy data for each write request
> from UCSI core driver for sending UCSI command. This doesn't look proper way
> of doing it.

Well, that's the way master_xfer works, so you will just have to live with it
if you do not want a stop in the middle.

Cheers,
Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-08 21:17, Peter Rosin wrote:
>> +
>> +if (msgs[i].flags & I2C_M_RD) {
>> +/* gpu_i2c_read has implicit start and stop */
>> +status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>> +if (status < 0)
>> +return status;
>> +} else {
>> +/* start on first write message */
>> +if (i == 0) {
> 
> This "if (i == 0)" test is completely bogus to me. I fail to see why the
> meat of the block should not happen for both writes in a double-write
> transfer.
> 
> If the second message is a write, you do not issue any start nor do you
> write out the address for the second message. You want to generate the
> following for a transfer consisting of 2 write-messages:
> 
>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>  =
> 
> (where "..." denotes further optional "Data [A]" instances)
> 
> As is, the stuff underlined by equal signs are not generated, at least as
> I read the code.
> 
> This is what I meant in my comment around this area for the v9 patch.

Oh, I just realized, this probably means that the ccg_write function in
patch 2/2 asks for the wrong thing. If this code actually works, the client
driver should probably ask for a single-message transfer consisting of the
2-byte rab concatenated with the data buffer. And that actually makes sense,
there is no reason to split the two (dependent) parts of the write into
separate messages.

Otherwise, fixing the bug here will break your client driver, but two bugs
that just happen to cancel each other out are not what we want...

All assuming my hunch is correct, of course.

Cheers,
Peter

Re: [PATCH v10 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

Andy, there's a question for you below.

On 2018-09-08 02:09, 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
>   
>  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 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,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..c231121
> --- /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
> + 

Re: [PATCH v9 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-08 16:13, Ajay Gupta wrote:
> 
> Hi Peter,
> 
> .
> 
>> SMBUS has nothing to do with the >problem, that was just an example. An I2C 
>> >client driver can issue such I2C xfers all by >itself without going through 
>> emulation, so >just dropping the _EMUL flag is not the >answer. And I'd be 
>> surprised if the >hardware doesn't support single message >reads.
> 
>> There is no quirk flag for this abnormality, >so you will have to open code 
>> the check in >your master_xfer if you can't make such >xfers work, but the 
>> best fix is certainly to >just make them work...
> 
> I have fixed that in v10. Please check

Ah, I didn't even bother to look earlier. I have now taken a peek, and
there are still issues. I'll comment inline for the v10 1/2 patch as
usual, but I'm short on time at the moment so it will probably be a few
hours. It's an option to hold on to v11 until that happens...

Cheers,
Peter

Re: [PATCH v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-08 02:09, Ajay Gupta wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
> 
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> 
> Signed-off-by: Ajay Gupta 
> Reviewed-by: Andy Shevchenko 
> Acked-by: Heikki Krogerus 
> ---
> Changes from v1 -> v2
>   Fixed identation in drivers/usb/typec/ucsi/Kconfig
> Changes from v2 -> v3
>   Fixed most of comments from Heikki
>   Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> Changes from v3 -> v4
>   Fixed comments from Andy
> Changes from v4 -> v5
>   Fixed comments from Andy
> Changes from v5 -> v6
>   Fixed review comments from Greg 
> Changes from v6 -> v7
>   None
> Changes from v7 -> v8
>   Fixed review comments from Peter 
>   - Removed empty STOP message
>   - Using stack memory for i2c_transfer()
> Changes from v8 -> v9
>   None
> Changes from v9 -> v10
>   Fixed review comments from Peter 
>   - Use UCSI macros
>   - Cleanups
> 
>  drivers/usb/typec/ucsi/Kconfig|  10 ++
>  drivers/usb/typec/ucsi/Makefile   |   2 +
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 324 
> ++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..7811888 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>  
>  if TYPEC_UCSI
>  
> +config UCSI_CCG
> + tristate "UCSI Interface Driver for Cypress CCGx"
> + depends on I2C
> + help
> +   This driver enables UCSI support on platforms that expose a
> +   Cypress CCGx Type-C controller over I2C interface.
> +
> +   To compile the driver as a module, choose M here: the module will be
> +   called ucsi_ccg.
> +
>  config UCSI_ACPI
>   tristate "UCSI ACPI Interface Driver"
>   depends on ACPI
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 7afbea5..2f4900b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -8,3 +8,5 @@ typec_ucsi-y  := ucsi.o
>  typec_ucsi-$(CONFIG_TRACING) += trace.o
>  
>  obj-$(CONFIG_UCSI_ACPI)  += ucsi_acpi.o
> +
> +obj-$(CONFIG_UCSI_CCG)   += ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c 
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> new file mode 100644
> index 000..c346e6a
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta 
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include "ucsi.h"
> +
> +struct ucsi_ccg {
> + struct device *dev;
> + struct ucsi *ucsi;
> + struct ucsi_ppm ppm;
> + struct i2c_client *client;
> + int irq;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE 0x00
> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x2
> +#define CCGX_I2C_RAB_INTR_REG0x06
> +#define CCGX_I2C_RAB_FW1_VERSION 0x28
> +#define CCGX_I2C_RAB_FW2_VERSION 0x20
> +#define CCGX_I2C_RAB_UCSI_CONTROL0x39
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START  BIT(0)
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP   BIT(1)
> +#define CCGX_I2C_RAB_RESPONSE_REG0x7E
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
> +
> +#define USBC_VERSION_OFFSET  (0x0)
> +#define USBC_VERSION_SIZE(2)
> +#define USBC_CCI_OFFSET  (0x4)
> +#define USBC_CCI_SIZE(4)
> +#define USBC_CONTROL_OFFSET  (0x8)
> +#define USBC_CONTROL_SIZE(8)
> +#define USBC_MSG_IN_OFFSET   (0x10)
> +#define USBC_MSG_IN_SIZE (16)
> +#define USBC_MSG_OUT_OFFSET  (0x20)
> +#define USBC_MSG_OUT_SIZE(16)
> +
> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> + struct i2c_client *client = uc->client;
> + unsigned char buf[2];
> + struct i2c_msg msgs[] = {
> + {
> + .addr   = client->addr,
> + .flags  = 0x0,
> + .len= 0x2,

sizeof(buf)?

> + .buf= buf,
> + },
> + {
> + .addr   = client->addr,
> + .flags  = I2C_M_RD,
> + .buf= data,
> + },
> + };
> + u32 rlen, rem_len = len;
> + int status;
> +

If your target I2C adapter had supported larger reads, this would have
been a single xfer instead of 

RE: [PATCH v9 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On September 7, 2018 11:48:37 PM GMT+02:00, Ajay Gupta  wrote:

>> >> Per your comments on v8, the address has to be programmed before
>the
>> >> transfer, but you fail to do that if the first message is a read.
>> > This will never happen. Hint: I2C_AQ_COMB_WRITE_FIRST
>> 
>> Yes, it will. If the transfer consists of a single read, i.e. w/o any
>leading write.
>> E.g. i2c_smbus_read_byte (which is emulated in your case and will
>generate
>> this pattern).
>I don't think we intend to support SMBUS commands and so I will drop
>I2C_FUNC_SMBUS_EMUL.

SMBUS has nothing to do with the problem, that was just an example. An I2C 
client driver can issue such I2C xfers all by itself without going through 
emulation, so just dropping the _EMUL flag is not the answer. And I'd be 
surprised if the hardware doesn't support single message reads.

There is no quirk flag for this abnormality, so you will have to open code the 
check in your master_xfer if you can't make such xfers work, but the best fix 
is certainly to just make them work...

Cheers,
Peter

Re: [PATCH v9 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-07 19:28, Ajay Gupta wrote:
> Hi Peter,
> 
>> -Original Message-----
>> From: Peter Rosin 
>> Sent: Friday, September 7, 2018 2:13 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 v9 2/2] usb: typec: ucsi: add support for Cypress CCGx
>>
>> On 2018-09-07 01:56, Ajay Gupta wrote:
>>> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller over I2C
>>> interface.
>>>
>>> This UCSI I2C driver uses I2C bus driver interface for communicating
>>> with Type-C controller.
>>>
>>> Signed-off-by: Ajay Gupta 
>>> Reviewed-by: Andy Shevchenko 
>>> Acked-by: Heikki Krogerus 
>>> ---
>>> Changes from v1 -> v2
>>> Fixed identation in drivers/usb/typec/ucsi/Kconfig Changes from v2 ->
>>> v3
>>> Fixed most of comments from Heikki
>>> Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
>>> Changes from v3 -> v4
>>> Fixed comments from Andy
>>> Changes from v4 -> v5
>>> Fixed comments from Andy
>>> Changes from v5 -> v6
>>> Fixed review comments from Greg
>>> Changes from v6 -> v7
>>> None
>>> Changes from v7 -> v8
>>> Fixed review comments from Peter
>>> - Removed empty STOP message
>>> - Using stack memory for i2c_transfer() Changes from v8 -> v9
>>> None
>>>
>>>  drivers/usb/typec/ucsi/Kconfig|  10 ++
>>>  drivers/usb/typec/ucsi/Makefile   |   2 +
>>>  drivers/usb/typec/ucsi/ucsi_ccg.c | 335
>>> ++
>>>  3 files changed, 347 insertions(+)
>>>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
>>>
>>> diff --git a/drivers/usb/typec/ucsi/Kconfig
>>> b/drivers/usb/typec/ucsi/Kconfig index e36d6c7..7811888 100644
>>> --- a/drivers/usb/typec/ucsi/Kconfig
>>> +++ b/drivers/usb/typec/ucsi/Kconfig
>>> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>>>
>>>  if TYPEC_UCSI
>>>
>>> +config UCSI_CCG
>>> +   tristate "UCSI Interface Driver for Cypress CCGx"
>>> +   depends on I2C
>>> +   help
>>> + This driver enables UCSI support on platforms that expose a
>>> + Cypress CCGx Type-C controller over I2C interface.
>>> +
>>> + To compile the driver as a module, choose M here: the module will
>> be
>>> + called ucsi_ccg.
>>> +
>>>  config UCSI_ACPI
>>> tristate "UCSI ACPI Interface Driver"
>>> depends on ACPI
>>> diff --git a/drivers/usb/typec/ucsi/Makefile
>>> b/drivers/usb/typec/ucsi/Makefile index 7afbea5..2f4900b 100644
>>> --- a/drivers/usb/typec/ucsi/Makefile
>>> +++ b/drivers/usb/typec/ucsi/Makefile
>>> @@ -8,3 +8,5 @@ typec_ucsi-y:= ucsi.o
>>>  typec_ucsi-$(CONFIG_TRACING)   += trace.o
>>>
>>>  obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
>>> +
>>> +obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
>>> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> new file mode 100644
>>> index 000..387b6fd
>>> --- /dev/null
>>> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
>>> @@ -0,0 +1,335 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * UCSI driver for Cypress CCGx Type-C controller
>>> + *
>>> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
>>> + * Author: Ajay Gupta 
>>> + *
>>> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
>>> + */
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include "ucsi.h"
>>> +
>>> +struct ucsi_ccg {
>>> +   struct device *dev;
>>> +   struct ucsi *ucsi;
>>> +   struct ucsi_ppm ppm;
>>> +   struct i2c_client *client;
>>> +   int irq;
>>> +};
>>> +
>>> +#define CCGX_I2C_RAB_DEVICE_MODE   0x00
>>> +#define CCGX_I2C_RAB_READ_SILICON_ID   0x2
>>> +#define CCGX_I2C_RAB_INTR_REG  0x06
>>> +#define CCGX_I2C_RAB_FW1_VERSION   0x28
>>> +#define CCGX_I2C_RAB_FW2_VERSION   

Re: [PATCH v9 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-07 19:15, Ajay Gupta wrote:
> Hi Peter,
>>> 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()
>>>
>>>  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 | 396
>> 
>>>  5 files changed, 431 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 9ad052a..2d1c5a1 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6797,6 +6797,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) 

Re: [PATCH v9 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-07 01:56, 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()
>   
>  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 | 396 
> 
>  5 files changed, 431 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 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,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..4a63a4e4
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,396 @@
> +// 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 
> +#i

Re: [PATCH v9 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-07 01:56, Ajay Gupta wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
> 
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> 
> Signed-off-by: Ajay Gupta 
> Reviewed-by: Andy Shevchenko 
> Acked-by: Heikki Krogerus 
> ---
> Changes from v1 -> v2
>   Fixed identation in drivers/usb/typec/ucsi/Kconfig
> Changes from v2 -> v3
>   Fixed most of comments from Heikki
>   Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> Changes from v3 -> v4
>   Fixed comments from Andy
> Changes from v4 -> v5
>   Fixed comments from Andy
> Changes from v5 -> v6
>   Fixed review comments from Greg 
> Changes from v6 -> v7
>   None
> Changes from v7 -> v8
>   Fixed review comments from Peter 
>   - Removed empty STOP message
>   - Using stack memory for i2c_transfer()
> Changes from v8 -> v9
>   None
> 
>  drivers/usb/typec/ucsi/Kconfig|  10 ++
>  drivers/usb/typec/ucsi/Makefile   |   2 +
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 335 
> ++
>  3 files changed, 347 insertions(+)
>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..7811888 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>  
>  if TYPEC_UCSI
>  
> +config UCSI_CCG
> + tristate "UCSI Interface Driver for Cypress CCGx"
> + depends on I2C
> + help
> +   This driver enables UCSI support on platforms that expose a
> +   Cypress CCGx Type-C controller over I2C interface.
> +
> +   To compile the driver as a module, choose M here: the module will be
> +   called ucsi_ccg.
> +
>  config UCSI_ACPI
>   tristate "UCSI ACPI Interface Driver"
>   depends on ACPI
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 7afbea5..2f4900b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -8,3 +8,5 @@ typec_ucsi-y  := ucsi.o
>  typec_ucsi-$(CONFIG_TRACING) += trace.o
>  
>  obj-$(CONFIG_UCSI_ACPI)  += ucsi_acpi.o
> +
> +obj-$(CONFIG_UCSI_CCG)   += ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c 
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> new file mode 100644
> index 000..387b6fd
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta 
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include "ucsi.h"
> +
> +struct ucsi_ccg {
> + struct device *dev;
> + struct ucsi *ucsi;
> + struct ucsi_ppm ppm;
> + struct i2c_client *client;
> + int irq;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE 0x00
> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x2
> +#define CCGX_I2C_RAB_INTR_REG0x06
> +#define CCGX_I2C_RAB_FW1_VERSION 0x28
> +#define CCGX_I2C_RAB_FW2_VERSION 0x20
> +#define CCGX_I2C_RAB_UCSI_CONTROL0x39
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START  BIT(0)
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP   BIT(1)
> +#define CCGX_I2C_RAB_RESPONSE_REG0x7E
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000
> +
> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> + struct i2c_client *client = uc->client;
> + unsigned char buf[2];
> + struct i2c_msg msgs[] = {
> + {
> + .addr   = client->addr,
> + .flags  = 0x0,
> + .len= 0x2,
> + .buf= buf,
> + },
> + {
> + .addr   = client->addr,
> + .flags  = I2C_M_RD,
> + .buf= data,
> + },
> + };
> + u32 rlen, rem_len = len;
> + int status;
> +
> + while (rem_len > 0) {
> + msgs[1].buf = [len - rem_len];
> + rlen = min_t(u16, rem_len, 4);
> + msgs[1].len = rlen;
> + put_unaligned_le16(rab, buf);

Why not simply do whichever is correct of

buf[0] = rab >> 8;
buf[1] = rab;

and

buf[0] = rab;
buf[1] = rab >> 8;

and feed rab as a cpu-native value and get rid of the endianess crap.

> + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (status < 0) {
> + dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> + 

Re: [PATCH v8 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-06 21:52, 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
> 
>  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 | 400 
> 
>  5 files changed, 435 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 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,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..4816a31
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,400 @@
> +// 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

Re: [PATCH v7 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin]

On 2018-09-06 20:10, Ajay Gupta wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
> 
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> 
> Signed-off-by: Ajay Gupta 
> Reviewed-by: Andy Shevchenko 
> Acked-by: Heikki Krogerus 
> ---
> Changes from v1 -> v2
>   Fixed identation in drivers/usb/typec/ucsi/Kconfig
> Changes from v2 -> v3
>   Fixed most of comments from Heikki
>   Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> Changes from v3 -> v4
>   Fixed comments from Andy
> Changes from v4 -> v5
>   Fixed comments from Andy
> Changes from v5 -> v6
>   Fixed review comments from Greg 
> Changes from v6 -> v7
>   None
> 
>  drivers/usb/typec/ucsi/Kconfig|  10 +
>  drivers/usb/typec/ucsi/Makefile   |   2 +
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 382 
> ++
>  3 files changed, 394 insertions(+)
>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..7811888 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>  
>  if TYPEC_UCSI
>  
> +config UCSI_CCG
> + tristate "UCSI Interface Driver for Cypress CCGx"
> + depends on I2C
> + help
> +   This driver enables UCSI support on platforms that expose a
> +   Cypress CCGx Type-C controller over I2C interface.
> +
> +   To compile the driver as a module, choose M here: the module will be
> +   called ucsi_ccg.
> +
>  config UCSI_ACPI
>   tristate "UCSI ACPI Interface Driver"
>   depends on ACPI
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 7afbea5..2f4900b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -8,3 +8,5 @@ typec_ucsi-y  := ucsi.o
>  typec_ucsi-$(CONFIG_TRACING) += trace.o
>  
>  obj-$(CONFIG_UCSI_ACPI)  += ucsi_acpi.o
> +
> +obj-$(CONFIG_UCSI_CCG)   += ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c 
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> new file mode 100644
> index 000..65292bf
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta 
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include "ucsi.h"
> +
> +struct ucsi_ccg {
> + struct device *dev;
> + struct ucsi *ucsi;
> + struct ucsi_ppm ppm;
> + struct i2c_client *client;
> + int irq;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE 0x00
> +#define CCGX_I2C_RAB_READ_SILICON_ID 0x2
> +#define CCGX_I2C_RAB_INTR_REG0x06
> +#define CCGX_I2C_RAB_FW1_VERSION 0x28
> +#define CCGX_I2C_RAB_FW2_VERSION 0x20
> +#define CCGX_I2C_RAB_UCSI_CONTROL0x39
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START  BIT(0)
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP   BIT(1)
> +#define CCGX_I2C_RAB_RESPONSE_REG0x7E
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000
> +
> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> + struct i2c_client *client = uc->client;
> + unsigned char *buf;
> + struct i2c_msg *msgs;
> + u32 rlen, rem_len = len;
> + int status;
> +
> + buf = kzalloc(2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + msgs = kcalloc(2, sizeof(struct i2c_msg), GFP_KERNEL);
> + if (!msgs) {
> + kfree(buf);
> + return -ENOMEM;
> + }

The heap alloc of struct i2c_msg is ridiculous IMHO. The only things
that can possibly matter for DMA are the msgs[x].buf buffers.
And since you don't even set I2S_M_DMA_SAFE I really don't see the
point of any of the heap allocs introduced in v6. v5 was simply
much more pleasant.

> +
> + msgs[0].addr = client->addr;
> + msgs[0].len = 2;
> + msgs[0].buf = buf;
> + msgs[1].addr = client->addr;
> + msgs[1].flags = I2C_M_RD;
> +
> + while (rem_len > 0) {
> + msgs[1].buf = [len - rem_len];
> + rlen = min_t(u16, rem_len, 4);
> + msgs[1].len = rlen;
> + put_unaligned_le16(rab, buf);
> + status = i2c_transfer(client->adapter, msgs, 2);
> + if (status < 0) {
> + dev_err(uc->dev, "i2c_transfer failed %d", status);
> + kfree(buf);
> + kfree(msgs);
> + return status;
> + }
> +   

Re: [PATCH v6 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-06 19:39, Ajay Gupta wrote:
> 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 
> 
>>> +static void enable_i2c_bus(struct gpu_i2c_dev *i2cd)
>>
>> Please prefix all your functions with gpu_, or nvidia_gpu_ or something like
>> that (because gpu sounds like a something graphics, not that nvidia_gpu helps
>> with that problem though...)
> Ok, will prefix them.
>  
>>> +{
>>> +   u32 val;
>>> +
>>> +   /* enable I2C */
>>> +   val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
>>> +   val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
>>> +   I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
>>> +   I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
>>> +   writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
>>> +
>>> +   /* enable 100KHZ mode */
>>> +   val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
>>> +   val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
>>> +   << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
>>> +   val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
>>> +   writel(val, i2cd->regs + I2C_MST_I2C0_TIMING); }
>>> +
>>> +static int i2c_check_status(struct gpu_i2c_dev *i2cd) {
>>> +   unsigned long target = jiffies + msecs_to_jiffies(1000);
>>> +   u32 val;
>>> +
>>> +   do {
>>> +   val = readl(i2cd->regs + I2C_MST_CNTL);
>>> +   if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
>>> +   break;
>>> +   if ((val & I2C_MST_CNTL_STATUS) !=
>>> +   I2C_MST_CNTL_STATUS_BUS_BUSY)
>>> +   break;
>>> +   usleep_range(1000, 2000);
>>> +   } while (time_is_after_jiffies(target));
>>> +   if (time_is_before_jiffies(target))
>>> +   return -EIO;
>>> +
>>> +   val = readl(i2cd->regs + I2C_MST_CNTL);
>>> +   switch (val & I2C_MST_CNTL_STATUS) {
>>> +   case I2C_MST_CNTL_STATUS_OKAY:
>>> +   return 0;
>>> +   case I2C_MST_CNTL_STATUS_NO_ACK:
>>> +   return -EIO;
>>> +   case I2C_MST_CNTL_STATUS_TIMEOUT:
>>> +   return -ETIME;
>>> +   case I2C_MST_CNTL_STATUS_BUS_BUSY:
>>> +   return -EBUSY;
>>> +   default:
>>> +   return 0;
>>> +   }
>>> +}
>>> +
>>> +static int i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len) {
>>> +   int status;
>>> +   u32 val;
>>> +
>>> +   val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
>>> +   I2C_MST_CNTL_CMD_READ | (len <<
>> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
>>> +   I2C_MST_CNTL_CYCLE_TRIGGER |
>> I2C_MST_CNTL_GEN_NACK;
>>> +   val &= ~I2C_MST_CNTL_GEN_RAB;
>>> +   writel(val, i2cd->regs + I2C_MST_CNTL);
>>> +
>>> +   status = i2c_check_status(i2cd);
>>> +   if (status < 0)
>>> +   return status;
>>> +
>>> +   val = readl(i2cd->regs + I2C_MST_DATA);
>>> +   switch (len) {
>>> +   case 1:
>>> +   data[0] = val;
>>> +   break;
>>> +   case 2:
>>> +   put_unaligned_be16(val, data);
>>> +   break;
>>> +   case 3:
>>> +   put_unaligned_be16(val >> 8, data);
>>> +   data[2] = val;
>>> +   break;
>>> +   case 4:
>>> +   put_unaligned_be32(val, data);
>>> +   break;
>>> +   default:
>>
>> This seems to behave rather strange for len > 4, and I do not see anything
>> preventing that from happening.
> Actually the check is in ccg_read() of the 
> client driver at https://marc.info/?l=linux-usb=153618608301206=2 
> 
>> Am I missing something, or do you need to add a quirk for max_read_len?
> I will add a check in this function as well.

No, you should add a pointer to a struct i2c_adapter_quirks, with
max_read_len set I think. At least that's how e.g. i2c-qup.c does it.

>>> +   break;
>>> +   }
>>> +   return status;
>>> +}
>>> +
>>> +static int i2c_start(struct gpu_i2c_dev *i2cd, u16 addr) {
>>> +   u32 val;
>>> +
>>> +   val = addr << I2C_MST_ADDR_DAB;
>>> +   writel(val, i2cd->regs + I2C_MST_ADDR);
>>> +
>>> +   val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
>>> +   I2C_MST_CNTL_GEN_NACK;
>>> +   val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
>>> +   writel(val, i2cd->regs + I2C_MST_CNTL);
>>> +
>>> +   return i2c_check_status(i2cd);
>>> +}
>>> +
>>> +static int i2c_stop(struct gpu_i2c_dev *i2cd) {
>>> +   u32 val;
>>> +
>>> +   val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
>>> +   I2C_MST_CNTL_GEN_NACK;
>>> +   val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
>>> +   writel(val, i2cd->regs + I2C_MST_CNTL);
>>> +
>>> +   return i2c_check_status(i2cd);
>>> +}
>>> +
>>> +static int i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
>>> +   u32 val;
>>> +
>>> +   writel(data, i2cd->regs + I2C_MST_DATA);
>>> +
>>> +   val = I2C_MST_CNTL_CMD_WRITE | (1 <<
>> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
>>> +   I2C_MST_CNTL_GEN_NACK;
>>> +   val &= ~(I2C_MST_CNTL_GEN_START | 

Re: [PATCH v6 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-06 00:20, 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 
> ---
> 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 
> 
>  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 | 405 
> 
>  5 files changed, 440 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 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,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..2ce6706
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,405 @@
> +// 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 0x00
> +#define I2C_MST_CNTL_GEN_START   BIT(0)
> +#define I2C_MST_CNTL_GEN_STOPBIT(1)
> +#define I2C_MST_CNTL_CMD_NONE(0 << 2)
> +#define I2C_MST_CNTL_CMD_READ(1 << 2)
> +#define I2C

Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2018-04-18 07:48, yos...@codeaurora.org wrote:
> On 2018-04-17 17:11, Peter Rosin wrote:
>> On 2018-04-17 15:52, Yossi Mansharoff wrote:
>>> On the db410c 96boards platform we have a TC7USB40MU on the board
>>> to mux the D+/D- lines coming from the controller between a micro
>>> usb "device" port and a USB hub for "host" roles[1]. During a
>>> role switch, we need to toggle this mux to forward the D+/D-
>>> lines to either the port or the hub. Add the necessary code to do
>>> the role switch in chipidea core via the generic mux framework.
>>> Board configurations like on db410c are expected to change roles
>>> via the sysfs API described in
>>> Documentation/ABI/testing/sysfs-platform-chipidea-usb2.
>>
>> Ok, so this is v2. Please describe what is different from v1.
>> I have told you before that this information helps.
>>
>>> [1] 
>>> https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf
>>
>> This link returns 404 for me.
>>
>> Cheers,
>> Peter
> 
> 
> Hi,
> This patch was split apart from the original patch into two patches
> one for chipidea and the other for bindings.
> this patch has no other changes two the code.
> 
> I will update the link.

Just a note: I will not feed the mux_control_get_optional patch upstream
until I feel confident that this patch is going also heading upstream.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2018-04-20 04:00, Peter Chen wrote:
>  
>  
>> --- a/drivers/usb/chipidea/Kconfig
>> +++ b/drivers/usb/chipidea/Kconfig
>> @@ -3,6 +3,8 @@ config USB_CHIPIDEA
>>  depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD
>> && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
>>  select EXTCON
>>  select RESET_CONTROLLER
>> +select MULTIPLEXER
>> +select MUX_GPIO
> 
> The above two configurations are only used at your specific platforms, please 
> add
> them at either your platform defconfig or the related hardware driver's 
> Kconfig. 

"select MUX_GPIO" is indeed questionable and should be somewhere else because
this driver will work with any other mux as well. It's simply something else
that requires it. If it was the case that MUX_GPIO is indeed required then the
whole use of the mux subsystem is questionable and the thing controlled might
as well be controlled directly with the GPIO line. The mux subsystem is good
when a single mux "controller" is shared between several unrelated drivers
utilizing different muxes controlled by that same mux "controller" (think
several muxes controlled by the same GPIO line/lines). The mux subsystem is
also useful when the driver does not want to handle/know how the specific mux
is controlled. That said, it's of course not wrong to use the mux subsystem
in cases like this either, but I think it might be much easier and direct to
just twiddle the single GPIO line directly here?

Or do you expect some future HW variant that will use some other means to
control this mux?

>>  help
>>Say Y here if your system has a dual role high speed USB
>>controller based on ChipIdea silicon IP. It supports:
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
>> 33ae87f..8fa0991 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -61,6 +61,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "ci.h"
>>  #include "udc.h"
>> @@ -687,6 +688,10 @@ static int ci_get_platdata(struct device *dev,
>>  if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
>>  platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
>>
>> +platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
>> +if (IS_ERR(platdata->usb_switch))
>> +return PTR_ERR(platdata->usb_switch);
>> +
>>  ext_id = ERR_PTR(-ENODEV);
>>  ext_vbus = ERR_PTR(-ENODEV);
>>  if (of_property_read_bool(dev->of_node, "extcon")) { diff --git
>> a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 
>> af45aa32..d9d2d00
>> 100644
>> --- a/drivers/usb/chipidea/host.c
>> +++ b/drivers/usb/chipidea/host.c
>> @@ -13,6 +13,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "../host/ehci.h"
>>
>> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
>>  if (ci_otg_is_fsm_mode(ci)) {
>>  otg->host = >self;
>>  hcd->self.otg_port = 1;
>> +} else {
>> +ret = mux_control_select(ci->platdata->usb_switch, 1);
>> +if (ret)
>> +goto disable_reg;
> 
> What will happen if ci->platdata->usb_switch  is NULL?

What has not been mentioned in this patch is that it depends on another
patch which is not yet upstream. You can google for

mux: add mux_control_get_optional() API

to get an idea (it's also in linux-next). Anyway, with that patch this
is not a problem.

>>  }
>>  }
>>
>> @@ -181,6 +186,8 @@ static void host_stop(struct ci_hdrc *ci)
>>  struct usb_hcd *hcd = ci->hcd;
>>
>>  if (hcd) {
>> +if (!ci_otg_is_fsm_mode(ci))
>> +mux_control_deselect(ci->platdata->usb_switch);
> 
> Ditto.
> 
>>  if (ci->platdata->notify_event)
>>  ci->platdata->notify_event(ci,
>>  CI_HDRC_CONTROLLER_STOPPED_EVENT);
>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index
>> 9852ec5..209d3f6 100644
>> --- a/drivers/usb/chipidea/udc.c
>> +++ b/drivers/usb/chipidea/udc.c
>> @@ -19,6 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "ci.h"
>>  #include "udc.h"
>> @@ -1965,16 +1966,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>>
>>  static int udc_id_switch_for_device(struct ci_hdrc *ci)  {
>> +int ret = 0;
>> +
>>  if (ci->is_otg)
>>  /* Clear and enable BSV irq */
>>  hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
>>  OTGSC_BSVIS | OTGSC_BSVIE);
>>
>> -return 0;
>> +if (!ci_otg_is_fsm_mode(ci))
>> +ret = mux_control_select(ci->platdata->usb_switch, 0);
>> +
> 
> Ditto
> 
>> +if (ci->is_otg && ret)
>> +hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS,
>> OTGSC_BSVIS);
>> +
>> +return ret;
>>  }
>>
>>  static void 

Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2018-04-17 15:52, Yossi Mansharoff wrote:
> On the db410c 96boards platform we have a TC7USB40MU on the board
> to mux the D+/D- lines coming from the controller between a micro
> usb "device" port and a USB hub for "host" roles[1]. During a
> role switch, we need to toggle this mux to forward the D+/D-
> lines to either the port or the hub. Add the necessary code to do
> the role switch in chipidea core via the generic mux framework.
> Board configurations like on db410c are expected to change roles
> via the sysfs API described in
> Documentation/ABI/testing/sysfs-platform-chipidea-usb2.

Ok, so this is v2. Please describe what is different from v1.
I have told you before that this information helps.

> [1] 
> https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf

This link returns 404 for me.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support

[Peter Rosin]

On 2017-09-25 13:35, Hans de Goede wrote:
> Hi,
> 
> On 25-09-17 12:34, Peter Rosin wrote:
>> On 2017-09-13 17:48, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 13-09-17 17:07, Rob Herring wrote:
>>>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdego...@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 13-09-17 15:38, Rob Herring wrote:
>>>>>>
>>>>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdego...@redhat.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> On 13-09-17 00:20, Rob Herring wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create()
>>>>>>>>> to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us.
>>>>>>>>>
>>>>>>>>> Also document the mux-names used by the generic tcpc_mux_dev code in
>>>>>>>>> our devicetree bindings.
>>>>>>>>>
>>>>>>>>> Cc: Rob Herring <robh...@kernel.org>
>>>>>>>>> Cc: Mark Rutland <mark.rutl...@arm.com>
>>>>>>>>> Cc: devicet...@vger.kernel.org
>>>>>>>>> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>>>>>>>>> ---
>>>>>>>>>  Documentation/devicetree/bindings/usb/fcs,fusb302.txt |  3 +++
>>>>>>>>>  drivers/staging/typec/fusb302/fusb302.c   | 11
>>>>>>>>> ++-
>>>>>>>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>>>>>>>> b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>>>>>>>> index 472facfa5a71..63d639eadacd 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>>>>>>>> @@ -6,6 +6,9 @@ Required properties :
>>>>>>>>>  - interrupts : Interrupt specifier
>>>>>>>>>Optional properties :
>>>>>>>>> +- mux-controls   : List of mux-ctrl-specifiers containing 1 
>>>>>>>>> or
>>>>>>>>> 2
>>>>>>>>> muxes
>>>>>>>>> +- mux-names  : "type-c-mode-mux" when using 1 mux, or
>>>>>>>>> +   "type-c-mode-mux", "usb-role-mux" when
>>>>>>>>> using
>>>>>>>>> 2 muxes
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I'm not sure this is the right place for this. The mux is outside the
>>>>>>>> FUSB302. In a type-C connector node or USB phy node would make more
>>>>>>>> sense to me.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The mux certainly does not belong in the USB phy node, it sits between
>>>>>>> the
>>>>>>> USB PHY
>>>>>>> and the Type-C connector and can for example also mux the Type-C pins to
>>>>>>> a
>>>>>>> Display
>>>>>>> Port PHY.
>>>>>>
>>>>>>
>>>>>> Thinking about this some more, the mux(es) should be its own node(s).
>>>>>> Then the question is where to put the nodes.
>>>>>
>>>>>
>>>>> Right, the mux will be its own node per
>>>>> Documentation/devicetree/bindings/mux/mux-controller.txt
>>>>> the bindings bit this patch is adding is only adding phandles pointing
>>>>> to that mux-node as the fusb302 "consumes" the mux functionality.
>>>>>
>>>>> So as such (the fusb302 is the component which should control

Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support

[Peter Rosin]

On 2017-09-13 17:48, Hans de Goede wrote:
> Hi,
> 
> On 13-09-17 17:07, Rob Herring wrote:
>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede  wrote:
>>> Hi,
>>>
>>>
>>> On 13-09-17 15:38, Rob Herring wrote:

 On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede 
 wrote:
>
> Hi,
>
>
> On 13-09-17 00:20, Rob Herring wrote:
>>
>>
>> On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote:
>>>
>>>
>>> Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create()
>>> to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us.
>>>
>>> Also document the mux-names used by the generic tcpc_mux_dev code in
>>> our devicetree bindings.
>>>
>>> Cc: Rob Herring 
>>> Cc: Mark Rutland 
>>> Cc: devicet...@vger.kernel.org
>>> Signed-off-by: Hans de Goede 
>>> ---
>>> Documentation/devicetree/bindings/usb/fcs,fusb302.txt |  3 +++
>>> drivers/staging/typec/fusb302/fusb302.c   | 11
>>> ++-
>>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>> b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>> index 472facfa5a71..63d639eadacd 100644
>>> --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>> +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>> @@ -6,6 +6,9 @@ Required properties :
>>> - interrupts : Interrupt specifier
>>>   Optional properties :
>>> +- mux-controls   : List of mux-ctrl-specifiers containing 1 or
>>> 2
>>> muxes
>>> +- mux-names  : "type-c-mode-mux" when using 1 mux, or
>>> +   "type-c-mode-mux", "usb-role-mux" when
>>> using
>>> 2 muxes
>>
>>
>>
>> I'm not sure this is the right place for this. The mux is outside the
>> FUSB302. In a type-C connector node or USB phy node would make more
>> sense to me.
>
>
>
> The mux certainly does not belong in the USB phy node, it sits between
> the
> USB PHY
> and the Type-C connector and can for example also mux the Type-C pins to
> a
> Display
> Port PHY.


 Thinking about this some more, the mux(es) should be its own node(s).
 Then the question is where to put the nodes.
>>>
>>>
>>> Right, the mux will be its own node per
>>> Documentation/devicetree/bindings/mux/mux-controller.txt
>>> the bindings bit this patch is adding is only adding phandles pointing
>>> to that mux-node as the fusb302 "consumes" the mux functionality.
>>>
>>> So as such (the fusb302 is the component which should control the mux)
>>> I do believe that the bindings this patch adds are correct.
>>
>> Humm, that's not how the mux binding works. The mux controller is what
>> drives the mux select lines and is the provider. The consumer is the
>> mux device itself. What decides the mux state is determined by what
>> you are muxing, not which node has mux-controls property.
>>
>> By putting mux-controls in fusb302 node, you are saying fusb302 is a
>> mux (or contains a mux).
>>
>>
> As for putting it in a type-C connector node, currently we do not have
> such
> a node,


 Well, you should. Type-C connectors are certainly complicated enough
 that we'll need one. Plus we already require connector nodes for
 display outputs, so what do we do once you add display muxing?
>>>
>>>
>>> An interesting question, I'm working on this for x86 + ACPI boards actually,
>>> not a board using DT I've been adding DT bindings docs for device-properties
>>> I use because that seems like the right thing to do where the binding is
>>> obvious
>>> (which I believe it is in this case as the fusb302 is the mux consumer) and
>>> because the device-property code should work the same on x86 + ACPI
>>> (where some platform-specific drivers attach the device properties) and
>>> on e.g. ARM + DT.
>>>
>>> The rest should probably be left to be figured out when an actual DT
>>> using device using the fusb302 or another Type-C controller shows up.
>>
>> Well this is a new one (maybe, I suppose others have sneaked by). If
>> ACPI folks want to use DT bindings, then what do I care. But I have no
>> interest in reviewing ACPI properties. The whole notion of sharing
>> bindings between DT and ACPI beyond anything trivial is flawed IMO.
>> The ptifalls have been discussed multiple times before, so I'm not
>> going to repeat them here.
> 
> Ok, so shall I just drop the 
> Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> part of this patch then ?

I totally agree with the concern that Rob expressed about handling USB C
muxes in a non-adhoc way. And this makes this series scary. I don't know
enough details about USB C muxes and PD (I 

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Peter Rosin]

On 2017-09-08 19:07, Hans de Goede wrote:
> Hi,
> 
> On 08-09-17 17:47, Peter Rosin wrote:
>> On 2017-09-05 18:42, Hans de Goede wrote:
>>> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
>>> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
>>> consumers to ensure that they agree on the meaning of the
>>> mux_control_select() state argument.
>>>
>>> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>>> ---

*snip*

>>> +/*
>>> + * Mux state values for Type-C polarity/role/altmode muxes.
>>> + *
>>> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
>>> + * inverted-polarity (Type-C plugged in upside down) can happen with any
>>> + * other mux-state.
>>> + */
>>> +#define MUX_TYPEC_POLARITY_INV BIT(0)   /* Polarity inverted 
>>> bit */
>>> +#define MUX_TYPEC_DEVICE   (0 << 1) /* USB device mode */
>>> +#define MUX_TYPEC_HOST (1 << 1) /* USB host mode */
>>> +#define MUX_TYPEC_HOST_AND_DP_SRC  (2 << 1) /* USB host + 2 lanes DP src */
>>> +#define MUX_TYPEC_DP_SRC   (3 << 1) /* 4 lanes Display Port src */
>>> +#define MUX_TYPEC_STATES   (4 << 1)
>>
>> But USB Type-C muxes need not support just these states If I read it right?
>> USB Type-C seems to be usable for a variety of protocols and the above list
>> seems pretty much like a special case for this mux (and perhaps a set of
>> other similar muxes). But when someone with a USB Type-C mux for different
>> protocols shows up, that person will probably be frustrated by these
>> defines, no? Or is there something I don't see that limits USB-C to DP?
> 
> In general almost all hardware is limited to the above (+ analog audio over
> the 2 Sideband use pins, but I expect that to have a separate mux).
> 
> You're right, theoretically there might be other cases, e.g. there is a spec
> for HDMI over Type-C (wishful thinking from the HDMI group, no one uses this),
> but:
> 
> 1) I expect most muxes to implement the above set, that is what all
> hardware out there supports (well that or less).
> 
> 2) We can always add extra defines here, that means that a Type-C mux may
> not implement all states and return -EINVAL when asked for something it
> does not implement, which I understand is a bit weird from a mux subsys
> pov. But that can be the case anyways because even though the mux supports
> these options, the board it is used on does no necessarily have to support
> these options, e.g. there may be only 2 lanes of DP hooked up to the mux
> (or no DP at all, but then I would them to expect a different mux).
> 
> So the Type-C Port Manager already needs to be passed some platform
> data describing which features the board has and keep that in mind
> when negotiation with the dongle attached to the Type-C port, so if
> we do get boards which do HDMI and no DP, then the TCPM would simply
> never use the MUX_TYPEC_HOST_AND_DP_SRC and MUX_TYPEC_DP_SRC states.

Ok, I googled "usb type c mux" and came up with HD3SS460 from Texas as
the first hit.

http://www.ti.com/lit/ds/symlink/hd3ss460.pdf

That one has three control pins, but two of them (AMSEL and EN) are
tri-state. So 18 states in theory. However, if EN is low everything is
HighZ, so that collapses 6 states into 1, and 2 other states are reserved.
Still 11 states, which is two more than what you have implemented for
PI3USB30532. If we ignore polarity switching, it's only a one state diff.
However, when I try to make sense of the states for the HD3SS460, I don't
see anything that selects USB device or host. And I don't really see why
a Type C mux has to know that; in my head the mux should just route the
signals. And then when I look in your PI3USB30532 driver I don't seen any
such difference either. Along the same lines, the Type C mux does not
know/care if DP is source or sink. Or?

How about:

#define MUX_TYPEC_POLARITY_INV  BIT(0)   /* Polarity inverted bit */
#define MUX_TYPEC_USB   (0 << 1) /* USB only mode */
#define MUX_TYPEC_USB_AND_DP(1 << 1) /* USB host + 2 lanes DP */
#define MUX_TYPEC_DP(2 << 1) /* 4 lanes Display Port */
#define MUX_TYPEC_STATES(3 << 1)

I'm not sure what 2 states the HS3SS460 have in addition to the above, but
the way I read the spec those to are variations on the MUX_TYPEC_USB_AND_DP
state, but routing the DP signals to alternate pins. Which suggests that
more documentation is needed to describe exactly what is meant when someone
selects MUX_TYPEC_USB_AND_DP?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mux: add mux_control_get_optional() API

[Peter Rosin]

On 2017-09-08 17:45, Peter Rosin wrote:
> From: Stephen Boyd <stephen.b...@linaro.org>
> 
> Sometimes drivers only use muxes under certain scenarios. For
> example, the chipidea usb controller may be connected to a usb
> switch on some platforms, and connected directly to a usb port on
> others. The driver won't know one way or the other though, so add
> a mux_control_get_optional() API that allows the driver to
> differentiate errors getting the mux from there not being a mux
> for the driver to use at all.
> ---
>  Documentation/driver-model/devres.txt |   1 +
>  drivers/mux/core.c| 104 
> +++---
>  include/linux/mux/consumer.h  |   4 ++
>  3 files changed, 89 insertions(+), 20 deletions(-)
> 
> I haven't tested this patch, and hence I have not signed it and I also
> removed the sign-off from Stephen...

Huh, I definitely intended to mention that this patch is based on [1]
from Stephen, but that I've made changes according to the comments in
that thread (and more). And those changes are what made me remove the
sign-off from Stephen...

Sorry for the noise,
Peter

[1] https://lkml.org/lkml/2017/7/14/655
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
> consumers to ensure that they agree on the meaning of the
> mux_control_select() state argument.
> 
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Start numbering of defines at 0 not 1
> -Use a new usb.h header, rather then adding these to consumer.h
> -Add separate MUX_USB_* and MUX_TYPEC_* defines
> ---
>  include/linux/mux/usb.h | 32 
>  1 file changed, 32 insertions(+)
>  create mode 100644 include/linux/mux/usb.h
> 
> diff --git a/include/linux/mux/usb.h b/include/linux/mux/usb.h
> new file mode 100644
> index ..44df5eca5256
> --- /dev/null
> +++ b/include/linux/mux/usb.h
> @@ -0,0 +1,32 @@
> +/*
> + * mux/usb.h - definitions for USB multiplexers
> + *
> + * Copyright (C) 2017 Hans de Goede 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_MUX_USB_H
> +#define _LINUX_MUX_USB_H
> +
> +/* Mux state values for USB device/host role muxes */
> +#define MUX_USB_DEVICE   (0) /* USB device mode */
> +#define MUX_USB_HOST (1) /* USB host mode */
> +#define MUX_USB_STATES   (2)
> +
> +/*
> + * Mux state values for Type-C polarity/role/altmode muxes.
> + *
> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
> + * inverted-polarity (Type-C plugged in upside down) can happen with any
> + * other mux-state.
> + */
> +#define MUX_TYPEC_POLARITY_INV   BIT(0)   /* Polarity inverted 
> bit */
> +#define MUX_TYPEC_DEVICE (0 << 1) /* USB device mode */
> +#define MUX_TYPEC_HOST   (1 << 1) /* USB host mode */
> +#define MUX_TYPEC_HOST_AND_DP_SRC(2 << 1) /* USB host + 2 lanes DP src */
> +#define MUX_TYPEC_DP_SRC (3 << 1) /* 4 lanes Display Port src */
> +#define MUX_TYPEC_STATES (4 << 1)

But USB Type-C muxes need not support just these states If I read it right?
USB Type-C seems to be usable for a variety of protocols and the above list
seems pretty much like a special case for this mux (and perhaps a set of
other similar muxes). But when someone with a USB Type-C mux for different
protocols shows up, that person will probably be frustrated by these
defines, no? Or is there something I don't see that limits USB-C to DP?

Cheers,
Peter

> +
> +#endif /* _LINUX_MUX_TYPEC_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] mux: add explicit hook to leave the mux as-is on init/registration

[Peter Rosin]

A board may need a mux controller to stay as-is for a while longer, e.g.
if setting the normally preferred idle state destroys booting.

The mechanism provided here is not perfect in two ways.
1. As soon as the mux controller is registered, some mux consumer can
   access it and set a state that destroys booting all the same.
2. The mux controller might linger in a state that is not the
   preferred idle state indefinitely, if no mux consumer ever selects
   and then deselects the mux.
---
 drivers/mux/core.c | 3 +++
 include/linux/mux/driver.h | 4 
 2 files changed, 7 insertions(+)

Untested -> no sign-off, and I'm also really unsure about it because of
the imperfections mentioned in the commit message.

Cheers,
peda

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 2eb234300669..c81db7d4f9c8 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -155,6 +155,9 @@ int mux_chip_register(struct mux_chip *mux_chip)
for (i = 0; i < mux_chip->controllers; ++i) {
struct mux_control *mux = _chip->mux[i];
 
+   if (mux->init_as_is)
+   continue;
+
if (mux->idle_state == mux->cached_state)
continue;
 
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 35c3579c3304..21cf6041a962 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -36,6 +36,9 @@ struct mux_control_ops {
  * @states:The number of mux controller states.
  * @idle_state:The mux controller state to use when inactive, 
or one
  * of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
+ * @init_as_is:Set to true to have the core leave the mux 
controller
+ * state as-is until first selection. If @idle_state is
+ * MUX_IDLE_AS_IS, @init_as_is is irrelevant.
  *
  * Mux drivers may only change @states and @idle_state, and may only do so
  * between allocation and registration of the mux controller. Specifically,
@@ -50,6 +53,7 @@ struct mux_control {
 
unsigned int states;
int idle_state;
+   bool init_as_is;
 };
 
 /**
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> The Intel cherrytrail xhci controller has an extended cap mmio-range
> which contains registers to control the muxing to the xhci (host mode)
> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
> 
> Having a mux driver included in the xhci code (or under drivers/usb/host)
> is not desirable. So this commit adds a simple handler for this extended
> capability, which creates a platform device with the caps mmio region as
> resource, this allows us to write a separate platform mux driver for the
> mux.
> 
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Check xHCI controller PCI device-id instead of only checking for the
>  Intel Extended capability ID, as the Extended capability ID is used on
>  other model Intel xHCI controllers too
> ---
>  drivers/usb/host/Makefile|  2 +-
>  drivers/usb/host/xhci-intel-quirks.c | 85 
> 
>  drivers/usb/host/xhci-pci.c  |  4 ++
>  drivers/usb/host/xhci.h  |  2 +
>  4 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/host/xhci-intel-quirks.c
> 
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index cf2691fffcc0..441edf82eb1c 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)  += ohci-da8xx.o
>  obj-$(CONFIG_USB_UHCI_HCD)   += uhci-hcd.o
>  obj-$(CONFIG_USB_FHCI_HCD)   += fhci.o
>  obj-$(CONFIG_USB_XHCI_HCD)   += xhci-hcd.o
> -obj-$(CONFIG_USB_XHCI_PCI)   += xhci-pci.o
> +obj-$(CONFIG_USB_XHCI_PCI)   += xhci-pci.o xhci-intel-quirks.o
>  obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>  obj-$(CONFIG_USB_XHCI_MTK)   += xhci-mtk.o
>  obj-$(CONFIG_USB_XHCI_TEGRA) += xhci-tegra.o
> diff --git a/drivers/usb/host/xhci-intel-quirks.c 
> b/drivers/usb/host/xhci-intel-quirks.c
> new file mode 100644
> index ..819f5f9da9ee
> --- /dev/null
> +++ b/drivers/usb/host/xhci-intel-quirks.c
> @@ -0,0 +1,85 @@
> +/*
> + * Intel Vendor Defined XHCI extended capability handling
> + *
> + * Copyright (c) 2016) Hans de Goede 

2017? And drop the stray bracket.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/11] mux: Add Intel Cherrytrail USB mux driver

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
> USB data lines between the xHCI host controller and the dwc3 gadget
> controller. On some Cherrytrail systems this mux is controlled through
> AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
> an _AIE ACPI method) so things just work.
> 
> But on other Cherrytrail systems we need to control the mux ourselves
> this driver exports the mux through the mux subsys, so that other drivers
> can control it if necessary.
> 
> This driver also updates the vbus-valid reporting to the dwc3 gadget
> controller, as this uses the same registers as the mux. This is needed
> for gadget/device mode to work properly (even on systems which control
> the mux from their AML code).
> 
> Note this depends on the xhci driver registering a platform device
> named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
> to the Intel Vendor Defined XHCI extended capabilities region.
> 
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Complete rewrite as a stand-alone platform-driver rather then as a phy
>  driver, since this is just a mux, not a phy
> 
> Changes in v3:
> -Make this a mux subsys driver instead of listening to USB_HOST extcon
>  cable events and responding to those
> 
> Changes in v4 (of patch, v2 of new mux based series):
> -Rename C-file to use - in name
> -Add MAINTAINERS entry
> -Various code-style fixes
> ---
>  MAINTAINERS |   5 +
>  drivers/mux/Kconfig |  11 ++
>  drivers/mux/Makefile|  10 +-
>  drivers/mux/intel-cht-usb-mux.c | 257 
> 
>  4 files changed, 279 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/mux/intel-cht-usb-mux.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 14a2fd905217..dfaed958db85 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8991,6 +8991,11 @@ F: include/linux/dt-bindings/mux/
>  F:   include/linux/mux/
>  F:   drivers/mux/
>  
> +MULTIPLEXER SUBSYSTEM INTEL CHT USB MUX DRIVER
> +M:   Hans de Goede 
> +S:   Maintained
> +F:   drivers/mix/intel-cht-usb-mux.c

s/mix/mux/

(also in patch 06/11)

> +
>  MULTISOUND SOUND DRIVER
>  M:   Andrew Veliath 
>  S:   Maintained
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 19e4e904c9bf..947cfd7af02a 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -34,6 +34,17 @@ config MUX_GPIO
> To compile the driver as a module, choose M here: the module will
> be called mux-gpio.
>  
> +config MUX_INTEL_CHT_USB_MUX
> + tristate "Intel Cherrytrail USB Multiplexer"
> + depends on ACPI && X86 && EXTCON
> + help
> +   This driver adds support for the internal USB mux for muxing the OTG
> +   USB data lines between the xHCI host controller and the dwc3 gadget
> +   controller found on Intel Cherrytrail SoCs.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-intel_cht_usb_mux.

s/_/-/g

> +
>  config MUX_MMIO
>   tristate "MMIO register bitfield-controlled Multiplexer"
>   depends on (OF && MFD_SYSCON) || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 0e1e59760e3f..6cf41be2754f 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -5,9 +5,11 @@
>  mux-core-objs:= core.o
>  mux-adg792a-objs := adg792a.o
>  mux-gpio-objs:= gpio.o
> +mux-intel-cht-usb-mux-objs   := intel-cht-usb-mux.o
>  mux-mmio-objs:= mmio.o
>  
> -obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
> -obj-$(CONFIG_MUX_ADG792A)+= mux-adg792a.o
> -obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
> -obj-$(CONFIG_MUX_MMIO)   += mux-mmio.o
> +obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
> +obj-$(CONFIG_MUX_ADG792A)+= mux-adg792a.o
> +obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
> +obj-$(CONFIG_MUX_INTEL_CHT_USB_MUX)  += mux-intel-cht-usb-mux.o
> +obj-$(CONFIG_MUX_MMIO)   += mux-mmio.o
> diff --git a/drivers/mux/intel-cht-usb-mux.c b/drivers/mux/intel-cht-usb-mux.c
> new file mode 100644
> index ..9cd1a1f5027f
> --- /dev/null
> +++ b/drivers/mux/intel-cht-usb-mux.c
> @@ -0,0 +1,257 @@
> +/*
> + * Intel Cherrytrail USB OTG MUX driver
> + *
> + * Copyright (c) 2016 Hans de Goede 

2017?

> + *
> + * Loosely based on android x86 kernel code which is:
> + *
> + * Copyright (C) 2014 Intel Corp.
> + *
> + * Author: Wu, Hao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation, or (at your option)
> + * any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

[PATCH 1/2] mux: add mux_control_get_optional() API

[Peter Rosin]

From: Stephen Boyd 

Sometimes drivers only use muxes under certain scenarios. For
example, the chipidea usb controller may be connected to a usb
switch on some platforms, and connected directly to a usb port on
others. The driver won't know one way or the other though, so add
a mux_control_get_optional() API that allows the driver to
differentiate errors getting the mux from there not being a mux
for the driver to use at all.
---
 Documentation/driver-model/devres.txt |   1 +
 drivers/mux/core.c| 104 +++---
 include/linux/mux/consumer.h  |   4 ++
 3 files changed, 89 insertions(+), 20 deletions(-)

I haven't tested this patch, and hence I have not signed it and I also
removed the sign-off from Stephen...

Cheers,
peda

diff --git a/Documentation/driver-model/devres.txt 
b/Documentation/driver-model/devres.txt
index 30e04f7a690d..4fdd3e63ff8b 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -342,6 +342,7 @@ MUX
   devm_mux_chip_alloc()
   devm_mux_chip_register()
   devm_mux_control_get()
+  devm_mux_control_get_optional()
 
 PER-CPU MEM
   devm_alloc_percpu()
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 2260063b0ea8..2eb234300669 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
  */
 unsigned int mux_control_states(struct mux_control *mux)
 {
+   if (!mux)
+   return 0;
+
return mux->states;
 }
 EXPORT_SYMBOL_GPL(mux_control_states);
@@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned 
int state)
 {
int ret;
 
+   if (!mux)
+   return 0;
+
ret = down_killable(>lock);
if (ret < 0)
return ret;
@@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
unsigned int state)
 {
int ret;
 
+   if (!mux)
+   return 0;
+
if (down_trylock(>lock))
return -EBUSY;
 
@@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
 {
int ret = 0;
 
+   if (!mux)
+   return 0;
+
if (mux->idle_state != MUX_IDLE_AS_IS &&
mux->idle_state != mux->cached_state)
ret = mux_control_set(mux, mux->idle_state);
@@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct 
device_node *np)
return dev ? to_mux_chip(dev) : NULL;
 }
 
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *
+__mux_control_get(struct device *dev, const char *mux_name, bool optional)
 {
struct device_node *np = dev->of_node;
struct of_phandle_args args;
@@ -441,16 +447,22 @@ struct mux_control *mux_control_get(struct device *dev, 
const char *mux_name)
if (mux_name) {
index = of_property_match_string(np, "mux-control-names",
 mux_name);
+   if ((index == -EINVAL || index == -ENODATA) && optional)
+   return NULL;
if (index < 0) {
dev_err(dev, "mux controller '%s' not found\n",
mux_name);
return ERR_PTR(index);
}
+   /* OF does point to a mux, so it's no longer optional */
+   optional = false;
}
 
ret = of_parse_phandle_with_args(np,
 "mux-controls", "#mux-control-cells",
 index, );
+   if (ret == -ENOENT && optional)
+   return NULL;
if (ret) {
dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
np, mux_name ?: "", index);
@@ -482,9 +494,36 @@ struct mux_control *mux_control_get(struct device *dev, 
const char *mux_name)
get_device(_chip->dev);
return _chip->mux[controller];
 }
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+   return __mux_control_get(dev, mux_name, false);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
 /**
+ * mux_control_get_optional() - Get the optional mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: NULL if no mux with the provided name is found, a pointer to
+ * the 

Re: [PATCH 06/11] mux: Add Pericom PI3USB30532 Type-C mux driver

[Peter Rosin]

On 2017-09-04 13:19, Peter Rosin wrote:
> Hi!
> 
> One comment inline...

Oh, and one more small nit, I think you should do

s/pi3usb30532_mux/pi3usb30532/g

to shorten the identifiers a bit. The _mux suffix (or infix) is kind of
selfevident from where the file lives anyway. pi3usb30532_mux_set_mux in
particular feels a bit much

Cheers,
peda

> On 2017-09-01 23:48, Hans de Goede wrote:
>> Add a driver for the Pericom PI3USB30532 Type-C cross switch /
>> mux chip found on some devices with a Type-C port.
>>
>> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>> ---
>>  drivers/mux/Kconfig   | 10 +
>>  drivers/mux/Makefile  |  2 +
>>  drivers/mux/pi3usb30532.c | 97 
>> +++
>>  3 files changed, 109 insertions(+)
>>  create mode 100644 drivers/mux/pi3usb30532.c
>>
>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>> index 17938918bf93..19a3065c34e6 100644
>> --- a/drivers/mux/Kconfig
>> +++ b/drivers/mux/Kconfig
>> @@ -58,4 +58,14 @@ config MUX_MMIO
>>To compile the driver as a module, choose M here: the module will
>>be called mux-mmio.
>>  
>> +config MUX_PI3USB30532
>> +tristate "Pericom PI3USB30532 Type-C cross switch driver"
>> +depends on I2C
>> +help
>> +  This driver adds support for the Pericom PI3USB30532 Type-C cross
>> +  switch / mux chip found on some devices with a Type-C port.
>> +
>> +  To compile the driver as a module, choose M here: the module will
>> +  be called mux-pi3usb30532.
>> +
>>  endmenu
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> index a12e812c7966..7563dbf04593 100644
>> --- a/drivers/mux/Makefile
>> +++ b/drivers/mux/Makefile
>> @@ -7,9 +7,11 @@ mux-adg792a-objs:= adg792a.o
>>  mux-gpio-objs   := gpio.o
>>  mux-mmio-objs   := mmio.o
>>  mux-intel_cht_usb_mux-objs  := intel_cht_usb_mux.o
>> +mux-pi3usb30532-objs:= pi3usb30532.o
>>  
>>  obj-$(CONFIG_MULTIPLEXER)   += mux-core.o
>>  obj-$(CONFIG_MUX_ADG792A)   += mux-adg792a.o
>>  obj-$(CONFIG_MUX_GPIO)  += mux-gpio.o
>>  obj-$(CONFIG_MUX_MMIO)  += mux-mmio.o
>>  obj-$(CONFIG_MUX_CHT_USB_MUX)   += mux-intel_cht_usb_mux.o
>> +obj-$(CONFIG_MUX_PI3USB30532)   += mux-pi3usb30532.o
>> diff --git a/drivers/mux/pi3usb30532.c b/drivers/mux/pi3usb30532.c
>> new file mode 100644
>> index ..fa8abd851520
>> --- /dev/null
>> +++ b/drivers/mux/pi3usb30532.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * Pericom PI3USB30532 Type-C cross switch / mux driver
>> + *
>> + * Copyright (c) 2017 Hans de Goede <hdego...@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include  /* For the MUX_USB_* defines */
>> +#include 
>> +
>> +#define PI3USB30532_CONF0x00
>> +
>> +#define PI3USB30532_CONF_OPEN   0x00
>> +#define PI3USB30532_CONF_SWAP   0x01
>> +#define PI3USB30532_CONF_4LANE_DP   0x02
>> +#define PI3USB30532_CONF_USB3   0x04
>> +#define PI3USB30532_CONF_USB3_AND_2LANE_DP  0x06
>> +
>> +struct pi3usb30532_mux {
>> +struct i2c_client *client;
>> +};
>> +
>> +static int pi3usb30532_mux_set_mux(struct mux_control *mux_ctrl, int state)
>> +{
>> +struct pi3usb30532_mux *mux = mux_chip_priv(mux_ctrl->chip);
> 
> The "mux" variable name is used for the mux_control in other drivers, and
> I don't think the private data is needed. Like so:
> 
> static int pi3usb30532_mux_set_mux(struct mux_control *mux, int state)
> {
>   struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
> 
> ...
> 
> Cheers,
> Peter
> 
>> +u8 conf = PI3USB30532_CONF_OPEN;
>> +
>> +switch (state & ~MUX_USB_POLARITY_INV) {
>> +case MUX_USB_NONE:
>> +conf = PI3USB30532_CONF_OPEN;
>> +break;
>> +case MUX_USB_DEVICE:
>> +case MUX_USB_HOST:
>> +conf = PI3USB30532_CONF_USB3;
>> +break;
>> +case MUX_USB_HOST_AND_DP_SRC:
>> +con

Re: [PATCH 05/11] mux: Add Intel Cherrytrail USB mux driver

[Peter Rosin]

Hi!

Some comments inline...

On 2017-09-01 23:48, Hans de Goede wrote:
> Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
> USB data lines between the xHCI host controller and the dwc3 gadget
> controller. On some Cherrytrail systems this mux is controlled through
> AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
> an _AIE ACPI method) so things just work.
> 
> But on other Cherrytrail systems we need to control the mux ourselves
> this driver exports the mux through the mux subsys, so that other drivers
> can control it if necessary.
> 
> This driver also updates the vbus-valid reporting to the dwc3 gadget
> controller, as this uses the same registers as the mux. This is needed
> for gadget/device mode to work properly (even on systems which control
> the mux from their AML code).
> 
> Note this depends on the xhci driver registering a platform device
> named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
> to the Intel Vendor Defined XHCI extended capabilities region.
> 
> Signed-off-by: Hans de Goede 
> ---
> Changes in v2:
> -Complete rewrite as a stand-alone platform-driver rather then as a phy
>  driver, since this is just a mux, not a phy
> 
> Changes in v3:
> -Make this a mux subsys driver instead of listening to USB_HOST extcon
>  cable events and responding to those
> ---
>  drivers/mux/Kconfig |  11 ++
>  drivers/mux/Makefile|   2 +
>  drivers/mux/intel_cht_usb_mux.c | 269 
> 
>  3 files changed, 282 insertions(+)
>  create mode 100644 drivers/mux/intel_cht_usb_mux.c
> 
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 19e4e904c9bf..17938918bf93 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -34,6 +34,17 @@ config MUX_GPIO
> To compile the driver as a module, choose M here: the module will
> be called mux-gpio.
>  
> +config MUX_CHT_USB_MUX
> + tristate "Intel Cherrytrail USB Multiplexer"
> + depends on ACPI && X86 && EXTCON
> + help
> +   This driver adds support for the internal USB mux for muxing the OTG
> +   USB data lines between the xHCI host controller and the dwc3 gadget
> +   controller found on Intel Cherrytrail SoCs.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-intel_cht_usb_mux.
> +
>  config MUX_MMIO
>   tristate "MMIO register bitfield-controlled Multiplexer"
>   depends on (OF && MFD_SYSCON) || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 0e1e59760e3f..a12e812c7966 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -6,8 +6,10 @@ mux-core-objs:= core.o
>  mux-adg792a-objs := adg792a.o
>  mux-gpio-objs:= gpio.o
>  mux-mmio-objs:= mmio.o
> +mux-intel_cht_usb_mux-objs   := intel_cht_usb_mux.o

I dislike underscores in file names (and other names), please use
dashes where possible. Also, please keep the list sorted.

>  
>  obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)+= mux-adg792a.o
>  obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
>  obj-$(CONFIG_MUX_MMIO)   += mux-mmio.o
> +obj-$(CONFIG_MUX_CHT_USB_MUX)+= mux-intel_cht_usb_mux.o

Dito.

> diff --git a/drivers/mux/intel_cht_usb_mux.c b/drivers/mux/intel_cht_usb_mux.c
> new file mode 100644
> index ..7b1621a081d8
> --- /dev/null
> +++ b/drivers/mux/intel_cht_usb_mux.c
> @@ -0,0 +1,269 @@
> +/*
> + * Intel Cherrytrail USB OTG MUX driver
> + *
> + * Copyright (c) 2016 Hans de Goede 
> + *
> + * Loosely based on android x86 kernel code which is:
> + *
> + * Copyright (C) 2014 Intel Corp.
> + *
> + * Author: Wu, Hao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation, or (at your option)
> + * any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include  /* For the MUX_USB_* defines */
> +#include 
> +#include 
> +#include 
> +
> +/* register definition */
> +#define DUAL_ROLE_CFG0   0x68
> +#define SW_VBUS_VALID(1 << 24)
> +#define SW_IDPIN_EN  (1 << 21)
> +#define SW_IDPIN (1 << 20)
> +
> +#define DUAL_ROLE_CFG1   0x6c
> +#define HOST_MODE(1 << 29)
> +
> +#define DUAL_ROLE_CFG1_POLL_TIMEOUT  1000
> +
> +#define DRV_NAME "intel_cht_usb_mux"

Dashes, please, if possible. Or are there perhaps a lot of precedent
for other cherrytrail driver names? Because consistency across
the tree is a tad more important than my issues with underscores...

> +
> +struct intel_cht_usb_mux {

Re: [PATCH 06/11] mux: Add Pericom PI3USB30532 Type-C mux driver

[Peter Rosin]

Hi!

One comment inline...

On 2017-09-01 23:48, Hans de Goede wrote:
> Add a driver for the Pericom PI3USB30532 Type-C cross switch /
> mux chip found on some devices with a Type-C port.
> 
> Signed-off-by: Hans de Goede 
> ---
>  drivers/mux/Kconfig   | 10 +
>  drivers/mux/Makefile  |  2 +
>  drivers/mux/pi3usb30532.c | 97 
> +++
>  3 files changed, 109 insertions(+)
>  create mode 100644 drivers/mux/pi3usb30532.c
> 
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 17938918bf93..19a3065c34e6 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -58,4 +58,14 @@ config MUX_MMIO
> To compile the driver as a module, choose M here: the module will
> be called mux-mmio.
>  
> +config MUX_PI3USB30532
> + tristate "Pericom PI3USB30532 Type-C cross switch driver"
> + depends on I2C
> + help
> +   This driver adds support for the Pericom PI3USB30532 Type-C cross
> +   switch / mux chip found on some devices with a Type-C port.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-pi3usb30532.
> +
>  endmenu
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index a12e812c7966..7563dbf04593 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -7,9 +7,11 @@ mux-adg792a-objs := adg792a.o
>  mux-gpio-objs:= gpio.o
>  mux-mmio-objs:= mmio.o
>  mux-intel_cht_usb_mux-objs   := intel_cht_usb_mux.o
> +mux-pi3usb30532-objs := pi3usb30532.o
>  
>  obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)+= mux-adg792a.o
>  obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
>  obj-$(CONFIG_MUX_MMIO)   += mux-mmio.o
>  obj-$(CONFIG_MUX_CHT_USB_MUX)+= mux-intel_cht_usb_mux.o
> +obj-$(CONFIG_MUX_PI3USB30532)+= mux-pi3usb30532.o
> diff --git a/drivers/mux/pi3usb30532.c b/drivers/mux/pi3usb30532.c
> new file mode 100644
> index ..fa8abd851520
> --- /dev/null
> +++ b/drivers/mux/pi3usb30532.c
> @@ -0,0 +1,97 @@
> +/*
> + * Pericom PI3USB30532 Type-C cross switch / mux driver
> + *
> + * Copyright (c) 2017 Hans de Goede 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation, or (at your option)
> + * any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include  /* For the MUX_USB_* defines */
> +#include 
> +
> +#define PI3USB30532_CONF 0x00
> +
> +#define PI3USB30532_CONF_OPEN0x00
> +#define PI3USB30532_CONF_SWAP0x01
> +#define PI3USB30532_CONF_4LANE_DP0x02
> +#define PI3USB30532_CONF_USB30x04
> +#define PI3USB30532_CONF_USB3_AND_2LANE_DP   0x06
> +
> +struct pi3usb30532_mux {
> + struct i2c_client *client;
> +};
> +
> +static int pi3usb30532_mux_set_mux(struct mux_control *mux_ctrl, int state)
> +{
> + struct pi3usb30532_mux *mux = mux_chip_priv(mux_ctrl->chip);

The "mux" variable name is used for the mux_control in other drivers, and
I don't think the private data is needed. Like so:

static int pi3usb30532_mux_set_mux(struct mux_control *mux, int state)
{
struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);

...

Cheers,
Peter

> + u8 conf = PI3USB30532_CONF_OPEN;
> +
> + switch (state & ~MUX_USB_POLARITY_INV) {
> + case MUX_USB_NONE:
> + conf = PI3USB30532_CONF_OPEN;
> + break;
> + case MUX_USB_DEVICE:
> + case MUX_USB_HOST:
> + conf = PI3USB30532_CONF_USB3;
> + break;
> + case MUX_USB_HOST_AND_DP_SRC:
> + conf = PI3USB30532_CONF_USB3_AND_2LANE_DP;
> + break;
> + case MUX_USB_DP_SRC:
> + conf = PI3USB30532_CONF_4LANE_DP;
> + break;
> + }
> +
> + if (state & MUX_USB_POLARITY_INV)
> + conf |= PI3USB30532_CONF_SWAP;
> +
> + return i2c_smbus_write_byte_data(mux->client, PI3USB30532_CONF, conf);
> +}
> +
> +static const struct mux_control_ops pi3usb30532_mux_ops = {
> + .set = pi3usb30532_mux_set_mux,
> +};
> +
> +static int pi3usb30532_mux_probe(struct i2c_client *client)
> +{
> + struct device *dev = >dev;
> + struct pi3usb30532_mux *mux;
> + struct mux_chip *mux_chip;
> +
> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux));
> + if (IS_ERR(mux_chip))
> + return PTR_ERR(mux_chip);
> +
> + mux_chip->ops = _mux_ops;
> + mux_chip->mux[0].states = MUX_USB_STATES;
> + mux = mux_chip_priv(mux_chip);
> + mux->client = client;
> +
> + return devm_mux_chip_register(dev, mux_chip);
> +}
> +
> +static const struct i2c_device_id pi3usb30532_mux_table[] = {
> + { "pi3usb30532" },
> 

Re: [PATCH 02/11] mux: core: Add support for getting a mux controller on a non DT platform

[Peter Rosin]

Hi!

Some comments inline...

On 2017-09-01 23:48, Hans de Goede wrote:
> On non DT platforms we cannot get the mux_chip by pnode. Other subsystems
> (regulator, clock, pwm) have the same problem and solve this by allowing
> platform / board-setup code to add entries to a lookup table and then use
> this table to look things up.
> 
> This commit adds support for getting a mux controller on a non DT platform
> following this pattern. It is based on a simplified version of the pwm
> subsys lookup code, the dev_id and mux_name parts of a lookup table entry
> are mandatory in the mux-core implementation.
> 
> Signed-off-by: Hans de Goede 
> ---
>  drivers/mux/core.c   | 96 
> +++-
>  include/linux/mux/consumer.h | 11 +
>  2 files changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 6142493c327b..8864cc745506 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -24,6 +24,9 @@
>  #include 
>  #include 
>  
> +static DEFINE_MUTEX(mux_lookup_lock);
> +static LIST_HEAD(mux_lookup_list);
> +
>  /*
>   * The idle-as-is "state" is not an actual state that may be selected, it
>   * only implies that the state should not be changed. So, use that state
> @@ -408,6 +411,23 @@ int mux_control_deselect(struct mux_control *mux)
>  }
>  EXPORT_SYMBOL_GPL(mux_control_deselect);
>  
> +static int parent_name_match(struct device *dev, const void *data)
> +{
> + const char *parent_name = dev_name(dev->parent);
> + const char *name = data;
> +
> + return strcmp(parent_name, name) == 0;
> +}
> +
> +static struct mux_chip *mux_chip_get_by_name(const char *name)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(_class, NULL, name, parent_name_match);
> +
> + return dev ? to_mux_chip(dev) : NULL;
> +}
> +
>  static int of_dev_node_match(struct device *dev, const void *data)
>  {
>   return dev->of_node == data;
> @@ -479,6 +499,42 @@ static struct mux_control *of_mux_control_get(struct 
> device *dev,
>  }
>  
>  /**
> + * mux_add_table() - register PWM device consumers

register mux controllers (because you are not registering consumers, right?
someone is registering controllers so that they can be found by consumers?)

> + * @table: array of consumers to register
> + * @num: number of consumers in table

controllers?

> + */
> +void mux_add_table(struct mux_lookup *table, size_t num)
> +{
> + mutex_lock(_lookup_lock);
> +
> + while (num--) {
> + list_add_tail(>list, _lookup_list);
> + table++;
> + }

I prefer

for (; num--; table++)
list_add_tail(>list, _lookup_list);

> +
> + mutex_unlock(_lookup_lock);
> +}
> +EXPORT_SYMBOL_GPL(mux_add_table);
> +
> +/**
> + * mux_remove_table() - unregister PWM device consumers

unregister mux controllers(?)

> + * @table: array of consumers to unregister
> + * @num: number of consumers in table

controllers?

> + */
> +void mux_remove_table(struct mux_lookup *table, size_t num)
> +{
> + mutex_lock(_lookup_lock);
> +
> + while (num--) {
> + list_del(>list);
> + table++;
> + }

for() loop here as well.

> +
> + mutex_unlock(_lookup_lock);
> +}
> +EXPORT_SYMBOL_GPL(mux_remove_table);
> +
> +/**
>   * mux_control_get() - Get the mux-control for a device.
>   * @dev: The device that needs a mux-control.
>   * @mux_name: The name identifying the mux-control.
> @@ -487,11 +543,49 @@ static struct mux_control *of_mux_control_get(struct 
> device *dev,
>   */
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  {
> + struct mux_lookup *m, *chosen = NULL;
> + const char *dev_id = dev_name(dev);
> + struct mux_chip *mux_chip;
> +
>   /* look up via DT first */
>   if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>   return of_mux_control_get(dev, mux_name);
>  
> - return ERR_PTR(-ENODEV);
> + /*
> +  * For non DT we look up the provider in the static table typically
> +  * provided by board setup code.
> +  *
> +  * If a match is found, the provider mux chip is looked up by name
> +  * and a mux-control is requested using the table provided index.
> +  */
> + mutex_lock(_lookup_lock);
> + list_for_each_entry(m, _lookup_list, list) {
> + if (WARN_ON(!m->dev_id || !m->mux_name || !m->provider))
> + continue;
> +
> + if (strcmp(m->dev_id, dev_id) == 0 &&
> + strcmp(m->mux_name, mux_name) == 0) {

I want the below format (with ! instead of == 0 and the brace on the next line
when the condition has a line break):

if (!strcmp(m->dev_id, dev_id) &&
!strcmp(m->mux_name, mux_name))
{

> + chosen = m;
> + break;
> + }
> + }
> + mutex_unlock(_lookup_lock);
> +
> + if 

Re: [PATCH 00/11] mux/typec: Add USB / TypeC mux drivers and hook them up on some x86 systems

[Peter Rosin]

On 2017-09-01 23:48, Hans de Goede wrote:
> Hi All,
> 
> This series consists of 4 parts:
> 
> 1) Core mux changes to add support for getting mux-controllers on
>non DT platforms and to add some standardised state values for USB
> 
> 2) Add Intel CHT USB mux and Pericom-PI3USB30532 Type-C mux drivers
> 
> 3) Hookup the Intel CHT USB mux driver for CHT devices without Type-C
> 
> 4) Hookup both the Intel CHT USB mux and Pericom-PI3USB30532 Type-C mux
>drivers to the fusb302 Type-C port controller found on some CHT x86 devs
> 
> Please review, or in the case of the drivers/mux changes (which are a dep
> for some of the other patches) merge if the patches are ready.

Hi Hans,

I see commonalities with this and the below patch seriess from Stephen
Boyd [added to Cc].

https://lkml.org/lkml/2017/7/11/696 [PATCH 0/3] USB Mux support for Chipidea
https://lkml.org/lkml/2017/7/14/654 [PATCH v2 0/3] USB Mux support for Chipidea

Stephen had a patch that added mux_control_get_optional() that I think
could be of use for this series?

Anyway, there seems to be some interest in using the mux subsystem for
handling the USB host/device role. I think the role-switch interface
between the USB and mux subsystems should be done similarly, regardless
of what particular USB driver needs to switch role using whatever
particular mux driver. If at all possible.

The way I read it, the pi3usb30532 driver is the one adding the need to
involve the DP states and the inverted bit. Those things are not used by
anything else, and I think it clutters up things for everybody when the
weird needs of one driver "invades" the mux states needed to control the
USB role. So, I would like USB role switching to have 2 states, device
and host (0 and 1 is used by the above chipidea code). And then the USB
switch can of course be idle, which is best represented with one of
MUX_IDLE_DISCONNECT, MUX_IDLE_AS_IS or one of the modes depending on if
the USB switch can or can't disconnect (or other considerations). Now,
you can't explicitly set the idle state using mux_control_select(), it
can only be set implicitly using mux_control_deselect(), so that will
require some changes in the consumer logic.

Regarding the pi3usb30532 driver, I think the above is in fact also
better since the "swap bit" means something totally different when the
switch is "open" (if I read the datasheet correctly).

I.e. PI3USB30532_CONF_OPEN | PI3USB30532_CONF_SWAP is not sane, and that
would just go away completely if the driver implemented MUX_IDLE_DISCONNECT
as PI3USB30532_CONF_OPEN and removed the possibility to explicitly set the
"open" state with mux_control_select().

So, I think the generic USB role switch interface should be something like:

#define MUX_USB_DEVICE  (0) /* USB device mode */
#define MUX_USB_HOST(1) /* USB host mode */

And then the pi3usb30532 driver can extend that:

#define MUX_PI3USB30352_HOST_AND_DP_SRC (2)/* USB host + 2 lanes Display 
Port */
#define MUX_PI3USB30352_DP_SRC  (3)/* 4 lanes Display Port source */
#define MUX_PI3USB30352_POLARITY_INVBIT(2) /* Polarity inverted bit */
#define MUX_PI3USB30352_STATES  (8)

Another idea is to expose the inverted bit as a separate mux controller,
but I suspect that you're not too keen on operating three muxes in the
tcpm driver... That will get simpler with mux_control_get_optional() though.

BTW, I don't think these USB defines belong in mux/consumer.h, please add
them in a new include/linux/mux/usb.h file or something. And I'd like some
MAINTAINER entry to cover the new mux drivers...

I'll respond to the individual patches with nits etc, but it generally looks
tidy, thank you for that!

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2017-08-12 00:26, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-08-08 05:46:30)
>> On 2017-08-08 03:51, Stephen Boyd wrote:
>>
>>>  It looked like we paired the start/stop ops with
>>> each other so that the mux is properly managed across these ops.
>>
>> Yes, it *looks* ok...
>>
>>>  My
>>> testing hasn't shown a problem, but maybe there's some corner case
>>> you're thinking of? I'll double check the code.
>>
>> ...but since I do not know the usb code, I can't tell. What I worry about
>> is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
>> more than once without any call to the other in between. Maybe that is a
>> guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
>> third mode (or if one is added in the future), then the calls to
>> mux_control_select and mux_control_deselect would not be paired correctly.
>> Ok, sure, a third mode probably doesn't exist and will probably not be
>> added, but but but...
>>
>> Also, what happens if udc_id_switch_for_device fails? Is it certain that
>> it will be called again before udc_id_switch_for_host is called, or is
>> the failure simply logged? If the latter, you might have a call to
>> mux_control_deselect without a preceding (and successful) call to
>> mux_control_select. That's fatal.
> 
> The only thing that could fail right now is the mux selection, so we
> wouldn't get into some sort of situation where that's locked in and
> unchangeable. We do rollback the role if it fails to switch, so we also
> wouldn't go into a half-way state of being in one role but not actually
> switching all the way over to it.

What do you roll back to if the initial setting of the role fails?
Hopefully that causes a probe failure?

Cheers,
Peter

>> I have similar worries for host_start/host_stop, but for that case
>> host_stop is not allowed to fail, and it seems like a safe bet that
>> host_stop will only be called if host_start succeeds. So, I'm not as
>> worried there.
>>
>> In other words, the question is if the usb core is designed to allow
>> this kind of "raw" resource administration in udc_id_switch_for_host and
>> udc_id_switch_for_device, or if you need to keep a local record of the
>> state so that you do not do double resource acquisition or attempt to
>> free resources you don't have?
>>
>> I think I would feel better if the muxing for the device mode could
>> be done in a start/stop pair of function just like the host mode is
>> doing. Again, I don't know the usb code and don't know if such hooks
>> exist or not?
>>
> 
> The host_start/host_stop functions are assigned to the same struct
> ci_role_driver ops that udc_idc_switch_for_{device,host} are for the
> gadget role. Really, these things are called from the same place by the
> chipidea driver so not much is different between the two files I modify
> to make the mux calls. Furthermore, we don't want to do this if we have
> HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode()
> check to make sure we don't do any muxing stuff based on fsm state
> changes. It doesn't really make any sense here anyway because this
> device I have doesn't support OTG, just role switching.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2017-08-08 03:51, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-31 03:33:22)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>>>  
>>>  static int udc_id_switch_for_device(struct ci_hdrc *ci)
>>>  {
>>> + int ret = 0;
>>> +
>>>   if (ci->is_otg)
>>>   /* Clear and enable BSV irq */
>>>   hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
>>>   OTGSC_BSVIS | OTGSC_BSVIE);
>>>  
>>> - return 0;
>>> + if (!ci_otg_is_fsm_mode(ci))
>>> + ret = mux_control_select(ci->platdata->usb_switch, 0);
>>> +
>>> + if (ci->is_otg && ret)
>>> + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS);
>>> +
>>> + return ret;
>>>  }
>>>  
>>>  static void udc_id_switch_for_host(struct ci_hdrc *ci)
>>>  {
>>> + mux_control_deselect(ci->platdata->usb_switch);
>>> +
>>
>> This looks broken. You conditionally lock the mux and you unconditionally
>> unlock it. Quoting the mux_control_deselect doc:
>>
>>  * It is required that a single call is made to mux_control_deselect() for
>>  * each and every successful call made to either of mux_control_select() or
>>  * mux_control_try_select().
>>
>> Think of the mux as a semaphore with a max count of one. If you lock it,
>> you have to unlock it when you're done. If you didn't lock it, you have
>> zero business unlocking it. If you try to lock it but fail, you also have
>> no business unlocking it. Just like a semaphore.
> 
> Good catch. I've added a if (!ci_otg_is_fsm_mode()) check here.
> 
>>
>> Another point: I do not know if udc_id_switch_for_host is *only* called
>> when there has been a prior call to udc_id_switch_for_device that
>> succeeded or how this works exactly. But this all looks fragile. Using
>> mux_control_select and mux_control_deselect *must* be done in pairs.
>> If you want a mux to be locked for "a while", such as in this case, you
>> have to make sure you stay within the rules. There is no room for half
>> measures, or you will likely cause a deadlock when something unexpected
>> happens.
>>
> 
> Can you elaborate? Is it bad that we keep it "locked" while we're in
> host or device mode?

No no, that's good. You do not want some other consumer of the same mux
controller to clobber your state (in case it's shared), so you absolutely
want to have the mux locked when you want it to remain in a specific
position.

>  It looked like we paired the start/stop ops with
> each other so that the mux is properly managed across these ops.

Yes, it *looks* ok...

>  My
> testing hasn't shown a problem, but maybe there's some corner case
> you're thinking of? I'll double check the code.

...but since I do not know the usb code, I can't tell. What I worry about
is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device
more than once without any call to the other in between. Maybe that is a
guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a
third mode (or if one is added in the future), then the calls to
mux_control_select and mux_control_deselect would not be paired correctly.
Ok, sure, a third mode probably doesn't exist and will probably not be
added, but but but...

Also, what happens if udc_id_switch_for_device fails? Is it certain that
it will be called again before udc_id_switch_for_host is called, or is
the failure simply logged? If the latter, you might have a call to
mux_control_deselect without a preceding (and successful) call to
mux_control_select. That's fatal.

I have similar worries for host_start/host_stop, but for that case
host_stop is not allowed to fail, and it seems like a safe bet that
host_stop will only be called if host_start succeeds. So, I'm not as
worried there.

In other words, the question is if the usb core is designed to allow
this kind of "raw" resource administration in udc_id_switch_for_host and
udc_id_switch_for_device, or if you need to keep a local record of the
state so that you do not do double resource acquisition or attempt to
free resources you don't have?

I think I would feel better if the muxing for the device mode could
be done in a start/stop pair of function just like the host mode is
doing. Again, I don't know the usb code and don't know if such hooks
exist or not?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2017-07-14 23:40, Stephen Boyd wrote:
> On the db410c 96boards platform we have a TC7USB40MU on the board
> to mux the D+/D- lines coming from the controller between a micro
> usb "device" port and a USB hub for "host" roles[1]. During a
> role switch, we need to toggle this mux to forward the D+/D-
> lines to either the port or the hub. Add the necessary code to do
> the role switch in chipidea core via the generic mux framework.
> Board configurations like on db410c are expected to change roles
> via the sysfs API described in
> Documentation/ABI/testing/sysfs-platform-chipidea-usb2.
> 
> [1] 
> https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf
> 
> Cc: Peter Rosin <p...@axentia.se>
> Cc: Peter Chen <peter.c...@nxp.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: <devicet...@vger.kernel.org>
> Signed-off-by: Stephen Boyd <stephen.b...@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt |  6 ++
>  drivers/usb/chipidea/Kconfig   |  1 +
>  drivers/usb/chipidea/core.c|  5 +
>  drivers/usb/chipidea/host.c|  7 +++
>  drivers/usb/chipidea/udc.c | 13 -
>  include/linux/usb/chipidea.h   |  2 ++
>  6 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 0e03344e2e8b..2e9318151df7 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -76,6 +76,10 @@ Optional properties:
>needs to make sure it does not send more than 90%
>maximum_periodic_data_per_frame. The use case is multiple transactions, but
>less frame rate.
> +- mux-controls: The mux control for toggling host/device output of this
> +  controller. It's expected that a mux state of 0 indicates device mode and a
> +  mux state of 1 indicates host mode.
> +- mux-control-names: Shall be "usb_switch" if mux-controls is specified.
>  
>  i.mx specific properties
>  - fsl,usbmisc: phandler of non-core register device, with one
> @@ -102,4 +106,6 @@ Example:
>   rx-burst-size-dword = <0x10>;
>   extcon = <0>, <_id>;
>   phy-clkgate-delay-us = <400>;
> + mux-controls = <_switch>;
> + mux-control-names = "usb_switch";
>   };
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 51f4157bbecf..3798e0e69d57 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -3,6 +3,7 @@ config USB_CHIPIDEA
>   depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && 
> !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
>   select EXTCON
>   select RESET_CONTROLLER
> + select MULTIPLEXER
>   help
> Say Y here if your system has a dual role high speed USB
> controller based on ChipIdea silicon IP. It supports:
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index b17ed3a9a304..d088c262ebb8 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -64,6 +64,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "ci.h"
>  #include "udc.h"
> @@ -690,6 +691,10 @@ static int ci_get_platdata(struct device *dev,
>   if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
>   platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
>  
> + platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
> + if (IS_ERR(platdata->usb_switch))
> + return PTR_ERR(platdata->usb_switch);
> +
>   ext_id = ERR_PTR(-ENODEV);
>   ext_vbus = ERR_PTR(-ENODEV);
>   if (of_property_read_bool(dev->of_node, "extcon")) {
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 18cb8e46262d..9ef3ecf27ad3 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../host/ehci.h"
>  
> @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci)
>   if (ci_otg_is_fsm_mode(ci)) {
>   otg->host = >self;
>   hcd->se

Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Peter Rosin]

On 2017-07-19 04:08, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-17 01:20:14)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>> index 90b8995f07cb..a0e5bf16f02f 100644
>>> --- a/drivers/mux/mux-core.c
>>> +++ b/drivers/mux/mux-core.c
>>> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>>>   */
>>>  unsigned int mux_control_states(struct mux_control *mux)
>>>  {
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> I don't think this is appropriate. For this function, it might be ok,
>> but...
>>
>>>   return mux->states;
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_states);
>>> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, 
>>> unsigned int state)
>>>  {
>>>   int ret;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> ...here and for other cases below it's very odd to return "ok", when
>> -EINVAL or something seems much more appropriate. And if -EINVAL is
>> returned here, the benefit of returning fake values for anything
>> pretty much falls apart.
>>
>> I simply don't like it, and prefer if the consumer code is arranged
>> to not call the mux functions when the optional get() does not find
>> the mux.
> 
> Do you want the callers of the mux APIs to know that an optional mux
> isn't there, and then have checks at all callsites on optional muxes to
> make sure consumers don't call the mux functions? Won't that duplicate
> lots of checks in drivers for something the core could treat as a don't
> care case? Sorry, I don't understand why the consumer cares that it was
> there or not when it is optional.

Ok, I had a look around to figure out how others handle this, and e.g.
gpio has (ugly) macros (VALIDATE_DESC) to handle this. I guess you are
right and I'm wrong. So, please keep all the if (!mux) checks.

Thanks for insisting!

>>
>>>   ret = down_killable(>lock);
>>>   if (ret < 0)
>>>   return ret;
>>> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
>>> unsigned int state)
>>>  {
>>>   int ret;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>>   if (down_trylock(>lock))
>>>   return -EBUSY;
>>>  
>>> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>>>  {
>>>   int ret = 0;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>>   if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>   mux->idle_state != mux->cached_state)
>>>   ret = mux_control_set(mux, mux->idle_state);
>>> @@ -422,14 +434,8 @@ static struct mux_chip 
>>> *of_find_mux_chip_by_node(struct device_node *np)
>>>   return dev ? to_mux_chip(dev) : NULL;
>>>  }
>>>  
>>> -/**
>>> - * mux_control_get() - Get the mux-control for a device.
>>> - * @dev: The device that needs a mux-control.
>>> - * @mux_name: The name identifying the mux-control.
>>> - *
>>> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative 
>>> errno.
>>> - */
>>> -struct mux_control *mux_control_get(struct device *dev, const char 
>>> *mux_name)
>>> +struct mux_control *
>>> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>>>  {
>>>   struct device_node *np = dev->of_node;
>>>   struct of_phandle_args args;
>>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, 
>>> const char *mux_name)
>>>   if (mux_name) {
>>>   index = of_property_match_string(np, "mux-control-names",
>>>mux_name);
>>> + if (index == -EINVAL && optional)
>>> + return NULL;
>>
>> What about -ENODATA?
> 
> At this point in the code we're finding the index of a mux-control-names
> property so I was trying to handle the case where there isn't a
> mux-control-names property at all

Yes, you indeed need to check for -EINVAL to catch that. No argument
about that.

>   but we're looking for something
> optional anyway. If there is a property, then we would see some other
> error if so

Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Peter Rosin]

Generally looks like I imagined, but there are a few nits and some
things that I'd like to do differently. Comments inline. Thanks!

On 2017-07-14 23:40, Stephen Boyd wrote:
> Sometimes drivers only use muxes under certain scenarios. For
> example, the chipidea usb controller may be connected to a usb
> switch on some platforms, and connected directly to a usb port on
> others. The driver won't know one way or the other though, so add
> a mux_control_get_optional() API that allows the driver to
> differentiate errors getting the mux from there not being a mux
> for the driver to use at all.
> 
> Cc: Jonathan Cameron 
> Cc: Philipp Zabel 
> Signed-off-by: Stephen Boyd 
> ---
>  Documentation/driver-model/devres.txt |  1 +
>  drivers/mux/mux-core.c| 98 
> ---
>  include/linux/mux/consumer.h  |  4 ++
>  3 files changed, 83 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/driver-model/devres.txt 
> b/Documentation/driver-model/devres.txt
> index 30e04f7a690d..4fdd3e63ff8b 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -342,6 +342,7 @@ MUX
>devm_mux_chip_alloc()
>devm_mux_chip_register()
>devm_mux_control_get()
> +  devm_mux_control_get_optional()
>  
>  PER-CPU MEM
>devm_alloc_percpu()
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> index 90b8995f07cb..a0e5bf16f02f 100644
> --- a/drivers/mux/mux-core.c
> +++ b/drivers/mux/mux-core.c
> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>   */
>  unsigned int mux_control_states(struct mux_control *mux)
>  {
> + if (!mux)
> + return 0;
> +

I don't think this is appropriate. For this function, it might be ok,
but...

>   return mux->states;
>  }
>  EXPORT_SYMBOL_GPL(mux_control_states);
> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned 
> int state)
>  {
>   int ret;
>  
> + if (!mux)
> + return 0;
> +

...here and for other cases below it's very odd to return "ok", when
-EINVAL or something seems much more appropriate. And if -EINVAL is
returned here, the benefit of returning fake values for anything
pretty much falls apart.

I simply don't like it, and prefer if the consumer code is arranged
to not call the mux functions when the optional get() does not find
the mux.

>   ret = down_killable(>lock);
>   if (ret < 0)
>   return ret;
> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
> unsigned int state)
>  {
>   int ret;
>  
> + if (!mux)
> + return 0;
> +
>   if (down_trylock(>lock))
>   return -EBUSY;
>  
> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>  {
>   int ret = 0;
>  
> + if (!mux)
> + return 0;
> +
>   if (mux->idle_state != MUX_IDLE_AS_IS &&
>   mux->idle_state != mux->cached_state)
>   ret = mux_control_set(mux, mux->idle_state);
> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct 
> device_node *np)
>   return dev ? to_mux_chip(dev) : NULL;
>  }
>  
> -/**
> - * mux_control_get() - Get the mux-control for a device.
> - * @dev: The device that needs a mux-control.
> - * @mux_name: The name identifying the mux-control.
> - *
> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> - */
> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +struct mux_control *
> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>  {
>   struct device_node *np = dev->of_node;
>   struct of_phandle_args args;
> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, 
> const char *mux_name)
>   if (mux_name) {
>   index = of_property_match_string(np, "mux-control-names",
>mux_name);
> + if (index == -EINVAL && optional)
> + return NULL;

What about -ENODATA? And if an optional mux is found here, but lookup
fails later in e.g. the of_parse_phandle_with_args call, then I think
an error should be returned. Because that seems like an indication that
DT specifies that there *should* be a mux, but then there isn't one.

>   if (index < 0) {
>   dev_err(dev, "mux controller '%s' not found\n",
>   mux_name);
> @@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, 
> const char *mux_name)
>   ret = of_parse_phandle_with_args(np,
>"mux-controls", "#mux-control-cells",
>index, );
> + if (ret == -ENOENT && optional)
> + return NULL;
>   if (ret) {
>   dev_err(dev, "%s: failed to get mux-control 

Re: [PATCH 2/3] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2017-07-12 03:02, Stephen Boyd wrote:
> On the db410c 96boards platform we have a TC7USB40MU on the board
> to mux the D+/D- lines coming from the controller between a micro
> usb "device" port and a USB hub for "host" roles[1]. During a
> role switch, we need to toggle this mux to forward the D+/D-
> lines to either the port or the hub. Add the necessary code to do
> the role switch in chipidea core via the generic mux framework.
> Board configurations like on db410c are expected to change roles
> via the sysfs API described in
> Documentation/ABI/testing/sysfs-platform-chipidea-usb2.
> 
> [1] 
> https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf
> 
> Cc: Peter Rosin <p...@axentia.se>
> Cc: Peter Chen <peter.c...@nxp.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: <devicet...@vger.kernel.org>
> Signed-off-by: Stephen Boyd <stephen.b...@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt |  8 
>  drivers/usb/chipidea/core.c| 17 +
>  drivers/usb/chipidea/host.c| 10 ++
>  drivers/usb/chipidea/udc.c | 11 +++
>  include/linux/usb/chipidea.h   | 14 ++
>  5 files changed, 60 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 0e03344e2e8b..96ce81d975d5 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -76,6 +76,11 @@ Optional properties:
>needs to make sure it does not send more than 90%
>maximum_periodic_data_per_frame. The use case is multiple transactions, but
>less frame rate.
> +- mux-controls: The mux control for toggling host/device output of this
> +  controller.
> +- mux-control-names: Shall be "usb_switch" if mux-controls is specified.
> +- usb-switch-states: Two u32's defining the state to set on the mux for the
> +  host mode and device modes respectively.
>  
>  i.mx specific properties
>  - fsl,usbmisc: phandler of non-core register device, with one
> @@ -102,4 +107,7 @@ Example:
>   rx-burst-size-dword = <0x10>;
>   extcon = <0>, <_id>;
>   phy-clkgate-delay-us = <400>;
> + mux-controls = <_switch>;
> + mux-control-names = "usb_switch";
> + usb-switch-states = <0>, <1>;

I don't see the need for usb-switch-states? Just assume states 0/1 and
if someone later needs some other states, make them add a property that
overrides the defaults. Just document that 0 is host and 1 is device.

>   };
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index b17ed3a9a304..6531d771f296 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -64,6 +64,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "ci.h"
>  #include "udc.h"
> @@ -606,6 +607,7 @@ static int ci_get_platdata(struct device *dev,
>  {
>   struct extcon_dev *ext_vbus, *ext_id;
>   struct ci_hdrc_cable *cable;
> + struct ci_hdrc_switch *usb_switch;
>   int ret;
>  
>   if (!platdata->phy_mode)
> @@ -690,6 +692,21 @@ static int ci_get_platdata(struct device *dev,
>   if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
>   platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
>  
> + if (IS_ENABLED(CONFIG_MULTIPLEXER)) {
> + usb_switch = >usb_switch;
> + usb_switch->mux = devm_mux_control_get(dev, "usb_switch");
> + if (!IS_ERR(usb_switch->mux)) {
> + if (of_property_read_u32_index(dev->of_node,
> +"usb-switch-states",
> +0, _switch->device))
> + return -EINVAL;
> + if (of_property_read_u32_index(dev->of_node,
> +"usb-switch-states",
> +1, _switch->host))
> + return -EINVAL;
> + }
> + }
> +
>   ext_id = ERR_PTR(-ENODEV);
>   ext_vbus = ERR_PTR(-ENODEV);
>   if (of_property_read_bool(dev->of_node, "extcon")) {
> diff --git a/drivers/u

Re: [PATCH 0/3] USB Mux support for Chipidea

[Peter Rosin]

On 2017-07-12 03:02, Stephen Boyd wrote:
> This patchset adds support for the TC7USB40MU usb mux found on 
> db410c 96boards platforms via the new multiplexer framework and
> hooks that into the chipidea driver. This allows us to properly
> control host or device mode on this board via the sysfs knob.
> 
> So far I've only tested this on db410c, and there are some rough
> edges to finish off before it can merge. Also I'm experiencing
> odd behavior with switching the role while gadget is enabled and
> the micro-usb cable is kept connected. Not sure what's wrong but
> it seems like the gadget never gets disconnected? I'll investigate
> more.
> 
> TODO:
> 
>  1. The mux framework has to be selected for consumers to use it. We'll
> need some stubs in the consumer header file to allow compilation to
> continue without mux always enabled by consumers.

Instead of "depends on MULTIPLEXER", just add "select MULTIPLEXER"
to the Kconfig. Otherwise, you'll have to convince Linus that we
really do need a Kconfig question for the subsystem :-)

https://lkml.org/lkml/2017/7/4/118

>  2. We probably need some sort of mux_control_get_optional() API so that
> we know if there was an error getting the mux control, instead of just
> ignoring errors. For now I can pass up EPROBE_DEFER errors and ignore
> other errors and consider it "missing from DT".

Yes, mux_control_get_optional should be easy to add.

>  3. Maybe we can get rid of the mux driver and just use mux-gpio.c with
> a compatible string update? I split it off because we may want to
> support the "S" pin on the TC7USB40MU one day that shuts off both
> mux outputs.

Maybe no need for a compatible update either, if it works to do something
like this in the DT?

usb_switch: usb-switch {
compatible = "gpio-mux";
mux-gpios = <_gpios 4 GPIO_ACTIVE_HIGH>,
<_gpios XXX GPIO_ACTIVE_XXX>;
idle-state = <2>;
#mux-control-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <_sw_sel_pm>;
};

But I obviously know little about how things are wired and really works,
so that might be totally off...

Otherwise, maybe a generic mux-pinctrl driver would do the trick?
(compare with drivers/i2c/muxes/i2c-mux-pinctrl.c)

Cheers,
peda

>  4. The userspace side of things is murky. What is expected to go and toggle
> the host/gadget side of things in userspace at this very specific location
> for chipidea devices?
> 
> Stephen Boyd (3):
>   usb: misc: Add a driver for TC7USB40MU
>   usb: chipidea: Hook into mux framework to toggle usb switch
>   arm64: dts: qcom: Collapse usb support into one node
> 
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt   |  8 +++
>  .../devicetree/bindings/usb/toshiba,tc7usb40mu.txt | 31 +
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi  | 39 +++-
>  arch/arm64/boot/dts/qcom/msm8916.dtsi  | 62 +-
>  drivers/usb/chipidea/core.c| 17 +
>  drivers/usb/chipidea/host.c| 10 +++
>  drivers/usb/chipidea/udc.c | 11 
>  drivers/usb/misc/Kconfig   | 11 
>  drivers/usb/misc/Makefile  |  1 +
>  drivers/usb/misc/tc7usb40mu.c  | 74 
> ++
>  include/linux/usb/chipidea.h   | 14 
>  11 files changed, 228 insertions(+), 50 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/usb/toshiba,tc7usb40mu.txt
>  create mode 100644 drivers/usb/misc/tc7usb40mu.c
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Peter Rosin]

On 2016-12-22 18:27, Greg Kroah-Hartman wrote:
> On Thu, Dec 22, 2016 at 08:43:55AM +0100, Peter Rosin wrote:
>> The gpiod_get* function family does not want the -gpio suffix.
>> Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
>> The descriptor based APIs handle active high/low automatically.
>> The vbus-gpios are output, request enable while getting the gpio.
>> Don't try to get any vbus-gpios for ports outside num-ports.
>>
>> WTF? Big sigh.
>>
>> Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
>> Signed-off-by: Peter Rosin <p...@axentia.se>
>> ---
>>
>> Hi!
>>
>> Resending this, since the only response I've got is that the merge
>> window is open and that this patch has been put on hold due to that.
>> But I think this regression (which happend between v4.9 and current
>> master) should be fixed before the merge window closes.
> 
> I don't merge patches before -rc1 comes out, sorry, people should have
> tested linux-next better :)

Neat, shift the blame for the shit patch over to the messenger :)

> I'll catch up the first week of January, relax.

As we all know, unrelated regressions are painful when trying to locate
other problems. It's seems silly to have a few extra for no good reason.

*I* know about this one, it's those you don't know about that are worst.

Cheers,
peda

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 REGRESSION RESEND] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Peter Rosin]

The gpiod_get* function family does not want the -gpio suffix.
Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
The descriptor based APIs handle active high/low automatically.
The vbus-gpios are output, request enable while getting the gpio.
Don't try to get any vbus-gpios for ports outside num-ports.

WTF? Big sigh.

Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
Signed-off-by: Peter Rosin <p...@axentia.se>
---

Hi!

Resending this, since the only response I've got is that the merge
window is open and that this patch has been put on hold due to that.
But I think this regression (which happend between v4.9 and current
master) should be fixed before the merge window closes.

v1 -> v2 changes:
- use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional

Cheers,
peda

 drivers/usb/host/ohci-at91.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 1b2e09c32c6b..66ec407df2db 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -43,7 +43,6 @@ struct at91_usbh_data {
struct gpio_desc *overcurrent_pin[AT91_MAX_USBH_PORTS];
u8 ports;   /* number of ports on root hub 
*/
u8 overcurrent_supported;
-   u8 vbus_pin_active_low[AT91_MAX_USBH_PORTS];
u8 overcurrent_status[AT91_MAX_USBH_PORTS];
u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
 };
@@ -268,8 +267,7 @@ static void ohci_at91_usb_set_power(struct at91_usbh_data 
*pdata, int port, int
if (!valid_port(port))
return;
 
-   gpiod_set_value(pdata->vbus_pin[port],
-   pdata->vbus_pin_active_low[port] ^ enable);
+   gpiod_set_value(pdata->vbus_pin[port], enable);
 }
 
 static int ohci_at91_usb_get_power(struct at91_usbh_data *pdata, int port)
@@ -277,8 +275,7 @@ static int ohci_at91_usb_get_power(struct at91_usbh_data 
*pdata, int port)
if (!valid_port(port))
return -EINVAL;
 
-   return gpiod_get_value(pdata->vbus_pin[port]) ^
-  pdata->vbus_pin_active_low[port];
+   return gpiod_get_value(pdata->vbus_pin[port]);
 }
 
 /*
@@ -535,18 +532,17 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
pdata->ports = ports;
 
at91_for_each_port(i) {
-   pdata->vbus_pin[i] = devm_gpiod_get_optional(>dev,
-"atmel,vbus-gpio",
-GPIOD_IN);
+   if (i >= pdata->ports)
+   break;
+
+   pdata->vbus_pin[i] =
+   devm_gpiod_get_index_optional(>dev, "atmel,vbus",
+ i, GPIOD_OUT_HIGH);
if (IS_ERR(pdata->vbus_pin[i])) {
err = PTR_ERR(pdata->vbus_pin[i]);
dev_err(>dev, "unable to claim gpio \"vbus\": 
%d\n", err);
continue;
}
-
-   pdata->vbus_pin_active_low[i] = 
gpiod_get_value(pdata->vbus_pin[i]);
-
-   ohci_at91_usb_set_power(pdata, i, 1);
}
 
at91_for_each_port(i) {
@@ -554,8 +550,8 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
break;
 
pdata->overcurrent_pin[i] =
-   devm_gpiod_get_optional(>dev,
-   "atmel,oc-gpio", GPIOD_IN);
+   devm_gpiod_get_index_optional(>dev, "atmel,oc",
+ i, GPIOD_IN);
if (IS_ERR(pdata->overcurrent_pin[i])) {
err = PTR_ERR(pdata->overcurrent_pin[i]);
dev_err(>dev, "unable to claim gpio 
\"overcurrent\": %d\n", err);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Peter Rosin]

The gpiod_get* function family does not want the -gpio suffix.
Use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional.
The descriptor based APIs handle active high/low automatically.
The vbus-gpios are output, request enable while getting the gpio.
Don't try to get any vbus-gpios for ports outside num-ports.

WTF? Big sigh.

Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
Signed-off-by: Peter Rosin <p...@axentia.se>
---

Hi!

Ok, I looked again and found one more problem with that garbage patch
(which did not affect me since I have just one port). You might want to
pick this up before v4.10-rc1, it's not nice to regress in the area of
over-current detection...

v1 -> v2 changes:
- use devm_gpiod_get_index_optional instead of devm_gpiod_get_optional

Cheers,
peda

 drivers/usb/host/ohci-at91.c | 24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 1b2e09c32c6b..66ec407df2db 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -43,7 +43,6 @@ struct at91_usbh_data {
struct gpio_desc *overcurrent_pin[AT91_MAX_USBH_PORTS];
u8 ports;   /* number of ports on root hub 
*/
u8 overcurrent_supported;
-   u8 vbus_pin_active_low[AT91_MAX_USBH_PORTS];
u8 overcurrent_status[AT91_MAX_USBH_PORTS];
u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
 };
@@ -268,8 +267,7 @@ static void ohci_at91_usb_set_power(struct at91_usbh_data 
*pdata, int port, int
if (!valid_port(port))
return;
 
-   gpiod_set_value(pdata->vbus_pin[port],
-   pdata->vbus_pin_active_low[port] ^ enable);
+   gpiod_set_value(pdata->vbus_pin[port], enable);
 }
 
 static int ohci_at91_usb_get_power(struct at91_usbh_data *pdata, int port)
@@ -277,8 +275,7 @@ static int ohci_at91_usb_get_power(struct at91_usbh_data 
*pdata, int port)
if (!valid_port(port))
return -EINVAL;
 
-   return gpiod_get_value(pdata->vbus_pin[port]) ^
-  pdata->vbus_pin_active_low[port];
+   return gpiod_get_value(pdata->vbus_pin[port]);
 }
 
 /*
@@ -535,18 +532,17 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
pdata->ports = ports;
 
at91_for_each_port(i) {
-   pdata->vbus_pin[i] = devm_gpiod_get_optional(>dev,
-"atmel,vbus-gpio",
-GPIOD_IN);
+   if (i >= pdata->ports)
+   break;
+
+   pdata->vbus_pin[i] =
+   devm_gpiod_get_index_optional(>dev, "atmel,vbus",
+ i, GPIOD_OUT_HIGH);
if (IS_ERR(pdata->vbus_pin[i])) {
err = PTR_ERR(pdata->vbus_pin[i]);
dev_err(>dev, "unable to claim gpio \"vbus\": 
%d\n", err);
continue;
}
-
-   pdata->vbus_pin_active_low[i] = 
gpiod_get_value(pdata->vbus_pin[i]);
-
-   ohci_at91_usb_set_power(pdata, i, 1);
}
 
at91_for_each_port(i) {
@@ -554,8 +550,8 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
break;
 
pdata->overcurrent_pin[i] =
-   devm_gpiod_get_optional(>dev,
-   "atmel,oc-gpio", GPIOD_IN);
+   devm_gpiod_get_index_optional(>dev, "atmel,oc",
+ i, GPIOD_IN);
if (IS_ERR(pdata->overcurrent_pin[i])) {
err = PTR_ERR(pdata->overcurrent_pin[i]);
dev_err(>dev, "unable to claim gpio 
\"overcurrent\": %d\n", err);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: ohci-at91: use descriptor-based gpio APIs correctly

[Peter Rosin]

The devm_gpiod_get_optional function does not want the -gpio suffix.
The descriptor based APIs handle active high/low automatically.
The vbus gpios are output, request enable while getting the gpio.
Don't try to get any vbus gpios for ports outside num-ports.

WTF? Big sigh.

Fixes: 054d4b7b577d ("usb: ohci-at91: Use descriptor-based gpio APIs")
Signed-off-by: Peter Rosin <p...@axentia.se>
---
 drivers/usb/host/ohci-at91.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 1b2e09c32c6b..789fc342b553 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -43,7 +43,6 @@ struct at91_usbh_data {
struct gpio_desc *overcurrent_pin[AT91_MAX_USBH_PORTS];
u8 ports;   /* number of ports on root hub 
*/
u8 overcurrent_supported;
-   u8 vbus_pin_active_low[AT91_MAX_USBH_PORTS];
u8 overcurrent_status[AT91_MAX_USBH_PORTS];
u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
 };
@@ -268,8 +267,7 @@ static void ohci_at91_usb_set_power(struct at91_usbh_data 
*pdata, int port, int
if (!valid_port(port))
return;
 
-   gpiod_set_value(pdata->vbus_pin[port],
-   pdata->vbus_pin_active_low[port] ^ enable);
+   gpiod_set_value(pdata->vbus_pin[port], enable);
 }
 
 static int ohci_at91_usb_get_power(struct at91_usbh_data *pdata, int port)
@@ -277,8 +275,7 @@ static int ohci_at91_usb_get_power(struct at91_usbh_data 
*pdata, int port)
if (!valid_port(port))
return -EINVAL;
 
-   return gpiod_get_value(pdata->vbus_pin[port]) ^
-  pdata->vbus_pin_active_low[port];
+   return gpiod_get_value(pdata->vbus_pin[port]);
 }
 
 /*
@@ -535,18 +532,17 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
pdata->ports = ports;
 
at91_for_each_port(i) {
+   if (i >= pdata->ports)
+   break;
+
pdata->vbus_pin[i] = devm_gpiod_get_optional(>dev,
-"atmel,vbus-gpio",
-GPIOD_IN);
+"atmel,vbus",
+GPIOD_OUT_HIGH);
if (IS_ERR(pdata->vbus_pin[i])) {
err = PTR_ERR(pdata->vbus_pin[i]);
dev_err(>dev, "unable to claim gpio \"vbus\": 
%d\n", err);
continue;
}
-
-   pdata->vbus_pin_active_low[i] = 
gpiod_get_value(pdata->vbus_pin[i]);
-
-   ohci_at91_usb_set_power(pdata, i, 1);
}
 
at91_for_each_port(i) {
@@ -555,7 +551,7 @@ static int ohci_hcd_at91_drv_probe(struct platform_device 
*pdev)
 
pdata->overcurrent_pin[i] =
devm_gpiod_get_optional(>dev,
-   "atmel,oc-gpio", GPIOD_IN);
+   "atmel,oc", GPIOD_IN);
if (IS_ERR(pdata->overcurrent_pin[i])) {
err = PTR_ERR(pdata->overcurrent_pin[i]);
dev_err(>dev, "unable to claim gpio 
\"overcurrent\": %d\n", err);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] dt-bindings: usb: atmel: fix a couple of copy-paste style typos

[Peter Rosin]

Signed-off-by: Peter Rosin <p...@axentia.se>
---
 Documentation/devicetree/bindings/usb/atmel-usb.txt | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt 
b/Documentation/devicetree/bindings/usb/atmel-usb.txt
index f4262ed60582..ad8ea56a9ed3 100644
--- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
+++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
@@ -6,9 +6,9 @@ Required properties:
  - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
used in host mode.
  - reg: Address and length of the register set for the device
- - interrupts: Should contain ehci interrupt
+ - interrupts: Should contain ohci interrupt
  - clocks: Should reference the peripheral, host and system clocks
- - clock-names: Should contains two strings
+ - clock-names: Should contain three strings
"ohci_clk" for the peripheral clock
"hclk" for the host clock
"uhpck" for the system clock
@@ -35,7 +35,7 @@ Required properties:
  - reg: Address and length of the register set for the device
  - interrupts: Should contain ehci interrupt
  - clocks: Should reference the peripheral and the UTMI clocks
- - clock-names: Should contains two strings
+ - clock-names: Should contain two strings
"ehci_clk" for the peripheral clock
"usb_clk" for the UTMI clock
 
@@ -58,7 +58,7 @@ Required properties:
  - reg: Address and length of the register set for the device
  - interrupts: Should contain macb interrupt
  - clocks: Should reference the peripheral and the AHB clocks
- - clock-names: Should contains two strings
+ - clock-names: Should contain two strings
"pclk" for the peripheral clock
"hclk" for the AHB clock
 
@@ -85,7 +85,7 @@ Required properties:
  - reg: Address and length of the register set for the device
  - interrupts: Should contain usba interrupt
  - clocks: Should reference the peripheral and host clocks
- - clock-names: Should contains two strings
+ - clock-names: Should contain two strings
"pclk" for the peripheral clock
"hclk" for the host clock
  - ep childnode: To specify the number of endpoints and their properties.
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression, was [PATCH 4/4] USB: host: ohci-at91: merge loops in ohci_hcd_at91_drv_probe

[Peter Rosin]

Hi Alexandre,

On 2015-12-02 19:20, Alexandre Belloni wrote:
> Hi Peter,
> 
> On 01/12/2015 at 18:17:16 +0100, Peter Rosin wrote :
>> [] (ohci_hcd_at91_overcurrent_irq) from [] 
>> (handle_irq_event_percpu+0x78/0x140)
>> [] (handle_irq_event_percpu) from [] 
>> (handle_irq_event+0x2c/0x40)
>> [] (handle_irq_event) from [] 
>> (handle_simple_irq+0x6c/0x80)
>> [] (handle_simple_irq) from [] 
>> (generic_handle_irq+0x2c/0x3c)
>> [] (generic_handle_irq) from [] 
>> (gpio_irq_handler+0xa4/0x12c)
>> [] (gpio_irq_handler) from [] 
>> (generic_handle_irq+0x2c/0x3c)
>> [] (generic_handle_irq) from [] 
>> (__handle_domain_irq+0x54/0xa8)
>> [] (__handle_domain_irq) from [] (__irq_svc+0x40/0x54)
>> [] (__irq_svc) from [] (__setup_irq+0x248/0x530)
>> [] (__setup_irq) from [] 
>> (request_threaded_irq+0xc4/0x144)
>> [] (request_threaded_irq) from [] 
>> (ohci_hcd_at91_drv_probe+0x460/0x518)
>> [] (ohci_hcd_at91_drv_probe) from [] 
>> (platform_drv_probe+0x44/0xa4)
>> [] (platform_drv_probe) from [] 
>> (driver_probe_device+0x1fc/0x43c)
>> [] (driver_probe_device) from [] 
>> (__driver_attach+0x8c/0x90)
>> [] (__driver_attach) from [] (bus_for_each_dev+0x6c/0xa0)
>> [] (bus_for_each_dev) from [] 
>> (bus_add_driver+0x1d0/0x268)
>> [] (bus_add_driver) from [] (driver_register+0x78/0xf8)
>> [] (driver_register) from [] (do_one_initcall+0xb8/0x1f0)
>> [] (do_one_initcall) from [] 
>> (kernel_init_freeable+0x138/0x1d8)
>> [] (kernel_init_freeable) from [] (kernel_init+0x8/0xe8)
>> [] (kernel_init) from [] (ret_from_fork+0x14/0x2c)
>> Code: e5916058 e1a08000 e3a04000 e2865008 (e5b50004)
>> ---[ end trace e66fbc480972ac43 ]---
>> Kernel panic - not syncing: Fatal exception in interrupt
>> ---[ end Kernel panic - not syncing: Fatal exception in interrupt
> 
> I think I understood what happens. Can you try the following patch?

Seems to work here. Excellent!

> 8<--
> From 402f8444bc92d218edc63dcc3c87459981a56c31 Mon Sep 17 00:00:00 2001
> From: Alexandre Belloni <alexandre.bell...@free-electrons.com>
> Date: Wed, 2 Dec 2015 18:49:34 +0100
> Subject: [PATCH] USB: host: ohci-at91: fix a crash in
>  ohci_hcd_at91_overcurrent_irq
> 
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>

Reported-by: Peter Rosin <p...@axentia.se>
Tested-by: Peter Rosin <p...@axentia.se>

(and bisected-by if anyone cares...)

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regression, was [PATCH 4/4] USB: host: ohci-at91: merge loops in ohci_hcd_at91_drv_probe

[Peter Rosin]

Hi!

Alexandre Belloni wrote:
> ohci_hcd_at91_drv_probe() has four at91_for_each_port. They can be merged
> into two loops without changing the driver behaviour.

Not so much, I bisected the following panic to the commit matching this patch
(e4df92279fd9e01532f65e5ba397877799ed6252).

Reverting that commit on top of 4.3 fixes things for me.

Cheers,
Peter

...
ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
ohci-atmel: OHCI Atmel driver
Unable to handle kernel NULL pointer dereference at virtual address 000c
pgd = c0004000
[000c] *pgd=
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.2.0-rc6+ #5
Hardware name: Atmel SAMA5
task: c3825ac0 ti: c3826000 task.ti: c3826000
PC is at ohci_hcd_at91_overcurrent_irq+0x18/0xe8
LR is at handle_irq_event_percpu+0x78/0x140
pc : []lr : []psr: 4193
sp : c3827d08  ip : c3000e34  fp : c3a016dc
r10: c06e8bbd  r9 : c3941880  r8 : 007f
r7 : c392be00  r6 :   r5 : 0008  r4 : 
r3 : c02f506c  r2 : c3941880  r1 : c392be00  r0 : 007f
Flags: nZcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c53c7d  Table: 20004059  DAC: 0015
Process swapper (pid: 1, stack limit = 0xc3826208)
Stack: (0xc3827d08 to 0xc3828000)
7d00:   c3a01600 c383fb10   007f c00433b0
7d20:   c3941880 c383fb10 c4856648 c383fb10 c485664c c3802460
7d40: c38a6044 c00434a4 c3941880 c00458b8 007f c0042bc4 000f c01fe4ac
7d60: ff0a0110 8000 0028   0001 c3802200 
7d80: 007f c0042bc4 c06c91d4 c0042dec c004451c 6113  c3827ddc
7da0: 6113 c0013500 c3941880 c3a01600   c3941880 007f
7dc0: c3a01600  6113  007f c3a016dc  c3827df0
7de0: c00446b4 c004451c 6113  c383fb10 c00914d0 c3a01600 0080
7e00: c3941880 c02f506c c392be00 c0044954 c392be00  c392be10 c3f77690
7e20: c3a016d0 c060d1e4  c02f5a0c c060d1e4 c392be00 004f 
7e40: 0001 0001 c3927178 c070a458 c392be10 c06d7bfc fdfb c06d7bfc
7e60:  c06b0f68  c02670ec c02670a8 c070a458 c392be10 
7e80: c06ecd78 c02655b4 c02657f4  c06ce538 c392be10 c06d7bfc c392be44
7ea0:  c06ce538 0096 c0265880  c06d7bfc c02657f4 c0263880
7ec0: c383ea4c c39168b0 c06d7bfc c39df280 c06ecca0 c0264b80 c060d318 c3a017c0
7ee0: c06d7bfc c06be860 c3a017c0 c06a1468  c0266078 c0267050 c06be860
7f00: c06be860 c00096c8 c3807280 c06fc48c c3839100 c04d9470  
7f20: 4140 c00dff9c  0096 c3ffca15 c0031e54  c06b0f44
7f40: c0612f0c c3ffcab1 0006 0006   c06b84b4 0006
7f60: c06b0f60 c06f4140 c06f4140 c06b0f68  c068bda4 0006 0006
7f80:  c068b5b0  c04d06ec    
7fa0:  c04d06f4  c000ff68    
7fc0:        
7fe0:     0013  8aaa 
[] (ohci_hcd_at91_overcurrent_irq) from [] 
(handle_irq_event_percpu+0x78/0x140)
[] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x2c/0x40)
[] (handle_irq_event) from [] (handle_simple_irq+0x6c/0x80)
[] (handle_simple_irq) from [] 
(generic_handle_irq+0x2c/0x3c)
[] (generic_handle_irq) from [] 
(gpio_irq_handler+0xa4/0x12c)
[] (gpio_irq_handler) from [] (generic_handle_irq+0x2c/0x3c)
[] (generic_handle_irq) from [] 
(__handle_domain_irq+0x54/0xa8)
[] (__handle_domain_irq) from [] (__irq_svc+0x40/0x54)
[] (__irq_svc) from [] (__setup_irq+0x248/0x530)
[] (__setup_irq) from [] (request_threaded_irq+0xc4/0x144)
[] (request_threaded_irq) from [] 
(ohci_hcd_at91_drv_probe+0x460/0x518)
[] (ohci_hcd_at91_drv_probe) from [] 
(platform_drv_probe+0x44/0xa4)
[] (platform_drv_probe) from [] 
(driver_probe_device+0x1fc/0x43c)
[] (driver_probe_device) from [] (__driver_attach+0x8c/0x90)
[] (__driver_attach) from [] (bus_for_each_dev+0x6c/0xa0)
[] (bus_for_each_dev) from [] (bus_add_driver+0x1d0/0x268)
[] (bus_add_driver) from [] (driver_register+0x78/0xf8)
[] (driver_register) from [] (do_one_initcall+0xb8/0x1f0)
[] (do_one_initcall) from [] 
(kernel_init_freeable+0x138/0x1d8)
[] (kernel_init_freeable) from [] (kernel_init+0x8/0xe8)
[] (kernel_init) from [] (ret_from_fork+0x14/0x2c)
Code: e5916058 e1a08000 e3a04000 e2865008 (e5b50004)
---[ end trace e66fbc480972ac43 ]---
Kernel panic - not syncing: Fatal exception in interrupt
---[ end Kernel panic - not syncing: Fatal exception in interrupt
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html