diff mbox

[v3,09/24] drm/sun4i: Don't skip TCONs if they don't have channel 0

Message ID 20180625120304.7543-10-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show

Commit Message

Jernej Škrabec June 25, 2018, 12:02 p.m. UTC
TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
Because of that, all output endpoints on such TCON node will point to a
encoder which is part of component framework.

Correct current graph traversing algorithm in such way that it doesn't
skip output enpoints with id 0 on TV TCONs.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 52 +++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 17 deletions(-)

Comments

Chen-Yu Tsai June 28, 2018, 1:51 a.m. UTC | #1
On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
> Because of that, all output endpoints on such TCON node will point to a
> encoder which is part of component framework.
>
> Correct current graph traversing algorithm in such way that it doesn't
> skip output enpoints with id 0 on TV TCONs.

No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
have channel 0, it must be skipped.

ChenYu
Jernej Škrabec June 28, 2018, 4:45 a.m. UTC | #2
Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai napisal(a):
> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net> 
wrote:
> > TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
> > Because of that, all output endpoints on such TCON node will point to a
> > encoder which is part of component framework.
> > 
> > Correct current graph traversing algorithm in such way that it doesn't
> > skip output enpoints with id 0 on TV TCONs.
> 
> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
> have channel 0, it must be skipped.

I'm not sure where this is stated. I read TCON binding again. Can you please 
point me to it?

So on TV TCONs on R40 (without channel 0) TVE would be endpoint 1 and HDMI 
endpoint 2 (or the other way around)?

Best regards,
Jernej
Chen-Yu Tsai June 28, 2018, 6:24 a.m. UTC | #3
On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
<jernej.skrabec@siol.net> wrote:
> Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai napisal(a):
>> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net>
> wrote:
>> > TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
>> > Because of that, all output endpoints on such TCON node will point to a
>> > encoder which is part of component framework.
>> >
>> > Correct current graph traversing algorithm in such way that it doesn't
>> > skip output enpoints with id 0 on TV TCONs.
>>
>> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
>> have channel 0, it must be skipped.
>
> I'm not sure where this is stated. I read TCON binding again. Can you please
> point me to it?

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt#L169

Our TCON driver still expects RGB or LVDS panel / bridges on endpoint 0.
So I guess this was sort of implied historically. It's no longer true.
This is something we should probably fix.

In practice our drivers don't look at it (yet), but rely on the downstream
encoder type to determine which channel to use.

But please add the "allwinner,tcon-channel" property as specified in
the binding.

> So on TV TCONs on R40 (without channel 0) TVE would be endpoint 1 and HDMI
> endpoint 2 (or the other way around)?

Which one goes first doesn't quite matter. IIRC there's also a mux for TVE?

ChenYu
Jernej Škrabec July 1, 2018, 8:27 a.m. UTC | #4
Dne četrtek, 28. junij 2018 ob 08:24:34 CEST je Chen-Yu Tsai napisal(a):
> On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
> 
> <jernej.skrabec@siol.net> wrote:
> > Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai napisal(a):
> >> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net>
> > 
> > wrote:
> >> > TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
> >> > Because of that, all output endpoints on such TCON node will point to a
> >> > encoder which is part of component framework.
> >> > 
> >> > Correct current graph traversing algorithm in such way that it doesn't
> >> > skip output enpoints with id 0 on TV TCONs.
> >> 
> >> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
> >> have channel 0, it must be skipped.
> > 
> > I'm not sure where this is stated. I read TCON binding again. Can you
> > please point me to it?
> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bind
> ings/display/sunxi/sun4i-drm.txt#L169
> 
> Our TCON driver still expects RGB or LVDS panel / bridges on endpoint 0.

Yes, but that can only happen on TCON which has channel 0. TV TCONs (those 
with channel 1 only) can't have panels or bridges connected to them, because 
they are internally always connected to either HDMI or TV encoder or both. 
Actually, R40 is the only SoC, where same TV TCON can be connected to TV 
encoder or HDMI. Others have specialized TV TCONs, which are connect to only 
one encoder.

IMO TV TCONs are really just stripped down LCD TCONs to support one (or max 
two) specific encoder.

> So I guess this was sort of implied historically. It's no longer true.
> This is something we should probably fix.

Fixed in what way? You mean update bindings to mention that TCON output 
endpoint 0 is reserved for panels or bridges?

> 
> In practice our drivers don't look at it (yet), but rely on the downstream
> encoder type to determine which channel to use.
> 
> But please add the "allwinner,tcon-channel" property as specified in
> the binding.

