[PATCH 0/3] Handle reversed SBU orientation for FSA4480

[PATCH 0/3] Handle reversed SBU orientation for FSA4480

[Luca Weiss]

Short reason:
Without swapping the SBU lanes, on QCM6490 Fairphone 5 the
DisplayPort-over-USB-C doesn't work.

The Orient-Chip OCP96011 used in this phone is generally compatible with
FSA4480 but has a difference how AUX+/- should be connected to SBU1/2.

Long explanation, with my current understanding:
* FSA4480 block diagram shows AUX+ connected to SBU2 and AUX- to SBU1.
* OCP96011 block diagram shows AUX+ connected to SBU1 and AUX- to SBU2
  (it's not 100% clear though in the picture but makes sense with the
  observed behavior)
* Fairphone 5 schematics have AUX+ connected to SBU2 and AUX- to SBU1,
  which would be correct for FSA4480 but since OCP96011 is used (which
  expects it to be the other way around) the Linux driver needs to
  reverse it.
  If AUX+ would be connected to SBU1 and AUX- to SBU2 as shown in the
  OCP96011 block diagram, then no driver/dts change would be needed.

Not sure if I've implemented the best solution in this patch. Other
solutions I could think of are:
* Add some custom boolean property to the node, e.g. 'fsa,swap-sbu'
* Reverse when ocs,ocp96011 compatible is used. This would be incorrect
  since when following the OCP96011 block diagram no reversing would be
  needed, as explained above.

However I think the current solution with data-lanes in the endpoint is
the best fit and is also already used for a similar purpose in another
USB mux driver.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Luca Weiss (3):
      dt-bindings: usb: fsa4480: Add data-lanes property to endpoint
      usb: typec: fsa4480: Add support to swap SBU orientation
      dt-bindings: usb: fsa4480: Add compatible for OCP96011

 .../devicetree/bindings/usb/fcs,fsa4480.yaml       | 43 +++++++++++-
 drivers/usb/typec/mux/fsa4480.c                    | 81 ++++++++++++++++++++++
 2 files changed, 121 insertions(+), 3 deletions(-)
---
base-commit: e3b18f7200f45d66f7141136c25554ac1e82009b
change-id: 20231013-fsa4480-swap-9b0f76d73c19

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

[PATCH 1/3] dt-bindings: usb: fsa4480: Add data-lanes property to endpoint

[Luca Weiss]

Allow specifying data-lanes to reverse the SBU muxing orientation where
necessary by the hardware design.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 .../devicetree/bindings/usb/fcs,fsa4480.yaml       | 29 +++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
index f6e7a5c1ff0b..86f6d633c2fb 100644
--- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
+++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
@@ -32,10 +32,37 @@ properties:
     type: boolean
 
   port:
-    $ref: /schemas/graph.yaml#/properties/port
+    $ref: /schemas/graph.yaml#/$defs/port-base
     description:
       A port node to link the FSA4480 to a TypeC controller for the purpose of
       handling altmode muxing and orientation switching.
+    unevaluatedProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/graph.yaml#/$defs/endpoint-base
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            description:
+              Specifies how the AUX+/- lines are connected to SBU1/2.
+            oneOf:
+              - items:
+                  - const: 0
+                  - const: 1
+                description: |
+                  Default AUX/SBU layout
+                  - AUX+ connected to SBU2
+                  - AUX- connected to SBU1
+              - items:
+                  - const: 1
+                  - const: 0
+                description: |
+                  Swapped AUX/SBU layout
+                  - AUX+ connected to SBU1
+                  - AUX- connected to SBU2
 
 required:
   - compatible

-- 
2.42.0

Re: [PATCH 1/3] dt-bindings: usb: fsa4480: Add data-lanes property to endpoint

[Rob Herring]

On Fri, Oct 13, 2023 at 01:38:05PM +0200, Luca Weiss wrote:
> Allow specifying data-lanes to reverse the SBU muxing orientation where
> necessary by the hardware design.

What situation in the hardware design makes this necessary. Please 
describe the problem.

> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  .../devicetree/bindings/usb/fcs,fsa4480.yaml       | 29 +++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> index f6e7a5c1ff0b..86f6d633c2fb 100644
> --- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> +++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> @@ -32,10 +32,37 @@ properties:
>      type: boolean
>  
>    port:
> -    $ref: /schemas/graph.yaml#/properties/port
> +    $ref: /schemas/graph.yaml#/$defs/port-base
>      description:
>        A port node to link the FSA4480 to a TypeC controller for the purpose of
>        handling altmode muxing and orientation switching.
> +    unevaluatedProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            description:
> +              Specifies how the AUX+/- lines are connected to SBU1/2.

