| Message ID | 20220302220739.144303-1-paul.kocialkowski@bootlin.com |
|---|---|
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 and A31 ISP / MIPI CSI-2 Support | expand |
On Wed, 02 Mar 2022 23:07:31 +0100, Paul Kocialkowski wrote: > The Allwinner A31 MIPI D-PHY block supports both tx and rx directions, > although each instance of the block is meant to be used in one > direction only. There will typically be one instance for MIPI DSI and > one for MIPI CSI-2 (it seems unlikely to ever see a shared instance). > > Describe the direction with a new allwinner,direction property. > For backwards compatibility, the property is optional and tx mode > should be assumed by default. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
Hi Paul, Thanks for the patch. On Wed, Mar 02, 2022 at 11:07:37PM +0100, Paul Kocialkowski wrote: > This introduces YAML bindings documentation for the Allwinner A83T > MIPI CSI-2 controller. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 138 ++++++++++++++++++ > 1 file changed, 138 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > new file mode 100644 > index 000000000000..75121b402435 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > @@ -0,0 +1,138 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/allwinner,sun8i-a83t-mipi-csi2.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Allwinner A83T MIPI CSI-2 Device Tree Bindings > + > +maintainers: > + - Paul Kocialkowski <paul.kocialkowski@bootlin.com> > + > +properties: > + compatible: > + const: allwinner,sun8i-a83t-mipi-csi2 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + items: > + - description: Bus Clock > + - description: Module Clock > + - description: MIPI-specific Clock > + - description: Misc CSI Clock > + > + clock-names: > + items: > + - const: bus > + - const: mod > + - const: mipi > + - const: misc > + > + resets: > + maxItems: 1 > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + description: Input port, connect to a MIPI CSI-2 sensor > + > + properties: > + reg: > + const: 0 > + > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + clock-lanes: > + maxItems: 1 Does the hardware support lane reordering? If not, the property should be omitted here. I can also remove the three lines here while applying the patches. > + > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + required: > + - data-lanes > + > + additionalProperties: false > + > + port@1: > + $ref: /schemas/graph.yaml#/$defs/port-base > + description: Output port, connect to a CSI controller > + > + properties: > + reg: > + const: 1 > + > + endpoint: > + $ref: video-interfaces.yaml# > + unevaluatedProperties: false > + > + additionalProperties: false > + > + required: > + - port@0 > + - port@1 > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - resets > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/sun8i-a83t-ccu.h> > + #include <dt-bindings/reset/sun8i-a83t-ccu.h> > + > + mipi_csi2: csi@1cb1000 { > + compatible = "allwinner,sun8i-a83t-mipi-csi2"; > + reg = <0x01cb1000 0x1000>; > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ccu CLK_BUS_CSI>, > + <&ccu CLK_CSI_SCLK>, > + <&ccu CLK_MIPI_CSI>, > + <&ccu CLK_CSI_MISC>; > + clock-names = "bus", "mod", "mipi", "misc"; > + resets = <&ccu RST_BUS_CSI>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mipi_csi2_in: port@0 { > + reg = <0>; > + > + mipi_csi2_in_ov8865: endpoint { > + data-lanes = <1 2 3 4>; > + > + remote-endpoint = <&ov8865_out_mipi_csi2>; > + }; > + }; > + > + mipi_csi2_out: port@1 { > + reg = <1>; > + > + mipi_csi2_out_csi: endpoint { > + remote-endpoint = <&csi_in_mipi_csi2>; > + }; > + }; > + }; > + }; > + > +...
Hi Paul, Thanks for the set. On Wed, Mar 02, 2022 at 11:07:35PM +0100, Paul Kocialkowski wrote: ... > +static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > +{ > + struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev); > + struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev; > + union phy_configure_opts dphy_opts = { 0 }; > + struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy; > + struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format; > + const struct sun6i_mipi_csi2_format *format; > + struct phy *dphy = csi2_dev->dphy; > + struct device *dev = csi2_dev->dev; > + struct v4l2_ctrl *ctrl; > + unsigned int lanes_count = > + csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > + unsigned long pixel_rate; > + /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > + int ret = 0; > + > + if (!source_subdev) > + return -ENODEV; > + > + if (!on) { > + v4l2_subdev_call(source_subdev, video, s_stream, 0); > + goto disable; > + } > + > + /* Runtime PM */ > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret < 0) > + return ret; > + > + /* Sensor Pixel Rate */ > + > + ctrl = v4l2_ctrl_find(source_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(dev, "missing sensor pixel rate\n"); > + ret = -ENODEV; > + goto error_pm; > + } > + > + pixel_rate = (unsigned long)v4l2_ctrl_g_ctrl_int64(ctrl); > + if (!pixel_rate) { > + dev_err(dev, "missing (zero) sensor pixel rate\n"); > + ret = -ENODEV; > + goto error_pm; > + } > + > + /* D-PHY */ > + > + if (!lanes_count) { I first thought this check could be moved to the beginning, but it's also redundant. v4l2_fwnode_endpoint_parse() will check the configuration is valid, i.e. the number of lanes is not zero. But should you add checks to make sure the hardware supports what has been configured? I'd do that right after parsing the endpoint. And you only seem to be using the number of data lanes, nothing more. So I'd store that, instead of the entire parsed v4l2_fwnode_endpoint. The same applies to patch 8. I think these could be done on top of this set after it is merged. Up to you. ... > +static int > +sun6i_mipi_csi2_bridge_source_setup(struct sun6i_mipi_csi2_device *csi2_dev) > +{ > + struct v4l2_async_notifier *notifier = &csi2_dev->bridge.notifier; > + struct v4l2_fwnode_endpoint *endpoint = &csi2_dev->bridge.endpoint; > + struct v4l2_async_subdev *subdev_async; > + struct fwnode_handle *handle; > + struct device *dev = csi2_dev->dev; > + int ret; > + > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + return -ENODEV; > + > + endpoint->bus_type = V4L2_MBUS_CSI2_DPHY; > + > + ret = v4l2_fwnode_endpoint_parse(handle, endpoint); > + if (ret) > + goto complete; > + > + subdev_async = v4l2_async_nf_add_fwnode_remote(notifier, handle, > + struct v4l2_async_subdev); > + if (IS_ERR(subdev_async)) > + ret = PTR_ERR(subdev_async); > + > +complete: > + fwnode_handle_put(handle); > + > + return ret; > +}
Hi Sakari, On Wed 16 Mar 22, 15:26, Sakari Ailus wrote: > Hi Paul, > > Thanks for the patch. And thanks for the review! > On Wed, Mar 02, 2022 at 11:07:37PM +0100, Paul Kocialkowski wrote: > > This introduces YAML bindings documentation for the Allwinner A83T > > MIPI CSI-2 controller. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Reviewed-by: Rob Herring <robh@kernel.org> > > --- > > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 138 ++++++++++++++++++ > > 1 file changed, 138 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > new file mode 100644 > > index 000000000000..75121b402435 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > @@ -0,0 +1,138 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/allwinner,sun8i-a83t-mipi-csi2.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Allwinner A83T MIPI CSI-2 Device Tree Bindings > > + > > +maintainers: > > + - Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > + > > +properties: > > + compatible: > > + const: allwinner,sun8i-a83t-mipi-csi2 > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: Bus Clock > > + - description: Module Clock > > + - description: MIPI-specific Clock > > + - description: Misc CSI Clock > > + > > + clock-names: > > + items: > > + - const: bus > > + - const: mod > > + - const: mipi > > + - const: misc > > + > > + resets: > > + maxItems: 1 > > + > > + ports: > > + $ref: /schemas/graph.yaml#/properties/ports > > + > > + properties: > > + port@0: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + description: Input port, connect to a MIPI CSI-2 sensor > > + > > + properties: > > + reg: > > + const: 0 > > + > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > Does the hardware support lane reordering? If not, the property should be > omitted here. I'm not sure what this relates to. Is it about inverting the clock lane with a data lane? I'm a bit confused about logical vs physical lane in the context of MIPI CSI-2. The controller has dedicated pins for the clock and data lanes and supports filtering packets based on virtual channel or data type. Are the clock-lanes and data-lanes only relevant for reordering? IIRC they are also necessary to get the lanes count in the driver. > I can also remove the three lines here while applying the patches. I think this series will need another iteration anyway, so let's wait. Paul > > + > > + data-lanes: > > + minItems: 1 > > + maxItems: 4 > > + > > + required: > > + - data-lanes > > + > > + additionalProperties: false > > + > > + port@1: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + description: Output port, connect to a CSI controller > > + > > + properties: > > + reg: > > + const: 1 > > + > > + endpoint: > > + $ref: video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + additionalProperties: false > > + > > + required: > > + - port@0 > > + - port@1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - resets > > + - ports > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/clock/sun8i-a83t-ccu.h> > > + #include <dt-bindings/reset/sun8i-a83t-ccu.h> > > + > > + mipi_csi2: csi@1cb1000 { > > + compatible = "allwinner,sun8i-a83t-mipi-csi2"; > > + reg = <0x01cb1000 0x1000>; > > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&ccu CLK_BUS_CSI>, > > + <&ccu CLK_CSI_SCLK>, > > + <&ccu CLK_MIPI_CSI>, > > + <&ccu CLK_CSI_MISC>; > > + clock-names = "bus", "mod", "mipi", "misc"; > > + resets = <&ccu RST_BUS_CSI>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mipi_csi2_in: port@0 { > > + reg = <0>; > > + > > + mipi_csi2_in_ov8865: endpoint { > > + data-lanes = <1 2 3 4>; > > + > > + remote-endpoint = <&ov8865_out_mipi_csi2>; > > + }; > > + }; > > + > > + mipi_csi2_out: port@1 { > > + reg = <1>; > > + > > + mipi_csi2_out_csi: endpoint { > > + remote-endpoint = <&csi_in_mipi_csi2>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > -- > Kind regards, > > Sakari Ailus
Hi Sakari, On Wed 16 Mar 22, 15:27, Sakari Ailus wrote: > Hi Paul, > > Thanks for the set. > > On Wed, Mar 02, 2022 at 11:07:35PM +0100, Paul Kocialkowski wrote: > ... > > +static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on) > > +{ > > + struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev); > > + struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev; > > + union phy_configure_opts dphy_opts = { 0 }; > > + struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy; > > + struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format; > > + const struct sun6i_mipi_csi2_format *format; > > + struct phy *dphy = csi2_dev->dphy; > > + struct device *dev = csi2_dev->dev; > > + struct v4l2_ctrl *ctrl; > > + unsigned int lanes_count = > > + csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes; > > + unsigned long pixel_rate; > > + /* Initialize to 0 to use both in disable label (ret != 0) and off. */ > > + int ret = 0; > > + > > + if (!source_subdev) > > + return -ENODEV; > > + > > + if (!on) { > > + v4l2_subdev_call(source_subdev, video, s_stream, 0); > > + goto disable; > > + } > > + > > + /* Runtime PM */ > > + > > + ret = pm_runtime_resume_and_get(dev); > > + if (ret < 0) > > + return ret; > > + > > + /* Sensor Pixel Rate */ > > + > > + ctrl = v4l2_ctrl_find(source_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + dev_err(dev, "missing sensor pixel rate\n"); > > + ret = -ENODEV; > > + goto error_pm; > > + } > > + > > + pixel_rate = (unsigned long)v4l2_ctrl_g_ctrl_int64(ctrl); > > + if (!pixel_rate) { > > + dev_err(dev, "missing (zero) sensor pixel rate\n"); > > + ret = -ENODEV; > > + goto error_pm; > > + } > > + > > + /* D-PHY */ > > + > > + if (!lanes_count) { > > I first thought this check could be moved to the beginning, but it's also > redundant. v4l2_fwnode_endpoint_parse() will check the configuration is > valid, i.e. the number of lanes is not zero. Good to know, thanks! > But should you add checks to make sure the hardware supports what has been > configured? I'd do that right after parsing the endpoint. I guess you mean checking that we don't get more than 4 lanes? And maybe something related to lane ordering too? > And you only seem to be using the number of data lanes, nothing more. So > I'd store that, instead of the entire parsed v4l2_fwnode_endpoint. That's correct, why not. > The same applies to patch 8. > > I think these could be done on top of this set after it is merged. Up to > you. I'll go for another iteration. Thanks! Paul > ... > > > +static int > > +sun6i_mipi_csi2_bridge_source_setup(struct sun6i_mipi_csi2_device *csi2_dev) > > +{ > > + struct v4l2_async_notifier *notifier = &csi2_dev->bridge.notifier; > > + struct v4l2_fwnode_endpoint *endpoint = &csi2_dev->bridge.endpoint; > > + struct v4l2_async_subdev *subdev_async; > > + struct fwnode_handle *handle; > > + struct device *dev = csi2_dev->dev; > > + int ret; > > + > > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, > > + FWNODE_GRAPH_ENDPOINT_NEXT); > > + if (!handle) > > + return -ENODEV; > > + > > + endpoint->bus_type = V4L2_MBUS_CSI2_DPHY; > > + > > + ret = v4l2_fwnode_endpoint_parse(handle, endpoint); > > + if (ret) > > + goto complete; > > + > > + subdev_async = v4l2_async_nf_add_fwnode_remote(notifier, handle, > > + struct v4l2_async_subdev); > > + if (IS_ERR(subdev_async)) > > + ret = PTR_ERR(subdev_async); > > + > > +complete: > > + fwnode_handle_put(handle); > > + > > + return ret; > > +} > > -- > Kind regards, > > Sakari Ailus
This new version is an offspring from the big "Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support" series, which was split into individual series for better clarity and handling. This part only concerns the MIPI CSI-2 controllers support changes. Changes since all-in-one v2: - Use the newly-introduced media/mipi-csi2.h header instead of local definitions; - Introduce a use a mutex for format access serialization; - Make both port@0 and port@1 as well as ports required in the binding; - Made one of the two CSI input ports required; Paul Kocialkowski (9): dt-bindings: sun6i-a31-mipi-dphy: Add optional direction property phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 dt-bindings: media: sun6i-a31-csi: Add MIPI CSI-2 input port dt-bindings: media: Add Allwinner A31 MIPI CSI-2 bindings documentation media: sunxi: Add support for the A31 MIPI CSI-2 controller MAINTAINERS: Add entry for the Allwinner A31 MIPI CSI-2 bridge driver dt-bindings: media: Add Allwinner A83T MIPI CSI-2 bindings documentation media: sunxi: Add support for the A83T MIPI CSI-2 controller MAINTAINERS: Add entry for the Allwinner A83T MIPI CSI-2 bridge .../media/allwinner,sun6i-a31-csi.yaml | 66 +- .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 147 ++++ .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 138 +++ .../phy/allwinner,sun6i-a31-mipi-dphy.yaml | 12 + MAINTAINERS | 16 + drivers/media/platform/sunxi/Kconfig | 2 + drivers/media/platform/sunxi/Makefile | 2 + .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 12 + .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 766 ++++++++++++++++ .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 53 ++ .../sun6i-mipi-csi2/sun6i_mipi_csi2_reg.h | 76 ++ .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 72 ++ .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 + .../sun8i_a83t_mipi_csi2.c | 833 ++++++++++++++++++ .../sun8i_a83t_mipi_csi2.h | 56 ++ .../sun8i_a83t_mipi_csi2_reg.h | 151 ++++ drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 166 +++- 20 files changed, 2609 insertions(+), 17 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h create mode 100644 drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2_reg.h create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/Kconfig create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/Makefile create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.h create mode 100644 drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2_reg.h