2023-05-12 10:37:15

by Jack Zhu

[permalink] [raw]
Subject: [PATCH v1 0/5] Add support for external dphy

This series adds support for external D-PHY and JH7110 SoC which has a Cadence
MIPI-CSI2 RX controller. The driver is tested on VisionFive V2 board.

PLEASE NOTE: this patch series was spun off from the v4 of another series that
included CSI and ISP driver. You can check the comments for earlier version in
the link below. I'm sorry that the first version link has been lost.

v4 link: https://lore.kernel.org/all/[email protected]/
v3 link: https://lore.kernel.org/all/[email protected]/
v2 link: https://lore.kernel.org/all/[email protected]/

Current review status:
- cdns,csi2rx.yaml:
Reviewed by: Krzysztof Kozlowski
Reviewed by: Laurent Pinchart
- cdns-csi2rx.c
Reviewed by: Laurent Pinchart
- MAINTAINERS
Reviewed by: Krzysztof Kozlowski
Reviewed by: Laurent Pinchart

Thanks to everyone who reviewed my work !

Jack Zhu (5):
media: dt-bindings: cadence-csi2rx: Convert to DT schema
media: dt-bindings: cadence-csi2rx: Add resets property
media: cadence: Add operation on reset
media: cadence: Add support for external dphy
media: cadence: Add support for JH7110 SoC

.../devicetree/bindings/media/cdns,csi2rx.txt | 100 ---------
.../bindings/media/cdns,csi2rx.yaml | 201 ++++++++++++++++++
MAINTAINERS | 1 +
drivers/media/platform/cadence/cdns-csi2rx.c | 107 ++++++++--
4 files changed, 294 insertions(+), 115 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt
create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.yaml

--
2.34.1



2023-05-12 10:37:29

by Jack Zhu

[permalink] [raw]
Subject: [PATCH v1 3/5] media: cadence: Add operation on reset

Add operation on reset for Cadence MIPI-CSI2 RX Controller.

Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jack Zhu <[email protected]>
---
drivers/media/platform/cadence/cdns-csi2rx.c | 40 +++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 9755d1c8ceb9..bb78f54e944e 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -13,6 +13,7 @@
#include <linux/of_graph.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>
#include <linux/slab.h>

#include <media/v4l2-ctrls.h>
@@ -68,6 +69,9 @@ struct csi2rx_priv {
struct clk *sys_clk;
struct clk *p_clk;
struct clk *pixel_clk[CSI2RX_STREAMS_MAX];
+ struct reset_control *sys_rst;
+ struct reset_control *p_rst;
+ struct reset_control *pixel_rst[CSI2RX_STREAMS_MAX];
struct phy *dphy;

u8 lanes[CSI2RX_LANES_MAX];
@@ -112,6 +116,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
if (ret)
return ret;

+ reset_control_deassert(csi2rx->p_rst);
csi2rx_reset(csi2rx);

reg = csi2rx->num_lanes << 8;
@@ -154,6 +159,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
if (ret)
goto err_disable_pixclk;

+ reset_control_deassert(csi2rx->pixel_rst[i]);
+
writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
csi2rx->base + CSI2RX_STREAM_CFG_REG(i));

@@ -169,13 +176,16 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
if (ret)
goto err_disable_pixclk;

+ reset_control_deassert(csi2rx->sys_rst);
clk_disable_unprepare(csi2rx->p_clk);

return 0;

err_disable_pixclk:
- for (; i > 0; i--)
+ for (; i > 0; i--) {
+ reset_control_assert(csi2rx->pixel_rst[i - 1]);
clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
+ }

err_disable_pclk:
clk_disable_unprepare(csi2rx->p_clk);
@@ -188,14 +198,17 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
unsigned int i;

clk_prepare_enable(csi2rx->p_clk);
+ reset_control_assert(csi2rx->sys_rst);
clk_disable_unprepare(csi2rx->sys_clk);

for (i = 0; i < csi2rx->max_streams; i++) {
writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));

+ reset_control_assert(csi2rx->pixel_rst[i]);
clk_disable_unprepare(csi2rx->pixel_clk[i]);
}

+ reset_control_assert(csi2rx->p_rst);
clk_disable_unprepare(csi2rx->p_clk);

if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
@@ -299,6 +312,16 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
return PTR_ERR(csi2rx->p_clk);
}

+ csi2rx->sys_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
+ "sys_rst");
+ if (IS_ERR(csi2rx->sys_rst))
+ return PTR_ERR(csi2rx->sys_rst);
+
+ csi2rx->p_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
+ "p_rst");
+ if (IS_ERR(csi2rx->p_rst))
+ return PTR_ERR(csi2rx->p_rst);
+
csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
if (IS_ERR(csi2rx->dphy)) {
dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
@@ -349,14 +372,21 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
}

