| Message ID | 20190926100727.71117-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: typec: UCSI driver overhaul | expand |
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Thursday, September 26, 2019 3:07 AM > To: Ajay Gupta <ajayg@nvidia.com> > Cc: linux-usb@vger.kernel.org > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > Hi Ajay, > > Here's the pretty much complete rewrite of the I/O handling that I was > talking about. The first seven patches are not actually related to > this stuff, but I'm including them here because the rest of the series > is made on top of them. I'm including also that fix patch I send you > earlier. > > After this it should be easier to handle quirks. My idea how to handle > the multi-instance connector alt modes is that we "emulate" the PPM in > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. > > We can now get the connector alternate modes that the actual > controller supplies during probe - before registering the ucsi > interface - and squash all alt modes with the same SVID into one that > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES > command. Also other alt mode commands like SET_NEW_CAM can have > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should > not be any problem with that anymore. I took the changes and loaded on my GPU system and do not see altmode devices under /sys/bus/typec/devices/*. Its empty. Below error is seen "ucsi_ccg 4-0008: con1: failed to register alternate modes" ucsi_run_command() is returning -16. I will review the ccg changes and try to debug above issue. Thanks > nvpublic > > thanks, > > Heikki Krogerus (14): > usb: typec: Copy everything from struct typec_capability during > registration > usb: typec: Introduce typec_get_drvdata() > usb: typec: Separate the operations vector > usb: typec: tcpm: Start using struct typec_operations > usb: typec: tps6598x: Start using struct typec_operations > usb: typec: ucsi: Start using struct typec_operations > usb: typec: Remove the callback members from struct typec_capability > usb: typec: ucsi: ccg: Remove run_isr flag > usb: typec: ucsi: Simplified interface registration and I/O API > usb: typec: ucsi: acpi: Move to the new API > usb: typec: ucsi: ccg: Move to the new API > usb: typec: ucsi: Remove the old API > usb: typec: ucsi: Remove struct ucsi_control > usb: typec: ucsi: Remove all bit-fields > > drivers/usb/typec/class.c | 125 +++--- > drivers/usb/typec/tcpm/tcpm.c | 47 +-- > drivers/usb/typec/tps6598x.c | 49 +-- > drivers/usb/typec/ucsi/displayport.c | 26 +- > drivers/usb/typec/ucsi/trace.c | 11 - > drivers/usb/typec/ucsi/trace.h | 79 +--- > drivers/usb/typec/ucsi/ucsi.c | 592 ++++++++++++++------------- > drivers/usb/typec/ucsi/ucsi.h | 410 +++++++------------ > drivers/usb/typec/ucsi/ucsi_acpi.c | 96 ++++- > drivers/usb/typec/ucsi/ucsi_ccg.c | 214 ++++------ > include/linux/usb/typec.h | 38 +- > 11 files changed, 785 insertions(+), 902 deletions(-) > > -- > 2.23.0
On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote: > Hi Heikki, > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Sent: Thursday, September 26, 2019 3:07 AM > > To: Ajay Gupta <ajayg@nvidia.com> > > Cc: linux-usb@vger.kernel.org > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > > > Hi Ajay, > > > > Here's the pretty much complete rewrite of the I/O handling that I was > > talking about. The first seven patches are not actually related to > > this stuff, but I'm including them here because the rest of the series > > is made on top of them. I'm including also that fix patch I send you > > earlier. > > > > After this it should be easier to handle quirks. My idea how to handle > > the multi-instance connector alt modes is that we "emulate" the PPM in > > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. > > > > We can now get the connector alternate modes that the actual > > controller supplies during probe - before registering the ucsi > > interface - and squash all alt modes with the same SVID into one that > > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES > > command. Also other alt mode commands like SET_NEW_CAM can have > > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should > > not be any problem with that anymore. > I took the changes and loaded on my GPU system and do not see > altmode devices under /sys/bus/typec/devices/*. Its empty. > > Below error is seen > "ucsi_ccg 4-0008: con1: failed to register alternate modes" > > ucsi_run_command() is returning -16. > > I will review the ccg changes and try to debug above issue. There is at least on obvious bug. The number of alternate modes field is wrong. The this: diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index accf54d987ad..506d963ecc6c 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -330,7 +330,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient) command |= UCSI_GET_ALTMODE_RECIPIENT(recipient); command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num); command |= UCSI_GET_ALTMODE_OFFSET(i); - command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a time */ + command |= UCSI_GET_ALTMODE_NUM_ALTMODES(0); /* One at a time */ len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt)); if (len <= 0) return len; Alternatively you can also just drop two last patches of the series. thanks,
On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote: > Hi Heikki, > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Sent: Thursday, September 26, 2019 3:07 AM > > To: Ajay Gupta <ajayg@nvidia.com> > > Cc: linux-usb@vger.kernel.org > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > > > Hi Ajay, > > > > Here's the pretty much complete rewrite of the I/O handling that I was > > talking about. The first seven patches are not actually related to > > this stuff, but I'm including them here because the rest of the series > > is made on top of them. I'm including also that fix patch I send you > > earlier. > > > > After this it should be easier to handle quirks. My idea how to handle > > the multi-instance connector alt modes is that we "emulate" the PPM in > > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. > > > > We can now get the connector alternate modes that the actual > > controller supplies during probe - before registering the ucsi > > interface - and squash all alt modes with the same SVID into one that > > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES > > command. Also other alt mode commands like SET_NEW_CAM can have > > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should > > not be any problem with that anymore. > I took the changes and loaded on my GPU system and do not see > altmode devices under /sys/bus/typec/devices/*. Its empty. > > Below error is seen > "ucsi_ccg 4-0008: con1: failed to register alternate modes" > > ucsi_run_command() is returning -16. > > I will review the ccg changes and try to debug above issue. I tested this series with the NVIDIA GPU PCI card that we have. The controller is reporting BUSY as responce to the GET_ALTERNATE_MODES command. I added a loop to ucsi_exec_command() where I read the CCI until the there is no UCSI_CCI_BUSY bit set, and it seems to work. Here are both changes: diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index accf54d987ad..d70ee8006c34 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -102,6 +102,7 @@ static int ucsi_read_error(struct ucsi *ucsi) static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd) { + unsigned long timeout; u32 cci; int ret; @@ -109,12 +110,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd) if (ret) return ret; - ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); - if (ret) - return ret; + timeout = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS); + do { + if (time_is_before_jiffies(timeout)) + return -ETIMEDOUT; - if (cci & UCSI_CCI_BUSY) - return -EBUSY; + ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); + if (ret) + return ret; + } while (cci & UCSI_CCI_BUSY); if (!(cci & UCSI_CCI_COMMAND_COMPLETE)) return -EIO; @@ -330,7 +334,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient) command |= UCSI_GET_ALTMODE_RECIPIENT(recipient); command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num); command |= UCSI_GET_ALTMODE_OFFSET(i); - command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a time */ + len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt)); if (len <= 0) return len; Let me know if that works. thanks,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Friday, September 27, 2019 5:53 AM > To: Ajay Gupta <ajayg@nvidia.com> > Cc: linux-usb@vger.kernel.org > Subject: Re: [PATCH 00/14] usb: typec: UCSI driver overhaul > > On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote: > > Hi Heikki, > > > -----Original Message----- > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Sent: Thursday, September 26, 2019 3:07 AM > > > To: Ajay Gupta <ajayg@nvidia.com> > > > Cc: linux-usb@vger.kernel.org > > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > > > > > Hi Ajay, > > > > > > Here's the pretty much complete rewrite of the I/O handling that I > > > was talking about. The first seven patches are not actually related > > > to this stuff, but I'm including them here because the rest of the > > > series is made on top of them. I'm including also that fix patch I > > > send you earlier. > > > > > > After this it should be easier to handle quirks. My idea how to > > > handle the multi-instance connector alt modes is that we "emulate" > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not touched at > all. > > > > > > We can now get the connector alternate modes that the actual > > > controller supplies during probe - before registering the ucsi > > > interface - and squash all alt modes with the same SVID into one > > > that we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES > > > command. Also other alt mode commands like SET_NEW_CAM can have > > > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should > > > not be any problem with that anymore. > > I took the changes and loaded on my GPU system and do not see altmode > > devices under /sys/bus/typec/devices/*. Its empty. > > > > Below error is seen > > "ucsi_ccg 4-0008: con1: failed to register alternate modes" > > > > ucsi_run_command() is returning -16. > > > > I will review the ccg changes and try to debug above issue. > > I tested this series with the NVIDIA GPU PCI card that we have. The controller > is reporting BUSY as responce to the GET_ALTERNATE_MODES command. I > added a loop to ucsi_exec_command() where I read the CCI until the there is > no UCSI_CCI_BUSY bit set, and it seems to work. > > Here are both changes: > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index > accf54d987ad..d70ee8006c34 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -102,6 +102,7 @@ static int ucsi_read_error(struct ucsi *ucsi) > > static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd) { > + unsigned long timeout; > u32 cci; > int ret; > > @@ -109,12 +110,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 > cmd) > if (ret) > return ret; > > - ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); > - if (ret) > - return ret; > + timeout = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS); > + do { > + if (time_is_before_jiffies(timeout)) > + return -ETIMEDOUT; > > - if (cci & UCSI_CCI_BUSY) > - return -EBUSY; > + ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); > + if (ret) > + return ret; > + } while (cci & UCSI_CCI_BUSY); > > if (!(cci & UCSI_CCI_COMMAND_COMPLETE)) > return -EIO; > @@ -330,7 +334,7 @@ static int ucsi_register_altmodes(struct > ucsi_connector *con, u8 recipient) > command |= UCSI_GET_ALTMODE_RECIPIENT(recipient); > command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con- > >num); > command |= UCSI_GET_ALTMODE_OFFSET(i); > - command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a > time */ > + > len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt)); > if (len <= 0) > return len; > > Let me know if that works. It works now for me too. I will review rest of changes anyways and also need to update my change on altmode squashing. Thanks >nvpublic > > thanks, > > -- > heikki
Hi Heikki > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Thursday, September 26, 2019 3:07 AM > To: Ajay Gupta <ajayg@nvidia.com> > Cc: linux-usb@vger.kernel.org > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > Hi Ajay, > > Here's the pretty much complete rewrite of the I/O handling that I was > talking about. The first seven patches are not actually related to > this stuff, but I'm including them here because the rest of the series > is made on top of them. I'm including also that fix patch I send you > earlier. > > After this it should be easier to handle quirks. My idea how to handle > the multi-instance connector alt modes is that we "emulate" the PPM in > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. > > We can now get the connector alternate modes that the actual > controller supplies during probe - before registering the ucsi > interface How can ucsi_ccg.c get the connector alternate modes before registering ucsi interface? PPM reset, notification enable, etc. is done during ucsi registration. UCSI spec says: " The only commands the PPM is required to process in the "PPM Idle (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and PPM_RESE" Also, it doesn't look correct if ucsi_ccg.c has to replicate most of the stuff done in ucsi_init() of ucsi.c. Thanks > nvpublic > - and squash all alt modes with the same SVID into one that > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES > command. Also other alt mode commands like SET_NEW_CAM can have > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should > not be any problem with that anymore. > > thanks, > > Heikki Krogerus (14): > usb: typec: Copy everything from struct typec_capability during > registration > usb: typec: Introduce typec_get_drvdata() > usb: typec: Separate the operations vector > usb: typec: tcpm: Start using struct typec_operations > usb: typec: tps6598x: Start using struct typec_operations > usb: typec: ucsi: Start using struct typec_operations > usb: typec: Remove the callback members from struct typec_capability > usb: typec: ucsi: ccg: Remove run_isr flag > usb: typec: ucsi: Simplified interface registration and I/O API > usb: typec: ucsi: acpi: Move to the new API > usb: typec: ucsi: ccg: Move to the new API > usb: typec: ucsi: Remove the old API > usb: typec: ucsi: Remove struct ucsi_control > usb: typec: ucsi: Remove all bit-fields > > drivers/usb/typec/class.c | 125 +++--- > drivers/usb/typec/tcpm/tcpm.c | 47 +-- > drivers/usb/typec/tps6598x.c | 49 +-- > drivers/usb/typec/ucsi/displayport.c | 26 +- > drivers/usb/typec/ucsi/trace.c | 11 - > drivers/usb/typec/ucsi/trace.h | 79 +--- > drivers/usb/typec/ucsi/ucsi.c | 592 ++++++++++++++------------- > drivers/usb/typec/ucsi/ucsi.h | 410 +++++++------------ > drivers/usb/typec/ucsi/ucsi_acpi.c | 96 ++++- > drivers/usb/typec/ucsi/ucsi_ccg.c | 214 ++++------ > include/linux/usb/typec.h | 38 +- > 11 files changed, 785 insertions(+), 902 deletions(-) > > -- > 2.23.0
Hi Ajay, On Tue, Oct 01, 2019 at 06:36:25PM +0000, Ajay Gupta wrote: > Hi Heikki > > > -----Original Message----- > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Sent: Thursday, September 26, 2019 3:07 AM > > To: Ajay Gupta <ajayg@nvidia.com> > > Cc: linux-usb@vger.kernel.org > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > > > Hi Ajay, > > > > Here's the pretty much complete rewrite of the I/O handling that I was > > talking about. The first seven patches are not actually related to > > this stuff, but I'm including them here because the rest of the series > > is made on top of them. I'm including also that fix patch I send you > > earlier. > > > > After this it should be easier to handle quirks. My idea how to handle > > the multi-instance connector alt modes is that we "emulate" the PPM in > > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. > > > > We can now get the connector alternate modes that the actual > > controller supplies during probe - before registering the ucsi > > interface > How can ucsi_ccg.c get the connector alternate modes before > registering ucsi interface? PPM reset, notification enable, etc. > is done during ucsi registration. UCSI spec says: > " The only commands the PPM is required to process in the > "PPM Idle (Notifications Disabled)" state are > SET_NOTIFICATION_ENABLE and PPM_RESE" > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most > of the stuff done in ucsi_init() of ucsi.c. How about if we split ucsi_init() into a function that first simply constructs the struct ucsi and struct ucsi_connector instances without registering anything, and into separate functions that then register the ports, altmodes and what have you. I don't think that should be a huge problem. It will make ucsi.c even more like a library, which is probable a good thing. I can prepare patches for that too if you like? After that you should be able to get the struct ucsi instance that represents the "real" PPM without registering anything by calling a single function, most likely ucsi_init(). And after getting that you can construct the connector alternate modes that we actually register. Finally you register the final interface which does not use ucsi_ccg_ops, but instead something like ucsi_nvidia_ops. How would this sound to you? Br,
Hi Heikki, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Thursday, October 3, 2019 7:25 AM > To: Ajay Gupta <ajayg@nvidia.com> > Cc: linux-usb@vger.kernel.org > Subject: Re: [PATCH 00/14] usb: typec: UCSI driver overhaul > > Hi Ajay, > > On Tue, Oct 01, 2019 at 06:36:25PM +0000, Ajay Gupta wrote: > > Hi Heikki > > > > > -----Original Message----- > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Sent: Thursday, September 26, 2019 3:07 AM > > > To: Ajay Gupta <ajayg@nvidia.com> > > > Cc: linux-usb@vger.kernel.org > > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul > > > > > > Hi Ajay, > > > > > > Here's the pretty much complete rewrite of the I/O handling that I > > > was talking about. The first seven patches are not actually related > > > to this stuff, but I'm including them here because the rest of the > > > series is made on top of them. I'm including also that fix patch I > > > send you earlier. > > > > > > After this it should be easier to handle quirks. My idea how to > > > handle the multi-instance connector alt modes is that we "emulate" > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not touched at > all. > > > > > > We can now get the connector alternate modes that the actual > > > controller supplies during probe - before registering the ucsi > > > interface > > How can ucsi_ccg.c get the connector alternate modes before > > registering ucsi interface? PPM reset, notification enable, etc. > > is done during ucsi registration. UCSI spec says: > > " The only commands the PPM is required to process in the "PPM Idle > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and > > PPM_RESE" > > > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of > > the stuff done in ucsi_init() of ucsi.c. > > How about if we split ucsi_init() into a function that first simply constructs the > struct ucsi and struct ucsi_connector instances without registering anything, > and into separate functions that then register the ports, altmodes and what > have you. I don't think that should be a huge problem. It will make ucsi.c even > more like a library, which is probable a good thing. Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ? a) ucsi_ccg.c calls first part of split ucsi_init() b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes. c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate altmode found. d1) ucsi_ccg.c calls new method to register connector alternate modes. This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is expected to send squashed alternate mode. This will require changes in .read(), .sync_write() and .async_write() to make it appear as if the squashed data coming from the ppm. OR d2) ucsi_ccg.c calls new method to register squashed connector alternate modes. This method doesn't issue GET_ALTERNATE_MODES commands to PPM but simply registers the alternate mode values passed to this function. If you mean the (a->b->c->d2) solution then it looks fine to me and would wait for patches from you. This solution would mean that GET_ALTERNATE_MODES for connector is done only by each ucsi_xxx.c and not by ucsi.c > I can prepare patches for that too if you like? > After that you should be able to get the struct ucsi instance that represents > the "real" PPM without registering anything by calling a single function, most > likely ucsi_init(). And after getting that you can construct the connector > alternate modes that we actually register. > Finally you register the final interface which does not use ucsi_ccg_ops, but > instead something like ucsi_nvidia_ops. I didn't understand this part. ucsi_ccg_ops has .read(), .sync_write() and .async_write() interface and they remain same for all ucsi_ccg controllers. Thanks >nvpublic > > How would this sound to you? > > Br, > > -- > heikki
Hi Heikki, > > > > Hi Ajay, > > > > > > > > Here's the pretty much complete rewrite of the I/O handling that I > > > > was talking about. The first seven patches are not actually > > > > related to this stuff, but I'm including them here because the > > > > rest of the series is made on top of them. I'm including also that > > > > fix patch I send you earlier. > > > > > > > > After this it should be easier to handle quirks. My idea how to > > > > handle the multi-instance connector alt modes is that we "emulate" > > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not > > > > touched at > > all. > > > > > > > > We can now get the connector alternate modes that the actual > > > > controller supplies during probe - before registering the ucsi > > > > interface > > > How can ucsi_ccg.c get the connector alternate modes before > > > registering ucsi interface? PPM reset, notification enable, etc. > > > is done during ucsi registration. UCSI spec says: > > > " The only commands the PPM is required to process in the "PPM Idle > > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and > > > PPM_RESE" > > > > > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of > > > the stuff done in ucsi_init() of ucsi.c. > > > > How about if we split ucsi_init() into a function that first simply > > constructs the struct ucsi and struct ucsi_connector instances without > > registering anything, and into separate functions that then register > > the ports, altmodes and what have you. I don't think that should be a > > huge problem. It will make ucsi.c even more like a library, which is probable > a good thing. > Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ? > a) ucsi_ccg.c calls first part of split ucsi_init() > b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes. > c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate > altmode found. > d1) ucsi_ccg.c calls new method to register connector alternate modes. > This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is > expected to send squashed alternate mode. This will require changes in > .read(), .sync_write() and > .async_write() to make it appear as if the squashed data coming from the ppm. > OR > d2) ucsi_ccg.c calls new method to register squashed connector alternate > modes. > This method doesn't issue GET_ALTERNATE_MODES commands to PPM but > simply registers the alternate mode values passed to this function. > > If you mean the (a->b->c->d2) solution then it looks fine to me and would wait > for patches from you. This solution would mean that GET_ALTERNATE_MODES > for connector is done only by each ucsi_xxx.c and not by ucsi.c I am waiting for your comments on this. Thanks > nvpublic > > I can prepare patches for that too if you like? > > After that you should be able to get the struct ucsi instance that > > represents the "real" PPM without registering anything by calling a > > single function, most likely ucsi_init(). And after getting that you > > can construct the connector alternate modes that we actually register. > > Finally you register the final interface which does not use > > ucsi_ccg_ops, but instead something like ucsi_nvidia_ops. > I didn't understand this part. ucsi_ccg_ops has .read(), .sync_write() and > .async_write() interface and they remain same for all ucsi_ccg controllers. > > Thanks > > How would this sound to you? > > > > Br, > > > > -- > > heikki
Hi, On Thu, Oct 10, 2019 at 05:51:23PM +0000, Ajay Gupta wrote: > Hi Heikki, > > > > > > Hi Ajay, > > > > > > > > > > Here's the pretty much complete rewrite of the I/O handling that I > > > > > was talking about. The first seven patches are not actually > > > > > related to this stuff, but I'm including them here because the > > > > > rest of the series is made on top of them. I'm including also that > > > > > fix patch I send you earlier. > > > > > > > > > > After this it should be easier to handle quirks. My idea how to > > > > > handle the multi-instance connector alt modes is that we "emulate" > > > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not > > > > > touched at > > > all. > > > > > > > > > > We can now get the connector alternate modes that the actual > > > > > controller supplies during probe - before registering the ucsi > > > > > interface > > > > How can ucsi_ccg.c get the connector alternate modes before > > > > registering ucsi interface? PPM reset, notification enable, etc. > > > > is done during ucsi registration. UCSI spec says: > > > > " The only commands the PPM is required to process in the "PPM Idle > > > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and > > > > PPM_RESE" > > > > > > > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of > > > > the stuff done in ucsi_init() of ucsi.c. > > > > > > How about if we split ucsi_init() into a function that first simply > > > constructs the struct ucsi and struct ucsi_connector instances without > > > registering anything, and into separate functions that then register > > > the ports, altmodes and what have you. I don't think that should be a > > > huge problem. It will make ucsi.c even more like a library, which is probable > > a good thing. > > Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ? > > a) ucsi_ccg.c calls first part of split ucsi_init() > > b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes. > > c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate > > altmode found. > > d1) ucsi_ccg.c calls new method to register connector alternate modes. > > This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is > > expected to send squashed alternate mode. This will require changes in > > .read(), .sync_write() and > > .async_write() to make it appear as if the squashed data coming from the ppm. > > OR > > d2) ucsi_ccg.c calls new method to register squashed connector alternate > > modes. > > This method doesn't issue GET_ALTERNATE_MODES commands to PPM but > > simply registers the alternate mode values passed to this function. > > > > If you mean the (a->b->c->d2) solution then it looks fine to me and would wait > > for patches from you. This solution would mean that GET_ALTERNATE_MODES > > for connector is done only by each ucsi_xxx.c and not by ucsi.c > > I am waiting for your comments on this. Sorry Ajay. I lost track of this thread. The above is not exactly what I had in mind, but something like that. But in any case, I'll start wringing the patches then. thanks,
Hi Ajay, Here's the pretty much complete rewrite of the I/O handling that I was talking about. The first seven patches are not actually related to this stuff, but I'm including them here because the rest of the series is made on top of them. I'm including also that fix patch I send you earlier. After this it should be easier to handle quirks. My idea how to handle the multi-instance connector alt modes is that we "emulate" the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all. We can now get the connector alternate modes that the actual controller supplies during probe - before registering the ucsi interface - and squash all alt modes with the same SVID into one that we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES command. Also other alt mode commands like SET_NEW_CAM can have special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should not be any problem with that anymore. thanks, Heikki Krogerus (14): usb: typec: Copy everything from struct typec_capability during registration usb: typec: Introduce typec_get_drvdata() usb: typec: Separate the operations vector usb: typec: tcpm: Start using struct typec_operations usb: typec: tps6598x: Start using struct typec_operations usb: typec: ucsi: Start using struct typec_operations usb: typec: Remove the callback members from struct typec_capability usb: typec: ucsi: ccg: Remove run_isr flag usb: typec: ucsi: Simplified interface registration and I/O API usb: typec: ucsi: acpi: Move to the new API usb: typec: ucsi: ccg: Move to the new API usb: typec: ucsi: Remove the old API usb: typec: ucsi: Remove struct ucsi_control usb: typec: ucsi: Remove all bit-fields drivers/usb/typec/class.c | 125 +++--- drivers/usb/typec/tcpm/tcpm.c | 47 +-- drivers/usb/typec/tps6598x.c | 49 +-- drivers/usb/typec/ucsi/displayport.c | 26 +- drivers/usb/typec/ucsi/trace.c | 11 - drivers/usb/typec/ucsi/trace.h | 79 +--- drivers/usb/typec/ucsi/ucsi.c | 592 ++++++++++++++------------- drivers/usb/typec/ucsi/ucsi.h | 410 +++++++------------ drivers/usb/typec/ucsi/ucsi_acpi.c | 96 ++++- drivers/usb/typec/ucsi/ucsi_ccg.c | 214 ++++------ include/linux/usb/typec.h | 38 +- 11 files changed, 785 insertions(+), 902 deletions(-)