Doesn't this depend on the connector orientation? Or it is both that and 
the lines can be swapped on the PCB?

Seems like an abuse of data-lanes which already has a definition which 
is not about swapping + and - differential lanes.

Rob

Re: [PATCH 1/3] dt-bindings: usb: fsa4480: Add data-lanes property to endpoint

[Neil Armstrong]

On 16/10/2023 16:22, Rob Herring wrote:
> On Fri, Oct 13, 2023 at 01:38:05PM +0200, Luca Weiss wrote:
>> Allow specifying data-lanes to reverse the SBU muxing orientation where
>> necessary by the hardware design.
> 
> What situation in the hardware design makes this necessary. Please
> describe the problem.
> 
>>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   .../devicetree/bindings/usb/fcs,fsa4480.yaml       | 29 +++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
>> index f6e7a5c1ff0b..86f6d633c2fb 100644
>> --- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
>> +++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
>> @@ -32,10 +32,37 @@ properties:
>>       type: boolean
>>   
>>     port:
>> -    $ref: /schemas/graph.yaml#/properties/port
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>       description:
>>         A port node to link the FSA4480 to a TypeC controller for the purpose of
>>         handling altmode muxing and orientation switching.
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/graph.yaml#/$defs/endpoint-base
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          data-lanes:
>> +            $ref: /schemas/types.yaml#/definitions/uint32-array
>> +            description:
>> +              Specifies how the AUX+/- lines are connected to SBU1/2.
> 
> Doesn't this depend on the connector orientation? Or it is both that and
> the lines can be swapped on the PCB?
> 
> Seems like an abuse of data-lanes which already has a definition which
> is not about swapping + and - differential lanes.

The FSA acts as a mux between DP AUX, Audio lanes on one side and
the USB-C SBU lanes on the other side.
_______          ______
       |          |     |
       |-- HP   --|     |
       |-- MIC  --|     |or
SoC   |          | MUX |-- SBU1 --->  To the USB-C
Codec |-- AUX+ --|     |-- SBU2 --->  connected
       |-- AUX- --|     |
______|          |____ |

The SBU1 & SBU2 are connected to the USB-C connector, and the actual orientation
to the connected devices/cable/whatever is determined by the TPCM and the MUX in
the FSA4480 with be dynamically changed according to the CC1/CC2 detection and PD alt mode.

But on the other side the orientation of the AUX+/AUX- connected to the SoC
is not tied to the USB-C orientation but how it's routed on the PCB.

This describes how the AUX+/AUX- are physically routed to the FSA4480 chip.

Neil

> 
> Rob

Re: [PATCH 1/3] dt-bindings: usb: fsa4480: Add data-lanes property to endpoint

[Rob Herring]

On Mon, Oct 16, 2023 at 04:32:55PM +0200, Neil Armstrong wrote:
> On 16/10/2023 16:22, Rob Herring wrote:
> > On Fri, Oct 13, 2023 at 01:38:05PM +0200, Luca Weiss wrote:
> > > Allow specifying data-lanes to reverse the SBU muxing orientation where
> > > necessary by the hardware design.
> > 
> > What situation in the hardware design makes this necessary. Please
> > describe the problem.
> > 
> > > 
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > >   .../devicetree/bindings/usb/fcs,fsa4480.yaml       | 29 +++++++++++++++++++++-
> > >   1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> > > index f6e7a5c1ff0b..86f6d633c2fb 100644
> > > --- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
> > > @@ -32,10 +32,37 @@ properties:
> > >       type: boolean
> > >     port:
> > > -    $ref: /schemas/graph.yaml#/properties/port
> > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > >       description:
> > >         A port node to link the FSA4480 to a TypeC controller for the purpose of
> > >         handling altmode muxing and orientation switching.
> > > +    unevaluatedProperties: false
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > > +        unevaluatedProperties: false
> > > +
> > > +        properties:
> > > +          data-lanes:
> > > +            $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +            description:
> > > +              Specifies how the AUX+/- lines are connected to SBU1/2.
> > 
> > Doesn't this depend on the connector orientation? Or it is both that and
> > the lines can be swapped on the PCB?
> > 
> > Seems like an abuse of data-lanes which already has a definition which
> > is not about swapping + and - differential lanes.
> 
> The FSA acts as a mux between DP AUX, Audio lanes on one side and
> the USB-C SBU lanes on the other side.
> _______          ______
>       |          |     |
>       |-- HP   --|     |
>       |-- MIC  --|     |or
> SoC   |          | MUX |-- SBU1 --->  To the USB-C
> Codec |-- AUX+ --|     |-- SBU2 --->  connected
>       |-- AUX- --|     |
> ______|          |____ |
> 
> The SBU1 & SBU2 are connected to the USB-C connector, and the actual orientation
> to the connected devices/cable/whatever is determined by the TPCM and the MUX in
> the FSA4480 with be dynamically changed according to the CC1/CC2 detection and PD alt mode.
> 
> But on the other side the orientation of the AUX+/AUX- connected to the SoC
> is not tied to the USB-C orientation but how it's routed on the PCB.
> 
> This describes how the AUX+/AUX- are physically routed to the FSA4480 chip.

