Re: [PATCH 2/4] dt-bindings: media: Add bindings for raspberrypi,rp1-cfe

From: Tomi Valkeinen
Date: Tue Mar 19 2024 - 09:57:29 EST


On 19/03/2024 11:31, Krzysztof Kozlowski wrote:
On 19/03/2024 07:46, Tomi Valkeinen wrote:
+ reg:
+ items:
+ - description: CSI-2 registers
+ - description: D-PHY registers
+ - description: MIPI CFG (a simple top-level mux) registers
+ - description: FE registers
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ port:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+ description: CSI-2 RX Port

Only one port, so there is nothing to output to?

The CFE has DMA, so it writes to memory. But no other outputs.

You still might have some sort of pipeline, e.g. some post processing
block. But if this is end block, then I guess it's fine.

There is a processing block, FE. But it's considered part of the same module.

Whether that's exactly correct or not, I'm not sure. I don't have detailed hardware docs, but my understanding of the architecture is that we have the D-PHY, CSI-2 RX and FE blocks, and a "MIPI CFG" wrapper around these (with some CSI-2 & FE muxing and interrupt status flags). These are all considered to be part of the same CFE module, and thus we represent it here as a single node.

In the patch 4 I explain a bit more about the HW blocks, but maybe it would've been beneficial to have some description here too. Here's the relevant part:

+The PiSP Camera Front End (CFE) is a module which combines a CSI-2 receiver with
+a simple ISP, called the Front End (FE).
+
+The CFE has four DMA engines and can write frames from four separate streams
+received from the CSI-2 to the memory. One of those streams can also be routed
+directly to the FE, which can do minimal image processing, write two versions
+(e.g. non-scaled and downscaled versions) of the received frames to memory and
+provide statistics of the received frames.


+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ clock-lanes:
+ maxItems: 1
+
+ clock-noncontinuous: true

Drop

Hmm, I saw this used in multiple other bindings, and thought it means
the property is allowed and copied it here.

If that's not the case, does this mean all the properties from
video-interfaces.yaml are allowed (even invalid ones, like pclk-sample)?

Yes, that's the meaning of unevaluatedProperties you have a bit above.


+
+ required:
+ - clock-lanes
+ - data-lanes
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/rp1.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/mfd/rp1.h>
+
+ rpi1 {

soc

That should actually be "rp1", not "rpi1". rp1 is the co-processor on
which the cfe is located, so it doesn't reside in the soc itself. But

Where is the co-processor located?

RP1 is a separate chip on the board, behind PCIe. It contains multiple blocks, dealing with I/O (like this CSI-2, but also USB, eth, ...). To the driver the CFE just shows up as a normal memory mapped IP. Afaics, in theory CFE could as well be in the main SoC itself, so, for an example, I don't see "soc" as a bad parent node.

Tomi

perhaps that's not relevant, and "soc" is just a generic container that
should always be used?

Does not have to be soc, but rp1 is not generic.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation




Best regards,
Krzysztof