Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA
On 18/01/2023 23:19, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Tue, Jan 17, 2023 at 03:51:52PM +0200, Tomi Valkeinen wrote:
rcar_du_crtc.c does a soc_device_match() in
rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
s/ES1/ES1.x/
Same below.
if so, apply a WA.
s/WA/workaround/
Same below.
Ok.
We will need another H3 ES1 check in the following patch, so rather than
adding more soc_device_match() calls, let's add a rcar_du_device_info
entry for the ES1, and a quirk flag,
RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 +---
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 51 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
3 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 3619e1ddeb62..f2d3266509cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -10,7 +10,6 @@
#include
#include
#include
-#include
#include
#include
@@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk, unsigned
long target,
}
}
-static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
- { .soc_id = "r8a7795", .revision = "ES1.*" },
- { /* sentinel */ }
-};
-
static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
{
const struct drm_display_mode *mode = >crtc.state->adjusted_mode;
@@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct
rcar_du_crtc *rcrtc)
* no post-divider when a display PLL is present (as shown by
* the workaround breaking HDMI output on M3-W during testing).
*/
- if (soc_device_match(rcar_du_r8a7795_es1)) {
+ if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
target *= 2;
div = 1;
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index c7c5217cfc1a..ba2e069fc0f7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -386,6 +387,42 @@ static const struct rcar_du_device_info
rcar_du_r8a7795_info = {
.dpll_mask = BIT(2) | BIT(1),
};
+static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
+ .gen = 3,
+ .features = RCAR_DU_FEATURE_CRTC_IRQ
+ | RCAR_DU_FEATURE_CRTC_CLOCK
+ | RCAR_DU_FEATURE_VSP1_SOURCE
+ | RCAR_DU_FEATURE_INTERLACED
+ | RCAR_DU_FEATURE_TVM_SYNC,
+ .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
+ .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
+ .routes = {
+ /*
+* R8A7795 has one RGB output, two HDMI outputs and one
+* LVDS output.
+*/
+ [RCAR_DU_OUTPUT_DPAD0] = {
+ .possible_crtcs = BIT(3),
+ .port = 0,
+ },
+ [RCAR_DU_OUTPUT_HDMI0] = {
+ .possible_crtcs = BIT(1),
+ .port = 1,
+ },
+ [RCAR_DU_OUTPUT_HDMI1] = {
+ .possible_crtcs = BIT(2),
+ .port = 2,
+ },
+ [RCAR_DU_OUTPUT_LVDS0] = {
+ .possible_crtcs = BIT(0),
+ .port = 3,
+ },
+ },
+ .num_lvds = 1,
+ .num_rpf = 5,
+ .dpll_mask = BIT(2) | BIT(1),
+};
+
static const struct rcar_du_device_info rcar_du_r8a7796_info = {
.gen = 3,
.features = RCAR_DU_FEATURE_CRTC_IRQ
@@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
MODULE_DEVICE_TABLE(of, rcar_du_of_table);
+static const struct soc_device_attribute rcar_du_soc_table[] = {
+ { .soc_id = "r8a7795", .revision = "ES1.*", .data =
_du_r8a7795_es1_info },
+ { /* sentinel */ }
+};
+
const char *rcar_du_output_name(enum rcar_du_output output)
{
static const char * const names[] = {
@@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
struct rcar_du_device *rcdu;
unsigned int mask;
int ret;
+ const struct soc_device_attribute *soc_attr;
Please move this up before rcdu.
Sure.
if (drm_firmware_drivers_only())
return -ENODEV;
@@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
return PTR_ERR(rcdu);
rcdu->dev = >dev;
- rcdu->info = of_device_get_match_data(rcdu->dev);
+
+ soc_attr = soc_device_match(rcar_du_soc_table);
+ if (soc_attr)
+ rcdu->info
Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA
On 17/01/2023 18:11, Geert Uytterhoeven wrote: Hi Tomi, On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen wrote: rcar_du_crtc.c does a soc_device_match() in rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and if so, apply a WA. We will need another H3 ES1 check in the following patch, so rather than adding more soc_device_match() calls, let's add a rcar_du_device_info entry for the ES1, and a quirk flag, RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA. Signed-off-by: Tomi Valkeinen Thanks for your patch! --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev) return PTR_ERR(rcdu); rcdu->dev = >dev; - rcdu->info = of_device_get_match_data(rcdu->dev); No need to remove this line... + + soc_attr = soc_device_match(rcar_du_soc_table); + if (soc_attr) + rcdu->info = soc_attr->data; + + if (!rcdu->info) + rcdu->info = of_device_get_match_data(rcdu->dev); ... and no need to add these two lines. The idiom is to set rcdu->info based on of_device_get_match_data() first, and override based on of_device_get_match_data() when needed. Ok. Tomi
Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA
Hi Tomi,
Thank you for the patch.
On Tue, Jan 17, 2023 at 03:51:52PM +0200, Tomi Valkeinen wrote:
> rcar_du_crtc.c does a soc_device_match() in
> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
s/ES1/ES1.x/
Same below.
> if so, apply a WA.
s/WA/workaround/
Same below.
> We will need another H3 ES1 check in the following patch, so rather than
> adding more soc_device_match() calls, let's add a rcar_du_device_info
> entry for the ES1, and a quirk flag,
> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
>
> Signed-off-by: Tomi Valkeinen
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 +---
> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 51 +-
> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
> 3 files changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 3619e1ddeb62..f2d3266509cc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -10,7 +10,6 @@
> #include
> #include
> #include
> -#include
>
> #include
> #include
> @@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk,
> unsigned long target,
> }
> }
>
> -static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
> - { .soc_id = "r8a7795", .revision = "ES1.*" },
> - { /* sentinel */ }
> -};
> -
> static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> {
> const struct drm_display_mode *mode = >crtc.state->adjusted_mode;
> @@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
>* no post-divider when a display PLL is present (as shown by
>* the workaround breaking HDMI output on M3-W during testing).
>*/
> - if (soc_device_match(rcar_du_r8a7795_es1)) {
> + if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
> target *= 2;
> div = 1;
> }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index c7c5217cfc1a..ba2e069fc0f7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -386,6 +387,42 @@ static const struct rcar_du_device_info
> rcar_du_r8a7795_info = {
> .dpll_mask = BIT(2) | BIT(1),
> };
>
> +static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
> + .gen = 3,
> + .features = RCAR_DU_FEATURE_CRTC_IRQ
> + | RCAR_DU_FEATURE_CRTC_CLOCK
> + | RCAR_DU_FEATURE_VSP1_SOURCE
> + | RCAR_DU_FEATURE_INTERLACED
> + | RCAR_DU_FEATURE_TVM_SYNC,
> + .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
> + .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
> + .routes = {
> + /*
> + * R8A7795 has one RGB output, two HDMI outputs and one
> + * LVDS output.
> + */
> + [RCAR_DU_OUTPUT_DPAD0] = {
> + .possible_crtcs = BIT(3),
> + .port = 0,
> + },
> + [RCAR_DU_OUTPUT_HDMI0] = {
> + .possible_crtcs = BIT(1),
> + .port = 1,
> + },
> + [RCAR_DU_OUTPUT_HDMI1] = {
> + .possible_crtcs = BIT(2),
> + .port = 2,
> + },
> + [RCAR_DU_OUTPUT_LVDS0] = {
> + .possible_crtcs = BIT(0),
> + .port = 3,
> + },
> + },
> + .num_lvds = 1,
> + .num_rpf = 5,
> + .dpll_mask = BIT(2) | BIT(1),
> +};
> +
> static const struct rcar_du_device_info rcar_du_r8a7796_info = {
> .gen = 3,
> .features = RCAR_DU_FEATURE_CRTC_IRQ
> @@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
>
> MODULE_DEVICE_TABLE(of, rcar_du_of_table);
>
> +static const struct soc_device_attribute rcar_du_soc_table[] = {
> + { .soc_id = "r8a7795", .revision = "ES1.*", .data =
> _du_r8a7795_es1_info },
> + { /* sentinel */ }
> +};
> +
> const char *rcar_du_output_name(enum rcar_du_output output)
> {
> static const char * const names[] = {
> @@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
> struct rcar_du_device *rcdu;
> unsigned int mask;
> int ret;
> + const struct soc_device_attribute *soc_attr;
Please move this up before rcdu.
>
> if (drm_firmware_drivers_only())
> return -ENODEV;
> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
> return PTR_ERR(rcdu);
>
> rcdu->dev = >dev;
> - rcdu->info = of_device_get_match_data(rcdu->dev);
> +
> + soc_attr =
[PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA
rcar_du_crtc.c does a soc_device_match() in
rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
if so, apply a WA.
We will need another H3 ES1 check in the following patch, so rather than
adding more soc_device_match() calls, let's add a rcar_du_device_info
entry for the ES1, and a quirk flag,
RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 8 +---
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 51 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +
3 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 3619e1ddeb62..f2d3266509cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -10,7 +10,6 @@
#include
#include
#include
-#include
#include
#include
@@ -204,11 +203,6 @@ static void rcar_du_escr_divider(struct clk *clk, unsigned
long target,
}
}
-static const struct soc_device_attribute rcar_du_r8a7795_es1[] = {
- { .soc_id = "r8a7795", .revision = "ES1.*" },
- { /* sentinel */ }
-};
-
static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
{
const struct drm_display_mode *mode = >crtc.state->adjusted_mode;
@@ -238,7 +232,7 @@ static void rcar_du_crtc_set_display_timing(struct
rcar_du_crtc *rcrtc)
* no post-divider when a display PLL is present (as shown by
* the workaround breaking HDMI output on M3-W during testing).
*/
- if (soc_device_match(rcar_du_r8a7795_es1)) {
+ if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY) {
target *= 2;
div = 1;
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index c7c5217cfc1a..ba2e069fc0f7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -386,6 +387,42 @@ static const struct rcar_du_device_info
rcar_du_r8a7795_info = {
.dpll_mask = BIT(2) | BIT(1),
};
+static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = {
+ .gen = 3,
+ .features = RCAR_DU_FEATURE_CRTC_IRQ
+ | RCAR_DU_FEATURE_CRTC_CLOCK
+ | RCAR_DU_FEATURE_VSP1_SOURCE
+ | RCAR_DU_FEATURE_INTERLACED
+ | RCAR_DU_FEATURE_TVM_SYNC,
+ .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY,
+ .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0),
+ .routes = {
+ /*
+* R8A7795 has one RGB output, two HDMI outputs and one
+* LVDS output.
+*/
+ [RCAR_DU_OUTPUT_DPAD0] = {
+ .possible_crtcs = BIT(3),
+ .port = 0,
+ },
+ [RCAR_DU_OUTPUT_HDMI0] = {
+ .possible_crtcs = BIT(1),
+ .port = 1,
+ },
+ [RCAR_DU_OUTPUT_HDMI1] = {
+ .possible_crtcs = BIT(2),
+ .port = 2,
+ },
+ [RCAR_DU_OUTPUT_LVDS0] = {
+ .possible_crtcs = BIT(0),
+ .port = 3,
+ },
+ },
+ .num_lvds = 1,
+ .num_rpf = 5,
+ .dpll_mask = BIT(2) | BIT(1),
+};
+
static const struct rcar_du_device_info rcar_du_r8a7796_info = {
.gen = 3,
.features = RCAR_DU_FEATURE_CRTC_IRQ
@@ -576,6 +613,11 @@ static const struct of_device_id rcar_du_of_table[] = {
MODULE_DEVICE_TABLE(of, rcar_du_of_table);
+static const struct soc_device_attribute rcar_du_soc_table[] = {
+ { .soc_id = "r8a7795", .revision = "ES1.*", .data =
_du_r8a7795_es1_info },
+ { /* sentinel */ }
+};
+
const char *rcar_du_output_name(enum rcar_du_output output)
{
static const char * const names[] = {
@@ -670,6 +712,7 @@ static int rcar_du_probe(struct platform_device *pdev)
struct rcar_du_device *rcdu;
unsigned int mask;
int ret;
+ const struct soc_device_attribute *soc_attr;
if (drm_firmware_drivers_only())
return -ENODEV;
@@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
return PTR_ERR(rcdu);
rcdu->dev = >dev;
- rcdu->info = of_device_get_match_data(rcdu->dev);
+
+ soc_attr = soc_device_match(rcar_du_soc_table);
+ if (soc_attr)
+ rcdu->info = soc_attr->data;
+
+ if (!rcdu->info)
+ rcdu->info = of_device_get_match_data(rcdu->dev);
platform_set_drvdata(pdev, rcdu);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 5cfa2bb7ad93..df87ccab146f
Re: [PATCH 4/6] drm: rcar-du: Add quirk for H3 ES1 pclk WA
Hi Tomi,
On Tue, Jan 17, 2023 at 2:54 PM Tomi Valkeinen
wrote:
> rcar_du_crtc.c does a soc_device_match() in
> rcar_du_crtc_set_display_timing() to find out if the SoC is H3 ES1, and
> if so, apply a WA.
>
> We will need another H3 ES1 check in the following patch, so rather than
> adding more soc_device_match() calls, let's add a rcar_du_device_info
> entry for the ES1, and a quirk flag,
> RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, for the WA.
>
> Signed-off-by: Tomi Valkeinen
Thanks for your patch!
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -681,7 +724,13 @@ static int rcar_du_probe(struct platform_device *pdev)
> return PTR_ERR(rcdu);
>
> rcdu->dev = >dev;
> - rcdu->info = of_device_get_match_data(rcdu->dev);
No need to remove this line...
> +
> + soc_attr = soc_device_match(rcar_du_soc_table);
> + if (soc_attr)
> + rcdu->info = soc_attr->data;
> +
> + if (!rcdu->info)
> + rcdu->info = of_device_get_match_data(rcdu->dev);
... and no need to add these two lines.
The idiom is to set rcdu->info based on of_device_get_match_data()
first, and override based on of_device_get_match_data() when needed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