I'd hate for this ASCII art to go to waste. Please add this detail to 
the commit message.

Rob

[PATCH 2/3] usb: typec: fsa4480: Add support to swap SBU orientation

[Luca Weiss]

On some hardware designs the AUX+/- lanes are connected reversed to
SBU1/2 compared to the expected design by FSA4480.

Made more complicated, the otherwise compatible Orient-Chip OCP96011
expects the lanes to be connected reversed compared to FSA4480.

* FSA4480 block diagram shows AUX+ connected to SBU2 and AUX- to SBU1.
* OCP96011 block diagram shows AUX+ connected to SBU1 and AUX- to SBU2.

So if OCP96011 is used as drop-in for FSA4480 then the orientation
handling in the driver needs to be reversed to match the expectation of
the OCP96011 hardware.

Support parsing the data-lanes parameter in the endpoint node to swap
this in the driver.

The parse_data_lanes_mapping function is mostly taken from nb7vpq904m.c.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/usb/typec/mux/fsa4480.c | 81 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c
index e0ee1f621abb..6ee467c96fb6 100644
--- a/drivers/usb/typec/mux/fsa4480.c
+++ b/drivers/usb/typec/mux/fsa4480.c
@@ -9,6 +9,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_graph.h>
 #include <linux/regmap.h>
 #include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_mux.h>
@@ -60,6 +61,7 @@ struct fsa4480 {
 	unsigned int svid;
 
 	u8 cur_enable;
+	bool swap_sbu_lanes;
 };
 
 static const struct regmap_config fsa4480_regmap_config = {
@@ -76,6 +78,9 @@ static int fsa4480_set(struct fsa4480 *fsa)
 	u8 enable = FSA4480_ENABLE_DEVICE;
 	u8 sel = 0;
 
+	if (fsa->swap_sbu_lanes)
+		reverse = !reverse;
+
 	/* USB Mode */
 	if (fsa->mode < TYPEC_STATE_MODAL ||
 	    (!fsa->svid && (fsa->mode == TYPEC_MODE_USB2 ||
@@ -179,12 +184,84 @@ static int fsa4480_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *st
 	return ret;
 }
 
+enum {
+	NORMAL_LANE_MAPPING,
+	INVERT_LANE_MAPPING,
+};
+
+#define DATA_LANES_COUNT	2
+
+static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
+	[NORMAL_LANE_MAPPING] = { 0, 1 },
+	[INVERT_LANE_MAPPING] = { 1, 0 },
+};
+
+static int fsa4480_parse_data_lanes_mapping(struct fsa4480 *fsa)
+{
+	struct device_node *ep;
+	u32 data_lanes[DATA_LANES_COUNT];
+	int ret, i, j;
+
+	ep = of_graph_get_next_endpoint(fsa->client->dev.of_node, NULL);
+	if (!ep)
+		return 0;
+
+	ret = of_property_count_u32_elems(ep, "data-lanes");
+	if (ret == -EINVAL)
+		/* Property isn't here, consider default mapping */
+		goto out_done;
+	if (ret < 0)
+		goto out_error;
+
+	if (ret != DATA_LANES_COUNT) {
+		dev_err(&fsa->client->dev, "expected 2 data lanes\n");
+		ret = -EINVAL;
+		goto out_error;
+	}
+
+	ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
+	if (ret)
+		goto out_error;
+
+	for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
+		for (j = 0; j < DATA_LANES_COUNT; j++) {
+			if (data_lanes[j] != supported_data_lane_mapping[i][j])
+				break;
+		}
+
+		if (j == DATA_LANES_COUNT)
+			break;
+	}
+
+	switch (i) {
+	case NORMAL_LANE_MAPPING:
+		break;
+	case INVERT_LANE_MAPPING:
+		fsa->swap_sbu_lanes = true;
+		dev_info(&fsa->client->dev, "using inverted data lanes mapping\n");
+		break;
+	default:
+		dev_err(&fsa->client->dev, "invalid data lanes mapping\n");
+		ret = -EINVAL;
+		goto out_error;
+	}
+
+out_done:
+	ret = 0;
+
+out_error:
+	of_node_put(ep);
+
+	return ret;
+}
+
 static int fsa4480_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct typec_switch_desc sw_desc = { };
 	struct typec_mux_desc mux_desc = { };
 	struct fsa4480 *fsa;
+	int ret;
 
 	fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL);
 	if (!fsa)
