On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the A31 MIPI CSI-2
> controller.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 ++++++++++++++++++
> 1 file changed, 168 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> new file mode 100644
> index 000000000000..9adc0bc27033
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: allwinner,sun6i-a31-mipi-csi2
> + - items:
> + - const: allwinner,sun8i-v3s-mipi-csi2
> + - const: allwinner,sun6i-a31-mipi-csi2
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: Bus Clock
> + - description: Module Clock
> +
> + clock-names:
> + items:
> + - const: bus
> + - const: mod
> +
> + phys:
> + items:
> + - description: MIPI D-PHY
> +
> + phy-names:
> + items:
> + - const: dphy
> +
> + 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
> +
> + clock-lanes:
> + maxItems: 1
> +
> + data-lanes:
> + minItems: 1
> + maxItems: 4
> +
> + 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
> +
> + 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-v3s-ccu.h>
> + #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> +
> + mipi_csi2: mipi-csi2@1cb1000 {

I agree with using 'csi' here as that is at least aligned with 'dsi'
meaning the host side of the protocol. We've not been consistent here...

> + compatible = "allwinner,sun8i-v3s-mipi-csi2",
> + "allwinner,sun6i-a31-mipi-csi2";
> + reg = <0x01cb1000 0x1000>;
> + interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_BUS_CSI>,
> + <&ccu CLK_CSI1_SCLK>;
> + clock-names = "bus", "mod";
> + resets = <&ccu RST_BUS_CSI>;
> +
> + phys = <&dphy>;
> + phy-names = "dphy";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mipi_csi2_in: port@0 {
> + reg = <0>;
> +
> + mipi_csi2_in_ov5648: endpoint {
> + bus-type = <4>; /* MIPI CSI-2 D-PHY */
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> +
> + remote-endpoint = <&ov5648_out_mipi_csi2>;
> + };
> + };
> +
> + mipi_csi2_out: port@1 {
> + reg = <1>;
> +
> + mipi_csi2_out_csi0: endpoint {
> + bus-type = <4>; /* MIPI CSI-2 D-PHY */
> + remote-endpoint = <&csi0_in_mipi_csi2>;
> + };
> + };
> + };
> + };
> +
> +...
> --
> 2.28.0
>

2020-10-30 17:12:06

by Sakari Ailus

[permalink] [raw]

Subject: Re: [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation

Hi Paul,

On Fri, Oct 23, 2020 at 07:45:39PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the A31 MIPI CSI-2
> controller.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> .../media/allwinner,sun6i-a31-mipi-csi2.yaml | 168 ++++++++++++++++++
> 1 file changed, 168 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> new file mode 100644
> index 000000000000..9adc0bc27033
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-mipi-csi2.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-mipi-csi2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner A31 MIPI CSI-2 Device Tree Bindings
> +
> +maintainers:
> + - Paul Kocialkowski <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - const: allwinner,sun6i-a31-mipi-csi2
> + - items:
> + - const: allwinner,sun8i-v3s-mipi-csi2
> + - const: allwinner,sun6i-a31-mipi-csi2
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: Bus Clock
> + - description: Module Clock
> +
> + clock-names:
> + items:
> + - const: bus
> + - const: mod
> +
> + phys:
> + items:
> + - description: MIPI D-PHY
> +
> + phy-names:
> + items:
> + - const: dphy
> +
> + 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
> +
> + clock-lanes:
> + maxItems: 1

You can drop bus-type and clock-lanes (assuming no lane remapping is
supported by the hardware).

> +
> + data-lanes:
> + minItems: 1
> + maxItems: 4
> +
> + 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

Same here.

> +
> + 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-v3s-ccu.h>
> + #include <dt-bindings/reset/sun8i-v3s-ccu.h>
> +
> + mipi_csi2: mipi-csi2@1cb1000 {
> + compatible = "allwinner,sun8i-v3s-mipi-csi2",
> + "allwinner,sun6i-a31-mipi-csi2";
> + reg = <0x01cb1000 0x1000>;
> + interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_BUS_CSI>,
> + <&ccu CLK_CSI1_SCLK>;
> + clock-names = "bus", "mod";
> + resets = <&ccu RST_BUS_CSI>;
> +
> + phys = <&dphy>;
> + phy-names = "dphy";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mipi_csi2_in: port@0 {
> + reg = <0>;
> +
> + mipi_csi2_in_ov5648: endpoint {
> + bus-type = <4>; /* MIPI CSI-2 D-PHY */
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> +
> + remote-endpoint = <&ov5648_out_mipi_csi2>;
> + };
> + };
> +
> + mipi_csi2_out: port@1 {
> + reg = <1>;
> +
> + mipi_csi2_out_csi0: endpoint {
> + bus-type = <4>; /* MIPI CSI-2 D-PHY */
> + remote-endpoint = <&csi0_in_mipi_csi2>;
> + };
> + };
> + };
> + };
> +
> +...

--
Kind regards,

Sakari Ailus

2020-10-30 22:47:02

by Helen Koike

[permalink] [raw]

Subject: Re: [PATCH 02/14] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2

Hello,

On 10/23/20 2:45 PM, Paul Kocialkowski wrote:
> The Allwinner A31 D-PHY supports both Rx and Tx modes. While the latter
> is already supported and used for MIPI DSI this adds support for the
> former, to be used with MIPI CSI-2.
>
> This implementation is inspired by the Allwinner BSP implementation.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 164 +++++++++++++++++++-
> 1 file changed, 160 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> index 1fa761ba6cbb..8bcd4bb79f60 100644
> --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> @@ -24,6 +24,14 @@
> #define SUN6I_DPHY_TX_CTL_REG 0x04
> #define SUN6I_DPHY_TX_CTL_HS_TX_CLK_CONT BIT(28)
>
> +#define SUN6I_DPHY_RX_CTL_REG 0x08
> +#define SUN6I_DPHY_RX_CTL_EN_DBC BIT(31)
> +#define SUN6I_DPHY_RX_CTL_RX_CLK_FORCE BIT(24)
> +#define SUN6I_DPHY_RX_CTL_RX_D3_FORCE BIT(23)
> +#define SUN6I_DPHY_RX_CTL_RX_D2_FORCE BIT(22)
> +#define SUN6I_DPHY_RX_CTL_RX_D1_FORCE BIT(21)
> +#define SUN6I_DPHY_RX_CTL_RX_D0_FORCE BIT(20)
> +
> #define SUN6I_DPHY_TX_TIME0_REG 0x10
> #define SUN6I_DPHY_TX_TIME0_HS_TRAIL(n) (((n) & 0xff) << 24)
> #define SUN6I_DPHY_TX_TIME0_HS_PREPARE(n) (((n) & 0xff) << 16)
> @@ -44,12 +52,29 @@
> #define SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(n) (((n) & 0xff) << 8)
> #define SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(n) ((n) & 0xff)
>
> +#define SUN6I_DPHY_RX_TIME0_REG 0x30
> +#define SUN6I_DPHY_RX_TIME0_HS_RX_SYNC(n) (((n) & 0xff) << 24)
> +#define SUN6I_DPHY_RX_TIME0_HS_RX_CLK_MISS(n) (((n) & 0xff) << 16)
> +#define SUN6I_DPHY_RX_TIME0_LP_RX(n) (((n) & 0xff) << 8)
> +
> +#define SUN6I_DPHY_RX_TIME1_REG 0x34
> +#define SUN6I_DPHY_RX_TIME1_RX_DLY(n) (((n) & 0xfff) << 20)
> +#define SUN6I_DPHY_RX_TIME1_LP_RX_ULPS_WP(n) ((n) & 0xfffff)
> +
> +#define SUN6I_DPHY_RX_TIME2_REG 0x38
> +#define SUN6I_DPHY_RX_TIME2_HS_RX_ANA1(n) (((n) & 0xff) << 8)
> +#define SUN6I_DPHY_RX_TIME2_HS_RX_ANA0(n) ((n) & 0xff)
> +
> +#define SUN6I_DPHY_RX_TIME3_REG 0x40
> +#define SUN6I_DPHY_RX_TIME3_LPRST_DLY(n) (((n) & 0xffff) << 16)
> +
> #define SUN6I_DPHY_ANA0_REG 0x4c
> #define SUN6I_DPHY_ANA0_REG_PWS BIT(31)
> #define SUN6I_DPHY_ANA0_REG_DMPC BIT(28)
> #define SUN6I_DPHY_ANA0_REG_DMPD(n) (((n) & 0xf) << 24)
> #define SUN6I_DPHY_ANA0_REG_SLV(n) (((n) & 7) << 12)
> #define SUN6I_DPHY_ANA0_REG_DEN(n) (((n) & 0xf) << 8)
> +#define SUN6I_DPHY_ANA0_REG_SFB(n) (((n) & 3) << 2)
>
> #define SUN6I_DPHY_ANA1_REG 0x50
> #define SUN6I_DPHY_ANA1_REG_VTTMODE BIT(31)
> @@ -92,6 +117,8 @@ struct sun6i_dphy {
>
> struct phy *phy;
> struct phy_configure_opts_mipi_dphy config;
> +
> + int submode;
> };
>
> static int sun6i_dphy_init(struct phy *phy)
> @@ -105,6 +132,18 @@ static int sun6i_dphy_init(struct phy *phy)
> return 0;
> }
>
> +static int sun6i_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> + struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> +
> + if (mode != PHY_MODE_MIPI_DPHY)
> + return -EINVAL;
> +
> + dphy->submode = submode;

Shouldn't you check if the submode is valid here?

