| Message ID | 20240704-add-mtk-isp-3-0-support-v5-0-bfccccc5ec21@baylibre.com |
|---|---|
| Headers | show |
| Series | Add Mediatek ISP3.0 | expand |
On Thu, Jul 04, 2024 at 03:36:40PM +0200, Julien Stephan wrote: > From: Louis Kuo <louis.kuo@mediatek.com> > > This adds the bindings, for the mediatek ISP3.0 SENINF module embedded in > some Mediatek SoC, such as the mt8365 > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com> > Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com> > Link: https://lore.kernel.org/r/20230807094940.329165-2-jstephan@baylibre.com > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Julien Stephan <jstephan@baylibre.com> I'm really confused by the link tag here. At first glance this looked like you were sending out something that had been applied by Laurent, given the Link, Rb and SoB from him. Why does he have a SoB on this patch? What did Phi-Bang Nguyen do with this patch, and should they have a Co-developed-by tag? > --- > .../bindings/media/mediatek,mt8365-seninf.yaml | 275 +++++++++++++++++++++ > MAINTAINERS | 7 + > 2 files changed, 282 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml > new file mode 100644 > index 000000000000..aeabea9f956a > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml > @@ -0,0 +1,275 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2023 MediaTek, BayLibre > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/mediatek,mt8365-seninf.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek Sensor Interface 3.0 > + > +maintainers: > + - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + - Julien Stephan <jstephan@baylibre.com> > + - Andy Hsieh <andy.hsieh@mediatek.com> > + > +description: > + The ISP3.0 SENINF is the CSI-2 and parallel camera sensor interface found in > + multiple MediaTek SoCs. It can support up to three physical CSI-2 input ports, > + configured in DPHY (2 or 4 data lanes) or CPHY depending on the SoC. > + On the output side, SENINF can be connected either to CAMSV instance or > + to the internal ISP. CAMSV is used to bypass the internal ISP processing > + in order to connect either an external ISP, or a sensor (RAW, YUV). > + > +properties: > + compatible: > + const: mediatek,mt8365-seninf > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + power-domains: > + maxItems: 1 > + > + clocks: > + items: > + - description: Seninf camsys clock > + - description: Seninf top mux clock > + > + clock-names: > + items: > + - const: camsys > + - const: top_mux > + > + phys: true > + > + phy-names: true > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI0 or CSI0A port > + > + properties: > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + clock-lanes: > + maxItems: 1 > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + port@1: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI1 port > + > + properties: > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + clock-lanes: > + maxItems: 1 > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + port@2: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI2 port > + > + properties: > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + clock-lanes: > + maxItems: 1 > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + port@3: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: CSI0B port > + > + properties: > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + clock-lanes: > + maxItems: 1 > + data-lanes: > + minItems: 1 > + maxItems: 2 > + > + port@4: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for cam0 > + > + port@5: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for cam1 > + > + port@6: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for camsv0 > + > + port@7: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for camsv1 > + > + port@8: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for camsv2 > + > + port@9: > + $ref: /schemas/graph.yaml#/properties/port > + description: connection point for camsv3 > + > + required: > + - port@0 > + - port@1 > + - port@2 > + - port@3 > + - port@4 > + - port@5 > + - port@6 > + - port@7 > + - port@8 > + - port@9 > + > +required: > + - compatible > + - interrupts > + - clocks > + - clock-names > + - power-domains > + - ports > + > +additionalProperties: false > + > +if: > + properties: > + compatible: > + contains: > + const: mediatek,mt8365-seninf The binding supports only a single compatible, why is this complexity required? I don't see other devices being added in this series. Cheers, Conor. > +then: > + properties: > + phys: > + minItems: 2 > + maxItems: 2 > + description: > + phandle to the PHYs connected to CSI0/A, CSI1, CSI0B > + > + phy-names: > + description: > + list of PHYs names > + minItems: 2 > + maxItems: 2 > + items: > + type: string > + enum: > + - csi0 > + - csi1 > + - csi0b > + uniqueItems: true
Le jeu. 4 juil. 2024 à 18:27, Conor Dooley <conor@kernel.org> a écrit : > > On Thu, Jul 04, 2024 at 03:36:40PM +0200, Julien Stephan wrote: > > From: Louis Kuo <louis.kuo@mediatek.com> > > > > This adds the bindings, for the mediatek ISP3.0 SENINF module embedded in > > some Mediatek SoC, such as the mt8365 > > > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com> > > Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com> > > Link: https://lore.kernel.org/r/20230807094940.329165-2-jstephan@baylibre.com > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > > I'm really confused by the link tag here. At first glance this looked > like you were sending out something that had been applied by Laurent, > given the Link, Rb and SoB from him. Why does he have a SoB on this > patch? What did Phi-Bang Nguyen do with this patch, and should they have > a Co-developed-by tag? Hi Conor, I was not using b4 for the previous revisions of this series, so maybe I messed something up here :( About Phi-Bang, this series has been in our internal tree for a long time, and Phi-Bang has his SoB on it, so I kept it. About Laurent's tags, they were already on v4. But maybe it was an error ? Should I remove them? > > > --- > > .../bindings/media/mediatek,mt8365-seninf.yaml | 275 +++++++++++++++++++++ > > MAINTAINERS | 7 + > > 2 files changed, 282 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml > > new file mode 100644 > > index 000000000000..aeabea9f956a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml > > @@ -0,0 +1,275 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2023 MediaTek, BayLibre > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/mediatek,mt8365-seninf.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: MediaTek Sensor Interface 3.0 > > + > > +maintainers: > > + - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + - Julien Stephan <jstephan@baylibre.com> > > + - Andy Hsieh <andy.hsieh@mediatek.com> > > + > > +description: > > + The ISP3.0 SENINF is the CSI-2 and parallel camera sensor interface found in > > + multiple MediaTek SoCs. It can support up to three physical CSI-2 input ports, > > + configured in DPHY (2 or 4 data lanes) or CPHY depending on the SoC. > > + On the output side, SENINF can be connected either to CAMSV instance or > > + to the internal ISP. CAMSV is used to bypass the internal ISP processing > > + in order to connect either an external ISP, or a sensor (RAW, YUV). > > + > > +properties: > > + compatible: > > + const: mediatek,mt8365-seninf > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + power-domains: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: Seninf camsys clock > > + - description: Seninf top mux clock > > + > > + clock-names: > > + items: > > + - const: camsys > > + - const: top_mux > > + > > + phys: true > > + > > + phy-names: true > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + unevaluatedProperties: false > > + description: CSI0 or CSI0A port > > + > > + properties: > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > + data-lanes: > > + minItems: 1 > > + maxItems: 4 > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + unevaluatedProperties: false > > + description: CSI1 port > > + > > + properties: > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > + data-lanes: > > + minItems: 1 > > + maxItems: 4 > > + > > + port@2: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + unevaluatedProperties: false > > + description: CSI2 port > > + > > + properties: > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > + data-lanes: > > + minItems: 1 > > + maxItems: 4 > > + > > + port@3: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + unevaluatedProperties: false > > + description: CSI0B port > > + > > + properties: > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > + data-lanes: > > + minItems: 1 > > + maxItems: 2 > > + > > + port@4: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for cam0 > > + > > + port@5: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for cam1 > > + > > + port@6: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for camsv0 > > + > > + port@7: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for camsv1 > > + > > + port@8: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for camsv2 > > + > > + port@9: > > + $ref: /schemas/graph.yaml#/properties/port > > + description: connection point for camsv3 > > + > > + required: > > + - port@0 > > + - port@1 > > + - port@2 > > + - port@3 > > + - port@4 > > + - port@5 > > + - port@6 > > + - port@7 > > + - port@8 > > + - port@9 > > + > > +required: > > + - compatible > > + - interrupts > > + - clocks > > + - clock-names > > + - power-domains > > + - ports > > + > > +additionalProperties: false > > + > > +if: > > + properties: > > + compatible: > > + contains: > > + const: mediatek,mt8365-seninf > > The binding supports only a single compatible, why is this complexity > required? I don't see other devices being added in this series. Right. The idea is that the number of PHYs depends on the SoC. In the previous revision of the series, the number of PHYs was not fixed, and Krzysztof asked me to fix it by SoC. So I wanted to make it clear that the number of PHYs depends on SoC but maybe I don't need that complexity for that? Is something like the following enough? And if complexity is added later if some other SoC are added? phys: minItems: 2 maxItems: 2 description: phandle to the PHYs connected to CSI0/A, CSI1, CSI0B phy-names: description: list of PHYs names minItems: 2 maxItems: 2 items: type: string enum: - csi0 - csi1 - csi0b uniqueItems: true Cheers Julien > > Cheers, > Conor. > > > +then: > > + properties: > > + phys: > > + minItems: 2 > > + maxItems: 2 > > + description: > > + phandle to the PHYs connected to CSI0/A, CSI1, CSI0B > > + > > + phy-names: > > + description: > > + list of PHYs names > > + minItems: 2 > > + maxItems: 2 > > + items: > > + type: string > > + enum: > > + - csi0 > > + - csi1 > > + - csi0b > > + uniqueItems: true
On Fri, Jul 05, 2024 at 09:50:59AM +0200, Julien Stephan wrote: > Le jeu. 4 juil. 2024 à 18:27, Conor Dooley <conor@kernel.org> a écrit : > > > > On Thu, Jul 04, 2024 at 03:36:40PM +0200, Julien Stephan wrote: > > > From: Louis Kuo <louis.kuo@mediatek.com> > > > > > > This adds the bindings, for the mediatek ISP3.0 SENINF module embedded in > > > some Mediatek SoC, such as the mt8365 > > > > > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com> > > > Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com> > > > Link: https://lore.kernel.org/r/20230807094940.329165-2-jstephan@baylibre.com > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > > > > I'm really confused by the link tag here. At first glance this looked > > like you were sending out something that had been applied by Laurent, > > given the Link, Rb and SoB from him. Why does he have a SoB on this > > patch? What did Phi-Bang Nguyen do with this patch, and should they have > > a Co-developed-by tag? > > I was not using b4 for the previous revisions of this series, so maybe > I messed something up here :( b4 am has an option to add a link to a patch you apply from the mailing list (-l, --add-link) but you should not be using that as a contributor. In this case, that link provides no value and is just confusing. > About Phi-Bang, this series has been in our internal tree for a long > time, and Phi-Bang has his SoB on it, so I kept it. > > About Laurent's tags, they were already on v4. But maybe it was an > error ? Should I remove them? They were also on v1. Did Laurent write part of these bindings, and should he have a Co-developed-by? > > > +additionalProperties: false > > > + > > > +if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: mediatek,mt8365-seninf > > > > The binding supports only a single compatible, why is this complexity > > required? I don't see other devices being added in this series. > > Right. The idea is that the number of PHYs depends on the SoC. In the > previous revision of the series, > the number of PHYs was not fixed, and Krzysztof asked me to fix it by > SoC. So I wanted to make it clear > that the number of PHYs depends on SoC but maybe I don't need that > complexity for that? > > Is something like the following enough? And if complexity is added > later if some other SoC are added? Yes, that looks reasonable to me. Adding conditional stuff can be done iff another soc re-uses the binding. Thanks, Conor. > phys: > minItems: 2 > maxItems: 2 > description: > phandle to the PHYs connected to CSI0/A, CSI1, CSI0B > > phy-names: > description: > list of PHYs names > minItems: 2 > maxItems: 2 > items: > type: string > enum: > - csi0 > - csi1 > - csi0b > uniqueItems: true
Le ven. 5 juil. 2024 à 11:24, Conor Dooley <conor.dooley@microchip.com> a écrit : > > On Fri, Jul 05, 2024 at 09:50:59AM +0200, Julien Stephan wrote: > > Le jeu. 4 juil. 2024 à 18:27, Conor Dooley <conor@kernel.org> a écrit : > > > > > > On Thu, Jul 04, 2024 at 03:36:40PM +0200, Julien Stephan wrote: > > > > From: Louis Kuo <louis.kuo@mediatek.com> > > > > > > > > This adds the bindings, for the mediatek ISP3.0 SENINF module embedded in > > > > some Mediatek SoC, such as the mt8365 > > > > > > > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com> > > > > Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com> > > > > Link: https://lore.kernel.org/r/20230807094940.329165-2-jstephan@baylibre.com > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > > > > > > I'm really confused by the link tag here. At first glance this looked > > > like you were sending out something that had been applied by Laurent, > > > given the Link, Rb and SoB from him. Why does he have a SoB on this > > > patch? What did Phi-Bang Nguyen do with this patch, and should they have > > > a Co-developed-by tag? > > > > I was not using b4 for the previous revisions of this series, so maybe > > I messed something up here :( > > b4 am has an option to add a link to a patch you apply from the mailing > list (-l, --add-link) but you should not be using that as a contributor. > In this case, that link provides no value and is just confusing. > > > About Phi-Bang, this series has been in our internal tree for a long > > time, and Phi-Bang has his SoB on it, so I kept it. > > > > About Laurent's tags, they were already on v4. But maybe it was an > > error ? Should I remove them? > > They were also on v1. Did Laurent write part of these bindings, and > should he have a Co-developed-by? > Got it! I understood where I messed up :) I'll remove the link and add the Co-developed-by tag of Laurent > > > > +additionalProperties: false > > > > + > > > > +if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: mediatek,mt8365-seninf > > > > > > The binding supports only a single compatible, why is this complexity > > > required? I don't see other devices being added in this series. > > > > Right. The idea is that the number of PHYs depends on the SoC. In the > > previous revision of the series, > > the number of PHYs was not fixed, and Krzysztof asked me to fix it by > > SoC. So I wanted to make it clear > > that the number of PHYs depends on SoC but maybe I don't need that > > complexity for that? > > > > Is something like the following enough? And if complexity is added > > later if some other SoC are added? > > Yes, that looks reasonable to me. Adding conditional stuff can be done > iff another soc re-uses the binding. Will do in the next series. Thank you for your feedback on this! Cheers Julien > > Thanks, > Conor. > > > phys: > > minItems: 2 > > maxItems: 2 > > description: > > phandle to the PHYs connected to CSI0/A, CSI1, CSI0B > > > > phy-names: > > description: > > list of PHYs names > > minItems: 2 > > maxItems: 2 > > items: > > type: string > > enum: > > - csi0 > > - csi1 > > - csi0b > > uniqueItems: true
This series adds the support of the Mediatek ISP3.0 found on some Mediatek SoCs such as the mt8365. The driver is divided into 2 parts: * SENINF: the sensor interface * CAMSV: this driver provides a path to bypass the SoC ISP so that image data coming from the SENINF can go directly into memory without any image processing. This allows the use of an external ISP or camera sensor directly. The SENINF driver is based on previous work done by Louis Kuo available as an RFC here: https://lore.kernel.org/all/20200708104023.3225-1-louis.kuo@mediatek.com/ This series depends on the following series for the phy [1] Changes on v5: drivers: - rebase on 6.10-rc1 - fix various comments from all reviews (mostly style issues and minor code refactor) - add a function to calculate the clock divider for the master sensor clock: NOTE: setting this register seems to have no effect at all, currently checking with mediatek apps engineer (OOO until 17/04) bindings: - camsv: update description - seninf: fix phy definition and example indentation - use generic name for node example dts: - sort nodes by addresses - use lower case for hexadecimal Changes in v4: - fix suspend/resume deadlock - fix various locking issues reported by Laurent Pinchart on v3 - run LOCKDEP - add missing include reported by kernel-test-robot for non mediatek arch and COMPILE_TEST=y - use atomic poll inside mtk_camsv30_setup - drop second lane support as it was not used - remove useless members in structs - fix media entity initialization - initialize correct pad for camsv video device - add isp support in mt8365.dtsi - rebase on 6.7 Changes in v3: - fix a lot of formatting issues/coding style issues found in camsv/seninf reported by Angelo on v2 - fix camsv/seninf binding file error reported by Rob Changes in v2: - renamed clock `cam_seninf` to `camsys` - renamed clock `top_mux_seninf` to `top_mux` - moved phy properties from port nodes to top level - remove patternProperties - specify power management dependency in the cover letter description to fix missing include in dt-binding example - change '$ref' properties on some endpoint nodes from '$ref: video-interfaces.yaml#' to '$ref: /schemas/graph.yaml#/$defs/endpoint-base' where applicable Best Julien Stephan Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- Julien Stephan (1): arm64: dts: mediatek: mt8365: Add support for camera Louis Kuo (2): dt-bindings: media: add mediatek ISP3.0 sensor interface media: platform: mediatek: isp_30: add mediatek ISP3.0 sensor interface Phi-bang Nguyen (2): dt-bindings: media: add mediatek ISP3.0 camsv media: platform: mediatek: isp_30: add mediatek ISP3.0 camsv .../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++ .../bindings/media/mediatek,mt8365-seninf.yaml | 275 ++++ MAINTAINERS | 10 + arch/arm64/boot/dts/mediatek/mt8365.dtsi | 125 ++ drivers/media/platform/mediatek/Kconfig | 1 + drivers/media/platform/mediatek/Makefile | 1 + drivers/media/platform/mediatek/isp/Kconfig | 2 + drivers/media/platform/mediatek/isp/Makefile | 3 + drivers/media/platform/mediatek/isp/isp_30/Kconfig | 35 + .../media/platform/mediatek/isp/isp_30/Makefile | 4 + .../platform/mediatek/isp/isp_30/camsv/Makefile | 7 + .../platform/mediatek/isp/isp_30/camsv/mtk_camsv.c | 327 ++++ .../platform/mediatek/isp/isp_30/camsv/mtk_camsv.h | 196 +++ .../mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c | 413 +++++ .../mediatek/isp/isp_30/camsv/mtk_camsv30_regs.h | 60 + .../mediatek/isp/isp_30/camsv/mtk_camsv_video.c | 750 ++++++++++ .../platform/mediatek/isp/isp_30/seninf/Makefile | 5 + .../mediatek/isp/isp_30/seninf/mtk_seninf.c | 1576 ++++++++++++++++++++ .../mediatek/isp/isp_30/seninf/mtk_seninf_reg.h | 117 ++ 19 files changed, 4016 insertions(+) --- base-commit: 99b9aaac4abdf30968b2ce9c9848951290fbde92 change-id: 20240704-add-mtk-isp-3-0-support-a08a978cac36 Best regards,