for (i = 0; i < csi2rx->max_streams; i++) {
- char clk_name[16];
+ char name[16];

- snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
- csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
+ snprintf(name, sizeof(name), "pixel_if%u_clk", i);
+ csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, name);
if (IS_ERR(csi2rx->pixel_clk[i])) {
- dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
+ dev_err(&pdev->dev, "Couldn't get clock %s\n", name);
return PTR_ERR(csi2rx->pixel_clk[i]);
}
+
+ snprintf(name, sizeof(name), "pixel_if%u_rst", i);
+ csi2rx->pixel_rst[i] =
+ devm_reset_control_get_optional_exclusive(&pdev->dev,
+ name);
+ if (IS_ERR(csi2rx->pixel_rst[i]))
+ return PTR_ERR(csi2rx->pixel_rst[i]);
}

return 0;
--
2.34.1


2023-05-15 14:56:25

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v1 3/5] media: cadence: Add operation on reset

Hi Jack,

On Fri, May 12, 2023 at 06:26:35PM +0800, Jack Zhu wrote:
[...]
> @@ -299,6 +312,16 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> return PTR_ERR(csi2rx->p_clk);
> }
>
> + csi2rx->sys_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> + "sys_rst");

This doesn't match the bindings documented in patch 2.
Should this be "sys"?

> + if (IS_ERR(csi2rx->sys_rst))
> + return PTR_ERR(csi2rx->sys_rst);
> +
> + csi2rx->p_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> + "p_rst");

This doesn't match the bindings documented in patch 2.
Should this be "reg_bank"?

> + if (IS_ERR(csi2rx->p_rst))
> + return PTR_ERR(csi2rx->p_rst);
> +
> csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
> if (IS_ERR(csi2rx->dphy)) {
> dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
> @@ -349,14 +372,21 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> }
>
> for (i = 0; i < csi2rx->max_streams; i++) {
> - char clk_name[16];
> + char name[16];
>
> - snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> - csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> + snprintf(name, sizeof(name), "pixel_if%u_clk", i);
> + csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, name);
> if (IS_ERR(csi2rx->pixel_clk[i])) {
> - dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
> + dev_err(&pdev->dev, "Couldn't get clock %s\n", name);
> return PTR_ERR(csi2rx->pixel_clk[i]);
> }
> +
> + snprintf(name, sizeof(name), "pixel_if%u_rst", i);

This doesn't match the bindings documented in patch 2.
Should this be "pixel_if%u"?

regards
Philipp

2023-05-16 01:10:00

by Jack Zhu

[permalink] [raw]
Subject: Re: [PATCH v1 3/5] media: cadence: Add operation on reset

Hi Philipp,

Thank you for your review!

On 2023/5/15 22:42, Philipp Zabel wrote:
> Hi Jack,
>
> On Fri, May 12, 2023 at 06:26:35PM +0800, Jack Zhu wrote:
> [...]
>> @@ -299,6 +312,16 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
>> return PTR_ERR(csi2rx->p_clk);
>> }
>>
>> + csi2rx->sys_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>> + "sys_rst");
>
> This doesn't match the bindings documented in patch 2.
> Should this be "sys"?

Yes, will fix it.

>
>> + if (IS_ERR(csi2rx->sys_rst))
>> + return PTR_ERR(csi2rx->sys_rst);
>> +
>> + csi2rx->p_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>> + "p_rst");
>
> This doesn't match the bindings documented in patch 2.
> Should this be "reg_bank"?
>

Yes, will fix it.

>> + if (IS_ERR(csi2rx->p_rst))
>> + return PTR_ERR(csi2rx->p_rst);
>> +
>> csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
>> if (IS_ERR(csi2rx->dphy)) {
>> dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
>> @@ -349,14 +372,21 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
>> }
>>
>> for (i = 0; i < csi2rx->max_streams; i++) {
>> - char clk_name[16];
>> + char name[16];
>>
>> - snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
>> - csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
>> + snprintf(name, sizeof(name), "pixel_if%u_clk", i);
>> + csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, name);
>> if (IS_ERR(csi2rx->pixel_clk[i])) {
>> - dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
>> + dev_err(&pdev->dev, "Couldn't get clock %s\n", name);
>> return PTR_ERR(csi2rx->pixel_clk[i]);
>> }
>> +
>> + snprintf(name, sizeof(name), "pixel_if%u_rst", i);
>
> This doesn't match the bindings documented in patch 2.
> Should this be "pixel_if%u"?
>

Yes, will fix it.

regards
Jack

> regards
> Philipp