> +
> + return 0;
> +}
> +
> static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> {
> struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> @@ -119,9 +158,8 @@ static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> return 0;
> }
>
> -static int sun6i_dphy_power_on(struct phy *phy)
> +static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy)
> {
> - struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
>
> regmap_write(dphy->regs, SUN6I_DPHY_TX_CTL_REG,
> @@ -211,12 +249,129 @@ static int sun6i_dphy_power_on(struct phy *phy)
> return 0;
> }
>
> +static int sun6i_dphy_rx_power_on(struct sun6i_dphy *dphy)
> +{
> + /* Physical clock rate is actually half of symbol rate with DDR. */
> + unsigned long mipi_symbol_rate = dphy->config.hs_clk_rate;
> + unsigned long dphy_clk_rate;
> + unsigned int rx_dly;
> + unsigned int lprst_dly;
> + u32 value;
> +
> + dphy_clk_rate = clk_get_rate(dphy->mod_clk);
> + if (!dphy_clk_rate)
> + return -1;
> +
> + /* Hardcoded timing parameters from the Allwinner BSP. */
> + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME0_REG,
> + SUN6I_DPHY_RX_TIME0_HS_RX_SYNC(255) |
> + SUN6I_DPHY_RX_TIME0_HS_RX_CLK_MISS(255) |
> + SUN6I_DPHY_RX_TIME0_LP_RX(255));
> +
> + /*
> + * Formula from the Allwinner BSP, with hardcoded coefficients
> + * (probably internal divider/multiplier).
> + */
> + rx_dly = 8 * (unsigned int)(dphy_clk_rate / (mipi_symbol_rate / 8));
> +
> + /*
> + * The Allwinner BSP has an alternative formula for LP_RX_ULPS_WP:
> + * lp_ulps_wp_cnt = lp_ulps_wp_ms * lp_clk / 1000
> + * but does not use it and hardcodes 255 instead.
> + */
> + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME1_REG,
> + SUN6I_DPHY_RX_TIME1_RX_DLY(rx_dly) |
> + SUN6I_DPHY_RX_TIME1_LP_RX_ULPS_WP(255));
> +
> + /* HS_RX_ANA0 value is hardcoded in the Allwinner BSP. */
> + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME2_REG,
> + SUN6I_DPHY_RX_TIME2_HS_RX_ANA0(4));
> +
> + /*
> + * Formula from the Allwinner BSP, with hardcoded coefficients
> + * (probably internal divider/multiplier).
> + */
> + lprst_dly = 4 * (unsigned int)(dphy_clk_rate / (mipi_symbol_rate / 2));
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_RX_TIME3_REG,
> + SUN6I_DPHY_RX_TIME3_LPRST_DLY(lprst_dly));
> +
> + /* Analog parameters are hardcoded in the Allwinner BSP. */
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG,
> + SUN6I_DPHY_ANA0_REG_PWS |
> + SUN6I_DPHY_ANA0_REG_SLV(7) |
> + SUN6I_DPHY_ANA0_REG_SFB(2));
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG,
> + SUN6I_DPHY_ANA1_REG_SVTT(4));
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG,
> + SUN6I_DPHY_ANA4_REG_DMPLVC |
> + SUN6I_DPHY_ANA4_REG_DMPLVD(1));
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA2_REG,
> + SUN6I_DPHY_ANA2_REG_ENIB);
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG,
> + SUN6I_DPHY_ANA3_EN_LDOR |
> + SUN6I_DPHY_ANA3_EN_LDOC |
> + SUN6I_DPHY_ANA3_EN_LDOD);
> +
> + /*
> + * Delay comes from the Allwinner BSP, likely for internal regulator
> + * ramp-up.
> + */
> + udelay(3);
> +
> + value = SUN6I_DPHY_RX_CTL_EN_DBC | SUN6I_DPHY_RX_CTL_RX_CLK_FORCE;
> +
> + /*
> + * Rx data lane force-enable bits are used as regular RX enable by the
> + * Allwinner BSP.
> + */
> + if (dphy->config.lanes >= 1)
> + value |= SUN6I_DPHY_RX_CTL_RX_D0_FORCE;
> + if (dphy->config.lanes >= 2)
> + value |= SUN6I_DPHY_RX_CTL_RX_D1_FORCE;
> + if (dphy->config.lanes >= 3)
> + value |= SUN6I_DPHY_RX_CTL_RX_D2_FORCE;
> + if (dphy->config.lanes == 4)
> + value |= SUN6I_DPHY_RX_CTL_RX_D3_FORCE;

I would replace this by a switch case with fallthrough to avoid too many comparisons
to the same value.

Regards,
Helen

