* [PATCH v2 0/3] Add Rockchip RK3288 support
@ 2020-04-06 7:30 Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Karthik Poduval @ 2020-04-06 7:30 UTC (permalink / raw)
To: linux-media; +Cc: Karthik Poduval
Here are the series of patches adding Rockchip RK3288 chipset support to the
Rockchip ISP1 driver and Rockchip MIPI DPHY RX0 driver. These patches have been
worked by referring to tinkerbaord debian kernel posted at:-
https://github.com/TinkerBoard/debian_kernel.git
They have been tested on the tinkerbaord using a ov5647 camera
Example test command (libcamera test tool):
cam -c 1 -C -F cap
Some modifications were required to the ov5647 driver to make it work with
libcamera which were derived from git://gitlab.collabora.com/koike/linux.git
rockchip/isp/v12 branch.
Those ov5647 driver changes are not a part of this patch series.
Karthik Poduval (3):
media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY
driver
media: staging: rkisp1: add rk3288 support
ARM: dts: rockchip: add rk3288 ISP and DPHY
arch/arm/boot/dts/rk3288.dtsi | 25 +++++++
.../bindings/phy/rockchip-mipi-dphy-rx0.yaml | 1 +
.../phy-rockchip-dphy-rx0.c | 69 +++++++++++++++++++
.../bindings/media/rockchip-isp1.yaml | 1 +
drivers/staging/media/rkisp1/rkisp1-dev.c | 18 +++++
5 files changed, 114 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver
2020-04-06 7:30 [PATCH v2 0/3] Add Rockchip RK3288 support Karthik Poduval
@ 2020-04-06 7:30 ` Karthik Poduval
2020-04-06 16:56 ` Helen Koike
2020-04-07 0:27 ` Ezequiel Garcia
2020-04-06 7:30 ` [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY Karthik Poduval
2 siblings, 2 replies; 16+ messages in thread
From: Karthik Poduval @ 2020-04-06 7:30 UTC (permalink / raw)
To: linux-media; +Cc: Karthik Poduval
add rk3288 support to Rockchip DPHY driver
tested on tinkerbaord with ov5647 using command
cam -c 1 -C -F cap
Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
---
.../bindings/phy/rockchip-mipi-dphy-rx0.yaml | 1 +
.../phy-rockchip-dphy-rx0.c | 69 +++++++++++++++++++
2 files changed, 70 insertions(+)
diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
index 5dacece35702..8927c36de532 100644
--- a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
+++ b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
@@ -17,6 +17,7 @@ description: |
properties:
compatible:
const: rockchip,rk3399-mipi-dphy-rx0
+ const: rockchip,rk3288-mipi-dphy-rx0
reg:
maxItems: 1
diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
index 7c4df6d48c43..36fc1b1ee7ae 100644
--- a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
+++ b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
@@ -26,6 +26,15 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#define RK3288_GRF_SOC_CON6 0x025c
+#define RK3288_GRF_SOC_CON8 0x0264
+#define RK3288_GRF_SOC_CON9 0x0268
+#define RK3288_GRF_SOC_CON10 0x026c
+#define RK3288_GRF_SOC_CON14 0x027c
+#define RK3288_GRF_SOC_STATUS21 0x02d4
+#define RK3288_GRF_IO_VSEL 0x0380
+#define RK3288_GRF_SOC_CON15 0x03a4
+
#define RK3399_GRF_SOC_CON9 0x6224
#define RK3399_GRF_SOC_CON21 0x6254
#define RK3399_GRF_SOC_CON22 0x6258
@@ -47,6 +56,19 @@ struct hsfreq_range {
u8 cfg_bit;
};
+static const struct hsfreq_range rk3288_mipidphy_hsfreq_ranges[] = {
+ { 89, 0x00}, { 99, 0x10}, { 109, 0x20}, { 129, 0x01},
+ { 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
+ { 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
+ { 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
+ { 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
+ { 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
+ { 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
+ { 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
+ {1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
+ {1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
+};
+
static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
{ 89, 0x00 }, { 99, 0x10 }, { 109, 0x20 }, { 129, 0x01 },
{ 139, 0x11 }, { 149, 0x21 }, { 169, 0x02 }, { 179, 0x12 },
@@ -60,6 +82,11 @@ static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
{ 1399, 0x1c }, { 1449, 0x2c }, { 1500, 0x3c }
};
+static const char * const rk3288_mipidphy_clks[] = {
+ "dphy-ref",
+ "pclk",
+};
+
static const char * const rk3399_mipidphy_clks[] = {
"dphy-ref",
"dphy-cfg",
@@ -109,6 +136,36 @@ struct dphy_reg {
#define PHY_REG(_offset, _width, _shift) \
{ .offset = _offset, .mask = BIT(_width) - 1, .shift = _shift, }
+static const struct dphy_reg rk3288_grf_dphy_regs[] = {
+ [GRF_CON_DISABLE_ISP] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 0),
+ [GRF_CON_ISP_DPHY_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 1),
+ [GRF_DSI_CSI_TESTBUS_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 14),
+ [GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 0),
+ [GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 4),
+ [GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 8),
+ [GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 0),
+ [GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 4),
+ [GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 8),
+ [GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 12),
+ [GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 0),
+ [GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 4),
+ [GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 8),
+ [GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 12),
+ [GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 0),
+ [GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 1),
+ [GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 2),
+ [GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3288_GRF_SOC_CON14, 8, 3),
+ [GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 12),
+ [GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 13),
+ [GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 14),
+ [GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 15),
+ [GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 0),
+ [GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 4),
+ [GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 3, 8),
+ [GRF_DVP_V18SEL] = PHY_REG(RK3288_GRF_IO_VSEL, 1, 1),
+ [GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3288_GRF_SOC_STATUS21, 8, 0),
+};
+
static const struct dphy_reg rk3399_grf_dphy_regs[] = {
[GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON9, 4, 0),
[GRF_DPHY_RX0_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 10),
@@ -303,6 +360,14 @@ static const struct phy_ops rk_dphy_ops = {
.owner = THIS_MODULE,
};
+static const struct rk_dphy_drv_data rk3288_mipidphy_drv_data = {
+ .clks = rk3288_mipidphy_clks,
+ .num_clks = ARRAY_SIZE(rk3288_mipidphy_clks),
+ .hsfreq_ranges = rk3288_mipidphy_hsfreq_ranges,
+ .num_hsfreq_ranges = ARRAY_SIZE(rk3288_mipidphy_hsfreq_ranges),
+ .regs = rk3288_grf_dphy_regs,
+};
+
static const struct rk_dphy_drv_data rk3399_mipidphy_drv_data = {
.clks = rk3399_mipidphy_clks,
.num_clks = ARRAY_SIZE(rk3399_mipidphy_clks),
@@ -312,6 +377,10 @@ static const struct rk_dphy_drv_data rk3399_mipidphy_drv_data = {
};
static const struct of_device_id rk_dphy_dt_ids[] = {
+ {
+ .compatible = "rockchip,rk3288-mipi-dphy-rx0",
+ .data = &rk3288_mipidphy_drv_data,
+ },
{
.compatible = "rockchip,rk3399-mipi-dphy-rx0",
.data = &rk3399_mipidphy_drv_data,
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support
2020-04-06 7:30 [PATCH v2 0/3] Add Rockchip RK3288 support Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
@ 2020-04-06 7:30 ` Karthik Poduval
2020-04-06 16:57 ` Helen Koike
2020-04-06 7:30 ` [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY Karthik Poduval
2 siblings, 1 reply; 16+ messages in thread
From: Karthik Poduval @ 2020-04-06 7:30 UTC (permalink / raw)
To: linux-media; +Cc: Karthik Poduval
Add rk3288 support to the rkisp1 driver.
tested on tinkerbaord with ov5647 using command
cam -c 1 -C -F cap
Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
---
.../bindings/media/rockchip-isp1.yaml | 1 +
drivers/staging/media/rkisp1/rkisp1-dev.c | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)
diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index af246b71eac6..4c31294cf14b 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -16,6 +16,7 @@ description: |
properties:
compatible:
const: rockchip,rk3399-cif-isp
+ const: rockchip,rk3288-rkisp1
reg:
maxItems: 1
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index b1b3c058e957..1df4f5906fd0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -403,6 +403,20 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
return IRQ_HANDLED;
}
+
+static const char * const rk3288_isp_clks[] = {
+ "clk_isp",
+ "aclk_isp",
+ "hclk_isp",
+ "pclk_isp_in",
+ "sclk_isp_jpe",
+};
+
+static const struct rkisp1_match_data rk3288_isp_clk_data = {
+ .clks = rk3288_isp_clks,
+ .size = ARRAY_SIZE(rk3288_isp_clks),
+};
+
static const char * const rk3399_isp_clks[] = {
"clk_isp",
"aclk_isp",
@@ -417,6 +431,10 @@ static const struct rkisp1_match_data rk3399_isp_clk_data = {
};
static const struct of_device_id rkisp1_of_match[] = {
+ {
+ .compatible = "rockchip,rk3288-rkisp1",
+ .data = &rk3288_isp_clk_data,
+ },
{
.compatible = "rockchip,rk3399-cif-isp",
.data = &rk3399_isp_clk_data,
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-06 7:30 [PATCH v2 0/3] Add Rockchip RK3288 support Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support Karthik Poduval
@ 2020-04-06 7:30 ` Karthik Poduval
2020-04-06 16:57 ` Helen Koike
2020-04-09 1:19 ` Johan Jonker
2 siblings, 2 replies; 16+ messages in thread
From: Karthik Poduval @ 2020-04-06 7:30 UTC (permalink / raw)
To: linux-media; +Cc: Karthik Poduval
ISP and DPHY device entries missing so add them.
tested on tinkerbaord with ov5647 using command
cam -c 1 -C -F cap
Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
---
arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 9beb662166aa..adea8189abd9 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -247,6 +247,23 @@
ports = <&vopl_out>, <&vopb_out>;
};
+ isp: isp@ff910000 {
+ compatible = "rockchip,rk3288-rkisp1";
+ reg = <0x0 0xff910000 0x0 0x4000>;
+ interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
+ <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
+ <&cru SCLK_ISP_JPE>;
+ clock-names = "clk_isp", "aclk_isp",
+ "hclk_isp", "pclk_isp_in",
+ "sclk_isp_jpe";
+ assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
+ assigned-clock-rates = <400000000>, <400000000>;
+ power-domains = <&power RK3288_PD_VIO>;
+ iommus = <&isp_mmu>;
+ status = "disabled";
+ };
+
sdmmc: mmc@ff0c0000 {
compatible = "rockchip,rk3288-dw-mshc";
max-frequency = <150000000>;
@@ -891,6 +908,14 @@
status = "disabled";
};
+ mipi_phy_rx0: mipi-phy-rx0 {
+ compatible = "rockchip,rk3288-mipi-dphy-rx0";
+ clocks = <&cru SCLK_MIPIDSI_24M>, <&cru PCLK_MIPI_CSI>;
+ clock-names = "dphy-ref", "pclk";
+ #phy-cells = <0>;
+ status = "disabled";
+ };
+
io_domains: io-domains {
compatible = "rockchip,rk3288-io-voltage-domain";
status = "disabled";
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
@ 2020-04-06 16:56 ` Helen Koike
2020-04-07 0:27 ` Ezequiel Garcia
1 sibling, 0 replies; 16+ messages in thread
From: Helen Koike @ 2020-04-06 16:56 UTC (permalink / raw)
To: Karthik Poduval, linux-media
Hi Karthik,
Thanks for your patch.
On 4/6/20 4:30 AM, Karthik Poduval wrote:
> add rk3288 support to Rockchip DPHY driver
>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
>
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
As I mentioned in the previous version, this Reported-by tag doesn't make much sense to me here, please check
Documentation/process/submitting-patches.rst
Same for the other commits in this series.
Let me know if you have any question regarding this.
> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
> .../bindings/phy/rockchip-mipi-dphy-rx0.yaml | 1 +
> .../phy-rockchip-dphy-rx0.c | 69 +++++++++++++++++++
> 2 files changed, 70 insertions(+)
>
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> index 5dacece35702..8927c36de532 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
Please, update bindings in a separated patchset.
> @@ -17,6 +17,7 @@ description: |
> properties:
> compatible:
> const: rockchip,rk3399-mipi-dphy-rx0
> + const: rockchip,rk3288-mipi-dphy-rx0
>
> reg:
> maxItems: 1
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> index 7c4df6d48c43..36fc1b1ee7ae 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> @@ -26,6 +26,15 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
>
> +#define RK3288_GRF_SOC_CON6 0x025c
> +#define RK3288_GRF_SOC_CON8 0x0264
> +#define RK3288_GRF_SOC_CON9 0x0268
> +#define RK3288_GRF_SOC_CON10 0x026c
> +#define RK3288_GRF_SOC_CON14 0x027c
> +#define RK3288_GRF_SOC_STATUS21 0x02d4
> +#define RK3288_GRF_IO_VSEL 0x0380
> +#define RK3288_GRF_SOC_CON15 0x03a4
> +
> #define RK3399_GRF_SOC_CON9 0x6224
> #define RK3399_GRF_SOC_CON21 0x6254
> #define RK3399_GRF_SOC_CON22 0x6258
> @@ -47,6 +56,19 @@ struct hsfreq_range {
> u8 cfg_bit;
> };
>
> +static const struct hsfreq_range rk3288_mipidphy_hsfreq_ranges[] = {
> + { 89, 0x00}, { 99, 0x10}, { 109, 0x20}, { 129, 0x01},
> + { 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
> + { 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
> + { 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
> + { 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
> + { 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
> + { 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
> + { 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
> + {1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
> + {1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
> +};
> +
> static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> { 89, 0x00 }, { 99, 0x10 }, { 109, 0x20 }, { 129, 0x01 },
> { 139, 0x11 }, { 149, 0x21 }, { 169, 0x02 }, { 179, 0x12 },
> @@ -60,6 +82,11 @@ static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> { 1399, 0x1c }, { 1449, 0x2c }, { 1500, 0x3c }
> };
>
> +static const char * const rk3288_mipidphy_clks[] = {
> + "dphy-ref",
> + "pclk",
> +};
clock-names needs to be updated in the bindings as well.
Regards,
Helen
> +
> static const char * const rk3399_mipidphy_clks[] = {
> "dphy-ref",
> "dphy-cfg",
> @@ -109,6 +136,36 @@ struct dphy_reg {
> #define PHY_REG(_offset, _width, _shift) \
> { .offset = _offset, .mask = BIT(_width) - 1, .shift = _shift, }
>
> +static const struct dphy_reg rk3288_grf_dphy_regs[] = {
> + [GRF_CON_DISABLE_ISP] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 0),
> + [GRF_CON_ISP_DPHY_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 1),
> + [GRF_DSI_CSI_TESTBUS_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 14),
> + [GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 0),
> + [GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 4),
> + [GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 8),
> + [GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 0),
> + [GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 4),
> + [GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 8),
> + [GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 12),
> + [GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 0),
> + [GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 4),
> + [GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 8),
> + [GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 12),
> + [GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 0),
> + [GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 1),
> + [GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 2),
> + [GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3288_GRF_SOC_CON14, 8, 3),
> + [GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 12),
> + [GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 13),
> + [GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 14),
> + [GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 15),
> + [GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 0),
> + [GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 4),
> + [GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 3, 8),
> + [GRF_DVP_V18SEL] = PHY_REG(RK3288_GRF_IO_VSEL, 1, 1),
> + [GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3288_GRF_SOC_STATUS21, 8, 0),
> +};
> +
> static const struct dphy_reg rk3399_grf_dphy_regs[] = {
> [GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON9, 4, 0),
> [GRF_DPHY_RX0_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 10),
> @@ -303,6 +360,14 @@ static const struct phy_ops rk_dphy_ops = {
> .owner = THIS_MODULE,
> };
>
> +static const struct rk_dphy_drv_data rk3288_mipidphy_drv_data = {
> + .clks = rk3288_mipidphy_clks,
> + .num_clks = ARRAY_SIZE(rk3288_mipidphy_clks),
> + .hsfreq_ranges = rk3288_mipidphy_hsfreq_ranges,
> + .num_hsfreq_ranges = ARRAY_SIZE(rk3288_mipidphy_hsfreq_ranges),
> + .regs = rk3288_grf_dphy_regs,
> +};
> +
> static const struct rk_dphy_drv_data rk3399_mipidphy_drv_data = {
> .clks = rk3399_mipidphy_clks,
> .num_clks = ARRAY_SIZE(rk3399_mipidphy_clks),
> @@ -312,6 +377,10 @@ static const struct rk_dphy_drv_data rk3399_mipidphy_drv_data = {
> };
>
> static const struct of_device_id rk_dphy_dt_ids[] = {
> + {
> + .compatible = "rockchip,rk3288-mipi-dphy-rx0",
> + .data = &rk3288_mipidphy_drv_data,
> + },
> {
> .compatible = "rockchip,rk3399-mipi-dphy-rx0",
> .data = &rk3399_mipidphy_drv_data,
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support
2020-04-06 7:30 ` [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support Karthik Poduval
@ 2020-04-06 16:57 ` Helen Koike
0 siblings, 0 replies; 16+ messages in thread
From: Helen Koike @ 2020-04-06 16:57 UTC (permalink / raw)
To: Karthik Poduval, linux-media
Hi Karthik,
Thanks for your patch.
On 4/6/20 4:30 AM, Karthik Poduval wrote:
> Add rk3288 support to the rkisp1 driver.
>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
>
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
Please, see my comment from previous patch regarding this tag.
> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
> .../bindings/media/rockchip-isp1.yaml | 1 +
> drivers/staging/media/rkisp1/rkisp1-dev.c | 18 ++++++++++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> index af246b71eac6..4c31294cf14b 100644
> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
Please, update bindings in a separated patchset.
> @@ -16,6 +16,7 @@ description: |
> properties:
> compatible:
> const: rockchip,rk3399-cif-isp
> + const: rockchip,rk3288-rkisp1
Please, keep consistency, or rk3288-cif-isp, or we change rk3399-cif-isp to be rk3399-rkisp1.
>
> reg:
> maxItems: 1
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index b1b3c058e957..1df4f5906fd0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -403,6 +403,20 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
> return IRQ_HANDLED;
> }
>
> +
Checkpatch error:
drivers/staging/media/rkisp1/rkisp1-dev.c:406: check: Please don't use multiple blank lines
> +static const char * const rk3288_isp_clks[] = {
> + "clk_isp",
> + "aclk_isp",
> + "hclk_isp",
> + "pclk_isp_in",
> + "sclk_isp_jpe",
> +};
You should also update clock-names in the bindings as well.
Regards,
Helen
> +
> +static const struct rkisp1_match_data rk3288_isp_clk_data = {
> + .clks = rk3288_isp_clks,
> + .size = ARRAY_SIZE(rk3288_isp_clks),
> +};
> +
> static const char * const rk3399_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> @@ -417,6 +431,10 @@ static const struct rkisp1_match_data rk3399_isp_clk_data = {
> };
>
> static const struct of_device_id rkisp1_of_match[] = {
> + {
> + .compatible = "rockchip,rk3288-rkisp1",
> + .data = &rk3288_isp_clk_data,
> + },
> {
> .compatible = "rockchip,rk3399-cif-isp",
> .data = &rk3399_isp_clk_data,
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-06 7:30 ` [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY Karthik Poduval
@ 2020-04-06 16:57 ` Helen Koike
2020-04-09 1:19 ` Johan Jonker
1 sibling, 0 replies; 16+ messages in thread
From: Helen Koike @ 2020-04-06 16:57 UTC (permalink / raw)
To: Karthik Poduval, linux-media
Hi Karthik,
On 4/6/20 4:30 AM, Karthik Poduval wrote:
> ISP and DPHY device entries missing so add them.>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
>
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
Please see my previous comment regarding this tag.
> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 9beb662166aa..adea8189abd9 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -247,6 +247,23 @@
> ports = <&vopl_out>, <&vopb_out>;
> };
>
> + isp: isp@ff910000 {
> + compatible = "rockchip,rk3288-rkisp1";
> + reg = <0x0 0xff910000 0x0 0x4000>;
> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
> + <&cru SCLK_ISP_JPE>;
> + clock-names = "clk_isp", "aclk_isp",
> + "hclk_isp", "pclk_isp_in",
> + "sclk_isp_jpe";
> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
> + assigned-clock-rates = <400000000>, <400000000>;
These two items are not in the bindings, are they required? Or should we modify the bindings?
> + power-domains = <&power RK3288_PD_VIO>;
> + iommus = <&isp_mmu>;
> + status = "disabled";
> + };
> +
> sdmmc: mmc@ff0c0000 {
> compatible = "rockchip,rk3288-dw-mshc";
> max-frequency = <150000000>;
> @@ -891,6 +908,14 @@
> status = "disabled";
> };
>
> + mipi_phy_rx0: mipi-phy-rx0 {
> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru PCLK_MIPI_CSI>;
> + clock-names = "dphy-ref", "pclk";
> + #phy-cells = <0>;
> + status = "disabled";
According to the bindings you also need power-domains here.
hmm, now I wonder if it is required or bindings which needs to
be updated (to be verified).
Also, I would separate this in two patches.
fyi, I'm moving bindings out of staging https://patchwork.linuxtv.org/project/linux-media/list/?series=2103
Thanks,
Helen
> + };
> +
> io_domains: io-domains {
> compatible = "rockchip,rk3288-io-voltage-domain";
> status = "disabled";
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
2020-04-06 16:56 ` Helen Koike
@ 2020-04-07 0:27 ` Ezequiel Garcia
1 sibling, 0 replies; 16+ messages in thread
From: Ezequiel Garcia @ 2020-04-07 0:27 UTC (permalink / raw)
To: Karthik Poduval; +Cc: linux-media
Hey there Karthik,
Thanks for your contribution. I'm happy to see this driver
is used and tested more.
The patch looks good, just some minor comments,
in addition to Helen's comments.
Hopefully you'll be able to nail this on the v3 :-)
It would be really better for us if you could add a cover letter
(i.e. PATCH 0/3) in this series. You would be able to put
important information there such as testing information,
series changelog, so on.
See for instance: https://patchwork.linuxtv.org/cover/62830/
Also, please scripts/get_maintainers.pl next time as indication
of who to Cc your patches. For instance, I am the author
of this driver, but I'm not Cced, so I could have missed your patch.
Finally, please put a note at the top of the driver,
explaining where the RK3288 support comes from.
Something like this would work:
"'"
RK3288 support based on <file> in <git repo>, branch <b>"
"""
See below a few more comments.
On Mon, 6 Apr 2020 at 04:30, Karthik Poduval <karthik.poduval@gmail.com> wrote:
>
> add rk3288 support to Rockchip DPHY driver
>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
>
Nitpick: s/tinkerbaord/tinkerboard
I'd like to have proper sentences in the commit log.
This means, sentences start with capital letters and end
with a stop.
The testing information is normally left for the cover letter.
Also, keep in mind you will need to add proper context
here: for instance, where does the "cam -c 1 -C -F cap"
command come from? I guess it's libcamera, you should
mention this.
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
As Helen already pointed out, if you are Reported-by is
incorrectly used here. Please read Documentation/process/submitting-patches.rst.
Helen mentioned this on the v1. Please make sure you
go thru all the feedback you get on each version,
before posting a new one. There's no rush :-)
> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
> .../bindings/phy/rockchip-mipi-dphy-rx0.yaml | 1 +
> .../phy-rockchip-dphy-rx0.c | 69 +++++++++++++++++++
> 2 files changed, 70 insertions(+)
>
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> index 5dacece35702..8927c36de532 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> @@ -17,6 +17,7 @@ description: |
> properties:
> compatible:
> const: rockchip,rk3399-mipi-dphy-rx0
> + const: rockchip,rk3288-mipi-dphy-rx0
>
> reg:
> maxItems: 1
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> index 7c4df6d48c43..36fc1b1ee7ae 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> @@ -26,6 +26,15 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
>
> +#define RK3288_GRF_SOC_CON6 0x025c
> +#define RK3288_GRF_SOC_CON8 0x0264
> +#define RK3288_GRF_SOC_CON9 0x0268
> +#define RK3288_GRF_SOC_CON10 0x026c
> +#define RK3288_GRF_SOC_CON14 0x027c
> +#define RK3288_GRF_SOC_STATUS21 0x02d4
> +#define RK3288_GRF_IO_VSEL 0x0380
> +#define RK3288_GRF_SOC_CON15 0x03a4
> +
> #define RK3399_GRF_SOC_CON9 0x6224
> #define RK3399_GRF_SOC_CON21 0x6254
> #define RK3399_GRF_SOC_CON22 0x6258
> @@ -47,6 +56,19 @@ struct hsfreq_range {
> u8 cfg_bit;
> };
>
> +static const struct hsfreq_range rk3288_mipidphy_hsfreq_ranges[] = {
> + { 89, 0x00}, { 99, 0x10}, { 109, 0x20}, { 129, 0x01},
> + { 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
> + { 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
> + { 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
> + { 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
> + { 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
> + { 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
> + { 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
> + {1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
> + {1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
> +};
> +
> static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> { 89, 0x00 }, { 99, 0x10 }, { 109, 0x20 }, { 129, 0x01 },
> { 139, 0x11 }, { 149, 0x21 }, { 169, 0x02 }, { 179, 0x12 },
> @@ -60,6 +82,11 @@ static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> { 1399, 0x1c }, { 1449, 0x2c }, { 1500, 0x3c }
> };
>
> +static const char * const rk3288_mipidphy_clks[] = {
> + "dphy-ref",
> + "pclk",
> +};
> +
... are you sure that you need this pclk? The driver
doesn't seem to be accessing any peripheral interface.
Can you test without requesting the pclk ?
Stay safe!
Ezequiel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-06 7:30 ` [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY Karthik Poduval
2020-04-06 16:57 ` Helen Koike
@ 2020-04-09 1:19 ` Johan Jonker
2020-04-12 5:13 ` karthik poduval
1 sibling, 1 reply; 16+ messages in thread
From: Johan Jonker @ 2020-04-09 1:19 UTC (permalink / raw)
To: Karthik Poduval, linux-media, heiko, linux-rockchip, helen.koike,
ezequiel
Hi Karthik and others,
Include all mail lists found with:
./scripts/get_maintainer.pl --nogit-fallback --nogit
Helen is moving isp1 bindings out of staging.
Clocks and other things don't fit with her patch serie.
Maybe fix this while still in staging?
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
'phys' is a required property
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
'phy-names' is a required property
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
'ports' is a required property
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
'assigned-clock-rates', 'assigned-clocks'
do not match any of the regexes: 'pinctrl-[0-9]+'
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
clock-names:2: 'aclk_isp_wrap' was expected
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
clock-names:3: 'hclk_isp' was expected
arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
clock-names:4: 'hclk_isp_wrap' was expected
arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
is a required property
arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
'dphy-cfg' was expected
arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
['dphy-ref', 'pclk'] is too short
arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
126], [7, 358]] is too short
Inside yaml:
Use enum and sort.
>> properties:
>> compatible:
>> const: rockchip,rk3399-cif-isp
>> + const: rockchip,rk3288-rkisp1
enum:
- rockchip,rk3288-rkisp1
- rockchip,rk3399-cif-isp
>> properties:
>> compatible:
>> const: rockchip,rk3399-mipi-dphy-rx0
>> + const: rockchip,rk3288-mipi-dphy-rx0
enum:
- rockchip,rk3288-mipi-dphy-rx0
- rockchip,rk3399-mipi-dphy-rx0
>
> Please, keep consistency, or rk3288-cif-isp, or we change rk3399-cif-isp to be rk3399-rkisp1.
On 4/6/20 9:30 AM, Karthik Poduval wrote:
> ISP and DPHY device entries missing so add them.
>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
Disclose dts node for ov5647 in cover letter, so people can reproduce
this experiment.
Caution!
Without dts node this command doesn't work correct.
make ARCH=arm dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
make ARCH=arm dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
Needed to detect missing: phys, phy-names and ports ,etc.
&isp {
status = "okay";
};
Needed to detect missing: power-domains, etc.
&mipi_phy_rx0 {
status = "okay";
};
>
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 9beb662166aa..adea8189abd9 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -247,6 +247,23 @@
> ports = <&vopl_out>, <&vopb_out>;
> };
>
> + isp: isp@ff910000 {
For nodes:
Sort things without reg alphabetical first,
then sort the rest by reg address.
> + compatible = "rockchip,rk3288-rkisp1";
> + reg = <0x0 0xff910000 0x0 0x4000>;
> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
> + <&cru SCLK_ISP_JPE>;
> + clock-names = "clk_isp", "aclk_isp",
> + "hclk_isp", "pclk_isp_in",
> + "sclk_isp_jpe";
> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
> + assigned-clock-rates = <400000000>, <400000000>;
> + power-domains = <&power RK3288_PD_VIO>;
> + iommus = <&isp_mmu>;
sort
Something missing?
Where are the ports and port nodes?
> + status = "disabled";
> + };
> +
> sdmmc: mmc@ff0c0000 {
> compatible = "rockchip,rk3288-dw-mshc";
> max-frequency = <150000000>;
> @@ -891,6 +908,14 @@
> status = "disabled";
> };
>
> + mipi_phy_rx0: mipi-phy-rx0 {
Use separate patch.
For nodes:
Sort things without reg alphabetical first,
then sort the rest by reg address.
> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru PCLK_MIPI_CSI>;
> + clock-names = "dphy-ref", "pclk";
Something missing?
Does this phy have a power domain?
> + #phy-cells = <0>;
> + status = "disabled";
> + };
> +
> io_domains: io-domains {
> compatible = "rockchip,rk3288-io-voltage-domain";
> status = "disabled";
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-09 1:19 ` Johan Jonker
@ 2020-04-12 5:13 ` karthik poduval
[not found] ` <CAFP0Ok9XGzVbghbnOOyfXiOOc5-a94uFRu7sD5wXz9sr-+AYEA@mail.gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: karthik poduval @ 2020-04-12 5:13 UTC (permalink / raw)
To: Johan Jonker
Cc: Linux Media Mailing List, heiko, linux-rockchip, Helen Koike,
Ezequiel Garcia
Thanks Johan for your valuable comments.
On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi Karthik and others,
>
> Include all mail lists found with:
> ./scripts/get_maintainer.pl --nogit-fallback --nogit
>
> Helen is moving isp1 bindings out of staging.
> Clocks and other things don't fit with her patch serie.
> Maybe fix this while still in staging?
>
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> 'phys' is a required property
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> 'phy-names' is a required property
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> 'ports' is a required property
>
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> 'assigned-clock-rates', 'assigned-clocks'
> do not match any of the regexes: 'pinctrl-[0-9]+'
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> clock-names:2: 'aclk_isp_wrap' was expected
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> clock-names:3: 'hclk_isp' was expected
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> clock-names:4: 'hclk_isp_wrap' was expected
>
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
> is a required property
>
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
> 'dphy-cfg' was expected
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
> ['dphy-ref', 'pclk'] is too short
> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
> 126], [7, 358]] is too short
>
>
> Inside yaml:
> Use enum and sort.
> >> properties:
> >> compatible:
>
> >> const: rockchip,rk3399-cif-isp
> >> + const: rockchip,rk3288-rkisp1
>
> enum:
> - rockchip,rk3288-rkisp1
> - rockchip,rk3399-cif-isp
>
> >> properties:
> >> compatible:
> >> const: rockchip,rk3399-mipi-dphy-rx0
> >> + const: rockchip,rk3288-mipi-dphy-rx0
>
> enum:
> - rockchip,rk3288-mipi-dphy-rx0
> - rockchip,rk3399-mipi-dphy-rx0
>
> >
> > Please, keep consistency, or rk3288-cif-isp, or we change rk3399-cif-isp to be rk3399-rkisp1.
>
>
> On 4/6/20 9:30 AM, Karthik Poduval wrote:
> > ISP and DPHY device entries missing so add them.
> >
>
> > tested on tinkerbaord with ov5647 using command
> > cam -c 1 -C -F cap
>
> Disclose dts node for ov5647 in cover letter, so people can reproduce
> this experiment.
>
> Caution!
> Without dts node this command doesn't work correct.
>
> make ARCH=arm dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>
> make ARCH=arm dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
>
> Needed to detect missing: phys, phy-names and ports ,etc.
>
> &isp {
> status = "okay";
> };
>
Makes sense, that's why my kernel compilation passed and I didn't
these errors. Thanks for the command, I will verify dts for
correctness in next patch series.
> Needed to detect missing: power-domains, etc.
>
> &mipi_phy_rx0 {
> status = "okay";
> };
>
> >
> > Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
> > Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> > ---
> > arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> > index 9beb662166aa..adea8189abd9 100644
> > --- a/arch/arm/boot/dts/rk3288.dtsi
> > +++ b/arch/arm/boot/dts/rk3288.dtsi
> > @@ -247,6 +247,23 @@
> > ports = <&vopl_out>, <&vopb_out>;
> > };
> >
>
> > + isp: isp@ff910000 {
>
> For nodes:
> Sort things without reg alphabetical first,
> then sort the rest by reg address.
>
Sure
> > + compatible = "rockchip,rk3288-rkisp1";
> > + reg = <0x0 0xff910000 0x0 0x4000>;
> > + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
> > + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
> > + <&cru SCLK_ISP_JPE>;
> > + clock-names = "clk_isp", "aclk_isp",
> > + "hclk_isp", "pclk_isp_in",
> > + "sclk_isp_jpe";
> > + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
> > + assigned-clock-rates = <400000000>, <400000000>;
>
> > + power-domains = <&power RK3288_PD_VIO>;
> > + iommus = <&isp_mmu>;
>
> sort
>
> Something missing?
> Where are the ports and port nodes?
I was assuming that this would be a part of the board specific dtsi or
dts entry where the isp node would be overriden and the i2c camera
port
and the isp port remote-endpoints would be connected. I had this as a
part of my first patch series. However I was advised by Helen to not
include the ov5647
dtsi as it isn't hardwired to the SoC and resides on an camera adapter board.
Should this be defined without the remote-endpoint phandle since we
don't know exactly which sensor gets connected in this dtsi file ?
>
> > + status = "disabled";
> > + };
> > +
> > sdmmc: mmc@ff0c0000 {
> > compatible = "rockchip,rk3288-dw-mshc";
> > max-frequency = <150000000>;
> > @@ -891,6 +908,14 @@
> > status = "disabled";
> > };
> >
>
> > + mipi_phy_rx0: mipi-phy-rx0 {
>
> Use separate patch.
>
> For nodes:
> Sort things without reg alphabetical first,
> then sort the rest by reg address.
>
Sure
> > + compatible = "rockchip,rk3288-mipi-dphy-rx0";
> > + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru PCLK_MIPI_CSI>;
> > + clock-names = "dphy-ref", "pclk";
> Something missing?
> Does this phy have a power domain?
The tinkerboard debian kernel (where I referred to for this patch)
didn't have it defined for the DPHY. I would guess that it would be
the same as the ISP i.e. RK3288_PD_VIO,
does anyone know the answer to this or do I have to reach out to
Rockchip engineering ?
>
> > + #phy-cells = <0>;
> > + status = "disabled";
> > + };
> > +
> > io_domains: io-domains {
> > compatible = "rockchip,rk3288-io-voltage-domain";
> > status = "disabled";
> >
>
--
Regards,
Karthik Poduval
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
[not found] ` <CAFP0Ok9XGzVbghbnOOyfXiOOc5-a94uFRu7sD5wXz9sr-+AYEA@mail.gmail.com>
@ 2020-04-23 11:12 ` Johan Jonker
2020-05-14 13:19 ` karthik poduval
2020-07-02 16:27 ` Helen Koike
0 siblings, 2 replies; 16+ messages in thread
From: Johan Jonker @ 2020-04-23 11:12 UTC (permalink / raw)
To: karthik poduval
Cc: Linux Media Mailing List, heiko, linux-rockchip, Helen Koike,
Ezequiel Garcia, Rob Herring, devicetree
Hi robh, Heiko, Karthik, Helen and others,
See comments below.
Should we change Helen's patch serie a little bit to accommodate other
isp1 compatibles as well? Could you give advise here? Thanks!
Johan
On 4/23/20 7:10 AM, karthik poduval wrote:
> Hi Johan/Helen,
>
> I was attempting to fix the yaml to work for both platforms rk3288 and
> rk3399. I couldn't find any specific example in the existing yaml files
> that deal with this exact scenario common driver but different clocks for
> different chipsets. Could you point me to an appropriate example ?
>
> Meanwhile here is the patch I was trying out.
> diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> index af246b71eac6..4ca76a1bbb63 100644
> --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> @@ -15,7 +15,11 @@ description: |
>
> properties:
> compatible:
> - const: rockchip,rk3399-cif-isp
> + items:
> + - enum:
> + - rockchip,rk3288-cif-isp
> + - rockchip,rk3399-cif-isp
> + - const: rockchip,rk3399-cif-isp
>
> reg:
> maxItems: 1
> @@ -37,20 +41,38 @@ properties:
> const: dphy
>
> clocks:
> - items:
> - - description: ISP clock
> - - description: ISP AXI clock clock
> - - description: ISP AXI clock wrapper clock
> - - description: ISP AHB clock clock
> - - description: ISP AHB wrapper clock
> + oneOf:
> + # rk3399 clocks
> + - items:
> + - description: ISP clock
> + - description: ISP AXI clock clock
> + - description: ISP AXI clock wrapper clock
> + - description: ISP AHB clock clock
> + - description: ISP AHB wrapper clock
> + # rk3288 clocks
> + - items:
> + - description: ISP clock
> + - description: ISP AXI clock clock
> + - description: ISP AHB clock clock
> + - description: ISP Pixel clock
> + - description: ISP JPEG source clock
>
We can expect a few more clocks here, so just use:
clocks:
minItems: 4
maxItems: 5
or
See question for Helen about 'pclk_isp_wrap':
clocks:
minItems: 4
maxItems: 6
From Rockchip tree:
static const char * const rk1808_isp_clks[] = {
"clk_isp",
"aclk_isp",
"hclk_isp",
"pclk_isp",
};
static const char * const rk3288_isp_clks[] = {
"clk_isp",
"aclk_isp",
"hclk_isp",
"pclk_isp_in",
"sclk_isp_jpe",
};
static const char * const rk3326_isp_clks[] = {
"clk_isp",
"aclk_isp",
"hclk_isp",
"pclk_isp",
};
static const char * const rk3368_isp_clks[] = {
"clk_isp",
"aclk_isp",
"hclk_isp",
"pclk_isp",
};
static const char * const rk3399_isp_clks[] = {
"clk_isp",
"aclk_isp",
"hclk_isp",
"aclk_isp_wrap",
"hclk_isp_wrap",
"pclk_isp_wrap"
};
Question for Helen:
Where did 'pclk_isp_wrap' go in your patch serie?
> clock-names:
> - items:
> - - const: clk_isp
> - - const: aclk_isp
> - - const: aclk_isp_wrap
> - - const: hclk_isp
> - - const: hclk_isp_wrap
> + oneOf:
> + # rk3399 clocks
sort on SoC
> + - items:
> + - const: clk_isp
> + - const: aclk_isp
> + - const: aclk_isp_wrap
> + - const: hclk_isp
> + - const: hclk_isp_wrap
> + # rk3288 clocks
sort on SoC
> + - items:
> + - const: clk_isp
> + - const: aclk_isp
> + - const: hclk_isp
> + - const: pclk_isp_in
> + - const: sclk_isp_jpe
clock-names:
oneOf:
# rk3288 clocks
- items:
- const: clk_isp
description: ISP clock
- const: aclk_isp
description: ISP AXI clock clock
- const: hclk_isp
description: ISP AHB clock clock
- const: pclk_isp_in
description: ISP Pixel clock
- const: sclk_isp_jpe
description: ISP JPEG source clock
# rk3399 clocks
- items:
- const: clk_isp
description: ISP clock
- const: aclk_isp
description: ISP AXI clock clock
- const: aclk_isp_wrap
description: ISP AXI clock wrapper clock
- const: hclk_isp
description: ISP AHB clock clock
- const: hclk_isp_wrap
description: ISP AHB wrapper clock
Question for robh:
Is this a proper way to add description or is there a beter way?
>
> on running command.
> make ARCH=arm dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>
> I see following messages for dts nodes.
> DTC arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> CHECK arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> isp@ff910000: clocks: [[7, 107], [7, 205], [7, 469], [7, 371], [7, 108]] is
> valid under each of {'additionalItems': False, 'items': [{}, {}, {}, {},
> {}], 'maxItems': 5, 'minItems': 5, 'type': 'array'}, {'additionalItems':
> False, 'items': [{}, {}, {}, {}, {}], 'maxItems': 5, 'minItems': 5, 'type':
> 'array'}
> /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> isp@ff910000: compatible: ['rockchip,rk3288-cif-isp'] is too short
>
> Are these cosidered as error messages ? The "too short" is being alerted
> for the orgianl yaml for rk3399 without any of my chnages.
> Kindly advise.
>
> --
> Regards,
> Karthik Poduval
>
> On Sat, Apr 11, 2020 at 10:13 PM karthik poduval <karthik.poduval@gmail.com>
> wrote:
>
>> Thanks Johan for your valuable comments.
>>
>> On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@gmail.com> wrote:
>>>
>>> Hi Karthik and others,
>>>
>>> Include all mail lists found with:
>>> ./scripts/get_maintainer.pl --nogit-fallback --nogit
>>>
>>> Helen is moving isp1 bindings out of staging.
>>> Clocks and other things don't fit with her patch serie.
>>> Maybe fix this while still in staging?
>>>
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> 'phys' is a required property
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> 'phy-names' is a required property
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> 'ports' is a required property
>>>
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> 'assigned-clock-rates', 'assigned-clocks'
>>> do not match any of the regexes: 'pinctrl-[0-9]+'
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> clock-names:2: 'aclk_isp_wrap' was expected
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> clock-names:3: 'hclk_isp' was expected
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>> clock-names:4: 'hclk_isp_wrap' was expected
>>>
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
>>> is a required property
>>>
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
>>> 'dphy-cfg' was expected
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
>>> ['dphy-ref', 'pclk'] is too short
>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
>>> 126], [7, 358]] is too short
>>>
>>>
>>> Inside yaml:
>>> Use enum and sort.
>>>>> properties:
>>>>> compatible:
>>>
>>>>> const: rockchip,rk3399-cif-isp
>>>>> + const: rockchip,rk3288-rkisp1
>>>
>>> enum:
>>> - rockchip,rk3288-rkisp1
>>> - rockchip,rk3399-cif-isp
>>>
>>>>> properties:
>>>>> compatible:
>>>>> const: rockchip,rk3399-mipi-dphy-rx0
>>>>> + const: rockchip,rk3288-mipi-dphy-rx0
>>>
>>> enum:
>>> - rockchip,rk3288-mipi-dphy-rx0
>>> - rockchip,rk3399-mipi-dphy-rx0
>>>
>>>>
>>>> Please, keep consistency, or rk3288-cif-isp, or we change
>> rk3399-cif-isp to be rk3399-rkisp1.
>>>
>>>
>>> On 4/6/20 9:30 AM, Karthik Poduval wrote:
>>>> ISP and DPHY device entries missing so add them.
>>>>
>>>
>>>> tested on tinkerbaord with ov5647 using command
>>>> cam -c 1 -C -F cap
>>>
>>> Disclose dts node for ov5647 in cover letter, so people can reproduce
>>> this experiment.
>>>
>>> Caution!
>>> Without dts node this command doesn't work correct.
>>>
>>> make ARCH=arm dtbs_check
>>>
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>
>>> make ARCH=arm dtbs_check
>>>
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
>>>
>>> Needed to detect missing: phys, phy-names and ports ,etc.
>>>
>>> &isp {
>>> status = "okay";
>>> };
>>>
>> Makes sense, that's why my kernel compilation passed and I didn't
>> these errors. Thanks for the command, I will verify dts for
>> correctness in next patch series.
>>
>>> Needed to detect missing: power-domains, etc.
>>>
>>> &mipi_phy_rx0 {
>>> status = "okay";
>>> };
>>>
>>>>
>>>> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
>>>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
>>>> ---
>>>> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/rk3288.dtsi
>> b/arch/arm/boot/dts/rk3288.dtsi
>>>> index 9beb662166aa..adea8189abd9 100644
>>>> --- a/arch/arm/boot/dts/rk3288.dtsi
>>>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>>>> @@ -247,6 +247,23 @@
>>>> ports = <&vopl_out>, <&vopb_out>;
>>>> };
>>>>
>>>
>>>> + isp: isp@ff910000 {
>>>
>>> For nodes:
>>> Sort things without reg alphabetical first,
>>> then sort the rest by reg address.
>>>
>> Sure
>>>> + compatible = "rockchip,rk3288-rkisp1";
>>>> + reg = <0x0 0xff910000 0x0 0x4000>;
>>>> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>>> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
>>>> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
>>>> + <&cru SCLK_ISP_JPE>;
>>>> + clock-names = "clk_isp", "aclk_isp",
>>>> + "hclk_isp", "pclk_isp_in",
>>>> + "sclk_isp_jpe";
>>>> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
>>>> + assigned-clock-rates = <400000000>, <400000000>;
>>>
>>>> + power-domains = <&power RK3288_PD_VIO>;
>>>> + iommus = <&isp_mmu>;
>>>
>>> sort
>>>
>>> Something missing?
>>> Where are the ports and port nodes?
>>
>> I was assuming that this would be a part of the board specific dtsi or
>> dts entry where the isp node would be overriden and the i2c camera
>> port
>> and the isp port remote-endpoints would be connected. I had this as a
>> part of my first patch series. However I was advised by Helen to not
>> include the ov5647
>> dtsi as it isn't hardwired to the SoC and resides on an camera adapter
>> board.
>>
>> Should this be defined without the remote-endpoint phandle since we
>> don't know exactly which sensor gets connected in this dtsi file ?
>>
>>>
>>>> + status = "disabled";
Question for Heiko:
Should we add status to Helen's serie as well?
>>>> + };
>>>> +
>>>> sdmmc: mmc@ff0c0000 {
>>>> compatible = "rockchip,rk3288-dw-mshc";
>>>> max-frequency = <150000000>;
>>>> @@ -891,6 +908,14 @@
>>>> status = "disabled";
>>>> };
>>>>
>>>
>>>> + mipi_phy_rx0: mipi-phy-rx0 {
>>>
>>> Use separate patch.
>>>
>>> For nodes:
>>> Sort things without reg alphabetical first,
>>> then sort the rest by reg address.
>>>
>> Sure
>>
>>>> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
>>>> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru
>> PCLK_MIPI_CSI>;
>>>> + clock-names = "dphy-ref", "pclk";
>>> Something missing?
>>> Does this phy have a power domain?
>> The tinkerboard debian kernel (where I referred to for this patch)
>> didn't have it defined for the DPHY. I would guess that it would be
>> the same as the ISP i.e. RK3288_PD_VIO,
>> does anyone know the answer to this or do I have to reach out to
>> Rockchip engineering ?
>>>
>>>> + #phy-cells = <0>;
>>>> + status = "disabled";
>>>> + };
>>>> +
>>>> io_domains: io-domains {
>>>> compatible = "rockchip,rk3288-io-voltage-domain";
>>>> status = "disabled";
>>>>
>>>
>>
>>
>> --
>> Regards,
>> Karthik Poduval
>>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-23 11:12 ` Johan Jonker
@ 2020-05-14 13:19 ` karthik poduval
2020-07-02 16:27 ` Helen Koike
1 sibling, 0 replies; 16+ messages in thread
From: karthik poduval @ 2020-05-14 13:19 UTC (permalink / raw)
To: Johan Jonker
Cc: Linux Media Mailing List, heiko, linux-rockchip, Helen Koike,
Ezequiel Garcia, Rob Herring, devicetree
Ping, gentle reminder, do we have a recommendation on how to deal with
common driver supporting multiple SoC with different types and number
of clocks.
On Thu, Apr 23, 2020 at 4:12 AM Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi robh, Heiko, Karthik, Helen and others,
>
> See comments below.
> Should we change Helen's patch serie a little bit to accommodate other
> isp1 compatibles as well? Could you give advise here? Thanks!
>
> Johan
>
>
> On 4/23/20 7:10 AM, karthik poduval wrote:
> > Hi Johan/Helen,
> >
> > I was attempting to fix the yaml to work for both platforms rk3288 and
> > rk3399. I couldn't find any specific example in the existing yaml files
> > that deal with this exact scenario common driver but different clocks for
> > different chipsets. Could you point me to an appropriate example ?
> >
> > Meanwhile here is the patch I was trying out.
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > index af246b71eac6..4ca76a1bbb63 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > @@ -15,7 +15,11 @@ description: |
> >
> > properties:
> > compatible:
> > - const: rockchip,rk3399-cif-isp
> > + items:
> > + - enum:
> > + - rockchip,rk3288-cif-isp
> > + - rockchip,rk3399-cif-isp
> > + - const: rockchip,rk3399-cif-isp
> >
> > reg:
> > maxItems: 1
> > @@ -37,20 +41,38 @@ properties:
> > const: dphy
> >
>
> > clocks:
> > - items:
> > - - description: ISP clock
> > - - description: ISP AXI clock clock
> > - - description: ISP AXI clock wrapper clock
> > - - description: ISP AHB clock clock
> > - - description: ISP AHB wrapper clock
> > + oneOf:
> > + # rk3399 clocks
> > + - items:
> > + - description: ISP clock
> > + - description: ISP AXI clock clock
> > + - description: ISP AXI clock wrapper clock
> > + - description: ISP AHB clock clock
> > + - description: ISP AHB wrapper clock
> > + # rk3288 clocks
> > + - items:
> > + - description: ISP clock
> > + - description: ISP AXI clock clock
> > + - description: ISP AHB clock clock
> > + - description: ISP Pixel clock
> > + - description: ISP JPEG source clock
> >
>
> We can expect a few more clocks here, so just use:
>
> clocks:
> minItems: 4
> maxItems: 5
>
> or
>
> See question for Helen about 'pclk_isp_wrap':
>
> clocks:
> minItems: 4
> maxItems: 6
>
>
> From Rockchip tree:
>
> static const char * const rk1808_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3288_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp_in",
> "sclk_isp_jpe",
> };
>
> static const char * const rk3326_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3368_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3399_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "aclk_isp_wrap",
> "hclk_isp_wrap",
> "pclk_isp_wrap"
> };
>
> Question for Helen:
>
> Where did 'pclk_isp_wrap' go in your patch serie?
>
> > clock-names:
> > - items:
> > - - const: clk_isp
> > - - const: aclk_isp
> > - - const: aclk_isp_wrap
> > - - const: hclk_isp
> > - - const: hclk_isp_wrap
> > + oneOf:
> > + # rk3399 clocks
>
> sort on SoC
>
> > + - items:
> > + - const: clk_isp
> > + - const: aclk_isp
> > + - const: aclk_isp_wrap
> > + - const: hclk_isp
> > + - const: hclk_isp_wrap
> > + # rk3288 clocks
>
> sort on SoC
>
> > + - items:
> > + - const: clk_isp
> > + - const: aclk_isp
> > + - const: hclk_isp
> > + - const: pclk_isp_in
> > + - const: sclk_isp_jpe
>
> clock-names:
> oneOf:
> # rk3288 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: pclk_isp_in
> description: ISP Pixel clock
> - const: sclk_isp_jpe
> description: ISP JPEG source clock
> # rk3399 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: aclk_isp_wrap
> description: ISP AXI clock wrapper clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: hclk_isp_wrap
> description: ISP AHB wrapper clock
>
> Question for robh:
> Is this a proper way to add description or is there a beter way?
>
> >
> > on running command.
> > make ARCH=arm dtbs_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >
> > I see following messages for dts nodes.
> > DTC arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> > CHECK arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> > /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> > isp@ff910000: clocks: [[7, 107], [7, 205], [7, 469], [7, 371], [7, 108]] is
> > valid under each of {'additionalItems': False, 'items': [{}, {}, {}, {},
> > {}], 'maxItems': 5, 'minItems': 5, 'type': 'array'}, {'additionalItems':
> > False, 'items': [{}, {}, {}, {}, {}], 'maxItems': 5, 'minItems': 5, 'type':
> > 'array'}
> > /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> > isp@ff910000: compatible: ['rockchip,rk3288-cif-isp'] is too short
> >
> > Are these cosidered as error messages ? The "too short" is being alerted
> > for the orgianl yaml for rk3399 without any of my chnages.
> > Kindly advise.
> >
> > --
> > Regards,
> > Karthik Poduval
> >
> > On Sat, Apr 11, 2020 at 10:13 PM karthik poduval <karthik.poduval@gmail.com>
> > wrote:
> >
> >> Thanks Johan for your valuable comments.
> >>
> >> On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@gmail.com> wrote:
> >>>
> >>> Hi Karthik and others,
> >>>
> >>> Include all mail lists found with:
> >>> ./scripts/get_maintainer.pl --nogit-fallback --nogit
> >>>
> >>> Helen is moving isp1 bindings out of staging.
> >>> Clocks and other things don't fit with her patch serie.
> >>> Maybe fix this while still in staging?
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'phys' is a required property
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'phy-names' is a required property
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'ports' is a required property
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'assigned-clock-rates', 'assigned-clocks'
> >>> do not match any of the regexes: 'pinctrl-[0-9]+'
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:2: 'aclk_isp_wrap' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:3: 'hclk_isp' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:4: 'hclk_isp_wrap' was expected
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
> >>> is a required property
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
> >>> 'dphy-cfg' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
> >>> ['dphy-ref', 'pclk'] is too short
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
> >>> 126], [7, 358]] is too short
> >>>
> >>>
> >>> Inside yaml:
> >>> Use enum and sort.
> >>>>> properties:
> >>>>> compatible:
> >>>
> >>>>> const: rockchip,rk3399-cif-isp
> >>>>> + const: rockchip,rk3288-rkisp1
> >>>
> >>> enum:
> >>> - rockchip,rk3288-rkisp1
> >>> - rockchip,rk3399-cif-isp
> >>>
> >>>>> properties:
> >>>>> compatible:
> >>>>> const: rockchip,rk3399-mipi-dphy-rx0
> >>>>> + const: rockchip,rk3288-mipi-dphy-rx0
> >>>
> >>> enum:
> >>> - rockchip,rk3288-mipi-dphy-rx0
> >>> - rockchip,rk3399-mipi-dphy-rx0
> >>>
> >>>>
> >>>> Please, keep consistency, or rk3288-cif-isp, or we change
> >> rk3399-cif-isp to be rk3399-rkisp1.
> >>>
> >>>
> >>> On 4/6/20 9:30 AM, Karthik Poduval wrote:
> >>>> ISP and DPHY device entries missing so add them.
> >>>>
> >>>
> >>>> tested on tinkerbaord with ov5647 using command
> >>>> cam -c 1 -C -F cap
> >>>
> >>> Disclose dts node for ov5647 in cover letter, so people can reproduce
> >>> this experiment.
> >>>
> >>> Caution!
> >>> Without dts node this command doesn't work correct.
> >>>
> >>> make ARCH=arm dtbs_check
> >>>
> >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >>>
> >>> make ARCH=arm dtbs_check
> >>>
> >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> >>>
> >>> Needed to detect missing: phys, phy-names and ports ,etc.
> >>>
> >>> &isp {
> >>> status = "okay";
> >>> };
> >>>
> >> Makes sense, that's why my kernel compilation passed and I didn't
> >> these errors. Thanks for the command, I will verify dts for
> >> correctness in next patch series.
> >>
> >>> Needed to detect missing: power-domains, etc.
> >>>
> >>> &mipi_phy_rx0 {
> >>> status = "okay";
> >>> };
> >>>
> >>>>
> >>>> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
> >>>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> >>>> ---
> >>>> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
> >>>> 1 file changed, 25 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/boot/dts/rk3288.dtsi
> >> b/arch/arm/boot/dts/rk3288.dtsi
> >>>> index 9beb662166aa..adea8189abd9 100644
> >>>> --- a/arch/arm/boot/dts/rk3288.dtsi
> >>>> +++ b/arch/arm/boot/dts/rk3288.dtsi
> >>>> @@ -247,6 +247,23 @@
> >>>> ports = <&vopl_out>, <&vopb_out>;
> >>>> };
> >>>>
> >>>
> >>>> + isp: isp@ff910000 {
> >>>
> >>> For nodes:
> >>> Sort things without reg alphabetical first,
> >>> then sort the rest by reg address.
> >>>
> >> Sure
> >>>> + compatible = "rockchip,rk3288-rkisp1";
> >>>> + reg = <0x0 0xff910000 0x0 0x4000>;
> >>>> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> >>>> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
> >>>> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
> >>>> + <&cru SCLK_ISP_JPE>;
> >>>> + clock-names = "clk_isp", "aclk_isp",
> >>>> + "hclk_isp", "pclk_isp_in",
> >>>> + "sclk_isp_jpe";
> >>>> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
> >>>> + assigned-clock-rates = <400000000>, <400000000>;
> >>>
> >>>> + power-domains = <&power RK3288_PD_VIO>;
> >>>> + iommus = <&isp_mmu>;
> >>>
> >>> sort
> >>>
> >>> Something missing?
> >>> Where are the ports and port nodes?
> >>
> >> I was assuming that this would be a part of the board specific dtsi or
> >> dts entry where the isp node would be overriden and the i2c camera
> >> port
> >> and the isp port remote-endpoints would be connected. I had this as a
> >> part of my first patch series. However I was advised by Helen to not
> >> include the ov5647
> >> dtsi as it isn't hardwired to the SoC and resides on an camera adapter
> >> board.
> >>
> >> Should this be defined without the remote-endpoint phandle since we
> >> don't know exactly which sensor gets connected in this dtsi file ?
> >>
> >>>
> >>>> + status = "disabled";
>
> Question for Heiko:
> Should we add status to Helen's serie as well?
>
> >>>> + };
> >>>> +
> >>>> sdmmc: mmc@ff0c0000 {
> >>>> compatible = "rockchip,rk3288-dw-mshc";
> >>>> max-frequency = <150000000>;
> >>>> @@ -891,6 +908,14 @@
> >>>> status = "disabled";
> >>>> };
> >>>>
> >>>
> >>>> + mipi_phy_rx0: mipi-phy-rx0 {
> >>>
> >>> Use separate patch.
> >>>
> >>> For nodes:
> >>> Sort things without reg alphabetical first,
> >>> then sort the rest by reg address.
> >>>
> >> Sure
> >>
> >>>> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
> >>>> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru
> >> PCLK_MIPI_CSI>;
> >>>> + clock-names = "dphy-ref", "pclk";
> >>> Something missing?
> >>> Does this phy have a power domain?
> >> The tinkerboard debian kernel (where I referred to for this patch)
> >> didn't have it defined for the DPHY. I would guess that it would be
> >> the same as the ISP i.e. RK3288_PD_VIO,
> >> does anyone know the answer to this or do I have to reach out to
> >> Rockchip engineering ?
> >>>
> >>>> + #phy-cells = <0>;
> >>>> + status = "disabled";
> >>>> + };
> >>>> +
> >>>> io_domains: io-domains {
> >>>> compatible = "rockchip,rk3288-io-voltage-domain";
> >>>> status = "disabled";
> >>>>
> >>>
> >>
> >>
> >> --
> >> Regards,
> >> Karthik Poduval
> >>
> >
> >
>
--
Regards,
Karthik Poduval
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-04-23 11:12 ` Johan Jonker
2020-05-14 13:19 ` karthik poduval
@ 2020-07-02 16:27 ` Helen Koike
2020-07-02 17:32 ` Robin Murphy
1 sibling, 1 reply; 16+ messages in thread
From: Helen Koike @ 2020-07-02 16:27 UTC (permalink / raw)
To: Johan Jonker, karthik poduval
Cc: Linux Media Mailing List, heiko, linux-rockchip, Ezequiel Garcia,
Rob Herring, devicetree
Hello all,
Sorry for my late reply, please see below,
On 4/23/20 8:12 AM, Johan Jonker wrote:
> Hi robh, Heiko, Karthik, Helen and others,
>
> See comments below.
> Should we change Helen's patch serie a little bit to accommodate other
> isp1 compatibles as well? Could you give advise here? Thanks!
>
> Johan
>
>
> On 4/23/20 7:10 AM, karthik poduval wrote:
>> Hi Johan/Helen,
>>
>> I was attempting to fix the yaml to work for both platforms rk3288 and
>> rk3399. I couldn't find any specific example in the existing yaml files
>> that deal with this exact scenario common driver but different clocks for
>> different chipsets. Could you point me to an appropriate example ?
>>
>> Meanwhile here is the patch I was trying out.
>> diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> index af246b71eac6..4ca76a1bbb63 100644
>> --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> @@ -15,7 +15,11 @@ description: |
>>
>> properties:
>> compatible:
>> - const: rockchip,rk3399-cif-isp
>> + items:
>> + - enum:
>> + - rockchip,rk3288-cif-isp
>> + - rockchip,rk3399-cif-isp
>> + - const: rockchip,rk3399-cif-isp
>>
>> reg:
>> maxItems: 1
>> @@ -37,20 +41,38 @@ properties:
>> const: dphy
>>
>
>> clocks:
>> - items:
>> - - description: ISP clock
>> - - description: ISP AXI clock clock
>> - - description: ISP AXI clock wrapper clock
>> - - description: ISP AHB clock clock
>> - - description: ISP AHB wrapper clock
>> + oneOf:
>> + # rk3399 clocks
>> + - items:
>> + - description: ISP clock
>> + - description: ISP AXI clock clock
>> + - description: ISP AXI clock wrapper clock
>> + - description: ISP AHB clock clock
>> + - description: ISP AHB wrapper clock
>> + # rk3288 clocks
>> + - items:
>> + - description: ISP clock
>> + - description: ISP AXI clock clock
>> + - description: ISP AHB clock clock
>> + - description: ISP Pixel clock
>> + - description: ISP JPEG source clock
>>
>
> We can expect a few more clocks here, so just use:
>
> clocks:
> minItems: 4
> maxItems: 5
>
> or
>
> See question for Helen about 'pclk_isp_wrap':
>
> clocks:
> minItems: 4
> maxItems: 6
>
>
> From Rockchip tree:
>
> static const char * const rk1808_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3288_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp_in",
> "sclk_isp_jpe",
> };
>
> static const char * const rk3326_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3368_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3399_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "aclk_isp_wrap",
> "hclk_isp_wrap",
> "pclk_isp_wrap"
> };
I was checking the clock topology, and we don't need aclk_isp and hclk_isp,
since they are parents of the "wrap" versions:
xin24m
...
pll_npll
npll
clk_isp1
clk_isp0
...
pll_cpll
...
cpll
aclk_isp1
aclk_isp1_noc
hclk_isp1
aclk_isp1_wrapper
hclk_isp1_noc
aclk_isp0
hclk_isp1_wrapper
aclk_isp0_wrapper
aclk_isp0_noc
hclk_isp0
hclk_isp0_wrapper
hclk_isp0_noc
...
pclkin_isp1_wrapper
(it is a bit weird that hclk_isp1_wrapper is a child of aclk_isp0, but this is how
is described in the SoC TRM)
I tested with only:
static const char * const rk3399_isp_clks[] = {
"clk_isp",
"aclk_isp_wrap",
"hclk_isp_wrap",
};
And it works, and it doesn't work when removing any of those.
>
> Question for Helen:
>
> Where did 'pclk_isp_wrap' go in your patch serie?
There are two instances of the ISPv1 in the SoC (isp0 and isp1), but I can only test
isp0 on rk3399
It seems that isp1 requires pclk_isp_wrap, so we can add it as optional in the bindings (see below).
>
>> clock-names:
>> - items:
>> - - const: clk_isp
>> - - const: aclk_isp
>> - - const: aclk_isp_wrap
>> - - const: hclk_isp
>> - - const: hclk_isp_wrap
>> + oneOf:
>> + # rk3399 clocks
>
> sort on SoC
>
>> + - items:
>> + - const: clk_isp
>> + - const: aclk_isp
>> + - const: aclk_isp_wrap
>> + - const: hclk_isp
>> + - const: hclk_isp_wrap
>> + # rk3288 clocks
>
> sort on SoC
>
>> + - items:
>> + - const: clk_isp
>> + - const: aclk_isp
>> + - const: hclk_isp
>> + - const: pclk_isp_in
>> + - const: sclk_isp_jpe
>
> clock-names:
> oneOf:
> # rk3288 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: pclk_isp_in
> description: ISP Pixel clock
> - const: sclk_isp_jpe
> description: ISP JPEG source clock
> # rk3399 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: aclk_isp_wrap
> description: ISP AXI clock wrapper clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: hclk_isp_wrap
> description: ISP AHB wrapper clock
I suggest this:
clocks:
maxItems: 5
minItems: 3
description:
rk3288 clocks
ISP clock
ISP AXI clock
ISP AHB clock
ISP Pixel clock
ISP JPEG source clock
rk3399 isp0 clocks
ISP clock
ISP AXI wrapper clock
ISP AHB wrapper clock
rk3399 isp1 clocks
ISP clock
ISP AXI wrapper clock
ISP AHB wrapper clock
ISP Pixel wrapper clock
clock-names:
oneOf:
# rk3288 clocks
- items:
- const: clk_isp
- const: aclk_isp
- const: hclk_isp
- const: pclk_isp_in
- const: sclk_isp_jpe
# rk3399 isp0 clocks
- items:
- const: clk_isp
- const: aclk_isp_wrap
- const: hclk_isp_wrap
# rk3399 isp1 clocks
- items:
- const: clk_isp
- const: aclk_isp_wrap
- const: hclk_isp_wrap
- const: pclk_isp_wrap
Where "# rk3288 clocks", "# rk3399 isp0 clocks" and "# rk3399 isp1 clocks" could be
added in separated patchsets that are sure those works corretly.
Regards,
Helen
>
> Question for robh:
> Is this a proper way to add description or is there a beter way?
>
>>
>> on running command.
>> make ARCH=arm dtbs_check
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>
>> I see following messages for dts nodes.
>> DTC arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
>> CHECK arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
>> /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
>> isp@ff910000: clocks: [[7, 107], [7, 205], [7, 469], [7, 371], [7, 108]] is
>> valid under each of {'additionalItems': False, 'items': [{}, {}, {}, {},
>> {}], 'maxItems': 5, 'minItems': 5, 'type': 'array'}, {'additionalItems':
>> False, 'items': [{}, {}, {}, {}, {}], 'maxItems': 5, 'minItems': 5, 'type':
>> 'array'}
>> /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
>> isp@ff910000: compatible: ['rockchip,rk3288-cif-isp'] is too short
>>
>> Are these cosidered as error messages ? The "too short" is being alerted
>> for the orgianl yaml for rk3399 without any of my chnages.
>> Kindly advise.
>>
>> --
>> Regards,
>> Karthik Poduval
>>
>> On Sat, Apr 11, 2020 at 10:13 PM karthik poduval <karthik.poduval@gmail.com>
>> wrote:
>>
>>> Thanks Johan for your valuable comments.
>>>
>>> On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@gmail.com> wrote:
>>>>
>>>> Hi Karthik and others,
>>>>
>>>> Include all mail lists found with:
>>>> ./scripts/get_maintainer.pl --nogit-fallback --nogit
>>>>
>>>> Helen is moving isp1 bindings out of staging.
>>>> Clocks and other things don't fit with her patch serie.
>>>> Maybe fix this while still in staging?
>>>>
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> 'phys' is a required property
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> 'phy-names' is a required property
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> 'ports' is a required property
>>>>
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> 'assigned-clock-rates', 'assigned-clocks'
>>>> do not match any of the regexes: 'pinctrl-[0-9]+'
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> clock-names:2: 'aclk_isp_wrap' was expected
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> clock-names:3: 'hclk_isp' was expected
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
>>>> clock-names:4: 'hclk_isp_wrap' was expected
>>>>
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
>>>> is a required property
>>>>
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
>>>> 'dphy-cfg' was expected
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
>>>> ['dphy-ref', 'pclk'] is too short
>>>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
>>>> 126], [7, 358]] is too short
>>>>
>>>>
>>>> Inside yaml:
>>>> Use enum and sort.
>>>>>> properties:
>>>>>> compatible:
>>>>
>>>>>> const: rockchip,rk3399-cif-isp
>>>>>> + const: rockchip,rk3288-rkisp1
>>>>
>>>> enum:
>>>> - rockchip,rk3288-rkisp1
>>>> - rockchip,rk3399-cif-isp
>>>>
>>>>>> properties:
>>>>>> compatible:
>>>>>> const: rockchip,rk3399-mipi-dphy-rx0
>>>>>> + const: rockchip,rk3288-mipi-dphy-rx0
>>>>
>>>> enum:
>>>> - rockchip,rk3288-mipi-dphy-rx0
>>>> - rockchip,rk3399-mipi-dphy-rx0
>>>>
>>>>>
>>>>> Please, keep consistency, or rk3288-cif-isp, or we change
>>> rk3399-cif-isp to be rk3399-rkisp1.
>>>>
>>>>
>>>> On 4/6/20 9:30 AM, Karthik Poduval wrote:
>>>>> ISP and DPHY device entries missing so add them.
>>>>>
>>>>
>>>>> tested on tinkerbaord with ov5647 using command
>>>>> cam -c 1 -C -F cap
>>>>
>>>> Disclose dts node for ov5647 in cover letter, so people can reproduce
>>>> this experiment.
>>>>
>>>> Caution!
>>>> Without dts node this command doesn't work correct.
>>>>
>>>> make ARCH=arm dtbs_check
>>>>
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>>
>>>> make ARCH=arm dtbs_check
>>>>
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
>>>>
>>>> Needed to detect missing: phys, phy-names and ports ,etc.
>>>>
>>>> &isp {
>>>> status = "okay";
>>>> };
>>>>
>>> Makes sense, that's why my kernel compilation passed and I didn't
>>> these errors. Thanks for the command, I will verify dts for
>>> correctness in next patch series.
>>>
>>>> Needed to detect missing: power-domains, etc.
>>>>
>>>> &mipi_phy_rx0 {
>>>> status = "okay";
>>>> };
>>>>
>>>>>
>>>>> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>
>>>>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
>>>>> ---
>>>>> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
>>>>> 1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/rk3288.dtsi
>>> b/arch/arm/boot/dts/rk3288.dtsi
>>>>> index 9beb662166aa..adea8189abd9 100644
>>>>> --- a/arch/arm/boot/dts/rk3288.dtsi
>>>>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>>>>> @@ -247,6 +247,23 @@
>>>>> ports = <&vopl_out>, <&vopb_out>;
>>>>> };
>>>>>
>>>>
>>>>> + isp: isp@ff910000 {
>>>>
>>>> For nodes:
>>>> Sort things without reg alphabetical first,
>>>> then sort the rest by reg address.
>>>>
>>> Sure
>>>>> + compatible = "rockchip,rk3288-rkisp1";
>>>>> + reg = <0x0 0xff910000 0x0 0x4000>;
>>>>> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>>>> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
>>>>> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
>>>>> + <&cru SCLK_ISP_JPE>;
>>>>> + clock-names = "clk_isp", "aclk_isp",
>>>>> + "hclk_isp", "pclk_isp_in",
>>>>> + "sclk_isp_jpe";
>>>>> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
>>>>> + assigned-clock-rates = <400000000>, <400000000>;
>>>>
>>>>> + power-domains = <&power RK3288_PD_VIO>;
>>>>> + iommus = <&isp_mmu>;
>>>>
>>>> sort
>>>>
>>>> Something missing?
>>>> Where are the ports and port nodes?
>>>
>>> I was assuming that this would be a part of the board specific dtsi or
>>> dts entry where the isp node would be overriden and the i2c camera
>>> port
>>> and the isp port remote-endpoints would be connected. I had this as a
>>> part of my first patch series. However I was advised by Helen to not
>>> include the ov5647
>>> dtsi as it isn't hardwired to the SoC and resides on an camera adapter
>>> board.
>>>
>>> Should this be defined without the remote-endpoint phandle since we
>>> don't know exactly which sensor gets connected in this dtsi file ?
>>>
>>>>
>>>>> + status = "disabled";
>
> Question for Heiko:
> Should we add status to Helen's serie as well?
>
>>>>> + };
>>>>> +
>>>>> sdmmc: mmc@ff0c0000 {
>>>>> compatible = "rockchip,rk3288-dw-mshc";
>>>>> max-frequency = <150000000>;
>>>>> @@ -891,6 +908,14 @@
>>>>> status = "disabled";
>>>>> };
>>>>>
>>>>
>>>>> + mipi_phy_rx0: mipi-phy-rx0 {
>>>>
>>>> Use separate patch.
>>>>
>>>> For nodes:
>>>> Sort things without reg alphabetical first,
>>>> then sort the rest by reg address.
>>>>
>>> Sure
>>>
>>>>> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
>>>>> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru
>>> PCLK_MIPI_CSI>;
>>>>> + clock-names = "dphy-ref", "pclk";
>>>> Something missing?
>>>> Does this phy have a power domain?
>>> The tinkerboard debian kernel (where I referred to for this patch)
>>> didn't have it defined for the DPHY. I would guess that it would be
>>> the same as the ISP i.e. RK3288_PD_VIO,
>>> does anyone know the answer to this or do I have to reach out to
>>> Rockchip engineering ?
>>>>
>>>>> + #phy-cells = <0>;
>>>>> + status = "disabled";
>>>>> + };
>>>>> +
>>>>> io_domains: io-domains {
>>>>> compatible = "rockchip,rk3288-io-voltage-domain";
>>>>> status = "disabled";
>>>>>
>>>>
>>>
>>>
>>> --
>>> Regards,
>>> Karthik Poduval
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-07-02 16:27 ` Helen Koike
@ 2020-07-02 17:32 ` Robin Murphy
2020-07-02 19:16 ` Helen Koike
0 siblings, 1 reply; 16+ messages in thread
From: Robin Murphy @ 2020-07-02 17:32 UTC (permalink / raw)
To: Helen Koike, Johan Jonker, karthik poduval
Cc: Rob Herring, heiko, devicetree, linux-rockchip, Ezequiel Garcia,
Linux Media Mailing List
On 2020-07-02 17:27, Helen Koike wrote:
[...]
> I suggest this:
>
> clocks:
> maxItems: 5
> minItems: 3
> description:
> rk3288 clocks
> ISP clock
> ISP AXI clock
> ISP AHB clock
> ISP Pixel clock
> ISP JPEG source clock
> rk3399 isp0 clocks
> ISP clock
> ISP AXI wrapper clock
> ISP AHB wrapper clock
> rk3399 isp1 clocks
> ISP clock
> ISP AXI wrapper clock
> ISP AHB wrapper clock
> ISP Pixel wrapper clock
>
> clock-names:
> oneOf:
> # rk3288 clocks
> - items:
> - const: clk_isp
> - const: aclk_isp
> - const: hclk_isp
> - const: pclk_isp_in
> - const: sclk_isp_jpe
> # rk3399 isp0 clocks
> - items:
> - const: clk_isp
> - const: aclk_isp_wrap
> - const: hclk_isp_wrap
> # rk3399 isp1 clocks
> - items:
> - const: clk_isp
> - const: aclk_isp_wrap
> - const: hclk_isp_wrap
> - const: pclk_isp_wrap
FWIW this looks a little more involved than it might need to be. Ideally
we're describing things from the point of view of what inputs the device
itself wants, so the details of exactly *how* a particular SoC's clock
tree delivers them shouldn't matter to the binding, only to the actual
clock specifier values ultimately given in the DT.
From the ISP's PoV, it seems like we've got the fairly standard core
clock, ACLK and HCLK trio, plus a pixel clock for RK3288 and RK3399
ISP1, plus a JPEG source clock for RK3288. I'd be inclined to model that
as simply something like:
clock-names:
minItems: 3
maxItems: 5
items:
- const: isp
- const: aclk
- const: hclk
- const: pclk
- const: sclk_jpe
Plus then not only do we have a nice clean binding, but we avoid all the
unnecessary faff of having to deal with the "same" clocks by different
names in drivers, and sidestep the conundrum of what to do when the next
SoC comes along providing the basic ISP clocks from yet again
slightly-differently-named branches of its clock tree.
Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-07-02 17:32 ` Robin Murphy
@ 2020-07-02 19:16 ` Helen Koike
2020-07-02 19:19 ` karthik poduval
0 siblings, 1 reply; 16+ messages in thread
From: Helen Koike @ 2020-07-02 19:16 UTC (permalink / raw)
To: Robin Murphy, Helen Koike, Johan Jonker, karthik poduval
Cc: Rob Herring, heiko, devicetree, linux-rockchip, Ezequiel Garcia,
Linux Media Mailing List
Hi Robin,
On 7/2/20 2:32 PM, Robin Murphy wrote:
> On 2020-07-02 17:27, Helen Koike wrote:
> [...]
>> I suggest this:
>>
>> clocks:
>> maxItems: 5
>> minItems: 3
>> description:
>> rk3288 clocks
>> ISP clock
>> ISP AXI clock
>> ISP AHB clock
>> ISP Pixel clock
>> ISP JPEG source clock
>> rk3399 isp0 clocks
>> ISP clock
>> ISP AXI wrapper clock
>> ISP AHB wrapper clock
>> rk3399 isp1 clocks
>> ISP clock
>> ISP AXI wrapper clock
>> ISP AHB wrapper clock
>> ISP Pixel wrapper clock
>>
>> clock-names:
>> oneOf:
>> # rk3288 clocks
>> - items:
>> - const: clk_isp
>> - const: aclk_isp
>> - const: hclk_isp
>> - const: pclk_isp_in
>> - const: sclk_isp_jpe
>> # rk3399 isp0 clocks
>> - items:
>> - const: clk_isp
>> - const: aclk_isp_wrap
>> - const: hclk_isp_wrap
>> # rk3399 isp1 clocks
>> - items:
>> - const: clk_isp
>> - const: aclk_isp_wrap
>> - const: hclk_isp_wrap
>> - const: pclk_isp_wrap
>
> FWIW this looks a little more involved than it might need to be. Ideally we're describing things from the point of view of what inputs the device itself wants, so the details of exactly *how* a particular SoC's clock tree delivers them shouldn't matter to the binding, only to the actual clock specifier values ultimately given in the DT.
>
> From the ISP's PoV, it seems like we've got the fairly standard core clock, ACLK and HCLK trio, plus a pixel clock for RK3288 and RK3399 ISP1, plus a JPEG source clock for RK3288. I'd be inclined to model that as simply something like:
>
> clock-names:
> minItems: 3
> maxItems: 5
> items:
> - const: isp
> - const: aclk
> - const: hclk
> - const: pclk
> - const: sclk_jpe
>
> Plus then not only do we have a nice clean binding, but we avoid all the unnecessary faff of having to deal with the "same" clocks by different names in drivers, and sidestep the conundrum of what to do when the next SoC comes along providing the basic ISP clocks from yet again slightly-differently-named branches of its clock tree.
I agree this is cleaner, thanks for this suggestions, I just submitted a new version
following this https://patchwork.linuxtv.org/project/linux-media/list/?series=2844
Thanks
Helen
>
> Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY
2020-07-02 19:16 ` Helen Koike
@ 2020-07-02 19:19 ` karthik poduval
0 siblings, 0 replies; 16+ messages in thread
From: karthik poduval @ 2020-07-02 19:19 UTC (permalink / raw)
To: Helen Koike
Cc: Robin Murphy, Helen Koike, Johan Jonker, Rob Herring, heiko,
devicetree, linux-rockchip, Ezequiel Garcia,
Linux Media Mailing List
I like the suggestion from Robin, drivers care about board clock
categories and don't need to specify the SoC specific clocks. But it
still looks like we are putting a union of all clocks based on current
and future versions of the ISP IP in the yaml. Is it necessary to list
them out at all ? Can't driver's simply get them from the device tree
as indexes instead of names using "of_clk_get". In that case does the
dts yaml need to check for the number of clocks per SoC variant ?
--
Regards,
Karthik Poduval
On Thu, Jul 2, 2020 at 12:17 PM Helen Koike <helen@koikeco.de> wrote:
>
> Hi Robin,
>
> On 7/2/20 2:32 PM, Robin Murphy wrote:
> > On 2020-07-02 17:27, Helen Koike wrote:
> > [...]
> >> I suggest this:
> >>
> >> clocks:
> >> maxItems: 5
> >> minItems: 3
> >> description:
> >> rk3288 clocks
> >> ISP clock
> >> ISP AXI clock
> >> ISP AHB clock
> >> ISP Pixel clock
> >> ISP JPEG source clock
> >> rk3399 isp0 clocks
> >> ISP clock
> >> ISP AXI wrapper clock
> >> ISP AHB wrapper clock
> >> rk3399 isp1 clocks
> >> ISP clock
> >> ISP AXI wrapper clock
> >> ISP AHB wrapper clock
> >> ISP Pixel wrapper clock
> >>
> >> clock-names:
> >> oneOf:
> >> # rk3288 clocks
> >> - items:
> >> - const: clk_isp
> >> - const: aclk_isp
> >> - const: hclk_isp
> >> - const: pclk_isp_in
> >> - const: sclk_isp_jpe
> >> # rk3399 isp0 clocks
> >> - items:
> >> - const: clk_isp
> >> - const: aclk_isp_wrap
> >> - const: hclk_isp_wrap
> >> # rk3399 isp1 clocks
> >> - items:
> >> - const: clk_isp
> >> - const: aclk_isp_wrap
> >> - const: hclk_isp_wrap
> >> - const: pclk_isp_wrap
> >
> > FWIW this looks a little more involved than it might need to be. Ideally we're describing things from the point of view of what inputs the device itself wants, so the details of exactly *how* a particular SoC's clock tree delivers them shouldn't matter to the binding, only to the actual clock specifier values ultimately given in the DT.
> >
> > From the ISP's PoV, it seems like we've got the fairly standard core clock, ACLK and HCLK trio, plus a pixel clock for RK3288 and RK3399 ISP1, plus a JPEG source clock for RK3288. I'd be inclined to model that as simply something like:
> >
> > clock-names:
> > minItems: 3
> > maxItems: 5
> > items:
> > - const: isp
> > - const: aclk
> > - const: hclk
> > - const: pclk
> > - const: sclk_jpe
> >
> > Plus then not only do we have a nice clean binding, but we avoid all the unnecessary faff of having to deal with the "same" clocks by different names in drivers, and sidestep the conundrum of what to do when the next SoC comes along providing the basic ISP clocks from yet again slightly-differently-named branches of its clock tree.
>
> I agree this is cleaner, thanks for this suggestions, I just submitted a new version
> following this https://patchwork.linuxtv.org/project/linux-media/list/?series=2844
>
> Thanks
> Helen
>
> >
> > Robin.
--
Regards,
Karthik Poduval
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-07-02 19:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-06 7:30 [PATCH v2 0/3] Add Rockchip RK3288 support Karthik Poduval
2020-04-06 7:30 ` [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver Karthik Poduval
2020-04-06 16:56 ` Helen Koike
2020-04-07 0:27 ` Ezequiel Garcia
2020-04-06 7:30 ` [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support Karthik Poduval
2020-04-06 16:57 ` Helen Koike
2020-04-06 7:30 ` [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY Karthik Poduval
2020-04-06 16:57 ` Helen Koike
2020-04-09 1:19 ` Johan Jonker
2020-04-12 5:13 ` karthik poduval
[not found] ` <CAFP0Ok9XGzVbghbnOOyfXiOOc5-a94uFRu7sD5wXz9sr-+AYEA@mail.gmail.com>
2020-04-23 11:12 ` Johan Jonker
2020-05-14 13:19 ` karthik poduval
2020-07-02 16:27 ` Helen Koike
2020-07-02 17:32 ` Robin Murphy
2020-07-02 19:16 ` Helen Koike
2020-07-02 19:19 ` karthik poduval
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).