Re: [PATCH v3 1/3] dt-bindings: media: atmel: csi2dc: add bindings for microchip csi2dc

[Jacopo Mondi]

Hello Eugen,

On Fri, Oct 16, 2020 at 12:30:59PM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> On 12.10.2020 16:04, Jacopo Mondi wrote:
> > Hello,
> >     just my 2 cents, as I've noticed this patch skimming through the
> >     list
> >
> > On Mon, Oct 12, 2020 at 07:19:43AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:

[snip]

>
> Hi,
>
> Thanks for helping,
> >
> > Is this property describing the CSI-2 clock continuous, non-continuous
> > mode configuration, or did I mis-interpreted it ?
>
> I think so. This is a setting inside the csi2dc regarding clock. If we
> can obtain it from pads operations, then it's good, but the question is,
> if the devices can provide this or not ?

The transmitter can provide this information, as it knows which clock
mode it is going to use (it's not clear from the CSI-2 spec which side
is in charge of the selection, but it seems natural to me that it's up
to the transmitter side). Whether they implement the right operation
to do so, well... see below

> >
> > We added support for retrieving run-time configuration of the media
> > bus with the get_mbus_config pad operations recently. Among the
> > configuration flags for MBUS_CSI2_DPHY there are inded CONTINUOUS and
> > NON_CONTINUOUS clock flags.
> >
> >>>
> >>>> +
> >>>> +  microchip,inter-line-delay:
> >>>> +    allOf:
> >>>> +    - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    - minimum: 1
> >>>> +    - maximum: 16
> >>>> +    default: 16
> >>>> +    description:
> >>>> +      Indicates how many clock cycles should be introduced between each line.
> >>>
> >>> This also sounds like a configuration parameter. How does one compute
> >>> the right value for this ?
> >>
> >> I think this is a delay that can be added inside the hardware block,
> >> depending on the interface speed and bandwidth. I will try to understand
> >> more details from the hardware design and come back with a more detailed
> >> answer.
> >>
>
> Regarding this, I will remove it. Our design team advised to have a
> hardcoded value for this product.
>
> >>>
> >>>> +
> >>>> +
> >>>> +        properties:
> >>>> +          reg:
> >>>> +            enum: [0, 1, 2, 3]
> >>>> +            description: virtual channel for the endpoint
> >>>
> >>> The virtual channel used by the source is also something that needs to
> >>> be queried from the source at runtime, it doesn't belong to this
> >>> binding.
> >>
> >> The same question as for the gated clock configuration. How can we use
> >> v4l2 subdevice API to obtain such information from the subdevice? And if
> >> the subdevice does not offer such information ?
> >
> > I think the subdev driver should be instrumented to report it instead of
> > hard-coding the information in DT which should be otherwise updated
> > depending on which sensor is connected to the board. Does it make
> > sense to you ?
>
> It does, but then, it won't work unless connected to instrumented
> subdevices. Which is not really something I would do, since it would
> completely limit the usability.
> Do you have any example on how to get the virtual id from the subdev ?

As examples of CSI-2 transmitters implementing get_mbus_config() we
have tc358743 and adv748x reporting the number of active data lanes.

Reporting the virtual channel in use is a matter of using one of the
following flags:

/* CSI-2 Virtual Channel identifiers. */
#define V4L2_MBUS_CSI2_CHANNEL_0		BIT(4)
#define V4L2_MBUS_CSI2_CHANNEL_1		BIT(5)
#define V4L2_MBUS_CSI2_CHANNEL_2		BIT(6)
#define V4L2_MBUS_CSI2_CHANNEL_3		BIT(7)

As an example of a receiver driver rcar-csi2 uses the operation to
dynamically negotiate the number of data lanes. To handle virtual
channel you would need to inspect which of the above flags have been
set by the subdevice.

On deciding if this better handled by using a subdev operation or a DT
property, I understand the subdev driver needs to implement
get_mbus_config, but hardcoding it in DT I fear is a no-go for
mainline and honestly it doesn't sound much more advantageous, as it
needs to be adapted according to the connected sensor anyway, doesn't
it ? But I'm happy to defer this call to the maintainers, I hope I
just have provided some useful references.

Cheers
   j

>
> Thanks again,
>
> Eugen
> >
> > Cheers
> >     j
> >
> >>
> >> Thanks again,
> >>
> >> Eugen
> >>>
> >>>> +
> >>>> +          remote-endpoint: true
> >>>> +
> >>>> +        required:
> >>>> +          - remote-endpoint
> >>>> +          - reg
> >>>> +
> >>>> +        additionalProperties: false
> >>>> +
> >>>> +    additionalProperties: false
> >>>> +
> >>>> +required:
> >>>> +  - compatible
> >>>> +  - reg
> >>>> +  - clocks
> >>>> +  - clock-names
> >>>> +  - port@0
> >>>> +
> >>>> +examples:
> >>>> +  - |
> >>>> +    csi2dc@e1404000 {
> >>>> +        compatible = "microchip,sama7g5-csi2dc";
> >>>> +        #address-cells = <1>;
> >>>> +        #size-cells = <0>;
> >>>> +        reg = <0xe1404000 0x500>;
> >>>> +        clocks = <&pclk>, <&scck>;
> >>>> +        clock-names = "pclk", "scck";
> >>>> +
> >>>> +        port@0 {
> >>>> +               reg = <0>; /* must be 0, first child port */
> >>>> +               csi2dc_in: endpoint { /* input from IDI interface */
> >>>> +                     remote-endpoint = <&csi2host_out>;
> >>>> +               };
> >>>> +        };
> >>>> +
> >>>> +        port@1 {
> >>>> +                #address-cells = <1>;
> >>>> +                #size-cells = <0>;
> >>>> +                reg = <1>; /* must be 1, second child port */
> >>>> +                csi2dc_out: endpoint@2 {
> >>>> +                        reg = <2>;  /* virtual channel identifier */
> >>>> +                        remote-endpoint = <&xisc_in>; /* output to sensor controller */
> >>>> +                };
> >>>> +        };
> >>>> +    };
> >>>> +
> >>>> +...
> >>>
> >>> --
> >>> Regards,
> >>>
> >>> Laurent Pinchart
> >>>
> >>
>