Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
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 = &data[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 = &data[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?
> > > + 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;
> > > + struct i2c_msg msgs[] = {
> > > + {
> > > + .addr = client->addr,
> > > + .flags = 0x0,
> > > + }
> > > + };
> > > + int status;
> > > +
> > > + buf = kzalloc(len + sizeof(rab), GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + put_unaligned_le16(rab, buf);
> > > + memcpy(buf + sizeof(rab), data, len);
> > > +
> > > + msgs[0].len = len + sizeof(rab);
> > > + msgs[0].buf = buf;
> > > +
> > > + status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > + if (status < 0) {
> > > + dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > > + kfree(buf);
> > > + return status;
> > > + }
> > > +
> > > + kfree(buf);
> > > + return 0;
> > > +}
> > > +
> > > +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
> > > + unsigned int count = 10;
> > > + u8 data;
> > > + int status;
> > > +
> > > + data = CCGX_RAB_UCSI_CONTROL_STOP;
> > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data,
> > sizeof(data));
> > > + if (status < 0)
> > > + return status;
> > > +
> > > + data = CCGX_RAB_UCSI_CONTROL_START;
> > > + status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data,
> > sizeof(data));
> > > + if (status < 0)
> > > + return status;
> > > +
> > > + /*
> > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi
> > control
> > > + * register write will push response which must be cleared.
> > > + */
> > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
> > > + if (status < 0)
> > > + return status;
> > > + do {
> > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,
> > sizeof(data));
> > > + if (status < 0)
> > > + return status;
> > > +
> > > + usleep_range(1, 11000);
> > > +
> > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> > sizeof(data));
Re: EPROTO when USB 3 GbE adapters are under load
On 25.10.2018 06:37, Hao Wei Tee wrote: Hi, There are multiple reports[1][2][3] (more elsewhere on the internet) of USB 3 GbE adapters throwing EPROTO errors on USB transfer especially when the devices are under load. Both of the two common chipsets (Realtek RTL8153 (r8152[4]) and Asix AX88179 (ax88179_178a[5])) seem to exhibit this behaviour. [1]: https://bugzilla.kernel.org/show_bug.cgi?id=75381 [2]: https://bugzilla.kernel.org/show_bug.cgi?id=196747 [3]: https://bugzilla.kernel.org/show_bug.cgi?id=198931 [4]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/r8152.c [5]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/usb/ax88179_178a.c I'm trying to figure out why this happens (while it doesn't seem to happen on other OSes, but I'm not sure). I think it's unlikely that both drivers are buggy, so perhaps it is something to do with the USB stack instead of the device drivers. It wouldn't be surprising if both devices actually don't adhere to the USB specs properly and other OSes are just more tolerant of that (?) but that is just conjecture on my part. Does anyone have any ideas? Reproducing the issue with a recent kernel with xhci traces enabled should show the reason for EPROTO error. Add xhci traces before triggering the issue with: mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable after issue is triggered save and send the trace at /sys/kernel/debug/tracing/trace Note that it might be huge -Mathias
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
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 = &data[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 = &data[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 v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Peter, > > 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... OK, thanks for the explanation. I think I'm repeating these questions. You guys already went through this, so sorry for the noise. Thanks, -- heikki
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Wed, Oct 24, 2018 at 05:43:27PM +, Ajay Gupta wrote:
> Hi Andy
>
> > > > > 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.
> >
> > > > > + /*
> > > > > + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi
> > > > control
> > > > > + * register write will push response which must be cleared.
> > > > > + */
> > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
> > > > > + if (status < 0)
> > > > > + return status;
> >
> > (1)
> >
> > > > > + do {
> > > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,
> > > > sizeof(data));
> > > > > + if (status < 0)
> > > > > + return status;
> >
> > (2)
> >
> > > > > +
> > > > > + usleep_range(1, 11000);
> > > > > +
> > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> > > > sizeof(data));
> > > > > + if (status < 0)
> > > > > + return status;
> > > > > + } while ((data != 0x00) && count--);
> > > >
> > > > What's the significance of that count?
> > > It is like a retry count to clear interrupt status.
> > >
> > > > Shouldn't you return -ETIMEDOUT if count == 0?
> > > Yes. Good catch. Does the below fix looks ok?
> >
> > At least for me looks OK (I dunno why I missed that what Heikki found
> > recently).
> > Nevertheless, I have one more question about (1) and (2) above.
> >
> > Is it necessary to do one more read before do-while loop?
> Yes, we need to read to get interrupt status bits and then write
> them back to clear the status.
Ah, I see, but why you not reorganize it to put this into do-while loop?
do {
read
write
check for data
sleep
} while (--count);
Also note predecrement.
--
With Best Regards,
Andy Shevchenko
Re: EPROTO when USB 3 GbE adapters are under load
On 25/10/18 4:45 PM, Mathias Nyman wrote: Reproducing the issue with a recent kernel with xhci traces enabled should show the reason for EPROTO error. Add xhci traces before triggering the issue with: mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable after issue is triggered save and send the trace at /sys/kernel/debug/tracing/trace Note that it might be huge Thanks for the suggestion. Here[1] is (part of) the trace starting about 250 lines before the EPROTO happens. [1]: https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace The first error happens at line 243 (timestamp 8144.248398) coinciding with the start of errors spewed into dmesg: [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 ... It doesn't seem to point to anything in particular, but I'm not really familiar with USB. I'll do some digging in any case... Thanks! -- Hao Wei
Re: Set reference clock for onboard hub in DT
Hi Frieder,
On Thu, Sep 20, 2018 at 10:52 AM Frieder Schrempf
wrote:
>
> Hi,
>
> I have a question concerning the setup for a board with an onboard USB
> hub. The SoC (i.MX6S) is expected to provide a 12 MHz clock on one of
> the clock output pins as a reference for the USB hub.
>
> Now I was looking for a way to configure this in the DT, which works
> fine up to the point, that there doesn't seem to be a way to set a
> reference clock for a generic USB device.
>
> Would it make sense to implement the enabling/disabling of the clock in
> the generic USB device driver?
> It doesn't seem right to write a separate driver for the hub only to
> turn the clock on and off.
>
> My intended DT setup would look something like this:
>
> &usbh1 {
> vbus-supply = <®_usb_h1_vbus>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_usbh1>;
> dr_mode = "host";
> status = "okay";
> #address-cells = <1>;
> #size-cells = <0>;
>
> hub@1 {
> compatible = "usb4b4,6570";
> reg = <1>;
> clocks = <&clks IMX6QDL_CLK_CKO>;
> assigned-clocks = <&clks IMX6QDL_CLK_CKO>,
> <&clks IMX6QDL_CLK_CKO2_SEL>,
> <&clks IMX6QDL_CLK_CKO2>;
> assigned-clock-parents = <&clks IMX6QDL_CLK_CKO2>,
> <&clks IMX6QDL_CLK_OSC>,
> <&clks IMX6QDL_CLK_CKO2_PODF>;
> assigned-clock-rates = <0 0 1200>;
> }
> };
Peter Chen has tried to implement such support:
https://lkml.org/lkml/2017/6/21/88
However it still needs some work so that it can be accepted in mainline.
Re: EPROTO when USB 3 GbE adapters are under load
On 25.10.2018 12:52, Hao Wei Tee wrote: On 25/10/18 4:45 PM, Mathias Nyman wrote: Reproducing the issue with a recent kernel with xhci traces enabled should show the reason for EPROTO error. Add xhci traces before triggering the issue with: mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable after issue is triggered save and send the trace at /sys/kernel/debug/tracing/trace Note that it might be huge Thanks for the suggestion. Here[1] is (part of) the trace starting about 250 lines before the EPROTO happens. [1]: https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace The first error happens at line 243 (timestamp 8144.248398) coinciding with the start of errors spewed into dmesg: [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 Thanks, xHC controller reports that there was a transaction error on one of the bulk TRBs. The transaction error causes the endpoint to halt (host side halt only). Xhci driver resets the host side endpoint to recover from the halt, then returns the broken URB (TRB) with -EPROTO status, and then moves past this TRB. Interesting thing here is that each TRB in the queue after the transaction error also triggers a transaction error. This might be a data toggle/sequence number sync issue. The host side endpoint reset clears the host side sequence number, and host expects device side endpoint to be reset and sequence to be cleared as well as a result of returning -EPROTO. If I remember correctly xhci driver does not wait for device side endpoint to be reset, so if there are TRBs in the queue they will be transferred, with a cleared sequence number out of sync with the device side. There is a patch in usb-next that might help. f8f80be xhci: Use soft retry to recover faster from transaction errors It soft resets the halted host side endpoint, clears the halt without clearing the sequence number. -Mathias
RE: ehci frame index goes backwards?
From: Daniel Goertzen > Sent: 24 October 2018 17:47 > > I am developing a custom USB device that makes use of isochronous IN > transfers. It works fine from my laptop (xHCI) however on an embedded > target (vortex86dx3, EHCI) I am seeing mysterious EFBIG errors. After > adding lots of debug to ehci-sched I discovered that the EHCI register > "FRINDEX" which is supposed to increase monotonically, is actually > taking a jump backwards: What happens if you read the register twice - do you get the same value. It might be that the hardware is failing to synchronise the counter to the host bus clock and generates garbage during an increment. Such errors can be a complete mix of the old and new values, not just a 'carry chain' error. If the two reads give different values a third one should be correct (even if it differs from the second one). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: ehci frame index goes backwards?
I did test double reads and always saw the same value both times. Another piece of info: I tested this software on an older vortex86dx and saw the same issue. It is plausible that a vortex86dx and vortex86dx3 could have the same flawed EHCI. Or perhaps there's some other systemic issue that I'm not seeing. On Thu, Oct 25, 2018 at 10:30 AM David Laight wrote: > > From: Daniel Goertzen > > Sent: 24 October 2018 17:47 > > > > I am developing a custom USB device that makes use of isochronous IN > > transfers. It works fine from my laptop (xHCI) however on an embedded > > target (vortex86dx3, EHCI) I am seeing mysterious EFBIG errors. After > > adding lots of debug to ehci-sched I discovered that the EHCI register > > "FRINDEX" which is supposed to increase monotonically, is actually > > taking a jump backwards: > > What happens if you read the register twice - do you get the same value. > It might be that the hardware is failing to synchronise the counter > to the host bus clock and generates garbage during an increment. > Such errors can be a complete mix of the old and new values, not just > a 'carry chain' error. > If the two reads give different values a third one should be correct > (even if it differs from the second one). > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales)
Re: EPROTO when USB 3 GbE adapters are under load
On Thu, 25 Oct 2018, Mathias Nyman wrote: > On 25.10.2018 12:52, Hao Wei Tee wrote: > > On 25/10/18 4:45 PM, Mathias Nyman wrote: > >> Reproducing the issue with a recent kernel with xhci traces enabled should > >> show the reason for EPROTO error. > >> > >> Add xhci traces before triggering the issue with: > >> > >> mount -t debugfs none /sys/kernel/debug > >> echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb > >> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable > >> > >> after issue is triggered save and send the trace at > >> /sys/kernel/debug/tracing/trace > >> Note that it might be huge > > > > Thanks for the suggestion. > > > > Here[1] is (part of) the trace starting about 250 lines before the EPROTO > > happens. > > > > [1]: > > https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace > > > > The first error happens at line 243 (timestamp 8144.248398) coinciding with > > the start of errors spewed into dmesg: > > > > [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > > [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > > [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > > [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 > > Thanks, > xHC controller reports that there was a transaction error on one of the bulk > TRBs. > > The transaction error causes the endpoint to halt (host side halt only). > Xhci driver resets the host side endpoint to recover from the halt, > then returns the broken URB (TRB) with -EPROTO status, and then moves past > this TRB. The host side of the endpoint should remain stopped until after the URB's completion routine has had a chance to carry out error recovery. Doesn't this imply the xHCI driver shouldn't reset the host-side endpoint until after the giveback call returns? > Interesting thing here is that each TRB in the queue after the transaction > error > also triggers a transaction error. > > This might be a data toggle/sequence number sync issue. It's more likely to be a problem on the device side. Data toggle or sequence number issues tend to be self-repairing (albeit with some data loss) after a little while. > The host side endpoint reset clears the host side sequence number, > and host expects device side endpoint to be reset and sequence to be cleared > as well > as a result of returning -EPROTO. > If I remember correctly xhci driver does not wait for device side endpoint to > be reset, > so if there are TRBs in the queue they will be transferred, with a cleared > sequence number > out of sync with the device side. That's why it's important to wait until after the higher-layer driver has had a chance to unlink the URBs that may be in the endpoint queue. The driver may even want to reset the device. > There is a patch in usb-next that might help. > f8f80be xhci: Use soft retry to recover faster from transaction errors > > It soft resets the halted host side endpoint, clears the halt without > clearing the sequence number. > > -Mathias Alan Stern
RE: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Andy,
> > > > > > 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.
> > >
> > > > > > + /*
> > > > > > +* Flush CCGx RESPONSE queue by acking interrupts. Above
> > > > > > +ucsi
> > > > > control
> > > > > > +* register write will push response which must be cleared.
> > > > > > +*/
> > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> sizeof(data));
> > > > > > + if (status < 0)
> > > > > > + return status;
> > >
> > > (1)
> > >
> > > > > > + do {
> > > > > > + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,
> > > > > sizeof(data));
> > > > > > + if (status < 0)
> > > > > > + return status;
> > >
> > > (2)
> > >
> > > > > > +
> > > > > > + usleep_range(1, 11000);
> > > > > > +
> > > > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> > > > > sizeof(data));
> > > > > > + if (status < 0)
> > > > > > + return status;
> > > > > > + } while ((data != 0x00) && count--);
> > > > >
> > > > > What's the significance of that count?
> > > > It is like a retry count to clear interrupt status.
> > > >
> > > > > Shouldn't you return -ETIMEDOUT if count == 0?
> > > > Yes. Good catch. Does the below fix looks ok?
> > >
> > > At least for me looks OK (I dunno why I missed that what Heikki
> > > found recently).
> > > Nevertheless, I have one more question about (1) and (2) above.
> > >
> > > Is it necessary to do one more read before do-while loop?
> > Yes, we need to read to get interrupt status bits and then write them
> > back to clear the status.
>
> Ah, I see, but why you not reorganize it to put this into do-while loop?
>
> do {
> read
> write
> check for data
> sleep
> } while (--count);
>
> Also note predecrement.
Sure, will do that way!
Thanks
Ajay
--nvpublic
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
RE: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Heikki, > > > 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... > > OK, thanks for the explanation. > > I think I'm repeating these questions. You guys already went through this, so > sorry for the noise. I will go ahead and post new set. Thank you Peter for explanation! Thanks Ajay --nvpublic > > > Thanks, > > -- > heikki
RE: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
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, &data,
> > sizeof(data));
> > if (status < 0)
> > return status;
> >
> > usleep_range(1, 11000);
> >
> > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> > 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).
> > 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);
Thanks
Ajay
-nvpublic
> > return -ETIMEDOUT;
> >
> > > Something like:
> > >
> > > ...
> > > while (count--)
> > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> sizeof(data));
> > > if (status < 0)
> > > return status;
> > > if (!data)
> > > return 0;
> > > }
> > >
> > > return -ETIMEDOUT;
> > >
> > > Or does the count of 10 have some specific meaning?
> > >
> > > > +}
> > > > +
> > > > +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
> > > > + u8 *ppm = (u8 *)uc->ppm.data;
> > > > + int status;
> > > > + u16 rab;
> > > > +
> > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
> > > message_out));
> > > > + status = ccg_write(uc, rab, ppm +
> > > > + offsetof(struct ucsi_data, message_out),
> > > > + sizeof(uc->ppm.data->message_out));
> > > > + if (status < 0)
> > > > + return status;
> > > > +
> > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
> > > > ctrl));
> > > > + return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data,
> > > > ctrl),
> > > > +sizeof(uc->ppm.data->ctrl));
> > > > +}
> > > > +
> > > > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
> > > > + u8 *ppm = (u8 *)uc->ppm.data;
> > > > + int status;
> > > > + u16 rab;
> > > > +
> > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci));
> > > > + status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data,
> > > > cci),
> > > > + sizeof(uc->ppm.data->cci));
> > > > + if (status < 0)
> > > > + return status;
> > > > +
> > > > + rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data,
> > > message_in));
> > > > + return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data,
> > > > message_in),
> > > > + sizeof(uc->ppm.data->message_in));
> > > > +}
> > > > +
> > > > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
> > > > + int status;
> > > > + unsigned char data;
> > > > +
> > > > + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
> > > > + if (status < 0)
> > > > + return status;
> > > > +
> > > > + return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data)); }
> > > > +
> > > > +static int ucsi_ccg_sync(struct ucsi_ppm *ppm) {
> > > > + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> > > > + int status;
> > > > +
> > > > + status = ucsi_ccg_recv_data(uc);
> > > > + if (status < 0)
> > > > + return status;
> > > > +
> > > > + /* ack interrupt to allow next command to run */
> > > > + return ucsi_ccg_ack_interrupt(uc); }
> > > > +
> > > > +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
> > > > +*ctrl) {
> > > > + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> > > > +
> > > > + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> > > > + return ucsi_ccg_send_data(uc);
> > > > +}
> > > > +
> > > > +static irqreturn_t ccg_irq_handler(int irq, void *data) {
> > > > + struct ucsi_ccg *uc = data;
> > > > +
> > > > + ucsi_notify(uc->ucsi);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int ucsi_ccg_probe(struct i2c_client *client,
> > > > + const struct i2c_device_id *id) {
> > > > + struct device *dev = &client->dev;
> > > > + struct ucsi_ccg *uc;
> > > > + int status;
> > > > + u16 rab;
> > > > +
> > > > + uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
> > > > + if (!uc)
> > > > + return -ENOMEM;
> > > > +
> > > > + uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucs
[PATCH v14 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
From: Ajay Gupta 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 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 Changes from v12 -> v13 Fixed review comments from Peter - Added comments on 4 byte read limitation - Added I2C_AC_COMB* flags Changes from v13 -> v14 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 | 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 ..31884d2b2eb5 --- /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 d63705445bf8..83df14ca45b4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6800,6 +6800,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 451d4ae50e66..eed827b44068 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 18b26af82b1c..d499813df038 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 ..744f5e42636b --- /dev/null +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c @@ -0,0 +1,368 @@ +// 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 +
[PATCH v14 2/2] usb: typec: ucsi: add support for Cypress CCGx
From: Ajay Gupta
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
---
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
Changes from v10 -> v11
Fixed review comments from Peter
- Combined two writes into one
- Used offsetof() for ucsi_data struct
Changes from v11 -> v12
- some cleanup
Changes from v12 -> v13
- changed "u8 buf[1]" -> "u8 data"
Changes from v13 -> v14
- Fixed commend from Heikki and Andy
- Added i2c adapters quirks check in ccg_read
- Removed "irq" field from struct ucsi_ccg
- Reorganize do {} while (); loop
drivers/usb/typec/ucsi/Kconfig| 10 +
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_ccg.c | 307 ++
3 files changed, 319 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 e36d6c73c4a4..78118883f96c 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 7afbea512207..2f4900b26210 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 ..f32e3ebcee22
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,307 @@
+// 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;
+};
+
+#define CCGX_RAB_INTR_REG 0x06
+#define CCGX_RAB_UCSI_CONTROL 0x39
+#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
+
+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[] = {
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .len= sizeof(buf),
+ .buf= buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .buf= data,
+ },
+ };
+ u32 rlen, rem_len = len, max_read_len = len;
+ int status;
+
+ /* 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 = &data[len - rem_len];
+ 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));
+ if (status < 0) {
+ dev
[PATCH v14 0/2] Add support for USB Type-C interface on latest NVIDIA GPU
From: Ajay Gupta Hi Heikki and Wolfram, These two changes add support for USB Type-C interface on latest NVIDIA GPU card. The Type-C controller used is Cypress CCGx and is over I2C interface. I2C host controller has known limitation of sending STOP after every read. Since each read can be of 4 byte maximum length so there is a limit of 4 byte read. This is mentioned in adapter quirks as "max_read_len = 4" I2C host controller is mainly used for "write-then-read" or "write" messages so added the flag I2C_AQ_COMB_WRITE_THEN_READ in adapter quirks. PATCH[2/2] on ucsi driver now have added logic to check i2c adapter quirks and issues i2c read transfer based on max_read_len quirk settings. This will make sure the read limitation is not affecting I2C host which do not have such limitation. I think the patches should through usb tree because the main functionality is usb Type-C. Thanks Ajay Ajay Gupta (2): i2c: buses: add i2c bus driver for NVIDIA GPU usb: typec: ucsi: add support for Cypress CCGx 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 drivers/usb/typec/ucsi/Kconfig | 10 + drivers/usb/typec/ucsi/Makefile | 2 + drivers/usb/typec/ucsi/ucsi_ccg.c | 307 8 files changed, 722 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c -- 2.17.1
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
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, &data,
>>> sizeof(data));
>>> if (status < 0)
>>> return status;
>>>
>>> usleep_range(1, 11000);
>>>
>>> status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
>>> 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: [RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node
On Wed, Oct 24, 2018 at 08:07:09PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 10/24/2018 06:05 PM, Heikki Krogerus wrote:
>
> > When the connections are defined in firmware, struct
> > device_connection will have the fwnode member pointing to
> > the device node (struct fwnode_handle) of the requested
> > device, and the endpoint will not be used at all in that
> > case.
> >
> > Signed-off-by: Heikki Krogerus
> > ---
> > drivers/usb/typec/class.c | 19 ---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 5db0593ca0bd..fe6f3a932a88 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct altmode
> > *altmode)
> > put_device(&adev->dev);
> > }
> >
> > -static int __typec_port_match(struct device *dev, const void *name)
> > +static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > + return dev_name(fwnode) == fwnode;
>
>dev_name(dev), maybe?
Yes.
thanks,
--
heikki
Re: [RFC PATCH 1/5] driver core: Add fwnode member to struct device_connection
On Wed, Oct 24, 2018 at 08:33:53AM -0700, Randy Dunlap wrote:
> On 10/24/18 8:05 AM, Heikki Krogerus wrote:
> > This will prepare the device connection API for connections
> > described in firmware.
> >
> > Signed-off-by: Heikki Krogerus
> > ---
> > include/linux/device.h | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 90224e75ade4..a964a0d614fa 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -753,11 +753,17 @@ struct device_dma_parameters {
> >
> > /**
> > * struct device_connection - Device Connection Descriptor
> > + * @fwnode: The device node of the connected device
> > * @endpoint: The names of the two devices connected together
> > * @id: Unique identifier for the connection
> > * @list: List head, private, for internal use only
> > + *
> > + * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
> > + * platform firmware defines the connection. When the connection is
> > registeded
>
> for your next version:)
> registered
OK.
thanks,
--
heikki
Re: [RFC PATCH 4/5] usb: typec: Find the ports by also matching against the device node
On Thu, Oct 25, 2018 at 10:27:20AM +0300, Heikki Krogerus wrote:
> On Wed, Oct 24, 2018 at 08:07:09PM +0300, Sergei Shtylyov wrote:
> > Hello!
> >
> > On 10/24/2018 06:05 PM, Heikki Krogerus wrote:
> >
> > > When the connections are defined in firmware, struct
> > > device_connection will have the fwnode member pointing to
> > > the device node (struct fwnode_handle) of the requested
> > > device, and the endpoint will not be used at all in that
> > > case.
> > >
> > > Signed-off-by: Heikki Krogerus
> > > ---
> > > drivers/usb/typec/class.c | 19 ---
> > > 1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index 5db0593ca0bd..fe6f3a932a88 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -204,15 +204,28 @@ static void typec_altmode_put_partner(struct
> > > altmode *altmode)
> > > put_device(&adev->dev);
> > > }
> > >
> > > -static int __typec_port_match(struct device *dev, const void *name)
> > > +static int typec_port_fwnode_match(struct device *dev, const void
> > > *fwnode)
> > > +{
> > > + return dev_name(fwnode) == fwnode;
> >
> >dev_name(dev), maybe?
Actually:
dev_fwnode(dev) == fwnode;
Cheers,
--
heikki
USB-C device hotplug issue
Hi all, I have a question regarding the usb hub driver (drivers/usb/core/hub.c). There is the following scenario. I am using the Lenovo T580 device with the Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the docking station there is one device which will not be recognized (SanDisk Ultra USB-C Flash Drive, https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An other usb-c devices (SanDisk Extreme 900 SSD, https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have that much USB-C devices available, so there is one device working and the other one not. I made some analysis of the situation where I attached the SanDisk Ultra USB-C Flash Drive. I added some debug logs in drivers/usb/core/hub.c in port_event and hub_port_reset and activated all dynamic debug prints in hub.c. The output is the following: [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! [ 724.110789] usb 4-1-port1: link state change [ 724.114953] usb 4-1-port1: do warm reset [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg evt 0002 [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! The same situation with SanDisk Extreme 900 SSD: [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg evt 0002 [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using xhci_hcd [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, SerialNumber=1 [ 863.840018] usb 4-1.1: Product: EXTREME900 [ 863.840022] usb 4-1.1: Manufacturer: SanDisk [ 863.840026] usb 4-1.1: SerialNumber: 1019DF56 So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if I attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? I read https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. In this test usb core was bootet wird autosuspend=-1.Additionally the /power/pm_qos_no_power_off sysfs variable is always set to 1. Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C Flash Drive. I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION bit is cleared at successful warm usb 3 reset in hub_port_reset. In case of the Ultra USB-C Flash Drive at first the link state change is detected. Directly after executing the actual hub_port_reset the USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code will set this bit to 0. At the next run the bit remains 0 and the connect_change flag in port_event will not be set. That means the USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into account. That’s why this device will not be detected correctly. If I modify the code in this way that I did’t clear the USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this flag is still set during the next run. This makes sure that the connect_change flag in port_event is set and the usb device is detected correctly. Is this code change correct or will it break other use cases? Are there any other ways to make the kernel detect that usb device at the docking station? Please refer to the output of the modified usb_hub_reset code: [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg evt 0002 [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! [ 121.566810] usb 4-1-port1: link state change [ 121.573481] usb 4-1-port1: do warm reset [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms [ 121.881091] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 121.881454] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 121.933109] hub 4-1:1.0: state 7 ports 4 chg evt 0002 [ 121.933407] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! [
Re: USB-C device hotplug issue
On Thu, Oct 25, 2018 at 02:20:50PM +0200, Dennis Wassenberg wrote: > The code change for Kernel 4.14 look as follows: 4.14 is a year old now, can you try 4.19 to see if it works better? Lots of good xhci fixes and features have been added since then. thanks, greg k-h
Re: USB-C device hotplug issue
Hi, Using kernel 4.19 its exactly the same behavior. Best regards, Dennis On 25.10.18 14:28, Greg Kroah-Hartman wrote: > On Thu, Oct 25, 2018 at 02:20:50PM +0200, Dennis Wassenberg wrote: >> The code change for Kernel 4.14 look as follows: > > 4.14 is a year old now, can you try 4.19 to see if it works better? > Lots of good xhci fixes and features have been added since then. > > thanks, > > greg k-h >
Re: USB-C device hotplug issue
On Thu, 25 Oct 2018, Dennis Wassenberg wrote: > Hi all, > > I have a question regarding the usb hub driver (drivers/usb/core/hub.c). > > There is the following scenario. I am using the Lenovo T580 device with the > Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the > docking station there is one device which will not be recognized (SanDisk > Ultra USB-C Flash Drive, > https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An > other usb-c devices (SanDisk Extreme 900 SSD, > https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have > that much USB-C devices available, so there is one device working and the > other one not. > > I made some analysis of the situation where I attached the SanDisk Ultra > USB-C Flash Drive. > I added some debug logs in drivers/usb/core/hub.c in port_event and > hub_port_reset and activated all dynamic debug prints in hub.c. The output is > the following: > > [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: > 0x40! > [ 724.110789] usb 4-1-port1: link state change > [ 724.114953] usb 4-1-port1: do warm reset > [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms > [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms > [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg evt 0002 > [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: > 0x0! > > The same situation with SanDisk Extreme 900 SSD: > > [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg evt 0002 > [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: > 0x1! > [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s > [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 > [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using > xhci_hcd > [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 > [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 > [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, > SerialNumber=1 > [ 863.840018] usb 4-1.1: Product: EXTREME900 > [ 863.840022] usb 4-1.1: Manufacturer: SanDisk > [ 863.840026] usb 4-1.1: SerialNumber: 1019DF56 > > So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if I > attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme > 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? > > I read > https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. > In this test usb core was bootet wird autosuspend=-1.Additionally the > /power/pm_qos_no_power_off sysfs variable is always set to 1. > Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C > Flash Drive. > > I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION bit > is cleared at successful warm usb 3 reset in hub_port_reset. > In case of the Ultra USB-C Flash Drive at first the link state change is > detected. Directly after executing the actual hub_port_reset the > USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code > will set this bit to 0. At the next run the bit remains 0 and the > connect_change flag in port_event will not be set. That means the > USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into > account. That’s why this device will not be detected correctly. > > If I modify the code in this way that I did’t clear the > USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this flag > is still set during the next run. This makes sure that the connect_change > flag in port_event is set and the usb device is detected correctly. > Is this code change correct or will it break other use cases? Are there any > other ways to make the kernel detect that usb device at the docking station? > > Please refer to the output of the modified usb_hub_reset code: > > [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg evt 0002 > [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: > 0x40! > [ 121.566810] usb 4-1-port1: link state change > [ 121.573481] usb 4-1-port1: do warm reset > [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms > [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms > [ 121.881091] usb 4-1-port1: XXX: hub_port_reset (before clear > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 121.881454] usb 4-1-port
Re: [PATCH] usbip: tools: fix atoi() on non-null terminated string
Hi Colin,
On 10/16/2018 12:03 PM, Colin King wrote:
> From: Colin Ian King
>
> Currently the call to atoi is being passed a single char string
> that is not null terminated, so there is a potential read overrun
> along the stack when parsing for an integer value. Fix this by
> instead using a 2 char string that is initialized to all zeros
> to ensure that a 1 char read into the string is always terminated
> with a \0.
>
> Detected by cppcheck:
> "Invalid atoi() argument nr 1. A nul-terminated string is required."
>
> Fixes: 3391ba0e2792 ("usbip: tools: Extract generic code to be shared with
> vudc backend")
> Signed-off-by: Colin Ian King
> ---
> tools/usb/usbip/libsrc/usbip_host_common.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c
> b/tools/usb/usbip/libsrc/usbip_host_common.c
> index dc93fadbee96..d79c7581b175 100644
> --- a/tools/usb/usbip/libsrc/usbip_host_common.c
> +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
> @@ -43,7 +43,7 @@ static int32_t read_attr_usbip_status(struct
> usbip_usb_device *udev)
> int size;
> int fd;
> int length;
> - char status;
> + char status[2] = { 0 }> int value = 0;
>
> size = snprintf(status_attr_path, sizeof(status_attr_path),
> @@ -61,14 +61,14 @@ static int32_t read_attr_usbip_status(struct
> usbip_usb_device *udev)
> return -1;
> }
>
> - length = read(fd, &status, 1);
> + length = read(fd, status, 1);
> if (length < 0) {
> err("error reading attribute %s", status_attr_path);
> close(fd);
> return -1;
> }
>
> - value = atoi(&status);
> + value = atoi(status);
>
> return value;
> }
>
Thanks for the patch. Looks good to me.
Acked-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH 2/3] net: ethernet: Remove dummy runtime PM callbacks from Renesas drivers
On Wed, Oct 24, 2018 at 04:51:33PM +0300, Jarkko Nikula wrote:
> Platform drivers don't need dummy runtime PM callbacks that just return
> success in order to have runtime PM happening. This has changed since
> following commits:
>
> 05aa55dddb9e ("PM / Runtime: Lenient generic runtime pm callbacks")
> 543f2503a956 ("PM / platform_bus: Allow runtime PM by default")
> 8b313a38ecff ("PM / Platform: Use generic runtime PM callbacks directly")
>
> Signed-off-by: Jarkko Nikula
Acked-by: Wolfram Sang
signature.asc
Description: PGP signature
Re: [PATCH 3/3] usb: renesas_usbhs: Remove dummy runtime PM callbacks
On Wed, Oct 24, 2018 at 04:51:34PM +0300, Jarkko Nikula wrote:
> Platform drivers don't need dummy runtime PM callbacks that just return
> success in order to have runtime PM happening. This has changed since
> following commits:
>
> 05aa55dddb9e ("PM / Runtime: Lenient generic runtime pm callbacks")
> 543f2503a956 ("PM / platform_bus: Allow runtime PM by default")
> 8b313a38ecff ("PM / Platform: Use generic runtime PM callbacks directly")
>
> Signed-off-by: Jarkko Nikula
Acked-by: Wolfram Sang
> static const struct dev_pm_ops usbhsc_pm_ops = {
> .suspend= usbhsc_suspend,
> .resume = usbhsc_resume,
Unrelated to this patch, but I wonder right now: is there a reason not
to use SET_SYSTEM_SLEEP_PM_OPS here? Shimoda-san?
signature.asc
Description: PGP signature
RE: [PATCH 3/3] usb: renesas_usbhs: Remove dummy runtime PM callbacks
Hi Wolfram-san,
> From: Wolfram Sang, Sent: Friday, October 26, 2018 7:58 AM
> > static const struct dev_pm_ops usbhsc_pm_ops = {
> > .suspend= usbhsc_suspend,
> > .resume = usbhsc_resume,
>
> Unrelated to this patch, but I wonder right now: is there a reason not
> to use SET_SYSTEM_SLEEP_PM_OPS here? Shimoda-san?
I don't know why because this code is contributed from Morimoto-san.
I'm guessing this code seems to come from sh_eth.c or i2c-sh_mobile.c
and we don't have the SET_SYSTEM_SLEEP_PM_OPS macro at 2009.
Morimoto-san contributed this code at 2010, but it seems not to realize
we have such macro.
Anyway, I'll try to use SET_SYSTEM_SLEEP_PM_OPS on the renesas_usbhs driver.
Best regards,
Yoshihiro Shimoda
[PATCH] USB: quirks: Add no-lpm quirk for Raydium touchscreens
Raydium USB touchscreen fails to set config if LPM is enabled:
[2.030658] usb 1-8: New USB device found, idVendor=2386, idProduct=3119
[2.030659] usb 1-8: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[2.030660] usb 1-8: Product: Raydium Touch System
[2.030661] usb 1-8: Manufacturer: Raydium Corporation
[7.132209] usb 1-8: can't set config #1, error -110
Same behavior can be observed on 2386:3114.
Raydium claims the touchscreen supports LPM under Windows, so I used
Microsoft USB Test Tools (MUTT) [1] to check its LPM status. MUTT shows
that the LPM doesn't work under Windows, either. So let's just disable LPM
for Raydium touchscreens.
[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/usbcon/usb-test-tools
Signed-off-by: Kai-Heng Feng
---
drivers/usb/core/quirks.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 178d6c6063c0..99f525ac4b2e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -411,6 +411,11 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x2040, 0x7200), .driver_info =
USB_QUIRK_CONFIG_INTF_STRINGS },
+ /* Raydium Touchscreen */
+ { USB_DEVICE(0x2386, 0x3114), .driver_info = USB_QUIRK_NO_LPM },
+
+ { USB_DEVICE(0x2386, 0x3119), .driver_info = USB_QUIRK_NO_LPM },
+
/* DJI CineSSD */
{ USB_DEVICE(0x2ca3, 0x0031), .driver_info = USB_QUIRK_NO_LPM },
--
2.19.1
