Date: Wed, 28 Feb 2024 12:09:50 -0600
From: Rob Herring <robh@...nel.org>
To: Paweł Anikiel <panikiel@...gle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, airlied@...il.com,
	akpm@...ux-foundation.org, conor+dt@...nel.org, daniel@...ll.ch,
	dinguyen@...nel.org, hverkuil-cisco@...all.nl,
	krzysztof.kozlowski+dt@...aro.org,
	maarten.lankhorst@...ux.intel.com, mchehab@...nel.org,
	mripard@...nel.org, tzimmermann@...e.de, devicetree@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org, chromeos-krk-upstreaming@...gle.com,
	ribalda@...omium.org
Subject: Re: [PATCH v2 8/9] media: dt-bindings: Add Intel Displayport RX IP

On Wed, Feb 28, 2024 at 02:09:33PM +0100, Paweł Anikiel wrote:
> On Wed, Feb 28, 2024 at 1:18 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@...aro.org> wrote:
> >
> > On 28/02/2024 12:05, Paweł Anikiel wrote:
> > > On Tue, Feb 27, 2024 at 3:29 PM Rob Herring <robh@...nel.org> wrote:
> > >>
> > >> On Mon, Feb 26, 2024 at 11:59:42AM +0100, Paweł Anikiel wrote:
> > >>> On Mon, Feb 26, 2024 at 10:13 AM Krzysztof Kozlowski
> > >>> <krzysztof.kozlowski@...aro.org> wrote:
> > >>>>
> > >>>> On 21/02/2024 17:02, Paweł Anikiel wrote:
> > >>>>> The Intel Displayport RX IP is a part of the DisplayPort Intel FPGA IP
> > >>>>> Core. It implements a DisplayPort 1.4 receiver capable of HBR3 video
> > >>>>> capture and Multi-Stream Transport. The user guide can be found here:
> > >>>>>
> > >>>>> https://www.intel.com/programmable/technical-pdfs/683273.pdf
> > >>>>>
> > >>>>> Signed-off-by: Paweł Anikiel <panikiel@...gle.com>
> > >>>>> ---
> > >>>>>  .../devicetree/bindings/media/intel,dprx.yaml | 160 ++++++++++++++++++
> > >>>>>  1 file changed, 160 insertions(+)
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/media/intel,dprx.yaml
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/media/intel,dprx.yaml b/Documentation/devicetree/bindings/media/intel,dprx.yaml
> > >>>>> new file mode 100644
> > >>>>> index 000000000000..31025f2d5dcd
> > >>>>> --- /dev/null
> > >>>>> +++ b/Documentation/devicetree/bindings/media/intel,dprx.yaml
> > >>>>> @@ -0,0 +1,160 @@
> > >>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > >>>>> +%YAML 1.2
> > >>>>> +---
> > >>>>> +$id: http://devicetree.org/schemas/media/intel,dprx.yaml#
> > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>>> +
> > >>>>> +title: Intel DisplayPort RX IP
> > >>>>> +
> > >>>>> +maintainers:
> > >>>>> +  - Paweł Anikiel <panikiel@...gle.com>
> > >>>>> +
> > >>>>> +description: |
> > >>>>> +  The Intel Displayport RX IP is a part of the DisplayPort Intel FPGA IP
> > >>>>> +  Core. It implements a DisplayPort 1.4 receiver capable of HBR3 video
> > >>>>> +  capture and Multi-Stream Transport.
> > >>>>> +
> > >>>>> +  The IP features a large number of configuration parameters, found at:
> > >>>>> +  https://www.intel.com/content/www/us/en/docs/programmable/683273/23-3-20-0-1/sink-parameters.html
> > >>>>> +
> > >>>>> +  The following parameters have to be enabled:
> > >>>>> +    - Support DisplayPort sink
> > >>>>> +    - Enable GPU control
> > >>>>> +  The following parameters' values have to be set in the devicetree:
> > >>>>> +    - RX maximum link rate
> > >>>>> +    - Maximum lane count
> > >>>>> +    - Support MST
> > >>>>> +    - Max stream count (only if Support MST is enabled)
> > >>>>> +
> > >>>>> +properties:
> > >>>>> +  compatible:
> > >>>>> +    const: intel,dprx-20.0.1
> > >>>>> +
> > >>>>> +  reg:
> > >>>>> +    maxItems: 1
> > >>>>> +
> > >>>>> +  interrupts:
> > >>>>> +    maxItems: 1
> > >>>>> +
> > >>>>> +  intel,max-link-rate:
> > >>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> > >>>>> +    description: Max link rate configuration parameter
> > >>>>
> > >>>> Please do not duplicate property name in description. It's useless.
> > >>>> Instead explain what is this responsible for.
> > >>>>
> > >>>> Why max-link-rate would differ for the same dprx-20.0.1? And why
> > >>>> standard properties cannot be used?
> > >>>>
> > >>>> Same for all questions below.
> > >>>
> > >>> These four properties are the IP configuration parameters mentioned in
> > >>> the device description. When generating the IP core you can set these
> > >>> parameters, which could make them differ for the same dprx-20.0.1.
> > >>> They are documented in the user guide, for which I also put a link in
> > >>> the description. Is that enough? Or should I also document these
> > >>> parameters here?
> > >>
> > >> Use the standard properties: link-frequencies and data-lanes. Those go
> > >> under the port(s) because they are inheritly per logical link.
> > >
> > > The DP receiver has one input interface (a deserialized DP stream),
> > > and up to four output interfaces (the decoded video streams). The "max
> > > link rate" and "max lane count" parameters only describe the input
> > > interface to the receiver. However, the port(s) I am using here are
> > > for the output streams. They are not affected by those parameters, so
> > > I don't think these properties should go under the output port(s).
> > >
> > > The receiver doesn't have an input port in the DT, because there isn't
> > > any controllable entity on the other side - the deserializer doesn't
> > > have any software interface. Since these standard properties
> > > (link-frequencies and data-lanes) are only defined in
> > > video-interfaces.yaml (which IIUC describes a graph endpoint), I can't
> > > use them directly in the device node.
> >
> > DT describes the hardware, so where does the input come? From something,
> > right? Regardless if you have a driver or not. There is dp-connector
> > binding, if this is physical port.
> 
> Yes, it is a physical port. I agree adding a DT node for the physical
> DP input connector would let us add link-frequencies to the input port
> of the receiver.
> 
> However, dp-connector seems to be a binding for an output port - it's
> under schemas/display/connector, and DP_PWR can be a power supply only
> for an output port (looking at the dp-pwr-supply property). Also, the
> driver for this binding is a DRM bridge driver (display-connector.c)
> which would not be compatible with a v4l2 (sub)device.

So then we should add 'dp-input-connector' because they are different. 
When we haven't defined connectors, properties of the connector have 
been shoved in whatever node is associated with a connector like you 
have done. That works for a while, but then becomes unmanageable. DP on 
USB-C connectors for example.

OTOH, maybe your use here is niche enough to not be worth the trouble. 
Depends if we see the need for video input connectors in general.

Rob

Powered by blists - more mailing lists