Hi Laurent, there are a few things currently in-flight for DU on my side: - non-DPLL channel clock source selction - ESCR/OTAR handling for non-DPLL channel
Before taking those two functional changes series into account, could you include these three cosmetic changes in your tree?
Patches based on your drm/du/next branch, with the following patch applied on top: '01449a9779b8 (" drm: rcar-du: Rework clock configuration based on hardware limits")'
Tested on Salvator-X M3-W with kms-modes-tests.py: no functional changes
Thanks j
Jacopo Mondi (3): drm: rcar-du: Rename and document dpll_ch field drm: rcar-du: Write ESCR register per channel drm: rcar-du: Write OTAR register per channel
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 7 +++---- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 6 +++--- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 8 ++++---- 4 files changed, 12 insertions(+), 12 deletions(-)
-- 2.7.4
Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for consistency with the 'channels_mask' field defined in 'struct rcar_du_device_info'.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 6 +++--- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 2664336..5454884 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) u32 dsmr; u32 escr;
- if (rcdu->info->dpll_ch & (1 << rcrtc->index)) { + if (rcdu->info->dpll_mask & (1 << rcrtc->index)) { unsigned long target = mode_clock; struct dpll_info dpll = { 0 }; unsigned long extclk; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..b42145c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -215,7 +215,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = { }, }, .num_lvds = 1, - .dpll_ch = BIT(2) | BIT(1), + .dpll_mask = BIT(2) | BIT(1), };
static const struct rcar_du_device_info rcar_du_r8a7796_info = { @@ -243,7 +243,7 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = { }, }, .num_lvds = 1, - .dpll_ch = BIT(1), + .dpll_mask = BIT(1), };
static const struct rcar_du_device_info rcar_du_r8a77965_info = { @@ -271,7 +271,7 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = { }, }, .num_lvds = 1, - .dpll_ch = BIT(1), + .dpll_mask = BIT(1), };
static const struct rcar_du_device_info rcar_du_r8a77970_info = { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..6453b33 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -55,6 +55,7 @@ struct rcar_du_output_routing { * @channels_mask: bit mask of available DU channels * @routes: array of CRTC to output routes, indexed by output (RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders + * @dpll_mask: mask of DU channels equipped with a DPLL */ struct rcar_du_device_info { unsigned int gen; @@ -63,7 +64,7 @@ struct rcar_du_device_info { unsigned int channels_mask; struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX]; unsigned int num_lvds; - unsigned int dpll_ch; + unsigned int dpll_mask; };
#define RCAR_DU_MAX_CRTCS 4
Hi Jacopo,
Thank you for the patch.
On Wednesday, 22 August 2018 10:21:47 EEST Jacopo Mondi wrote:
Document and re-name the 'dpll_ch' field to a more precise 'dpll_mask' for consistency with the 'channels_mask' field defined in 'struct rcar_du_device_info'.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 6 +++--- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 2664336..5454884 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -211,7 +211,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) u32 dsmr; u32 escr;
- if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
- if (rcdu->info->dpll_mask & (1 << rcrtc->index)) { unsigned long target = mode_clock; struct dpll_info dpll = { 0 }; unsigned long extclk;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..b42145c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -215,7 +215,7 @@ static const struct rcar_du_device_info rcar_du_r8a7795_info = { }, }, .num_lvds = 1,
- .dpll_ch = BIT(2) | BIT(1),
- .dpll_mask = BIT(2) | BIT(1),
};
static const struct rcar_du_device_info rcar_du_r8a7796_info = { @@ -243,7 +243,7 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = { }, }, .num_lvds = 1,
- .dpll_ch = BIT(1),
- .dpll_mask = BIT(1),
};
static const struct rcar_du_device_info rcar_du_r8a77965_info = { @@ -271,7 +271,7 @@ static const struct rcar_du_device_info rcar_du_r8a77965_info = { }, }, .num_lvds = 1,
- .dpll_ch = BIT(1),
- .dpll_mask = BIT(1),
};
static const struct rcar_du_device_info rcar_du_r8a77970_info = { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index b3a25e8..6453b33 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -55,6 +55,7 @@ struct rcar_du_output_routing {
- @channels_mask: bit mask of available DU channels
- @routes: array of CRTC to output routes, indexed by output
(RCAR_DU_OUTPUT_*) * @num_lvds: number of internal LVDS encoders
- @dpll_mask: mask of DU channels equipped with a DPLL
I'd way "bit mask" instead of "mask" in the description. Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
and taken in my tree.
*/ struct rcar_du_device_info { unsigned int gen; @@ -63,7 +64,7 @@ struct rcar_du_device_info { unsigned int channels_mask; struct rcar_du_output_routing routes[RCAR_DU_OUTPUT_MAX]; unsigned int num_lvds;
- unsigned int dpll_ch;
- unsigned int dpll_mask;
};
#define RCAR_DU_MAX_CRTCS 4
The ESCR registers offset definition is confusing, as each channel is equipped with an ESCR register instance, but the names suggest only ESCR and ESCR2 are taken into account.
Rename the offsets to a name that includes the channels they apply to, and write them to each channel with 'rcar_du_crtc_write()'.
Cosmetic patch, no functional changes intended.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5454884..714c1fc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) } }
- rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR, - escr); + rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr); rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
/* Signal polarities */ diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -492,8 +492,8 @@ * External Synchronization Control Registers */
-#define ESCR 0x10000 -#define ESCR2 0x31000 +#define ESCR02 0x10000 +#define ESCR13 0x01000 #define ESCR_DCLKOINV (1 << 25) #define ESCR_DCLKSEL_DCLKIN (0 << 20) #define ESCR_DCLKSEL_CLKS (1 << 20)
Hi Jacopo,
Thank you for the patch.
On Wednesday, 22 August 2018 10:21:48 EEST Jacopo Mondi wrote:
The ESCR registers offset definition is confusing, as each channel is equipped with an ESCR register instance, but the names suggest only ESCR and ESCR2 are taken into account.
Rename the offsets to a name that includes the channels they apply to, and write them to each channel with 'rcar_du_crtc_write()'.
Cosmetic patch, no functional changes intended.
I think patches 2/3 and 3/3 can be squashed together, there's no real reason to keep them separate. I propose updating the commit message to
"drm: rcar-du: Write ESCR and OTAR as CRTC registers
The ESCR and OTAR registers exist in each DU channel, but at different offsets for odd and even channels. This led to usage of the group register access API to write them, with offsets macros named ESCR/OTAR and ESCR2/OTAR2 for the first and second ESCR/OTAR register in the group respectively.
The names are confusing as it suggests that the ESCR/OTAR registers for DU0 and DU2 are taken into account, especially with writes performed to the group register access API.
Rename the offsets to ESCR/OTAR02 and ESCR/OTAR13, and use the CRTC register access API to clarify the code. The offsets values are updated accordingly.
Cosmetic patch, no functional changes intended."
Otherwise the patches look good to me, so
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
and applied the squashed version to my tree.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5454884..714c1fc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) } }
- rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
escr);
rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr); rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
/* Signal polarities */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -492,8 +492,8 @@
- External Synchronization Control Registers
*/
-#define ESCR 0x10000 -#define ESCR2 0x31000 +#define ESCR02 0x10000 +#define ESCR13 0x01000 #define ESCR_DCLKOINV (1 << 25) #define ESCR_DCLKSEL_DCLKIN (0 << 20) #define ESCR_DCLKSEL_CLKS (1 << 20)
Hi Jacopo,
On 22/08/18 08:21, Jacopo Mondi wrote:
The ESCR registers offset definition is confusing, as each channel is equipped with an ESCR register instance, but the names suggest only ESCR and ESCR2 are taken into account.
Rename the offsets to a name that includes the channels they apply to, and write them to each channel with 'rcar_du_crtc_write()'.
Cosmetic patch, no functional changes intended.
Noted, ...
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5454884..714c1fc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) } }
- rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
escr);
rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr); rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
/* Signal polarities */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -492,8 +492,8 @@
- External Synchronization Control Registers
*/
-#define ESCR 0x10000 -#define ESCR2 0x31000 +#define ESCR02 0x10000 +#define ESCR13 0x01000
Assertion failed at: ASSERT(ESCR2 == ESCR13)
This looks intentional ? but that makes this a bit more than a cosmetic change?
#define ESCR_DCLKOINV (1 << 25) #define ESCR_DCLKSEL_DCLKIN (0 << 20) #define ESCR_DCLKSEL_CLKS (1 << 20)
Hi Jacopo,
On 30/08/18 17:00, Kieran Bingham wrote:
Hi Jacopo,
On 22/08/18 08:21, Jacopo Mondi wrote:
The ESCR registers offset definition is confusing, as each channel is equipped with an ESCR register instance, but the names suggest only ESCR and ESCR2 are taken into account.
Rename the offsets to a name that includes the channels they apply to, and write them to each channel with 'rcar_du_crtc_write()'.
Cosmetic patch, no functional changes intended.
Noted, ...
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5454884..714c1fc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -294,8 +294,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) } }
- rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
escr);
- rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr);
rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
/* Signal polarities */ diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index 9dfd220..ebc4aea 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -492,8 +492,8 @@
- External Synchronization Control Registers
*/
-#define ESCR 0x10000 -#define ESCR2 0x31000 +#define ESCR02 0x10000 +#define ESCR13 0x01000
Assertion failed at: ASSERT(ESCR2 == ESCR13)
This looks intentional ? but that makes this a bit more than a cosmetic change?
Aha - sorry - I see.
It looks like the '0x3' from '0x31000' was providing the offset into the group I assume.
I'm surprised an equivalent offset wasn't removed from the ESCR02.
But I assume this is handled by the change of "rcar_du_crtc_write() -> rcar_du_group_write()"?
Regards
Kieran
#define ESCR_DCLKOINV (1 << 25) #define ESCR_DCLKSEL_DCLKIN (0 << 20) #define ESCR_DCLKSEL_CLKS (1 << 20)
The OTAR registers offset definition is confusing, as each channel is equipped with an OTAR register instance, but the names suggest only OTAR and OTAR2 are taken into account.
Rename the offsets to a name that includes the channels they apply to, and write them to each channel with 'rcar_du_crtc_write()'.
Cosmetic patch, no functional changes intended.
Signed-off-by: Jacopo Mondi jacopo+renesas@jmondi.org --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_regs.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 714c1fc..e2d9653 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -295,7 +295,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) }
rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr); - rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0); + rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0);
/* Signal polarities */ dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index ebc4aea..4144950 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -504,8 +504,8 @@ #define ESCR_SYNCSEL_EXHSYNC (3 << 8) #define ESCR_FRQSEL_MASK (0x3f << 0)
-#define OTAR 0x10004 -#define OTAR2 0x31004 +#define OTAR02 0x10004 +#define OTAR13 0x01004
/* ----------------------------------------------------------------------------- * Dual Display Output Control Registers
dri-devel@lists.freedesktop.org
-
Jacopo Mondi -
Kieran Bingham -
Laurent Pinchart