eDP panels might need some power sequencing and backlight management,
so make it possible to associate a drm_panel with a DP instance and
prepare and enable the panel accordingly.
Signed-off-by: Bjorn Andersson <[email protected]>
---
This solves my immediate problem on my 8cx laptops, of indirectly controlling
the backlight during DPMS. But my panel is powered when I boot it and as such I
get the hpd interrupt and I don't actually have to deal with a power on
sequence - so I'm posting this as an RFC, hoping to get some input on these
other aspects.
If this is acceptable I'd be happy to write up an accompanying DT binding
change that marks port 2 of the DP controller's of_graph as a reference to the
attached panel.
drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++++++++--
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_parser.c | 19 +++++++++++++++++++
drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 206bf7806f51..1db5a3f752d2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include <linux/component.h>
#include <linux/of_irq.h>
#include <linux/delay.h>
+#include <drm/drm_panel.h>
#include "msm_drv.h"
#include "msm_kms.h"
@@ -252,6 +253,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
goto end;
}
+ dp->dp_display.drm_panel = dp->parser->drm_panel;
+
rc = dp_aux_register(dp->aux, drm);
if (rc) {
DRM_ERROR("DRM DP AUX register failed\n");
@@ -867,8 +870,10 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
return 0;
}
-static int dp_display_prepare(struct msm_dp *dp)
+static int dp_display_prepare(struct msm_dp *dp_display)
{
+ drm_panel_prepare(dp_display->drm_panel);
+
return 0;
}
@@ -886,6 +891,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
if (!rc)
dp_display->power_on = true;
+ drm_panel_enable(dp_display->drm_panel);
+
return rc;
}
@@ -915,6 +922,8 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
if (!dp_display->power_on)
return 0;
+ drm_panel_disable(dp_display->drm_panel);
+
/* wait only if audio was enabled */
if (dp_display->audio_enabled) {
/* signal the disconnect event */
@@ -939,8 +948,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
return 0;
}
-static int dp_display_unprepare(struct msm_dp *dp)
+static int dp_display_unprepare(struct msm_dp *dp_display)
{
+ drm_panel_unprepare(dp_display->drm_panel);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 8b47cdabb67e..ce337824c95d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -15,6 +15,7 @@ struct msm_dp {
struct device *codec_dev;
struct drm_connector *connector;
struct drm_encoder *encoder;
+ struct drm_panel *drm_panel;
bool is_connected;
bool audio_enabled;
bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index fc8a6452f641..e6a6e9007bfd 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -6,6 +6,7 @@
#include <linux/of_gpio.h>
#include <linux/phy/phy.h>
+#include <drm/drm_of.h>
#include <drm/drm_print.h>
#include "dp_parser.h"
@@ -276,6 +277,20 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
+static int dp_parser_find_panel(struct dp_parser *parser)
+{
+ struct device_node *np = parser->pdev->dev.of_node;
+ int rc;
+
+ rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
+ if (rc == -ENODEV)
+ rc = 0;
+ else if (rc)
+ DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
+
+ return rc;
+}
+
static int dp_parser_parse(struct dp_parser *parser)
{
int rc = 0;
@@ -297,6 +312,10 @@ static int dp_parser_parse(struct dp_parser *parser)
if (rc)
return rc;
+ rc = dp_parser_find_panel(parser);
+ if (rc)
+ return rc;
+
/* Map the corresponding regulator information according to
* version. Currently, since we only have one supported platform,
* mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3266b529c090..994ca9336acd 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -122,6 +122,7 @@ struct dp_parser {
struct dp_display_data disp_data;
const struct dp_regulator_cfg *regulator_cfg;
u32 max_dp_lanes;
+ struct drm_panel *drm_panel;
int (*parse)(struct dp_parser *parser);
};
--
2.29.2
On 27/07/2021 02:13, Bjorn Andersson wrote:
> eDP panels might need some power sequencing and backlight management,
> so make it possible to associate a drm_panel with a DP instance and
> prepare and enable the panel accordingly.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
The idea looks good from my point of view. For v1 could you please
extend it with the `if (panel)` checks and handling of the error codes.
> ---
>
> This solves my immediate problem on my 8cx laptops, of indirectly controlling
> the backlight during DPMS. But my panel is powered when I boot it and as such I
> get the hpd interrupt and I don't actually have to deal with a power on
> sequence - so I'm posting this as an RFC, hoping to get some input on these
> other aspects.
>
> If this is acceptable I'd be happy to write up an accompanying DT binding
> change that marks port 2 of the DP controller's of_graph as a reference to the
> attached panel.
>
> drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++++++++--
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_parser.c | 19 +++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> 4 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 206bf7806f51..1db5a3f752d2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,6 +10,7 @@
> #include <linux/component.h>
> #include <linux/of_irq.h>
> #include <linux/delay.h>
> +#include <drm/drm_panel.h>
>
> #include "msm_drv.h"
> #include "msm_kms.h"
> @@ -252,6 +253,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> + dp->dp_display.drm_panel = dp->parser->drm_panel;
> +
> rc = dp_aux_register(dp->aux, drm);
> if (rc) {
> DRM_ERROR("DRM DP AUX register failed\n");
> @@ -867,8 +870,10 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
> return 0;
> }
>
> -static int dp_display_prepare(struct msm_dp *dp)
> +static int dp_display_prepare(struct msm_dp *dp_display)
> {
> + drm_panel_prepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> @@ -886,6 +891,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
> if (!rc)
> dp_display->power_on = true;
>
> + drm_panel_enable(dp_display->drm_panel);
> +
> return rc;
> }
>
> @@ -915,6 +922,8 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> if (!dp_display->power_on)
> return 0;
>
> + drm_panel_disable(dp_display->drm_panel);
> +
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> @@ -939,8 +948,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> return 0;
> }
>
> -static int dp_display_unprepare(struct msm_dp *dp)
> +static int dp_display_unprepare(struct msm_dp *dp_display)
> {
> + drm_panel_unprepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 8b47cdabb67e..ce337824c95d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -15,6 +15,7 @@ struct msm_dp {
> struct device *codec_dev;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> + struct drm_panel *drm_panel;
> bool is_connected;
> bool audio_enabled;
> bool power_on;
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index fc8a6452f641..e6a6e9007bfd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -6,6 +6,7 @@
> #include <linux/of_gpio.h>
> #include <linux/phy/phy.h>
>
> +#include <drm/drm_of.h>
> #include <drm/drm_print.h>
>
> #include "dp_parser.h"
> @@ -276,6 +277,20 @@ static int dp_parser_clock(struct dp_parser *parser)
> return 0;
> }
>
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device_node *np = parser->pdev->dev.of_node;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> + if (rc == -ENODEV)
> + rc = 0;
> + else if (rc)
> + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> +
> + return rc;
> +}
> +
> static int dp_parser_parse(struct dp_parser *parser)
> {
> int rc = 0;
> @@ -297,6 +312,10 @@ static int dp_parser_parse(struct dp_parser *parser)
> if (rc)
> return rc;
>
> + rc = dp_parser_find_panel(parser);
> + if (rc)
> + return rc;
> +
> /* Map the corresponding regulator information according to
> * version. Currently, since we only have one supported platform,
> * mapping the regulator directly.
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 3266b529c090..994ca9336acd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -122,6 +122,7 @@ struct dp_parser {
> struct dp_display_data disp_data;
> const struct dp_regulator_cfg *regulator_cfg;
> u32 max_dp_lanes;
> + struct drm_panel *drm_panel;
>
> int (*parse)(struct dp_parser *parser);
> };
>
--
With best wishes
Dmitry
On Thu 29 Jul 04:59 CDT 2021, Dmitry Baryshkov wrote:
> On 27/07/2021 02:13, Bjorn Andersson wrote:
> > eDP panels might need some power sequencing and backlight management,
> > so make it possible to associate a drm_panel with a DP instance and
> > prepare and enable the panel accordingly.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
>
> The idea looks good from my point of view.
Thanks.
> For v1 could you please extend it with the `if (panel)` checks and
> handling of the error codes.
Passing NULL to the drm_panel_* api is valid and a nop, and I run the
same code for both eDP and DP.
Would you still like it to indicate to a future reader that this might
be NULL?
Regards,
Bjorn
>
> > ---
> >
> > This solves my immediate problem on my 8cx laptops, of indirectly controlling
> > the backlight during DPMS. But my panel is powered when I boot it and as such I
> > get the hpd interrupt and I don't actually have to deal with a power on
> > sequence - so I'm posting this as an RFC, hoping to get some input on these
> > other aspects.
> >
> > If this is acceptable I'd be happy to write up an accompanying DT binding
> > change that marks port 2 of the DP controller's of_graph as a reference to the
> > attached panel.
> >
> > drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++++++++--
> > drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> > drivers/gpu/drm/msm/dp/dp_parser.c | 19 +++++++++++++++++++
> > drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> > 4 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 206bf7806f51..1db5a3f752d2 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,6 +10,7 @@
> > #include <linux/component.h>
> > #include <linux/of_irq.h>
> > #include <linux/delay.h>
> > +#include <drm/drm_panel.h>
> > #include "msm_drv.h"
> > #include "msm_kms.h"
> > @@ -252,6 +253,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
> > goto end;
> > }
> > + dp->dp_display.drm_panel = dp->parser->drm_panel;
> > +
> > rc = dp_aux_register(dp->aux, drm);
> > if (rc) {
> > DRM_ERROR("DRM DP AUX register failed\n");
> > @@ -867,8 +870,10 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
> > return 0;
> > }
> > -static int dp_display_prepare(struct msm_dp *dp)
> > +static int dp_display_prepare(struct msm_dp *dp_display)
> > {
> > + drm_panel_prepare(dp_display->drm_panel);
> > +
> > return 0;
> > }
> > @@ -886,6 +891,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
> > if (!rc)
> > dp_display->power_on = true;
> > + drm_panel_enable(dp_display->drm_panel);
> > +
> > return rc;
> > }
> > @@ -915,6 +922,8 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> > if (!dp_display->power_on)
> > return 0;
> > + drm_panel_disable(dp_display->drm_panel);
> > +
> > /* wait only if audio was enabled */
> > if (dp_display->audio_enabled) {
> > /* signal the disconnect event */
> > @@ -939,8 +948,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> > return 0;
> > }
> > -static int dp_display_unprepare(struct msm_dp *dp)
> > +static int dp_display_unprepare(struct msm_dp *dp_display)
> > {
> > + drm_panel_unprepare(dp_display->drm_panel);
> > +
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 8b47cdabb67e..ce337824c95d 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -15,6 +15,7 @@ struct msm_dp {
> > struct device *codec_dev;
> > struct drm_connector *connector;
> > struct drm_encoder *encoder;
> > + struct drm_panel *drm_panel;
> > bool is_connected;
> > bool audio_enabled;
> > bool power_on;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index fc8a6452f641..e6a6e9007bfd 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -6,6 +6,7 @@
> > #include <linux/of_gpio.h>
> > #include <linux/phy/phy.h>
> > +#include <drm/drm_of.h>
> > #include <drm/drm_print.h>
> > #include "dp_parser.h"
> > @@ -276,6 +277,20 @@ static int dp_parser_clock(struct dp_parser *parser)
> > return 0;
> > }
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > + struct device_node *np = parser->pdev->dev.of_node;
> > + int rc;
> > +
> > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > + if (rc == -ENODEV)
> > + rc = 0;
> > + else if (rc)
> > + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> > +
> > + return rc;
> > +}
> > +
> > static int dp_parser_parse(struct dp_parser *parser)
> > {
> > int rc = 0;
> > @@ -297,6 +312,10 @@ static int dp_parser_parse(struct dp_parser *parser)
> > if (rc)
> > return rc;
> > + rc = dp_parser_find_panel(parser);
> > + if (rc)
> > + return rc;
> > +
> > /* Map the corresponding regulator information according to
> > * version. Currently, since we only have one supported platform,
> > * mapping the regulator directly.
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> > index 3266b529c090..994ca9336acd 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> > @@ -122,6 +122,7 @@ struct dp_parser {
> > struct dp_display_data disp_data;
> > const struct dp_regulator_cfg *regulator_cfg;
> > u32 max_dp_lanes;
> > + struct drm_panel *drm_panel;
> > int (*parse)(struct dp_parser *parser);
> > };
> >
>
>
> --
> With best wishes
> Dmitry
Quoting Bjorn Andersson (2021-07-26 16:13:51)
> eDP panels might need some power sequencing and backlight management,
> so make it possible to associate a drm_panel with a DP instance and
> prepare and enable the panel accordingly.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> This solves my immediate problem on my 8cx laptops, of indirectly controlling
> the backlight during DPMS. But my panel is powered when I boot it and as such I
> get the hpd interrupt and I don't actually have to deal with a power on
> sequence - so I'm posting this as an RFC, hoping to get some input on these
> other aspects.
>
> If this is acceptable I'd be happy to write up an accompanying DT binding
> change that marks port 2 of the DP controller's of_graph as a reference to the
> attached panel.
dianders@ mentioned creating a connector (and maybe a bridge) for the DP
connector (not eDP)[1]. I'm not sure that's directly related, but I
think with the aux bus code the panel isn't managed in the encoder
driver. Instead the encoder sees a bridge and tries to power it up and
then query things over the aux bus? It's all a little too fuzzy to me
right now so I could be spewing nonsense but I think we want to take
this bridge route if possible.
-Stephen
[1] https://lore.kernel.org/r/CAD=FV=Xd9fizYdxfXYOkpJ_1fZcHp3-ROJ7k4iPg0g0RQ_+A3Q@mail.gmail.com/
>
> drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++++++++--
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_parser.c | 19 +++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> 4 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 206bf7806f51..1db5a3f752d2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,6 +10,7 @@
> #include <linux/component.h>
> #include <linux/of_irq.h>
> #include <linux/delay.h>
> +#include <drm/drm_panel.h>
>
> #include "msm_drv.h"
> #include "msm_kms.h"
> @@ -252,6 +253,8 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> + dp->dp_display.drm_panel = dp->parser->drm_panel;
> +
> rc = dp_aux_register(dp->aux, drm);
> if (rc) {
> DRM_ERROR("DRM DP AUX register failed\n");
> @@ -867,8 +870,10 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
> return 0;
> }
>
> -static int dp_display_prepare(struct msm_dp *dp)
> +static int dp_display_prepare(struct msm_dp *dp_display)
> {
> + drm_panel_prepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> @@ -886,6 +891,8 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
> if (!rc)
> dp_display->power_on = true;
>
> + drm_panel_enable(dp_display->drm_panel);
> +
> return rc;
> }
>
> @@ -915,6 +922,8 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> if (!dp_display->power_on)
> return 0;
>
> + drm_panel_disable(dp_display->drm_panel);
> +
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> @@ -939,8 +948,10 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
> return 0;
> }
>
> -static int dp_display_unprepare(struct msm_dp *dp)
> +static int dp_display_unprepare(struct msm_dp *dp_display)
> {
> + drm_panel_unprepare(dp_display->drm_panel);
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 8b47cdabb67e..ce337824c95d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -15,6 +15,7 @@ struct msm_dp {
> struct device *codec_dev;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> + struct drm_panel *drm_panel;
> bool is_connected;
> bool audio_enabled;
> bool power_on;
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index fc8a6452f641..e6a6e9007bfd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -6,6 +6,7 @@
> #include <linux/of_gpio.h>
> #include <linux/phy/phy.h>
>
> +#include <drm/drm_of.h>
> #include <drm/drm_print.h>
>
> #include "dp_parser.h"
> @@ -276,6 +277,20 @@ static int dp_parser_clock(struct dp_parser *parser)
> return 0;
> }
>
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device_node *np = parser->pdev->dev.of_node;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> + if (rc == -ENODEV)
> + rc = 0;
> + else if (rc)
> + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> +
> + return rc;
> +}
> +
> static int dp_parser_parse(struct dp_parser *parser)
> {
> int rc = 0;
> @@ -297,6 +312,10 @@ static int dp_parser_parse(struct dp_parser *parser)
> if (rc)
> return rc;
>
> + rc = dp_parser_find_panel(parser);
> + if (rc)
> + return rc;
> +
> /* Map the corresponding regulator information according to
> * version. Currently, since we only have one supported platform,
> * mapping the regulator directly.
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 3266b529c090..994ca9336acd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -122,6 +122,7 @@ struct dp_parser {
> struct dp_display_data disp_data;
> const struct dp_regulator_cfg *regulator_cfg;
> u32 max_dp_lanes;
> + struct drm_panel *drm_panel;
>
> int (*parse)(struct dp_parser *parser);
> };
> --
Hi,
On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
<[email protected]> wrote:
>
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device_node *np = parser->pdev->dev.of_node;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> + if (rc == -ENODEV)
> + rc = 0;
> + else if (rc)
> + DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> +
> + return rc;
So rather than storing the drm_panel, I suggest that you actually wrap
it with a "panel_bridge". Follow the ideas from commit 4e5763f03e10
("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge") and the fix
in commit c7782443a889 ("drm/bridge: ti-sn65dsi86: Avoid creating
multiple connectors").
If you do that then actually a bunch of your patch becomes
unnecessary. You basically just have to attach the "next" bridge in
the right place and you're good, right?
-Doug
Hi,
On Wed, Aug 25, 2021 at 6:31 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Bjorn Andersson (2021-07-26 16:13:51)
> > eDP panels might need some power sequencing and backlight management,
> > so make it possible to associate a drm_panel with a DP instance and
> > prepare and enable the panel accordingly.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> >
> > This solves my immediate problem on my 8cx laptops, of indirectly controlling
> > the backlight during DPMS. But my panel is powered when I boot it and as such I
> > get the hpd interrupt and I don't actually have to deal with a power on
> > sequence - so I'm posting this as an RFC, hoping to get some input on these
> > other aspects.
> >
> > If this is acceptable I'd be happy to write up an accompanying DT binding
> > change that marks port 2 of the DP controller's of_graph as a reference to the
> > attached panel.
>
> dianders@ mentioned creating a connector (and maybe a bridge) for the DP
> connector (not eDP)[1]. I'm not sure that's directly related, but I
> think with the aux bus code the panel isn't managed in the encoder
> driver. Instead the encoder sees a bridge and tries to power it up and
> then query things over the aux bus? It's all a little too fuzzy to me
> right now so I could be spewing nonsense but I think we want to take
> this bridge route if possible.
>
> -Stephen
>
> [1] https://lore.kernel.org/r/CAD=FV=Xd9fizYdxfXYOkpJ_1fZcHp3-ROJ7k4iPg0g0RQ_+A3Q@mail.gmail.com/
The idea of modeling the connector as a bridge is something that makes
sense to me, but it probably makes sense to tackle that separately. We
don't need to block on it.
The whole DP AUX bus can also be tackled separately after the fact. It
really doesn't change things very much--we still have to find/attach
the panel. It just makes the panel probe in a slightly different way
and as a side effect gives the panel access to the DP AUX bus.
-Doug
Hi,
On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
<[email protected]> wrote:
>
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> + struct device_node *np = parser->pdev->dev.of_node;
> + int rc;
> +
> + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
Why port 2? Shouldn't this just be port 1 always? The yaml says that
port 1 is "Output endpoint of the controller". We should just use port
1 here, right?
-Doug
On Fri 27 Aug 15:52 CDT 2021, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > + struct device_node *np = parser->pdev->dev.of_node;
> > + int rc;
> > +
> > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
>
> Why port 2? Shouldn't this just be port 1 always? The yaml says that
> port 1 is "Output endpoint of the controller". We should just use port
> 1 here, right?
>
I thought port 1 was the link to the Type-C controller, didn't give it a
second thought and took the next available.
But per the binding it makes sense that the panel is the "Output
endpoint of the controller" and I guess one will have either a Type-C
controller or a panel - even after the DP rework?
Regards,
Bjorn
Hi,
On Sat, Aug 28, 2021 at 7:40 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Fri 27 Aug 15:52 CDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > +{
> > > + struct device_node *np = parser->pdev->dev.of_node;
> > > + int rc;
> > > +
> > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> >
> > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > port 1 is "Output endpoint of the controller". We should just use port
> > 1 here, right?
> >
>
> I thought port 1 was the link to the Type-C controller, didn't give it a
> second thought and took the next available.
>
> But per the binding it makes sense that the panel is the "Output
> endpoint of the controller" and I guess one will have either a Type-C
> controller or a panel - even after the DP rework?
Right, my understanding is that "port 1" is the output port
irregardless of whether you're outputting to a panel or a DP
connector. I think the only case it would make sense to add a new port
is if it was possible for the output to be connected to both a panel
and a DP port simultaneously. ...but that doesn't make sense.
-Doug
On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > + struct device_node *np = parser->pdev->dev.of_node;
> > + int rc;
> > +
> > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
>
> Why port 2? Shouldn't this just be port 1 always? The yaml says that
> port 1 is "Output endpoint of the controller". We should just use port
> 1 here, right?
>
Finally got back to this, changed it to 1 and figured out why I left it
at 2.
drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
reference to the USB-C controller, scan through the registered panels
and conclude that the of_node of the USB-C controller isn't a registered
panel and return -EPROBE_DEFER.
So I picked 2, because I'm not able to figure out a way to distinguish
between a not yet probed panel and the USB-C controller...
Any suggestions?
Regards,
Bjorn
Hi,
On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > +{
> > > + struct device_node *np = parser->pdev->dev.of_node;
> > > + int rc;
> > > +
> > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> >
> > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > port 1 is "Output endpoint of the controller". We should just use port
> > 1 here, right?
> >
>
> Finally got back to this, changed it to 1 and figured out why I left it
> at 2.
>
> drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> reference to the USB-C controller, scan through the registered panels
> and conclude that the of_node of the USB-C controller isn't a registered
> panel and return -EPROBE_DEFER.
I'm confused, but maybe it would help if I could see something
concrete. Is there a specific board this was happening on?
Under the DP node in the device tree I expect:
ports {
port@1 {
reg = <1>;
edp_out: endpoint {
remote-endpoint = <&edp_panel_in>;
};
};
};
If you have "port@1" pointing to a USB-C controller but this instance
of the DP controller is actually hooked up straight to a panel then
you should simply delete the "port@1" that points to the typeC and
replace it with one that points to a panel, right?
-Doug
On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > <[email protected]> wrote:
> > > >
> > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > +{
> > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > + int rc;
> > > > +
> > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > >
> > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > port 1 is "Output endpoint of the controller". We should just use port
> > > 1 here, right?
> > >
> >
> > Finally got back to this, changed it to 1 and figured out why I left it
> > at 2.
> >
> > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > reference to the USB-C controller, scan through the registered panels
> > and conclude that the of_node of the USB-C controller isn't a registered
> > panel and return -EPROBE_DEFER.
>
> I'm confused, but maybe it would help if I could see something
> concrete. Is there a specific board this was happening on?
>
Right, let's make this more concrete with a snippet from the actual
SC8180x DT.
> Under the DP node in the device tree I expect:
>
> ports {
> port@1 {
> reg = <1>;
> edp_out: endpoint {
> remote-endpoint = <&edp_panel_in>;
> };
> };
> };
>
/* We got a panel */
panel {
...
ports {
port {
auo_b133han05_in: endpoint {
remote-endpoint = <&mdss_edp_out>;
};
};
};
};
/* And a 2-port USB-C controller */
type-c-controller {
...
connector@0 {
ports {
port@0 {
reg = <0>;
ucsi_port_0_dp: endpoint {
remote-endpoint = <&dp0_mode>;
};
};
port@1 {
reg = <1>;
ucsi_port_0_switch: endpoint {
remote-endpoint = <&primary_qmp_phy>;
};
};
};
};
connector@1 {
ports {
port@0 {
reg = <0>;
ucsi_port_1_dp: endpoint {
remote-endpoint = <&dp1_mode>;
};
};
port@1 {
reg = <1>;
ucsi_port_1_switch: endpoint {
remote-endpoint = <&second_qmp_phy>;
};
};
};
};
};
/* And then our 2 DP and single eDP controllers */
&mdss_dp0 {
ports {
port@1 {
reg = <1>;
dp0_mode: endpoint {
remote-endpoint = <&ucsi_port_0_dp>;
};
};
};
};
&mdss_dp1 {
ports {
port@1 {
reg = <1>;
dp1_mode: endpoint {
remote-endpoint = <&ucsi_port_1_dp>;
};
};
};
};
&mdss_edp {
ports {
port@1 {
reg = <1>;
mdss_edp_out: endpoint {
remote-endpoint = <&auo_b133han05_in>;
};
};
};
};
> If you have "port@1" pointing to a USB-C controller but this instance
> of the DP controller is actually hooked up straight to a panel then
> you should simply delete the "port@1" that points to the typeC and
> replace it with one that points to a panel, right?
>
As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
connectors and the eDP points to the panel, exactly like we agreed.
So now I call:
drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
which for the two DP nodes will pass respective UCSI connector to
drm_find_panel() and get EPROBE_DEFER back - because they are not on
panel_list.
There's nothing indicating in the of_graph that the USB connectors
aren't panels (or bridges), so I don't see a way to distinguish the two
types remotes.
Hope that clarifies my conundrum.
Regards,
Bjorn
Quoting Bjorn Andersson (2021-10-04 18:11:11)
> On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > <[email protected]> wrote:
> > > > >
> > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > +{
> > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > + int rc;
> > > > > +
> > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > >
> > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > 1 here, right?
> > > >
> > >
> > > Finally got back to this, changed it to 1 and figured out why I left it
> > > at 2.
> > >
> > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > reference to the USB-C controller, scan through the registered panels
> > > and conclude that the of_node of the USB-C controller isn't a registered
> > > panel and return -EPROBE_DEFER.
> >
> > I'm confused, but maybe it would help if I could see something
> > concrete. Is there a specific board this was happening on?
> >
>
> Right, let's make this more concrete with a snippet from the actual
> SC8180x DT.
Where is this DT? Is it in the kernel tree?
>
> > Under the DP node in the device tree I expect:
> >
> > ports {
> > port@1 {
> > reg = <1>;
> > edp_out: endpoint {
> > remote-endpoint = <&edp_panel_in>;
> > };
> > };
> > };
> >
>
> /* We got a panel */
> panel {
> ...
> ports {
> port {
> auo_b133han05_in: endpoint {
> remote-endpoint = <&mdss_edp_out>;
> };
> };
> };
> };
>
> /* And a 2-port USB-C controller */
> type-c-controller {
> ...
> connector@0 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_0_dp: endpoint {
> remote-endpoint = <&dp0_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_0_switch: endpoint {
> remote-endpoint = <&primary_qmp_phy>;
> };
> };
> };
> };
>
> connector@1 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_1_dp: endpoint {
> remote-endpoint = <&dp1_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_1_switch: endpoint {
> remote-endpoint = <&second_qmp_phy>;
> };
> };
> };
> };
> };
>
> /* And then our 2 DP and single eDP controllers */
> &mdss_dp0 {
> ports {
> port@1 {
> reg = <1>;
> dp0_mode: endpoint {
> remote-endpoint = <&ucsi_port_0_dp>;
> };
> };
> };
> };
>
> &mdss_dp1 {
> ports {
> port@1 {
> reg = <1>;
> dp1_mode: endpoint {
> remote-endpoint = <&ucsi_port_1_dp>;
> };
> };
> };
> };
>
> &mdss_edp {
> ports {
> port@1 {
> reg = <1>;
> mdss_edp_out: endpoint {
> remote-endpoint = <&auo_b133han05_in>;
> };
> };
> };
> };
>
> > If you have "port@1" pointing to a USB-C controller but this instance
> > of the DP controller is actually hooked up straight to a panel then
> > you should simply delete the "port@1" that points to the typeC and
> > replace it with one that points to a panel, right?
> >
>
> As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> connectors and the eDP points to the panel, exactly like we agreed.
>
> So now I call:
> drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>
> which for the two DP nodes will pass respective UCSI connector to
> drm_find_panel() and get EPROBE_DEFER back - because they are not on
> panel_list.
That's "good" right?
>
> There's nothing indicating in the of_graph that the USB connectors
> aren't panels (or bridges), so I don't see a way to distinguish the two
> types remotes.
>
I'd like to create a bridge, not panel, for USB connectors, so that we
can push sideband HPD signaling through to the DP driver. But either way
this should work, right? If drm_of_find_panel_or_bridge() returns
-EPROBE_DEFER, then assume the connector is DP. Otherwise if there's a
valid pointer then treat it as eDP. We can't go too crazy though because
once we attach a bridge we're assuming eDP which may not actually be
true.
If we make a bridge for type-C USB connectors then we'll be able to use
the drm_bridge_connector code to automatically figure out the connector
type (eDP vs. DP vs. whatever else is chained onto the end of the DP
connector). That would require updating the bridge connector code to
treat DP as a connector type though. And then the eDP path would need to
be handled when there's no bridge really involved, like in your case
where the eDP hardware is directly connected to the eDP panel.
In this case I think we're supposed to make a bridge in this DP driver
itself that does pretty basic stuff and assumes the connector is eDP or
DP based on the hardware type it is. Then if we wire a type-c connector
up to the eDP hardware the eDP bridge we make in this driver will see a
type-c connector that makes a bridge saying "I'm a DP connector" and the
drm_bridge_connector code will look at the last bridge in the chain to
see that it's actually a DP connector.
On Mon 04 Oct 20:50 CDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-04 18:11:11)
> > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > <[email protected]> wrote:
> > > >
> > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > +{
> > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > + int rc;
> > > > > > +
> > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > > >
> > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > 1 here, right?
> > > > >
> > > >
> > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > at 2.
> > > >
> > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > reference to the USB-C controller, scan through the registered panels
> > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > panel and return -EPROBE_DEFER.
> > >
> > > I'm confused, but maybe it would help if I could see something
> > > concrete. Is there a specific board this was happening on?
> > >
> >
> > Right, let's make this more concrete with a snippet from the actual
> > SC8180x DT.
>
> Where is this DT? Is it in the kernel tree?
>
Still missing a bunch of driver pieces, so I haven't yet pushed any of
this upstream.
But if you're interested you can find some work-in-progress here:
https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
> >
> > > Under the DP node in the device tree I expect:
> > >
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > edp_out: endpoint {
> > > remote-endpoint = <&edp_panel_in>;
> > > };
> > > };
> > > };
> > >
> >
> > /* We got a panel */
> > panel {
> > ...
> > ports {
> > port {
> > auo_b133han05_in: endpoint {
> > remote-endpoint = <&mdss_edp_out>;
> > };
> > };
> > };
> > };
> >
> > /* And a 2-port USB-C controller */
> > type-c-controller {
> > ...
> > connector@0 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_0_dp: endpoint {
> > remote-endpoint = <&dp0_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_0_switch: endpoint {
> > remote-endpoint = <&primary_qmp_phy>;
> > };
> > };
> > };
> > };
> >
> > connector@1 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_1_dp: endpoint {
> > remote-endpoint = <&dp1_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_1_switch: endpoint {
> > remote-endpoint = <&second_qmp_phy>;
> > };
> > };
> > };
> > };
> > };
> >
> > /* And then our 2 DP and single eDP controllers */
> > &mdss_dp0 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp0_mode: endpoint {
> > remote-endpoint = <&ucsi_port_0_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_dp1 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp1_mode: endpoint {
> > remote-endpoint = <&ucsi_port_1_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_edp {
> > ports {
> > port@1 {
> > reg = <1>;
> > mdss_edp_out: endpoint {
> > remote-endpoint = <&auo_b133han05_in>;
> > };
> > };
> > };
> > };
> >
> > > If you have "port@1" pointing to a USB-C controller but this instance
> > > of the DP controller is actually hooked up straight to a panel then
> > > you should simply delete the "port@1" that points to the typeC and
> > > replace it with one that points to a panel, right?
> > >
> >
> > As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> > connectors and the eDP points to the panel, exactly like we agreed.
> >
> > So now I call:
> > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> >
> > which for the two DP nodes will pass respective UCSI connector to
> > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > panel_list.
>
> That's "good" right?
>
Well, it's expected that the connectors aren't panels...
> >
> > There's nothing indicating in the of_graph that the USB connectors
> > aren't panels (or bridges), so I don't see a way to distinguish the two
> > types remotes.
> >
>
> I'd like to create a bridge, not panel, for USB connectors, so that we
> can push sideband HPD signaling through to the DP driver. But either way
> this should work, right? If drm_of_find_panel_or_bridge() returns
> -EPROBE_DEFER, then assume the connector is DP. Otherwise if there's a
> valid pointer then treat it as eDP. We can't go too crazy though because
> once we attach a bridge we're assuming eDP which may not actually be
> true.
>
How will I be able to distinguish this from "the eDP panel is not yet
probed"? Unless we first implement the rest of this suggestion to make
sure drm_of_find_panel_or_bridge() has something to find in both cases.
> If we make a bridge for type-C USB connectors then we'll be able to use
> the drm_bridge_connector code to automatically figure out the connector
> type (eDP vs. DP vs. whatever else is chained onto the end of the DP
> connector). That would require updating the bridge connector code to
> treat DP as a connector type though. And then the eDP path would need to
> be handled when there's no bridge really involved, like in your case
> where the eDP hardware is directly connected to the eDP panel.
>
> In this case I think we're supposed to make a bridge in this DP driver
> itself that does pretty basic stuff and assumes the connector is eDP or
> DP based on the hardware type it is. Then if we wire a type-c connector
> up to the eDP hardware the eDP bridge we make in this driver will see a
> type-c connector that makes a bridge saying "I'm a DP connector" and the
> drm_bridge_connector code will look at the last bridge in the chain to
> see that it's actually a DP connector.
This is rather far from how I do handle USB, and its HPD interrupts
today. But perhaps I'm missing something there...
Let me get that patch on the list as well then.
Regards,
Bjorn
Hi,
On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > <[email protected]> wrote:
> > > > >
> > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > +{
> > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > + int rc;
> > > > > +
> > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > >
> > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > 1 here, right?
> > > >
> > >
> > > Finally got back to this, changed it to 1 and figured out why I left it
> > > at 2.
> > >
> > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > reference to the USB-C controller, scan through the registered panels
> > > and conclude that the of_node of the USB-C controller isn't a registered
> > > panel and return -EPROBE_DEFER.
> >
> > I'm confused, but maybe it would help if I could see something
> > concrete. Is there a specific board this was happening on?
> >
>
> Right, let's make this more concrete with a snippet from the actual
> SC8180x DT.
>
> > Under the DP node in the device tree I expect:
> >
> > ports {
> > port@1 {
> > reg = <1>;
> > edp_out: endpoint {
> > remote-endpoint = <&edp_panel_in>;
> > };
> > };
> > };
> >
>
> /* We got a panel */
> panel {
> ...
> ports {
> port {
> auo_b133han05_in: endpoint {
> remote-endpoint = <&mdss_edp_out>;
> };
> };
> };
> };
>
> /* And a 2-port USB-C controller */
> type-c-controller {
> ...
> connector@0 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_0_dp: endpoint {
> remote-endpoint = <&dp0_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_0_switch: endpoint {
> remote-endpoint = <&primary_qmp_phy>;
> };
> };
> };
> };
>
> connector@1 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_1_dp: endpoint {
> remote-endpoint = <&dp1_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_1_switch: endpoint {
> remote-endpoint = <&second_qmp_phy>;
> };
> };
> };
> };
> };
>
> /* And then our 2 DP and single eDP controllers */
> &mdss_dp0 {
> ports {
> port@1 {
> reg = <1>;
> dp0_mode: endpoint {
> remote-endpoint = <&ucsi_port_0_dp>;
> };
> };
> };
> };
>
> &mdss_dp1 {
> ports {
> port@1 {
> reg = <1>;
> dp1_mode: endpoint {
> remote-endpoint = <&ucsi_port_1_dp>;
> };
> };
> };
> };
>
> &mdss_edp {
> ports {
> port@1 {
> reg = <1>;
> mdss_edp_out: endpoint {
> remote-endpoint = <&auo_b133han05_in>;
> };
> };
> };
> };
>
> > If you have "port@1" pointing to a USB-C controller but this instance
> > of the DP controller is actually hooked up straight to a panel then
> > you should simply delete the "port@1" that points to the typeC and
> > replace it with one that points to a panel, right?
> >
>
> As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> connectors and the eDP points to the panel, exactly like we agreed.
>
> So now I call:
> drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>
> which for the two DP nodes will pass respective UCSI connector to
> drm_find_panel() and get EPROBE_DEFER back - because they are not on
> panel_list.
>
> There's nothing indicating in the of_graph that the USB connectors
> aren't panels (or bridges), so I don't see a way to distinguish the two
> types remotes.
As far as I can tell the way this would be solved would be to actually
pass &bridge in and then make sure that a bridge would be in place for
the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
the connector hasn't been probed but once it's probed then it should
register as a bridge and thus give you the info you need (AKA that
this isn't a panel).
I haven't done the digging to see how all this works, but according to
Laurent [1]: "Physical connectors are already handled as bridges, see
drivers/gpu/drm/bridge/display-connector.c"
So basically I think this is solvable in code and there's no reason to
mess with the devicetree bindings to solve this problem. Does that
sound right?
[1] https://lore.kernel.org/r/[email protected]/
On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > <[email protected]> wrote:
> > > >
> > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > +{
> > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > + int rc;
> > > > > > +
> > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > > >
> > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > 1 here, right?
> > > > >
> > > >
> > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > at 2.
> > > >
> > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > reference to the USB-C controller, scan through the registered panels
> > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > panel and return -EPROBE_DEFER.
> > >
> > > I'm confused, but maybe it would help if I could see something
> > > concrete. Is there a specific board this was happening on?
> > >
> >
> > Right, let's make this more concrete with a snippet from the actual
> > SC8180x DT.
> >
> > > Under the DP node in the device tree I expect:
> > >
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > edp_out: endpoint {
> > > remote-endpoint = <&edp_panel_in>;
> > > };
> > > };
> > > };
> > >
> >
> > /* We got a panel */
> > panel {
> > ...
> > ports {
> > port {
> > auo_b133han05_in: endpoint {
> > remote-endpoint = <&mdss_edp_out>;
> > };
> > };
> > };
> > };
> >
> > /* And a 2-port USB-C controller */
> > type-c-controller {
> > ...
> > connector@0 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_0_dp: endpoint {
> > remote-endpoint = <&dp0_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_0_switch: endpoint {
> > remote-endpoint = <&primary_qmp_phy>;
> > };
> > };
> > };
> > };
> >
> > connector@1 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_1_dp: endpoint {
> > remote-endpoint = <&dp1_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_1_switch: endpoint {
> > remote-endpoint = <&second_qmp_phy>;
> > };
> > };
> > };
> > };
> > };
> >
> > /* And then our 2 DP and single eDP controllers */
> > &mdss_dp0 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp0_mode: endpoint {
> > remote-endpoint = <&ucsi_port_0_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_dp1 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp1_mode: endpoint {
> > remote-endpoint = <&ucsi_port_1_dp>;
> > };
> > };
> > };
> > };
> >
> > &mdss_edp {
> > ports {
> > port@1 {
> > reg = <1>;
> > mdss_edp_out: endpoint {
> > remote-endpoint = <&auo_b133han05_in>;
> > };
> > };
> > };
> > };
> >
> > > If you have "port@1" pointing to a USB-C controller but this instance
> > > of the DP controller is actually hooked up straight to a panel then
> > > you should simply delete the "port@1" that points to the typeC and
> > > replace it with one that points to a panel, right?
> > >
> >
> > As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> > connectors and the eDP points to the panel, exactly like we agreed.
> >
> > So now I call:
> > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> >
> > which for the two DP nodes will pass respective UCSI connector to
> > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > panel_list.
> >
> > There's nothing indicating in the of_graph that the USB connectors
> > aren't panels (or bridges), so I don't see a way to distinguish the two
> > types remotes.
>
> As far as I can tell the way this would be solved would be to actually
> pass &bridge in and then make sure that a bridge would be in place for
> the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
> the connector hasn't been probed but once it's probed then it should
> register as a bridge and thus give you the info you need (AKA that
> this isn't a panel).
>
> I haven't done the digging to see how all this works, but according to
> Laurent [1]: "Physical connectors are already handled as bridges, see
> drivers/gpu/drm/bridge/display-connector.c"
>
All this seems to make sense for both eDP and "native" DP.
> So basically I think this is solvable in code and there's no reason to
> mess with the devicetree bindings to solve this problem. Does that
> sound right?
>
But I don't have a DisplayPort connector.
I have a USB-C connector, that upon determining that it's time to play
DisplayPort will use the typec_mux abstraction to tell someone on the
other side of the of_graph about DisplayPort events (HPD).
So where would I put this drm_bridge in the USB-C case?
I don't see that it fits in the Type-C side of things and putting it on
the DP side would leave us with exactly the problem we have here. So we
would have to put a fake "DP connector" inbetween the DP node and the
Type-C controller?
For reference, this is how I thought one is supposed to tie the Type-C
controller to the display driver:
https://lore.kernel.org/all/[email protected]/
I'm afraid I must be missing something in Laurent and yours proposal
(although I think Laurent is talking about the native DP case?).
Regards,
Bjorn
> [1] https://lore.kernel.org/r/[email protected]/
Hi,
On Tue, Oct 5, 2021 at 10:33 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > > +{
> > > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > > + int rc;
> > > > > > > +
> > > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > > > >
> > > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > > 1 here, right?
> > > > > >
> > > > >
> > > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > > at 2.
> > > > >
> > > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > > reference to the USB-C controller, scan through the registered panels
> > > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > > panel and return -EPROBE_DEFER.
> > > >
> > > > I'm confused, but maybe it would help if I could see something
> > > > concrete. Is there a specific board this was happening on?
> > > >
> > >
> > > Right, let's make this more concrete with a snippet from the actual
> > > SC8180x DT.
> > >
> > > > Under the DP node in the device tree I expect:
> > > >
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > edp_out: endpoint {
> > > > remote-endpoint = <&edp_panel_in>;
> > > > };
> > > > };
> > > > };
> > > >
> > >
> > > /* We got a panel */
> > > panel {
> > > ...
> > > ports {
> > > port {
> > > auo_b133han05_in: endpoint {
> > > remote-endpoint = <&mdss_edp_out>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > /* And a 2-port USB-C controller */
> > > type-c-controller {
> > > ...
> > > connector@0 {
> > > ports {
> > > port@0 {
> > > reg = <0>;
> > > ucsi_port_0_dp: endpoint {
> > > remote-endpoint = <&dp0_mode>;
> > > };
> > > };
> > >
> > > port@1 {
> > > reg = <1>;
> > > ucsi_port_0_switch: endpoint {
> > > remote-endpoint = <&primary_qmp_phy>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > connector@1 {
> > > ports {
> > > port@0 {
> > > reg = <0>;
> > > ucsi_port_1_dp: endpoint {
> > > remote-endpoint = <&dp1_mode>;
> > > };
> > > };
> > >
> > > port@1 {
> > > reg = <1>;
> > > ucsi_port_1_switch: endpoint {
> > > remote-endpoint = <&second_qmp_phy>;
> > > };
> > > };
> > > };
> > > };
> > > };
> > >
> > > /* And then our 2 DP and single eDP controllers */
> > > &mdss_dp0 {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > dp0_mode: endpoint {
> > > remote-endpoint = <&ucsi_port_0_dp>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > &mdss_dp1 {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > dp1_mode: endpoint {
> > > remote-endpoint = <&ucsi_port_1_dp>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > &mdss_edp {
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > mdss_edp_out: endpoint {
> > > remote-endpoint = <&auo_b133han05_in>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > > If you have "port@1" pointing to a USB-C controller but this instance
> > > > of the DP controller is actually hooked up straight to a panel then
> > > > you should simply delete the "port@1" that points to the typeC and
> > > > replace it with one that points to a panel, right?
> > > >
> > >
> > > As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> > > connectors and the eDP points to the panel, exactly like we agreed.
> > >
> > > So now I call:
> > > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > >
> > > which for the two DP nodes will pass respective UCSI connector to
> > > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > > panel_list.
> > >
> > > There's nothing indicating in the of_graph that the USB connectors
> > > aren't panels (or bridges), so I don't see a way to distinguish the two
> > > types remotes.
To summarize where I think our out-of-band discussion went, I think
you're OK w/ keeping this at "port@1" for both the DP and eDP case and
we'll figure out _some_ way to make it work.
> > As far as I can tell the way this would be solved would be to actually
> > pass &bridge in and then make sure that a bridge would be in place for
> > the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
> > the connector hasn't been probed but once it's probed then it should
> > register as a bridge and thus give you the info you need (AKA that
> > this isn't a panel).
> >
> > I haven't done the digging to see how all this works, but according to
> > Laurent [1]: "Physical connectors are already handled as bridges, see
> > drivers/gpu/drm/bridge/display-connector.c"
> >
>
> All this seems to make sense for both eDP and "native" DP.
>
> > So basically I think this is solvable in code and there's no reason to
> > mess with the devicetree bindings to solve this problem. Does that
> > sound right?
> >
>
> But I don't have a DisplayPort connector.
>
> I have a USB-C connector, that upon determining that it's time to play
> DisplayPort will use the typec_mux abstraction to tell someone on the
> other side of the of_graph about DisplayPort events (HPD).
>
> So where would I put this drm_bridge in the USB-C case?
>
> I don't see that it fits in the Type-C side of things and putting it on
> the DP side would leave us with exactly the problem we have here. So we
> would have to put a fake "DP connector" inbetween the DP node and the
> Type-C controller?
>
>
> For reference, this is how I thought one is supposed to tie the Type-C
> controller to the display driver:
> https://lore.kernel.org/all/[email protected]/
OK, so I looked at that a bit. Fair warning that I've never looked at
the type C code before today so anything I say could be totally wrong!
:-)
...but I _think_ you're abusing the "mux" API for this. I think a type
C port can have exactly 1 mux, right? Right now you are claiming to be
_the_ mux in the DP driver, but what about for other alt modes? If
those wanted to be notified about similar things it would be
impossible because you're already _the_ mux, right?
I _think_ a mux is supposed to be something more like
`drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
the type C framework we're looking at here). There the phy can do all
the work of remuxing things / flipping orientation / etc. I don't
think it's a requirement that every SoC be able to do this remuxing
itself but (if memory serves) rk3399 implemented it so we didn't have
to do it on the TCPC and could use a cheaper solution there.
In any case, my point is that I think there is supposed to be a
_single_ mux per port that handles reassigning pins and that's what
this API is for.
...so I will still assert that the right thing to do is to have a
drm_bridge for the type c connector and _that's_ what should be
sending HPD.
> I'm afraid I must be missing something in Laurent and yours proposal
> (although I think Laurent is talking about the native DP case?).
>
> Regards,
> Bjorn
>
> > [1] https://lore.kernel.org/r/[email protected]/
On Tue 05 Oct 16:09 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 5, 2021 at 10:33 AM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Tue 05 Oct 08:39 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Mon, Oct 4, 2021 at 6:09 PM Bjorn Andersson
> > > <[email protected]> wrote:
> > > >
> > > > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > > > +{
> > > > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > > > + int rc;
> > > > > > > > +
> > > > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, &parser->drm_panel, NULL);
> > > > > > >
> > > > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > > > 1 here, right?
> > > > > > >
> > > > > >
> > > > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > > > at 2.
> > > > > >
> > > > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > > > reference to the USB-C controller, scan through the registered panels
> > > > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > > > panel and return -EPROBE_DEFER.
> > > > >
> > > > > I'm confused, but maybe it would help if I could see something
> > > > > concrete. Is there a specific board this was happening on?
> > > > >
> > > >
> > > > Right, let's make this more concrete with a snippet from the actual
> > > > SC8180x DT.
> > > >
> > > > > Under the DP node in the device tree I expect:
> > > > >
> > > > > ports {
> > > > > port@1 {
> > > > > reg = <1>;
> > > > > edp_out: endpoint {
> > > > > remote-endpoint = <&edp_panel_in>;
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > >
> > > > /* We got a panel */
> > > > panel {
> > > > ...
> > > > ports {
> > > > port {
> > > > auo_b133han05_in: endpoint {
> > > > remote-endpoint = <&mdss_edp_out>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > /* And a 2-port USB-C controller */
> > > > type-c-controller {
> > > > ...
> > > > connector@0 {
> > > > ports {
> > > > port@0 {
> > > > reg = <0>;
> > > > ucsi_port_0_dp: endpoint {
> > > > remote-endpoint = <&dp0_mode>;
> > > > };
> > > > };
> > > >
> > > > port@1 {
> > > > reg = <1>;
> > > > ucsi_port_0_switch: endpoint {
> > > > remote-endpoint = <&primary_qmp_phy>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > connector@1 {
> > > > ports {
> > > > port@0 {
> > > > reg = <0>;
> > > > ucsi_port_1_dp: endpoint {
> > > > remote-endpoint = <&dp1_mode>;
> > > > };
> > > > };
> > > >
> > > > port@1 {
> > > > reg = <1>;
> > > > ucsi_port_1_switch: endpoint {
> > > > remote-endpoint = <&second_qmp_phy>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > /* And then our 2 DP and single eDP controllers */
> > > > &mdss_dp0 {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > dp0_mode: endpoint {
> > > > remote-endpoint = <&ucsi_port_0_dp>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > &mdss_dp1 {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > dp1_mode: endpoint {
> > > > remote-endpoint = <&ucsi_port_1_dp>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > &mdss_edp {
> > > > ports {
> > > > port@1 {
> > > > reg = <1>;
> > > > mdss_edp_out: endpoint {
> > > > remote-endpoint = <&auo_b133han05_in>;
> > > > };
> > > > };
> > > > };
> > > > };
> > > >
> > > > > If you have "port@1" pointing to a USB-C controller but this instance
> > > > > of the DP controller is actually hooked up straight to a panel then
> > > > > you should simply delete the "port@1" that points to the typeC and
> > > > > replace it with one that points to a panel, right?
> > > > >
> > > >
> > > > As you can see, port 1 on &mdss_dp0 and &mdss_dp1 points to the two UCSI
> > > > connectors and the eDP points to the panel, exactly like we agreed.
> > > >
> > > > So now I call:
> > > > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > > >
> > > > which for the two DP nodes will pass respective UCSI connector to
> > > > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > > > panel_list.
> > > >
> > > > There's nothing indicating in the of_graph that the USB connectors
> > > > aren't panels (or bridges), so I don't see a way to distinguish the two
> > > > types remotes.
>
> To summarize where I think our out-of-band discussion went, I think
> you're OK w/ keeping this at "port@1" for both the DP and eDP case and
> we'll figure out _some_ way to make it work.
>
>
> > > As far as I can tell the way this would be solved would be to actually
> > > pass &bridge in and then make sure that a bridge would be in place for
> > > the DP connector. In the full DP case you'll get an -EPROBE_DEFER if
> > > the connector hasn't been probed but once it's probed then it should
> > > register as a bridge and thus give you the info you need (AKA that
> > > this isn't a panel).
> > >
> > > I haven't done the digging to see how all this works, but according to
> > > Laurent [1]: "Physical connectors are already handled as bridges, see
> > > drivers/gpu/drm/bridge/display-connector.c"
> > >
> >
> > All this seems to make sense for both eDP and "native" DP.
> >
> > > So basically I think this is solvable in code and there's no reason to
> > > mess with the devicetree bindings to solve this problem. Does that
> > > sound right?
> > >
> >
> > But I don't have a DisplayPort connector.
> >
> > I have a USB-C connector, that upon determining that it's time to play
> > DisplayPort will use the typec_mux abstraction to tell someone on the
> > other side of the of_graph about DisplayPort events (HPD).
> >
> > So where would I put this drm_bridge in the USB-C case?
> >
> > I don't see that it fits in the Type-C side of things and putting it on
> > the DP side would leave us with exactly the problem we have here. So we
> > would have to put a fake "DP connector" inbetween the DP node and the
> > Type-C controller?
> >
> >
> > For reference, this is how I thought one is supposed to tie the Type-C
> > controller to the display driver:
> > https://lore.kernel.org/all/[email protected]/
>
> OK, so I looked at that a bit. Fair warning that I've never looked at
> the type C code before today so anything I say could be totally wrong!
> :-)
>
> ...but I _think_ you're abusing the "mux" API for this. I think a type
> C port can have exactly 1 mux, right? Right now you are claiming to be
> _the_ mux in the DP driver, but what about for other alt modes? If
> those wanted to be notified about similar things it would be
> impossible because you're already _the_ mux, right?
>
I actually don't think so, because I acquire the typec_mux handle by the
means of:
mux_desc.svid = USB_TYPEC_DP_SID;
mux_desc.mode = USB_TYPEC_DP_MODE;
alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc);
And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>;
So I will be able to reference multiple different altmode
implementors using this scheme.
> I _think_ a mux is supposed to be something more like
> `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
> the type C framework we're looking at here). There the phy can do all
> the work of remuxing things / flipping orientation / etc. I don't
> think it's a requirement that every SoC be able to do this remuxing
> itself but (if memory serves) rk3399 implemented it so we didn't have
> to do it on the TCPC and could use a cheaper solution there.
>
I'm afraid I don't see how this interacts with a display controller. It
seems more like it's the phy side of things, what we have split between
the Type-C controller and the QMP phy to set the pins in the right
state.
> In any case, my point is that I think there is supposed to be a
> _single_ mux per port that handles reassigning pins and that's what
> this API is for.
>
If that's the case things such as typec_mux_match() is just completely
backwards.
> ...so I will still assert that the right thing to do is to have a
> drm_bridge for the type c connector and _that's_ what should be
> sending HPD.
>
That still implies that all the current typec_mux code got it all wrong
and should be thrown out. If you instead consider that you have a Type-C
controller that upon switching DisplayPort on/off calls typec_mux_set()
to inform the functions that things has changed then all the current
code makes sense.
It also maps nicely to how the TypeC controller would call
typec_switch_set() to inform, in our case the QMP phy that the
orientation has switched.
It seems reasonable to have some common helper code that registers the
typec_mux and turn its notifications into HPD notifications to the
display code, but I still think that should live in the DRM framework,
separate from the USB code.
Regards,
Bjorn
>
> > I'm afraid I must be missing something in Laurent and yours proposal
> > (although I think Laurent is talking about the native DP case?).
> >
> > Regards,
> > Bjorn
> >
> > > [1] https://lore.kernel.org/r/[email protected]/
Hi,
On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson
<[email protected]> wrote:
>
> > > For reference, this is how I thought one is supposed to tie the Type-C
> > > controller to the display driver:
> > > https://lore.kernel.org/all/[email protected]/
> >
> > OK, so I looked at that a bit. Fair warning that I've never looked at
> > the type C code before today so anything I say could be totally wrong!
> > :-)
> >
> > ...but I _think_ you're abusing the "mux" API for this. I think a type
> > C port can have exactly 1 mux, right? Right now you are claiming to be
> > _the_ mux in the DP driver, but what about for other alt modes? If
> > those wanted to be notified about similar things it would be
> > impossible because you're already _the_ mux, right?
> >
>
> I actually don't think so, because I acquire the typec_mux handle by the
> means of:
>
> mux_desc.svid = USB_TYPEC_DP_SID;
> mux_desc.mode = USB_TYPEC_DP_MODE;
> alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc);
Hrm, I guess I need to go find that code. Ah, I see it in your WIP
tree, but not posted anywhere. :-P The only code I can see calling
fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`.
In that code it passes NULL for the mux_desc and I'm nearly certain
that it just handles one "mux" per connector despite the fact that it
handles lots of different types of alternate modes. That doesn't mean
that the cros_ec implementation is correct / finalized, but it's a
reference point.
> And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>;
>
> So I will be able to reference multiple different altmode
> implementors using this scheme.
OK, so I'm trying to grok this more. Let's see.
I'm looking at ucsi_glink_probe() and looking at the matching dts in
your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so:
1. It's looping once per _connector_ by looping with
`device_for_each_child_node(dev, fwnode)`.
2. For each connector, it has exactly one `alt_port` structure.
3. For each `alt_port` structure it has exactly one `mux`.
...so currently with your WIP tree there is one "mux" per type C connector.
Perhaps what you're saying, though, is that the UCSI code in your WIP
tree can/should be changed to support more than one mux per port. Then
I guess it would have logic figuring out what muxes to notify about
which things? ...and I guess that would mean that it's currently a bug
that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about
USB changes?
> > I _think_ a mux is supposed to be something more like
> > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
> > the type C framework we're looking at here). There the phy can do all
> > the work of remuxing things / flipping orientation / etc. I don't
> > think it's a requirement that every SoC be able to do this remuxing
> > itself but (if memory serves) rk3399 implemented it so we didn't have
> > to do it on the TCPC and could use a cheaper solution there.
> >
>
> I'm afraid I don't see how this interacts with a display controller.
This was actually kinda my point. ;-) Specifically I think
`phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I
think your display controller isn't a mux. Yeah, it's handy that muxes
get told about DP HPD status, but that doesn't mean it's the right
abstraction for you to implement. In my mental model, it's the same as
implementing your "i2c" controller with a "pinctrl" driver. :-P
> It
> seems more like it's the phy side of things, what we have split between
> the Type-C controller and the QMP phy to set the pins in the right
> state.
>
> > In any case, my point is that I think there is supposed to be a
> > _single_ mux per port that handles reassigning pins and that's what
> > this API is for.
> >
>
> If that's the case things such as typec_mux_match() is just completely
> backwards.
Yeah, I have no explanation for typec_mux_match(). Let me see if I can
lure some type C folks into this discussion.
> > ...so I will still assert that the right thing to do is to have a
> > drm_bridge for the type c connector and _that's_ what should be
> > sending HPD.
> >
>
> That still implies that all the current typec_mux code got it all wrong
> and should be thrown out. If you instead consider that you have a Type-C
> controller that upon switching DisplayPort on/off calls typec_mux_set()
> to inform the functions that things has changed then all the current
> code makes sense.
>
> It also maps nicely to how the TypeC controller would call
> typec_switch_set() to inform, in our case the QMP phy that the
> orientation has switched.
>
>
> It seems reasonable to have some common helper code that registers the
> typec_mux and turn its notifications into HPD notifications to the
> display code, but I still think that should live in the DRM framework,
> separate from the USB code.
I think I'm going to step back and hope that the experts can chime in.
[1] https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
-Doug
(CC+ Heikki)
Hi,
On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > > > For reference, this is how I thought one is supposed to tie the Type-C
> > > > controller to the display driver:
> > > > https://lore.kernel.org/all/[email protected]/
> > >
> > > OK, so I looked at that a bit. Fair warning that I've never looked at
> > > the type C code before today so anything I say could be totally wrong!
> > > :-)
> > >
> > > ...but I _think_ you're abusing the "mux" API for this. I think a type
> > > C port can have exactly 1 mux, right? Right now you are claiming to be
> > > _the_ mux in the DP driver, but what about for other alt modes? If
> > > those wanted to be notified about similar things it would be
> > > impossible because you're already _the_ mux, right?
> > >
> >
> > I actually don't think so, because I acquire the typec_mux handle by the
> > means of:
> >
> > mux_desc.svid = USB_TYPEC_DP_SID;
> > mux_desc.mode = USB_TYPEC_DP_MODE;
> > alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc);
>
> Hrm, I guess I need to go find that code. Ah, I see it in your WIP
> tree, but not posted anywhere. :-P The only code I can see calling
> fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`.
> In that code it passes NULL for the mux_desc and I'm nearly certain
> that it just handles one "mux" per connector despite the fact that it
> handles lots of different types of alternate modes. That doesn't mean
> that the cros_ec implementation is correct / finalized, but it's a
> reference point.
>
>
> > And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>;
> >
> > So I will be able to reference multiple different altmode
> > implementors using this scheme.
>
> OK, so I'm trying to grok this more. Let's see.
>
> I'm looking at ucsi_glink_probe() and looking at the matching dts in
> your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so:
>
> 1. It's looping once per _connector_ by looping with
> `device_for_each_child_node(dev, fwnode)`.
>
> 2. For each connector, it has exactly one `alt_port` structure.
>
> 3. For each `alt_port` structure it has exactly one `mux`.
>
> ...so currently with your WIP tree there is one "mux" per type C connector.
>
>
> Perhaps what you're saying, though, is that the UCSI code in your WIP
> tree can/should be changed to support more than one mux per port. Then
> I guess it would have logic figuring out what muxes to notify about
> which things? ...and I guess that would mean that it's currently a bug
> that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about
> USB changes?
>
>
> > > I _think_ a mux is supposed to be something more like
> > > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
> > > the type C framework we're looking at here). There the phy can do all
> > > the work of remuxing things / flipping orientation / etc. I don't
> > > think it's a requirement that every SoC be able to do this remuxing
> > > itself but (if memory serves) rk3399 implemented it so we didn't have
> > > to do it on the TCPC and could use a cheaper solution there.
> > >
> >
> > I'm afraid I don't see how this interacts with a display controller.
>
> This was actually kinda my point. ;-) Specifically I think
> `phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I
> think your display controller isn't a mux. Yeah, it's handy that muxes
> get told about DP HPD status, but that doesn't mean it's the right
> abstraction for you to implement. In my mental model, it's the same as
> implementing your "i2c" controller with a "pinctrl" driver. :-P
>
>
> > It
> > seems more like it's the phy side of things, what we have split between
> > the Type-C controller and the QMP phy to set the pins in the right
> > state.
> >
> > > In any case, my point is that I think there is supposed to be a
> > > _single_ mux per port that handles reassigning pins and that's what
> > > this API is for.
> > >
> >
> > If that's the case things such as typec_mux_match() is just completely
> > backwards.
>
> Yeah, I have no explanation for typec_mux_match(). Let me see if I can
> lure some type C folks into this discussion.
This aligns with the model I have in my mind (not that that is
necessarily the right one).
I took that matching code to be meant to handle cases where the
firmware doesn't explicitly
define a "mode-switch" for the port (and so we look at the SVIDs
listed in the Mux fwnode descriptor).
The matcher code does suggest there could be a mux for each alternate
mode. But then, how does the
bus code know which mux to set [2] ? In that code, the struct altmode
has a pointer to the struct typec_mux, but I
don't see where that pointer is assigned. I assumed that it was set to
whatever the mux node of the
Type C port was whenever the port driver registered its altmodes for
each port, but I can't substantiate
that assumption in code.
Heikki, do you have any guidance regarding what the expected usage is
here? One typec_mux struct per type C port? Or
1 typec_mux per altmode per port?
>
>
> > > ...so I will still assert that the right thing to do is to have a
> > > drm_bridge for the type c connector and _that's_ what should be
> > > sending HPD.
> > >
> >
> > That still implies that all the current typec_mux code got it all wrong
> > and should be thrown out. If you instead consider that you have a Type-C
> > controller that upon switching DisplayPort on/off calls typec_mux_set()
> > to inform the functions that things has changed then all the current
> > code makes sense.
> >
> > It also maps nicely to how the TypeC controller would call
> > typec_switch_set() to inform, in our case the QMP phy that the
> > orientation has switched.
> >
> >
> > It seems reasonable to have some common helper code that registers the
> > typec_mux and turn its notifications into HPD notifications to the
> > display code, but I still think that should live in the DRM framework,
> > separate from the USB code.
>
> I think I'm going to step back and hope that the experts can chime in.
>
>
> [1] https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
[2]: https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/usb/typec/bus.c#L27
>
> -Doug
Hi guys,
On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
> (CC+ Heikki)
>
> Hi,
>
> On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> > > > > For reference, this is how I thought one is supposed to tie the Type-C
> > > > > controller to the display driver:
> > > > > https://lore.kernel.org/all/[email protected]/
> > > >
> > > > OK, so I looked at that a bit. Fair warning that I've never looked at
> > > > the type C code before today so anything I say could be totally wrong!
> > > > :-)
> > > >
> > > > ...but I _think_ you're abusing the "mux" API for this. I think a type
> > > > C port can have exactly 1 mux, right? Right now you are claiming to be
> > > > _the_ mux in the DP driver, but what about for other alt modes? If
> > > > those wanted to be notified about similar things it would be
> > > > impossible because you're already _the_ mux, right?
> > > >
> > >
> > > I actually don't think so, because I acquire the typec_mux handle by the
> > > means of:
> > >
> > > mux_desc.svid = USB_TYPEC_DP_SID;
> > > mux_desc.mode = USB_TYPEC_DP_MODE;
> > > alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc);
> >
> > Hrm, I guess I need to go find that code. Ah, I see it in your WIP
> > tree, but not posted anywhere. :-P The only code I can see calling
> > fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`.
> > In that code it passes NULL for the mux_desc and I'm nearly certain
> > that it just handles one "mux" per connector despite the fact that it
> > handles lots of different types of alternate modes. That doesn't mean
> > that the cros_ec implementation is correct / finalized, but it's a
> > reference point.
> >
> >
> > > And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>;
> > >
> > > So I will be able to reference multiple different altmode
> > > implementors using this scheme.
> >
> > OK, so I'm trying to grok this more. Let's see.
> >
> > I'm looking at ucsi_glink_probe() and looking at the matching dts in
> > your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so:
> >
> > 1. It's looping once per _connector_ by looping with
> > `device_for_each_child_node(dev, fwnode)`.
> >
> > 2. For each connector, it has exactly one `alt_port` structure.
> >
> > 3. For each `alt_port` structure it has exactly one `mux`.
> >
> > ...so currently with your WIP tree there is one "mux" per type C connector.
> >
> >
> > Perhaps what you're saying, though, is that the UCSI code in your WIP
> > tree can/should be changed to support more than one mux per port. Then
> > I guess it would have logic figuring out what muxes to notify about
> > which things? ...and I guess that would mean that it's currently a bug
> > that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about
> > USB changes?
> >
> >
> > > > I _think_ a mux is supposed to be something more like
> > > > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
> > > > the type C framework we're looking at here). There the phy can do all
> > > > the work of remuxing things / flipping orientation / etc. I don't
> > > > think it's a requirement that every SoC be able to do this remuxing
> > > > itself but (if memory serves) rk3399 implemented it so we didn't have
> > > > to do it on the TCPC and could use a cheaper solution there.
> > > >
> > >
> > > I'm afraid I don't see how this interacts with a display controller.
> >
> > This was actually kinda my point. ;-) Specifically I think
> > `phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I
> > think your display controller isn't a mux. Yeah, it's handy that muxes
> > get told about DP HPD status, but that doesn't mean it's the right
> > abstraction for you to implement. In my mental model, it's the same as
> > implementing your "i2c" controller with a "pinctrl" driver. :-P
> >
> >
> > > It
> > > seems more like it's the phy side of things, what we have split between
> > > the Type-C controller and the QMP phy to set the pins in the right
> > > state.
> > >
> > > > In any case, my point is that I think there is supposed to be a
> > > > _single_ mux per port that handles reassigning pins and that's what
> > > > this API is for.
> > > >
> > >
> > > If that's the case things such as typec_mux_match() is just completely
> > > backwards.
> >
> > Yeah, I have no explanation for typec_mux_match(). Let me see if I can
> > lure some type C folks into this discussion.
>
> This aligns with the model I have in my mind (not that that is
> necessarily the right one).
> I took that matching code to be meant to handle cases where the
> firmware doesn't explicitly
> define a "mode-switch" for the port (and so we look at the SVIDs
> listed in the Mux fwnode descriptor).
>
> The matcher code does suggest there could be a mux for each alternate
> mode. But then, how does the
> bus code know which mux to set [2] ? In that code, the struct altmode
> has a pointer to the struct typec_mux, but I
> don't see where that pointer is assigned. I assumed that it was set to
> whatever the mux node of the
> Type C port was whenever the port driver registered its altmodes for
> each port, but I can't substantiate
> that assumption in code.
>
> Heikki, do you have any guidance regarding what the expected usage is
> here? One typec_mux struct per type C port? Or
> 1 typec_mux per altmode per port?
I didn't go over the whole thread, so I may have misunderstood
something, but I don't think this has anything to do with muxes. The
mux should not be a problem for the DRM side under no circumstance.
Like Doug said, the mux API is being abused here.
HPD was one use case here, so I'll try to explain how that happens...
If the USB Type-C connector is in DP alt mode, then ideally your USB
Type-C controller/port driver has registered the partner device DP alt
mode the moment it detected that the partner supports that mode, and
that partner DP alt mode will have then been bind to the DP alt mode
driver:
drivers/usb/typec/altmodes/displayport.c
After that, if the DP alt mode driver sees HPD - HPD is message
signalled in DP alt mode (in case some of you guys didn't know) - the
DP alt mode driver notifies the DRM connector about it by calling
this function:
void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
If your USB Type-C controller/port driver does not yet register the DP
alt mode, the it's responsible of handling HPD separately by calling
drm_connector_oob_hotplug_event() on its own.
Either way, the only thing needed here is description of the
connection between the USB Type-C connector and the DisplayPort in
firmware - the mux is not relevant here. There are no DT bindings
defined for that AFAIK (or are there?), but presumable you want to use
OF graph with DT. Right now the DP alt mode driver does not try to
find the connection from device graph (so OF graph), but it should not
be a problem to add support for it.
> > > > ...so I will still assert that the right thing to do is to have a
> > > > drm_bridge for the type c connector and _that's_ what should be
> > > > sending HPD.
> > > >
> > >
> > > That still implies that all the current typec_mux code got it all wrong
> > > and should be thrown out. If you instead consider that you have a Type-C
> > > controller that upon switching DisplayPort on/off calls typec_mux_set()
> > > to inform the functions that things has changed then all the current
> > > code makes sense.
> > >
> > > It also maps nicely to how the TypeC controller would call
> > > typec_switch_set() to inform, in our case the QMP phy that the
> > > orientation has switched.
> > >
> > >
> > > It seems reasonable to have some common helper code that registers the
> > > typec_mux and turn its notifications into HPD notifications to the
> > > display code, but I still think that should live in the DRM framework,
> > > separate from the USB code.
> >
> > I think I'm going to step back and hope that the experts can chime in.
> >
> >
> > [1] https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
> [2]: https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/usb/typec/bus.c#L27
>
> >
> > -Doug
thanks,
--
heikki
On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote:
> Hi guys,
>
> On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
> > (CC+ Heikki)
> >
> > Hi,
> >
> > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson
> > > <[email protected]> wrote:
> > > >
> > > > > > For reference, this is how I thought one is supposed to tie the Type-C
> > > > > > controller to the display driver:
> > > > > > https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > > OK, so I looked at that a bit. Fair warning that I've never looked at
> > > > > the type C code before today so anything I say could be totally wrong!
> > > > > :-)
> > > > >
> > > > > ...but I _think_ you're abusing the "mux" API for this. I think a type
> > > > > C port can have exactly 1 mux, right? Right now you are claiming to be
> > > > > _the_ mux in the DP driver, but what about for other alt modes? If
> > > > > those wanted to be notified about similar things it would be
> > > > > impossible because you're already _the_ mux, right?
> > > > >
> > > >
> > > > I actually don't think so, because I acquire the typec_mux handle by the
> > > > means of:
> > > >
> > > > mux_desc.svid = USB_TYPEC_DP_SID;
> > > > mux_desc.mode = USB_TYPEC_DP_MODE;
> > > > alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc);
> > >
> > > Hrm, I guess I need to go find that code. Ah, I see it in your WIP
> > > tree, but not posted anywhere. :-P The only code I can see calling
> > > fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`.
> > > In that code it passes NULL for the mux_desc and I'm nearly certain
> > > that it just handles one "mux" per connector despite the fact that it
> > > handles lots of different types of alternate modes. That doesn't mean
> > > that the cros_ec implementation is correct / finalized, but it's a
> > > reference point.
> > >
> > >
> > > > And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>;
> > > >
> > > > So I will be able to reference multiple different altmode
> > > > implementors using this scheme.
> > >
> > > OK, so I'm trying to grok this more. Let's see.
> > >
> > > I'm looking at ucsi_glink_probe() and looking at the matching dts in
> > > your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so:
> > >
> > > 1. It's looping once per _connector_ by looping with
> > > `device_for_each_child_node(dev, fwnode)`.
> > >
> > > 2. For each connector, it has exactly one `alt_port` structure.
> > >
> > > 3. For each `alt_port` structure it has exactly one `mux`.
> > >
> > > ...so currently with your WIP tree there is one "mux" per type C connector.
> > >
> > >
> > > Perhaps what you're saying, though, is that the UCSI code in your WIP
> > > tree can/should be changed to support more than one mux per port. Then
> > > I guess it would have logic figuring out what muxes to notify about
> > > which things? ...and I guess that would mean that it's currently a bug
> > > that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about
> > > USB changes?
> > >
> > >
> > > > > I _think_ a mux is supposed to be something more like
> > > > > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates
> > > > > the type C framework we're looking at here). There the phy can do all
> > > > > the work of remuxing things / flipping orientation / etc. I don't
> > > > > think it's a requirement that every SoC be able to do this remuxing
> > > > > itself but (if memory serves) rk3399 implemented it so we didn't have
> > > > > to do it on the TCPC and could use a cheaper solution there.
> > > > >
> > > >
> > > > I'm afraid I don't see how this interacts with a display controller.
> > >
> > > This was actually kinda my point. ;-) Specifically I think
> > > `phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I
> > > think your display controller isn't a mux. Yeah, it's handy that muxes
> > > get told about DP HPD status, but that doesn't mean it's the right
> > > abstraction for you to implement. In my mental model, it's the same as
> > > implementing your "i2c" controller with a "pinctrl" driver. :-P
> > >
> > >
> > > > It
> > > > seems more like it's the phy side of things, what we have split between
> > > > the Type-C controller and the QMP phy to set the pins in the right
> > > > state.
> > > >
> > > > > In any case, my point is that I think there is supposed to be a
> > > > > _single_ mux per port that handles reassigning pins and that's what
> > > > > this API is for.
> > > > >
> > > >
> > > > If that's the case things such as typec_mux_match() is just completely
> > > > backwards.
> > >
> > > Yeah, I have no explanation for typec_mux_match(). Let me see if I can
> > > lure some type C folks into this discussion.
> >
> > This aligns with the model I have in my mind (not that that is
> > necessarily the right one).
> > I took that matching code to be meant to handle cases where the
> > firmware doesn't explicitly
> > define a "mode-switch" for the port (and so we look at the SVIDs
> > listed in the Mux fwnode descriptor).
> >
> > The matcher code does suggest there could be a mux for each alternate
> > mode. But then, how does the
> > bus code know which mux to set [2] ? In that code, the struct altmode
> > has a pointer to the struct typec_mux, but I
> > don't see where that pointer is assigned. I assumed that it was set to
> > whatever the mux node of the
> > Type C port was whenever the port driver registered its altmodes for
> > each port, but I can't substantiate
> > that assumption in code.
> >
> > Heikki, do you have any guidance regarding what the expected usage is
> > here? One typec_mux struct per type C port? Or
> > 1 typec_mux per altmode per port?
>
> I didn't go over the whole thread, so I may have misunderstood
> something, but I don't think this has anything to do with muxes. The
> mux should not be a problem for the DRM side under no circumstance.
> Like Doug said, the mux API is being abused here.
>
No need to read up on the thread, your answer further confirms the
understanding gained in a lengthy offline chat we had yesterday
afternoon as well.
> HPD was one use case here, so I'll try to explain how that happens...
>
> If the USB Type-C connector is in DP alt mode, then ideally your USB
> Type-C controller/port driver has registered the partner device DP alt
> mode the moment it detected that the partner supports that mode, and
> that partner DP alt mode will have then been bind to the DP alt mode
> driver:
>
> drivers/usb/typec/altmodes/displayport.c
>
> After that, if the DP alt mode driver sees HPD - HPD is message
> signalled in DP alt mode (in case some of you guys didn't know) - the
> DP alt mode driver notifies the DRM connector about it by calling
> this function:
>
> void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
>
> If your USB Type-C controller/port driver does not yet register the DP
> alt mode, the it's responsible of handling HPD separately by calling
> drm_connector_oob_hotplug_event() on its own.
>
The drm_connector_oob_hotplug_event() didn't exist when I tried to get
this working earlier this year and I couldn't figure out what the
intended design was to feed the HPD information into our DP driver.
Misplacing the typec_mux made all the pieces fall in place and it looked
good, but I now agree that the typec_mux should be used to mux in/out
the DP PHY on the pads as a result of the PD negotiation and then
separate of that the HPD signals should be sent towards the DRM driver
using drm_connector_oob_hotplug_event() - hopefully by reusing the
displayport altmode driver, but I still need to figure out how to
incorporate that in my custom TypeC controller driver.
> Either way, the only thing needed here is description of the
> connection between the USB Type-C connector and the DisplayPort in
> firmware - the mux is not relevant here. There are no DT bindings
> defined for that AFAIK (or are there?), but presumable you want to use
> OF graph with DT. Right now the DP alt mode driver does not try to
> find the connection from device graph (so OF graph), but it should not
> be a problem to add support for it.
>
I'll poke around and see what's missing to get
drm_connector_oob_hotplug_event() work in my model.
>
The one thing that I still don't understand though is, if the typec_mux
is used by the typec controller to inform _the_ mux about the function
to be used, what's up with the complexity in typec_mux_match()? This is
what lead me to believe that typec_mux was enabling/disabling individual
altmodes, rather just flipping the physical switch at the bottom.
Thanks,
Bjorn
> > > > > ...so I will still assert that the right thing to do is to have a
> > > > > drm_bridge for the type c connector and _that's_ what should be
> > > > > sending HPD.
> > > > >
> > > >
> > > > That still implies that all the current typec_mux code got it all wrong
> > > > and should be thrown out. If you instead consider that you have a Type-C
> > > > controller that upon switching DisplayPort on/off calls typec_mux_set()
> > > > to inform the functions that things has changed then all the current
> > > > code makes sense.
> > > >
> > > > It also maps nicely to how the TypeC controller would call
> > > > typec_switch_set() to inform, in our case the QMP phy that the
> > > > orientation has switched.
> > > >
> > > >
> > > > It seems reasonable to have some common helper code that registers the
> > > > typec_mux and turn its notifications into HPD notifications to the
> > > > display code, but I still think that should live in the DRM framework,
> > > > separate from the USB code.
> > >
> > > I think I'm going to step back and hope that the experts can chime in.
> > >
> > >
> > > [1] https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
> > [2]: https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/usb/typec/bus.c#L27
> >
> > >
> > > -Doug
>
> thanks,
>
> --
> heikki
Hi,
On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> The one thing that I still don't understand though is, if the typec_mux
> is used by the typec controller to inform _the_ mux about the function
> to be used, what's up with the complexity in typec_mux_match()? This is
> what lead me to believe that typec_mux was enabling/disabling individual
> altmodes, rather just flipping the physical switch at the bottom.
Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
the code in that function is not used by anybody. If I remember
correctly, all that complexity is attempting to solve some
hypothetical corner case(s). Probable a case where we have multiple
muxes per port to deal with.
I think it would probable be best to clean the function to the bare
minimum by keeping only the parts that are actually used today
(attached).
thanks,
--
heikki
On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote:
> On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
> > (CC+ Heikki)
[..]
> > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
[..]
> void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
>
> If your USB Type-C controller/port driver does not yet register the DP
> alt mode, the it's responsible of handling HPD separately by calling
> drm_connector_oob_hotplug_event() on its own.
>
Finally found my way back to this topic and it doesn't look like I can
reuse the existing altmode code with the firmware interface provided by
Qualcomm, so I just hacked something up that invokes
drm_connector_oob_hotplug_event().
But I'm not able to make sense of what the expected usage is. Reading
altmode/displayport.c, it seems that I should only invoke
drm_connector_oob_hotplug_event() as HPD state toggles.
I made a trial implementation of this, where my firmware interface
driver calls drm_connector_oob_hotplug_event() every time HPD state
changes and then in my oob_hotplug_event callback I flip the DP
controller between on and off.
Unfortunately when I then connect my HDMI dongle, I get HPD state HIGH,
call the oob_hotplug_event, the DP driver powers up and concludes that
there's nothing connected to the dongle and goes to idle. I then connect
the HDMI cable to the dongle, the firmware sends me another message with
HPD irq and state HIGH, which I ignore because it's not a change in
state.
In the end I hacked up drm_connector_oob_hotplug_event() to allow me to
pass the HPD state and this solves my problem. I can now distinguish
between connect, disconnect and attention.
Can you please help shed some light on what I might be missing?
Thanks,
Bjorn
+Hans and Imre
On Mon, Dec 06, 2021 at 02:31:40PM -0800, Bjorn Andersson wrote:
> On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote:
> > On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
> > > (CC+ Heikki)
> [..]
> > > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
> [..]
> > void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
> >
> > If your USB Type-C controller/port driver does not yet register the DP
> > alt mode, the it's responsible of handling HPD separately by calling
> > drm_connector_oob_hotplug_event() on its own.
> >
>
> Finally found my way back to this topic and it doesn't look like I can
> reuse the existing altmode code with the firmware interface provided by
> Qualcomm, so I just hacked something up that invokes
> drm_connector_oob_hotplug_event().
>
> But I'm not able to make sense of what the expected usage is. Reading
> altmode/displayport.c, it seems that I should only invoke
> drm_connector_oob_hotplug_event() as HPD state toggles.
>
> I made a trial implementation of this, where my firmware interface
> driver calls drm_connector_oob_hotplug_event() every time HPD state
> changes and then in my oob_hotplug_event callback I flip the DP
> controller between on and off.
>
> Unfortunately when I then connect my HDMI dongle, I get HPD state HIGH,
> call the oob_hotplug_event, the DP driver powers up and concludes that
> there's nothing connected to the dongle and goes to idle. I then connect
> the HDMI cable to the dongle, the firmware sends me another message with
> HPD irq and state HIGH, which I ignore because it's not a change in
> state.
>
> In the end I hacked up drm_connector_oob_hotplug_event() to allow me to
> pass the HPD state and this solves my problem. I can now distinguish
> between connect, disconnect and attention.
>
> Can you please help shed some light on what I might be missing?
Originally I wanted an API that we could use to pass all the details
that we have in the USB Type-C drivers (that would be the
configuration and status) to the GPU drivers, but Hans was against
that because, if I remember correctly, the OOB hotplug event may need
to be delivered to the GPU drivers in some cases even when the
connector is not USB Type-C connector, and he wanted a common API.
Hans, please correct me if I got it wrong.
I think that the GPU drivers need to handle USB Type-C connectors
separately one way or the other, but maybe the notification from the
connector can continue to be generic - not USB Type-C specific.
Imre proposed that the GPU drivers should be able to query the
DisplayPort configuration and status from the USB Type-C drivers
instead of the USB Type-C drivers just dumping the information
together with the notification about some event (so connection,
disconnection or attention) like I originally proposed. Imre, please
correct me if I misunderstood you :-).
I'm fine with anything, but we do need improvement here as you guys
can see.
thanks,
--
heikki
Hi all,
On 12/7/21 13:26, Heikki Krogerus wrote:
> +Hans and Imre
>
> On Mon, Dec 06, 2021 at 02:31:40PM -0800, Bjorn Andersson wrote:
>> On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote:
>>> On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
>>>> (CC+ Heikki)
>> [..]
>>>> On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
>> [..]
>>> void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
>>>
>>> If your USB Type-C controller/port driver does not yet register the DP
>>> alt mode, the it's responsible of handling HPD separately by calling
>>> drm_connector_oob_hotplug_event() on its own.
>>>
>>
>> Finally found my way back to this topic and it doesn't look like I can
>> reuse the existing altmode code with the firmware interface provided by
>> Qualcomm, so I just hacked something up that invokes
>> drm_connector_oob_hotplug_event().
>>
>> But I'm not able to make sense of what the expected usage is. Reading
>> altmode/displayport.c, it seems that I should only invoke
>> drm_connector_oob_hotplug_event() as HPD state toggles.
>>
>> I made a trial implementation of this, where my firmware interface
>> driver calls drm_connector_oob_hotplug_event() every time HPD state
>> changes and then in my oob_hotplug_event callback I flip the DP
>> controller between on and off.
>>
>> Unfortunately when I then connect my HDMI dongle, I get HPD state HIGH,
>> call the oob_hotplug_event, the DP driver powers up and concludes that
>> there's nothing connected to the dongle and goes to idle. I then connect
>> the HDMI cable to the dongle, the firmware sends me another message with
>> HPD irq and state HIGH, which I ignore because it's not a change in
>> state.
>>
>> In the end I hacked up drm_connector_oob_hotplug_event() to allow me to
>> pass the HPD state and this solves my problem. I can now distinguish
>> between connect, disconnect and attention.
>>
>> Can you please help shed some light on what I might be missing?
The plan always was to pass some extra information, like the number
of available DP lanes (which can make training faster) along as
parameter to the drm_connector_oob_hotplug_event().
The merged version ended up not doing this because there were no
consumers, but passing additional info like HPD state definitely
is ok.
Regards,
Hans
i
On Tue 07 Dec 08:56 PST 2021, Hans de Goede wrote:
> Hi all,
>
> On 12/7/21 13:26, Heikki Krogerus wrote:
> > +Hans and Imre
> >
> > On Mon, Dec 06, 2021 at 02:31:40PM -0800, Bjorn Andersson wrote:
> >> On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote:
> >>> On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote:
> >>>> (CC+ Heikki)
> >> [..]
> >>>> On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <[email protected]> wrote:
> >> [..]
> >>> void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode);
> >>>
> >>> If your USB Type-C controller/port driver does not yet register the DP
> >>> alt mode, the it's responsible of handling HPD separately by calling
> >>> drm_connector_oob_hotplug_event() on its own.
> >>>
> >>
> >> Finally found my way back to this topic and it doesn't look like I can
> >> reuse the existing altmode code with the firmware interface provided by
> >> Qualcomm, so I just hacked something up that invokes
> >> drm_connector_oob_hotplug_event().
> >>
> >> But I'm not able to make sense of what the expected usage is. Reading
> >> altmode/displayport.c, it seems that I should only invoke
> >> drm_connector_oob_hotplug_event() as HPD state toggles.
> >>
> >> I made a trial implementation of this, where my firmware interface
> >> driver calls drm_connector_oob_hotplug_event() every time HPD state
> >> changes and then in my oob_hotplug_event callback I flip the DP
> >> controller between on and off.
> >>
> >> Unfortunately when I then connect my HDMI dongle, I get HPD state HIGH,
> >> call the oob_hotplug_event, the DP driver powers up and concludes that
> >> there's nothing connected to the dongle and goes to idle. I then connect
> >> the HDMI cable to the dongle, the firmware sends me another message with
> >> HPD irq and state HIGH, which I ignore because it's not a change in
> >> state.
> >>
> >> In the end I hacked up drm_connector_oob_hotplug_event() to allow me to
> >> pass the HPD state and this solves my problem. I can now distinguish
> >> between connect, disconnect and attention.
> >>
> >> Can you please help shed some light on what I might be missing?
>
> The plan always was to pass some extra information, like the number
> of available DP lanes (which can make training faster) along as
> parameter to the drm_connector_oob_hotplug_event().
>
> The merged version ended up not doing this because there were no
> consumers, but passing additional info like HPD state definitely
> is ok.
>
Thanks, that clarifies things.
I think it makes sense to pass #lanes, as that would rule out the
possibility of attempting to run 4 lanes per dpcd information over a
2-lane mux configuration as well.
I will write up some patches.
Regards,
Bjorn
On Tue, Dec 07, 2021 at 02:26:05PM +0200, Heikki Krogerus wrote:
> +Hans and Imre
> [...]
>
> Originally I wanted an API that we could use to pass all the details
> that we have in the USB Type-C drivers (that would be the
> configuration and status) to the GPU drivers, but Hans was against
> that because, if I remember correctly, the OOB hotplug event may need
> to be delivered to the GPU drivers in some cases even when the
> connector is not USB Type-C connector, and he wanted a common API.
> Hans, please correct me if I got it wrong.
>
> I think that the GPU drivers need to handle USB Type-C connectors
> separately one way or the other, but maybe the notification from the
> connector can continue to be generic - not USB Type-C specific.
>
> Imre proposed that the GPU drivers should be able to query the
> DisplayPort configuration and status from the USB Type-C drivers
> instead of the USB Type-C drivers just dumping the information
> together with the notification about some event (so connection,
> disconnection or attention) like I originally proposed. Imre, please
> correct me if I misunderstood you :-).
I think the link config may be useful on Intel systems as well where the
hotplug event is delivered by another mean, the Gfx driver getting an
actual HW interrupt. Also on systems where the hotplug event does get
delivered OOB, the Gfx driver may want to query the current state, for
instance during booting or system resuming, independently of a hotplug
event.
Based on the above imo it would make sense to decouple the hotplug event
notification and the link configuration querying interface, or at least
make the querying possible in addition to the notification.
> I'm fine with anything, but we do need improvement here as you guys
> can see.
>
> thanks,
>
> --
> heikki
On Fri, Oct 08, 2021 at 03:38:21PM +0300, Heikki Krogerus wrote:
> Hi,
>
> On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> > The one thing that I still don't understand though is, if the typec_mux
> > is used by the typec controller to inform _the_ mux about the function
> > to be used, what's up with the complexity in typec_mux_match()? This is
> > what lead me to believe that typec_mux was enabling/disabling individual
> > altmodes, rather just flipping the physical switch at the bottom.
>
> Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
> the code in that function is not used by anybody. If I remember
> correctly, all that complexity is attempting to solve some
> hypothetical corner case(s). Probable a case where we have multiple
> muxes per port to deal with.
>
> I think it would probable be best to clean the function to the bare
> minimum by keeping only the parts that are actually used today
> (attached).
>
Sorry for not replying to this in a timely manner Heikki. I've been
ignoring this issue for a long time now, just adding "svid" to our dts
files. But, this obviously shows up in DT validation - and I'd prefer
not defining these properties as valid.
The attached patch works as expected.
Could you please spin this as a proper patch, so we can get it merged?
Regards,
Bjorn
> thanks,
>
> --
> heikki
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index c8340de0ed495..44f168c9bd9bf 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -193,56 +193,15 @@ static int mux_fwnode_match(struct device *dev, const void *fwnode)
> static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
> void *data)
> {
> - const struct typec_altmode_desc *desc = data;
> struct device *dev;
> - bool match;
> - int nval;
> - u16 *val;
> - int ret;
> - int i;
>
> /*
> - * Check has the identifier already been "consumed". If it
> - * has, no need to do any extra connection identification.
> + * The connection identifier will be needed with device graph (OF graph).
> + * Device graph is not supported by this code yet, so bailing out.
> */
> - match = !id;
> - if (match)
> - goto find_mux;
> -
> - /* Accessory Mode muxes */
> - if (!desc) {
> - match = fwnode_property_present(fwnode, "accessory");
> - if (match)
> - goto find_mux;
> - return NULL;
> - }
> -
> - /* Alternate Mode muxes */
> - nval = fwnode_property_count_u16(fwnode, "svid");
> - if (nval <= 0)
> - return NULL;
> -
> - val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> - if (!val)
> - return ERR_PTR(-ENOMEM);
> -
> - ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
> - if (ret < 0) {
> - kfree(val);
> - return ERR_PTR(ret);
> - }
> -
> - for (i = 0; i < nval; i++) {
> - match = val[i] == desc->svid;
> - if (match) {
> - kfree(val);
> - goto find_mux;
> - }
> - }
> - kfree(val);
> - return NULL;
> + if (id)
> + return ERR_PTR(-ENOTSUPP);
>
> -find_mux:
> dev = class_find_device(&typec_mux_class, NULL, fwnode,
> mux_fwnode_match);
>
On Mon, May 22, 2023 at 03:51:01PM -0500, Bjorn Andersson wrote:
> On Fri, Oct 08, 2021 at 03:38:21PM +0300, Heikki Krogerus wrote:
> > Hi,
> >
> > On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> > > The one thing that I still don't understand though is, if the typec_mux
> > > is used by the typec controller to inform _the_ mux about the function
> > > to be used, what's up with the complexity in typec_mux_match()? This is
> > > what lead me to believe that typec_mux was enabling/disabling individual
> > > altmodes, rather just flipping the physical switch at the bottom.
> >
> > Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
> > the code in that function is not used by anybody. If I remember
> > correctly, all that complexity is attempting to solve some
> > hypothetical corner case(s). Probable a case where we have multiple
> > muxes per port to deal with.
> >
> > I think it would probable be best to clean the function to the bare
> > minimum by keeping only the parts that are actually used today
> > (attached).
> >
>
> Sorry for not replying to this in a timely manner Heikki. I've been
> ignoring this issue for a long time now, just adding "svid" to our dts
> files. But, this obviously shows up in DT validation - and I'd prefer
> not defining these properties as valid.
>
> The attached patch works as expected.
>
Sorry, I must have failed at applying the patch - it doesn't work...
> Could you please spin this as a proper patch, so we can get it merged?
>
> Regards,
> Bjorn
>
> > thanks,
> >
> > --
> > heikki
>
> > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> > index c8340de0ed495..44f168c9bd9bf 100644
> > --- a/drivers/usb/typec/mux.c
> > +++ b/drivers/usb/typec/mux.c
> > @@ -193,56 +193,15 @@ static int mux_fwnode_match(struct device *dev, const void *fwnode)
> > static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
> > void *data)
> > {
> > - const struct typec_altmode_desc *desc = data;
> > struct device *dev;
> > - bool match;
> > - int nval;
> > - u16 *val;
> > - int ret;
> > - int i;
> >
> > /*
> > - * Check has the identifier already been "consumed". If it
> > - * has, no need to do any extra connection identification.
> > + * The connection identifier will be needed with device graph (OF graph).
> > + * Device graph is not supported by this code yet, so bailing out.
> > */
> > - match = !id;
> > - if (match)
> > - goto find_mux;
> > -
> > - /* Accessory Mode muxes */
> > - if (!desc) {
> > - match = fwnode_property_present(fwnode, "accessory");
> > - if (match)
> > - goto find_mux;
> > - return NULL;
> > - }
> > -
> > - /* Alternate Mode muxes */
> > - nval = fwnode_property_count_u16(fwnode, "svid");
> > - if (nval <= 0)
> > - return NULL;
> > -
> > - val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > - if (!val)
> > - return ERR_PTR(-ENOMEM);
> > -
> > - ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
> > - if (ret < 0) {
> > - kfree(val);
> > - return ERR_PTR(ret);
> > - }
> > -
> > - for (i = 0; i < nval; i++) {
> > - match = val[i] == desc->svid;
> > - if (match) {
> > - kfree(val);
> > - goto find_mux;
> > - }
> > - }
> > - kfree(val);
> > - return NULL;
> > + if (id)
We pass id as "mode-switch", so this will never be NULL. But we also only
want to consider endpoints with "mode-switch", otherwise we'll fail if
any of the referred endpoints is not implementing a typec_mux...
So this needs the same snippet we find in typec_switch_match():
/*
* Device graph (OF graph) does not give any means to identify the
* device type or the device class of the remote port parent that @fwnode
* represents, so in order to identify the type or the class of @fwnode
* an additional device property is needed. With typec switches the
* property is named "orientation-switch" (@id). The value of the device
* property is ignored.
*/
if (id && !fwnode_property_present(fwnode, id))
return NULL;
With that, this works as expected!
Regards,
Bjorn
> > + return ERR_PTR(-ENOTSUPP);
> >
> > -find_mux:
> > dev = class_find_device(&typec_mux_class, NULL, fwnode,
> > mux_fwnode_match);
> >
>
On Mon, May 22, 2023 at 02:53:48PM -0700, Bjorn Andersson wrote:
> On Mon, May 22, 2023 at 03:51:01PM -0500, Bjorn Andersson wrote:
> > On Fri, Oct 08, 2021 at 03:38:21PM +0300, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Thu, Oct 07, 2021 at 09:15:12AM -0700, Bjorn Andersson wrote:
> > > > The one thing that I still don't understand though is, if the typec_mux
> > > > is used by the typec controller to inform _the_ mux about the function
> > > > to be used, what's up with the complexity in typec_mux_match()? This is
> > > > what lead me to believe that typec_mux was enabling/disabling individual
> > > > altmodes, rather just flipping the physical switch at the bottom.
> > >
> > > Ah, typec_mux_match() is a mess. I'm sorry about that. I think most of
> > > the code in that function is not used by anybody. If I remember
> > > correctly, all that complexity is attempting to solve some
> > > hypothetical corner case(s). Probable a case where we have multiple
> > > muxes per port to deal with.
> > >
> > > I think it would probable be best to clean the function to the bare
> > > minimum by keeping only the parts that are actually used today
> > > (attached).
> > >
> >
> > Sorry for not replying to this in a timely manner Heikki. I've been
> > ignoring this issue for a long time now, just adding "svid" to our dts
> > files. But, this obviously shows up in DT validation - and I'd prefer
> > not defining these properties as valid.
> >
> > The attached patch works as expected.
> >
>
> Sorry, I must have failed at applying the patch - it doesn't work...
>
> > Could you please spin this as a proper patch, so we can get it merged?
> >
> > Regards,
> > Bjorn
> >
> > > thanks,
> > >
> > > --
> > > heikki
> >
> > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> > > index c8340de0ed495..44f168c9bd9bf 100644
> > > --- a/drivers/usb/typec/mux.c
> > > +++ b/drivers/usb/typec/mux.c
> > > @@ -193,56 +193,15 @@ static int mux_fwnode_match(struct device *dev, const void *fwnode)
> > > static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id,
> > > void *data)
> > > {
> > > - const struct typec_altmode_desc *desc = data;
> > > struct device *dev;
> > > - bool match;
> > > - int nval;
> > > - u16 *val;
> > > - int ret;
> > > - int i;
> > >
> > > /*
> > > - * Check has the identifier already been "consumed". If it
> > > - * has, no need to do any extra connection identification.
> > > + * The connection identifier will be needed with device graph (OF graph).
> > > + * Device graph is not supported by this code yet, so bailing out.
> > > */
> > > - match = !id;
> > > - if (match)
> > > - goto find_mux;
> > > -
> > > - /* Accessory Mode muxes */
> > > - if (!desc) {
> > > - match = fwnode_property_present(fwnode, "accessory");
> > > - if (match)
> > > - goto find_mux;
> > > - return NULL;
> > > - }
> > > -
> > > - /* Alternate Mode muxes */
> > > - nval = fwnode_property_count_u16(fwnode, "svid");
> > > - if (nval <= 0)
> > > - return NULL;
> > > -
> > > - val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
> > > - if (!val)
> > > - return ERR_PTR(-ENOMEM);
> > > -
> > > - ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval);
> > > - if (ret < 0) {
> > > - kfree(val);
> > > - return ERR_PTR(ret);
> > > - }
> > > -
> > > - for (i = 0; i < nval; i++) {
> > > - match = val[i] == desc->svid;
> > > - if (match) {
> > > - kfree(val);
> > > - goto find_mux;
> > > - }
> > > - }
> > > - kfree(val);
> > > - return NULL;
> > > + if (id)
>
> We pass id as "mode-switch", so this will never be NULL. But we also only
> want to consider endpoints with "mode-switch", otherwise we'll fail if
> any of the referred endpoints is not implementing a typec_mux...
>
> So this needs the same snippet we find in typec_switch_match():
>
> /*
> * Device graph (OF graph) does not give any means to identify the
> * device type or the device class of the remote port parent that @fwnode
> * represents, so in order to identify the type or the class of @fwnode
> * an additional device property is needed. With typec switches the
> * property is named "orientation-switch" (@id). The value of the device
> * property is ignored.
> */
> if (id && !fwnode_property_present(fwnode, id))
> return NULL;
>
> With that, this works as expected!
Okay. I'll change that and send the patch out.
thanks,
--
heikki