> +
> + regmap_write(dphy->regs, SUN6I_DPHY_RX_CTL_REG, value);
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG,
> + SUN6I_DPHY_GCTL_LANE_NUM(dphy->config.lanes) |
> + SUN6I_DPHY_GCTL_EN);
> +
> + return 0;
> +}
> +
> +static int sun6i_dphy_power_on(struct phy *phy)
> +{
> + struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> +
> + switch (dphy->submode) {
> + case PHY_MIPI_DPHY_SUBMODE_TX:
> + return sun6i_dphy_tx_power_on(dphy);
> + case PHY_MIPI_DPHY_SUBMODE_RX:
> + return sun6i_dphy_rx_power_on(dphy);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int sun6i_dphy_power_off(struct phy *phy)
> {
> struct sun6i_dphy *dphy = phy_get_drvdata(phy);
>
> - regmap_update_bits(dphy->regs, SUN6I_DPHY_ANA1_REG,
> - SUN6I_DPHY_ANA1_REG_VTTMODE, 0);
> + regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, 0);
> +
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, 0);
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG, 0);
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA2_REG, 0);
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG, 0);
> + regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG, 0);
>
> return 0;
> }
> @@ -234,6 +389,7 @@ static int sun6i_dphy_exit(struct phy *phy)
>
>
> static const struct phy_ops sun6i_dphy_ops = {
> + .set_mode = sun6i_dphy_set_mode,
> .configure = sun6i_dphy_configure,
> .power_on = sun6i_dphy_power_on,
> .power_off = sun6i_dphy_power_off,
>

2020-10-30 22:48:16

by Helen Koike

[permalink] [raw]

Subject: Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller

Hi Paul,

On 10/23/20 2:45 PM, Paul Kocialkowski wrote:
> The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 controller
> found on Allwinner SoCs such as the A31 and V3/V3s.
>
> It is a standalone block, connected to the CSI controller on one side
> and to the MIPI D-PHY block on the other. It has a dedicated address
> space, interrupt line and clock.
>
> Currently, the MIPI CSI-2 controller is hard-tied to a specific CSI
> controller (CSI0) but newer SoCs (such as the V5) may allow switching
> MIPI CSI-2 controllers between CSI controllers.
>
> It is represented as a V4L2 subdev to the CSI controller and takes a
> MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
> media controller API.

Maybe this is a bad idea, but I was thinking:
This driver basically just turn on/off and catch some interrupts for errors,
and all the rest of v4l2 config you just forward to the next subdevice
on the pipeline.

So instead of exposing it as a subdevice, I was wondering if modeling
this driver also through the phy subsystem wouldn't be cleaner, so
you won't need all the v4l2 subdevice/topology boilerplate code that
it seems you are not using (unless you have plans to add controls or
some specific configuration on this node later).

But this would require changes on the sun6i-csi driver.

What do you think?

Same comment for patch 12/14 (A83T MIPI CSI-2 controller).

>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/media/platform/sunxi/Kconfig | 1 +
> drivers/media/platform/sunxi/Makefile | 1 +
> .../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 ++++
> 6 files changed, 768 insertions(+)
> 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
>
> diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
> index 7151cc249afa..9684e07454ad 100644
> --- a/drivers/media/platform/sunxi/Kconfig
> +++ b/drivers/media/platform/sunxi/Kconfig
> @@ -2,3 +2,4 @@
>
> source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
> source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> +source "drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig"
> diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
> index fc537c9f5ca9..887a7cae8fca 100644
> --- a/drivers/media/platform/sunxi/Makefile
> +++ b/drivers/media/platform/sunxi/Makefile
> @@ -2,5 +2,6 @@
>
> obj-y += sun4i-csi/
> obj-y += sun6i-csi/
> +obj-y += sun6i-mipi-csi2/
> obj-y += sun8i-di/
> obj-y += sun8i-rotate/
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> new file mode 100644
> index 000000000000..7033bda483b4
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VIDEO_SUN6I_MIPI_CSI2
> + tristate "Allwinner A31 MIPI CSI-2 Controller Driver"
> + depends on VIDEO_V4L2 && COMMON_CLK
> + depends on ARCH_SUNXI || COMPILE_TEST
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select REGMAP_MMIO
> + select V4L2_FWNODE
> + help
> + Support for the Allwinner A31 MIPI CSI-2 Controller.
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> new file mode 100644
> index 000000000000..14e4e03818b5
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +sun6i-mipi-csi2-y += sun6i_mipi_csi2.o
> +
> +obj-$(CONFIG_VIDEO_SUN6I_MIPI_CSI2) += sun6i-mipi-csi2.o
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> new file mode 100644
> index 000000000000..ce89c35f5b86
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> @@ -0,0 +1,635 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2020 Bootlin
> + * Author: Paul Kocialkowski <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "sun6i_mipi_csi2.h"
> +
> +#define MODULE_NAME "sun6i-mipi-csi2"
> +
> +/* Core */
> +
> +static irqreturn_t sun6i_mipi_csi2_isr(int irq, void *dev_id)
> +{
> + struct sun6i_mipi_csi2_dev *cdev = (struct sun6i_mipi_csi2_dev *)dev_id;
> + struct regmap *regmap = cdev->regmap;
> + u32 pending;
> +
> + WARN_ONCE(1, MODULE_NAME
> + ": Unsolicited interrupt, an error likely occurred!\n");
> +
> + regmap_read(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, &pending);
> + regmap_write(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, pending);
> +
> + /*
> + * The interrupt can be used to catch transmission errors.
> + * However, we currently lack plumbing for reporting that to the
> + * A31 CSI controller driver.
> + */
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sun6i_mipi_csi2_s_power(struct v4l2_subdev *subdev, int on)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> + int ret;
> +
> + if (!on) {
> + clk_disable_unprepare(cdev->clk_mod);
> + reset_control_assert(cdev->reset);
> +
> + return 0;
> + }
> +
> + ret = clk_prepare_enable(cdev->clk_mod);
> + if (ret) {
> + dev_err(cdev->dev, "failed to enable module clock\n");
> + return ret;
> + }
> +
> + ret = reset_control_deassert(cdev->reset);
> + if (ret) {
> + dev_err(cdev->dev, "failed to deassert reset\n");
> + goto error_clk;
> + }
> +
> + return 0;
> +
> +error_clk:
> + clk_disable_unprepare(cdev->clk_mod);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops sun6i_mipi_csi2_subdev_core_ops = {
> + .s_power = sun6i_mipi_csi2_s_power,
> +};
> +
> +/* Video */
> +
> +static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> + struct v4l2_subdev *remote_subdev = video->remote_subdev;
> + struct v4l2_fwnode_bus_mipi_csi2 *bus_mipi_csi2 =
> + &video->endpoint.bus.mipi_csi2;
> + union phy_configure_opts dphy_opts = { 0 };
> + struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
> + struct regmap *regmap = cdev->regmap;
> + struct v4l2_ctrl *ctrl;
> + unsigned int lanes_count;
> + unsigned int bpp;
> + unsigned long pixel_rate;
> + u8 data_type = 0;
> + u32 version = 0;
> + /* Initialize to 0 to use both in disable label (ret != 0) and off. */
> + int ret = 0;
> +
> + if (!remote_subdev)
> + return -ENODEV;
> +
> + if (!on) {
> + v4l2_subdev_call(remote_subdev, video, s_stream, 0);
> +
> +disable:
> + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> + SUN6I_MIPI_CSI2_CTL_EN, 0);
> +
> + phy_power_off(cdev->dphy);
> +
> + return ret;
> + }
> +
> + switch (video->mbus_code) {
> + case MEDIA_BUS_FMT_SBGGR8_1X8:
> + case MEDIA_BUS_FMT_SGBRG8_1X8:
> + case MEDIA_BUS_FMT_SGRBG8_1X8:
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + data_type = MIPI_CSI2_DATA_TYPE_RAW8;
> + bpp = 8;
> + break;
> + case MEDIA_BUS_FMT_SBGGR10_1X10:
> + case MEDIA_BUS_FMT_SGBRG10_1X10:
> + case MEDIA_BUS_FMT_SGRBG10_1X10:
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + data_type = MIPI_CSI2_DATA_TYPE_RAW10;
> + bpp = 10;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /* Sensor pixel rate */
> +
> + ctrl = v4l2_ctrl_find(remote_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> + if (!ctrl) {
> + dev_err(cdev->dev,
> + "%s: no MIPI CSI-2 pixel rate from the sensor\n",
> + __func__);
> + return -ENODEV;
> + }
> +
> + pixel_rate = (unsigned long)v4l2_ctrl_g_ctrl_int64(ctrl);
> + if (!pixel_rate) {
> + dev_err(cdev->dev,
> + "%s: zero MIPI CSI-2 pixel rate from the sensor\n",
> + __func__);
> + return -ENODEV;
> + }
> +
> + /* D-PHY configuration */
> +
> + lanes_count = bus_mipi_csi2->num_data_lanes;
> + phy_mipi_dphy_get_default_config(pixel_rate, bpp, lanes_count,
> + dphy_cfg);
> +
> +
> + /*
> + * Note that our hardware is using DDR, which is not taken in account by
> + * phy_mipi_dphy_get_default_config when calculating hs_clk_rate from
> + * the pixel rate, lanes count and bpp.
> + *
> + * The resulting clock rate is basically the symbol rate over the whole
> + * link. The actual clock rate is calculated with division by two since
> + * DDR samples both on rising and falling edges.
> + */
> +
> + dev_dbg(cdev->dev, "A31 MIPI CSI-2 config:\n");
> + dev_dbg(cdev->dev, "%ld pixels/s, %u bits/pixel, %lu Hz clock\n",
> + pixel_rate, bpp, dphy_cfg->hs_clk_rate / 2);
> +
> + ret = 0;
> + ret |= phy_reset(cdev->dphy);
> + ret |= phy_set_mode_ext(cdev->dphy, PHY_MODE_MIPI_DPHY,
> + PHY_MIPI_DPHY_SUBMODE_RX);
> + ret |= phy_configure(cdev->dphy, &dphy_opts);
> +
> + if (ret) {
> + dev_err(cdev->dev, "failed to setup MIPI D-PHY\n");
> + return ret;
> + }
> +
> + ret = phy_power_on(cdev->dphy);
> + if (ret) {
> + dev_err(cdev->dev, "failed to power on MIPI D-PHY\n");
> + return ret;
> + }
> +
> + /* MIPI CSI-2 controller setup */
> +
> + /*
> + * The enable flow in the Allwinner BSP is a bit different: the enable
> + * and reset bits are set together before starting the CSI controller.
> + *
> + * In mainline we enable the CSI controller first (due to subdev logic).
> + * One reliable way to make this work is to deassert reset, configure
> + * registers and enable the controller when everything's ready.
> + *
> + * However, reading the version appears necessary for it to work
> + * reliably. Replacing it with a delay doesn't do the trick.
> + */
> + regmap_write(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> + SUN6I_MIPI_CSI2_CTL_RESET_N |
> + SUN6I_MIPI_CSI2_CTL_VERSION_EN |
> + SUN6I_MIPI_CSI2_CTL_UNPK_EN);
> +
> + regmap_read(regmap, SUN6I_MIPI_CSI2_VERSION_REG, &version);
> +
> + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> + SUN6I_MIPI_CSI2_CTL_VERSION_EN, 0);
> +
> + dev_dbg(cdev->dev, "A31 MIPI CSI-2 version: %04x\n", version);
> +
> + regmap_write(regmap, SUN6I_MIPI_CSI2_CFG_REG,
> + SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(1) |
> + SUN6I_MIPI_CSI2_CFG_LANE_COUNT(lanes_count));
> +
> + regmap_write(regmap, SUN6I_MIPI_CSI2_VCDT_RX_REG,
> + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(3, 3) |
> + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(2, 2) |
> + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(1, 1) |
> + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(0, 0) |
> + SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(0, data_type));
> +
> + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> + SUN6I_MIPI_CSI2_CTL_EN, SUN6I_MIPI_CSI2_CTL_EN);
> +
> + ret = v4l2_subdev_call(remote_subdev, video, s_stream, 1);
> + if (ret)
> + goto disable;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops sun6i_mipi_csi2_subdev_video_ops = {
> + .s_stream = sun6i_mipi_csi2_s_stream,
> +};
> +
> +/* Pad */
> +
> +static int sun6i_mipi_csi2_enum_mbus_code(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *config,
> + struct v4l2_subdev_mbus_code_enum *code_enum)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> +
> + if (!video->remote_subdev)
> + return -ENODEV;
> +
> + /* Forward to the sensor. */
> + code_enum->pad = video->remote_pad_index;
> +
> + return v4l2_subdev_call(video->remote_subdev, pad, enum_mbus_code,
> + config, code_enum);
> +}
> +
> +static int sun6i_mipi_csi2_get_fmt(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *config,
> + struct v4l2_subdev_format *format)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> +
> + if (!video->remote_subdev)
> + return -ENODEV;
> +
> + /* Forward to the sensor. */
> + format->pad = video->remote_pad_index;
> +
> + return v4l2_subdev_call(video->remote_subdev, pad, get_fmt, config,
> + format);
> +}
> +
> +static int sun6i_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *config,
> + struct v4l2_subdev_format *format)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> +
> + if (!video->remote_subdev)
> + return -ENODEV;
> +
> + /* Forward to the sensor. */
> + format->pad = video->remote_pad_index;
> +
> + return v4l2_subdev_call(video->remote_subdev, pad, set_fmt, config,
> + format);

I see v4l2_subdev_call() being called in the remote subdevice over this driver,
but from what I understand, we should leave to the driver that decides to call
v4l2_device_register_subdev_nodes() or not, to call these operations
over the topology (in this case, sun6i_csi driver).

Same comment for patch 12/14 (A83T MIPI CSI-2 controller).

Regards,
Helen