@@ -193,6 +270,10 @@ static int fsa4480_probe(struct i2c_client *client)
 	fsa->client = client;
 	mutex_init(&fsa->lock);
 
+	ret = fsa4480_parse_data_lanes_mapping(fsa);
+	if (ret)
+		return ret;
+
 	fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config);
 	if (IS_ERR(fsa->regmap))
 		return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n");

-- 
2.42.0

Re: [PATCH 2/3] usb: typec: fsa4480: Add support to swap SBU orientation

[Neil Armstrong]

On 13/10/2023 13:38, Luca Weiss wrote:
> On some hardware designs the AUX+/- lanes are connected reversed to
> SBU1/2 compared to the expected design by FSA4480.
> 
> Made more complicated, the otherwise compatible Orient-Chip OCP96011
> expects the lanes to be connected reversed compared to FSA4480.
> 
> * FSA4480 block diagram shows AUX+ connected to SBU2 and AUX- to SBU1.
> * OCP96011 block diagram shows AUX+ connected to SBU1 and AUX- to SBU2.
> 
> So if OCP96011 is used as drop-in for FSA4480 then the orientation
> handling in the driver needs to be reversed to match the expectation of
> the OCP96011 hardware.
> 
> Support parsing the data-lanes parameter in the endpoint node to swap
> this in the driver.
> 
> The parse_data_lanes_mapping function is mostly taken from nb7vpq904m.c.

I see the inspiration :-)

> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   drivers/usb/typec/mux/fsa4480.c | 81 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c
> index e0ee1f621abb..6ee467c96fb6 100644
> --- a/drivers/usb/typec/mux/fsa4480.c
> +++ b/drivers/usb/typec/mux/fsa4480.c
> @@ -9,6 +9,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/of_graph.h>
>   #include <linux/regmap.h>
>   #include <linux/usb/typec_dp.h>
>   #include <linux/usb/typec_mux.h>
> @@ -60,6 +61,7 @@ struct fsa4480 {
>   	unsigned int svid;
>   
>   	u8 cur_enable;
> +	bool swap_sbu_lanes;
>   };
>   
>   static const struct regmap_config fsa4480_regmap_config = {
> @@ -76,6 +78,9 @@ static int fsa4480_set(struct fsa4480 *fsa)
>   	u8 enable = FSA4480_ENABLE_DEVICE;
>   	u8 sel = 0;
>   
> +	if (fsa->swap_sbu_lanes)
> +		reverse = !reverse;
> +
>   	/* USB Mode */
>   	if (fsa->mode < TYPEC_STATE_MODAL ||
>   	    (!fsa->svid && (fsa->mode == TYPEC_MODE_USB2 ||
> @@ -179,12 +184,84 @@ static int fsa4480_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *st
>   	return ret;
>   }
>   
> +enum {
> +	NORMAL_LANE_MAPPING,
> +	INVERT_LANE_MAPPING,
> +};
> +
> +#define DATA_LANES_COUNT	2
> +
> +static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
> +	[NORMAL_LANE_MAPPING] = { 0, 1 },
> +	[INVERT_LANE_MAPPING] = { 1, 0 },
> +};
> +
> +static int fsa4480_parse_data_lanes_mapping(struct fsa4480 *fsa)
> +{
> +	struct device_node *ep;
> +	u32 data_lanes[DATA_LANES_COUNT];
> +	int ret, i, j;
> +
> +	ep = of_graph_get_next_endpoint(fsa->client->dev.of_node, NULL);
> +	if (!ep)
> +		return 0;
> +
> +	ret = of_property_count_u32_elems(ep, "data-lanes");
> +	if (ret == -EINVAL)
> +		/* Property isn't here, consider default mapping */
> +		goto out_done;
> +	if (ret < 0)
> +		goto out_error;
> +
> +	if (ret != DATA_LANES_COUNT) {
> +		dev_err(&fsa->client->dev, "expected 2 data lanes\n");
> +		ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
> +	if (ret)
> +		goto out_error;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> +		for (j = 0; j < DATA_LANES_COUNT; j++) {
> +			if (data_lanes[j] != supported_data_lane_mapping[i][j])
> +				break;
> +		}
> +
> +		if (j == DATA_LANES_COUNT)
> +			break;
> +	}
> +
> +	switch (i) {
> +	case NORMAL_LANE_MAPPING:
> +		break;
> +	case INVERT_LANE_MAPPING:
> +		fsa->swap_sbu_lanes = true;
> +		dev_info(&fsa->client->dev, "using inverted data lanes mapping\n");
> +		break;
> +	default:
> +		dev_err(&fsa->client->dev, "invalid data lanes mapping\n");
> +		ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +out_done:
> +	ret = 0;
> +
> +out_error:
> +	of_node_put(ep);
> +
> +	return ret;
> +}

It's probbaly slighly over engineered for 2 lanes, and at some point
this could go into an helper somewhere because we will need it for all
SBU muxes and redrivers/retimers.

> +
>   static int fsa4480_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct typec_switch_desc sw_desc = { };
>   	struct typec_mux_desc mux_desc = { };
>   	struct fsa4480 *fsa;
> +	int ret;
>   
>   	fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL);
>   	if (!fsa)
> @@ -193,6 +270,10 @@ static int fsa4480_probe(struct i2c_client *client)
>   	fsa->client = client;
>   	mutex_init(&fsa->lock);
>   
> +	ret = fsa4480_parse_data_lanes_mapping(fsa);
> +	if (ret)
> +		return ret;
> +
>   	fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config);
>   	if (IS_ERR(fsa->regmap))
>   		return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n");
> 

But since I did the same in nb7vpq904m, and the SBU can be inverted, LGTM

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Neil

Re: [PATCH 2/3] usb: typec: fsa4480: Add support to swap SBU orientation

[Heikki Krogerus]

Hi Luca,

On Fri, Oct 13, 2023 at 01:38:06PM +0200, Luca Weiss wrote:
> On some hardware designs the AUX+/- lanes are connected reversed to
> SBU1/2 compared to the expected design by FSA4480.
> 
> Made more complicated, the otherwise compatible Orient-Chip OCP96011
> expects the lanes to be connected reversed compared to FSA4480.
> 
> * FSA4480 block diagram shows AUX+ connected to SBU2 and AUX- to SBU1.
> * OCP96011 block diagram shows AUX+ connected to SBU1 and AUX- to SBU2.
> 
> So if OCP96011 is used as drop-in for FSA4480 then the orientation
> handling in the driver needs to be reversed to match the expectation of
> the OCP96011 hardware.
> 
> Support parsing the data-lanes parameter in the endpoint node to swap
> this in the driver.
> 
> The parse_data_lanes_mapping function is mostly taken from nb7vpq904m.c.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/usb/typec/mux/fsa4480.c | 81 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c
> index e0ee1f621abb..6ee467c96fb6 100644
> --- a/drivers/usb/typec/mux/fsa4480.c
> +++ b/drivers/usb/typec/mux/fsa4480.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of_graph.h>

If you don't mind, let's keep this driver ready for ACPI, just in
case...

>  #include <linux/regmap.h>
>  #include <linux/usb/typec_dp.h>
>  #include <linux/usb/typec_mux.h>
> @@ -60,6 +61,7 @@ struct fsa4480 {
>  	unsigned int svid;
>  
>  	u8 cur_enable;
> +	bool swap_sbu_lanes;
>  };
>  
>  static const struct regmap_config fsa4480_regmap_config = {
> @@ -76,6 +78,9 @@ static int fsa4480_set(struct fsa4480 *fsa)
>  	u8 enable = FSA4480_ENABLE_DEVICE;
>  	u8 sel = 0;
>  
> +	if (fsa->swap_sbu_lanes)
> +		reverse = !reverse;
> +
>  	/* USB Mode */
>  	if (fsa->mode < TYPEC_STATE_MODAL ||
>  	    (!fsa->svid && (fsa->mode == TYPEC_MODE_USB2 ||
> @@ -179,12 +184,84 @@ static int fsa4480_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *st
>  	return ret;
>  }
>  
> +enum {
> +	NORMAL_LANE_MAPPING,
> +	INVERT_LANE_MAPPING,
> +};
> +
> +#define DATA_LANES_COUNT	2
> +
> +static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
> +	[NORMAL_LANE_MAPPING] = { 0, 1 },
> +	[INVERT_LANE_MAPPING] = { 1, 0 },
> +};
> +
> +static int fsa4480_parse_data_lanes_mapping(struct fsa4480 *fsa)
> +{
> +	struct device_node *ep;

        struct fwnode_handle *ep;

> +	u32 data_lanes[DATA_LANES_COUNT];
> +	int ret, i, j;
> +
> +	ep = of_graph_get_next_endpoint(fsa->client->dev.of_node, NULL);

Shouldn't you loop through the endpoints? In any case:

        ep = fwnode_graph_get_next_endpoint(dev_fwnode(&fsa->client->dev, NULL));

> +	if (!ep)
> +		return 0;
> +
> +	ret = of_property_count_u32_elems(ep, "data-lanes");

        ret = fwnode_property_count_u32(ep, "data-lanes");

But is this necessary at all in this case - why not just read the
array since you expect a fixed size for it (if the read fails it fails)?

> +	if (ret == -EINVAL)
> +		/* Property isn't here, consider default mapping */
> +		goto out_done;
> +	if (ret < 0)
> +		goto out_error;
> +
> +	if (ret != DATA_LANES_COUNT) {
> +		dev_err(&fsa->client->dev, "expected 2 data lanes\n");
> +		ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +	ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);

        ret = fwnode_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);

> +	if (ret)
> +		goto out_error;
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> +		for (j = 0; j < DATA_LANES_COUNT; j++) {
> +			if (data_lanes[j] != supported_data_lane_mapping[i][j])
> +				break;
> +		}
> +
> +		if (j == DATA_LANES_COUNT)
> +			break;
> +	}
> +
> +	switch (i) {
> +	case NORMAL_LANE_MAPPING:
> +		break;
> +	case INVERT_LANE_MAPPING:
> +		fsa->swap_sbu_lanes = true;
> +		dev_info(&fsa->client->dev, "using inverted data lanes mapping\n");

That is just noise. Please drop it.

> +		break;
> +	default:
> +		dev_err(&fsa->client->dev, "invalid data lanes mapping\n");
> +		ret = -EINVAL;
> +		goto out_error;
> +	}
> +
> +out_done:
> +	ret = 0;
> +
> +out_error:
> +	of_node_put(ep);
> +
> +	return ret;
> +}
> +
>  static int fsa4480_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct typec_switch_desc sw_desc = { };
>  	struct typec_mux_desc mux_desc = { };
>  	struct fsa4480 *fsa;
> +	int ret;
>  
>  	fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL);
>  	if (!fsa)
> @@ -193,6 +270,10 @@ static int fsa4480_probe(struct i2c_client *client)
>  	fsa->client = client;
>  	mutex_init(&fsa->lock);
>  
> +	ret = fsa4480_parse_data_lanes_mapping(fsa);
> +	if (ret)
> +		return ret;
> +
>  	fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config);
>  	if (IS_ERR(fsa->regmap))
>  		return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n");
> 
> -- 
> 2.42.0

