| Message ID | 1525213273-6103-2-git-send-email-mats.dev.list@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mats, On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: > The current naming used for tcpc_mux_mode constants makes > too much assumptioms about the usage of the signals. > This patch replaces the names with generic names more closely > tied to the Type-C specifications and also adds some new ones. > At the same time TCPC_MUX_* defines are removed as they do not > fit the new concept and currently have no in-tree users. I'm afraid trying to generalize the modal connector states even like this is not going to work. We can't make any assumptions about how the alternate modes configure the pins, or the connector in general. The only way this will work is that every alternate mode has its own configurations defined separately, and I'm talking about the actual pin configurations that the specifications for each alternate mode defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not work for sure. The connector states that are defined in USB Type-C specification (so basically USB Operation and USB Safe State) can be generalized, but those states just should not be defined in tcpm.h. We need to use them in other drivers as well. I'm in the middle of preparing more complete support for alternate modes. If you check the RFC [1] I send previously, in the first patch of the series I'm adding documentation that should explain the plan. > Signed-off-by: Mats Karrman <mats.dev.list@gmail.com> > --- > drivers/usb/typec/mux/pi3usb30532.c | 7 ++++--- > drivers/usb/typec/tcpm.c | 2 +- > include/linux/usb/tcpm.h | 21 ++++++++++----------- > 3 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/typec/mux/pi3usb30532.c b/drivers/usb/typec/mux/pi3usb30532.c > index b0e88db..279f3c3 100644 > --- a/drivers/usb/typec/mux/pi3usb30532.c > +++ b/drivers/usb/typec/mux/pi3usb30532.c > @@ -83,18 +83,19 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int state) > new_conf = pi->conf; > > switch (state) { > + default: > case TYPEC_MUX_NONE: > new_conf = PI3USB30532_CONF_OPEN; > break; > - case TYPEC_MUX_USB: > + case TYPEC_MUX_2CH_USBSS: > new_conf = (new_conf & PI3USB30532_CONF_SWAP) | > PI3USB30532_CONF_USB3; > break; > - case TYPEC_MUX_DP: > + case TYPEC_MUX_4CH_AM: > new_conf = (new_conf & PI3USB30532_CONF_SWAP) | > PI3USB30532_CONF_4LANE_DP; > break; > - case TYPEC_MUX_DOCK: > + case TYPEC_MUX_2CH_USBSS_2CH_AM: > new_conf = (new_conf & PI3USB30532_CONF_SWAP) | > PI3USB30532_CONF_USB3_AND_2LANE_DP; > break; > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c > index 7ee417a..0451ea0 100644 > --- a/drivers/usb/typec/tcpm.c > +++ b/drivers/usb/typec/tcpm.c > @@ -790,7 +790,7 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached, > else > usb_role = USB_ROLE_DEVICE; > > - ret = tcpm_mux_set(port, TYPEC_MUX_USB, usb_role, orientation); > + ret = tcpm_mux_set(port, TYPEC_MUX_2CH_USBSS, usb_role, orientation); > if (ret < 0) > return ret; > > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h > index b231b93..3518965 100644 > --- a/include/linux/usb/tcpm.h > +++ b/include/linux/usb/tcpm.h > @@ -93,20 +93,19 @@ struct tcpc_config { > const struct typec_altmode_desc *alt_modes; > }; > > -/* Mux state attributes */ > -#define TCPC_MUX_USB_ENABLED BIT(0) /* USB enabled */ > -#define TCPC_MUX_DP_ENABLED BIT(1) /* DP enabled */ > -#define TCPC_MUX_POLARITY_INVERTED BIT(2) /* Polarity inverted */ > - > -/* Mux modes, decoded to attributes */ > +/* Mux modes */ > enum tcpc_mux_mode { > - TYPEC_MUX_NONE = 0, /* Open switch */ > - TYPEC_MUX_USB = TCPC_MUX_USB_ENABLED, /* USB only */ > - TYPEC_MUX_DP = TCPC_MUX_DP_ENABLED, /* DP only */ > - TYPEC_MUX_DOCK = TCPC_MUX_USB_ENABLED | /* Both USB and DP */ > - TCPC_MUX_DP_ENABLED, > + TYPEC_MUX_NONE, /* Open switch */ > + TYPEC_MUX_2CH_USBSS, /* 2ch USB SS */ > + TYPEC_MUX_4CH_AM, /* 4ch Alt Mode */ > + TYPEC_MUX_2CH_USBSS_2CH_AM, /* 2ch USB SS + 2ch Alt Mode */ > + > + // Example of additional modes that may be needed in future: > + TYPEC_MUX_4CH_USBSS, /* 4ch USB SS */ > + TYPEC_MUX_2CH_USBSS_2CH_AM_B, /* 2ch USB SS + 2ch Alt Mode (e.g. DP GPU2) */ > }; > > + > /** > * struct tcpc_dev - Port configuration and callback functions > * @config: Pointer to port configuration Thanks,
On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: > Hi Mats, > > On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: > > The current naming used for tcpc_mux_mode constants makes > > too much assumptioms about the usage of the signals. > > This patch replaces the names with generic names more closely > > tied to the Type-C specifications and also adds some new ones. > > At the same time TCPC_MUX_* defines are removed as they do not > > fit the new concept and currently have no in-tree users. > > I'm afraid trying to generalize the modal connector states even like > this is not going to work. We can't make any assumptions about how the > alternate modes configure the pins, or the connector in general. > > The only way this will work is that every alternate mode has its own > configurations defined separately, and I'm talking about the actual > pin configurations that the specifications for each alternate mode > defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not > work for sure. > > The connector states that are defined in USB Type-C specification (so > basically USB Operation and USB Safe State) can be generalized, but > those states just should not be defined in tcpm.h. We need to use > them in other drivers as well. > > I'm in the middle of preparing more complete support for alternate > modes. If you check the RFC [1] I send previously, in the first patch > of the series I'm adding documentation that should explain the > plan. Sorry, I forgot the link: [1] https://www.spinics.net/lists/linux-usb/msg166520.html Cheers,
Hi Heikki, On 2018-05-02 10:25, Heikki Krogerus wrote: > On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: >> Hi Mats, >> >> On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: >>> The current naming used for tcpc_mux_mode constants makes >>> too much assumptioms about the usage of the signals. >>> This patch replaces the names with generic names more closely >>> tied to the Type-C specifications and also adds some new ones. >>> At the same time TCPC_MUX_* defines are removed as they do not >>> fit the new concept and currently have no in-tree users. >> I'm afraid trying to generalize the modal connector states even like >> this is not going to work. We can't make any assumptions about how the >> alternate modes configure the pins, or the connector in general. >> >> The only way this will work is that every alternate mode has its own >> configurations defined separately, and I'm talking about the actual >> pin configurations that the specifications for each alternate mode >> defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not >> work for sure. >> >> The connector states that are defined in USB Type-C specification (so >> basically USB Operation and USB Safe State) can be generalized, but >> those states just should not be defined in tcpm.h. We need to use >> them in other drivers as well. >> >> I'm in the middle of preparing more complete support for alternate >> modes. If you check the RFC [1] I send previously, in the first patch >> of the series I'm adding documentation that should explain the >> plan. > Sorry, I forgot the link: > > [1] https://www.spinics.net/lists/linux-usb/msg166520.html Oh, sorry I forgot about that post in the first place... I reread it now. Since the modal TYPEC_STATE_ are overlapping for each AM, this means that the mux driver "set" must know which AM is active, right? And each mux driver also need to support all possible alt modes? I was thinking that it must be a finite number of possible routes between the local connections of a mux and the four available SS lanes of the cable but of course there is no theoretical limit to the number of local connections... Do we want to set a limitation of one mux device per port? I guess we could and then let people write "composite" mux drivers if it should ever be necessary. Still it's difficult to write a mux device driver that support everything, but I'm thinking that it might be possible to write a "mode agnostic" mux driver that uses properties to match the "AM:STATE" pairs the board needs to support to the hardware mux device specific muxing modes available? It would be very interesting to see a devicetree example on how you picture things being connected to each other. Btw. You're using "mode" and "state" interchangeably which is a bit confusing. Could you settle for one? BR // Mats > Cheers, > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 02, 2018 at 03:13:44PM +0200, Mats Karrman wrote: > Hi Heikki, > > On 2018-05-02 10:25, Heikki Krogerus wrote: > > > On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: > > > Hi Mats, > > > > > > On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: > > > > The current naming used for tcpc_mux_mode constants makes > > > > too much assumptioms about the usage of the signals. > > > > This patch replaces the names with generic names more closely > > > > tied to the Type-C specifications and also adds some new ones. > > > > At the same time TCPC_MUX_* defines are removed as they do not > > > > fit the new concept and currently have no in-tree users. > > > I'm afraid trying to generalize the modal connector states even like > > > this is not going to work. We can't make any assumptions about how the > > > alternate modes configure the pins, or the connector in general. > > > > > > The only way this will work is that every alternate mode has its own > > > configurations defined separately, and I'm talking about the actual > > > pin configurations that the specifications for each alternate mode > > > defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not > > > work for sure. > > > > > > The connector states that are defined in USB Type-C specification (so > > > basically USB Operation and USB Safe State) can be generalized, but > > > those states just should not be defined in tcpm.h. We need to use > > > them in other drivers as well. > > > > > > I'm in the middle of preparing more complete support for alternate > > > modes. If you check the RFC [1] I send previously, in the first patch > > > of the series I'm adding documentation that should explain the > > > plan. > > Sorry, I forgot the link: > > > > [1] https://www.spinics.net/lists/linux-usb/msg166520.html > > Oh, sorry I forgot about that post in the first place... I reread it now. > > Since the modal TYPEC_STATE_ are overlapping for each AM, this means that > the mux driver "set" must know which AM is active, right? > > And each mux driver also need to support all possible alt modes? There are two options for the mux drivers to link with the alternate modes. They can use the typec API where a single mux is linked to a single port. Alternatively they can use the notifiers from the alternate modes themselves, which allows a single mux to be liked to multiple alternate modes. Both methods will be available. Is this what you were asking? > I was thinking that it must be a finite number of possible routes > between the local connections of a mux and the four available SS lanes of > the cable but of course there is no theoretical limit to the number of > local connections... > > Do we want to set a limitation of one mux device per port? I guess we > could and then let people write "composite" mux drivers if it should ever > be necessary. I think I answered to this one above. > Still it's difficult to write a mux device driver that support everything, > but I'm thinking that it might be possible to write a "mode agnostic" mux > driver that uses properties to match the "AM:STATE" pairs the board needs > to support to the hardware mux device specific muxing modes available? > > It would be very interesting to see a devicetree example on how you > picture things being connected to each other. > > Btw. You're using "mode" and "state" interchangeably which is a bit > confusing. Could you settle for one? OK, I'll try to be more consistent and pick one. Thanks,
On 2018-05-04 16:56, Heikki Krogerus wrote: > On Wed, May 02, 2018 at 03:13:44PM +0200, Mats Karrman wrote: >> Hi Heikki, >> >> On 2018-05-02 10:25, Heikki Krogerus wrote: >> >>> On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: >>>> Hi Mats, >>>> >>>> On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: >>>>> The current naming used for tcpc_mux_mode constants makes >>>>> too much assumptioms about the usage of the signals. >>>>> This patch replaces the names with generic names more closely >>>>> tied to the Type-C specifications and also adds some new ones. >>>>> At the same time TCPC_MUX_* defines are removed as they do not >>>>> fit the new concept and currently have no in-tree users. >>>> I'm afraid trying to generalize the modal connector states even like >>>> this is not going to work. We can't make any assumptions about how the >>>> alternate modes configure the pins, or the connector in general. >>>> >>>> The only way this will work is that every alternate mode has its own >>>> configurations defined separately, and I'm talking about the actual >>>> pin configurations that the specifications for each alternate mode >>>> defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not >>>> work for sure. >>>> >>>> The connector states that are defined in USB Type-C specification (so >>>> basically USB Operation and USB Safe State) can be generalized, but >>>> those states just should not be defined in tcpm.h. We need to use >>>> them in other drivers as well. >>>> >>>> I'm in the middle of preparing more complete support for alternate >>>> modes. If you check the RFC [1] I send previously, in the first patch >>>> of the series I'm adding documentation that should explain the >>>> plan. >>> Sorry, I forgot the link: >>> >>> [1] https://www.spinics.net/lists/linux-usb/msg166520.html >> Oh, sorry I forgot about that post in the first place... I reread it now. >> >> Since the modal TYPEC_STATE_ are overlapping for each AM, this means that >> the mux driver "set" must know which AM is active, right? >> >> And each mux driver also need to support all possible alt modes? > There are two options for the mux drivers to link with the alternate > modes. They can use the typec API where a single mux is linked to a > single port. Alternatively they can use the notifiers from the > alternate modes themselves, which allows a single mux to be liked to > multiple alternate modes. Both methods will be available. > > Is this what you were asking? Well, sort of... My angle is from the mux driver writers side. A hardware mux device does not care what logical signals is being muxed and neither should the mux driver I think. But the system designer must be able to make the mux driver respond to the correct AM:STATE. So, do you think it would be a viable approach to let the mux driver be configured through dt/acpi properties that link AM:STATE pairs to some internal muxing mode of the hardware mux device? Even so, when the mux driver "set" function is called, it will just get the mode argument but since the mode (TYPEC_STATE_...) is overlapping for different AMs if I understand your proposal correctly, the mux also needs to know what AM is active. Does this imply that the mux set function signature need to change? >> I was thinking that it must be a finite number of possible routes >> between the local connections of a mux and the four available SS lanes of >> the cable but of course there is no theoretical limit to the number of >> local connections... >> >> Do we want to set a limitation of one mux device per port? I guess we >> could and then let people write "composite" mux drivers if it should ever >> be necessary. > I think I answered to this one above. > >> Still it's difficult to write a mux device driver that support everything, >> but I'm thinking that it might be possible to write a "mode agnostic" mux >> driver that uses properties to match the "AM:STATE" pairs the board needs >> to support to the hardware mux device specific muxing modes available? >> >> It would be very interesting to see a devicetree example on how you >> picture things being connected to each other. >> >> Btw. You're using "mode" and "state" interchangeably which is a bit >> confusing. Could you settle for one? > OK, I'll try to be more consistent and pick one. > > > Thanks, > > BR // Mats -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mats, On Fri, May 04, 2018 at 06:57:31PM +0200, Mats Karrman wrote: > > On 2018-05-04 16:56, Heikki Krogerus wrote: > > > On Wed, May 02, 2018 at 03:13:44PM +0200, Mats Karrman wrote: > > > Hi Heikki, > > > > > > On 2018-05-02 10:25, Heikki Krogerus wrote: > > > > > > > On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: > > > > > Hi Mats, > > > > > > > > > > On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: > > > > > > The current naming used for tcpc_mux_mode constants makes > > > > > > too much assumptioms about the usage of the signals. > > > > > > This patch replaces the names with generic names more closely > > > > > > tied to the Type-C specifications and also adds some new ones. > > > > > > At the same time TCPC_MUX_* defines are removed as they do not > > > > > > fit the new concept and currently have no in-tree users. > > > > > I'm afraid trying to generalize the modal connector states even like > > > > > this is not going to work. We can't make any assumptions about how the > > > > > alternate modes configure the pins, or the connector in general. > > > > > > > > > > The only way this will work is that every alternate mode has its own > > > > > configurations defined separately, and I'm talking about the actual > > > > > pin configurations that the specifications for each alternate mode > > > > > defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not > > > > > work for sure. > > > > > > > > > > The connector states that are defined in USB Type-C specification (so > > > > > basically USB Operation and USB Safe State) can be generalized, but > > > > > those states just should not be defined in tcpm.h. We need to use > > > > > them in other drivers as well. > > > > > > > > > > I'm in the middle of preparing more complete support for alternate > > > > > modes. If you check the RFC [1] I send previously, in the first patch > > > > > of the series I'm adding documentation that should explain the > > > > > plan. > > > > Sorry, I forgot the link: > > > > > > > > [1] https://www.spinics.net/lists/linux-usb/msg166520.html > > > Oh, sorry I forgot about that post in the first place... I reread it now. > > > > > > Since the modal TYPEC_STATE_ are overlapping for each AM, this means that > > > the mux driver "set" must know which AM is active, right? > > > > > > And each mux driver also need to support all possible alt modes? > > There are two options for the mux drivers to link with the alternate > > modes. They can use the typec API where a single mux is linked to a > > single port. Alternatively they can use the notifiers from the > > alternate modes themselves, which allows a single mux to be liked to > > multiple alternate modes. Both methods will be available. > > > > Is this what you were asking? > > Well, sort of... My angle is from the mux driver writers side. A hardware mux > device does not care what logical signals is being muxed and neither should the > mux driver I think. But the system designer must be able to make the mux driver > respond to the correct AM:STATE. So what you want is "generic" mux drivers? The mux drivers are the ones that need to interpret the alternate mode specific connector states into actual pin muxing, not the type-c drivers. The type-c alternate mode drivers and frameworks should not even need to know if an alternate mode specific connector state requires pin muxing at all. They should be able quite simply pass forward the negotiated connector state, and be done with it. That is the only way we can keep this whole thing flexible enough to fit all kinds of platforms. Interpreting the alternate mode specific connector states is straightforward for the mux drivers, and that is all that they need to do in this case, but expecting the type-c drivers to supply the exact pin configuration to the mux drivers just so the mux driver can be a little bit more "dummy" does not only mean duplicated effort (the alternate mode drivers and the type-c frameworks still need to handle the actual alternate mode specific connector states), but also a roadmap to hacks like virtual/NOP mux drivers and platform specific quirks in the alternate mode drivers. The configuration of the type-c connector will simply not always happen the same way. We will not always have the muxes directly under operating system control. Sometimes the USB Type-C/PD controller handles them, or like in my case, a microcontroller on the system controls the muxes and the drivers need to communicate with the microcontroller. We don't know the requirements the various ways to configure the connectors have. So we don't want to end up in a scenario where we are uncertain about what does the underlying mux handling expect from the alternate mode driver (for example, do we need to inform about the Linux specific pin configuration value you are proposing, or the actual alternate mode specific connector state) but that is exactly what will happen. > So, do you think it would be a viable approach to let the mux driver be > configured through dt/acpi properties that link AM:STATE pairs to some internal > muxing mode of the hardware mux device? If the mux is able to route the pins to multiple locations on top of USB, the driver for it needs to get all the supported SVIDs, modes and their SVID specific connector states that require pin re-configuration from the hardware description. That should be enough for them. > Even so, when the mux driver "set" function is called, it will just get the > mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > AMs if I understand your proposal correctly, the mux also needs to know what AM > is active. > Does this imply that the mux set function signature need to change? My plan was actually to propose we get rid of the current mux handling (just leave the orientation switch) in favour of the notifications I'm introducing with the type-c bus for the alternate modes. The current mux handling is definitely not enough, and does not work in every scenario, like also you pointed out. Btw. Could you please not use abbreviations like "AM". Please just say "alternate mode" when you want to talk about alternate modes. Thanks,
Hi Heikki, On 05/07/2018 03:39 PM, Heikki Krogerus wrote: > Hi Mats, > > On Fri, May 04, 2018 at 06:57:31PM +0200, Mats Karrman wrote: >> On 2018-05-04 16:56, Heikki Krogerus wrote: >> >>> On Wed, May 02, 2018 at 03:13:44PM +0200, Mats Karrman wrote: >>>> Hi Heikki, >>>> >>>> On 2018-05-02 10:25, Heikki Krogerus wrote: >>>> >>>>> On Wed, May 02, 2018 at 11:23:35AM +0300, Heikki Krogerus wrote: >>>>>> Hi Mats, >>>>>> >>>>>> On Wed, May 02, 2018 at 12:21:07AM +0200, Mats Karrman wrote: >>>>>>> The current naming used for tcpc_mux_mode constants makes >>>>>>> too much assumptioms about the usage of the signals. >>>>>>> This patch replaces the names with generic names more closely >>>>>>> tied to the Type-C specifications and also adds some new ones. >>>>>>> At the same time TCPC_MUX_* defines are removed as they do not >>>>>>> fit the new concept and currently have no in-tree users. >>>>>> I'm afraid trying to generalize the modal connector states even like >>>>>> this is not going to work. We can't make any assumptions about how the >>>>>> alternate modes configure the pins, or the connector in general. >>>>>> >>>>>> The only way this will work is that every alternate mode has its own >>>>>> configurations defined separately, and I'm talking about the actual >>>>>> pin configurations that the specifications for each alternate mode >>>>>> defines, so something like TYPEC_MUX_DP and TYPEC_MUX_DOCK will not >>>>>> work for sure. >>>>>> >>>>>> The connector states that are defined in USB Type-C specification (so >>>>>> basically USB Operation and USB Safe State) can be generalized, but >>>>>> those states just should not be defined in tcpm.h. We need to use >>>>>> them in other drivers as well. >>>>>> >>>>>> I'm in the middle of preparing more complete support for alternate >>>>>> modes. If you check the RFC [1] I send previously, in the first patch >>>>>> of the series I'm adding documentation that should explain the >>>>>> plan. >>>>> Sorry, I forgot the link: >>>>> >>>>> [1] https://www.spinics.net/lists/linux-usb/msg166520.html >>>> Oh, sorry I forgot about that post in the first place... I reread it now. >>>> >>>> Since the modal TYPEC_STATE_ are overlapping for each AM, this means that >>>> the mux driver "set" must know which AM is active, right? >>>> >>>> And each mux driver also need to support all possible alt modes? >>> There are two options for the mux drivers to link with the alternate >>> modes. They can use the typec API where a single mux is linked to a >>> single port. Alternatively they can use the notifiers from the >>> alternate modes themselves, which allows a single mux to be liked to >>> multiple alternate modes. Both methods will be available. >>> >>> Is this what you were asking? >> Well, sort of... My angle is from the mux driver writers side. A hardware mux >> device does not care what logical signals is being muxed and neither should the >> mux driver I think. But the system designer must be able to make the mux driver >> respond to the correct AM:STATE. > So what you want is "generic" mux drivers? What I really want is to understand where you're going with this. I started to write a lot of answers to your statements below but then I got to where you write that you are thinking about ditching the current mux handling and that really changes a lot of things so I skip that. > The mux drivers are the > ones that need to interpret the alternate mode specific connector > states into actual pin muxing, not the type-c drivers. The type-c > alternate mode drivers and frameworks should not even need to know if > an alternate mode specific connector state requires pin muxing at all. > They should be able quite simply pass forward the negotiated connector > state, and be done with it. That is the only way we can keep this > whole thing flexible enough to fit all kinds of platforms. > > Interpreting the alternate mode specific connector states is > straightforward for the mux drivers, and that is all that they need to > do in this case, but expecting the type-c drivers to supply the exact > pin configuration to the mux drivers just so the mux driver can be a > little bit more "dummy" does not only mean duplicated effort (the > alternate mode drivers and the type-c frameworks still need to handle > the actual alternate mode specific connector states), but also a > roadmap to hacks like virtual/NOP mux drivers and platform specific > quirks in the alternate mode drivers. > > The configuration of the type-c connector will simply not always > happen the same way. We will not always have the muxes directly under > operating system control. Sometimes the USB Type-C/PD controller > handles them, or like in my case, a microcontroller on the system > controls the muxes and the drivers need to communicate with the > microcontroller. We don't know the requirements the various ways to > configure the connectors have. > > So we don't want to end up in a scenario where we are uncertain about > what does the underlying mux handling expect from the alternate mode > driver (for example, do we need to inform about the Linux specific pin > configuration value you are proposing, or the actual alternate mode > specific connector state) but that is exactly what will happen. > >> So, do you think it would be a viable approach to let the mux driver be >> configured through dt/acpi properties that link AM:STATE pairs to some internal >> muxing mode of the hardware mux device? > If the mux is able to route the pins to multiple locations on top of > USB, the driver for it needs to get all the supported SVIDs, modes and > their SVID specific connector states that require pin re-configuration > from the hardware description. That should be enough for them. > >> Even so, when the mux driver "set" function is called, it will just get the >> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >> AMs if I understand your proposal correctly, the mux also needs to know what AM >> is active. >> Does this imply that the mux set function signature need to change? > My plan was actually to propose we get rid of the current mux handling > (just leave the orientation switch) in favour of the notifications I'm > introducing with the type-c bus for the alternate modes. The current > mux handling is definitely not enough, and does not work in every > scenario, like also you pointed out. So, the mux need to subscribe to each svid:mode pair it is interested in using typec_altmode_register_notifier() and then use those callbacks to switch the correct signals to the connector. And a driver for an off-the-shelf mux device could have the translation between svid:mode pairs and mux device specific control specified by of/acpi properties. Right? > Btw. Could you please not use abbreviations like "AM". Please just say > "alternate mode" when you want to talk about alternate modes. I will try ;-) > Thanks, > BR // Mats -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > >> Even so, when the mux driver "set" function is called, it will just get the > >> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > >> AMs if I understand your proposal correctly, the mux also needs to know what AM > >> is active. > >> Does this imply that the mux set function signature need to change? > > My plan was actually to propose we get rid of the current mux handling > > (just leave the orientation switch) in favour of the notifications I'm > > introducing with the type-c bus for the alternate modes. The current > > mux handling is definitely not enough, and does not work in every > > scenario, like also you pointed out. > > So, the mux need to subscribe to each svid:mode pair it is interested in using > typec_altmode_register_notifier() and then use those callbacks to switch the correct > signals to the connector. And a driver for an off-the-shelf mux device could have > the translation between svid:mode pairs and mux device specific control specified by > of/acpi properties. Right? Yes. That is the plan. Would it work for you? Thanks,
Hi, On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > Hi, > > On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: >>>> Even so, when the mux driver "set" function is called, it will just get the >>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >>>> AMs if I understand your proposal correctly, the mux also needs to know what AM >>>> is active. >>>> Does this imply that the mux set function signature need to change? >>> My plan was actually to propose we get rid of the current mux handling >>> (just leave the orientation switch) in favour of the notifications I'm >>> introducing with the type-c bus for the alternate modes. The current >>> mux handling is definitely not enough, and does not work in every >>> scenario, like also you pointed out. >> So, the mux need to subscribe to each svid:mode pair it is interested in using >> typec_altmode_register_notifier() and then use those callbacks to switch the correct >> signals to the connector. And a driver for an off-the-shelf mux device could have >> the translation between svid:mode pairs and mux device specific control specified by >> of/acpi properties. Right? > Yes. That is the plan. Would it work for you? I think so. I'll give it a go. When about do you think you'll post the next version of your RFC? Or do you have an updated series available somewhere public? BR // Mats > Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: > Hi, > > On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > > > Hi, > > > > On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > >>>> Even so, when the mux driver "set" function is called, it will just get the > >>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > >>>> AMs if I understand your proposal correctly, the mux also needs to know what AM > >>>> is active. > >>>> Does this imply that the mux set function signature need to change? > >>> My plan was actually to propose we get rid of the current mux handling > >>> (just leave the orientation switch) in favour of the notifications I'm > >>> introducing with the type-c bus for the alternate modes. The current > >>> mux handling is definitely not enough, and does not work in every > >>> scenario, like also you pointed out. > >> So, the mux need to subscribe to each svid:mode pair it is interested in using > >> typec_altmode_register_notifier() and then use those callbacks to switch the correct > >> signals to the connector. And a driver for an off-the-shelf mux device could have > >> the translation between svid:mode pairs and mux device specific control specified by > >> of/acpi properties. Right? > > Yes. That is the plan. Would it work for you? > > I think so. I'll give it a go. When about do you think you'll post the next version > of your RFC? Or do you have an updated series available somewhere public? I'll try to put together and post the next version tomorrow. My original plan was actually to use just the notifications with the muxes, but one thing to consider with the notifications is that in practice we have to increment the ref count for the alt mode devices when ever something registers a notifier. To me that does not feel ideal. The dependency should go the other way around in case of the muxes. That is why I liked the separate API and handling for the muxes felt better, as it will do just that. The mux is then a "service" that the port driver can as for, and if it gets a handle to a mux, the mux will have its ref count incremented. So I think fixing the mux API would perhaps be better after all. Thoughts? Thanks,
On 2018-05-11 13:14, Heikki Krogerus wrote: > On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: >> On 2018-05-10 19:49, Heikki Krogerus wrote: >> >>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: >>>> Hi, >>>> >>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: >>>> >>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: >>>>>> Hi, >>>>>> >>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: >>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the >>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM >>>>>>>>>> is active. >>>>>>>>>> Does this imply that the mux set function signature need to change? >>>>>>>>> My plan was actually to propose we get rid of the current mux handling >>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm >>>>>>>>> introducing with the type-c bus for the alternate modes. The current >>>>>>>>> mux handling is definitely not enough, and does not work in every >>>>>>>>> scenario, like also you pointed out. >>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using >>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct >>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have >>>>>>>> the translation between svid:mode pairs and mux device specific control specified by >>>>>>>> of/acpi properties. Right? >>>>>>> Yes. That is the plan. Would it work for you? >>>>>> I think so. I'll give it a go. When about do you think you'll post the next version >>>>>> of your RFC? Or do you have an updated series available somewhere public? >>>>> I'll try to put together and post the next version tomorrow. >>>>> >>>>> My original plan was actually to use just the notifications with the >>>>> muxes, but one thing to consider with the notifications is that in >>>>> practice we have to increment the ref count for the alt mode devices >>>>> when ever something registers a notifier. >>>>> >>>>> To me that does not feel ideal. The dependency should go the other way >>>>> around in case of the muxes. That is why I liked the separate API and >>>>> handling for the muxes felt better, as it will do just that. The mux >>>>> is then a "service" that the port driver can as for, and if it gets a >>>>> handle to a mux, the mux will have its ref count incremented. >>>>> >>>>> So I think fixing the mux API would perhaps be better after all. >>>>> Thoughts? >>>> So, we're back to a mux API similar to the current one, where: >>>> - the port driver and the mux driver are connected through "graph" >>>> - alt mode driver finds its port mux using the typec class mux api >>>> - the mux mode setting function arguments now include both svid and mode >>>> >>>> I like that. >>>> >>>> One thought that popped up again is if we, somewhere down the line, >>>> will see some super device that support many different alternate modes >>>> on the same port and therefore will need to have multiple mux devices? >>>> However I think the mux api could be extended (later on) to support some >>>> aggregate mux device that manages multiple physical devices. >>> If we simply had always a mux for every alternate mode, that would not >>> be a problem. So the port would have its own mux, and every supported >>> alternate mode also had its own. I think that removes the need to deal >>> with the svid:mode when using the muxes, as they are already tied to a >>> specific alternate modes, right? With a single mux device, for example >>> pi3usb30532, the driver just needs to register a mux for the port and >>> separate mux for DP, but I don't think that's a huge problem. >> Hmm... As an hypothetical example I have written a driver for another mux >> from TI and according to its data sheet: >> >> """ >> The HD3SS460 is a generic analog differential >> passive switch that can work for any high speed >> interface applications as long as it is biased at a >> common mode voltage range of 0-2V and has >> differential signaling with differential amplitude up to >> 1800mVpp.... >> """ >> >> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + >> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is >> that it is a general mux device so the driver writer does not know what types of >> muxes to register. I guess it could also be configured using properties but that >> would be very complicated. > Why? All the mux driver needs to get from device properties is the > SVID and the mode. Sigh... Again, if the same mux handles signals for more than one alternate mode the driver won't know what alternate mode is intended if it only receives the connector state which use overlapping numbers for different alternate modes. > Is there a problem providing both svid and sub-mode in the mux set call? The >> partner drivers should all know what svid they implement. > By sub-mode, what do you mean? The SVID specific connector state value > you already get with the mux ->set callback. The mode index number is > not very useful (with DP for example it will always be 1). > > In any case, the mux driver will still need to interpret the SVID > specific connector states, so what would it change if we supplied also > the SVID and mode on top of that with the ->set callback? Ditto > Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mats, On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: > On 2018-05-11 13:14, Heikki Krogerus wrote: > > > On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: > > > On 2018-05-10 19:49, Heikki Krogerus wrote: > > > > > > > On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: > > > > > Hi, > > > > > > > > > > On 05/09/2018 02:49 PM, Heikki Krogerus wrote: > > > > > > > > > > > On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > > > > > > > > > > > Even so, when the mux driver "set" function is called, it will just get the > > > > > > > > > > > mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > > > > > > > > > > > AMs if I understand your proposal correctly, the mux also needs to know what AM > > > > > > > > > > > is active. > > > > > > > > > > > Does this imply that the mux set function signature need to change? > > > > > > > > > > My plan was actually to propose we get rid of the current mux handling > > > > > > > > > > (just leave the orientation switch) in favour of the notifications I'm > > > > > > > > > > introducing with the type-c bus for the alternate modes. The current > > > > > > > > > > mux handling is definitely not enough, and does not work in every > > > > > > > > > > scenario, like also you pointed out. > > > > > > > > > So, the mux need to subscribe to each svid:mode pair it is interested in using > > > > > > > > > typec_altmode_register_notifier() and then use those callbacks to switch the correct > > > > > > > > > signals to the connector. And a driver for an off-the-shelf mux device could have > > > > > > > > > the translation between svid:mode pairs and mux device specific control specified by > > > > > > > > > of/acpi properties. Right? > > > > > > > > Yes. That is the plan. Would it work for you? > > > > > > > I think so. I'll give it a go. When about do you think you'll post the next version > > > > > > > of your RFC? Or do you have an updated series available somewhere public? > > > > > > I'll try to put together and post the next version tomorrow. > > > > > > > > > > > > My original plan was actually to use just the notifications with the > > > > > > muxes, but one thing to consider with the notifications is that in > > > > > > practice we have to increment the ref count for the alt mode devices > > > > > > when ever something registers a notifier. > > > > > > > > > > > > To me that does not feel ideal. The dependency should go the other way > > > > > > around in case of the muxes. That is why I liked the separate API and > > > > > > handling for the muxes felt better, as it will do just that. The mux > > > > > > is then a "service" that the port driver can as for, and if it gets a > > > > > > handle to a mux, the mux will have its ref count incremented. > > > > > > > > > > > > So I think fixing the mux API would perhaps be better after all. > > > > > > Thoughts? > > > > > So, we're back to a mux API similar to the current one, where: > > > > > - the port driver and the mux driver are connected through "graph" > > > > > - alt mode driver finds its port mux using the typec class mux api > > > > > - the mux mode setting function arguments now include both svid and mode > > > > > > > > > > I like that. > > > > > > > > > > One thought that popped up again is if we, somewhere down the line, > > > > > will see some super device that support many different alternate modes > > > > > on the same port and therefore will need to have multiple mux devices? > > > > > However I think the mux api could be extended (later on) to support some > > > > > aggregate mux device that manages multiple physical devices. > > > > If we simply had always a mux for every alternate mode, that would not > > > > be a problem. So the port would have its own mux, and every supported > > > > alternate mode also had its own. I think that removes the need to deal > > > > with the svid:mode when using the muxes, as they are already tied to a > > > > specific alternate modes, right? With a single mux device, for example > > > > pi3usb30532, the driver just needs to register a mux for the port and > > > > separate mux for DP, but I don't think that's a huge problem. > > > Hmm... As an hypothetical example I have written a driver for another mux > > > from TI and according to its data sheet: > > > > > > """ > > > The HD3SS460 is a generic analog differential > > > passive switch that can work for any high speed > > > interface applications as long as it is biased at a > > > common mode voltage range of 0-2V and has > > > differential signaling with differential amplitude up to > > > 1800mVpp.... > > > """ > > > > > > What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + > > > 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is > > > that it is a general mux device so the driver writer does not know what types of > > > muxes to register. I guess it could also be configured using properties but that > > > would be very complicated. > > Why? All the mux driver needs to get from device properties is the > > SVID and the mode. > > Sigh... Again, if the same mux handles signals for more than one alternate mode > the driver won't know what alternate mode is intended if it only receives the > connector state which use overlapping numbers for different alternate modes. You are missing the point. We are now registering separate struct typec_mux for every alt mode. The ->set callback will need to be implemented separately for every alt mode. So in case of TI HD3SS460, we need to initially register a struct typec_mux for DP and implement a function for the ->set callback for DP only. If we later need to support another alt mode with that mux (HDMI perhaps), we need to register second struct typec_mux and implement separate function for that alt mode alone and point the ->set callback of the second struct typec_mux to that. > > Is there a problem providing both svid and sub-mode in the mux set call? The > > > partner drivers should all know what svid they implement. > > By sub-mode, what do you mean? The SVID specific connector state value > > you already get with the mux ->set callback. The mode index number is > > not very useful (with DP for example it will always be 1). > > > > In any case, the mux driver will still need to interpret the SVID > > specific connector states, so what would it change if we supplied also > > the SVID and mode on top of that with the ->set callback? > > Ditto Thanks,
Hi Heikki, On 05/15/2018 09:30 AM, Heikki Krogerus wrote: > Hi Mats, > > On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: >> On 2018-05-11 13:14, Heikki Krogerus wrote: >> >>> On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: >>>> On 2018-05-10 19:49, Heikki Krogerus wrote: >>>> >>>>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: >>>>>> Hi, >>>>>> >>>>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: >>>>>> >>>>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: >>>>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the >>>>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >>>>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM >>>>>>>>>>>> is active. >>>>>>>>>>>> Does this imply that the mux set function signature need to change? >>>>>>>>>>> My plan was actually to propose we get rid of the current mux handling >>>>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm >>>>>>>>>>> introducing with the type-c bus for the alternate modes. The current >>>>>>>>>>> mux handling is definitely not enough, and does not work in every >>>>>>>>>>> scenario, like also you pointed out. >>>>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using >>>>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct >>>>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have >>>>>>>>>> the translation between svid:mode pairs and mux device specific control specified by >>>>>>>>>> of/acpi properties. Right? >>>>>>>>> Yes. That is the plan. Would it work for you? >>>>>>>> I think so. I'll give it a go. When about do you think you'll post the next version >>>>>>>> of your RFC? Or do you have an updated series available somewhere public? >>>>>>> I'll try to put together and post the next version tomorrow. >>>>>>> >>>>>>> My original plan was actually to use just the notifications with the >>>>>>> muxes, but one thing to consider with the notifications is that in >>>>>>> practice we have to increment the ref count for the alt mode devices >>>>>>> when ever something registers a notifier. >>>>>>> >>>>>>> To me that does not feel ideal. The dependency should go the other way >>>>>>> around in case of the muxes. That is why I liked the separate API and >>>>>>> handling for the muxes felt better, as it will do just that. The mux >>>>>>> is then a "service" that the port driver can as for, and if it gets a >>>>>>> handle to a mux, the mux will have its ref count incremented. >>>>>>> >>>>>>> So I think fixing the mux API would perhaps be better after all. >>>>>>> Thoughts? >>>>>> So, we're back to a mux API similar to the current one, where: >>>>>> - the port driver and the mux driver are connected through "graph" >>>>>> - alt mode driver finds its port mux using the typec class mux api >>>>>> - the mux mode setting function arguments now include both svid and mode >>>>>> >>>>>> I like that. >>>>>> >>>>>> One thought that popped up again is if we, somewhere down the line, >>>>>> will see some super device that support many different alternate modes >>>>>> on the same port and therefore will need to have multiple mux devices? >>>>>> However I think the mux api could be extended (later on) to support some >>>>>> aggregate mux device that manages multiple physical devices. >>>>> If we simply had always a mux for every alternate mode, that would not >>>>> be a problem. So the port would have its own mux, and every supported >>>>> alternate mode also had its own. I think that removes the need to deal >>>>> with the svid:mode when using the muxes, as they are already tied to a >>>>> specific alternate modes, right? With a single mux device, for example >>>>> pi3usb30532, the driver just needs to register a mux for the port and >>>>> separate mux for DP, but I don't think that's a huge problem. >>>> Hmm... As an hypothetical example I have written a driver for another mux >>>> from TI and according to its data sheet: >>>> >>>> """ >>>> The HD3SS460 is a generic analog differential >>>> passive switch that can work for any high speed >>>> interface applications as long as it is biased at a >>>> common mode voltage range of 0-2V and has >>>> differential signaling with differential amplitude up to >>>> 1800mVpp.... >>>> """ >>>> >>>> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + >>>> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is >>>> that it is a general mux device so the driver writer does not know what types of >>>> muxes to register. I guess it could also be configured using properties but that >>>> would be very complicated. >>> Why? All the mux driver needs to get from device properties is the >>> SVID and the mode. >> Sigh... Again, if the same mux handles signals for more than one alternate mode >> the driver won't know what alternate mode is intended if it only receives the >> connector state which use overlapping numbers for different alternate modes. > You are missing the point. We are now registering separate struct > typec_mux for every alt mode. The ->set callback will need to be > implemented separately for every alt mode. > > So in case of TI HD3SS460, we need to initially register a struct > typec_mux for DP and implement a function for the ->set callback for > DP only. If we later need to support another alt mode with that mux > (HDMI perhaps), we need to register second struct typec_mux and > implement separate function for that alt mode alone and point the > ->set callback of the second struct typec_mux to that. No, I'm not missing the point... At least not that one :) But I think you are missing my point that a driver for a general purpose mux device will end up having to register a struct typec_mux and implement a ->set function for every possible alternate mode that eventually will exist (and can be used with that mux). BR // Mats >>> Is there a problem providing both svid and sub-mode in the mux set call? The >>>> partner drivers should all know what svid they implement. >>> By sub-mode, what do you mean? The SVID specific connector state value >>> you already get with the mux ->set callback. The mode index number is >>> not very useful (with DP for example it will always be 1). >>> >>> In any case, the mux driver will still need to interpret the SVID >>> specific connector states, so what would it change if we supplied also >>> the SVID and mode on top of that with the ->set callback? >> Ditto > Thanks, > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 11:24:07PM +0200, Mats Karrman wrote: > Hi Heikki, > > On 05/15/2018 09:30 AM, Heikki Krogerus wrote: > > > Hi Mats, > > > > On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: > >> On 2018-05-11 13:14, Heikki Krogerus wrote: > >> > >>> On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: > >>>> On 2018-05-10 19:49, Heikki Krogerus wrote: > >>>> > >>>>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: > >>>>>> > >>>>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > >>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > >>>>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the > >>>>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > >>>>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM > >>>>>>>>>>>> is active. > >>>>>>>>>>>> Does this imply that the mux set function signature need to change? > >>>>>>>>>>> My plan was actually to propose we get rid of the current mux handling > >>>>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm > >>>>>>>>>>> introducing with the type-c bus for the alternate modes. The current > >>>>>>>>>>> mux handling is definitely not enough, and does not work in every > >>>>>>>>>>> scenario, like also you pointed out. > >>>>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using > >>>>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct > >>>>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have > >>>>>>>>>> the translation between svid:mode pairs and mux device specific control specified by > >>>>>>>>>> of/acpi properties. Right? > >>>>>>>>> Yes. That is the plan. Would it work for you? > >>>>>>>> I think so. I'll give it a go. When about do you think you'll post the next version > >>>>>>>> of your RFC? Or do you have an updated series available somewhere public? > >>>>>>> I'll try to put together and post the next version tomorrow. > >>>>>>> > >>>>>>> My original plan was actually to use just the notifications with the > >>>>>>> muxes, but one thing to consider with the notifications is that in > >>>>>>> practice we have to increment the ref count for the alt mode devices > >>>>>>> when ever something registers a notifier. > >>>>>>> > >>>>>>> To me that does not feel ideal. The dependency should go the other way > >>>>>>> around in case of the muxes. That is why I liked the separate API and > >>>>>>> handling for the muxes felt better, as it will do just that. The mux > >>>>>>> is then a "service" that the port driver can as for, and if it gets a > >>>>>>> handle to a mux, the mux will have its ref count incremented. > >>>>>>> > >>>>>>> So I think fixing the mux API would perhaps be better after all. > >>>>>>> Thoughts? > >>>>>> So, we're back to a mux API similar to the current one, where: > >>>>>> - the port driver and the mux driver are connected through "graph" > >>>>>> - alt mode driver finds its port mux using the typec class mux api > >>>>>> - the mux mode setting function arguments now include both svid and mode > >>>>>> > >>>>>> I like that. > >>>>>> > >>>>>> One thought that popped up again is if we, somewhere down the line, > >>>>>> will see some super device that support many different alternate modes > >>>>>> on the same port and therefore will need to have multiple mux devices? > >>>>>> However I think the mux api could be extended (later on) to support some > >>>>>> aggregate mux device that manages multiple physical devices. > >>>>> If we simply had always a mux for every alternate mode, that would not > >>>>> be a problem. So the port would have its own mux, and every supported > >>>>> alternate mode also had its own. I think that removes the need to deal > >>>>> with the svid:mode when using the muxes, as they are already tied to a > >>>>> specific alternate modes, right? With a single mux device, for example > >>>>> pi3usb30532, the driver just needs to register a mux for the port and > >>>>> separate mux for DP, but I don't think that's a huge problem. > >>>> Hmm... As an hypothetical example I have written a driver for another mux > >>>> from TI and according to its data sheet: > >>>> > >>>> """ > >>>> The HD3SS460 is a generic analog differential > >>>> passive switch that can work for any high speed > >>>> interface applications as long as it is biased at a > >>>> common mode voltage range of 0-2V and has > >>>> differential signaling with differential amplitude up to > >>>> 1800mVpp.... > >>>> """ > >>>> > >>>> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + > >>>> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is > >>>> that it is a general mux device so the driver writer does not know what types of > >>>> muxes to register. I guess it could also be configured using properties but that > >>>> would be very complicated. > >>> Why? All the mux driver needs to get from device properties is the > >>> SVID and the mode. > >> Sigh... Again, if the same mux handles signals for more than one alternate mode > >> the driver won't know what alternate mode is intended if it only receives the > >> connector state which use overlapping numbers for different alternate modes. > > You are missing the point. We are now registering separate struct > > typec_mux for every alt mode. The ->set callback will need to be > > implemented separately for every alt mode. > > > > So in case of TI HD3SS460, we need to initially register a struct > > typec_mux for DP and implement a function for the ->set callback for > > DP only. If we later need to support another alt mode with that mux > > (HDMI perhaps), we need to register second struct typec_mux and > > implement separate function for that alt mode alone and point the > > ->set callback of the second struct typec_mux to that. > > No, I'm not missing the point... At least not that one :) > But I think you are missing my point that a driver for a general > purpose mux device will end up having to register a struct typec_mux > and implement a ->set function for every possible alternate mode > that eventually will exist (and can be used with that mux). OK, we are on the same page. So back to my question. I'll rephrase: How would separate ->set functions differ from delivering the SVID:mode on top of the SVID specific connector state to the mux? You still need to handle every alternate mode separately in the mux driver. I doubt that HD3SS460 will (or even can) be used with anything else except DP. Maybe with HDMI. It definitely will not be usable with all alternate modes. Realistically, I think we are talking about two, max three alternate modes any mux driver will ever need to handle. Thanks,
On 05/16/2018 01:43 PM, Heikki Krogerus wrote: > On Tue, May 15, 2018 at 11:24:07PM +0200, Mats Karrman wrote: >> Hi Heikki, >> >> On 05/15/2018 09:30 AM, Heikki Krogerus wrote: >> >>> Hi Mats, >>> >>> On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: >>>> On 2018-05-11 13:14, Heikki Krogerus wrote: >>>> >>>>> On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: >>>>>> On 2018-05-10 19:49, Heikki Krogerus wrote: >>>>>> >>>>>>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: >>>>>>>> >>>>>>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: >>>>>>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the >>>>>>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >>>>>>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM >>>>>>>>>>>>>> is active. >>>>>>>>>>>>>> Does this imply that the mux set function signature need to change? >>>>>>>>>>>>> My plan was actually to propose we get rid of the current mux handling >>>>>>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm >>>>>>>>>>>>> introducing with the type-c bus for the alternate modes. The current >>>>>>>>>>>>> mux handling is definitely not enough, and does not work in every >>>>>>>>>>>>> scenario, like also you pointed out. >>>>>>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using >>>>>>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct >>>>>>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have >>>>>>>>>>>> the translation between svid:mode pairs and mux device specific control specified by >>>>>>>>>>>> of/acpi properties. Right? >>>>>>>>>>> Yes. That is the plan. Would it work for you? >>>>>>>>>> I think so. I'll give it a go. When about do you think you'll post the next version >>>>>>>>>> of your RFC? Or do you have an updated series available somewhere public? >>>>>>>>> I'll try to put together and post the next version tomorrow. >>>>>>>>> >>>>>>>>> My original plan was actually to use just the notifications with the >>>>>>>>> muxes, but one thing to consider with the notifications is that in >>>>>>>>> practice we have to increment the ref count for the alt mode devices >>>>>>>>> when ever something registers a notifier. >>>>>>>>> >>>>>>>>> To me that does not feel ideal. The dependency should go the other way >>>>>>>>> around in case of the muxes. That is why I liked the separate API and >>>>>>>>> handling for the muxes felt better, as it will do just that. The mux >>>>>>>>> is then a "service" that the port driver can as for, and if it gets a >>>>>>>>> handle to a mux, the mux will have its ref count incremented. >>>>>>>>> >>>>>>>>> So I think fixing the mux API would perhaps be better after all. >>>>>>>>> Thoughts? >>>>>>>> So, we're back to a mux API similar to the current one, where: >>>>>>>> - the port driver and the mux driver are connected through "graph" >>>>>>>> - alt mode driver finds its port mux using the typec class mux api >>>>>>>> - the mux mode setting function arguments now include both svid and mode >>>>>>>> >>>>>>>> I like that. >>>>>>>> >>>>>>>> One thought that popped up again is if we, somewhere down the line, >>>>>>>> will see some super device that support many different alternate modes >>>>>>>> on the same port and therefore will need to have multiple mux devices? >>>>>>>> However I think the mux api could be extended (later on) to support some >>>>>>>> aggregate mux device that manages multiple physical devices. >>>>>>> If we simply had always a mux for every alternate mode, that would not >>>>>>> be a problem. So the port would have its own mux, and every supported >>>>>>> alternate mode also had its own. I think that removes the need to deal >>>>>>> with the svid:mode when using the muxes, as they are already tied to a >>>>>>> specific alternate modes, right? With a single mux device, for example >>>>>>> pi3usb30532, the driver just needs to register a mux for the port and >>>>>>> separate mux for DP, but I don't think that's a huge problem. >>>>>> Hmm... As an hypothetical example I have written a driver for another mux >>>>>> from TI and according to its data sheet: >>>>>> >>>>>> """ >>>>>> The HD3SS460 is a generic analog differential >>>>>> passive switch that can work for any high speed >>>>>> interface applications as long as it is biased at a >>>>>> common mode voltage range of 0-2V and has >>>>>> differential signaling with differential amplitude up to >>>>>> 1800mVpp.... >>>>>> """ >>>>>> >>>>>> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + >>>>>> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is >>>>>> that it is a general mux device so the driver writer does not know what types of >>>>>> muxes to register. I guess it could also be configured using properties but that >>>>>> would be very complicated. >>>>> Why? All the mux driver needs to get from device properties is the >>>>> SVID and the mode. >>>> Sigh... Again, if the same mux handles signals for more than one alternate mode >>>> the driver won't know what alternate mode is intended if it only receives the >>>> connector state which use overlapping numbers for different alternate modes. >>> You are missing the point. We are now registering separate struct >>> typec_mux for every alt mode. The ->set callback will need to be >>> implemented separately for every alt mode. >>> >>> So in case of TI HD3SS460, we need to initially register a struct >>> typec_mux for DP and implement a function for the ->set callback for >>> DP only. If we later need to support another alt mode with that mux >>> (HDMI perhaps), we need to register second struct typec_mux and >>> implement separate function for that alt mode alone and point the >>> ->set callback of the second struct typec_mux to that. >> No, I'm not missing the point... At least not that one :) >> But I think you are missing my point that a driver for a general >> purpose mux device will end up having to register a struct typec_mux >> and implement a ->set function for every possible alternate mode >> that eventually will exist (and can be used with that mux). > OK, we are on the same page. So back to my question. I'll rephrase: > How would separate ->set functions differ from delivering the > SVID:mode on top of the SVID specific connector state to the mux? You > still need to handle every alternate mode separately in the mux > driver. My idea, as I tried to explain before, is to use properties for mapping mux device specific states to svid:mode:state. The mux driver would not need to know anything about alternate modes, the responsibility would be with the system architect to get the mapping right in firmware. > I doubt that HD3SS460 will (or even can) be used with anything else > except DP. Maybe with HDMI. It definitely will not be usable with all > alternate modes. Realistically, I think we are talking about two, max > three alternate modes any mux driver will ever need to handle. I'm not so sure. Time will tell. And using HD3SS460 as an example, there are multiple ways to connect the DP signals to the MUX that the hw designer may want to take advantage of to get the best possible lay-out BR // Mats -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 16, 2018 at 11:11:02PM +0200, Mats Karrman wrote: > On 05/16/2018 01:43 PM, Heikki Krogerus wrote: > > > On Tue, May 15, 2018 at 11:24:07PM +0200, Mats Karrman wrote: > >> Hi Heikki, > >> > >> On 05/15/2018 09:30 AM, Heikki Krogerus wrote: > >> > >>> Hi Mats, > >>> > >>> On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: > >>>> On 2018-05-11 13:14, Heikki Krogerus wrote: > >>>> > >>>>> On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: > >>>>>> On 2018-05-10 19:49, Heikki Krogerus wrote: > >>>>>> > >>>>>>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: > >>>>>>>> > >>>>>>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: > >>>>>>>>>> Hi, > >>>>>>>>>> > >>>>>>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > >>>>>>>>>> > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > >>>>>>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the > >>>>>>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > >>>>>>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM > >>>>>>>>>>>>>> is active. > >>>>>>>>>>>>>> Does this imply that the mux set function signature need to change? > >>>>>>>>>>>>> My plan was actually to propose we get rid of the current mux handling > >>>>>>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm > >>>>>>>>>>>>> introducing with the type-c bus for the alternate modes. The current > >>>>>>>>>>>>> mux handling is definitely not enough, and does not work in every > >>>>>>>>>>>>> scenario, like also you pointed out. > >>>>>>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using > >>>>>>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct > >>>>>>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have > >>>>>>>>>>>> the translation between svid:mode pairs and mux device specific control specified by > >>>>>>>>>>>> of/acpi properties. Right? > >>>>>>>>>>> Yes. That is the plan. Would it work for you? > >>>>>>>>>> I think so. I'll give it a go. When about do you think you'll post the next version > >>>>>>>>>> of your RFC? Or do you have an updated series available somewhere public? > >>>>>>>>> I'll try to put together and post the next version tomorrow. > >>>>>>>>> > >>>>>>>>> My original plan was actually to use just the notifications with the > >>>>>>>>> muxes, but one thing to consider with the notifications is that in > >>>>>>>>> practice we have to increment the ref count for the alt mode devices > >>>>>>>>> when ever something registers a notifier. > >>>>>>>>> > >>>>>>>>> To me that does not feel ideal. The dependency should go the other way > >>>>>>>>> around in case of the muxes. That is why I liked the separate API and > >>>>>>>>> handling for the muxes felt better, as it will do just that. The mux > >>>>>>>>> is then a "service" that the port driver can as for, and if it gets a > >>>>>>>>> handle to a mux, the mux will have its ref count incremented. > >>>>>>>>> > >>>>>>>>> So I think fixing the mux API would perhaps be better after all. > >>>>>>>>> Thoughts? > >>>>>>>> So, we're back to a mux API similar to the current one, where: > >>>>>>>> - the port driver and the mux driver are connected through "graph" > >>>>>>>> - alt mode driver finds its port mux using the typec class mux api > >>>>>>>> - the mux mode setting function arguments now include both svid and mode > >>>>>>>> > >>>>>>>> I like that. > >>>>>>>> > >>>>>>>> One thought that popped up again is if we, somewhere down the line, > >>>>>>>> will see some super device that support many different alternate modes > >>>>>>>> on the same port and therefore will need to have multiple mux devices? > >>>>>>>> However I think the mux api could be extended (later on) to support some > >>>>>>>> aggregate mux device that manages multiple physical devices. > >>>>>>> If we simply had always a mux for every alternate mode, that would not > >>>>>>> be a problem. So the port would have its own mux, and every supported > >>>>>>> alternate mode also had its own. I think that removes the need to deal > >>>>>>> with the svid:mode when using the muxes, as they are already tied to a > >>>>>>> specific alternate modes, right? With a single mux device, for example > >>>>>>> pi3usb30532, the driver just needs to register a mux for the port and > >>>>>>> separate mux for DP, but I don't think that's a huge problem. > >>>>>> Hmm... As an hypothetical example I have written a driver for another mux > >>>>>> from TI and according to its data sheet: > >>>>>> > >>>>>> """ > >>>>>> The HD3SS460 is a generic analog differential > >>>>>> passive switch that can work for any high speed > >>>>>> interface applications as long as it is biased at a > >>>>>> common mode voltage range of 0-2V and has > >>>>>> differential signaling with differential amplitude up to > >>>>>> 1800mVpp.... > >>>>>> """ > >>>>>> > >>>>>> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + > >>>>>> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is > >>>>>> that it is a general mux device so the driver writer does not know what types of > >>>>>> muxes to register. I guess it could also be configured using properties but that > >>>>>> would be very complicated. > >>>>> Why? All the mux driver needs to get from device properties is the > >>>>> SVID and the mode. > >>>> Sigh... Again, if the same mux handles signals for more than one alternate mode > >>>> the driver won't know what alternate mode is intended if it only receives the > >>>> connector state which use overlapping numbers for different alternate modes. > >>> You are missing the point. We are now registering separate struct > >>> typec_mux for every alt mode. The ->set callback will need to be > >>> implemented separately for every alt mode. > >>> > >>> So in case of TI HD3SS460, we need to initially register a struct > >>> typec_mux for DP and implement a function for the ->set callback for > >>> DP only. If we later need to support another alt mode with that mux > >>> (HDMI perhaps), we need to register second struct typec_mux and > >>> implement separate function for that alt mode alone and point the > >>> ->set callback of the second struct typec_mux to that. > >> No, I'm not missing the point... At least not that one :) > >> But I think you are missing my point that a driver for a general > >> purpose mux device will end up having to register a struct typec_mux > >> and implement a ->set function for every possible alternate mode > >> that eventually will exist (and can be used with that mux). > > OK, we are on the same page. So back to my question. I'll rephrase: > > How would separate ->set functions differ from delivering the > > SVID:mode on top of the SVID specific connector state to the mux? You > > still need to handle every alternate mode separately in the mux > > driver. > > My idea, as I tried to explain before, is to use properties for mapping > mux device specific states to svid:mode:state. The mux driver would not > need to know anything about alternate modes, the responsibility would be > with the system architect to get the mapping right in firmware. OK, I understand. I'm a little bit scared about that kind of mappings. At least the connector state value we are dealing with here is Linux kernel specific index number, so I don't know if it's OK to get that from a device property. I wonder if string identifiers would be more acceptable for hardware descriptions? Strings or numbers, I guess we would need to document somewhere to which alternate mode connector state a number/string is mapped to. I don't know if OF bindings is enough? Btw. I'm not convinced we would ever get this information from ACPI tables on devices targeted for windows, but let's not worry about that now. In any case, I'm not against your idea. > > I doubt that HD3SS460 will (or even can) be used with anything else > > except DP. Maybe with HDMI. It definitely will not be usable with all > > alternate modes. Realistically, I think we are talking about two, max > > three alternate modes any mux driver will ever need to handle. > > I'm not so sure. Time will tell. > And using HD3SS460 as an example, there are multiple ways to connect the > DP signals to the MUX that the hw designer may want to take advantage of > to get the best possible lay-out OK. Thanks,
On 2018-05-17 13:50, Heikki Krogerus wrote: > On Wed, May 16, 2018 at 11:11:02PM +0200, Mats Karrman wrote: >> On 05/16/2018 01:43 PM, Heikki Krogerus wrote: >> >>> On Tue, May 15, 2018 at 11:24:07PM +0200, Mats Karrman wrote: >>>> Hi Heikki, >>>> >>>> On 05/15/2018 09:30 AM, Heikki Krogerus wrote: >>>> >>>>> Hi Mats, >>>>> >>>>> On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: >>>>>> On 2018-05-11 13:14, Heikki Krogerus wrote: >>>>>> >>>>>>> On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: >>>>>>>> On 2018-05-10 19:49, Heikki Krogerus wrote: >>>>>>>> >>>>>>>>> On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 05/09/2018 02:49 PM, Heikki Krogerus wrote: >>>>>>>>>> >>>>>>>>>>> On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>> On 05/08/2018 04:25 PM, Heikki Krogerus wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: >>>>>>>>>>>>>>>> Even so, when the mux driver "set" function is called, it will just get the >>>>>>>>>>>>>>>> mode argument but since the mode (TYPEC_STATE_...) is overlapping for different >>>>>>>>>>>>>>>> AMs if I understand your proposal correctly, the mux also needs to know what AM >>>>>>>>>>>>>>>> is active. >>>>>>>>>>>>>>>> Does this imply that the mux set function signature need to change? >>>>>>>>>>>>>>> My plan was actually to propose we get rid of the current mux handling >>>>>>>>>>>>>>> (just leave the orientation switch) in favour of the notifications I'm >>>>>>>>>>>>>>> introducing with the type-c bus for the alternate modes. The current >>>>>>>>>>>>>>> mux handling is definitely not enough, and does not work in every >>>>>>>>>>>>>>> scenario, like also you pointed out. >>>>>>>>>>>>>> So, the mux need to subscribe to each svid:mode pair it is interested in using >>>>>>>>>>>>>> typec_altmode_register_notifier() and then use those callbacks to switch the correct >>>>>>>>>>>>>> signals to the connector. And a driver for an off-the-shelf mux device could have >>>>>>>>>>>>>> the translation between svid:mode pairs and mux device specific control specified by >>>>>>>>>>>>>> of/acpi properties. Right? >>>>>>>>>>>>> Yes. That is the plan. Would it work for you? >>>>>>>>>>>> I think so. I'll give it a go. When about do you think you'll post the next version >>>>>>>>>>>> of your RFC? Or do you have an updated series available somewhere public? >>>>>>>>>>> I'll try to put together and post the next version tomorrow. >>>>>>>>>>> >>>>>>>>>>> My original plan was actually to use just the notifications with the >>>>>>>>>>> muxes, but one thing to consider with the notifications is that in >>>>>>>>>>> practice we have to increment the ref count for the alt mode devices >>>>>>>>>>> when ever something registers a notifier. >>>>>>>>>>> >>>>>>>>>>> To me that does not feel ideal. The dependency should go the other way >>>>>>>>>>> around in case of the muxes. That is why I liked the separate API and >>>>>>>>>>> handling for the muxes felt better, as it will do just that. The mux >>>>>>>>>>> is then a "service" that the port driver can as for, and if it gets a >>>>>>>>>>> handle to a mux, the mux will have its ref count incremented. >>>>>>>>>>> >>>>>>>>>>> So I think fixing the mux API would perhaps be better after all. >>>>>>>>>>> Thoughts? >>>>>>>>>> So, we're back to a mux API similar to the current one, where: >>>>>>>>>> - the port driver and the mux driver are connected through "graph" >>>>>>>>>> - alt mode driver finds its port mux using the typec class mux api >>>>>>>>>> - the mux mode setting function arguments now include both svid and mode >>>>>>>>>> >>>>>>>>>> I like that. >>>>>>>>>> >>>>>>>>>> One thought that popped up again is if we, somewhere down the line, >>>>>>>>>> will see some super device that support many different alternate modes >>>>>>>>>> on the same port and therefore will need to have multiple mux devices? >>>>>>>>>> However I think the mux api could be extended (later on) to support some >>>>>>>>>> aggregate mux device that manages multiple physical devices. >>>>>>>>> If we simply had always a mux for every alternate mode, that would not >>>>>>>>> be a problem. So the port would have its own mux, and every supported >>>>>>>>> alternate mode also had its own. I think that removes the need to deal >>>>>>>>> with the svid:mode when using the muxes, as they are already tied to a >>>>>>>>> specific alternate modes, right? With a single mux device, for example >>>>>>>>> pi3usb30532, the driver just needs to register a mux for the port and >>>>>>>>> separate mux for DP, but I don't think that's a huge problem. >>>>>>>> Hmm... As an hypothetical example I have written a driver for another mux >>>>>>>> from TI and according to its data sheet: >>>>>>>> >>>>>>>> """ >>>>>>>> The HD3SS460 is a generic analog differential >>>>>>>> passive switch that can work for any high speed >>>>>>>> interface applications as long as it is biased at a >>>>>>>> common mode voltage range of 0-2V and has >>>>>>>> differential signaling with differential amplitude up to >>>>>>>> 1800mVpp.... >>>>>>>> """ >>>>>>>> >>>>>>>> What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + >>>>>>>> 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is >>>>>>>> that it is a general mux device so the driver writer does not know what types of >>>>>>>> muxes to register. I guess it could also be configured using properties but that >>>>>>>> would be very complicated. >>>>>>> Why? All the mux driver needs to get from device properties is the >>>>>>> SVID and the mode. >>>>>> Sigh... Again, if the same mux handles signals for more than one alternate mode >>>>>> the driver won't know what alternate mode is intended if it only receives the >>>>>> connector state which use overlapping numbers for different alternate modes. >>>>> You are missing the point. We are now registering separate struct >>>>> typec_mux for every alt mode. The ->set callback will need to be >>>>> implemented separately for every alt mode. >>>>> >>>>> So in case of TI HD3SS460, we need to initially register a struct >>>>> typec_mux for DP and implement a function for the ->set callback for >>>>> DP only. If we later need to support another alt mode with that mux >>>>> (HDMI perhaps), we need to register second struct typec_mux and >>>>> implement separate function for that alt mode alone and point the >>>>> ->set callback of the second struct typec_mux to that. >>>> No, I'm not missing the point... At least not that one :) >>>> But I think you are missing my point that a driver for a general >>>> purpose mux device will end up having to register a struct typec_mux >>>> and implement a ->set function for every possible alternate mode >>>> that eventually will exist (and can be used with that mux). >>> OK, we are on the same page. So back to my question. I'll rephrase: >>> How would separate ->set functions differ from delivering the >>> SVID:mode on top of the SVID specific connector state to the mux? You >>> still need to handle every alternate mode separately in the mux >>> driver. >> My idea, as I tried to explain before, is to use properties for mapping >> mux device specific states to svid:mode:state. The mux driver would not >> need to know anything about alternate modes, the responsibility would be >> with the system architect to get the mapping right in firmware. > OK, I understand. I'm a little bit scared about that kind of mappings. > At least the connector state value we are dealing with here is Linux > kernel specific index number, so I don't know if it's OK to get that > from a device property. I wonder if string identifiers would be more > acceptable for hardware descriptions? Yes, something in the line of: mappings { map { svid = "displayport"; state = "pin-assy-c"; setting = "somethingsomething"; // device driver specific }; map { ...etc... }; Then the svid and state strings could be mapped to numbers by typec/class API and setting strings by the mux driver internally and the mux driver will act on svid:state info from the partner driver(s) tied to the mux. > Strings or numbers, I guess we would need to document somewhere to > which alternate mode connector state a number/string is mapped to. I > don't know if OF bindings is enough? That's what I had in mind. > Btw. I'm not convinced we would ever get this information from ACPI > tables on devices targeted for windows, but let's not worry about that > now. Ah, I admit I did not consider this case. Being used to embeddeed/devicetree systems I just imagined the hardware description as something you are always in control of. I'm afraid I have no idea about this. BR // Mats > In any case, I'm not against your idea. > >>> I doubt that HD3SS460 will (or even can) be used with anything else >>> except DP. Maybe with HDMI. It definitely will not be usable with all >>> alternate modes. Realistically, I think we are talking about two, max >>> three alternate modes any mux driver will ever need to handle. >> I'm not so sure. Time will tell. >> And using HD3SS460 as an example, there are multiple ways to connect the >> DP signals to the MUX that the hw designer may want to take advantage of >> to get the best possible lay-out > OK. > > Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 07:26:40AM +0200, Mats Karrman wrote: > On 2018-05-17 13:50, Heikki Krogerus wrote: > > > On Wed, May 16, 2018 at 11:11:02PM +0200, Mats Karrman wrote: > > > On 05/16/2018 01:43 PM, Heikki Krogerus wrote: > > > > > > > On Tue, May 15, 2018 at 11:24:07PM +0200, Mats Karrman wrote: > > > > > Hi Heikki, > > > > > > > > > > On 05/15/2018 09:30 AM, Heikki Krogerus wrote: > > > > > > > > > > > Hi Mats, > > > > > > > > > > > > On Fri, May 11, 2018 at 09:28:04PM +0200, Mats Karrman wrote: > > > > > > > On 2018-05-11 13:14, Heikki Krogerus wrote: > > > > > > > > > > > > > > > On Fri, May 11, 2018 at 11:05:55AM +0200, Mats Karrman wrote: > > > > > > > > > On 2018-05-10 19:49, Heikki Krogerus wrote: > > > > > > > > > > > > > > > > > > > On Thu, May 10, 2018 at 10:04:21AM +0200, Mats Karrman wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > On 05/09/2018 02:49 PM, Heikki Krogerus wrote: > > > > > > > > > > > > > > > > > > > > > > > On Tue, May 08, 2018 at 09:10:13PM +0200, Mats Karrman wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > On 05/08/2018 04:25 PM, Heikki Krogerus wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, May 07, 2018 at 11:19:40PM +0200, Mats Karrman wrote: > > > > > > > > > > > > > > > > > Even so, when the mux driver "set" function is called, it will just get the > > > > > > > > > > > > > > > > > mode argument but since the mode (TYPEC_STATE_...) is overlapping for different > > > > > > > > > > > > > > > > > AMs if I understand your proposal correctly, the mux also needs to know what AM > > > > > > > > > > > > > > > > > is active. > > > > > > > > > > > > > > > > > Does this imply that the mux set function signature need to change? > > > > > > > > > > > > > > > > My plan was actually to propose we get rid of the current mux handling > > > > > > > > > > > > > > > > (just leave the orientation switch) in favour of the notifications I'm > > > > > > > > > > > > > > > > introducing with the type-c bus for the alternate modes. The current > > > > > > > > > > > > > > > > mux handling is definitely not enough, and does not work in every > > > > > > > > > > > > > > > > scenario, like also you pointed out. > > > > > > > > > > > > > > > So, the mux need to subscribe to each svid:mode pair it is interested in using > > > > > > > > > > > > > > > typec_altmode_register_notifier() and then use those callbacks to switch the correct > > > > > > > > > > > > > > > signals to the connector. And a driver for an off-the-shelf mux device could have > > > > > > > > > > > > > > > the translation between svid:mode pairs and mux device specific control specified by > > > > > > > > > > > > > > > of/acpi properties. Right? > > > > > > > > > > > > > > Yes. That is the plan. Would it work for you? > > > > > > > > > > > > > I think so. I'll give it a go. When about do you think you'll post the next version > > > > > > > > > > > > > of your RFC? Or do you have an updated series available somewhere public? > > > > > > > > > > > > I'll try to put together and post the next version tomorrow. > > > > > > > > > > > > > > > > > > > > > > > > My original plan was actually to use just the notifications with the > > > > > > > > > > > > muxes, but one thing to consider with the notifications is that in > > > > > > > > > > > > practice we have to increment the ref count for the alt mode devices > > > > > > > > > > > > when ever something registers a notifier. > > > > > > > > > > > > > > > > > > > > > > > > To me that does not feel ideal. The dependency should go the other way > > > > > > > > > > > > around in case of the muxes. That is why I liked the separate API and > > > > > > > > > > > > handling for the muxes felt better, as it will do just that. The mux > > > > > > > > > > > > is then a "service" that the port driver can as for, and if it gets a > > > > > > > > > > > > handle to a mux, the mux will have its ref count incremented. > > > > > > > > > > > > > > > > > > > > > > > > So I think fixing the mux API would perhaps be better after all. > > > > > > > > > > > > Thoughts? > > > > > > > > > > > So, we're back to a mux API similar to the current one, where: > > > > > > > > > > > - the port driver and the mux driver are connected through "graph" > > > > > > > > > > > - alt mode driver finds its port mux using the typec class mux api > > > > > > > > > > > - the mux mode setting function arguments now include both svid and mode > > > > > > > > > > > > > > > > > > > > > > I like that. > > > > > > > > > > > > > > > > > > > > > > One thought that popped up again is if we, somewhere down the line, > > > > > > > > > > > will see some super device that support many different alternate modes > > > > > > > > > > > on the same port and therefore will need to have multiple mux devices? > > > > > > > > > > > However I think the mux api could be extended (later on) to support some > > > > > > > > > > > aggregate mux device that manages multiple physical devices. > > > > > > > > > > If we simply had always a mux for every alternate mode, that would not > > > > > > > > > > be a problem. So the port would have its own mux, and every supported > > > > > > > > > > alternate mode also had its own. I think that removes the need to deal > > > > > > > > > > with the svid:mode when using the muxes, as they are already tied to a > > > > > > > > > > specific alternate modes, right? With a single mux device, for example > > > > > > > > > > pi3usb30532, the driver just needs to register a mux for the port and > > > > > > > > > > separate mux for DP, but I don't think that's a huge problem. > > > > > > > > > Hmm... As an hypothetical example I have written a driver for another mux > > > > > > > > > from TI and according to its data sheet: > > > > > > > > > > > > > > > > > > """ > > > > > > > > > The HD3SS460 is a generic analog differential > > > > > > > > > passive switch that can work for any high speed > > > > > > > > > interface applications as long as it is biased at a > > > > > > > > > common mode voltage range of 0-2V and has > > > > > > > > > differential signaling with differential amplitude up to > > > > > > > > > 1800mVpp.... > > > > > > > > > """ > > > > > > > > > > > > > > > > > > What I am thinking is that it e.g. would be possible to use this/a mux with USBSS + > > > > > > > > > 2ch DP + 2ch something else (HDMI?, ThunderBolt?, ???). The problem here is > > > > > > > > > that it is a general mux device so the driver writer does not know what types of > > > > > > > > > muxes to register. I guess it could also be configured using properties but that > > > > > > > > > would be very complicated. > > > > > > > > Why? All the mux driver needs to get from device properties is the > > > > > > > > SVID and the mode. > > > > > > > Sigh... Again, if the same mux handles signals for more than one alternate mode > > > > > > > the driver won't know what alternate mode is intended if it only receives the > > > > > > > connector state which use overlapping numbers for different alternate modes. > > > > > > You are missing the point. We are now registering separate struct > > > > > > typec_mux for every alt mode. The ->set callback will need to be > > > > > > implemented separately for every alt mode. > > > > > > > > > > > > So in case of TI HD3SS460, we need to initially register a struct > > > > > > typec_mux for DP and implement a function for the ->set callback for > > > > > > DP only. If we later need to support another alt mode with that mux > > > > > > (HDMI perhaps), we need to register second struct typec_mux and > > > > > > implement separate function for that alt mode alone and point the > > > > > > ->set callback of the second struct typec_mux to that. > > > > > No, I'm not missing the point... At least not that one :) > > > > > But I think you are missing my point that a driver for a general > > > > > purpose mux device will end up having to register a struct typec_mux > > > > > and implement a ->set function for every possible alternate mode > > > > > that eventually will exist (and can be used with that mux). > > > > OK, we are on the same page. So back to my question. I'll rephrase: > > > > How would separate ->set functions differ from delivering the > > > > SVID:mode on top of the SVID specific connector state to the mux? You > > > > still need to handle every alternate mode separately in the mux > > > > driver. > > > My idea, as I tried to explain before, is to use properties for mapping > > > mux device specific states to svid:mode:state. The mux driver would not > > > need to know anything about alternate modes, the responsibility would be > > > with the system architect to get the mapping right in firmware. > > OK, I understand. I'm a little bit scared about that kind of mappings. > > At least the connector state value we are dealing with here is Linux > > kernel specific index number, so I don't know if it's OK to get that > > from a device property. I wonder if string identifiers would be more > > acceptable for hardware descriptions? > > Yes, something in the line of: > > ?????? mappings { > ?????? ?????? map { > ?????? ?????? ?????? svid = "displayport"; > ?????? ?????? ?????? state = "pin-assy-c"; > ?????? ?????? ?????? setting = "somethingsomething";?????? // device driver specific > ?????? ?????? }; > ?????? ?????? map { > ?????? ?????? ?????? ...etc... > ?????? }; > > Then the svid and state strings could be mapped to numbers by typec/class API > and setting strings by the mux driver internally and the mux driver will act on > svid:state info from the partner driver(s) tied to the mux. > > > Strings or numbers, I guess we would need to document somewhere to > > which alternate mode connector state a number/string is mapped to. I > > don't know if OF bindings is enough? > > That's what I had in mind. > > > Btw. I'm not convinced we would ever get this information from ACPI > > tables on devices targeted for windows, but let's not worry about that > > now. > > Ah, I admit I did not consider this case. Being used to embeddeed/devicetree systems > I just imagined the hardware description as something you are always in control of. > I'm afraid I have no idea about this. It should not prevent us from using the solution. We just can't assume that the HW description can always supply this information, and implement the solution with that in mind as usual. Thanks,
diff --git a/drivers/usb/typec/mux/pi3usb30532.c b/drivers/usb/typec/mux/pi3usb30532.c index b0e88db..279f3c3 100644 --- a/drivers/usb/typec/mux/pi3usb30532.c +++ b/drivers/usb/typec/mux/pi3usb30532.c @@ -83,18 +83,19 @@ static int pi3usb30532_mux_set(struct typec_mux *mux, int state) new_conf = pi->conf; switch (state) { + default: case TYPEC_MUX_NONE: new_conf = PI3USB30532_CONF_OPEN; break; - case TYPEC_MUX_USB: + case TYPEC_MUX_2CH_USBSS: new_conf = (new_conf & PI3USB30532_CONF_SWAP) | PI3USB30532_CONF_USB3; break; - case TYPEC_MUX_DP: + case TYPEC_MUX_4CH_AM: new_conf = (new_conf & PI3USB30532_CONF_SWAP) | PI3USB30532_CONF_4LANE_DP; break; - case TYPEC_MUX_DOCK: + case TYPEC_MUX_2CH_USBSS_2CH_AM: new_conf = (new_conf & PI3USB30532_CONF_SWAP) | PI3USB30532_CONF_USB3_AND_2LANE_DP; break; diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index 7ee417a..0451ea0 100644 --- a/drivers/usb/typec/tcpm.c +++ b/drivers/usb/typec/tcpm.c @@ -790,7 +790,7 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached, else usb_role = USB_ROLE_DEVICE; - ret = tcpm_mux_set(port, TYPEC_MUX_USB, usb_role, orientation); + ret = tcpm_mux_set(port, TYPEC_MUX_2CH_USBSS, usb_role, orientation); if (ret < 0) return ret; diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index b231b93..3518965 100644 --- a/include/linux/usb/tcpm.h +++ b/include/linux/usb/tcpm.h @@ -93,20 +93,19 @@ struct tcpc_config { const struct typec_altmode_desc *alt_modes; }; -/* Mux state attributes */ -#define TCPC_MUX_USB_ENABLED BIT(0) /* USB enabled */ -#define TCPC_MUX_DP_ENABLED BIT(1) /* DP enabled */ -#define TCPC_MUX_POLARITY_INVERTED BIT(2) /* Polarity inverted */ - -/* Mux modes, decoded to attributes */ +/* Mux modes */ enum tcpc_mux_mode { - TYPEC_MUX_NONE = 0, /* Open switch */ - TYPEC_MUX_USB = TCPC_MUX_USB_ENABLED, /* USB only */ - TYPEC_MUX_DP = TCPC_MUX_DP_ENABLED, /* DP only */ - TYPEC_MUX_DOCK = TCPC_MUX_USB_ENABLED | /* Both USB and DP */ - TCPC_MUX_DP_ENABLED, + TYPEC_MUX_NONE, /* Open switch */ + TYPEC_MUX_2CH_USBSS, /* 2ch USB SS */ + TYPEC_MUX_4CH_AM, /* 4ch Alt Mode */ + TYPEC_MUX_2CH_USBSS_2CH_AM, /* 2ch USB SS + 2ch Alt Mode */ + + // Example of additional modes that may be needed in future: + TYPEC_MUX_4CH_USBSS, /* 4ch USB SS */ + TYPEC_MUX_2CH_USBSS_2CH_AM_B, /* 2ch USB SS + 2ch Alt Mode (e.g. DP GPU2) */ }; + /** * struct tcpc_dev - Port configuration and callback functions * @config: Pointer to port configuration
The current naming used for tcpc_mux_mode constants makes too much assumptioms about the usage of the signals. This patch replaces the names with generic names more closely tied to the Type-C specifications and also adds some new ones. At the same time TCPC_MUX_* defines are removed as they do not fit the new concept and currently have no in-tree users. Signed-off-by: Mats Karrman <mats.dev.list@gmail.com> --- drivers/usb/typec/mux/pi3usb30532.c | 7 ++++--- drivers/usb/typec/tcpm.c | 2 +- include/linux/usb/tcpm.h | 21 ++++++++++----------- 3 files changed, 15 insertions(+), 15 deletions(-)