It's my understanding of TCON binding documentation that property 
"allwinner,tcon-channel" is needed only if TCON supports both channels. TV 
TCON clearly supports only channel 1. In that case, there is no doubt to which 
channel output endpoint belongs.

If that's not true, dt bindings documentation should be reworded to contain 
word "needed" or something similar. Currently, no DT for newer SoC contains 
that property (for example, A83T, H3, H5 or even A33). On A33 this is even 
more interesting, since tcon0 has only channel 0 and has DSI output endpoint 
with number 1. According to TCON binding docs, if "allwinner,tcon-channel" is 
not preset, endpoint number represent channel. So, that would mean DSI needs 
channel 1 on tcon which supports clearly only channel 0. So either there TCON 
bindings documentation needs updates or DT for A33 has to be updated.

BTW, "allwinner,tcon-channel" property is not used at all in the code.

> 
> > So on TV TCONs on R40 (without channel 0) TVE would be endpoint 1 and HDMI
> > endpoint 2 (or the other way around)?
> 
> Which one goes first doesn't quite matter. IIRC there's also a mux for TVE?

AFAIK, TV TCON is by default connected to TV encoder unless HDMI mux in TCON 
TOP points to it. I think it's best put TVE with lower number since it's 
default and HDMI with higher number.

Best regards,
Jernej
Chen-Yu Tsai July 1, 2018, 3:11 p.m. UTC | #5
On Sun, Jul 1, 2018 at 4:27 PM, Jernej Škrabec <jernej.skrabec@siol.net> wrote:
> Dne četrtek, 28. junij 2018 ob 08:24:34 CEST je Chen-Yu Tsai napisal(a):
>> On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
>>
>> <jernej.skrabec@siol.net> wrote:
>> > Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai napisal(a):
>> >> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net>
>> >
>> > wrote:
>> >> > TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
>> >> > Because of that, all output endpoints on such TCON node will point to a
>> >> > encoder which is part of component framework.
>> >> >
>> >> > Correct current graph traversing algorithm in such way that it doesn't
>> >> > skip output enpoints with id 0 on TV TCONs.
>> >>
>> >> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
>> >> have channel 0, it must be skipped.
>> >
>> > I'm not sure where this is stated. I read TCON binding again. Can you
>> > please point me to it?
>>
>> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bind
>> ings/display/sunxi/sun4i-drm.txt#L169
>>
>> Our TCON driver still expects RGB or LVDS panel / bridges on endpoint 0.
>
> Yes, but that can only happen on TCON which has channel 0. TV TCONs (those
> with channel 1 only) can't have panels or bridges connected to them, because
> they are internally always connected to either HDMI or TV encoder or both.
> Actually, R40 is the only SoC, where same TV TCON can be connected to TV
> encoder or HDMI. Others have specialized TV TCONs, which are connect to only
> one encoder.
>
> IMO TV TCONs are really just stripped down LCD TCONs to support one (or max
> two) specific encoder.

I agree. We've seen these first in the H3, and the reverse, TCONs only with
LCD, on the A23/A33.

>> So I guess this was sort of implied historically. It's no longer true.
>> This is something we should probably fix.
>
> Fixed in what way? You mean update bindings to mention that TCON output
> endpoint 0 is reserved for panels or bridges?

Either that, or have the drm driver look at other endpoints. I guess we
should ask Maxime if this is already done or not, since the DSI driver
isn't endpoint 0 in the A33 dtsi.

>>
>> In practice our drivers don't look at it (yet), but rely on the downstream
>> encoder type to determine which channel to use.
>>
>> But please add the "allwinner,tcon-channel" property as specified in
>> the binding.
>
> It's my understanding of TCON binding documentation that property
> "allwinner,tcon-channel" is needed only if TCON supports both channels. TV
> TCON clearly supports only channel 1. In that case, there is no doubt to which
> channel output endpoint belongs.
>
> If that's not true, dt bindings documentation should be reworded to contain
> word "needed" or something similar. Currently, no DT for newer SoC contains
> that property (for example, A83T, H3, H5 or even A33). On A33 this is even
> more interesting, since tcon0 has only channel 0 and has DSI output endpoint
> with number 1. According to TCON binding docs, if "allwinner,tcon-channel" is
> not preset, endpoint number represent channel. So, that would mean DSI needs
> channel 1 on tcon which supports clearly only channel 0. So either there TCON
> bindings documentation needs updates or DT for A33 has to be updated.

