Hi,
There is actually only one change to the driver in patch 2/4 where I'm
adding handling for the properties that are used with static/fixed SBU
and HSL line orientation. On top of that, I add firmware documentation
for the driver, and I'm also adding an entry for it to the MAINTAINERS
file in the last patch.
Let me know if you want me to change anything.
thanks,
Heikki Krogerus (4):
usb: typec: Add typec_find_orientation()
usb: typec: mux: intel_pmc_mux: Support for static SBU/HSL orientation
usb: typec: Add firmware documentation for the Intel PMC mux control
MAINTAINERS: Add entry for Intel PMC mux driver
.../firmware-guide/acpi/intel-pmc-mux.rst | 153 ++++++++++++++++++
MAINTAINERS | 7 +
drivers/usb/typec/class.c | 36 +++--
drivers/usb/typec/mux/intel_pmc_mux.c | 42 ++++-
include/linux/usb/typec.h | 1 +
5 files changed, 221 insertions(+), 18 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/intel-pmc-mux.rst
--
2.26.2
The SBU and HSL orientation may be fixed/static from the mux
PoW. Apparently the retimer may take care of the orientation
of these lines. Handling the static SBU (AUX) and HSL
orientation with device properties.
If the SBU orientation is static, a device property
"sbu-orintation" can be used. When the property exists, the
driver always sets the SBU orientation according to the
property value, and when it's not set, the driver uses the
cable plug orientation with SBU.
And with static HSL orientation, "hsl-orientation" device
property can be used in the same way.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/mux/intel_pmc_mux.c | 42 +++++++++++++++++++++++----
1 file changed, 36 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index f5c5e0aef66f..1aac218099f3 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -91,6 +91,9 @@ struct pmc_usb_port {
u8 usb2_port;
u8 usb3_port;
+
+ enum typec_orientation sbu_orientation;
+ enum typec_orientation hsl_orientation;
};
struct pmc_usb {
@@ -99,6 +102,22 @@ struct pmc_usb {
struct pmc_usb_port *port;
};
+static int sbu_orientation(struct pmc_usb_port *port)
+{
+ if (port->sbu_orientation)
+ return port->sbu_orientation - 1;
+
+ return port->orientation - 1;
+}
+
+static int hsl_orientation(struct pmc_usb_port *port)
+{
+ if (port->hsl_orientation)
+ return port->hsl_orientation - 1;
+
+ return port->orientation - 1;
+}
+
static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
{
u8 response[4];
@@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
- req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
- req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
+
+ req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
+ req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
req.mode_data |= (state->mode - TYPEC_STATE_MODAL) <<
PMC_USB_ALTMODE_DP_MODE_SHIFT;
@@ -173,8 +193,9 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
- req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
- req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
+
+ req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
+ req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
if (TBT_ADAPTER(data->device_mode) == TBT_ADAPTER_TBT3)
req.mode_data |= PMC_USB_ALTMODE_TBT_TYPE;
@@ -211,8 +232,8 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
- msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_HSL_SHIFT;
- msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_AUX_SHIFT;
+ msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT;
+ msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT;
return pmc_usb_command(port, msg, sizeof(msg));
}
@@ -296,6 +317,7 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
struct usb_role_switch_desc desc = { };
struct typec_switch_desc sw_desc = { };
struct typec_mux_desc mux_desc = { };
+ const char *str;
int ret;
ret = fwnode_property_read_u8(fwnode, "usb2-port", &port->usb2_port);
@@ -306,6 +328,14 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
if (ret)
return ret;
+ ret = fwnode_property_read_string(fwnode, "sbu-orientation", &str);
+ if (!ret)
+ port->sbu_orientation = typec_find_orientation(str);
+
+ ret = fwnode_property_read_string(fwnode, "hsl-orientation", &str);
+ if (!ret)
+ port->hsl_orientation = typec_find_orientation(str);
+
port->num = index;
port->pmc = pmc;
--
2.26.2
Adding documentation that describes how the PMC mux-agent
function is described in the ACPI tables.
Signed-off-by: Heikki Krogerus <[email protected]>
---
.../firmware-guide/acpi/intel-pmc-mux.rst | 153 ++++++++++++++++++
1 file changed, 153 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/intel-pmc-mux.rst
diff --git a/Documentation/firmware-guide/acpi/intel-pmc-mux.rst b/Documentation/firmware-guide/acpi/intel-pmc-mux.rst
new file mode 100644
index 000000000000..99b86710f02b
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/intel-pmc-mux.rst
@@ -0,0 +1,153 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================
+Intel North Mux-Agent
+=====================
+
+Introduction
+============
+
+North Mux-Agent is a function of the Intel PMC firmware that is supported on
+most Intel based platforms that have the PMC microcontroller. It's used for
+configuring the various USB Multiplexer/DeMultiplexers on the system. The
+platforms that allow the mux-agent to be configured from the operating system
+have an ACPI device object (node) with HID "INTC105C" that represents it.
+
+The North Mux-Agent (aka. Intel PMC Mux Control, or just mux-agent) driver
+communicates with the PMC microcontroller by using the PMC IPC method
+(drivers/platform/x86/intel_scu_ipc.c). The driver registers with the USB Type-C
+Mux Class which allows the USB Type-C Controller and Interface drivers to
+configure the cable plug orientation and mode (with Alternate Modes). The driver
+also registers with the USB Role Class in order to support both USB Host and
+Device modes. The driver is located here: drivers/usb/typec/mux/intel_pmc_mux.c.
+
+Port nodes
+==========
+
+General
+-------
+
+For every USB Type-C connector under the mux-agent control on the system, there
+is a separate child node under the PMC mux-agent device node. Those nodes do not
+represent the actual connectors, but instead the "channels" in the mux-agent
+that are associated with the connectors::
+
+ Scope (_SB.PCI0.PMC.MUX)
+ {
+ Device (CH0)
+ {
+ Name (_ADR, 0)
+ }
+
+ Device (CH1)
+ {
+ Name (_ADR, 1)
+ }
+ }
+
+_PLD (Physical Location of Device)
+----------------------------------
+
+The optional _PLD object can be used with the port (the channel) nodes. If _PLD
+is supplied, it should match the connector node _PLD::
+
+ Scope (_SB.PCI0.PMC.MUX)
+ {
+ Device (CH0)
+ {
+ Name (_ADR, 0)
+ Method (_PLD, 0, NotSerialized)
+ {
+ /* Consider this as pseudocode. */
+ Return (\_SB.USBC.CON0._PLD())
+ }
+ }
+ }
+
+Mux-agent specific _DSD Device Properties
+-----------------------------------------
+
+Port Numbers
+~~~~~~~~~~~~
+
+In order to configure the muxes behind a USB Type-C connector, the PMC firmware
+needs to know the USB2 port and the USB3 port that is associated with the
+connector. The driver extracts the correct port numbers by reading specific _DSD
+device properties named "usb2-port-number" and "usb3-port-number". These
+properties have integer value that means the port index. The port index number
+is 1's based, and value 0 is illegal. The driver uses the numbers extracted from
+these device properties as-is when sending the mux-agent specific messages to
+the PMC::
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {
+ Package () {"usb2-port-number", 6},
+ Package () {"usb3-port-number", 3},
+ },
+ })
+
+Orientation
+~~~~~~~~~~~
+
+Depending on the platform, the data and SBU lines coming from the connector may
+be "fixed" from the mux-agent's point of view, which means the mux-agent driver
+should not configure them according to the cable plug orientation. This can
+happen for example if a retimer on the platform handles the cable plug
+orientation. The driver uses a specific device properties "sbu-orientation"
+(SBU) and "hsl-orientation" (data) to know if those lines are "fixed", and to
+which orientation. The value that these properties have is a string value, and
+it can be one that is defined for the USB Type-C connector orientation: "normal"
+or "reversed"::
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {
+ Package () {"sbu-orientation", "normal"},
+ Package () {"hsl-orientation", "normal"},
+ },
+ })
+
+Example ASL
+===========
+
+The following ASL is an example that shows the mux-agent node, and two
+connectors under its control::
+
+ Scope (_SB.PCI0.PMC)
+ {
+ Device (MUX)
+ {
+ Name (_HID, "INTC105C")
+
+ Device (CH0)
+ {
+ Name (_ADR, 0)
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {
+ Package () {"usb2-port-number", 6},
+ Package () {"usb3-port-number", 3},
+ Package () {"sbu-orientation", "normal"},
+ Package () {"hsl-orientation", "normal"},
+ },
+ })
+ }
+
+ Device (CH1)
+ {
+ Name (_ADR, 1)
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {
+ Package () {"usb2-port-number", 5},
+ Package () {"usb3-port-number", 2},
+ Package () {"sbu-orientation", "normal"},
+ Package () {"hsl-orientation", "normal"},
+ },
+ })
+ }
+ }
+ }
--
2.26.2
I will be maintaining the Intel PMC mux control driver.
Signed-off-by: Heikki Krogerus <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2926327e4976..5a7b0205397b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17590,6 +17590,13 @@ F: Documentation/driver-api/usb/typec.rst
F: drivers/usb/typec/
F: include/linux/usb/typec.h
+USB TYPEC INTEL PMC MUX DRIVER
+M: Heikki Krogerus <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/firmware-guide/acpi/intel-pmc-mux.rst
+F: drivers/usb/typec/mux/intel_pmc_mux.c
+
USB TYPEC PI3USB30532 MUX DRIVER
M: Hans de Goede <[email protected]>
L: [email protected]
--
2.26.2
Hi Heikki,
Thanks for the patches.
On Thu, May 07, 2020 at 06:08:58PM +0300, Heikki Krogerus wrote:
> The SBU and HSL orientation may be fixed/static from the mux
> PoW. Apparently the retimer may take care of the orientation
> of these lines. Handling the static SBU (AUX) and HSL
> orientation with device properties.
>
> If the SBU orientation is static, a device property
> "sbu-orintation" can be used. When the property exists, the
> driver always sets the SBU orientation according to the
> property value, and when it's not set, the driver uses the
> cable plug orientation with SBU.
>
> And with static HSL orientation, "hsl-orientation" device
> property can be used in the same way.
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/mux/intel_pmc_mux.c | 42 +++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index f5c5e0aef66f..1aac218099f3 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -91,6 +91,9 @@ struct pmc_usb_port {
>
> u8 usb2_port;
> u8 usb3_port;
> +
> + enum typec_orientation sbu_orientation;
> + enum typec_orientation hsl_orientation;
> };
>
> struct pmc_usb {
> @@ -99,6 +102,22 @@ struct pmc_usb {
> struct pmc_usb_port *port;
> };
>
> +static int sbu_orientation(struct pmc_usb_port *port)
> +{
> + if (port->sbu_orientation)
> + return port->sbu_orientation - 1;
> +
> + return port->orientation - 1;
> +}
> +
> +static int hsl_orientation(struct pmc_usb_port *port)
> +{
> + if (port->hsl_orientation)
> + return port->hsl_orientation - 1;
> +
> + return port->orientation - 1;
> +}
> +
> static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> {
> u8 response[4];
> @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
>
> req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> +
> + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
I'm curious to know what would happen when sbu-orientation == "normal".
That means |port->sbu_orientation| == 1.
It sounds like what should happen is the AUX_SHIFT orientation
setting should follow what |port->orientation| is, but here it
looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
it should be set to 1 ?
Apologies if I misunderstood the code...
Best regards,
> + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
>
> req.mode_data |= (state->mode - TYPEC_STATE_MODAL) <<
> PMC_USB_ALTMODE_DP_MODE_SHIFT;
> @@ -173,8 +193,9 @@ pmc_usb_mux_tbt(struct pmc_usb_port *port, struct typec_mux_state *state)
>
> req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> +
> + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> + req.mode_data |= hsl_orientation(port) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
>
> if (TBT_ADAPTER(data->device_mode) == TBT_ADAPTER_TBT3)
> req.mode_data |= PMC_USB_ALTMODE_TBT_TYPE;
> @@ -211,8 +232,8 @@ static int pmc_usb_connect(struct pmc_usb_port *port)
> msg[0] |= port->usb3_port << PMC_USB_MSG_USB3_PORT_SHIFT;
>
> msg[1] = port->usb2_port << PMC_USB_MSG_USB2_PORT_SHIFT;
> - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_HSL_SHIFT;
> - msg[1] |= (port->orientation - 1) << PMC_USB_MSG_ORI_AUX_SHIFT;
> + msg[1] |= hsl_orientation(port) << PMC_USB_MSG_ORI_HSL_SHIFT;
> + msg[1] |= sbu_orientation(port) << PMC_USB_MSG_ORI_AUX_SHIFT;
>
> return pmc_usb_command(port, msg, sizeof(msg));
> }
> @@ -296,6 +317,7 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
> struct usb_role_switch_desc desc = { };
> struct typec_switch_desc sw_desc = { };
> struct typec_mux_desc mux_desc = { };
> + const char *str;
> int ret;
>
> ret = fwnode_property_read_u8(fwnode, "usb2-port", &port->usb2_port);
> @@ -306,6 +328,14 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
> if (ret)
> return ret;
>
> + ret = fwnode_property_read_string(fwnode, "sbu-orientation", &str);
> + if (!ret)
> + port->sbu_orientation = typec_find_orientation(str);
> +
> + ret = fwnode_property_read_string(fwnode, "hsl-orientation", &str);
> + if (!ret)
> + port->hsl_orientation = typec_find_orientation(str);
> +
> port->num = index;
> port->pmc = pmc;
>
> --
> 2.26.2
>
Hi Prashant,
On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > +static int sbu_orientation(struct pmc_usb_port *port)
> > +{
> > + if (port->sbu_orientation)
> > + return port->sbu_orientation - 1;
> > +
> > + return port->orientation - 1;
> > +}
> > +
> > +static int hsl_orientation(struct pmc_usb_port *port)
> > +{
> > + if (port->hsl_orientation)
> > + return port->hsl_orientation - 1;
> > +
> > + return port->orientation - 1;
> > +}
> > +
> > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > {
> > u8 response[4];
> > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> >
> > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > +
> > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
>
> I'm curious to know what would happen when sbu-orientation == "normal".
> That means |port->sbu_orientation| == 1.
>
> It sounds like what should happen is the AUX_SHIFT orientation
> setting should follow what |port->orientation| is, but here it
> looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> it should be set to 1 ?
I'll double check this, and get back to you..
Thanks a lot for reviewing this. If you guys have time, then please
check also that the documentation I'm proposing in patch 3/4 for this
driver has everything explained clearly enough, and nothing is missing.
Br,
--
heikki
On Fri, May 08, 2020 at 02:18:40PM +0300, Heikki Krogerus wrote:
> Hi Prashant,
>
> On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > +{
> > > + if (port->sbu_orientation)
> > > + return port->sbu_orientation - 1;
> > > +
> > > + return port->orientation - 1;
> > > +}
> > > +
> > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > +{
> > > + if (port->hsl_orientation)
> > > + return port->hsl_orientation - 1;
> > > +
> > > + return port->orientation - 1;
> > > +}
> > > +
> > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > {
> > > u8 response[4];
> > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > >
> > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > +
> > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> >
> > I'm curious to know what would happen when sbu-orientation == "normal".
> > That means |port->sbu_orientation| == 1.
> >
> > It sounds like what should happen is the AUX_SHIFT orientation
> > setting should follow what |port->orientation| is, but here it
> > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > it should be set to 1 ?
>
> I'll double check this, and get back to you..
>
> Thanks a lot for reviewing this. If you guys have time, then please
> check also that the documentation I'm proposing in patch 3/4 for this
> driver has everything explained clearly enough, and nothing is missing.
>
Sure thing, we'll take a look.
Best,
-Prashant
> Br,
>
> --
> heikki
On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote:
> Hi Prashant,
>
> On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > +{
> > > + if (port->sbu_orientation)
> > > + return port->sbu_orientation - 1;
> > > +
> > > + return port->orientation - 1;
> > > +}
> > > +
> > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > +{
> > > + if (port->hsl_orientation)
> > > + return port->hsl_orientation - 1;
> > > +
> > > + return port->orientation - 1;
> > > +}
> > > +
> > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > {
> > > u8 response[4];
> > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > >
> > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > +
> > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> >
> > I'm curious to know what would happen when sbu-orientation == "normal".
> > That means |port->sbu_orientation| == 1.
> >
> > It sounds like what should happen is the AUX_SHIFT orientation
> > setting should follow what |port->orientation| is, but here it
> > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > it should be set to 1 ?
>
> I'll double check this, and get back to you..
This is not exactly an answer to your question, but it seems that
those bits are only valid if "Alternate-Direct" message is used.
Currently the driver does not support that message.
I think the correct thing to do now is to remove the two lines from
the driver where those bits (ORI-HSL and ORI-Aux) are set.
Let me know if that's OK, and I'll update the series.
thanks,
--
heikki
Hi Heikki,
Thanks a lot for looking into this. Kindly see my response inline:
On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote:
> On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote:
> > Hi Prashant,
> >
> > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > > +{
> > > > + if (port->sbu_orientation)
> > > > + return port->sbu_orientation - 1;
> > > > +
> > > > + return port->orientation - 1;
> > > > +}
> > > > +
> > > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > > +{
> > > > + if (port->hsl_orientation)
> > > > + return port->hsl_orientation - 1;
> > > > +
> > > > + return port->orientation - 1;
> > > > +}
> > > > +
> > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > > {
> > > > u8 response[4];
> > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > > >
> > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > > +
> > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > >
> > > I'm curious to know what would happen when sbu-orientation == "normal".
> > > That means |port->sbu_orientation| == 1.
> > >
> > > It sounds like what should happen is the AUX_SHIFT orientation
> > > setting should follow what |port->orientation| is, but here it
> > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > > it should be set to 1 ?
> >
> > I'll double check this, and get back to you..
>
> This is not exactly an answer to your question, but it seems that
> those bits are only valid if "Alternate-Direct" message is used.
> Currently the driver does not support that message.
Could you kindly provide some detail on when "Alternate-Direct" would be
preferred to the current method?
Also, is there anything on the PMC side which is preventing the use of
"Alternate-Direct" messages? It seems like the state transition diagram
there would be simpler, although I'm likely missing significant details
here.
>
> I think the correct thing to do now is to remove the two lines from
> the driver where those bits (ORI-HSL and ORI-Aux) are set.
I see. How would orientation then be handled in a retimer configuration
where AUX/SBU is flipped by the retimer itself?
Best regards,
-Prashant
>
> Let me know if that's OK, and I'll update the series.
>
> thanks,
>
> --
> heikki
Hi Prashant,
On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote:
> Hi Heikki,
>
> Thanks a lot for looking into this. Kindly see my response inline:
>
> On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote:
> > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote:
> > > Hi Prashant,
> > >
> > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > > > +{
> > > > > + if (port->sbu_orientation)
> > > > > + return port->sbu_orientation - 1;
> > > > > +
> > > > > + return port->orientation - 1;
> > > > > +}
> > > > > +
> > > > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > > > +{
> > > > > + if (port->hsl_orientation)
> > > > > + return port->hsl_orientation - 1;
> > > > > +
> > > > > + return port->orientation - 1;
> > > > > +}
> > > > > +
> > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > > > {
> > > > > u8 response[4];
> > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > > > >
> > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > > > +
> > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > >
> > > > I'm curious to know what would happen when sbu-orientation == "normal".
> > > > That means |port->sbu_orientation| == 1.
> > > >
> > > > It sounds like what should happen is the AUX_SHIFT orientation
> > > > setting should follow what |port->orientation| is, but here it
> > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > > > it should be set to 1 ?
> > >
> > > I'll double check this, and get back to you..
> >
> > This is not exactly an answer to your question, but it seems that
> > those bits are only valid if "Alternate-Direct" message is used.
> > Currently the driver does not support that message.
> Could you kindly provide some detail on when "Alternate-Direct" would be
> preferred to the current method?
Alternate Mode Direct request is supposed to be used if an alternate
mode is entered directly from disconnected state.
> Also, is there anything on the PMC side which is preventing the use of
> "Alternate-Direct" messages? It seems like the state transition diagram
> there would be simpler, although I'm likely missing significant details
> here.
So we actually should use the "direct" request if we are in
disconnected state to enter alt modes if I understood correctly. But
otherwise we should use the normal Alternate Mode request and not the
Alternate Mode "direct" request. And I'm afraid I don't know why.
> > I think the correct thing to do now is to remove the two lines from
> > the driver where those bits (ORI-HSL and ORI-Aux) are set.
> I see. How would orientation then be handled in a retimer configuration
> where AUX/SBU is flipped by the retimer itself?
Note that if we send a separate "connection" request first, then we
already tell the HSL and SBU orientation as part of the payload of
that request. That is why there is no need to tell about the HSL and
SBU orientation with the normal Alternate Mode Request.
So we have already handled the HSL and SBU orientation by the time
this function is called.
thanks,
--
heikki
Hi Heikki,
On Tue, May 12, 2020 at 05:22:51PM +0300, Heikki Krogerus wrote:
> Hi Prashant,
>
> On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote:
> > Hi Heikki,
> >
> > Thanks a lot for looking into this. Kindly see my response inline:
> >
> > On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote:
> > > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote:
> > > > Hi Prashant,
> > > >
> > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > > > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > > > > +{
> > > > > > + if (port->sbu_orientation)
> > > > > > + return port->sbu_orientation - 1;
> > > > > > +
> > > > > > + return port->orientation - 1;
> > > > > > +}
> > > > > > +
> > > > > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > > > > +{
> > > > > > + if (port->hsl_orientation)
> > > > > > + return port->hsl_orientation - 1;
> > > > > > +
> > > > > > + return port->orientation - 1;
> > > > > > +}
> > > > > > +
> > > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > > > > {
> > > > > > u8 response[4];
> > > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > > > > >
> > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > > > > +
> > > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > >
> > > > > I'm curious to know what would happen when sbu-orientation == "normal".
> > > > > That means |port->sbu_orientation| == 1.
> > > > >
> > > > > It sounds like what should happen is the AUX_SHIFT orientation
> > > > > setting should follow what |port->orientation| is, but here it
> > > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > > > > it should be set to 1 ?
> > > >
> > > > I'll double check this, and get back to you..
> > >
> > > This is not exactly an answer to your question, but it seems that
> > > those bits are only valid if "Alternate-Direct" message is used.
> > > Currently the driver does not support that message.
> > Could you kindly provide some detail on when "Alternate-Direct" would be
> > preferred to the current method?
>
> Alternate Mode Direct request is supposed to be used if an alternate
> mode is entered directly from disconnected state.
Ack.
>
> > Also, is there anything on the PMC side which is preventing the use of
> > "Alternate-Direct" messages? It seems like the state transition diagram
> > there would be simpler, although I'm likely missing significant details
> > here.
>
> So we actually should use the "direct" request if we are in
> disconnected state to enter alt modes if I understood correctly. But
> otherwise we should use the normal Alternate Mode request and not the
> Alternate Mode "direct" request. And I'm afraid I don't know why.
SG.
>
> > > I think the correct thing to do now is to remove the two lines from
> > > the driver where those bits (ORI-HSL and ORI-Aux) are set.
> > I see. How would orientation then be handled in a retimer configuration
> > where AUX/SBU is flipped by the retimer itself?
>
> Note that if we send a separate "connection" request first, then we
> already tell the HSL and SBU orientation as part of the payload of
> that request. That is why there is no need to tell about the HSL and
> SBU orientation with the normal Alternate Mode Request.
>
> So we have already handled the HSL and SBU orientation by the time
> this function is called.
Thanks for the explanation. I assume the HSL and SBU bit setting lines
will be removed from pmc_usb_mux_tbt() too?
Best regards,
>
>
> thanks,
>
> --
> heikki
Hi Heikki,
On Tue, May 12, 2020 at 12:19 PM Prashant Malani <[email protected]> wrote:
>
> Hi Heikki,
>
> On Tue, May 12, 2020 at 05:22:51PM +0300, Heikki Krogerus wrote:
> > Hi Prashant,
> >
> > On Mon, May 11, 2020 at 10:57:19AM -0700, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > Thanks a lot for looking into this. Kindly see my response inline:
> > >
> > > On Mon, May 11, 2020 at 04:32:02PM +0300, Heikki Krogerus wrote:
> > > > On Fri, May 08, 2020 at 02:18:44PM +0300, Heikki Krogerus wrote:
> > > > > Hi Prashant,
> > > > >
> > > > > On Thu, May 07, 2020 at 03:40:41PM -0700, Prashant Malani wrote:
> > > > > > > +static int sbu_orientation(struct pmc_usb_port *port)
> > > > > > > +{
> > > > > > > + if (port->sbu_orientation)
> > > > > > > + return port->sbu_orientation - 1;
> > > > > > > +
> > > > > > > + return port->orientation - 1;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int hsl_orientation(struct pmc_usb_port *port)
> > > > > > > +{
> > > > > > > + if (port->hsl_orientation)
> > > > > > > + return port->hsl_orientation - 1;
> > > > > > > +
> > > > > > > + return port->orientation - 1;
> > > > > > > +}
> > > > > > > +
> > > > > > > static int pmc_usb_command(struct pmc_usb_port *port, u8 *msg, u32 len)
> > > > > > > {
> > > > > > > u8 response[4];
> > > > > > > @@ -151,8 +170,9 @@ pmc_usb_mux_dp(struct pmc_usb_port *port, struct typec_mux_state *state)
> > > > > > >
> > > > > > > req.mode_data = (port->orientation - 1) << PMC_USB_ALTMODE_ORI_SHIFT;
> > > > > > > req.mode_data |= (port->role - 1) << PMC_USB_ALTMODE_UFP_SHIFT;
> > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > > > > - req.mode_data |= (port->orientation - 1) << PMC_USB_ALTMODE_ORI_HSL_SHIFT;
> > > > > > > +
> > > > > > > + req.mode_data |= sbu_orientation(port) << PMC_USB_ALTMODE_ORI_AUX_SHIFT;
> > > > > >
> > > > > > I'm curious to know what would happen when sbu-orientation == "normal".
> > > > > > That means |port->sbu_orientation| == 1.
> > > > > >
> > > > > > It sounds like what should happen is the AUX_SHIFT orientation
> > > > > > setting should follow what |port->orientation| is, but here it
> > > > > > looks like it will always be set to |port->sbu_orientation - 1|, i.e 0,
> > > > > > even if port->orientation == TYPEC_ORIENTATION_REVERSE, i.e 2, meaning
> > > > > > it should be set to 1 ?
> > > > >
> > > > > I'll double check this, and get back to you..
> > > >
> > > > This is not exactly an answer to your question, but it seems that
> > > > those bits are only valid if "Alternate-Direct" message is used.
> > > > Currently the driver does not support that message.
> > > Could you kindly provide some detail on when "Alternate-Direct" would be
> > > preferred to the current method?
> >
> > Alternate Mode Direct request is supposed to be used if an alternate
> > mode is entered directly from disconnected state.
>
> Ack.
> >
> > > Also, is there anything on the PMC side which is preventing the use of
> > > "Alternate-Direct" messages? It seems like the state transition diagram
> > > there would be simpler, although I'm likely missing significant details
> > > here.
> >
> > So we actually should use the "direct" request if we are in
> > disconnected state to enter alt modes if I understood correctly. But
> > otherwise we should use the normal Alternate Mode request and not the
> > Alternate Mode "direct" request. And I'm afraid I don't know why.
>
> SG.
> >
> > > > I think the correct thing to do now is to remove the two lines from
> > > > the driver where those bits (ORI-HSL and ORI-Aux) are set.
> > > I see. How would orientation then be handled in a retimer configuration
> > > where AUX/SBU is flipped by the retimer itself?
> >
> > Note that if we send a separate "connection" request first, then we
> > already tell the HSL and SBU orientation as part of the payload of
> > that request. That is why there is no need to tell about the HSL and
> > SBU orientation with the normal Alternate Mode Request.
> >
> > So we have already handled the HSL and SBU orientation by the time
> > this function is called.
>
> Thanks for the explanation. I assume the HSL and SBU bit setting lines
> will be removed from pmc_usb_mux_tbt() too?
>
I just realized, the issue I initially pointed out would apply to the
connect message too, i.e I'm not sure if "normal" orientation setting
handles the case where port orientation is reversed correctly.
Overall, I am not sure that re-using the typec_orientations[] string
list is the best option for this use-case.
we're looking for "normal" (i.e follows port->orientation) and "fixed"
(i.e is always the same orientation, regardless of what
port->orientation is), so it is perhaps better to just define a new
array just for this driver.
> Best regards,
> >
> >
> > thanks,
> >
> > --
> > heikki
Hi Prashant,
> I just realized, the issue I initially pointed out would apply to the
> connect message too, i.e I'm not sure if "normal" orientation setting
> handles the case where port orientation is reversed correctly.
> Overall, I am not sure that re-using the typec_orientations[] string
> list is the best option for this use-case.
> we're looking for "normal" (i.e follows port->orientation) and "fixed"
> (i.e is always the same orientation, regardless of what
> port->orientation is), so it is perhaps better to just define a new
> array just for this driver.
Sorry, I got sidetracked with that Alternate-Direct Request stuff.
Let's start over..
The property itself is the indicator that the orientation is fixed for
those lines, not its value. If the property exists, we know the
orientation is fixed, and if it doesn't exist, we know we need to use
the cable plug orientation. So if we only want to use the property as
a flag, then it does not need to have any value at all. It would be a
boolean property.
But we would then always leave the ORI-HSL field with value 0 when the
orientation is fixed, and that would rule out the possibility of
supporting a platform where we have to use a fixed value of 1 there
(fixed-reversed). If we ever needed to support configuration like
that, then we would need to add a new property.
That scenario may not be relevant on this platform, and it may seem
like an unlikely, or even impossible case now, but experience (and the
north mux-agent documentation) tells me we should prepare also for
that. That is why I use the typec_orientation strings as the values
for these properties. Then the fixed-reversed orientation is also
covered with the same properties if we ever need to support it.
Maybe this code would be better, or more clear in the driver:
/*
* Returns the value for the HSL-ORI field.
*/
static int hsl_orientation(struct pmc_usb_port *port)
{
enum typec_orientation orientation;
/*
* Is the orientation fixed:
* Yes, use the fixed orientation.
* No, use cable plug orientation.
*/
if (port->hsl_orientation)
orientation = hsl_orientation;
else
orientation = port->orientation;
switch (orientation) {
case TYPEC_ORIENTATION_NORMAL:
return 0;
case TYPEC_ORIENTATION_REVERSE:
return 1;
}
return -EINVAL;
}
thanks,
--
heikki