| Message ID | 20190319104114.12077-22-tomi.valkeinen@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
> tc358767 driver doesn't have any HPD handling at the moment, as it was > originally developed to support only eDP. > This patch implements a naive, polling-based HPD handling, which is used > when the driver is in DP mode. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 18 deletions(-) > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 8606de29c9b2..2b252f7ac070 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > struct tc_data *tc = bridge_to_tc(bridge); > int ret; > + ret = tc_get_display_props(tc); > + if (ret < 0) { > + dev_err(tc->dev, "failed to read display props: %d\n", ret); > + return; > + } > + > ret = tc_main_link_enable(tc); > if (ret < 0) { > dev_err(tc->dev, "main link enable error: %d\n", ret); > @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector) > return count; > } > -static void tc_connector_set_polling(struct tc_data *tc, > - struct drm_connector *connector) > -{ > - /* TODO: add support for HPD */ > - connector->polled = DRM_CONNECTOR_POLL_CONNECT | > - DRM_CONNECTOR_POLL_DISCONNECT; > -} > - > static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { > .get_modes = tc_connector_get_modes, > }; > +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct tc_data *tc = connector_to_tc(connector); > + u32 val; > + int ret; > + bool conn; > + > + tc_read(GPIOI, &val); > + > + conn = val & BIT(0); TC358767 has two GPIO pins that can be used for HPD signal. I think instead of hardcoding GPIO0 here it would be more flexible to expose boths gpios as a gpiochip and use gpiolib API to query the value of HPD as well as use "hpd-gpios" binidng in DT to select which input to use. Another argument in favour of this solution is that Toshiba's FAEs (at least some) recommend thier customers to connect HPD signal to SoC's GPIOs and bypass TC358767 entirely. Their reasoning being that TC358767 implements a generic GPIO contoller and there's no advantage in going through TC358767 if you could use your "normal" GPIOs. Using gpiolib API would allow us to handle both usecase with the same code. Lastly, by treating "hpd-gpios" as an optional property, we can preserve old driver behaviour regardless if it is connected to DP or eDP panel. Not saying that this is really worth doing, just pointing out that this option would be on the table as well. Thanks, Andrey Smirnov
On Tue, Mar 19, 2019 at 11:18 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > tc358767 driver doesn't have any HPD handling at the moment, as it was > > originally developed to support only eDP. > > > This patch implements a naive, polling-based HPD handling, which is used > > when the driver is in DP mode. > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > --- > > drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++---------- > > 1 file changed, 38 insertions(+), 18 deletions(-) > > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > > index 8606de29c9b2..2b252f7ac070 100644 > > --- a/drivers/gpu/drm/bridge/tc358767.c > > +++ b/drivers/gpu/drm/bridge/tc358767.c > > @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > > struct tc_data *tc = bridge_to_tc(bridge); > > int ret; > > > + ret = tc_get_display_props(tc); > > + if (ret < 0) { > > + dev_err(tc->dev, "failed to read display props: %d\n", ret); > > + return; > > + } > > + > > ret = tc_main_link_enable(tc); > > if (ret < 0) { > > dev_err(tc->dev, "main link enable error: %d\n", ret); > > @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector) > > return count; > > } > > > -static void tc_connector_set_polling(struct tc_data *tc, > > - struct drm_connector *connector) > > -{ > > - /* TODO: add support for HPD */ > > - connector->polled = DRM_CONNECTOR_POLL_CONNECT | > > - DRM_CONNECTOR_POLL_DISCONNECT; > > -} > > - > > static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { > > .get_modes = tc_connector_get_modes, > > }; > > > +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) > > +{ > > + struct tc_data *tc = connector_to_tc(connector); > > + u32 val; > > + int ret; > > + bool conn; > > + > > + tc_read(GPIOI, &val); > > + > > + conn = val & BIT(0); Another thing I noticed when trying this patch is that tc_connector_detect() will get called via drm_helper_probe_detect() even if tc->panel is not NULL and tc->connector.polled is zero, which creates a problem for eDP use-case. Thanks, Andrey Smirnov
On 19/03/2019 20:18, Andrey Smirnov wrote: > TC358767 has two GPIO pins that can be used for HPD signal. I think > instead of hardcoding GPIO0 here it would be more flexible to expose > boths gpios as a gpiochip and use gpiolib API to query the value of > HPD as well as use "hpd-gpios" binidng in DT to select which input to > use. > > Another argument in favour of this solution is that Toshiba's FAEs (at > least some) recommend thier customers to connect HPD signal to SoC's > GPIOs and bypass TC358767 entirely. Their reasoning being that > TC358767 implements a generic GPIO contoller and there's no advantage > in going through TC358767 if you could use your "normal" GPIOs. > > Using gpiolib API would allow us to handle both usecase with the same > code. > > Lastly, by treating "hpd-gpios" as an optional property, we can > preserve old driver behaviour regardless if it is connected to DP or > eDP panel. Not saying that this is really worth doing, just pointing > out that this option would be on the table as well. I think that's a good point. There's one thing that TC358767 offers, which may not be available on most generic GPIO controllers, though: it can detect short (programmable length) pulses, thus it's possible to easily implement the DisplayPort IRQ mechanism. I'm not sure if it's possible to implement reliable DP IRQ detection without HW support. Still, I think using standard gpios makes sense. Tomi
On 20/03/2019 07:55, Andrey Smirnov wrote: > On Tue, Mar 19, 2019 at 11:18 AM Andrey Smirnov > <andrew.smirnov@gmail.com> wrote: >> >>> tc358767 driver doesn't have any HPD handling at the moment, as it was >>> originally developed to support only eDP. >> >>> This patch implements a naive, polling-based HPD handling, which is used >>> when the driver is in DP mode. >> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> --- >>> drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++---------- >>> 1 file changed, 38 insertions(+), 18 deletions(-) >> >>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >>> index 8606de29c9b2..2b252f7ac070 100644 >>> --- a/drivers/gpu/drm/bridge/tc358767.c >>> +++ b/drivers/gpu/drm/bridge/tc358767.c >>> @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) >>> struct tc_data *tc = bridge_to_tc(bridge); >>> int ret; >> >>> + ret = tc_get_display_props(tc); >>> + if (ret < 0) { >>> + dev_err(tc->dev, "failed to read display props: %d\n", ret); >>> + return; >>> + } >>> + >>> ret = tc_main_link_enable(tc); >>> if (ret < 0) { >>> dev_err(tc->dev, "main link enable error: %d\n", ret); >>> @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector) >>> return count; >>> } >> >>> -static void tc_connector_set_polling(struct tc_data *tc, >>> - struct drm_connector *connector) >>> -{ >>> - /* TODO: add support for HPD */ >>> - connector->polled = DRM_CONNECTOR_POLL_CONNECT | >>> - DRM_CONNECTOR_POLL_DISCONNECT; >>> -} >>> - >>> static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { >>> .get_modes = tc_connector_get_modes, >>> }; >> >>> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) >>> +{ >>> + struct tc_data *tc = connector_to_tc(connector); >>> + u32 val; >>> + int ret; >>> + bool conn; >>> + >>> + tc_read(GPIOI, &val); >>> + >>> + conn = val & BIT(0); > > Another thing I noticed when trying this patch is that > tc_connector_detect() will get called via drm_helper_probe_detect() > even if tc->panel is not NULL and tc->connector.polled is zero, which > creates a problem for eDP use-case. Ok, thanks for testing. So we need to return "connected" from the detect function for eDP cases. I think I need to create a fake setup where I can also run with an eDP panel to test this series properly. Probably I can create a "panel" driver for my DP monitor, so I would still get a picture. Tomi
On 20/03/2019 08:57, Tomi Valkeinen wrote: > On 19/03/2019 20:18, Andrey Smirnov wrote: > >> TC358767 has two GPIO pins that can be used for HPD signal. I think >> instead of hardcoding GPIO0 here it would be more flexible to expose >> boths gpios as a gpiochip and use gpiolib API to query the value of >> HPD as well as use "hpd-gpios" binidng in DT to select which input to >> use. >> >> Another argument in favour of this solution is that Toshiba's FAEs (at >> least some) recommend thier customers to connect HPD signal to SoC's >> GPIOs and bypass TC358767 entirely. Their reasoning being that >> TC358767 implements a generic GPIO contoller and there's no advantage >> in going through TC358767 if you could use your "normal" GPIOs. >> >> Using gpiolib API would allow us to handle both usecase with the same >> code. >> >> Lastly, by treating "hpd-gpios" as an optional property, we can >> preserve old driver behaviour regardless if it is connected to DP or >> eDP panel. Not saying that this is really worth doing, just pointing >> out that this option would be on the table as well. > > I think that's a good point. > > There's one thing that TC358767 offers, which may not be available on > most generic GPIO controllers, though: it can detect short (programmable > length) pulses, thus it's possible to easily implement the DisplayPort > IRQ mechanism. I'm not sure if it's possible to implement reliable DP > IRQ detection without HW support. > > Still, I think using standard gpios makes sense. After implementing the gpiochip (it works), I started to wonder... If TC358767 is used as a gpio expander, for whatever purpose, outside the TC358767 driver, then obviously we need the gpiochip driver. But I don't think anyone needs that. Then we have two cases 1) HPD connected to TC358767, 2) HPD goes directly to the SoC, or worded differently, HPD is handled by something else than TC358767. 1) was implemented in this current patch, and there's no real benefit with the gpiochip. It's somewhat confusing that the driver provides a gpiochip which the same driver then uses, for its internal functionality. 2) should actually not involve TC358767 driver at all as it's totally outside TC358767. If the HPD goes from the DP connector to the SoC, we should have the DP connector driver handle it. Currently that connector is in the TC358767 driver, but it should really be separated. So... Obviously what's missing from the current patch is that we need to be able to say which of the two GPIOs are used for the HPD (if any). But I'm debating with myself whether gpiochip here is a sane choice or not. Tomi
On Wed, Mar 20, 2019 at 6:03 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > On 20/03/2019 08:57, Tomi Valkeinen wrote: > > On 19/03/2019 20:18, Andrey Smirnov wrote: > > > >> TC358767 has two GPIO pins that can be used for HPD signal. I think > >> instead of hardcoding GPIO0 here it would be more flexible to expose > >> boths gpios as a gpiochip and use gpiolib API to query the value of > >> HPD as well as use "hpd-gpios" binidng in DT to select which input to > >> use. > >> > >> Another argument in favour of this solution is that Toshiba's FAEs (at > >> least some) recommend thier customers to connect HPD signal to SoC's > >> GPIOs and bypass TC358767 entirely. Their reasoning being that > >> TC358767 implements a generic GPIO contoller and there's no advantage > >> in going through TC358767 if you could use your "normal" GPIOs. > >> > >> Using gpiolib API would allow us to handle both usecase with the same > >> code. > >> > >> Lastly, by treating "hpd-gpios" as an optional property, we can > >> preserve old driver behaviour regardless if it is connected to DP or > >> eDP panel. Not saying that this is really worth doing, just pointing > >> out that this option would be on the table as well. > > > > I think that's a good point. > > > > There's one thing that TC358767 offers, which may not be available on > > most generic GPIO controllers, though: it can detect short (programmable > > length) pulses, thus it's possible to easily implement the DisplayPort > > IRQ mechanism. I'm not sure if it's possible to implement reliable DP > > IRQ detection without HW support. > > > > Still, I think using standard gpios makes sense. > > After implementing the gpiochip (it works), I started to wonder... > > If TC358767 is used as a gpio expander, for whatever purpose, outside > the TC358767 driver, then obviously we need the gpiochip driver. But I > don't think anyone needs that. > Regardless of how it's going to be implemented in the end, there'd have to be a way to specify which HPD input is being used. Which means a either a new DT binding or re-using already existing to be agreed on by DT folks. It just seems to me that there exists a much stronger case to solve this using existing "language" of GPIO references as opposed to introducing some vendor specific binding serving just this single purpose. If DT is supposed to be used to describe the HW, then, IMHO, it might be the other way around, TC358767 is also a GPIO expander and has to be modeled/implemented as such, whether anyone would ever use it in such capacity fully isn't that significant. > Then we have two cases 1) HPD connected to TC358767, 2) HPD goes > directly to the SoC, or worded differently, HPD is handled by something > else than TC358767. > > 1) was implemented in this current patch, and there's no real benefit > with the gpiochip. It's somewhat confusing that the driver provides a > gpiochip which the same driver then uses, for its internal functionality. > > 2) should actually not involve TC358767 driver at all as it's totally > outside TC358767. > There's already precedent for such usage in ti-tfp410.c, analogix/ and andanalogix-anx78xx.c, so it's not unheard of. > If the HPD goes from the DP connector to the SoC, we should have the DP > connector driver handle it. Currently that connector is in the TC358767 > driver, but it should really be separated. > Sure, there's definitely more than one way to solve this. > So... Obviously what's missing from the current patch is that we need to > be able to say which of the two GPIOs are used for the HPD (if any). But > I'm debating with myself whether gpiochip here is a sane choice or not. Yeah, maybe it'd be best to submit a patch to DT mailing list and see what they have to say? Thanks, Andrey Smirnov
On 21/03/2019 00:58, Andrey Smirnov wrote: > Regardless of how it's going to be implemented in the end, there'd > have to be a way to specify which HPD input is being used. Which means True. > a either a new DT binding or re-using already existing to be agreed on > by DT folks. It just seems to me that there exists a much stronger > case to solve this using existing "language" of GPIO references as > opposed to introducing some vendor specific binding serving just this > single purpose. If DT is supposed to be used to describe the HW, then, > IMHO, it might be the other way around, TC358767 is also a GPIO > expander and has to be modeled/implemented as such, whether anyone > would ever use it in such capacity fully isn't that significant. Yep. But few points: - TC358767 node will expose gpios and then it uses them itself. It does look slightly silly in the DT data =). It's not often when you create a reference from a node to itself. - This also needs irqchip implemention to support HPD irq. I have never written one, but I presume it's not too complex, but not trivial either. - All this adds quite a big amount of code, compared to only few lines of code if this is done internally. >> Then we have two cases 1) HPD connected to TC358767, 2) HPD goes >> directly to the SoC, or worded differently, HPD is handled by something >> else than TC358767. >> >> 1) was implemented in this current patch, and there's no real benefit >> with the gpiochip. It's somewhat confusing that the driver provides a >> gpiochip which the same driver then uses, for its internal functionality. >> >> 2) should actually not involve TC358767 driver at all as it's totally >> outside TC358767. >> > > There's already precedent for such usage in ti-tfp410.c, analogix/ and > andanalogix-anx78xx.c, so it's not unheard of. Yes, but I believe the direction has been to move away from that. >> If the HPD goes from the DP connector to the SoC, we should have the DP >> connector driver handle it. Currently that connector is in the TC358767 >> driver, but it should really be separated. >> > > Sure, there's definitely more than one way to solve this. > >> So... Obviously what's missing from the current patch is that we need to >> be able to say which of the two GPIOs are used for the HPD (if any). But >> I'm debating with myself whether gpiochip here is a sane choice or not. > > Yeah, maybe it'd be best to submit a patch to DT mailing list and see > what they have to say? Yep. I'll write the irchip support too, out of interest, and see what it looks like. But this has the feel of over-engineering. Tomi
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 8606de29c9b2..2b252f7ac070 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge) struct tc_data *tc = bridge_to_tc(bridge); int ret; + ret = tc_get_display_props(tc); + if (ret < 0) { + dev_err(tc->dev, "failed to read display props: %d\n", ret); + return; + } + ret = tc_main_link_enable(tc); if (ret < 0) { dev_err(tc->dev, "main link enable error: %d\n", ret); @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct drm_connector *connector) return count; } -static void tc_connector_set_polling(struct tc_data *tc, - struct drm_connector *connector) -{ - /* TODO: add support for HPD */ - connector->polled = DRM_CONNECTOR_POLL_CONNECT | - DRM_CONNECTOR_POLL_DISCONNECT; -} - static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { .get_modes = tc_connector_get_modes, }; +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force) +{ + struct tc_data *tc = connector_to_tc(connector); + u32 val; + int ret; + bool conn; + + tc_read(GPIOI, &val); + + conn = val & BIT(0); + + if (force && conn) + tc_get_display_props(tc); + + if (conn) + return connector_status_connected; + else + return connector_status_disconnected; + +err: + return connector_status_unknown; +} + static const struct drm_connector_funcs tc_connector_funcs = { + .detect = tc_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, @@ -1227,7 +1249,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge) struct drm_device *drm = bridge->dev; int ret; - /* Create eDP connector */ + /* Create DP/eDP connector */ drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, tc->panel ? DRM_MODE_CONNECTOR_eDP : @@ -1235,6 +1257,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge) if (ret) return ret; + /* Don't poll if we have eDP panel */ + if (!tc->panel) { + /* TODO: add support for HPD */ + tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + } + if (tc->panel) drm_panel_attach(tc->panel, &tc->connector); @@ -1377,12 +1406,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) if (ret) return ret; - ret = tc_get_display_props(tc); - if (ret) - goto err_unregister_aux; - - tc_connector_set_polling(tc, &tc->connector); - tc->bridge.funcs = &tc_bridge_funcs; tc->bridge.of_node = dev->of_node; drm_bridge_add(&tc->bridge); @@ -1390,9 +1413,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) i2c_set_clientdata(client, tc); return 0; -err_unregister_aux: - drm_dp_aux_unregister(&tc->aux); - return ret; } static int tc_remove(struct i2c_client *client)
tc358767 driver doesn't have any HPD handling at the moment, as it was originally developed to support only eDP. This patch implements a naive, polling-based HPD handling, which is used when the driver is in DP mode. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 56 +++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 18 deletions(-)