Maxime? You did the A33 DSI stuff.

> BTW, "allwinner,tcon-channel" property is not used at all in the code.

Yes I'm aware. We just base the channel selection on the encoder type instead.
TV-like ones use channel 1, LCD ones use channel 0.

>>
>> > So on TV TCONs on R40 (without channel 0) TVE would be endpoint 1 and HDMI
>> > endpoint 2 (or the other way around)?
>>
>> Which one goes first doesn't quite matter. IIRC there's also a mux for TVE?
>
> AFAIK, TV TCON is by default connected to TV encoder unless HDMI mux in TCON
> TOP points to it. I think it's best put TVE with lower number since it's
> default and HDMI with higher number.

Ok. As long as its specified in the binding, as a contract between the dts
and the graph parsing code.

Regards
ChenYu
Maxime Ripard July 5, 2018, 7:03 a.m. UTC | #6
On Sun, Jul 01, 2018 at 11:11:06PM +0800, Chen-Yu Tsai wrote:
> On Sun, Jul 1, 2018 at 4:27 PM, Jernej Škrabec <jernej.skrabec@siol.net> wrote:
> > Dne četrtek, 28. junij 2018 ob 08:24:34 CEST je Chen-Yu Tsai napisal(a):
> >> On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
> >>
> >> <jernej.skrabec@siol.net> wrote:
> >> > Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai napisal(a):
> >> >> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec <jernej.skrabec@siol.net>
> >> >
> >> > wrote:
> >> >> > TV TCONs (channel 1 only) are always connected to TV or HDMI encoder.
> >> >> > Because of that, all output endpoints on such TCON node will point to a
> >> >> > encoder which is part of component framework.
> >> >> >
> >> >> > Correct current graph traversing algorithm in such way that it doesn't
> >> >> > skip output enpoints with id 0 on TV TCONs.
> >> >>
> >> >> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that don't
> >> >> have channel 0, it must be skipped.
> >> >
> >> > I'm not sure where this is stated. I read TCON binding again. Can you
> >> > please point me to it?
> >>
> >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bind
> >> ings/display/sunxi/sun4i-drm.txt#L169
> >>
> >> Our TCON driver still expects RGB or LVDS panel / bridges on endpoint 0.
> >
> > Yes, but that can only happen on TCON which has channel 0. TV TCONs (those
> > with channel 1 only) can't have panels or bridges connected to them, because
> > they are internally always connected to either HDMI or TV encoder or both.
> > Actually, R40 is the only SoC, where same TV TCON can be connected to TV
> > encoder or HDMI. Others have specialized TV TCONs, which are connect to only
> > one encoder.
> >
> > IMO TV TCONs are really just stripped down LCD TCONs to support one (or max
> > two) specific encoder.
> 
> I agree. We've seen these first in the H3, and the reverse, TCONs only with
> LCD, on the A23/A33.
> 
> >> So I guess this was sort of implied historically. It's no longer true.
> >> This is something we should probably fix.
> >
> > Fixed in what way? You mean update bindings to mention that TCON output
> > endpoint 0 is reserved for panels or bridges?
> 
> Either that, or have the drm driver look at other endpoints. I guess we
> should ask Maxime if this is already done or not, since the DSI driver
> isn't endpoint 0 in the A33 dtsi.

The DSI driver isn't really a good example for this, since the panel
isn't described as part of the OF graph, but DSI binding require that
it's a child of the DSI controller node.

> >>
> >> In practice our drivers don't look at it (yet), but rely on the downstream
> >> encoder type to determine which channel to use.
> >>
> >> But please add the "allwinner,tcon-channel" property as specified in
> >> the binding.
> >
> > It's my understanding of TCON binding documentation that property
> > "allwinner,tcon-channel" is needed only if TCON supports both channels. TV
> > TCON clearly supports only channel 1. In that case, there is no doubt to which
> > channel output endpoint belongs.
> >
> > If that's not true, dt bindings documentation should be reworded to contain
> > word "needed" or something similar. Currently, no DT for newer SoC contains
> > that property (for example, A83T, H3, H5 or even A33).

Yeah, but that's mainly because we have a single output enabled for
each channel on those newer SoCs. When / if we enable the RGB and LVDS
output for example, we will have to set this.

> > On A33 this is even more interesting, since tcon0 has only channel
> > 0 and has DSI output endpoint with number 1. According to TCON
> > binding docs, if "allwinner,tcon-channel" is not preset, endpoint
> > number represent channel. So, that would mean DSI needs channel 1
> > on tcon which supports clearly only channel 0. So either there
> > TCON bindings documentation needs updates or DT for A33 has to be
> > updated.
> 
> Maxime? You did the A33 DSI stuff.

