Re: [PATCH v2 2/2] phy: mtk-mipi-csi: add driver for CSI phy

[AngeloGioacchino Del Regno]

Il 16/05/23 11:30, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 04:32:42PM +0200, AngeloGioacchino Del Regno wrote:
> > Il 15/05/23 16:07, Julien Stephan ha scritto:
> > > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> > > > > +#define CSIxB_OFFSET		0x1000
> > > >
> > > > What if we grab two (or three?) iospaces from devicetree?
> > > >
> > > > - base (global)
> > > > - csi_a
> > > > - csi_b
> > > >
> > > > That would make it possible to maybe eventually extend this driver to more
> > > > versions (older or newer) of the CSI PHY IP without putting fixes offsets
> > > > inside of platform data structures and such.
> > > Hi Angelo,
> > > The register bank of the CSI port is divided into 2:
> > > * from base address to base + 0x1000 (port A)
> > > * from base + 0x1000 to base +0x2000 (port B)
> > > Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
> > > the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
> > > 1 clock) and use either port A or port B.
> > >
> > > For example  mt8365 has CSI0 that can be used either in 4D1C mode or in
> > > 2 * 2D1C and CSI1 which can use only 4D1C mode
> > >
> > > 2D1C mode can not be tested and is not implemented in the driver so
> > > I guess adding csi_a and csi_b reg value may be confusing?
> > >
> > > What do you think?
> >
> > Ok so we're talking about two data lanes per CSI port... it may still be
> > beneficial to split the two register regions as
> >
> > reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
> >                                 and that was a mistake, sorry!)
> >
> > ....but that would be actually good only if we are expecting to get a CSI
> > PHY in the future with four data lanes per port.
> >
> > If you do *not* expect at all such a CSI PHY, or you do *not* expect such
> > a PHY to ever be compatible with this driver (read as: if you expect such
> > a PHY to be literally completely different from this one), then it would
> > not change much to have the registers split in two.
> >
> > Another case in which it would make sense is if we were to get a PHY that
> > provides more than two CSI ports: in that case, we'd avoid platform data
> > machinery to check the number of actual ports in the IP, as we would be
> > just checking how many register regions we were given from the devicetree,
> > meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
> > ports.
> >
> > Besides, another thing to think about is... yes you cannot test nor implement
> > 2D1C mode in your submission, but this doesn't mean that others won't ever be
> > interested in this and that other people won't be actually implementing that;
> > Providing them with the right initial driver structure will surely make things
> > easier, encouraging other people from the community to spend their precious
> > time on the topic.
> Hi Angelo,
> Ok, I see your point, but for future potential upgrade to support A/B
> ports I was thinking of something else: adding independent nodes for csixA
> and csixB such as:
>
> csi0_rx: phy@11c10000 {
>    reg = <0 0x11C10000 0 0x2000>;
>    mediatek,mode = <4D1c>;
>    ...
> };
>
> csi0a_rx: phy@11c10000 {
>    reg = <0 0x11C10000 0 0x1000>;
>    mediatek,mode = <2D1c>;
>    ...
> };
> csi0b_rx: phy@11c11000 {
>    reg = <0 0x11C11000 0 0x1000>;
>    mediatek,mode = <2D1c>;
>    ...
> };
>
> giving the correct register range. One thing I did not mention is that if
> csi0_rx is used csi0a_rx and csi0b_rx cannot be used (they share same
> physical lanes as csio_rx), but csi0a_rx and csi0b_rx can be used simultaneously.
> So platform device will enable only the node(s) it needs and enabling
> csi0_rx and csioa/b_rx will fail because they share the same register
> region and map will fail and it does not have any sense because you
> either have a camera using the whole port or sub port but you cannot have
> both plugged in. What do you think about it?

Your description of the hardware makes me even more confident in pushing for
having one single node with multiple iospaces.

You could have a node such as:

csi0_rx: phy@11c10000 {
	compatible = ....
	reg = <0 0x11c10000 0 0x1000>, <0 0x11c20000 0 0x1000>;
	reg-names = "csi-a", "csi-b";
	/* 4 means 4D1C */
	num-lanes = <4>;
		or
	/* 2 means 2D1C */
	num-lanes = <2>;
};

You would then reference the csi0_rx node as:

	/* PHY is configured as 4 lanes (4D1C) */
	something = <&csi0_rx 0>;

	or

	/* First two lanes (CSI0 PORT-A) */
	something = <&csi0_rx 0>;
	/* Second two lanes (CSI0 PORT-B) */
	something = <&csi0_rx 1>;

Preferrably, you should (or shall?) use a graph to describe such connections,
anyway.

This is because overriding the number of lanes on a per-board basis becomes
*otherwise* difficult, in the sense of human readability issues, other than
duplicated nodes being a real issue.

Regards,
Angelo