Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Cyprian,

On Tuesday, 22 August 2017 22:25:49 EEST Cyprian Wronka wrote:
> On 22/08/2017, 02:01, Laurent Pinchart wrote:
>> On Tuesday, 22 August 2017 11:53:20 EEST Maxime Ripard wrote:
>>> On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
>>>> On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
>>>> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that
>>>>> supports up to 4 CSI-2 lanes, and can route the frames to up to 4
>>>>> streams, depending on the hardware implementation.
>>>>> 
>>>>> It can operate with an external D-PHY, an internal one or no D-PHY
>>>>> at all in some configurations.
>>>> 
>>>> Without any PHY ? I'm curious, how does that work ?
>>>
>>> We're currently working on an FPGA exactly with that configuration.
>>> So I guess the answer would be "it doesn't on an ASIC" :)
>>>
>> What's connected to the input of the CSI-2 receiver ?
>> 
> It is connected to an instance of a CSI TX digital interface.

That makes sense. I suppose it's a test setup

>>>>> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>>>> ---
>>>>> 
>>>>> 
>>>>>  .../devicetree/bindings/media/cdns-csi2rx.txt> | 87
>>>>>  ++++++++++++++++
>>>>>  1 file changed, 87 insertions(+)
>>>>>  create mode 100644
>>>>>  Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>> 
>>>>> 
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>> b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file
>>>>> mode 100644
>>>>> index 000000000000..e08547abe885
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>> @@ -0,0 +1,87 @@
>>>>> +Cadence MIPI-CSI2 RX controller
>>>>> +===============================
>>>>> +
>>>>> +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting
>>>>> up to 4 CSI
>>>>> +lanes in input, and 4 different pixel streams in output.
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: must be set to "cdns,csi2rx" and an SoC-specific
>>>>> compatible
>>>>> +  - reg: base address and size of the memory mapped region
>>>>> +  - clocks: phandles to the clocks driving the controller
>>>>> +  - clock-names: must contain:
>>>>> +    * sys_clk: main clock
>>>>> +    * p_clk: register bank clock
>>>>> +    * p_free_clk: free running register bank clock
>>>>> +    * pixel_ifX_clk: pixel stream output clock, one for each
>>>>> stream
>>>>> +      implemented in hardware, between 0 and 3
>>>> 
>>>> Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me
>>>> a few seconds to see that the X was a placeholder.
>>> 
>>> Ok.
>>> 
>>>>> +    * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
>>>>> +  - phys: phandle to the external D-PHY
>>>>> +  - phy-names: must contain dphy, if the implementation uses an
>>>>> +     external D-PHY
>>>> 
>>>> I would move the last two properties in an optional category as
>>>> they're effectively optional. I think you should also explain a bit more
>>>> clearly that the phys property must not be present if the phy-names
>>>> property is not present.
>>> 
>>> It's not really optional. The IP has a configuration register that
>>> allows you to see if it's been synthesized with or without a PHY. If
>>> the right bit is set, that property will be mandatory, if not, it's
>>> useless.
>> 
>> Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2
>> receiver input interface different when used with a PHY and when used
>> without one ? Could a third-party PHY be used as well ? If so, would the
>> PHY synthesis bit be set or not ?
> 
> The PHY (in our case a D-PHY) is a separate entity, it can be from a 3rd
> party as the IP interface is standard, the SoC integrator would set the bit
> accordingly based on whether any PHY is present or not. There is also an
> option of routing digital output from a CSI-TX to a CSI-RX and in such case
> a PHY would not need to be used (as in the case of our current platform). 

OK, thank you for the clarification. 

Maxime mentioned that a bit can be read from a register to notify whether a 
PHY has been synthesized or not. Does it influence the CSI-2 RX input 
interface at all, or is the CSI-2 RX IP core synthesized the same way 
regardless of whether a PHY is present or not ?

>>> Maybe it's just semantics, but to me, optional means that it can
>>> operate with or without it under any circumstances. It's not really
>>> the case here.
>> 
>> It's a semantic issue, but documenting a property as required when it can
>> be ommitted under some circumstances seems even weirder to me :-) I
>> understand the optional category as "can be ommitted in certain
>> circumstances". 
>> 
>>>>> +
>>>>> +Required subnodes:
>>>>> +  - ports: A ports node with endpoint definitions as defined in
>>>>> + Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>>>> + first port subnode should be the input endpoint, the
>>>>> second one the
>>>>> + outputs
>>>>> +
>>>>> +  The output port should have as many endpoints as stream
>>>>> supported by
>>>>> +  the hardware implementation, between 1 and 4, their ID being
>>>>> the
>>>>> +  stream output number used in the implementation.
>>>> 
>>>> I don't think that's correct. The IP has four independent outputs,
>>>> it should have 4 output ports for a total for 5 ports. Multiple
>>>> endpoints per port would describe multiple connections from the same
>>>> output to different sinks.
>>>
>>> Ok.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html