| Message ID | 20201023174546.504028-1-paul.kocialkowski@bootlin.com |
|---|---|
| Headers | show |
| Series | Allwinner MIPI CSI-2 support for A31/V3s/A83T | expand |
Hi! Dne petek, 23. oktober 2020 ob 19:45:33 CEST je Paul Kocialkowski napisal(a): > As some D-PHY controllers support both Rx and Tx mode, we need a way for > users to explicitly request one or the other. For instance, Rx mode can > be used along with MIPI CSI-2 while Tx mode can be used with MIPI DSI. > > Introduce new MIPI D-PHY PHY submodes to use with PHY_MODE_MIPI_DPHY. > The default (zero value) is kept to Tx so only the rkisp1 driver, which > uses D-PHY in Rx mode, needs to be adapted. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 ++- > include/linux/phy/phy-mipi-dphy.h | 13 +++++++++++++ I think some changes are missing in this patch. For example, phy_set_mode_ext() must be modified to take another argument, otherwise change of rkisp1-isp driver doesn't make much sense. Best regards, Jernej > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/ media/rkisp1/rkisp1-isp.c > index 6ec1e9816e9f..0afbce00121e 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -902,7 +902,8 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp, > > phy_mipi_dphy_get_default_config(pixel_clock, isp->sink_fmt- >bus_width, > sensor->lanes, cfg); > - phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY); > + phy_set_mode_ext(cdev->dphy, PHY_MODE_MIPI_DPHY, > + PHY_MIPI_DPHY_SUBMODE_RX); > phy_configure(sensor->dphy, &opts); > phy_power_on(sensor->dphy); > > diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi- dphy.h > index a877ffee845d..0f57ef46a8b5 100644 > --- a/include/linux/phy/phy-mipi-dphy.h > +++ b/include/linux/phy/phy-mipi-dphy.h > @@ -6,6 +6,19 @@ > #ifndef __PHY_MIPI_DPHY_H_ > #define __PHY_MIPI_DPHY_H_ > > +/** > + * enum phy_mipi_dphy_submode - MIPI D-PHY sub-mode > + * > + * A MIPI D-PHY can be used to transmit or receive data. > + * Since some controllers can support both, the direction to enable is specified > + * with the PHY sub-mode. Transmit is assumed by default with phy_set_mode. > + */ > + > +enum phy_mipi_dphy_submode { > + PHY_MIPI_DPHY_SUBMODE_TX = 0, > + PHY_MIPI_DPHY_SUBMODE_RX, > +}; > + > /** > * struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set > * > -- > 2.28.0 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/ linux-sunxi/20201023174546.504028-2-paul.kocialkowski%40bootlin.com. >
Hi Jernej, On Fri 23 Oct 20, 20:18, Jernej Škrabec wrote: > Dne petek, 23. oktober 2020 ob 19:45:33 CEST je Paul Kocialkowski napisal(a): > > As some D-PHY controllers support both Rx and Tx mode, we need a way for > > users to explicitly request one or the other. For instance, Rx mode can > > be used along with MIPI CSI-2 while Tx mode can be used with MIPI DSI. > > > > Introduce new MIPI D-PHY PHY submodes to use with PHY_MODE_MIPI_DPHY. > > The default (zero value) is kept to Tx so only the rkisp1 driver, which > > uses D-PHY in Rx mode, needs to be adapted. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 ++- > > include/linux/phy/phy-mipi-dphy.h | 13 +++++++++++++ > > I think some changes are missing in this patch. For example, > phy_set_mode_ext() must be modified to take another argument, otherwise change > of rkisp1-isp driver doesn't make much sense. Thanks for looking into this! As you can see in: https://elixir.bootlin.com/linux/latest/source/include/linux/phy/phy.h#L213 phy_set_mode_ext already takes a submode argument (which is already used for USB mode selection, for instance) and phy_set_mode is just a macro which calls phy_set_mode_ext with submode set to 0. In our case, that means that most current users of phy_set_mode with PHY_MODE_MIPI_DPHY will select Tx mode by default, so there is no particular need for adaptation. Only the rkisp1 driver uses PHY_MODE_MIPI_DPHY for Rx, so this one was changed to use phy_set_mode_ext with PHY_MIPI_DPHY_SUBMODE_RX instead. As a result, there should be no missing changes. Do you agree? Cheers, Paul > Best regards, > Jernej > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/ > media/rkisp1/rkisp1-isp.c > > index 6ec1e9816e9f..0afbce00121e 100644 > > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > > @@ -902,7 +902,8 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp > *isp, > > > > phy_mipi_dphy_get_default_config(pixel_clock, isp->sink_fmt- > >bus_width, > > sensor->lanes, cfg); > > - phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY); > > + phy_set_mode_ext(cdev->dphy, PHY_MODE_MIPI_DPHY, > > + PHY_MIPI_DPHY_SUBMODE_RX); > > phy_configure(sensor->dphy, &opts); > > phy_power_on(sensor->dphy); > > > > diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi- > dphy.h > > index a877ffee845d..0f57ef46a8b5 100644 > > --- a/include/linux/phy/phy-mipi-dphy.h > > +++ b/include/linux/phy/phy-mipi-dphy.h > > @@ -6,6 +6,19 @@ > > #ifndef __PHY_MIPI_DPHY_H_ > > #define __PHY_MIPI_DPHY_H_ > > > > +/** > > + * enum phy_mipi_dphy_submode - MIPI D-PHY sub-mode > > + * > > + * A MIPI D-PHY can be used to transmit or receive data. > > + * Since some controllers can support both, the direction to enable is > specified > > + * with the PHY sub-mode. Transmit is assumed by default with phy_set_mode. > > + */ > > + > > +enum phy_mipi_dphy_submode { > > + PHY_MIPI_DPHY_SUBMODE_TX = 0, > > + PHY_MIPI_DPHY_SUBMODE_RX, > > +}; > > + > > /** > > * struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set > > * > > -- > > 2.28.0 > > > > -- > > You received this message because you are subscribed to the Google Groups > "linux-sunxi" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to linux-sunxi+unsubscribe@googlegroups.com. > > To view this discussion on the web, visit https://groups.google.com/d/msgid/ > linux-sunxi/20201023174546.504028-2-paul.kocialkowski%40bootlin.com. > > > >
On Fri, Oct 23, 2020 at 07:45:44PM +0200, Paul Kocialkowski wrote: > +static int sun8i_a83t_mipi_csi2_v4l2_setup(struct sun8i_a83t_mipi_csi2_dev *cdev) > +{ > + struct sun8i_a83t_mipi_csi2_video *video = &cdev->video; > + struct v4l2_subdev *subdev = &video->subdev; > + struct v4l2_async_notifier *notifier = &video->notifier; > + struct fwnode_handle *handle; > + struct v4l2_fwnode_endpoint *endpoint; > + int ret; > + > + /* Subdev */ > + > + v4l2_subdev_init(subdev, &sun8i_a83t_mipi_csi2_subdev_ops); > + subdev->dev = cdev->dev; > + strscpy(subdev->name, MODULE_NAME, sizeof(subdev->name)); > + v4l2_set_subdevdata(subdev, cdev); > + > + /* Entity */ > + > + subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > + subdev->entity.ops = &sun8i_a83t_mipi_csi2_entity_ops; > + > + /* Pads */ > + > + video->pads[0].flags = MEDIA_PAD_FL_SINK; > + video->pads[1].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&subdev->entity, 2, video->pads); > + if (ret) > + return ret; > + > + /* Endpoint */ > + > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(cdev->dev), 0, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + goto error_media_entity; Missing error code. > + > + endpoint = &video->endpoint; > + endpoint->bus_type = V4L2_MBUS_CSI2_DPHY; > + > + ret = v4l2_fwnode_endpoint_parse(handle, endpoint); > + fwnode_handle_put(handle); > + if (ret) > + goto error_media_entity; > + > + /* Notifier */ > + > + v4l2_async_notifier_init(notifier); > + > + ret = v4l2_async_notifier_add_fwnode_remote_subdev(notifier, handle, > + &video->subdev_async); > + if (ret) > + goto error_media_entity; > + > + video->notifier.ops = &sun8i_a83t_mipi_csi2_notifier_ops; > + > + ret = v4l2_async_subdev_notifier_register(subdev, notifier); > + if (ret < 0) > + goto error_notifier; > + > + /* Subdev */ > + > + ret = v4l2_async_register_subdev(subdev); > + if (ret < 0) > + goto error_notifier_registered; > + > + return 0; > + > +error_notifier_registered: > + v4l2_async_notifier_unregister(notifier); > +error_notifier: > + v4l2_async_notifier_cleanup(notifier); > +error_media_entity: > + media_entity_cleanup(&subdev->entity); > + > + return ret; > +} regards, dan carpenter
On Fri, Oct 23, 2020 at 07:45:37PM +0200, Paul Kocialkowski wrote: > Bits related to the interface data width do not have any effect when > the CSI controller is taking input from the MIPI CSI-2 controller. I guess it would be clearer to mention that the data width is only applicable for parallel here. > In prevision of adding support for this case, set these bits > conditionally so there is no ambiguity. > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 42 +++++++++++-------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 5d2389a5cd17..a876a05ea3c7 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -378,8 +378,13 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > unsigned char bus_width; > u32 flags; > u32 cfg; > + bool input_parallel = false; > bool input_interlaced = false; > > + if (endpoint->bus_type == V4L2_MBUS_PARALLEL || > + endpoint->bus_type == V4L2_MBUS_BT656) > + input_parallel = true; > + > if (csi->config.field == V4L2_FIELD_INTERLACED > || csi->config.field == V4L2_FIELD_INTERLACED_TB > || csi->config.field == V4L2_FIELD_INTERLACED_BT) > @@ -395,6 +400,26 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > CSI_IF_CFG_HREF_POL_MASK | CSI_IF_CFG_FIELD_MASK | > CSI_IF_CFG_SRC_TYPE_MASK); > > + if (input_parallel) { > + switch (bus_width) { > + case 8: > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > + break; > + case 10: > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > + break; > + case 12: > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > + break; > + case 16: /* No need to configure DATA_WIDTH for 16bit */ > + break; > + default: > + dev_warn(sdev->dev, "Unsupported bus width: %u\n", > + bus_width); > + break; > + } > + } > + > if (input_interlaced) > cfg |= CSI_IF_CFG_SRC_TYPE_INTERLACED; > else > @@ -440,23 +465,6 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > break; > } > > - switch (bus_width) { > - case 8: > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > - break; > - case 10: > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > - break; > - case 12: > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > - break; > - case 16: /* No need to configure DATA_WIDTH for 16bit */ > - break; > - default: > - dev_warn(sdev->dev, "Unsupported bus width: %u\n", bus_width); > - break; > - } > - Is there any reason to move it around? Maxime
On Fri, Oct 23, 2020 at 07:45:42PM +0200, Paul Kocialkowski wrote: > MIPI CSI-2 is supported on the V3s with an A31 controller, which seems > to be used on all Allwinner chips supporting it, except for the A83T. > The controller is connected to CSI0 through fwnode endpoints. > The mipi_csi2_in port node is provided to connect MIPI CSI-2 sensors. > > The D-PHY part is the same that already drives DSI, but used in Rx mode. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Especially since you haven't tested CSI0 without MIPI-CSI, you can squash that patch into the previous one. Maxime
On Fri, Oct 23, 2020 at 07:45:43PM +0200, Paul Kocialkowski wrote: > This introduces YAML bindings documentation for the A83T MIPI CSI-2 > controller. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> What is the difference with the a31/v3s one? > --- > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 ++++++++++++++++++ > 1 file changed, 158 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..2384ae4e7be0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > @@ -0,0 +1,158 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%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 If it's only due to the clock, it's soemething you can deal with in the first schema, there's no need to duplicate them. Maxime
On Fri, Oct 23, 2020 at 07:45:46PM +0200, Paul Kocialkowski wrote: > The A83T MIPI CSI-2 apparently produces interrupts regardless of the mask > registers, for example when a transmission error occurs. > > This generates quite a flood when unsolicited interrupts are received on > each received frame. As a result, disable the interrupt for now since > we are not currently using it for error reporting. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> This should be merged into the driver patch Maxime
On Fri, Oct 23, 2020 at 07:45:44PM +0200, Paul Kocialkowski wrote: > The A83T supports MIPI CSI-2 with a composite controller, covering both the > protocol logic and the D-PHY implementation. This controller seems to be found > on the A83T only and probably was abandonned since. > > This implementation splits the protocol and D-PHY registers and uses the PHY > framework internally. The D-PHY is not registered as a standalone PHY driver > since it cannot be used with any other controller. > > There are a few notable points about the controller: > - The initialisation sequence involes writing specific magic init values that > do not seem to make any particular sense given the concerned register fields. > - Interrupts appear to be hitting regardless of the interrupt mask registers, > which can cause a serious flood when transmission errors occur. Ah, so it's a separate driver too. > This work is based on the first version of the driver submitted by > Kévin L'hôpital, which was adapted to mainline from the Allwinner BSP. > This version integrates MIPI CSI-2 support as a standalone V4L2 subdev > instead of merging it in the sun6i-csi driver. > > It was tested on a Banana Pi M3 board with an OV8865 sensor in a 4-lane > configuration. Co-developped-by and SoB from Kevin? Looking at the driver, the same comments from the v3s apply there Maxime
Hi, On Mon 26 Oct 20, 17:00, Maxime Ripard wrote: > On Fri, Oct 23, 2020 at 07:45:37PM +0200, Paul Kocialkowski wrote: > > Bits related to the interface data width do not have any effect when > > the CSI controller is taking input from the MIPI CSI-2 controller. > > I guess it would be clearer to mention that the data width is only > applicable for parallel here. Understood, will change the wording in the next version. > > In prevision of adding support for this case, set these bits > > conditionally so there is no ambiguity. > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 42 +++++++++++-------- > > 1 file changed, 25 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > index 5d2389a5cd17..a876a05ea3c7 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > @@ -378,8 +378,13 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > unsigned char bus_width; > > u32 flags; > > u32 cfg; > > + bool input_parallel = false; > > bool input_interlaced = false; > > > > + if (endpoint->bus_type == V4L2_MBUS_PARALLEL || > > + endpoint->bus_type == V4L2_MBUS_BT656) > > + input_parallel = true; > > + > > if (csi->config.field == V4L2_FIELD_INTERLACED > > || csi->config.field == V4L2_FIELD_INTERLACED_TB > > || csi->config.field == V4L2_FIELD_INTERLACED_BT) > > @@ -395,6 +400,26 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > CSI_IF_CFG_HREF_POL_MASK | CSI_IF_CFG_FIELD_MASK | > > CSI_IF_CFG_SRC_TYPE_MASK); > > > > + if (input_parallel) { > > + switch (bus_width) { > > + case 8: > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > > + break; > > + case 10: > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > > + break; > > + case 12: > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > > + break; > > + case 16: /* No need to configure DATA_WIDTH for 16bit */ > > + break; > > + default: > > + dev_warn(sdev->dev, "Unsupported bus width: %u\n", > > + bus_width); > > + break; > > + } > > + } > > + > > if (input_interlaced) > > cfg |= CSI_IF_CFG_SRC_TYPE_INTERLACED; > > else > > @@ -440,23 +465,6 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > break; > > } > > > > - switch (bus_width) { > > - case 8: > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > > - break; > > - case 10: > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > > - break; > > - case 12: > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > > - break; > > - case 16: /* No need to configure DATA_WIDTH for 16bit */ > > - break; > > - default: > > - dev_warn(sdev->dev, "Unsupported bus width: %u\n", bus_width); > > - break; > > - } > > - > > Is there any reason to move it around? The main reason is cosmetics: input_parallel is introduced to match the already existing input_interlaced variable, so it made sense to me to have both of these conditionals one after the other instead of spreading them randomly. I can mention this in the commit log if you prefer.
On Tue, Oct 27, 2020 at 10:31:19AM +0100, Paul Kocialkowski wrote: > Hi, > > On Mon 26 Oct 20, 17:00, Maxime Ripard wrote: > > On Fri, Oct 23, 2020 at 07:45:37PM +0200, Paul Kocialkowski wrote: > > > Bits related to the interface data width do not have any effect when > > > the CSI controller is taking input from the MIPI CSI-2 controller. > > > > I guess it would be clearer to mention that the data width is only > > applicable for parallel here. > > Understood, will change the wording in the next version. > > > > In prevision of adding support for this case, set these bits > > > conditionally so there is no ambiguity. > > > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > --- > > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 42 +++++++++++-------- > > > 1 file changed, 25 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > index 5d2389a5cd17..a876a05ea3c7 100644 > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > @@ -378,8 +378,13 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > > unsigned char bus_width; > > > u32 flags; > > > u32 cfg; > > > + bool input_parallel = false; > > > bool input_interlaced = false; > > > > > > + if (endpoint->bus_type == V4L2_MBUS_PARALLEL || > > > + endpoint->bus_type == V4L2_MBUS_BT656) > > > + input_parallel = true; > > > + > > > if (csi->config.field == V4L2_FIELD_INTERLACED > > > || csi->config.field == V4L2_FIELD_INTERLACED_TB > > > || csi->config.field == V4L2_FIELD_INTERLACED_BT) > > > @@ -395,6 +400,26 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > > CSI_IF_CFG_HREF_POL_MASK | CSI_IF_CFG_FIELD_MASK | > > > CSI_IF_CFG_SRC_TYPE_MASK); > > > > > > + if (input_parallel) { > > > + switch (bus_width) { > > > + case 8: > > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > > > + break; > > > + case 10: > > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > > > + break; > > > + case 12: > > > + cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > > > + break; > > > + case 16: /* No need to configure DATA_WIDTH for 16bit */ > > > + break; > > > + default: > > > + dev_warn(sdev->dev, "Unsupported bus width: %u\n", > > > + bus_width); > > > + break; > > > + } > > > + } > > > + > > > if (input_interlaced) > > > cfg |= CSI_IF_CFG_SRC_TYPE_INTERLACED; > > > else > > > @@ -440,23 +465,6 @@ static void sun6i_csi_setup_bus(struct sun6i_csi_dev *sdev) > > > break; > > > } > > > > > > - switch (bus_width) { > > > - case 8: > > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT; > > > - break; > > > - case 10: > > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT; > > > - break; > > > - case 12: > > > - cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT; > > > - break; > > > - case 16: /* No need to configure DATA_WIDTH for 16bit */ > > > - break; > > > - default: > > > - dev_warn(sdev->dev, "Unsupported bus width: %u\n", bus_width); > > > - break; > > > - } > > > - > > > > Is there any reason to move it around? > > The main reason is cosmetics: input_parallel is introduced to match the already > existing input_interlaced variable, so it made sense to me to have both of these > conditionals one after the other instead of spreading them randomly. > > I can mention this in the commit log if you prefer. Yeah, that would great Maxime
Hi Paul, I have some comments through the series, I hope this helps. On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > This series introduces support for MIPI CSI-2, with the A31 controller that is > found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > controller. While the former uses the same MIPI D-PHY that is already supported > for DSI, the latter embeds its own D-PHY. > > In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) > and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. > This allows adding Rx support in the A31 D-PHY driver. > > A few changes and fixes are applied to the A31 CSI controller driver, in order > to support the MIPI CSI-2 use-case. > > Follows is the V4L2 device topology representing the interactions between > the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) > and the CSI controller: > - entity 1: sun6i-csi (1 pad, 1 link) > type Node subtype V4L flags 0 > device node name /dev/video0 > pad0: Sink > <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] > > - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) > type V4L2 subdev subtype Unknown flags 0 > pad0: Sink > <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] > pad1: Source > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > - entity 8: ov5648 0-0036 (1 pad, 1 link) > type V4L2 subdev subtype Sensor flags 0 > device node name /dev/v4l-subdev0 Question: I noticed is that sun6i-mipi-csi2 doesn't expose a node under /dev/, but the sensor exposes it. Probably because it uses V4L2_SUBDEV_FL_HAS_DEVNODE and sun6i-csi() calls v4l2_device_register_subdev_nodes(). I find this weird from a userspace pov, since usually we don't mix manual and auto propagation of the configs, so I started wondering if sun6i-csi driver should be calling v4l2_device_register_subdev_nodes() in the first place. Also, sun6i-csi doesn't seem to be used by any board dts (it's declared on the dtsi, but I didn't find any dts enabling it), so I wonder if it would be a bad thing if we update it. > pad0: Source > [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] If I understand correctly, this is very similar to ipu3: sensor->bus->dma_engine in the case of ipu3-cio2: sensor->ipu3-csi2->ipu3-cio2 in this case: ov5648->sun6i-mipi-csi2->sun6i-csi On thing that is confusing me is the name csi2 with csi (that makes me think of csi vesun6i-csirsion one, which is not the case), I would rename it to sun6i-video (or maybe it is just me who gets confused). I know this driver is already upstream and not part of this series, but on the other hand it doesn't seem to be used. On another note, I always wonder if we should expose the bus in the topology, I'm not sure if it provides any useful API or information for userspace, and you could have a cleaner code (maybe code could be under phy subsystem). But at the same time, it seems this is a pattern on v4l2. I'd like to hear what others think on the above. Regards, Helen > > Happy reviewing! > > Paul Kocialkowski (14): > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI > CSI-2 > media: sun6i-csi: Support an optional dedicated memory pool > media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer > formats > media: sun6i-csi: Only configure the interface data width for parallel > media: sun6i-csi: Support feeding from the MIPI CSI-2 controller > dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation > media: sunxi: Add support for the A31 MIPI CSI-2 controller > ARM: dts: sun8i: v3s: Add CSI0 camera interface node > ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes > dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation > media: sunxi: Add support for the A83T MIPI CSI-2 controller > ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node > media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) > interrupt > > .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 +++++ > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 +++++ > arch/arm/boot/dts/sun8i-a83t.dtsi | 26 + > arch/arm/boot/dts/sun8i-v3s.dtsi | 62 ++ > drivers/media/platform/sunxi/Kconfig | 2 + > drivers/media/platform/sunxi/Makefile | 2 + > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 +- > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +- > .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 11 + > .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 +++++++++++++++++ > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116 +++ > .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + > .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 92 +++ > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 ++ > .../sun8i_a83t_mipi_csi2.c | 660 ++++++++++++++++++ > .../sun8i_a83t_mipi_csi2.h | 196 ++++++ > drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 ++++- > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 +- > include/linux/phy/phy-mipi-dphy.h | 13 + > 21 files changed, 2408 insertions(+), 32 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/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 >
Hi Paul, On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > As some D-PHY controllers support both Rx and Tx mode, we need a way for > users to explicitly request one or the other. For instance, Rx mode can > be used along with MIPI CSI-2 while Tx mode can be used with MIPI DSI. > > Introduce new MIPI D-PHY PHY submodes to use with PHY_MODE_MIPI_DPHY. > The default (zero value) is kept to Tx so only the rkisp1 driver, which > uses D-PHY in Rx mode, needs to be adapted. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 ++- > include/linux/phy/phy-mipi-dphy.h | 13 +++++++++++++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c > index 6ec1e9816e9f..0afbce00121e 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-isp.c > +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c > @@ -902,7 +902,8 @@ static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp, > > phy_mipi_dphy_get_default_config(pixel_clock, isp->sink_fmt->bus_width, > sensor->lanes, cfg); > - phy_set_mode(sensor->dphy, PHY_MODE_MIPI_DPHY); > + phy_set_mode_ext(cdev->dphy, PHY_MODE_MIPI_DPHY, > + PHY_MIPI_DPHY_SUBMODE_RX);
Hi On Fri, Oct 30, 2020 at 07:44:28PM -0300, Helen Koike wrote: > On thing that is confusing me is the name csi2 with csi (that makes me > think of csi vesun6i-csirsion one, which is not the case), I would > rename it to sun6i-video (or maybe it is just me who gets confused). > > I know this driver is already upstream and not part of this series, > but on the other hand it doesn't seem to be used. It's definitely confusing but CSI is the name of the IP, but it supports more than just MIPI-CSI :) Maxime
Hi, On Mon 26 Oct 20, 17:56, Maxime Ripard wrote: > On Fri, Oct 23, 2020 at 07:45:43PM +0200, Paul Kocialkowski wrote: > > This introduces YAML bindings documentation for the A83T MIPI CSI-2 > > controller. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > What is the difference with the a31/v3s one? It's a different controller, not a variation of the A31 one. I'll rework the commit log to make this clearer. > > --- > > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 ++++++++++++++++++ > > 1 file changed, 158 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..2384ae4e7be0 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > > @@ -0,0 +1,158 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%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 > > If it's only due to the clock, it's soemething you can deal with in the > first schema, there's no need to duplicate them. It's a completely different controller so I don't think it makes sense to have a single schema for both. Even if the bindings look similar. Paul
Hi, On Mon 26 Oct 20, 18:00, Maxime Ripard wrote: > On Fri, Oct 23, 2020 at 07:45:44PM +0200, Paul Kocialkowski wrote: > > The A83T supports MIPI CSI-2 with a composite controller, covering both the > > protocol logic and the D-PHY implementation. This controller seems to be found > > on the A83T only and probably was abandonned since. > > > > This implementation splits the protocol and D-PHY registers and uses the PHY > > framework internally. The D-PHY is not registered as a standalone PHY driver > > since it cannot be used with any other controller. > > > > There are a few notable points about the controller: > > - The initialisation sequence involes writing specific magic init values that > > do not seem to make any particular sense given the concerned register fields. > > - Interrupts appear to be hitting regardless of the interrupt mask registers, > > which can cause a serious flood when transmission errors occur. > > Ah, so it's a separate driver too. > > > This work is based on the first version of the driver submitted by > > Kévin L'hôpital, which was adapted to mainline from the Allwinner BSP. > > This version integrates MIPI CSI-2 support as a standalone V4L2 subdev > > instead of merging it in the sun6i-csi driver. > > > > It was tested on a Banana Pi M3 board with an OV8865 sensor in a 4-lane > > configuration. > > Co-developped-by and SoB from Kevin? Not really. I wrote this driver from scratch and even significantly reworked the register descriptions to the point that I don't think it makes sense to consider that he's an author. For parts that can be considered a derivative work, copyright attribution was given in the header. Cheers, Paul > Looking at the driver, the same comments from the v3s apply there > > Maxime
Hi Helen, On Fri 30 Oct 20, 19:45, Helen Koike wrote: > Hi Paul, > > On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > > Both 10 and 12-bit Bayer formats are stored aligned as 16-bit values > > in memory, not unaligned 10 or 12 bits. > > > > Since the current code for retreiving the bpp is used only to > > calculate the memory storage size of the picture (which is what > > pixel formats describe, unlike media bus formats), fix it there. > > > > Fixes: 5cc7522d8965 ("media: sun6i: Add support for Allwinner CSI V3s") > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > index c626821aaedb..7f2be70ae641 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > @@ -86,7 +86,7 @@ void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr); > > */ > > void sun6i_csi_set_stream(struct sun6i_csi *csi, bool enable); > > > > -/* get bpp form v4l2 pixformat */ > > +/* get memory storage bpp from v4l2 pixformat */ > > static inline int sun6i_csi_get_bpp(unsigned int pixformat) > > { > > switch (pixformat) { > > @@ -96,15 +96,6 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat) > > case V4L2_PIX_FMT_SRGGB8: > > case V4L2_PIX_FMT_JPEG: > > return 8; > > - case V4L2_PIX_FMT_SBGGR10: > > - case V4L2_PIX_FMT_SGBRG10: > > - case V4L2_PIX_FMT_SGRBG10: > > - case V4L2_PIX_FMT_SRGGB10: > > - return 10; > > - case V4L2_PIX_FMT_SBGGR12: > > - case V4L2_PIX_FMT_SGBRG12: > > - case V4L2_PIX_FMT_SGRBG12: > > - case V4L2_PIX_FMT_SRGGB12: > > case V4L2_PIX_FMT_HM12: > > case V4L2_PIX_FMT_NV12: > > case V4L2_PIX_FMT_NV21: > > @@ -121,6 +112,15 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat) > > case V4L2_PIX_FMT_RGB565: > > case V4L2_PIX_FMT_RGB565X: > > return 16; > > + case V4L2_PIX_FMT_SBGGR10: > > + case V4L2_PIX_FMT_SGBRG10: > > + case V4L2_PIX_FMT_SGRBG10: > > + case V4L2_PIX_FMT_SRGGB10: > > + case V4L2_PIX_FMT_SBGGR12: > > + case V4L2_PIX_FMT_SGBRG12: > > + case V4L2_PIX_FMT_SGRBG12: > > + case V4L2_PIX_FMT_SRGGB12: > > + return 16; > > case V4L2_PIX_FMT_RGB24: > > case V4L2_PIX_FMT_BGR24: > > return 24; > > > > Instead of updating this table, how about using v4l2_format_info() instead? Yes that would be a very good thing to do indeed! Thanks, Paul
Hi Helen, On Fri 30 Oct 20, 19:44, Helen Koike wrote: > Hi Paul, > > I have some comments through the series, I hope this helps. Thanks for your comments :) > On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > > This series introduces support for MIPI CSI-2, with the A31 controller that is > > found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > > controller. While the former uses the same MIPI D-PHY that is already supported > > for DSI, the latter embeds its own D-PHY. > > > > In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) > > and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. > > This allows adding Rx support in the A31 D-PHY driver. > > > > A few changes and fixes are applied to the A31 CSI controller driver, in order > > to support the MIPI CSI-2 use-case. > > > > Follows is the V4L2 device topology representing the interactions between > > the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) > > and the CSI controller: > > - entity 1: sun6i-csi (1 pad, 1 link) > > type Node subtype V4L flags 0 > > device node name /dev/video0 > > pad0: Sink > > <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] > > > > - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) > > type V4L2 subdev subtype Unknown flags 0 > > pad0: Sink > > <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] > > pad1: Source > > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > > > - entity 8: ov5648 0-0036 (1 pad, 1 link) > > type V4L2 subdev subtype Sensor flags 0 > > device node name /dev/v4l-subdev0 > > Question: I noticed is that sun6i-mipi-csi2 doesn't expose a node under /dev/, but the sensor > exposes it. Probably because it uses V4L2_SUBDEV_FL_HAS_DEVNODE and sun6i-csi() calls > v4l2_device_register_subdev_nodes(). > > I find this weird from a userspace pov, since usually we don't mix manual and auto propagation > of the configs, so I started wondering if sun6i-csi driver should be calling > v4l2_device_register_subdev_nodes() in the first place. I must admit that I didn't really pay attention to that, but since sun6i-mipi-csi2 is basically a bridge driver, it doesn't make sense to apply manual configuration to it. It is actually designed to forward most subdev ops to its own subdev so configuring it manually would actually result in configuring the sensor. XXX > Also, sun6i-csi doesn't seem to be used by any board dts (it's declared on the dtsi, but I > didn't find any dts enabling it), so I wonder if it would be a bad thing if we update it. > > > pad0: Source > > [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > > -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] > > If I understand correctly, this is very similar to ipu3: > sensor->bus->dma_engine > > in the case of ipu3-cio2: > sensor->ipu3-csi2->ipu3-cio2 > > in this case: > ov5648->sun6i-mipi-csi2->sun6i-csi Yes this is the correct picture. > On thing that is confusing me is the name csi2 with csi (that makes me think of csi > version one, which is not the case), I would rename it to sun6i-video (or maybe > it is just me who gets confused). So the CSI name comes from the Allwinner litterature and implementation for that controller. Since it supports parallel input on its own, it does in fact support parallel CSI. The DMA engine part alone from that controller is also used for MIPI CSI-2, so in this case the name looses its relevance. > I know this driver is already upstream and not part of this series, but on the other hand it > doesn't seem to be used. Personally I don't find a rename to be necessary and while I agree that nothing would apparently prevent us from renaming it, I would prefer to keep the naming in line with Allwinner's litterature. > On another note, I always wonder if we should expose the bus in the topology, I'm not > sure if it provides any useful API or information for userspace, and you could have > a cleaner code (maybe code could be under phy subsystem). But at the same time, it > seems this is a pattern on v4l2. > > I'd like to hear what others think on the above. My view on this is that we are dealing with two distinct controllers here, one that acts as a DMA engine and one that acts as a bridge. As a result, two chained subdevs looks like the most appropriate representation to me. Using the PHY subsystem would probably be abusing the framework since the MIPI CSI-2 controller is not a PHY (and we do have a D-PHY driver for the D-PHY part that uses the PHY API already). So tl;dr I don't agree that it would be cleaner. Cheers, Paul > > Happy reviewing! > > > > Paul Kocialkowski (14): > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI > > CSI-2 > > media: sun6i-csi: Support an optional dedicated memory pool > > media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer > > formats > > media: sun6i-csi: Only configure the interface data width for parallel > > media: sun6i-csi: Support feeding from the MIPI CSI-2 controller > > dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation > > media: sunxi: Add support for the A31 MIPI CSI-2 controller > > ARM: dts: sun8i: v3s: Add CSI0 camera interface node > > ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes > > dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation > > media: sunxi: Add support for the A83T MIPI CSI-2 controller > > ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node > > media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) > > interrupt > > > > .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 +++++ > > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 +++++ > > arch/arm/boot/dts/sun8i-a83t.dtsi | 26 + > > arch/arm/boot/dts/sun8i-v3s.dtsi | 62 ++ > > drivers/media/platform/sunxi/Kconfig | 2 + > > drivers/media/platform/sunxi/Makefile | 2 + > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 +- > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +- > > .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 11 + > > .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + > > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 +++++++++++++++++ > > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116 +++ > > .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + > > .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + > > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 92 +++ > > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 ++ > > .../sun8i_a83t_mipi_csi2.c | 660 ++++++++++++++++++ > > .../sun8i_a83t_mipi_csi2.h | 196 ++++++ > > drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 ++++- > > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 +- > > include/linux/phy/phy-mipi-dphy.h | 13 + > > 21 files changed, 2408 insertions(+), 32 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/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 > >
Hi again, On Wed 04 Nov 20, 12:11, Paul Kocialkowski wrote: > Hi Helen, > > On Fri 30 Oct 20, 19:44, Helen Koike wrote: > > Hi Paul, > > > > I have some comments through the series, I hope this helps. > > Thanks for your comments :) > > > On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > > > This series introduces support for MIPI CSI-2, with the A31 controller that is > > > found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > > > controller. While the former uses the same MIPI D-PHY that is already supported > > > for DSI, the latter embeds its own D-PHY. > > > > > > In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) > > > and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. > > > This allows adding Rx support in the A31 D-PHY driver. > > > > > > A few changes and fixes are applied to the A31 CSI controller driver, in order > > > to support the MIPI CSI-2 use-case. > > > > > > Follows is the V4L2 device topology representing the interactions between > > > the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) > > > and the CSI controller: > > > - entity 1: sun6i-csi (1 pad, 1 link) > > > type Node subtype V4L flags 0 > > > device node name /dev/video0 > > > pad0: Sink > > > <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] > > > > > > - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) > > > type V4L2 subdev subtype Unknown flags 0 > > > pad0: Sink > > > <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] > > > pad1: Source > > > -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > > > > > > - entity 8: ov5648 0-0036 (1 pad, 1 link) > > > type V4L2 subdev subtype Sensor flags 0 > > > device node name /dev/v4l-subdev0 > > > > Question: I noticed is that sun6i-mipi-csi2 doesn't expose a node under /dev/, but the sensor > > exposes it. Probably because it uses V4L2_SUBDEV_FL_HAS_DEVNODE and sun6i-csi() calls > > v4l2_device_register_subdev_nodes(). > > > > I find this weird from a userspace pov, since usually we don't mix manual and auto propagation > > of the configs, so I started wondering if sun6i-csi driver should be calling > > v4l2_device_register_subdev_nodes() in the first place. > > I must admit that I didn't really pay attention to that, but since > sun6i-mipi-csi2 is basically a bridge driver, it doesn't make sense to apply > manual configuration to it. It is actually designed to forward most subdev ops > to its own subdev so configuring it manually would actually result in > configuring the sensor. > > XXX Hum, I meant to add something here and then forgot. I'm pretty new to auto vs manual propagation so I don't really have a clear opinion on this and whether we should consider removing the sensor /dev node as well. I'm satisfied with the way things are currently, but it might be due to my own ignorance. Paul > > Also, sun6i-csi doesn't seem to be used by any board dts (it's declared on the dtsi, but I > > didn't find any dts enabling it), so I wonder if it would be a bad thing if we update it. > > > > > pad0: Source > > > [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > > > -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] > > > > If I understand correctly, this is very similar to ipu3: > > sensor->bus->dma_engine > > > > in the case of ipu3-cio2: > > sensor->ipu3-csi2->ipu3-cio2 > > > > in this case: > > ov5648->sun6i-mipi-csi2->sun6i-csi > > Yes this is the correct picture. > > > On thing that is confusing me is the name csi2 with csi (that makes me think of csi > > version one, which is not the case), I would rename it to sun6i-video (or maybe > > it is just me who gets confused). > > So the CSI name comes from the Allwinner litterature and implementation for that > controller. Since it supports parallel input on its own, it does in fact support > parallel CSI. The DMA engine part alone from that controller is also used for > MIPI CSI-2, so in this case the name looses its relevance. > > > I know this driver is already upstream and not part of this series, but on the other hand it > > doesn't seem to be used. > > Personally I don't find a rename to be necessary and while I agree that > nothing would apparently prevent us from renaming it, I would prefer to keep > the naming in line with Allwinner's litterature. > > > On another note, I always wonder if we should expose the bus in the topology, I'm not > > sure if it provides any useful API or information for userspace, and you could have > > a cleaner code (maybe code could be under phy subsystem). But at the same time, it > > seems this is a pattern on v4l2. > > > > I'd like to hear what others think on the above. > > My view on this is that we are dealing with two distinct controllers here, > one that acts as a DMA engine and one that acts as a bridge. As a result, two > chained subdevs looks like the most appropriate representation to me. > > Using the PHY subsystem would probably be abusing the framework since the > MIPI CSI-2 controller is not a PHY (and we do have a D-PHY driver for the D-PHY > part that uses the PHY API already). > > So tl;dr I don't agree that it would be cleaner. > > Cheers, > > Paul > > > > Happy reviewing! > > > > > > Paul Kocialkowski (14): > > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI > > > CSI-2 > > > media: sun6i-csi: Support an optional dedicated memory pool > > > media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer > > > formats > > > media: sun6i-csi: Only configure the interface data width for parallel > > > media: sun6i-csi: Support feeding from the MIPI CSI-2 controller > > > dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation > > > media: sunxi: Add support for the A31 MIPI CSI-2 controller > > > ARM: dts: sun8i: v3s: Add CSI0 camera interface node > > > ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes > > > dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation > > > media: sunxi: Add support for the A83T MIPI CSI-2 controller > > > ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node > > > media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) > > > interrupt > > > > > > .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 +++++ > > > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 +++++ > > > arch/arm/boot/dts/sun8i-a83t.dtsi | 26 + > > > arch/arm/boot/dts/sun8i-v3s.dtsi | 62 ++ > > > drivers/media/platform/sunxi/Kconfig | 2 + > > > drivers/media/platform/sunxi/Makefile | 2 + > > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 +- > > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +- > > > .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 11 + > > > .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + > > > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 +++++++++++++++++ > > > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116 +++ > > > .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + > > > .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + > > > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 92 +++ > > > .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 ++ > > > .../sun8i_a83t_mipi_csi2.c | 660 ++++++++++++++++++ > > > .../sun8i_a83t_mipi_csi2.h | 196 ++++++ > > > drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 ++++- > > > drivers/staging/media/rkisp1/rkisp1-isp.c | 3 +- > > > include/linux/phy/phy-mipi-dphy.h | 13 + > > > 21 files changed, 2408 insertions(+), 32 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/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 > > > > > -- > Paul Kocialkowski, Bootlin > Embedded Linux and kernel engineering > https://bootlin.com
Hi Paul, On Fri, Oct 23, 2020 at 07:45:43PM +0200, Paul Kocialkowski wrote: > This introduces YAML bindings documentation for the A83T MIPI CSI-2 > controller. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 ++++++++++++++++++ > 1 file changed, 158 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..2384ae4e7be0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/allwinner,sun8i-a83t-mipi-csi2.yaml > @@ -0,0 +1,158 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%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 > + > + # See ./video-interfaces.txt for details > + ports: > + type: object > + > + properties: > + port@0: > + type: object > + description: Input port, connect to a MIPI CSI-2 sensor > + > + properties: > + reg: > + const: 0 > + > + endpoint: > + type: object > + > + properties: > + remote-endpoint: true > + > + bus-type: > + const: 4 Again, if this is D-PHY only, you can remove this. > + > + clock-lanes: > + maxItems: 1 > + > + data-lanes: > + minItems: 1 > + maxItems: 4 Does the device support lane reordering? If not, you can remove clock-lanes. > + > + required: > + - bus-type > + - data-lanes > + - remote-endpoint > + > + additionalProperties: false > + > + required: > + - endpoint > + > + additionalProperties: false > + > + port@1: > + type: object > + description: Output port, connect to a CSI controller > + > + properties: > + reg: > + const: 1 > + > + endpoint: > + type: object > + > + properties: > + remote-endpoint: true > + > + bus-type: > + const: 4 Is it a MIPI CSI-2 D-PHY -> MIPI CSI-2 D-PHY device? I call that "cable". :-) > + > + additionalProperties: false > + > + required: > + - endpoint > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - resets > + > +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: mipi-csi2@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 { > + bus-type = <4>; /* MIPI CSI-2 D-PHY */ > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + > + remote-endpoint = <&ov8865_out_mipi_csi2>; > + }; > + }; > + > + mipi_csi2_out: port@1 { > + reg = <1>; > + > + mipi_csi2_out_csi: endpoint { > + bus-type = <4>; /* MIPI CSI-2 D-PHY */ > + remote-endpoint = <&csi_in_mipi_csi2>; > + }; > + }; > + }; > + }; > + > +...
Hi, On Wed 04 Nov 20, 13:36, Helen Koike wrote: > Hi Paul, > > On 11/4/20 8:11 AM, Paul Kocialkowski wrote: > > Hi Helen, > > > > On Fri 30 Oct 20, 19:44, Helen Koike wrote: > >> Hi Paul, > >> > >> I have some comments through the series, I hope this helps. > > > > Thanks for your comments :) > > > >> On 10/23/20 2:45 PM, Paul Kocialkowski wrote: > >>> This series introduces support for MIPI CSI-2, with the A31 controller that is > >>> found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific > >>> controller. While the former uses the same MIPI D-PHY that is already supported > >>> for DSI, the latter embeds its own D-PHY. > >>> > >>> In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) > >>> and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. > >>> This allows adding Rx support in the A31 D-PHY driver. > >>> > >>> A few changes and fixes are applied to the A31 CSI controller driver, in order > >>> to support the MIPI CSI-2 use-case. > >>> > >>> Follows is the V4L2 device topology representing the interactions between > >>> the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) > >>> and the CSI controller: > >>> - entity 1: sun6i-csi (1 pad, 1 link) > >>> type Node subtype V4L flags 0 > >>> device node name /dev/video0 > >>> pad0: Sink > >>> <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] > >>> > >>> - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) > >>> type V4L2 subdev subtype Unknown flags 0 > >>> pad0: Sink > >>> <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] > >>> pad1: Source > >>> -> "sun6i-csi":0 [ENABLED,IMMUTABLE] > >>> > >>> - entity 8: ov5648 0-0036 (1 pad, 1 link) > >>> type V4L2 subdev subtype Sensor flags 0 > >>> device node name /dev/v4l-subdev0 > >> > >> Question: I noticed is that sun6i-mipi-csi2 doesn't expose a node under /dev/, but the sensor > >> exposes it. Probably because it uses V4L2_SUBDEV_FL_HAS_DEVNODE and sun6i-csi() calls > >> v4l2_device_register_subdev_nodes(). > >> > >> I find this weird from a userspace pov, since usually we don't mix manual and auto propagation > >> of the configs, so I started wondering if sun6i-csi driver should be calling > >> v4l2_device_register_subdev_nodes() in the first place. > > > > I must admit that I didn't really pay attention to that, but since > > sun6i-mipi-csi2 is basically a bridge driver, it doesn't make sense to apply > > manual configuration to it. It is actually designed to forward most subdev ops > > to its own subdev so configuring it manually would actually result in > > configuring the sensor. > > Ack, then maybe sun6i-csi needs a patch removing the call to v4l2_device_register_subdev_nodes() Apparently Sakari suggested that we do need a subdev node for the MIPI CSI-2 bridge so I'll just do that. This implementation that fowards the ops to the sensor was apparently a mistake. Paul > > > > XXX > > > >> Also, sun6i-csi doesn't seem to be used by any board dts (it's declared on the dtsi, but I > >> didn't find any dts enabling it), so I wonder if it would be a bad thing if we update it. > >> > >>> pad0: Source > >>> [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] > >>> -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] > >> > >> If I understand correctly, this is very similar to ipu3: > >> sensor->bus->dma_engine > >> > >> in the case of ipu3-cio2: > >> sensor->ipu3-csi2->ipu3-cio2 > >> > >> in this case: > >> ov5648->sun6i-mipi-csi2->sun6i-csi > > > > Yes this is the correct picture. > > > >> On thing that is confusing me is the name csi2 with csi (that makes me think of csi > >> version one, which is not the case), I would rename it to sun6i-video (or maybe > >> it is just me who gets confused). > > > > So the CSI name comes from the Allwinner litterature and implementation for that > > controller. Since it supports parallel input on its own, it does in fact support > > parallel CSI. The DMA engine part alone from that controller is also used for > > MIPI CSI-2, so in this case the name looses its relevance. > > > >> I know this driver is already upstream and not part of this series, but on the other hand it > >> doesn't seem to be used. > > > > Personally I don't find a rename to be necessary and while I agree that > > nothing would apparently prevent us from renaming it, I would prefer to keep > > the naming in line with Allwinner's litterature. > > Ok, I didn't know it was from Allwinner's litterature, I don't mind keeping the name. Great :) > >> On another note, I always wonder if we should expose the bus in the topology, I'm not > >> sure if it provides any useful API or information for userspace, and you could have > >> a cleaner code (maybe code could be under phy subsystem). But at the same time, it > >> seems this is a pattern on v4l2. > >> > >> I'd like to hear what others think on the above. > > > > My view on this is that we are dealing with two distinct controllers here, > > one that acts as a DMA engine and one that acts as a bridge. As a result, two > > chained subdevs looks like the most appropriate representation to me. > > > > Using the PHY subsystem would probably be abusing the framework since the > > MIPI CSI-2 controller is not a PHY (and we do have a D-PHY driver for the D-PHY > > part that uses the PHY API already). > > > > So tl;dr I don't agree that it would be cleaner. > > My point is, this is a "dummy" subdevice in userspace pov, > but if it is only used with auto-propagation of the configurations, then > it doesn't matter (since userspace won't interact with that node). > And in the kernel space you need to implement media boilerplate code. > So I was trying to think in another alternative, but tbh I don't mind > keeping it in the media topology. Undestood and thanks for sharing your thoughts either way, they are definitely welcome! Cheers, Paul > Regards, > Helen > > > > > Cheers, > > > > Paul > > > >>> Happy reviewing! > >>> > >>> Paul Kocialkowski (14): > >>> phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > >>> phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI > >>> CSI-2 > >>> media: sun6i-csi: Support an optional dedicated memory pool > >>> media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer > >>> formats > >>> media: sun6i-csi: Only configure the interface data width for parallel > >>> media: sun6i-csi: Support feeding from the MIPI CSI-2 controller > >>> dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation > >>> media: sunxi: Add support for the A31 MIPI CSI-2 controller > >>> ARM: dts: sun8i: v3s: Add CSI0 camera interface node > >>> ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes > >>> dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation > >>> media: sunxi: Add support for the A83T MIPI CSI-2 controller > >>> ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node > >>> media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) > >>> interrupt > >>> > >>> .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 +++++ > >>> .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 +++++ > >>> arch/arm/boot/dts/sun8i-a83t.dtsi | 26 + > >>> arch/arm/boot/dts/sun8i-v3s.dtsi | 62 ++ > >>> drivers/media/platform/sunxi/Kconfig | 2 + > >>> drivers/media/platform/sunxi/Makefile | 2 + > >>> .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 +- > >>> .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +- > >>> .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 11 + > >>> .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + > >>> .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 +++++++++++++++++ > >>> .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116 +++ > >>> .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + > >>> .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + > >>> .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 92 +++ > >>> .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 ++ > >>> .../sun8i_a83t_mipi_csi2.c | 660 ++++++++++++++++++ > >>> .../sun8i_a83t_mipi_csi2.h | 196 ++++++ > >>> drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 ++++- > >>> drivers/staging/media/rkisp1/rkisp1-isp.c | 3 +- > >>> include/linux/phy/phy-mipi-dphy.h | 13 + > >>> 21 files changed, 2408 insertions(+), 32 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/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 > >>> > >
This series introduces support for MIPI CSI-2, with the A31 controller that is found on most SoCs (A31, V3s and probably V5) as well as the A83T-specific controller. While the former uses the same MIPI D-PHY that is already supported for DSI, the latter embeds its own D-PHY. In order to distinguish the use of the D-PHY between Rx mode (for MIPI CSI-2) and Tx mode (for MIPI DSI), a submode is introduced for D-PHY in the PHY API. This allows adding Rx support in the A31 D-PHY driver. A few changes and fixes are applied to the A31 CSI controller driver, in order to support the MIPI CSI-2 use-case. Follows is the V4L2 device topology representing the interactions between the MIPI CSI-2 sensor, the MIPI CSI-2 controller (which controls the D-PHY) and the CSI controller: - entity 1: sun6i-csi (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "sun6i-mipi-csi2":1 [ENABLED,IMMUTABLE] - entity 5: sun6i-mipi-csi2 (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 pad0: Sink <- "ov5648 0-0036":0 [ENABLED,IMMUTABLE] pad1: Source -> "sun6i-csi":0 [ENABLED,IMMUTABLE] - entity 8: ov5648 0-0036 (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev0 pad0: Source [fmt:SBGGR8_1X8/640x480@1/30 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range] -> "sun6i-mipi-csi2":0 [ENABLED,IMMUTABLE] Happy reviewing! Paul Kocialkowski (14): phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 media: sun6i-csi: Support an optional dedicated memory pool media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer formats media: sun6i-csi: Only configure the interface data width for parallel media: sun6i-csi: Support feeding from the MIPI CSI-2 controller dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation media: sunxi: Add support for the A31 MIPI CSI-2 controller ARM: dts: sun8i: v3s: Add CSI0 camera interface node ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation media: sunxi: Add support for the A83T MIPI CSI-2 controller ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) interrupt .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 +++++ .../media/allwinner,sun8i-a83t-mipi-csi2.yaml | 158 +++++ arch/arm/boot/dts/sun8i-a83t.dtsi | 26 + arch/arm/boot/dts/sun8i-v3s.dtsi | 62 ++ drivers/media/platform/sunxi/Kconfig | 2 + drivers/media/platform/sunxi/Makefile | 2 + .../platform/sunxi/sun6i-csi/sun6i_csi.c | 54 +- .../platform/sunxi/sun6i-csi/sun6i_csi.h | 20 +- .../platform/sunxi/sun6i-mipi-csi2/Kconfig | 11 + .../platform/sunxi/sun6i-mipi-csi2/Makefile | 4 + .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 635 +++++++++++++++++ .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h | 116 +++ .../sunxi/sun8i-a83t-mipi-csi2/Kconfig | 11 + .../sunxi/sun8i-a83t-mipi-csi2/Makefile | 4 + .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.c | 92 +++ .../sun8i-a83t-mipi-csi2/sun8i_a83t_dphy.h | 39 ++ .../sun8i_a83t_mipi_csi2.c | 660 ++++++++++++++++++ .../sun8i_a83t_mipi_csi2.h | 196 ++++++ drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 ++++- drivers/staging/media/rkisp1/rkisp1-isp.c | 3 +- include/linux/phy/phy-mipi-dphy.h | 13 + 21 files changed, 2408 insertions(+), 32 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/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