I guess it's missing on the A33 DSI endpoint.

Maxime
Jernej Škrabec July 5, 2018, 8:03 p.m. UTC | #7
Dne četrtek, 05. julij 2018 ob 09:03:58 CEST je Maxime Ripard napisal(a):
> On Sun, Jul 01, 2018 at 11:11:06PM +0800, Chen-Yu Tsai wrote:
> > On Sun, Jul 1, 2018 at 4:27 PM, Jernej Škrabec <jernej.skrabec@siol.net> 
wrote:
> > > Dne četrtek, 28. junij 2018 ob 08:24:34 CEST je Chen-Yu Tsai napisal(a):
> > >> On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
> > >> 
> > >> <jernej.skrabec@siol.net> wrote:
> > >> > Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai 
napisal(a):
> > >> >> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec
> > >> >> <jernej.skrabec@siol.net>
> > >> > 
> > >> > wrote:
> > >> >> > TV TCONs (channel 1 only) are always connected to TV or HDMI
> > >> >> > encoder.
> > >> >> > Because of that, all output endpoints on such TCON node will point
> > >> >> > to a
> > >> >> > encoder which is part of component framework.
> > >> >> > 
> > >> >> > Correct current graph traversing algorithm in such way that it
> > >> >> > doesn't
> > >> >> > skip output enpoints with id 0 on TV TCONs.
> > >> >> 
> > >> >> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that
> > >> >> don't
> > >> >> have channel 0, it must be skipped.
> > >> > 
> > >> > I'm not sure where this is stated. I read TCON binding again. Can you
> > >> > please point me to it?
> > >> 
> > >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree
> > >> /bind ings/display/sunxi/sun4i-drm.txt#L169
> > >> 
> > >> Our TCON driver still expects RGB or LVDS panel / bridges on endpoint
> > >> 0.
> > > 
> > > Yes, but that can only happen on TCON which has channel 0. TV TCONs
> > > (those
> > > with channel 1 only) can't have panels or bridges connected to them,
> > > because they are internally always connected to either HDMI or TV
> > > encoder or both. Actually, R40 is the only SoC, where same TV TCON can
> > > be connected to TV encoder or HDMI. Others have specialized TV TCONs,
> > > which are connect to only one encoder.
> > > 
> > > IMO TV TCONs are really just stripped down LCD TCONs to support one (or
> > > max
> > > two) specific encoder.
> > 
> > I agree. We've seen these first in the H3, and the reverse, TCONs only
> > with
> > LCD, on the A23/A33.
> > 
> > >> So I guess this was sort of implied historically. It's no longer true.
> > >> This is something we should probably fix.
> > > 
> > > Fixed in what way? You mean update bindings to mention that TCON output
> > > endpoint 0 is reserved for panels or bridges?
> > 
> > Either that, or have the drm driver look at other endpoints. I guess we
> > should ask Maxime if this is already done or not, since the DSI driver
> > isn't endpoint 0 in the A33 dtsi.
> 
> The DSI driver isn't really a good example for this, since the panel
> isn't described as part of the OF graph, but DSI binding require that
> it's a child of the DSI controller node.

So should be new behaviour reverted? I mean ignoring endpoint 0 on TV TCONs.

For me, it doesn't make sense to check for panel or bridges on TV TCONs, 
because they are never exposed on physical pins. They are always connected to 
TVE or HDMI encoder internally.

> 
> > >> In practice our drivers don't look at it (yet), but rely on the
> > >> downstream
> > >> encoder type to determine which channel to use.
> > >> 
> > >> But please add the "allwinner,tcon-channel" property as specified in
> > >> the binding.
> > > 
> > > It's my understanding of TCON binding documentation that property
> > > "allwinner,tcon-channel" is needed only if TCON supports both channels.
> > > TV
> > > TCON clearly supports only channel 1. In that case, there is no doubt to
> > > which channel output endpoint belongs.
> > > 
> > > If that's not true, dt bindings documentation should be reworded to
> > > contain
> > > word "needed" or something similar. Currently, no DT for newer SoC
> > > contains
> > > that property (for example, A83T, H3, H5 or even A33).
> 
> Yeah, but that's mainly because we have a single output enabled for
> each channel on those newer SoCs. When / if we enable the RGB and LVDS
> output for example, we will have to set this.