-- 
heikki

Re: [PATCH 2/3] usb: typec: fsa4480: Add support to swap SBU orientation

[Luca Weiss]

On Tue Oct 17, 2023 at 11:01 AM CEST, Heikki Krogerus wrote:
> Hi Luca,
>
> On Fri, Oct 13, 2023 at 01:38:06PM +0200, Luca Weiss wrote:
> > On some hardware designs the AUX+/- lanes are connected reversed to
> > SBU1/2 compared to the expected design by FSA4480.
> > 
> > Made more complicated, the otherwise compatible Orient-Chip OCP96011
> > expects the lanes to be connected reversed compared to FSA4480.
> > 
> > * FSA4480 block diagram shows AUX+ connected to SBU2 and AUX- to SBU1.
> > * OCP96011 block diagram shows AUX+ connected to SBU1 and AUX- to SBU2.
> > 
> > So if OCP96011 is used as drop-in for FSA4480 then the orientation
> > handling in the driver needs to be reversed to match the expectation of
> > the OCP96011 hardware.
> > 
> > Support parsing the data-lanes parameter in the endpoint node to swap
> > this in the driver.
> > 
> > The parse_data_lanes_mapping function is mostly taken from nb7vpq904m.c.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  drivers/usb/typec/mux/fsa4480.c | 81 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/mux/fsa4480.c b/drivers/usb/typec/mux/fsa4480.c
> > index e0ee1f621abb..6ee467c96fb6 100644
> > --- a/drivers/usb/typec/mux/fsa4480.c
> > +++ b/drivers/usb/typec/mux/fsa4480.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/of_graph.h>
>
> If you don't mind, let's keep this driver ready for ACPI, just in
> case...

