| Message ID | 20231013-fsa4480-swap-v1-2-b877f62046cc@fairphone.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle reversed SBU orientation for FSA4480 | expand |
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
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 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
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,
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");
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(+)