So just to be clear, before I send follow up series, I have to add 
"allwinner,tcon-channel" property to DT, because both TV TCONs can be 
connected to either TVE or HDMI (trough TCON TOP)?

BTW, since this property is not used at all in the code, why not just simply 
deprecate it and remove it from existing DTs? I noticed this is standard 
practice for obsolete properties.

Best regards,
Jernej

> 
> > > On A33 this is even more interesting, since tcon0 has only channel
> > > 0 and has DSI output endpoint with number 1. According to TCON
> > > binding docs, if "allwinner,tcon-channel" is not preset, endpoint
> > > number represent channel. So, that would mean DSI needs channel 1
> > > on tcon which supports clearly only channel 0. So either there
> > > TCON bindings documentation needs updates or DT for A33 has to be
> > > updated.
> > 
> > Maxime? You did the A33 DSI stuff.
> 
> I guess it's missing on the A33 DSI endpoint.
> 
> Maxime
Maxime Ripard July 9, 2018, 8:59 a.m. UTC | #8
On Thu, Jul 05, 2018 at 10:03:35PM +0200, Jernej Škrabec wrote:
> Dne četrtek, 05. julij 2018 ob 09:03:58 CEST je Maxime Ripard napisal(a):
> > On Sun, Jul 01, 2018 at 11:11:06PM +0800, Chen-Yu Tsai wrote:
> > > On Sun, Jul 1, 2018 at 4:27 PM, Jernej Škrabec <jernej.skrabec@siol.net> 
> wrote:
> > > > Dne četrtek, 28. junij 2018 ob 08:24:34 CEST je Chen-Yu Tsai napisal(a):
> > > >> On Thu, Jun 28, 2018 at 12:45 PM, Jernej Škrabec
> > > >> 
> > > >> <jernej.skrabec@siol.net> wrote:
> > > >> > Dne četrtek, 28. junij 2018 ob 03:51:31 CEST je Chen-Yu Tsai 
> napisal(a):
> > > >> >> On Mon, Jun 25, 2018 at 8:02 PM, Jernej Skrabec
> > > >> >> <jernej.skrabec@siol.net>
> > > >> > 
> > > >> > wrote:
> > > >> >> > TV TCONs (channel 1 only) are always connected to TV or HDMI
> > > >> >> > encoder.
> > > >> >> > Because of that, all output endpoints on such TCON node will point
> > > >> >> > to a
> > > >> >> > encoder which is part of component framework.
> > > >> >> > 
> > > >> >> > Correct current graph traversing algorithm in such way that it
> > > >> >> > doesn't
> > > >> >> > skip output enpoints with id 0 on TV TCONs.
> > > >> >> 
> > > >> >> No. Our bindings say that endpoint 0 _is_ channel 0. For TCONs that
> > > >> >> don't
> > > >> >> have channel 0, it must be skipped.
> > > >> > 
> > > >> > I'm not sure where this is stated. I read TCON binding again. Can you
> > > >> > please point me to it?
> > > >> 
> > > >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree
> > > >> /bind ings/display/sunxi/sun4i-drm.txt#L169
> > > >> 
> > > >> Our TCON driver still expects RGB or LVDS panel / bridges on endpoint
> > > >> 0.
> > > > 
> > > > Yes, but that can only happen on TCON which has channel 0. TV TCONs
> > > > (those
> > > > with channel 1 only) can't have panels or bridges connected to them,
> > > > because they are internally always connected to either HDMI or TV
> > > > encoder or both. Actually, R40 is the only SoC, where same TV TCON can
> > > > be connected to TV encoder or HDMI. Others have specialized TV TCONs,
> > > > which are connect to only one encoder.
> > > > 
> > > > IMO TV TCONs are really just stripped down LCD TCONs to support one (or
> > > > max
> > > > two) specific encoder.
> > > 
> > > I agree. We've seen these first in the H3, and the reverse, TCONs only
> > > with
> > > LCD, on the A23/A33.
> > > 
> > > >> So I guess this was sort of implied historically. It's no longer true.
> > > >> This is something we should probably fix.
> > > > 
> > > > Fixed in what way? You mean update bindings to mention that TCON output
> > > > endpoint 0 is reserved for panels or bridges?
> > > 
> > > Either that, or have the drm driver look at other endpoints. I guess we
> > > should ask Maxime if this is already done or not, since the DSI driver
> > > isn't endpoint 0 in the A33 dtsi.
> > 
> > The DSI driver isn't really a good example for this, since the panel
> > isn't described as part of the OF graph, but DSI binding require that
> > it's a child of the DSI controller node.
> 
> So should be new behaviour reverted? I mean ignoring endpoint 0 on TV TCONs.
> 
> For me, it doesn't make sense to check for panel or bridges on TV TCONs, 
> because they are never exposed on physical pins. They are always connected to 
> TVE or HDMI encoder internally.

