| Message ID | 20211228052116.1748443-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | typec: mux: Introduce support for multiple TypeC muxes | expand |
Hi Bjorn, On 12/28/21 06:21, Bjorn Andersson wrote: > This series introduces a level of indirection between the controller's view of > a typec_mux/switch and the implementation and then expands that to support > multiple drivers. > > This is needed in order to support devices such as the Qualcomm Snapdragon 888 > HDK, which does muxing and orientation handling in the QMP (USB+DP) PHY and SBU > muxing in the external FSA4480 chip. > > Additionally integration of typec mux and switch in the QMP PHY is included in > the series, as is the new FSA4480 driver. This is done to deal with the > renaming of the driver-side typec_mux -> typec_mux_dev. > > Bjorn Andersson (8): > dt-bindings: phy: qcom,qmp-usb3-dp: Add altmode/switch properties > phy: qcom-qmp: Register typec mux and orientation switch > device property: Helper to match multiple connections > device property: Use multi-connection matchers for single case > typec: mux: Introduce indirection > typec: mux: Allow multiple mux_devs per mux > dt-bindings: usb: Add binding for fcs,fsa4480 > usb: typec: mux: Add On Semi fsa4480 driver Thank you for your series, I will leave commenting on the dt-bindings and typec-mux changes to others. But what I can do is test this on an x86 device using a pi3usb30532 mux for USB super-speed and DP-alt-mode muxing / orientation switching. I'm going to wait a bit with doing that till this has had some reviews and possibly also some newer versions because of those reviews. If you haven't received a Tested-by from me when this looks like it is ready for merging please ping me. Regards, Hans > > .../bindings/phy/qcom,qmp-usb3-dp-phy.yaml | 14 + > .../devicetree/bindings/usb/fcs,fsa4480.yaml | 72 +++++ > drivers/base/property.c | 83 ++++-- > drivers/phy/qualcomm/phy-qcom-qmp.c | 176 ++++++++++-- > drivers/usb/typec/bus.c | 2 +- > drivers/usb/typec/mux.c | 257 +++++++++++++----- > drivers/usb/typec/mux.h | 12 +- > drivers/usb/typec/mux/Kconfig | 9 + > drivers/usb/typec/mux/Makefile | 1 + > drivers/usb/typec/mux/fsa4480.c | 220 +++++++++++++++ > drivers/usb/typec/mux/intel_pmc_mux.c | 8 +- > drivers/usb/typec/mux/pi3usb30532.c | 8 +- > include/linux/property.h | 5 + > include/linux/usb/typec_mux.h | 22 +- > 14 files changed, 762 insertions(+), 127 deletions(-) > create mode 100644 Documentation/devicetree/bindings/usb/fcs,fsa4480.yaml > create mode 100644 drivers/usb/typec/mux/fsa4480.c >
On 28/12/2021 08:21, Bjorn Andersson wrote: > In some cases multiple connections with the same connection id > needs to be resolved from a fwnode graph. > > One such example is when separate hardware is used for performing muxing and/or > orientation switching of the SuperSpeed and SBU lines in a USB-C > connector. In this case the connector needs to belong to a graph with > multiple matching remote endpoints, and the TypeC controller needs to be > able to resolve them both. > > Add a new API that allows this kind of lookup. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/base/property.c | 94 ++++++++++++++++++++++++++++++++++++++++ > include/linux/property.h | 5 +++ > 2 files changed, 99 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index cbe4fa298413..0aa0296fd991 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -1180,6 +1180,36 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > return NULL; > } > > +static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > + const char *con_id, void *data, > + devcon_match_fn_t match, > + void **matches, > + unsigned int matches_len) > +{ > + struct fwnode_handle *node; > + struct fwnode_handle *ep; > + unsigned int count = 0; > + void *ret; > + > + fwnode_graph_for_each_endpoint(fwnode, ep) { > + if (count >= matches_len) { > + fwnode_handle_put(ep); > + return count; > + } > + > + node = fwnode_graph_get_remote_port_parent(ep); > + if (!fwnode_device_is_available(node)) > + continue; > + > + ret = match(node, con_id, data); > + fwnode_handle_put(node); > + > + if (ret) > + matches[count++] = ret; > + } > + return count; > +} This API doesn't let it's user know if there are more matches found in the device tree or not. I'd suggest to add 'count' mode that would return the amount of found matches if (matches == NULL) && (matches_len == 0). > + > static void * > fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > void *data, devcon_match_fn_t match) > @@ -1202,6 +1232,35 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > return NULL; > } > > +static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode, > + const char *con_id, void *data, > + devcon_match_fn_t match, > + void **matches, > + unsigned int matches_len) > +{ > + struct fwnode_handle *node; > + unsigned int count = 0; > + void *ret; > + int i; > + > + for (i = 0; ; i++) { > + if (count >= matches_len) > + return count; > + > + node = fwnode_find_reference(fwnode, con_id, i); > + if (IS_ERR(node)) > + break; > + > + ret = match(node, NULL, data); > + fwnode_handle_put(node); > + > + if (ret) > + matches[count++] = ret; > + } > + > + return count; > +} > + Same comment applies. > /** > * fwnode_connection_find_match - Find connection from a device node > * @fwnode: Device node with the connection > @@ -1229,3 +1288,38 @@ void *fwnode_connection_find_match(struct fwnode_handle *fwnode, > return fwnode_devcon_match(fwnode, con_id, data, match); > } > EXPORT_SYMBOL_GPL(fwnode_connection_find_match); > + > +/** > + * fwnode_connection_find_matches - Find connections from a device node > + * @fwnode: Device node with the connection > + * @con_id: Identifier for the connection > + * @data: Data for the match function > + * @match: Function to check and convert the connection description > + * @matches: Array of pointers to fill with matches > + * @matches_len: Length of @matches > + * > + * Find up to @matches_len connections with unique identifier @con_id between > + * @fwnode and other device nodes. @match will be used to convert the > + * connection description to data the caller is expecting to be returned > + * through the @matches array. > + * > + * Return: Number of matches resolved, of negative errno. > + */ > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > + const char *con_id, void *data, > + devcon_match_fn_t match, > + void **matches, unsigned int matches_len) > +{ > + unsigned int count; > + > + if (!fwnode || !match || !matches) > + return -EINVAL; > + > + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, > + matches, matches_len); > + > + return count + fwnode_devcon_matches(fwnode, con_id, data, match, > + matches + count, > + matches_len - count); > +} > +EXPORT_SYMBOL_GPL(fwnode_connection_find_matches); > diff --git a/include/linux/property.h b/include/linux/property.h > index 16f736c698a2..59484ccb260e 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -444,6 +444,11 @@ static inline void *device_connection_find_match(struct device *dev, > return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match); > } > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > + const char *con_id, void *data, > + devcon_match_fn_t match, > + void **matches, unsigned int matches_len); > + > /* -------------------------------------------------------------------------- */ > /* Software fwnode support - when HW description is incomplete or missing */ >
On Tue 28 Dec 05:09 PST 2021, Dmitry Baryshkov wrote: > On 28/12/2021 08:21, Bjorn Andersson wrote: > > In some cases multiple connections with the same connection id > > needs to be resolved from a fwnode graph. > > > > One such example is when separate hardware is used for performing muxing and/or > > orientation switching of the SuperSpeed and SBU lines in a USB-C > > connector. In this case the connector needs to belong to a graph with > > multiple matching remote endpoints, and the TypeC controller needs to be > > able to resolve them both. > > > > Add a new API that allows this kind of lookup. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/base/property.c | 94 ++++++++++++++++++++++++++++++++++++++++ > > include/linux/property.h | 5 +++ > > 2 files changed, 99 insertions(+) > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > index cbe4fa298413..0aa0296fd991 100644 > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -1180,6 +1180,36 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > return NULL; > > } > > +static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > > + const char *con_id, void *data, > > + devcon_match_fn_t match, > > + void **matches, > > + unsigned int matches_len) > > +{ > > + struct fwnode_handle *node; > > + struct fwnode_handle *ep; > > + unsigned int count = 0; > > + void *ret; > > + > > + fwnode_graph_for_each_endpoint(fwnode, ep) { > > + if (count >= matches_len) { > > + fwnode_handle_put(ep); > > + return count; > > + } > > + > > + node = fwnode_graph_get_remote_port_parent(ep); > > + if (!fwnode_device_is_available(node)) > > + continue; > > + > > + ret = match(node, con_id, data); > > + fwnode_handle_put(node); > > + > > + if (ret) > > + matches[count++] = ret; > > + } > > + return count; > > +} > > This API doesn't let it's user know if there are more matches found in the > device tree or not. I'd suggest to add 'count' mode that would return the > amount of found matches if (matches == NULL) && (matches_len == 0). > Unfortunately in this code path we don't know how to "free" the objects returned by match(), e.g. see how typec_switch_match() returns wrapper of a refcounted device. So we must return all the match results to the caller to it can free things up based on its knowledge of what matches[] actually contains.. Regards, Bjorn > > + > > static void * > > fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > void *data, devcon_match_fn_t match) > > @@ -1202,6 +1232,35 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > return NULL; > > } > > +static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode, > > + const char *con_id, void *data, > > + devcon_match_fn_t match, > > + void **matches, > > + unsigned int matches_len) > > +{ > > + struct fwnode_handle *node; > > + unsigned int count = 0; > > + void *ret; > > + int i; > > + > > + for (i = 0; ; i++) { > > + if (count >= matches_len) > > + return count; > > + > > + node = fwnode_find_reference(fwnode, con_id, i); > > + if (IS_ERR(node)) > > + break; > > + > > + ret = match(node, NULL, data); > > + fwnode_handle_put(node); > > + > > + if (ret) > > + matches[count++] = ret; > > + } > > + > > + return count; > > +} > > + > > Same comment applies. > > > /** > > * fwnode_connection_find_match - Find connection from a device node > > * @fwnode: Device node with the connection > > @@ -1229,3 +1288,38 @@ void *fwnode_connection_find_match(struct fwnode_handle *fwnode, > > return fwnode_devcon_match(fwnode, con_id, data, match); > > } > > EXPORT_SYMBOL_GPL(fwnode_connection_find_match); > > + > > +/** > > + * fwnode_connection_find_matches - Find connections from a device node > > + * @fwnode: Device node with the connection > > + * @con_id: Identifier for the connection > > + * @data: Data for the match function > > + * @match: Function to check and convert the connection description > > + * @matches: Array of pointers to fill with matches > > + * @matches_len: Length of @matches > > + * > > + * Find up to @matches_len connections with unique identifier @con_id between > > + * @fwnode and other device nodes. @match will be used to convert the > > + * connection description to data the caller is expecting to be returned > > + * through the @matches array. > > + * > > + * Return: Number of matches resolved, of negative errno. > > + */ > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > + const char *con_id, void *data, > > + devcon_match_fn_t match, > > + void **matches, unsigned int matches_len) > > +{ > > + unsigned int count; > > + > > + if (!fwnode || !match || !matches) > > + return -EINVAL; > > + > > + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, > > + matches, matches_len); > > + > > + return count + fwnode_devcon_matches(fwnode, con_id, data, match, > > + matches + count, > > + matches_len - count); > > +} > > +EXPORT_SYMBOL_GPL(fwnode_connection_find_matches); > > diff --git a/include/linux/property.h b/include/linux/property.h > > index 16f736c698a2..59484ccb260e 100644 > > --- a/include/linux/property.h > > +++ b/include/linux/property.h > > @@ -444,6 +444,11 @@ static inline void *device_connection_find_match(struct device *dev, > > return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match); > > } > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > + const char *con_id, void *data, > > + devcon_match_fn_t match, > > + void **matches, unsigned int matches_len); > > + > > /* -------------------------------------------------------------------------- */ > > /* Software fwnode support - when HW description is incomplete or missing */ > > > -- > With best wishes > Dmitry
On Tue, 28 Dec 2021 at 20:03, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Tue 28 Dec 05:09 PST 2021, Dmitry Baryshkov wrote: > > > On 28/12/2021 08:21, Bjorn Andersson wrote: > > > In some cases multiple connections with the same connection id > > > needs to be resolved from a fwnode graph. > > > > > > One such example is when separate hardware is used for performing muxing and/or > > > orientation switching of the SuperSpeed and SBU lines in a USB-C > > > connector. In this case the connector needs to belong to a graph with > > > multiple matching remote endpoints, and the TypeC controller needs to be > > > able to resolve them both. > > > > > > Add a new API that allows this kind of lookup. > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > drivers/base/property.c | 94 ++++++++++++++++++++++++++++++++++++++++ > > > include/linux/property.h | 5 +++ > > > 2 files changed, 99 insertions(+) > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > > index cbe4fa298413..0aa0296fd991 100644 > > > --- a/drivers/base/property.c > > > +++ b/drivers/base/property.c > > > @@ -1180,6 +1180,36 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > > return NULL; > > > } > > > +static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, > > > + const char *con_id, void *data, > > > + devcon_match_fn_t match, > > > + void **matches, > > > + unsigned int matches_len) > > > +{ > > > + struct fwnode_handle *node; > > > + struct fwnode_handle *ep; > > > + unsigned int count = 0; > > > + void *ret; > > > + > > > + fwnode_graph_for_each_endpoint(fwnode, ep) { > > > + if (count >= matches_len) { > > > + fwnode_handle_put(ep); > > > + return count; > > > + } > > > + > > > + node = fwnode_graph_get_remote_port_parent(ep); > > > + if (!fwnode_device_is_available(node)) > > > + continue; > > > + > > > + ret = match(node, con_id, data); > > > + fwnode_handle_put(node); > > > + > > > + if (ret) > > > + matches[count++] = ret; > > > + } > > > + return count; > > > +} > > > > This API doesn't let it's user know if there are more matches found in the > > device tree or not. I'd suggest to add 'count' mode that would return the > > amount of found matches if (matches == NULL) && (matches_len == 0). > > > > Unfortunately in this code path we don't know how to "free" the objects > returned by match(), e.g. see how typec_switch_match() returns wrapper > of a refcounted device. > > So we must return all the match results to the caller to it can free > things up based on its knowledge of what matches[] actually contains.. Ugh. Then we should probably return -E2BIG, -ENOSPC or any other such error. Another option might be to split match into match & map functions, first one returning bool and second one returning actual corresponding object.. > > Regards, > Bjorn > > > > + > > > static void * > > > fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > > void *data, devcon_match_fn_t match) > > > @@ -1202,6 +1232,35 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, > > > return NULL; > > > } > > > +static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode, > > > + const char *con_id, void *data, > > > + devcon_match_fn_t match, > > > + void **matches, > > > + unsigned int matches_len) > > > +{ > > > + struct fwnode_handle *node; > > > + unsigned int count = 0; > > > + void *ret; > > > + int i; > > > + > > > + for (i = 0; ; i++) { > > > + if (count >= matches_len) > > > + return count; > > > + > > > + node = fwnode_find_reference(fwnode, con_id, i); > > > + if (IS_ERR(node)) > > > + break; > > > + > > > + ret = match(node, NULL, data); > > > + fwnode_handle_put(node); > > > + > > > + if (ret) > > > + matches[count++] = ret; > > > + } > > > + > > > + return count; > > > +} > > > + > > > > Same comment applies. > > > > > /** > > > * fwnode_connection_find_match - Find connection from a device node > > > * @fwnode: Device node with the connection > > > @@ -1229,3 +1288,38 @@ void *fwnode_connection_find_match(struct fwnode_handle *fwnode, > > > return fwnode_devcon_match(fwnode, con_id, data, match); > > > } > > > EXPORT_SYMBOL_GPL(fwnode_connection_find_match); > > > + > > > +/** > > > + * fwnode_connection_find_matches - Find connections from a device node > > > + * @fwnode: Device node with the connection > > > + * @con_id: Identifier for the connection > > > + * @data: Data for the match function > > > + * @match: Function to check and convert the connection description > > > + * @matches: Array of pointers to fill with matches > > > + * @matches_len: Length of @matches > > > + * > > > + * Find up to @matches_len connections with unique identifier @con_id between > > > + * @fwnode and other device nodes. @match will be used to convert the > > > + * connection description to data the caller is expecting to be returned > > > + * through the @matches array. > > > + * > > > + * Return: Number of matches resolved, of negative errno. > > > + */ > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > + const char *con_id, void *data, > > > + devcon_match_fn_t match, > > > + void **matches, unsigned int matches_len) > > > +{ > > > + unsigned int count; > > > + > > > + if (!fwnode || !match || !matches) > > > + return -EINVAL; > > > + > > > + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, > > > + matches, matches_len); > > > + > > > + return count + fwnode_devcon_matches(fwnode, con_id, data, match, > > > + matches + count, > > > + matches_len - count); > > > +} > > > +EXPORT_SYMBOL_GPL(fwnode_connection_find_matches); > > > diff --git a/include/linux/property.h b/include/linux/property.h > > > index 16f736c698a2..59484ccb260e 100644 > > > --- a/include/linux/property.h > > > +++ b/include/linux/property.h > > > @@ -444,6 +444,11 @@ static inline void *device_connection_find_match(struct device *dev, > > > return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match); > > > } > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > + const char *con_id, void *data, > > > + devcon_match_fn_t match, > > > + void **matches, unsigned int matches_len); > > > + > > > /* -------------------------------------------------------------------------- */ > > > /* Software fwnode support - when HW description is incomplete or missing */ > > > > > > -- > > With best wishes > > Dmitry
On 27-12-21, 21:21, Bjorn Andersson wrote: > The QMP PHY handles muxing of USB vs DisplayPort, as well as orientation > switching of the SuperSpeed lanes. So register typec handlers for the > two types. > > The TypeC mux allows switching between four lanes of DisplayPort and a > mixed USB+DP combination. This makes it possible to reach resolutions > that requires 4 lanes. > > The TypeC switch allows switching the SuperSpeed pins and have been > tested with both 2 and 4 lane DisplayPort. > > It's possible that in the USB mode the DP_MODE should be disabled, but > this is left untouched. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > My suggestion is that if/once this patch is deemed acceptable the PHY > maintainers could create a immutable branch/tag which can be merged into the > PHY tree as well as the USB tree. > > drivers/phy/qualcomm/phy-qcom-qmp.c | 176 +++++++++++++++++++++++++--- > 1 file changed, 158 insertions(+), 18 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 7bea6a60dc54..8d8139df9d8e 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -19,6 +19,8 @@ > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > #include <linux/slab.h> > +#include <linux/usb/typec_dp.h> > +#include <linux/usb/typec_mux.h> > > #include <dt-bindings/phy/phy.h> > > @@ -3017,6 +3019,9 @@ struct qmp_phy_dp_clks { > * @phy_mutex: mutex lock for PHY common block initialization > * @init_count: phy common block initialization count > * @ufs_reset: optional UFS PHY reset handle > + * @sw: typec switch for receiving orientation changes > + * @mux: typec mux for DP muxing > + * @orientation: carries current CC orientation > */ > struct qcom_qmp { > struct device *dev; > @@ -3032,6 +3037,10 @@ struct qcom_qmp { > int init_count; > > struct reset_control *ufs_reset; > + > + struct typec_switch *sw; > + struct typec_mux *mux; > + enum typec_orientation orientation; > }; > > static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_phy *qphy); > @@ -4378,30 +4387,23 @@ static void qcom_qmp_v3_phy_configure_dp_tx(struct qmp_phy *qphy) > > static bool qcom_qmp_phy_configure_dp_mode(struct qmp_phy *qphy) > { > + const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts; > + bool reverse = qphy->qmp->orientation == TYPEC_ORIENTATION_REVERSE; > u32 val; > - bool reverse = false; > > val = DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN; > > - /* > - * TODO: Assume orientation is CC1 for now and two lanes, need to > - * use type-c connector to understand orientation and lanes. > - * > - * Otherwise val changes to be like below if this code understood > - * the orientation of the type-c cable. > - * > - * if (lane_cnt == 4 || orientation == ORIENTATION_CC2) > - * val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; > - * if (lane_cnt == 4 || orientation == ORIENTATION_CC1) > - * val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > - * if (orientation == ORIENTATION_CC2) > - * writel(0x4c, qphy->pcs + QSERDES_V3_DP_PHY_MODE); > - */ > - val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > + if (dp_opts->lanes == 4 || reverse) > + val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; > + if (dp_opts->lanes == 4 || !reverse) > + val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > writel(val, qphy->pcs + QSERDES_DP_PHY_PD_CTL); > > - writel(0x5c, qphy->pcs + QSERDES_DP_PHY_MODE); > + if (reverse) > + writel(0x4c, qphy->pcs + QSERDES_DP_PHY_MODE); > + else > + writel(0x5c, qphy->pcs + QSERDES_DP_PHY_MODE); > > return reverse; > } > @@ -5809,6 +5811,123 @@ static const struct dev_pm_ops qcom_qmp_phy_pm_ops = { > qcom_qmp_phy_runtime_resume, NULL) > }; > > +#if IS_ENABLED(CONFIG_TYPEC) > +static int qcom_qmp_phy_typec_switch_set(struct typec_switch *sw, > + enum typec_orientation orientation) pls align to preceding open brace (here and other places) > +{ > + struct qcom_qmp *qmp = typec_switch_get_drvdata(sw); > + void __iomem *dp_com = qmp->dp_com; > + > + qmp->orientation = orientation; > + > + if (orientation == TYPEC_ORIENTATION_REVERSE) > + qphy_setbits(dp_com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x01); > + else > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x01); > + > + return 0; > +} > + > +static int qcom_qmp_phy_typec_mux_set(struct typec_mux *mux, > + struct typec_mux_state *state) > +{ > + struct qcom_qmp *qmp = typec_mux_get_drvdata(mux); > + void __iomem *dp_com = qmp->dp_com; > + bool dp_mode; > + bool usb_mode; > + > + switch (state->mode) { > + case TYPEC_STATE_SAFE: > + case TYPEC_STATE_USB: > + /* > + * TODO: Figure out if we should clear DP_MODE when we enter a > + * USB-only state. > + */ > + dp_mode = true; should this be false for these states? > + usb_mode = true; > + break; > + case TYPEC_DP_STATE_A: > + case TYPEC_DP_STATE_C: > + case TYPEC_DP_STATE_E: > + dp_mode = true; > + usb_mode = false; > + break; > + case TYPEC_DP_STATE_B: > + case TYPEC_DP_STATE_D: > + case TYPEC_DP_STATE_F: > + dp_mode = true; > + usb_mode = true; > + break; > + } looks like dp_mode is true always. And only for DP state A C E, usb_mode is false... > + > + qphy_setbits(dp_com, QPHY_V3_DP_COM_RESET_OVRD_CTRL, > + SW_DPPHY_RESET_MUX | SW_USB3PHY_RESET_MUX); > + if (dp_mode) > + qphy_setbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, DP_MODE); > + else > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, DP_MODE); > + > + if (usb_mode) > + qphy_setbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE); > + else > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE); > + > + qphy_setbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_SWI_CTRL, 0x03); > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > + > + return 0; > +} > + > +static int qcom_qmp_phy_typec_register(struct qcom_qmp *qmp, > + const struct qmp_phy_cfg *cfg) > +{ > + struct typec_switch_desc sw_desc = {}; > + struct typec_mux_desc mux_desc = {}; > + struct device *dev = qmp->dev; > + > + if (!cfg->has_phy_dp_com_ctrl) > + return 0; > + > + sw_desc.drvdata = qmp; > + sw_desc.fwnode = dev->fwnode; > + sw_desc.set = qcom_qmp_phy_typec_switch_set; > + qmp->sw = typec_switch_register(dev, &sw_desc); > + if (IS_ERR(qmp->sw)) { > + dev_err(dev, "Error registering typec switch: %pe\n", qmp->sw); > + return PTR_ERR(qmp->sw); > + } > + > + mux_desc.drvdata = qmp; > + mux_desc.fwnode = dev->fwnode; > + mux_desc.set = qcom_qmp_phy_typec_mux_set; > + qmp->mux = typec_mux_register(dev, &mux_desc); > + if (IS_ERR(qmp->mux)) { > + dev_err(dev, "Error registering typec mux: %pe\n", qmp->mux); > + typec_switch_unregister(qmp->sw); > + return PTR_ERR(qmp->mux); > + } > + > + return 0; > +} > + > +static void qcom_qmp_phy_typec_unregister(struct qcom_qmp *qmp) > +{ > + typec_mux_unregister(qmp->mux); > + typec_switch_unregister(qmp->sw); > +} > +#else empty line here pls > +static int qcom_qmp_phy_typec_register(struct qcom_qmp *qmp, > + const struct qmp_phy_cfg *cfg) > +{ > + return 0; > +} > + > +static void qcom_qmp_phy_typec_unregister(struct qcom_qmp *qmp) > +{ > +} > +#endif > + > static int qcom_qmp_phy_probe(struct platform_device *pdev) > { > struct qcom_qmp *qmp; > @@ -5891,7 +6010,15 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > return ret; > } > > - num = of_get_available_child_count(dev->of_node); > + ret = qcom_qmp_phy_typec_register(qmp, cfg); > + if (ret) > + return ret; > + > + num = 0; > + for_each_available_child_of_node(dev->of_node, child) { > + if (!of_node_name_eq(child, "port")) > + num++; > + } > /* do we have a rogue child node ? */ > if (num > expected_phys) > return -EINVAL; > @@ -5918,6 +6045,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > serdes = usb_serdes; > } > > + if (of_node_name_eq(child, "port")) > + continue; > + > /* Create per-lane phy */ > ret = qcom_qmp_phy_create(dev, child, id, serdes, cfg); > if (ret) { > @@ -5962,8 +6092,18 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > return ret; > } > > +static int qcom_qmp_phy_remove(struct platform_device *pdev) > +{ > + struct qcom_qmp *qmp = platform_get_drvdata(pdev); > + > + qcom_qmp_phy_typec_unregister(qmp); > + > + return 0; > +} > + > static struct platform_driver qcom_qmp_phy_driver = { > .probe = qcom_qmp_phy_probe, > + .remove = qcom_qmp_phy_remove, > .driver = { > .name = "qcom-qmp-phy", > .pm = &qcom_qmp_phy_pm_ops, > -- > 2.33.1
On Tue 28 Dec 21:27 PST 2021, Vinod Koul wrote: > On 27-12-21, 21:21, Bjorn Andersson wrote: > > The QMP PHY handles muxing of USB vs DisplayPort, as well as orientation > > switching of the SuperSpeed lanes. So register typec handlers for the > > two types. > > > > The TypeC mux allows switching between four lanes of DisplayPort and a > > mixed USB+DP combination. This makes it possible to reach resolutions > > that requires 4 lanes. > > > > The TypeC switch allows switching the SuperSpeed pins and have been > > tested with both 2 and 4 lane DisplayPort. > > > > It's possible that in the USB mode the DP_MODE should be disabled, but > > this is left untouched. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > My suggestion is that if/once this patch is deemed acceptable the PHY > > maintainers could create a immutable branch/tag which can be merged into the > > PHY tree as well as the USB tree. > > > > drivers/phy/qualcomm/phy-qcom-qmp.c | 176 +++++++++++++++++++++++++--- > > 1 file changed, 158 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > > index 7bea6a60dc54..8d8139df9d8e 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > > @@ -19,6 +19,8 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/reset.h> > > #include <linux/slab.h> > > +#include <linux/usb/typec_dp.h> > > +#include <linux/usb/typec_mux.h> > > > > #include <dt-bindings/phy/phy.h> > > > > @@ -3017,6 +3019,9 @@ struct qmp_phy_dp_clks { > > * @phy_mutex: mutex lock for PHY common block initialization > > * @init_count: phy common block initialization count > > * @ufs_reset: optional UFS PHY reset handle > > + * @sw: typec switch for receiving orientation changes > > + * @mux: typec mux for DP muxing > > + * @orientation: carries current CC orientation > > */ > > struct qcom_qmp { > > struct device *dev; > > @@ -3032,6 +3037,10 @@ struct qcom_qmp { > > int init_count; > > > > struct reset_control *ufs_reset; > > + > > + struct typec_switch *sw; > > + struct typec_mux *mux; > > + enum typec_orientation orientation; > > }; > > > > static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_phy *qphy); > > @@ -4378,30 +4387,23 @@ static void qcom_qmp_v3_phy_configure_dp_tx(struct qmp_phy *qphy) > > > > static bool qcom_qmp_phy_configure_dp_mode(struct qmp_phy *qphy) > > { > > + const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts; > > + bool reverse = qphy->qmp->orientation == TYPEC_ORIENTATION_REVERSE; > > u32 val; > > - bool reverse = false; > > > > val = DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN; > > > > - /* > > - * TODO: Assume orientation is CC1 for now and two lanes, need to > > - * use type-c connector to understand orientation and lanes. > > - * > > - * Otherwise val changes to be like below if this code understood > > - * the orientation of the type-c cable. > > - * > > - * if (lane_cnt == 4 || orientation == ORIENTATION_CC2) > > - * val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; > > - * if (lane_cnt == 4 || orientation == ORIENTATION_CC1) > > - * val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > > - * if (orientation == ORIENTATION_CC2) > > - * writel(0x4c, qphy->pcs + QSERDES_V3_DP_PHY_MODE); > > - */ > > - val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > > + if (dp_opts->lanes == 4 || reverse) > > + val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN; > > + if (dp_opts->lanes == 4 || !reverse) > > + val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN; > > writel(val, qphy->pcs + QSERDES_DP_PHY_PD_CTL); > > > > - writel(0x5c, qphy->pcs + QSERDES_DP_PHY_MODE); > > + if (reverse) > > + writel(0x4c, qphy->pcs + QSERDES_DP_PHY_MODE); > > + else > > + writel(0x5c, qphy->pcs + QSERDES_DP_PHY_MODE); > > > > return reverse; > > } > > @@ -5809,6 +5811,123 @@ static const struct dev_pm_ops qcom_qmp_phy_pm_ops = { > > qcom_qmp_phy_runtime_resume, NULL) > > }; > > > > +#if IS_ENABLED(CONFIG_TYPEC) > > +static int qcom_qmp_phy_typec_switch_set(struct typec_switch *sw, > > + enum typec_orientation orientation) > > pls align to preceding open brace (here and other places) > Sure thing. > > +{ > > + struct qcom_qmp *qmp = typec_switch_get_drvdata(sw); > > + void __iomem *dp_com = qmp->dp_com; > > + > > + qmp->orientation = orientation; > > + > > + if (orientation == TYPEC_ORIENTATION_REVERSE) > > + qphy_setbits(dp_com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x01); > > + else > > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x01); > > + > > + return 0; > > +} > > + > > +static int qcom_qmp_phy_typec_mux_set(struct typec_mux *mux, > > + struct typec_mux_state *state) > > +{ > > + struct qcom_qmp *qmp = typec_mux_get_drvdata(mux); > > + void __iomem *dp_com = qmp->dp_com; > > + bool dp_mode; > > + bool usb_mode; > > + > > + switch (state->mode) { > > + case TYPEC_STATE_SAFE: > > + case TYPEC_STATE_USB: > > + /* > > + * TODO: Figure out if we should clear DP_MODE when we enter a > > + * USB-only state. > > + */ > > + dp_mode = true; > > should this be false for these states? > I think it should, but figured that I better change the behavior in a separate follow-up commit. Might also be that SAFE should be false/false - or in some other way turning off the output. Hence the TODO. > > + usb_mode = true; > > + break; > > + case TYPEC_DP_STATE_A: > > + case TYPEC_DP_STATE_C: > > + case TYPEC_DP_STATE_E: > > + dp_mode = true; > > + usb_mode = false; > > + break; > > + case TYPEC_DP_STATE_B: > > + case TYPEC_DP_STATE_D: > > + case TYPEC_DP_STATE_F: > > + dp_mode = true; > > + usb_mode = true; > > + break; > > + } > > looks like dp_mode is true always. And only for DP state A C E, usb_mode > is false... > A, C and E are 4-lanes DP, while B, D, F are 2-lane DP and 2-lane USB. > > + > > + qphy_setbits(dp_com, QPHY_V3_DP_COM_RESET_OVRD_CTRL, > > + SW_DPPHY_RESET_MUX | SW_USB3PHY_RESET_MUX); > > + if (dp_mode) > > + qphy_setbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, DP_MODE); > > + else > > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, DP_MODE); > > + > > + if (usb_mode) > > + qphy_setbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE); > > + else > > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE); > > + > > + qphy_setbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_SWI_CTRL, 0x03); > > + qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET); > > + > > + return 0; > > +} > > + > > +static int qcom_qmp_phy_typec_register(struct qcom_qmp *qmp, > > + const struct qmp_phy_cfg *cfg) > > +{ > > + struct typec_switch_desc sw_desc = {}; > > + struct typec_mux_desc mux_desc = {}; > > + struct device *dev = qmp->dev; > > + > > + if (!cfg->has_phy_dp_com_ctrl) > > + return 0; > > + > > + sw_desc.drvdata = qmp; > > + sw_desc.fwnode = dev->fwnode; > > + sw_desc.set = qcom_qmp_phy_typec_switch_set; > > + qmp->sw = typec_switch_register(dev, &sw_desc); > > + if (IS_ERR(qmp->sw)) { > > + dev_err(dev, "Error registering typec switch: %pe\n", qmp->sw); > > + return PTR_ERR(qmp->sw); > > + } > > + > > + mux_desc.drvdata = qmp; > > + mux_desc.fwnode = dev->fwnode; > > + mux_desc.set = qcom_qmp_phy_typec_mux_set; > > + qmp->mux = typec_mux_register(dev, &mux_desc); > > + if (IS_ERR(qmp->mux)) { > > + dev_err(dev, "Error registering typec mux: %pe\n", qmp->mux); > > + typec_switch_unregister(qmp->sw); > > + return PTR_ERR(qmp->mux); > > + } > > + > > + return 0; > > +} > > + > > +static void qcom_qmp_phy_typec_unregister(struct qcom_qmp *qmp) > > +{ > > + typec_mux_unregister(qmp->mux); > > + typec_switch_unregister(qmp->sw); > > +} > > +#else > > empty line here pls > Okay. Thanks, Bjorn > > +static int qcom_qmp_phy_typec_register(struct qcom_qmp *qmp, > > + const struct qmp_phy_cfg *cfg) > > +{ > > + return 0; > > +} > > + > > +static void qcom_qmp_phy_typec_unregister(struct qcom_qmp *qmp) > > +{ > > +} > > +#endif > > + > > static int qcom_qmp_phy_probe(struct platform_device *pdev) > > { > > struct qcom_qmp *qmp; > > @@ -5891,7 +6010,15 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > return ret; > > } > > > > - num = of_get_available_child_count(dev->of_node); > > + ret = qcom_qmp_phy_typec_register(qmp, cfg); > > + if (ret) > > + return ret; > > + > > + num = 0; > > + for_each_available_child_of_node(dev->of_node, child) { > > + if (!of_node_name_eq(child, "port")) > > + num++; > > + } > > /* do we have a rogue child node ? */ > > if (num > expected_phys) > > return -EINVAL; > > @@ -5918,6 +6045,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > serdes = usb_serdes; > > } > > > > + if (of_node_name_eq(child, "port")) > > + continue; > > + > > /* Create per-lane phy */ > > ret = qcom_qmp_phy_create(dev, child, id, serdes, cfg); > > if (ret) { > > @@ -5962,8 +6092,18 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev) > > return ret; > > } > > > > +static int qcom_qmp_phy_remove(struct platform_device *pdev) > > +{ > > + struct qcom_qmp *qmp = platform_get_drvdata(pdev); > > + > > + qcom_qmp_phy_typec_unregister(qmp); > > + > > + return 0; > > +} > > + > > static struct platform_driver qcom_qmp_phy_driver = { > > .probe = qcom_qmp_phy_probe, > > + .remove = qcom_qmp_phy_remove, > > .driver = { > > .name = "qcom-qmp-phy", > > .pm = &qcom_qmp_phy_pm_ops, > > -- > > 2.33.1 > > -- > ~Vinod