| Message ID | 20231013-fsa4480-swap-v1-0-b877f62046cc@fairphone.com |
|---|---|
| Headers | show |
| Series | Handle reversed SBU orientation for FSA4480 | expand |
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
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
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
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
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,
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,