It doesn't make sense to check for panels, yes, but it also makes sens
to have a child node at the endpoint 0 for TV TCONs.

> > > >> In practice our drivers don't look at it (yet), but rely on the
> > > >> downstream
> > > >> encoder type to determine which channel to use.
> > > >> 
> > > >> But please add the "allwinner,tcon-channel" property as specified in
> > > >> the binding.
> > > > 
> > > > It's my understanding of TCON binding documentation that property
> > > > "allwinner,tcon-channel" is needed only if TCON supports both channels.
> > > > TV
> > > > TCON clearly supports only channel 1. In that case, there is no doubt to
> > > > which channel output endpoint belongs.
> > > > 
> > > > If that's not true, dt bindings documentation should be reworded to
> > > > contain
> > > > word "needed" or something similar. Currently, no DT for newer SoC
> > > > contains
> > > > that property (for example, A83T, H3, H5 or even A33).
> > 
> > Yeah, but that's mainly because we have a single output enabled for
> > each channel on those newer SoCs. When / if we enable the RGB and LVDS
> > output for example, we will have to set this.
> 
> So just to be clear, before I send follow up series, I have to add 
> "allwinner,tcon-channel" property to DT, because both TV TCONs can be 
> connected to either TVE or HDMI (trough TCON TOP)?
> 
> BTW, since this property is not used at all in the code, why not just simply 
> deprecate it and remove it from existing DTs? I noticed this is standard 
> practice for obsolete properties.

It's not deprecated or obsolete, it's future-proofing.

Maxime
diff mbox

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index e6c62c079146..6ddf4eaccb40 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -198,6 +198,22 @@  static bool sun4i_drv_node_is_tcon(struct device_node *node)
 	return !!of_match_node(sun4i_tcon_of_table, node);
 }
 
+static bool sun4i_drv_node_is_tcon_with_ch0(struct device_node *node)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(sun4i_tcon_of_table, node);
+	if (match) {
+		struct sun4i_tcon_quirks *quirks;
+
+		quirks = (struct sun4i_tcon_quirks *)match->data;
+
+		return quirks->has_channel_0;
+	}
+
+	return false;
+}
+
 static bool sun4i_drv_node_is_tcon_top(struct device_node *node)
 {
 	return !!of_match_node(sun8i_tcon_top_of_table, node);
@@ -256,14 +272,7 @@  static void sun4i_drv_traverse_endpoints(struct endpoint_list *list,
 			continue;
 		}
 
-		/*
-		 * If the node is our TCON, the first port is used for
-		 * panel or bridges, and will not be part of the
-		 * component framework.
-		 */
 		if (sun4i_drv_node_is_tcon(node)) {
-			struct of_endpoint endpoint;
-
 			/*
 			 * TCON TOP is always probed before TCON. However, TCON
 			 * points back to TCON TOP when it is source for HDMI.
@@ -276,16 +285,25 @@  static void sun4i_drv_traverse_endpoints(struct endpoint_list *list,
 				continue;
 			}
 
-			if (of_graph_parse_endpoint(ep, &endpoint)) {
-				DRM_DEBUG_DRIVER("Couldn't parse endpoint\n");
-				of_node_put(remote);
-				continue;
-			}
-
-			if (!endpoint.id) {
-				DRM_DEBUG_DRIVER("Endpoint is our panel... skipping\n");
-				of_node_put(remote);
-				continue;
+			/*
+			 * If the node is our TCON with channel 0, the first
+			 * port is used for panel or bridges, and will not be
+			 * part of the component framework.
+			 */
+			if (sun4i_drv_node_is_tcon_with_ch0(node)) {
+				struct of_endpoint endpoint;
+
+				if (of_graph_parse_endpoint(ep, &endpoint)) {
+					DRM_DEBUG_DRIVER("Couldn't parse endpoint\n");
+					of_node_put(remote);
+					continue;
+				}
+
+				if (!endpoint.id) {
+					DRM_DEBUG_DRIVER("Endpoint is our panel... skipping\n");
+					of_node_put(remote);
+					continue;
+				}
 			}
 		}