I'm quite clueless about any details about ACPI but makes sense to use
the generic APIs.

>
> >  #include <linux/regmap.h>
> >  #include <linux/usb/typec_dp.h>
> >  #include <linux/usb/typec_mux.h>
> > @@ -60,6 +61,7 @@ struct fsa4480 {
> >  	unsigned int svid;
> >  
> >  	u8 cur_enable;
> > +	bool swap_sbu_lanes;
> >  };
> >  
> >  static const struct regmap_config fsa4480_regmap_config = {
> > @@ -76,6 +78,9 @@ static int fsa4480_set(struct fsa4480 *fsa)
> >  	u8 enable = FSA4480_ENABLE_DEVICE;
> >  	u8 sel = 0;
> >  
> > +	if (fsa->swap_sbu_lanes)
> > +		reverse = !reverse;
> > +
> >  	/* USB Mode */
> >  	if (fsa->mode < TYPEC_STATE_MODAL ||
> >  	    (!fsa->svid && (fsa->mode == TYPEC_MODE_USB2 ||
> > @@ -179,12 +184,84 @@ static int fsa4480_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *st
> >  	return ret;
> >  }
> >  
> > +enum {
> > +	NORMAL_LANE_MAPPING,
> > +	INVERT_LANE_MAPPING,
> > +};
> > +
> > +#define DATA_LANES_COUNT	2
> > +
> > +static const int supported_data_lane_mapping[][DATA_LANES_COUNT] = {
> > +	[NORMAL_LANE_MAPPING] = { 0, 1 },
> > +	[INVERT_LANE_MAPPING] = { 1, 0 },
> > +};
> > +
> > +static int fsa4480_parse_data_lanes_mapping(struct fsa4480 *fsa)
> > +{
> > +	struct device_node *ep;
>
>         struct fwnode_handle *ep;
>
> > +	u32 data_lanes[DATA_LANES_COUNT];
> > +	int ret, i, j;
> > +
> > +	ep = of_graph_get_next_endpoint(fsa->client->dev.of_node, NULL);
>
> Shouldn't you loop through the endpoints? In any case:
>
>         ep = fwnode_graph_get_next_endpoint(dev_fwnode(&fsa->client->dev, NULL));

The docs only mention one endpoint so I'm assuming just next_endpoint is
fine?

>
> > +	if (!ep)
> > +		return 0;
> > +
> > +	ret = of_property_count_u32_elems(ep, "data-lanes");
>
>         ret = fwnode_property_count_u32(ep, "data-lanes");
>
> But is this necessary at all in this case - why not just read the
> array since you expect a fixed size for it (if the read fails it fails)?

Hm yeah that should be okay.. Will check the docs
of_property_read_u32_array (or well fwnode_property_read_u32_array) to
see if there's any gotchas if there's less or more elements provided.

Regards
Luca

>
> > +	if (ret == -EINVAL)
> > +		/* Property isn't here, consider default mapping */
> > +		goto out_done;
> > +	if (ret < 0)
> > +		goto out_error;
> > +
> > +	if (ret != DATA_LANES_COUNT) {
> > +		dev_err(&fsa->client->dev, "expected 2 data lanes\n");
> > +		ret = -EINVAL;
> > +		goto out_error;
> > +	}
> > +
> > +	ret = of_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
>
>         ret = fwnode_property_read_u32_array(ep, "data-lanes", data_lanes, DATA_LANES_COUNT);
>
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> > +		for (j = 0; j < DATA_LANES_COUNT; j++) {
> > +			if (data_lanes[j] != supported_data_lane_mapping[i][j])
> > +				break;
> > +		}
> > +
> > +		if (j == DATA_LANES_COUNT)
> > +			break;
> > +	}
> > +
> > +	switch (i) {
> > +	case NORMAL_LANE_MAPPING:
> > +		break;
> > +	case INVERT_LANE_MAPPING:
> > +		fsa->swap_sbu_lanes = true;
> > +		dev_info(&fsa->client->dev, "using inverted data lanes mapping\n");
>
> That is just noise. Please drop it.
>
> > +		break;
> > +	default:
> > +		dev_err(&fsa->client->dev, "invalid data lanes mapping\n");
> > +		ret = -EINVAL;
> > +		goto out_error;
> > +	}
> > +
> > +out_done:
> > +	ret = 0;
> > +
> > +out_error:
> > +	of_node_put(ep);
> > +
> > +	return ret;
> > +}
> > +
> >  static int fsa4480_probe(struct i2c_client *client)
> >  {
> >  	struct device *dev = &client->dev;
> >  	struct typec_switch_desc sw_desc = { };
> >  	struct typec_mux_desc mux_desc = { };
> >  	struct fsa4480 *fsa;
> > +	int ret;
> >  
> >  	fsa = devm_kzalloc(dev, sizeof(*fsa), GFP_KERNEL);
> >  	if (!fsa)
> > @@ -193,6 +270,10 @@ static int fsa4480_probe(struct i2c_client *client)
> >  	fsa->client = client;
> >  	mutex_init(&fsa->lock);
> >  
> > +	ret = fsa4480_parse_data_lanes_mapping(fsa);
> > +	if (ret)
> > +		return ret;
> > +
> >  	fsa->regmap = devm_regmap_init_i2c(client, &fsa4480_regmap_config);
> >  	if (IS_ERR(fsa->regmap))
> >  		return dev_err_probe(dev, PTR_ERR(fsa->regmap), "failed to initialize regmap\n");
> > 
> > -- 
> > 2.42.0

Re: [PATCH 2/3] usb: typec: fsa4480: Add support to swap SBU orientation

[Heikki Krogerus]

Hi Luca,

> > Shouldn't you loop through the endpoints? In any case:
> >
> >         ep = fwnode_graph_get_next_endpoint(dev_fwnode(&fsa->client->dev, NULL));
> 
> The docs only mention one endpoint so I'm assuming just next_endpoint is
> fine?

I'm mostly concerned about what we may have in the future. If one day
you have more than the one connection in your graph, then you have to
be able to identify the endpoint you are after.

But that may not be a problem in this case (maybe that "data-lanes"
device property can be used to identify the correct endpoint?).

We can worry about it then when/if we ever have another endpoint to
deal with.

thanks,

-- 
heikki

[PATCH 3/3] dt-bindings: usb: fsa4480: Add compatible for OCP96011

[Luca Weiss]

The Orient-Chip OCP96011 is generally compatible with the FSA4480, add a
compatible for it with the fallback on fsa4480.

However the AUX/SBU connections are expected to be swapped compared to
FSA4480, so document this in the data-lanes description.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
index 86f6d633c2fb..f9410eb76a62 100644
--- a/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
+++ b/Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml
@@ -11,8 +11,12 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - fcs,fsa4480
+    oneOf:
+      - const: fcs,fsa4480
+      - items:
+          - enum:
+              - ocs,ocp96011
+          - const: fcs,fsa4480
 
   reg:
     maxItems: 1
@@ -53,16 +57,22 @@ properties:
                   - const: 0
                   - const: 1
                 description: |
-                  Default AUX/SBU layout
+                  Default AUX/SBU layout (FSA4480)
                   - AUX+ connected to SBU2
                   - AUX- connected to SBU1
+                  Default AUX/SBU layout (OCP96011)
+                  - AUX+ connected to SBU1
+                  - AUX- connected to SBU2
               - items:
                   - const: 1
                   - const: 0
                 description: |
-                  Swapped AUX/SBU layout
+                  Swapped AUX/SBU layout (FSA4480)
                   - AUX+ connected to SBU1
                   - AUX- connected to SBU2
+                  Swapped AUX/SBU layout (OCP96011)
+                  - AUX+ connected to SBU2
+                  - AUX- connected to SBU1
 
 required:
   - compatible

-- 
2.42.0