> +}
> +
> +static int sun6i_mipi_csi2_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *config,
> + struct v4l2_subdev_frame_size_enum *size_enum)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> +
> + if (!video->remote_subdev)
> + return -ENODEV;
> +
> + /* Forward to the sensor. */
> + size_enum->pad = video->remote_pad_index;
> +
> + return v4l2_subdev_call(video->remote_subdev, pad, enum_frame_size,
> + config, size_enum);
> +}
> +
> +static int sun6i_mipi_csi2_enum_frame_interval(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *config,
> + struct v4l2_subdev_frame_interval_enum *interval_enum)
> +{
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> +
> + if (!video->remote_subdev)
> + return -ENODEV;
> +
> + /* Forward to the sensor. */
> + interval_enum->pad = video->remote_pad_index;
> +
> + return v4l2_subdev_call(video->remote_subdev, pad, enum_frame_interval,
> + config, interval_enum);
> +}
> +
> +static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_subdev_pad_ops = {
> + .enum_mbus_code = sun6i_mipi_csi2_enum_mbus_code,
> + .get_fmt = sun6i_mipi_csi2_get_fmt,
> + .set_fmt = sun6i_mipi_csi2_set_fmt,
> + .enum_frame_size = sun6i_mipi_csi2_enum_frame_size,
> + .enum_frame_interval = sun6i_mipi_csi2_enum_frame_interval,
> +};
> +
> +/* Subdev */
> +
> +static const struct v4l2_subdev_ops sun6i_mipi_csi2_subdev_ops = {
> + .core = &sun6i_mipi_csi2_subdev_core_ops,
> + .video = &sun6i_mipi_csi2_subdev_video_ops,
> + .pad = &sun6i_mipi_csi2_subdev_pad_ops,
> +};
> +
> +/* Notifier */
> +
> +static int sun6i_mipi_csi2_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *remote_subdev,
> + struct v4l2_async_subdev *remote_subdev_async)
> +{
> + struct v4l2_subdev *subdev = notifier->sd;
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> + int source_pad;
> + int ret;
> +
> + source_pad = media_entity_get_fwnode_pad(&remote_subdev->entity,
> + remote_subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (source_pad < 0)
> + return source_pad;
> +
> + ret = media_create_pad_link(&remote_subdev->entity, source_pad,
> + &subdev->entity, 0,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> + if (ret) {
> + dev_err(cdev->dev, "failed to create %s:%u -> %s:%u link\n",
> + remote_subdev->entity.name, source_pad,
> + subdev->entity.name, 0);
> + return ret;
> + }
> +
> + video->remote_subdev = remote_subdev;
> + video->remote_pad_index = source_pad;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations sun6i_mipi_csi2_notifier_ops = {
> + .bound = sun6i_mipi_csi2_notifier_bound,
> +};
> +
> +/* Media Entity */
> +
> +static int sun6i_mipi_csi2_link_validate(struct media_link *link)
> +{
> + struct v4l2_subdev *subdev =
> + container_of(link->sink->entity, struct v4l2_subdev, entity);
> + struct sun6i_mipi_csi2_video *video =
> + sun6i_mipi_csi2_subdev_video(subdev);
> + struct v4l2_subdev *remote_subdev;
> + struct v4l2_subdev_format format = { 0 };
> + int ret;
> +
> + if (!is_media_entity_v4l2_subdev(link->source->entity))
> + return -EINVAL;
> +
> + remote_subdev = media_entity_to_v4l2_subdev(link->source->entity);
> +
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = link->source->index;
> +
> + ret = v4l2_subdev_call(remote_subdev, pad, get_fmt, NULL, &format);
> + if (ret)
> + return ret;
> +
> + video->mbus_code = format.format.code;
> +
> + return 0;
> +}
> +
> +static const struct media_entity_operations sun6i_mipi_csi2_entity_ops = {
> + .link_validate = sun6i_mipi_csi2_link_validate,
> +};
> +
> +/* Base Driver */
> +
> +static int sun6i_mipi_csi2_v4l2_setup(struct sun6i_mipi_csi2_dev *cdev)
> +{
> + struct sun6i_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, &sun6i_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 = &sun6i_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;
> +
> + 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 = &sun6i_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;
> +}
> +
> +static int sun6i_mipi_csi2_v4l2_teardown(struct sun6i_mipi_csi2_dev *cdev)
> +{
> + struct sun6i_mipi_csi2_video *video = &cdev->video;
> + struct v4l2_subdev *subdev = &video->subdev;
> + struct v4l2_async_notifier *notifier = &video->notifier;
> +
> + v4l2_async_unregister_subdev(subdev);
> + v4l2_async_notifier_unregister(notifier);
> + v4l2_async_notifier_cleanup(notifier);
> + media_entity_cleanup(&subdev->entity);
> + v4l2_device_unregister_subdev(subdev);
> +
> + return 0;
> +}
> +
> +static const struct regmap_config sun6i_mipi_csi2_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = 0x400,
> +};
> +
> +static int sun6i_mipi_csi2_resource_request(struct sun6i_mipi_csi2_dev *cdev,
> + struct platform_device *pdev)
> +{
> + struct resource *res;
> + void __iomem *io_base;
> + int irq;
> + int ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + io_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(io_base))
> + return PTR_ERR(io_base);
> +
> + cdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", io_base,
> + &sun6i_mipi_csi2_regmap_config);
> + if (IS_ERR(cdev->regmap)) {
> + dev_err(&pdev->dev, "failed to init register map\n");
> + return PTR_ERR(cdev->regmap);
> + }
> +
> + cdev->clk_mod = devm_clk_get(&pdev->dev, "mod");
> + if (IS_ERR(cdev->clk_mod)) {
> + dev_err(&pdev->dev, "failed to acquire csi clock\n");
> + return PTR_ERR(cdev->clk_mod);
> + }
> +
> + cdev->reset = devm_reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(cdev->reset)) {
> + dev_err(&pdev->dev, "failed to get reset controller\n");
> + return PTR_ERR(cdev->reset);
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -ENXIO;
> +
> + ret = devm_request_irq(&pdev->dev, irq, sun6i_mipi_csi2_isr, 0,
> + MODULE_NAME, cdev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request MIPI CSI-2 IRQ\n");
> + return ret;
> + }
> +
> + cdev->dphy = devm_phy_get(&pdev->dev, "dphy");
> + if (IS_ERR(cdev->dphy)) {
> + dev_err(&pdev->dev, "failed to get the MIPI D-PHY\n");
> + return PTR_ERR(cdev->dphy);
> + }
> +
> + ret = phy_init(cdev->dphy);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to initialize the MIPI D-PHY\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int sun6i_mipi_csi2_probe(struct platform_device *pdev)
> +{
> + struct sun6i_mipi_csi2_dev *cdev;
> + int ret;
> +
> + cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
> + if (!cdev)
> + return -ENOMEM;
> +
> + cdev->dev = &pdev->dev;
> +
> + ret = sun6i_mipi_csi2_resource_request(cdev, pdev);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, cdev);
> +
> + ret = sun6i_mipi_csi2_v4l2_setup(cdev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int sun6i_mipi_csi2_remove(struct platform_device *pdev)
> +{
> + struct sun6i_mipi_csi2_dev *cdev = platform_get_drvdata(pdev);
> +
> + phy_exit(cdev->dphy);
> +
> + return sun6i_mipi_csi2_v4l2_teardown(cdev);
> +}
> +
> +static const struct of_device_id sun6i_mipi_csi2_of_match[] = {
> + { .compatible = "allwinner,sun6i-a31-mipi-csi2" },
> + { .compatible = "allwinner,sun8i-v3s-mipi-csi2", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sun6i_mipi_csi2_of_match);
> +
> +static struct platform_driver sun6i_mipi_csi2_platform_driver = {
> + .probe = sun6i_mipi_csi2_probe,
> + .remove = sun6i_mipi_csi2_remove,
> + .driver = {
> + .name = MODULE_NAME,
> + .of_match_table = of_match_ptr(sun6i_mipi_csi2_of_match),
> + },
> +};
> +module_platform_driver(sun6i_mipi_csi2_platform_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A31 MIPI CSI-2 Controller Driver");
> +MODULE_AUTHOR("Paul Kocialkowski <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> new file mode 100644
> index 000000000000..f3cce99bfd44
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2020 Bootlin
> + * Author: Paul Kocialkowski <[email protected]>
> + */
> +
> +#ifndef __SUN6I_MIPI_CSI2_H__
> +#define __SUN6I_MIPI_CSI2_H__
> +
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#define SUN6I_MIPI_CSI2_CTL_REG 0x0
> +#define SUN6I_MIPI_CSI2_CTL_RESET_N BIT(31)
> +#define SUN6I_MIPI_CSI2_CTL_VERSION_EN BIT(30)
> +#define SUN6I_MIPI_CSI2_CTL_UNPK_EN BIT(1)
> +#define SUN6I_MIPI_CSI2_CTL_EN BIT(0)
> +#define SUN6I_MIPI_CSI2_CFG_REG 0x4
> +#define SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(v) ((((v) - 1) << 8) & \
> + GENMASK(9, 8))
> +#define SUN6I_MIPI_CSI2_CFG_LANE_COUNT(v) (((v) - 1) & GENMASK(1, 0))
> +#define SUN6I_MIPI_CSI2_VCDT_RX_REG 0x8
> +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(ch, vc) (((vc) & GENMASK(1, 0)) << \
> + ((ch) * 8 + 6))
> +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(ch, t) (((t) & GENMASK(5, 0)) << \
> + ((ch) * 8))
> +#define SUN6I_MIPI_CSI2_RX_PKT_NUM_REG 0xc
> +
> +#define SUN6I_MIPI_CSI2_VERSION_REG 0x3c
> +
> +#define SUN6I_MIPI_CSI2_CH_BASE 0x1000
> +#define SUN6I_MIPI_CSI2_CH_OFFSET 0x100
> +
> +#define SUN6I_MIPI_CSI2_CH_CFG_REG 0x40
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_REG 0x50
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_EOT_ERR BIT(29)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_CHKSUM_ERR BIT(28)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_WRN BIT(27)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_ERR BIT(26)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_SYNC_ERR BIT(25)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_SYNC_ERR BIT(24)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_EMB_DATA BIT(18)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_PF BIT(17)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_PH_UPDATE BIT(16)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_START_SYNC BIT(11)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_END_SYNC BIT(10)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_START_SYNC BIT(9)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_END_SYNC BIT(8)
> +#define SUN6I_MIPI_CSI2_CH_INT_EN_FIFO_OVER BIT(0)
> +
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_REG 0x58
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_EOT_ERR BIT(29)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_CHKSUM_ERR BIT(28)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_WRN BIT(27)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_ERR BIT(26)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_SYNC_ERR BIT(25)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_SYNC_ERR BIT(24)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_EMB_DATA BIT(18)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_PF BIT(17)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_PH_UPDATE BIT(16)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_START_SYNC BIT(11)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_END_SYNC BIT(10)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_START_SYNC BIT(9)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_END_SYNC BIT(8)
> +#define SUN6I_MIPI_CSI2_CH_INT_PD_FIFO_OVER BIT(0)
> +
> +#define SUN6I_MIPI_CSI2_CH_DT_TRIGGER_REG 0x60
> +#define SUN6I_MIPI_CSI2_CH_CUR_PH_REG 0x70
> +#define SUN6I_MIPI_CSI2_CH_ECC_REG 0x74
> +#define SUN6I_MIPI_CSI2_CH_CKS_REG 0x78
> +#define SUN6I_MIPI_CSI2_CH_FRAME_NUM_REG 0x7c
> +#define SUN6I_MIPI_CSI2_CH_LINE_NUM_REG 0x80
> +
> +#define SUN6I_MIPI_CSI2_CH_REG(reg, ch) \
> + (SUN6I_MIPI_CSI2_CH_BASE + SUN6I_MIPI_CSI2_CH_OFFSET * (ch) + (reg))
> +
> +enum mipi_csi2_data_type {
> + MIPI_CSI2_DATA_TYPE_RAW8 = 0x2a,
> + MIPI_CSI2_DATA_TYPE_RAW10 = 0x2b,
> + MIPI_CSI2_DATA_TYPE_RAW12 = 0x2c,
> +};
> +
> +struct sun6i_mipi_csi2_video {
> + struct v4l2_fwnode_endpoint endpoint;
> + struct v4l2_subdev subdev;
> + struct media_pad pads[2];
> +
> + struct v4l2_async_subdev subdev_async;
> + struct v4l2_async_notifier notifier;
> +
> + struct v4l2_subdev *remote_subdev;
> + u32 remote_pad_index;
> + u32 mbus_code;
> +};
> +
> +struct sun6i_mipi_csi2_dev {
> + struct device *dev;
> +
> + struct regmap *regmap;
> + struct clk *clk_mod;
> + struct reset_control *reset;
> + struct phy *dphy;
> +
> + struct sun6i_mipi_csi2_video video;
> +};
> +
> +#define sun6i_mipi_csi2_subdev_video(subdev) \
> + container_of(subdev, struct sun6i_mipi_csi2_video, subdev)
> +
> +#define sun6i_mipi_csi2_video_dev(video) \
> + container_of(video, struct sun6i_mipi_csi2_dev, video)
> +
> +#endif /* __SUN6I_MIPI_CSI2_H__ */
>

2020-10-30 22:48:22

by Helen Koike

[permalink] [raw]

Subject: Re: [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T

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()

>
> 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.

>
>> 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.

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
>>>
>

2020-11-04 16:55:48

On Wed, Nov 04, 2020 at 01:38:08PM -0300, Helen Koike wrote:
>
>
> On 11/4/20 8:17 AM, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Mon 02 Nov 20, 10:21, Maxime Ripard wrote:
> >> On Fri, Oct 30, 2020 at 07:45:18PM -0300, Helen Koike wrote:
> >>> On 10/23/20 2:45 PM, Paul Kocialkowski wrote:
> >>>> The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 controller
> >>>> found on Allwinner SoCs such as the A31 and V3/V3s.
> >>>>
> >>>> It is a standalone block, connected to the CSI controller on one side
> >>>> and to the MIPI D-PHY block on the other. It has a dedicated address
> >>>> space, interrupt line and clock.
> >>>>
> >>>> Currently, the MIPI CSI-2 controller is hard-tied to a specific CSI
> >>>> controller (CSI0) but newer SoCs (such as the V5) may allow switching
> >>>> MIPI CSI-2 controllers between CSI controllers.
> >>>>
> >>>> It is represented as a V4L2 subdev to the CSI controller and takes a
> >>>> MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
> >>>> media controller API.
> >>>
> >>> Maybe this is a bad idea, but I was thinking:
> >>> This driver basically just turn on/off and catch some interrupts for errors,
> >>> and all the rest of v4l2 config you just forward to the next subdevice
> >>> on the pipeline.
> >>>
> >>> So instead of exposing it as a subdevice, I was wondering if modeling
> >>> this driver also through the phy subsystem wouldn't be cleaner, so
> >>> you won't need all the v4l2 subdevice/topology boilerplate code that
> >>> it seems you are not using (unless you have plans to add controls or
> >>> some specific configuration on this node later).
> >>>
> >>> But this would require changes on the sun6i-csi driver.
> >>>
> >>> What do you think?
> >>
> >> Eventually we'll need to filter the virtual channels / datatypes I
> >> guess, so it's definitely valuable to have it in v4l2
>
> Which kind of datatypes?

MIPI-CSI datatypes. Each packet on the MIPI-CSI bus is assigned a
virtual channel and data type so that you can multiplex multiple streams
(like a 3d camera would send for example, through the virtual channels)
and data types (like frames and metadata) and MIPI-CSI controllers
usually allow to filter them based on what you want.

> I ask to know if this shouldn't be configured through the video node
> instead of subdevice.

Not really, some setups have a mux that can split the multiple virtual
channels to multiple video nodes for example.

> Regarding channels, we had a discussion to implement it through the video
> node (and not subdevice) [1]. But we discussed about blitters and multi-scalers,
> so now I'm wondering if we could use the same API for mipi-csi virtual channels
> in the video entity device, or if it doesn't apply and we need another API
> for that in a subdevice instead.
>
> [1] https://patchwork.linuxtv.org/project/linux-media/cover/[email protected]/

There's already an API to deal with MIPI-CSI virtual channels:
https://patchwork.kernel.org/project/linux-renesas-soc/cover/[email protected]/

Maxime

Attachments:

(No filename) (3.09 kB)
signature.asc (235.00 B)
Download all attachments

2020-11-04 19:24:03

Hi,

On Wed 04 Nov 20, 19:56, Maxime Ripard wrote:
> On Wed, Nov 04, 2020 at 12:34:58PM +0100, Paul Kocialkowski wrote:
> > > > + regmap_write(regmap, SUN6I_MIPI_CSI2_CFG_REG,
> > > > + SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(1) |
> > > > + SUN6I_MIPI_CSI2_CFG_LANE_COUNT(lanes_count));
> > >
> > > It's not really clear what the channel is here? The number of virtual
> > > channels? Something else?
> >
> > That's somewhat described in the controller documentation. Channels refers to
> > physical channels of the controller, which can be used to redirect data
> > matching either a specific data type, a specific virtual channel, or both.
> > There's a somewhat similar concept of channels in the CSI controller too.
> >
> > We're currently only using one...
> >
> > > > + regmap_write(regmap, SUN6I_MIPI_CSI2_VCDT_RX_REG,
> > > > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(3, 3) |
> > > > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(2, 2) |
> > > > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(1, 1) |
> > > > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(0, 0) |
> > > > + SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(0, data_type));
> >
> > ... but it's safer to configure them all to virtual channel numbers so we don't
> > end up with multiple channels matching virtual channel 0.
> >
> > I'll add a comment about that.
>
> Maybe we should have pads for all of them then, even if we don't support
> changing anything?

If that's something we can add later (I think it is), I would rather do this in
a sub-sequent series to keep the current one lightweight and merged ASAP.

It would also require some investigation to find out if the MIPI CSI-2 channel
number i goes directly to the CSI controller channel number i or if some
remapping can take place.

What do you think?

> > > > +static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_subdev_pad_ops = {
> > > > + .enum_mbus_code = sun6i_mipi_csi2_enum_mbus_code,
> > > > + .get_fmt = sun6i_mipi_csi2_get_fmt,
> > > > + .set_fmt = sun6i_mipi_csi2_set_fmt,
> > > > + .enum_frame_size = sun6i_mipi_csi2_enum_frame_size,
> > > > + .enum_frame_interval = sun6i_mipi_csi2_enum_frame_interval,
> > > > +};
> > > > +
> > > > +/* Subdev */
> > > > +
> > > > +static const struct v4l2_subdev_ops sun6i_mipi_csi2_subdev_ops = {
> > > > + .core = &sun6i_mipi_csi2_subdev_core_ops,
> > > > + .video = &sun6i_mipi_csi2_subdev_video_ops,
> > > > + .pad = &sun6i_mipi_csi2_subdev_pad_ops,
> > > > +};
> > > > +
> > > > +/* Notifier */
> > > > +
> > > > +static int sun6i_mipi_csi2_notifier_bound(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *remote_subdev,
> > > > + struct v4l2_async_subdev *remote_subdev_async)
> > > > +{
> > > > + struct v4l2_subdev *subdev = notifier->sd;
> > > > + struct sun6i_mipi_csi2_video *video =
> > > > + sun6i_mipi_csi2_subdev_video(subdev);
> > > > + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > > > + int source_pad;
> > > > + int ret;
> > > > +
> > > > + source_pad = media_entity_get_fwnode_pad(&remote_subdev->entity,
> > > > + remote_subdev->fwnode,
> > > > + MEDIA_PAD_FL_SOURCE);
> > > > + if (source_pad < 0)
> > > > + return source_pad;
> > > > +
> > > > + ret = media_create_pad_link(&remote_subdev->entity, source_pad,
> > > > + &subdev->entity, 0,
> > > > + MEDIA_LNK_FL_ENABLED |
> > > > + MEDIA_LNK_FL_IMMUTABLE);
> > > > + if (ret) {
> > > > + dev_err(cdev->dev, "failed to create %s:%u -> %s:%u link\n",
> > > > + remote_subdev->entity.name, source_pad,
> > > > + subdev->entity.name, 0);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + video->remote_subdev = remote_subdev;
> > > > + video->remote_pad_index = source_pad;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct v4l2_async_notifier_operations sun6i_mipi_csi2_notifier_ops = {
> > > > + .bound = sun6i_mipi_csi2_notifier_bound,
> > > > +};
> > > > +
> > > > +/* Media Entity */
> > > > +
> > > > +static int sun6i_mipi_csi2_link_validate(struct media_link *link)
> > > > +{
> > > > + struct v4l2_subdev *subdev =
> > > > + container_of(link->sink->entity, struct v4l2_subdev, entity);
> > > > + struct sun6i_mipi_csi2_video *video =
> > > > + sun6i_mipi_csi2_subdev_video(subdev);
> > > > + struct v4l2_subdev *remote_subdev;
> > > > + struct v4l2_subdev_format format = { 0 };
> > > > + int ret;
> > > > +
> > > > + if (!is_media_entity_v4l2_subdev(link->source->entity))
> > > > + return -EINVAL;
> > > > +
> > > > + remote_subdev = media_entity_to_v4l2_subdev(link->source->entity);
> > > > +
> > > > + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > > + format.pad = link->source->index;
> > > > +
> > > > + ret = v4l2_subdev_call(remote_subdev, pad, get_fmt, NULL, &format);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + video->mbus_code = format.format.code;
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > I'm not really sure what you're trying to validate here?
> >
> > The whole purpose is to retreive video->mbus_code from the subdev, like it's
> > done in the sun6i-csi driver. Maybe there is a more appropriate op to do it?
>
> I'm not sure why you need to do that in the link_validate though?
>
> You just need to init the pad format, and then you'll have a
> get_fmt/set_fmt for your pads.

Okay I may have misunderstood how manual/automatic propagation is supposed to
work then. The fact that this is done this way in the CSI driver maybe gave me
a bad example to follow. I'll revisit this.

> > > > + cdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", io_base,
> > > > + &sun6i_mipi_csi2_regmap_config);
> > > > + if (IS_ERR(cdev->regmap)) {
> > > > + dev_err(&pdev->dev, "failed to init register map\n");
> > > > + return PTR_ERR(cdev->regmap);
> > > > + }
> > >
> > > Yeah, so that won't work. regmap expects to have access to those
> > > registers when you enable that clock, but that won't happen since the
> > > reset line can be disabled. You would be better off using runtime_pm
> > > here.
> >
> > I don't understand what you mean here or what the problem could be.
> > Here we're just initializing regmap and while this is done before the
> > registers are available for I/O, I don't see why it would cause any
> > issue at this point.
>
> The regmap here is supposed to take care of the resources, except it
> only does it for some of the resources here, which kind of breaks the
> expectations. And it doesn't allow you to have the reset / clock
> sequence properly done.

I'm not following any of this. Could you break it down some more?
In particular I don't see the relationship between initializing the regmap
API (which does take IORESOURCE_MEM in) and reset / clocks.

Just initializing the regmap API for I/O mem doesn't mean that the registers
are ready for use, does it? We're not requesting any I/O operation from it
at this point.

> > The exact same thing is done in the CSI driver.
>
> That's not an argument though, is it? :)

That's not the point, the point is rather that if this is wrong, we should fix
it in the CSI driver as well.

Paul

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachments:

(No filename) (7.21 kB)
signature.asc (499.00 B)
Download all attachments

2020-11-05 14:57:16

by Paul Kocialkowski

[permalink] [raw]

Subject: Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller

Hi Sakari and thanks for the review!

On Thu 05 Nov 20, 10:45, Sakari Ailus wrote:
> On Fri, Oct 23, 2020 at 07:45:40PM +0200, Paul Kocialkowski wrote:
> > The A31 MIPI CSI-2 controller is a dedicated MIPI CSI-2 controller
> > found on Allwinner SoCs such as the A31 and V3/V3s.
> >
> > It is a standalone block, connected to the CSI controller on one side
> > and to the MIPI D-PHY block on the other. It has a dedicated address
> > space, interrupt line and clock.
> >
> > Currently, the MIPI CSI-2 controller is hard-tied to a specific CSI
> > controller (CSI0) but newer SoCs (such as the V5) may allow switching
> > MIPI CSI-2 controllers between CSI controllers.
> >
> > It is represented as a V4L2 subdev to the CSI controller and takes a
> > MIPI CSI-2 sensor as its own subdev, all using the fwnode graph and
> > media controller API.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > drivers/media/platform/sunxi/Kconfig | 1 +
> > drivers/media/platform/sunxi/Makefile | 1 +
> > .../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 ++++
> > 6 files changed, 768 insertions(+)
> > 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
> >
> > diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
> > index 7151cc249afa..9684e07454ad 100644
> > --- a/drivers/media/platform/sunxi/Kconfig
> > +++ b/drivers/media/platform/sunxi/Kconfig
> > @@ -2,3 +2,4 @@
> >
> > source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
> > source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> > +source "drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig"
> > diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
> > index fc537c9f5ca9..887a7cae8fca 100644
> > --- a/drivers/media/platform/sunxi/Makefile
> > +++ b/drivers/media/platform/sunxi/Makefile
> > @@ -2,5 +2,6 @@
> >
> > obj-y += sun4i-csi/
> > obj-y += sun6i-csi/
> > +obj-y += sun6i-mipi-csi2/
> > obj-y += sun8i-di/
> > obj-y += sun8i-rotate/
> > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> > new file mode 100644
> > index 000000000000..7033bda483b4
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Kconfig
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config VIDEO_SUN6I_MIPI_CSI2
> > + tristate "Allwinner A31 MIPI CSI-2 Controller Driver"
> > + depends on VIDEO_V4L2 && COMMON_CLK
> > + depends on ARCH_SUNXI || COMPILE_TEST
> > + select MEDIA_CONTROLLER
> > + select VIDEO_V4L2_SUBDEV_API
> > + select REGMAP_MMIO
> > + select V4L2_FWNODE
> > + help
> > + Support for the Allwinner A31 MIPI CSI-2 Controller.
> > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> > new file mode 100644
> > index 000000000000..14e4e03818b5
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +sun6i-mipi-csi2-y += sun6i_mipi_csi2.o
> > +
> > +obj-$(CONFIG_VIDEO_SUN6I_MIPI_CSI2) += sun6i-mipi-csi2.o
> > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> > new file mode 100644
> > index 000000000000..ce89c35f5b86
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> > @@ -0,0 +1,635 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2020 Bootlin
> > + * Author: Paul Kocialkowski <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#include "sun6i_mipi_csi2.h"
> > +
> > +#define MODULE_NAME "sun6i-mipi-csi2"
> > +
> > +/* Core */
> > +
> > +static irqreturn_t sun6i_mipi_csi2_isr(int irq, void *dev_id)
> > +{
> > + struct sun6i_mipi_csi2_dev *cdev = (struct sun6i_mipi_csi2_dev *)dev_id;
>
> Unnecessary casting from void *.
>
> > + struct regmap *regmap = cdev->regmap;
> > + u32 pending;
> > +
> > + WARN_ONCE(1, MODULE_NAME
> > + ": Unsolicited interrupt, an error likely occurred!\n");
> > +
> > + regmap_read(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, &pending);
> > + regmap_write(regmap, SUN6I_MIPI_CSI2_CH_INT_PD_REG, pending);
> > +
> > + /*
> > + * The interrupt can be used to catch transmission errors.
> > + * However, we currently lack plumbing for reporting that to the
> > + * A31 CSI controller driver.
> > + */
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int sun6i_mipi_csi2_s_power(struct v4l2_subdev *subdev, int on)
>
> Please use runtime PM instead.
>
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > + int ret;
> > +
> > + if (!on) {
> > + clk_disable_unprepare(cdev->clk_mod);
> > + reset_control_assert(cdev->reset);
> > +
> > + return 0;
> > + }
> > +
> > + ret = clk_prepare_enable(cdev->clk_mod);
> > + if (ret) {
> > + dev_err(cdev->dev, "failed to enable module clock\n");
> > + return ret;
> > + }
> > +
> > + ret = reset_control_deassert(cdev->reset);
> > + if (ret) {
> > + dev_err(cdev->dev, "failed to deassert reset\n");
> > + goto error_clk;
> > + }
> > +
> > + return 0;
> > +
> > +error_clk:
> > + clk_disable_unprepare(cdev->clk_mod);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops sun6i_mipi_csi2_subdev_core_ops = {
> > + .s_power = sun6i_mipi_csi2_s_power,
> > +};
> > +
> > +/* Video */
> > +
> > +static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > + struct v4l2_subdev *remote_subdev = video->remote_subdev;
> > + struct v4l2_fwnode_bus_mipi_csi2 *bus_mipi_csi2 =
> > + &video->endpoint.bus.mipi_csi2;
> > + union phy_configure_opts dphy_opts = { 0 };
> > + struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
> > + struct regmap *regmap = cdev->regmap;
> > + struct v4l2_ctrl *ctrl;
> > + unsigned int lanes_count;
> > + unsigned int bpp;
> > + unsigned long pixel_rate;
> > + u8 data_type = 0;
> > + u32 version = 0;
> > + /* Initialize to 0 to use both in disable label (ret != 0) and off. */
> > + int ret = 0;
> > +
> > + if (!remote_subdev)
> > + return -ENODEV;
> > +
> > + if (!on) {
> > + v4l2_subdev_call(remote_subdev, video, s_stream, 0);
> > +
> > +disable:
>
> I think this label would be nicer at the end of the function (so you'd have
> a goto here).

It looks like everyone hates the backwards goto, even when there are no bad
side-effects attached to it. I'll go for moving it to the end then :)

> > + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> > + SUN6I_MIPI_CSI2_CTL_EN, 0);
> > +
> > + phy_power_off(cdev->dphy);
> > +
> > + return ret;
> > + }
> > +
> > + switch (video->mbus_code) {
> > + case MEDIA_BUS_FMT_SBGGR8_1X8:
> > + case MEDIA_BUS_FMT_SGBRG8_1X8:
> > + case MEDIA_BUS_FMT_SGRBG8_1X8:
> > + case MEDIA_BUS_FMT_SRGGB8_1X8:
> > + data_type = MIPI_CSI2_DATA_TYPE_RAW8;
> > + bpp = 8;
> > + break;
> > + case MEDIA_BUS_FMT_SBGGR10_1X10:
> > + case MEDIA_BUS_FMT_SGBRG10_1X10:
> > + case MEDIA_BUS_FMT_SGRBG10_1X10:
> > + case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + data_type = MIPI_CSI2_DATA_TYPE_RAW10;
> > + bpp = 10;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /* Sensor pixel rate */
> > +
> > + ctrl = v4l2_ctrl_find(remote_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > + if (!ctrl) {
> > + dev_err(cdev->dev,
> > + "%s: no MIPI CSI-2 pixel rate from the sensor\n",
> > + __func__);
> > + return -ENODEV;
> > + }
> > +
> > + pixel_rate = (unsigned long)v4l2_ctrl_g_ctrl_int64(ctrl);
> > + if (!pixel_rate) {
> > + dev_err(cdev->dev,
> > + "%s: zero MIPI CSI-2 pixel rate from the sensor\n",
> > + __func__);
> > + return -ENODEV;
> > + }
> > +
> > + /* D-PHY configuration */
> > +
> > + lanes_count = bus_mipi_csi2->num_data_lanes;
> > + phy_mipi_dphy_get_default_config(pixel_rate, bpp, lanes_count,
> > + dphy_cfg);
> > +
> > +
> > + /*
> > + * Note that our hardware is using DDR, which is not taken in account by
> > + * phy_mipi_dphy_get_default_config when calculating hs_clk_rate from
> > + * the pixel rate, lanes count and bpp.
> > + *
> > + * The resulting clock rate is basically the symbol rate over the whole
> > + * link. The actual clock rate is calculated with division by two since
> > + * DDR samples both on rising and falling edges.
> > + */
> > +
> > + dev_dbg(cdev->dev, "A31 MIPI CSI-2 config:\n");
> > + dev_dbg(cdev->dev, "%ld pixels/s, %u bits/pixel, %lu Hz clock\n",
> > + pixel_rate, bpp, dphy_cfg->hs_clk_rate / 2);
> > +
> > + ret = 0;
> > + ret |= phy_reset(cdev->dphy);
> > + ret |= phy_set_mode_ext(cdev->dphy, PHY_MODE_MIPI_DPHY,
> > + PHY_MIPI_DPHY_SUBMODE_RX);
> > + ret |= phy_configure(cdev->dphy, &dphy_opts);
>
> Huh. Please don't do bitwise or operations on error codes.
>
> > +
> > + if (ret) {
> > + dev_err(cdev->dev, "failed to setup MIPI D-PHY\n");
> > + return ret;
> > + }
> > +
> > + ret = phy_power_on(cdev->dphy);
> > + if (ret) {
> > + dev_err(cdev->dev, "failed to power on MIPI D-PHY\n");
> > + return ret;
> > + }
> > +
> > + /* MIPI CSI-2 controller setup */
> > +
> > + /*
> > + * The enable flow in the Allwinner BSP is a bit different: the enable
> > + * and reset bits are set together before starting the CSI controller.
> > + *
> > + * In mainline we enable the CSI controller first (due to subdev logic).
> > + * One reliable way to make this work is to deassert reset, configure
> > + * registers and enable the controller when everything's ready.
> > + *
> > + * However, reading the version appears necessary for it to work
> > + * reliably. Replacing it with a delay doesn't do the trick.
> > + */
> > + regmap_write(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> > + SUN6I_MIPI_CSI2_CTL_RESET_N |
> > + SUN6I_MIPI_CSI2_CTL_VERSION_EN |
> > + SUN6I_MIPI_CSI2_CTL_UNPK_EN);
> > +
> > + regmap_read(regmap, SUN6I_MIPI_CSI2_VERSION_REG, &version);
> > +
> > + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> > + SUN6I_MIPI_CSI2_CTL_VERSION_EN, 0);
> > +
> > + dev_dbg(cdev->dev, "A31 MIPI CSI-2 version: %04x\n", version);
> > +
> > + regmap_write(regmap, SUN6I_MIPI_CSI2_CFG_REG,
> > + SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(1) |
> > + SUN6I_MIPI_CSI2_CFG_LANE_COUNT(lanes_count));
> > +
> > + regmap_write(regmap, SUN6I_MIPI_CSI2_VCDT_RX_REG,
> > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(3, 3) |
> > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(2, 2) |
> > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(1, 1) |
> > + SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(0, 0) |
> > + SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(0, data_type));
> > +
> > + regmap_update_bits(regmap, SUN6I_MIPI_CSI2_CTL_REG,
> > + SUN6I_MIPI_CSI2_CTL_EN, SUN6I_MIPI_CSI2_CTL_EN);
> > +
> > + ret = v4l2_subdev_call(remote_subdev, video, s_stream, 1);
> > + if (ret)
> > + goto disable;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops sun6i_mipi_csi2_subdev_video_ops = {
> > + .s_stream = sun6i_mipi_csi2_s_stream,
> > +};
> > +
> > +/* Pad */
> > +
> > +static int sun6i_mipi_csi2_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *config,
> > + struct v4l2_subdev_mbus_code_enum *code_enum)
>
> You can easily get under 80 characters per line if you wrap after the
> function's return type.
>
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > +
> > + if (!video->remote_subdev)
> > + return -ENODEV;
> > +
> > + /* Forward to the sensor. */
> > + code_enum->pad = video->remote_pad_index;
> > +
> > + return v4l2_subdev_call(video->remote_subdev, pad, enum_mbus_code,
> > + config, code_enum);
> > +}
> > +
> > +static int sun6i_mipi_csi2_get_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *config,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > +
> > + if (!video->remote_subdev)
> > + return -ENODEV;
> > +
> > + /* Forward to the sensor. */
> > + format->pad = video->remote_pad_index;
> > +
> > + return v4l2_subdev_call(video->remote_subdev, pad, get_fmt, config,
> > + format);
>
> The entity this driver controls is independent of the upstream device in
> its configuratione, you should not try to propagate the configuration
> upstream in the pipeline.
>
> The same applies to other similar functions in the driver.
>
> > +}
> > +
> > +static int sun6i_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *config,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > +
> > + if (!video->remote_subdev)
> > + return -ENODEV;
> > +
> > + /* Forward to the sensor. */
> > + format->pad = video->remote_pad_index;
> > +
> > + return v4l2_subdev_call(video->remote_subdev, pad, set_fmt, config,
> > + format);
> > +}
> > +
> > +static int sun6i_mipi_csi2_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *config,
> > + struct v4l2_subdev_frame_size_enum *size_enum)
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > +
> > + if (!video->remote_subdev)
> > + return -ENODEV;
> > +
> > + /* Forward to the sensor. */
> > + size_enum->pad = video->remote_pad_index;
> > +
> > + return v4l2_subdev_call(video->remote_subdev, pad, enum_frame_size,
> > + config, size_enum);
> > +}
> > +
> > +static int sun6i_mipi_csi2_enum_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *config,
> > + struct v4l2_subdev_frame_interval_enum *interval_enum)
> > +{
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > +
> > + if (!video->remote_subdev)
> > + return -ENODEV;
> > +
> > + /* Forward to the sensor. */
> > + interval_enum->pad = video->remote_pad_index;
> > +
> > + return v4l2_subdev_call(video->remote_subdev, pad, enum_frame_interval,
> > + config, interval_enum);
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_subdev_pad_ops = {
> > + .enum_mbus_code = sun6i_mipi_csi2_enum_mbus_code,
> > + .get_fmt = sun6i_mipi_csi2_get_fmt,
> > + .set_fmt = sun6i_mipi_csi2_set_fmt,
> > + .enum_frame_size = sun6i_mipi_csi2_enum_frame_size,
> > + .enum_frame_interval = sun6i_mipi_csi2_enum_frame_interval,
> > +};
> > +
> > +/* Subdev */
> > +
> > +static const struct v4l2_subdev_ops sun6i_mipi_csi2_subdev_ops = {
> > + .core = &sun6i_mipi_csi2_subdev_core_ops,
> > + .video = &sun6i_mipi_csi2_subdev_video_ops,
> > + .pad = &sun6i_mipi_csi2_subdev_pad_ops,
> > +};
> > +
> > +/* Notifier */
> > +
> > +static int sun6i_mipi_csi2_notifier_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *remote_subdev,
> > + struct v4l2_async_subdev *remote_subdev_async)
> > +{
> > + struct v4l2_subdev *subdev = notifier->sd;
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > + struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > + int source_pad;
> > + int ret;
> > +
> > + source_pad = media_entity_get_fwnode_pad(&remote_subdev->entity,
> > + remote_subdev->fwnode,
> > + MEDIA_PAD_FL_SOURCE);
> > + if (source_pad < 0)
> > + return source_pad;
> > +
> > + ret = media_create_pad_link(&remote_subdev->entity, source_pad,
> > + &subdev->entity, 0,
> > + MEDIA_LNK_FL_ENABLED |
> > + MEDIA_LNK_FL_IMMUTABLE);
> > + if (ret) {
> > + dev_err(cdev->dev, "failed to create %s:%u -> %s:%u link\n",
> > + remote_subdev->entity.name, source_pad,
> > + subdev->entity.name, 0);
> > + return ret;
> > + }
> > +
> > + video->remote_subdev = remote_subdev;
> > + video->remote_pad_index = source_pad;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_async_notifier_operations sun6i_mipi_csi2_notifier_ops = {
> > + .bound = sun6i_mipi_csi2_notifier_bound,
> > +};
> > +
> > +/* Media Entity */
> > +
> > +static int sun6i_mipi_csi2_link_validate(struct media_link *link)
> > +{
> > + struct v4l2_subdev *subdev =
> > + container_of(link->sink->entity, struct v4l2_subdev, entity);
> > + struct sun6i_mipi_csi2_video *video =
> > + sun6i_mipi_csi2_subdev_video(subdev);
> > + struct v4l2_subdev *remote_subdev;
> > + struct v4l2_subdev_format format = { 0 };
> > + int ret;
> > +
> > + if (!is_media_entity_v4l2_subdev(link->source->entity))
> > + return -EINVAL;
> > +
> > + remote_subdev = media_entity_to_v4l2_subdev(link->source->entity);
> > +
> > + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + format.pad = link->source->index;
> > +
> > + ret = v4l2_subdev_call(remote_subdev, pad, get_fmt, NULL, &format);
> > + if (ret)
> > + return ret;
> > +
> > + video->mbus_code = format.format.code;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct media_entity_operations sun6i_mipi_csi2_entity_ops = {
> > + .link_validate = sun6i_mipi_csi2_link_validate,
> > +};
> > +
> > +/* Base Driver */
> > +
> > +static int sun6i_mipi_csi2_v4l2_setup(struct sun6i_mipi_csi2_dev *cdev)
> > +{
> > + struct sun6i_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, &sun6i_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 = &sun6i_mipi_csi2_entity_ops;
>
> Why is there no device node? This driver appears to be MC-enabled.

Right, it looks like we need a subdev node here. I'll change that.

Paul

> > +
> > + /* 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;
> > +
> > + 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 = &sun6i_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;
> > +}
> > +
> > +static int sun6i_mipi_csi2_v4l2_teardown(struct sun6i_mipi_csi2_dev *cdev)
> > +{
> > + struct sun6i_mipi_csi2_video *video = &cdev->video;
> > + struct v4l2_subdev *subdev = &video->subdev;
> > + struct v4l2_async_notifier *notifier = &video->notifier;
> > +
> > + v4l2_async_unregister_subdev(subdev);
> > + v4l2_async_notifier_unregister(notifier);
> > + v4l2_async_notifier_cleanup(notifier);
> > + media_entity_cleanup(&subdev->entity);
> > + v4l2_device_unregister_subdev(subdev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct regmap_config sun6i_mipi_csi2_regmap_config = {
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .max_register = 0x400,
> > +};
> > +
> > +static int sun6i_mipi_csi2_resource_request(struct sun6i_mipi_csi2_dev *cdev,
> > + struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + void __iomem *io_base;
> > + int irq;
> > + int ret;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + io_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(io_base))
> > + return PTR_ERR(io_base);
> > +
> > + cdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", io_base,
> > + &sun6i_mipi_csi2_regmap_config);
> > + if (IS_ERR(cdev->regmap)) {
> > + dev_err(&pdev->dev, "failed to init register map\n");
> > + return PTR_ERR(cdev->regmap);
> > + }
> > +
> > + cdev->clk_mod = devm_clk_get(&pdev->dev, "mod");
> > + if (IS_ERR(cdev->clk_mod)) {
> > + dev_err(&pdev->dev, "failed to acquire csi clock\n");
> > + return PTR_ERR(cdev->clk_mod);
> > + }
> > +
> > + cdev->reset = devm_reset_control_get_shared(&pdev->dev, NULL);
> > + if (IS_ERR(cdev->reset)) {
> > + dev_err(&pdev->dev, "failed to get reset controller\n");
> > + return PTR_ERR(cdev->reset);
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return -ENXIO;
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, sun6i_mipi_csi2_isr, 0,
> > + MODULE_NAME, cdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to request MIPI CSI-2 IRQ\n");
> > + return ret;
> > + }
> > +
> > + cdev->dphy = devm_phy_get(&pdev->dev, "dphy");
> > + if (IS_ERR(cdev->dphy)) {
> > + dev_err(&pdev->dev, "failed to get the MIPI D-PHY\n");
> > + return PTR_ERR(cdev->dphy);
> > + }
> > +
> > + ret = phy_init(cdev->dphy);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to initialize the MIPI D-PHY\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sun6i_mipi_csi2_probe(struct platform_device *pdev)
> > +{
> > + struct sun6i_mipi_csi2_dev *cdev;
> > + int ret;
> > +
> > + cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
> > + if (!cdev)
> > + return -ENOMEM;
> > +
> > + cdev->dev = &pdev->dev;
> > +
> > + ret = sun6i_mipi_csi2_resource_request(cdev, pdev);
> > + if (ret)
> > + return ret;
> > +
> > + platform_set_drvdata(pdev, cdev);
> > +
> > + ret = sun6i_mipi_csi2_v4l2_setup(cdev);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int sun6i_mipi_csi2_remove(struct platform_device *pdev)
> > +{
> > + struct sun6i_mipi_csi2_dev *cdev = platform_get_drvdata(pdev);
> > +
> > + phy_exit(cdev->dphy);
> > +
> > + return sun6i_mipi_csi2_v4l2_teardown(cdev);
> > +}
> > +
> > +static const struct of_device_id sun6i_mipi_csi2_of_match[] = {
> > + { .compatible = "allwinner,sun6i-a31-mipi-csi2" },
> > + { .compatible = "allwinner,sun8i-v3s-mipi-csi2", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sun6i_mipi_csi2_of_match);
> > +
> > +static struct platform_driver sun6i_mipi_csi2_platform_driver = {
> > + .probe = sun6i_mipi_csi2_probe,
> > + .remove = sun6i_mipi_csi2_remove,
> > + .driver = {
> > + .name = MODULE_NAME,
> > + .of_match_table = of_match_ptr(sun6i_mipi_csi2_of_match),
> > + },
> > +};
> > +module_platform_driver(sun6i_mipi_csi2_platform_driver);
> > +
> > +MODULE_DESCRIPTION("Allwinner A31 MIPI CSI-2 Controller Driver");
> > +MODULE_AUTHOR("Paul Kocialkowski <[email protected]>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> > new file mode 100644
> > index 000000000000..f3cce99bfd44
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2020 Bootlin
> > + * Author: Paul Kocialkowski <[email protected]>
> > + */
> > +
> > +#ifndef __SUN6I_MIPI_CSI2_H__
> > +#define __SUN6I_MIPI_CSI2_H__
> > +
> > +#include <linux/phy/phy.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> > +
> > +#define SUN6I_MIPI_CSI2_CTL_REG 0x0
> > +#define SUN6I_MIPI_CSI2_CTL_RESET_N BIT(31)
> > +#define SUN6I_MIPI_CSI2_CTL_VERSION_EN BIT(30)
> > +#define SUN6I_MIPI_CSI2_CTL_UNPK_EN BIT(1)
> > +#define SUN6I_MIPI_CSI2_CTL_EN BIT(0)
> > +#define SUN6I_MIPI_CSI2_CFG_REG 0x4
> > +#define SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(v) ((((v) - 1) << 8) & \
> > + GENMASK(9, 8))
> > +#define SUN6I_MIPI_CSI2_CFG_LANE_COUNT(v) (((v) - 1) & GENMASK(1, 0))
> > +#define SUN6I_MIPI_CSI2_VCDT_RX_REG 0x8
> > +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(ch, vc) (((vc) & GENMASK(1, 0)) << \
> > + ((ch) * 8 + 6))
> > +#define SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(ch, t) (((t) & GENMASK(5, 0)) << \
> > + ((ch) * 8))
> > +#define SUN6I_MIPI_CSI2_RX_PKT_NUM_REG 0xc
> > +
> > +#define SUN6I_MIPI_CSI2_VERSION_REG 0x3c
> > +
> > +#define SUN6I_MIPI_CSI2_CH_BASE 0x1000
> > +#define SUN6I_MIPI_CSI2_CH_OFFSET 0x100
> > +
> > +#define SUN6I_MIPI_CSI2_CH_CFG_REG 0x40
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_REG 0x50
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_EOT_ERR BIT(29)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_CHKSUM_ERR BIT(28)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_WRN BIT(27)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_ECC_ERR BIT(26)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_SYNC_ERR BIT(25)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_SYNC_ERR BIT(24)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_EMB_DATA BIT(18)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_PF BIT(17)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_PH_UPDATE BIT(16)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_START_SYNC BIT(11)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_LINE_END_SYNC BIT(10)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_START_SYNC BIT(9)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_FRAME_END_SYNC BIT(8)
> > +#define SUN6I_MIPI_CSI2_CH_INT_EN_FIFO_OVER BIT(0)
> > +
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_REG 0x58
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_EOT_ERR BIT(29)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_CHKSUM_ERR BIT(28)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_WRN BIT(27)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_ECC_ERR BIT(26)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_SYNC_ERR BIT(25)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_SYNC_ERR BIT(24)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_EMB_DATA BIT(18)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_PF BIT(17)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_PH_UPDATE BIT(16)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_START_SYNC BIT(11)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_LINE_END_SYNC BIT(10)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_START_SYNC BIT(9)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_FRAME_END_SYNC BIT(8)
> > +#define SUN6I_MIPI_CSI2_CH_INT_PD_FIFO_OVER BIT(0)
> > +
> > +#define SUN6I_MIPI_CSI2_CH_DT_TRIGGER_REG 0x60
> > +#define SUN6I_MIPI_CSI2_CH_CUR_PH_REG 0x70
> > +#define SUN6I_MIPI_CSI2_CH_ECC_REG 0x74
> > +#define SUN6I_MIPI_CSI2_CH_CKS_REG 0x78
> > +#define SUN6I_MIPI_CSI2_CH_FRAME_NUM_REG 0x7c
> > +#define SUN6I_MIPI_CSI2_CH_LINE_NUM_REG 0x80
> > +
> > +#define SUN6I_MIPI_CSI2_CH_REG(reg, ch) \
> > + (SUN6I_MIPI_CSI2_CH_BASE + SUN6I_MIPI_CSI2_CH_OFFSET * (ch) + (reg))
> > +
> > +enum mipi_csi2_data_type {
> > + MIPI_CSI2_DATA_TYPE_RAW8 = 0x2a,
> > + MIPI_CSI2_DATA_TYPE_RAW10 = 0x2b,
> > + MIPI_CSI2_DATA_TYPE_RAW12 = 0x2c,
> > +};
> > +
> > +struct sun6i_mipi_csi2_video {
> > + struct v4l2_fwnode_endpoint endpoint;
> > + struct v4l2_subdev subdev;
> > + struct media_pad pads[2];
> > +
> > + struct v4l2_async_subdev subdev_async;
> > + struct v4l2_async_notifier notifier;
> > +
> > + struct v4l2_subdev *remote_subdev;
> > + u32 remote_pad_index;
> > + u32 mbus_code;
> > +};
> > +
> > +struct sun6i_mipi_csi2_dev {
> > + struct device *dev;
> > +
> > + struct regmap *regmap;
> > + struct clk *clk_mod;
> > + struct reset_control *reset;
> > + struct phy *dphy;
> > +
> > + struct sun6i_mipi_csi2_video video;
> > +};
> > +
> > +#define sun6i_mipi_csi2_subdev_video(subdev) \
> > + container_of(subdev, struct sun6i_mipi_csi2_video, subdev)
> > +
> > +#define sun6i_mipi_csi2_video_dev(video) \
> > + container_of(video, struct sun6i_mipi_csi2_dev, video)
> > +
> > +#endif /* __SUN6I_MIPI_CSI2_H__ */
>
> --
> Regards,
>
> Sakari Ailus

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachments:

(No filename) (30.01 kB)
signature.asc (499.00 B)
Download all attachments

2020-11-05 15:00:24

by Paul Kocialkowski

[permalink] [raw]

Subject: Re: [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T

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
> >>>
> >

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachments:

(No filename) (9.75 kB)
signature.asc (499.00 B)
Download all attachments