| Message ID | 20230117135154.387208-6-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Misc patches | expand |
Hi Tomi, Thank you for the patch. On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote: > On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock s/ES1/ES1.x/ Same below. > source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On > non-ES1, only the higher bits are used (bits 21 and 23), and the lower > bits are reserved and should be set to 0 (or not set at all). How do you not set a bit ? :-) > The current code always sets the lower bits, even on non-ES1. I think that's harmless, and not worth making the driver more complex, but I'll stop fighting. > For both DU1 and DU2, on all SoC versions, when writing zeroes to those > bits the input clock is DCLKIN, and thus there's no difference between > ES1 and non-ES1. > > For DU1, writing 0b10 to the bits (or only writing the higher bit) > results in using PLL0 as the input clock, so in this case there's also > no difference between ES1 and non-ES1. > > However, for DU2, writing 0b10 to the bits results in using PLL0 as the > input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 > you need to write 0b11 to select PLL1. > > The current code always writes 0b11 to PLCS0 field to select PLL1 on all > SoC versions, which works but causes an illegal (in the sense of not > allowed by the documentation) write to a reserved bit field. > > To remove the illegal bit write on PLSC0 we need to handle the input dot > clock selection differently for ES1 and non-ES1. > > Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new > rcar_du_device_info entry for the ES1 SoC. Using these, we can always The new entry was added in the previous patch already. > set the bit 21 on PLSC0 when choosing the PLL as the source clock, and > additionally set the bit 20 when on ES1. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + > drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- > 4 files changed, 15 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 f2d3266509cc..8d660a6141bf 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) > | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) > | DPLLCR_STBY; > > - if (rcrtc->index == 1) > + if (rcrtc->index == 1) { > dpllcr |= DPLLCR_PLCS1 > | DPLLCR_INCS_DOTCLKIN1; > - else > + } else { > dpllcr |= DPLLCR_PLCS0 > | DPLLCR_INCS_DOTCLKIN0; > + /* > + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), > + * also bit 20 has to be set to select PLL1 as the > + * clock source. I'd add "On ES2 and newer, PLL1 is selected unconditionally.". > + */ > + if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL) > + dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL; > + } > > rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr); > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > index ba2e069fc0f7..d689f2510081 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = { > | RCAR_DU_FEATURE_VSP1_SOURCE > | RCAR_DU_FEATURE_INTERLACED > | RCAR_DU_FEATURE_TVM_SYNC, > - .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, > + .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY > + | RCAR_DU_QUIRK_H3_ES1_PLL, > .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0), > .routes = { > /* > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > index df87ccab146f..acc3673fefe1 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > @@ -35,6 +35,7 @@ struct rcar_du_device; > > #define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */ > #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1) /* H3 ES1 has pclk stability issue */ > +#define RCAR_DU_QUIRK_H3_ES1_PLL BIT(2) /* H3 ES1 PLL setup differs from non-ES1 */ > > enum rcar_du_output { > RCAR_DU_OUTPUT_DPAD0, > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h > index c1bcb0e8b5b4..94d913f66c8f 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h > @@ -288,7 +288,8 @@ > * isn't implemented by other SoC in the Gen3 family it can safely be set > * unconditionally. > */ This comment should be dropped. > -#define DPLLCR_PLCS0 (3 << 20) > +#define DPLLCR_PLCS0 (1 << 21) > +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL (1 << 20) Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit 20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and newer, PLL1 is selected unconditionally. Could we name the two bits accodingly ? Maybe #define DPLLCR_PLCS0_PLL (1 << 21) #define DPLLCR_PLCS0_PLL1 (1 << 20) > #define DPLLCR_CLKE (1 << 18) > #define DPLLCR_FDPLL(n) ((n) << 12) > #define DPLLCR_N(n) ((n) << 5)
On 18/01/2023 23:38, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote: >> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock > > s/ES1/ES1.x/ > > Same below. Ok. But I do wonder, is there a difference? What's the case when ES1 could be mistaken to mean something else? >> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On >> non-ES1, only the higher bits are used (bits 21 and 23), and the lower >> bits are reserved and should be set to 0 (or not set at all). > > How do you not set a bit ? :-) By leaving it to the value the register already has. But as we don't read the register as a base value here, I guess that comment is a bit misleading. >> The current code always sets the lower bits, even on non-ES1. > > I think that's harmless, and not worth making the driver more complex, > but I'll stop fighting. > >> For both DU1 and DU2, on all SoC versions, when writing zeroes to those >> bits the input clock is DCLKIN, and thus there's no difference between >> ES1 and non-ES1. >> >> For DU1, writing 0b10 to the bits (or only writing the higher bit) >> results in using PLL0 as the input clock, so in this case there's also >> no difference between ES1 and non-ES1. >> >> However, for DU2, writing 0b10 to the bits results in using PLL0 as the >> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 >> you need to write 0b11 to select PLL1. >> >> The current code always writes 0b11 to PLCS0 field to select PLL1 on all >> SoC versions, which works but causes an illegal (in the sense of not >> allowed by the documentation) write to a reserved bit field. >> >> To remove the illegal bit write on PLSC0 we need to handle the input dot >> clock selection differently for ES1 and non-ES1. >> >> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new >> rcar_du_device_info entry for the ES1 SoC. Using these, we can always > > The new entry was added in the previous patch already. Indeed. >> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and >> additionally set the bit 20 when on ES1. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- >> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + >> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- >> 4 files changed, 15 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 f2d3266509cc..8d660a6141bf 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) >> | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) >> | DPLLCR_STBY; >> >> - if (rcrtc->index == 1) >> + if (rcrtc->index == 1) { >> dpllcr |= DPLLCR_PLCS1 >> | DPLLCR_INCS_DOTCLKIN1; >> - else >> + } else { >> dpllcr |= DPLLCR_PLCS0 >> | DPLLCR_INCS_DOTCLKIN0; >> + /* >> + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), >> + * also bit 20 has to be set to select PLL1 as the >> + * clock source. > > I'd add "On ES2 and newer, PLL1 is selected unconditionally.". It's not selected unconditionally, we need to set bit 21. And possibly we need to set bit 20 to 0, although it's not documented what bit 20 would do when set to 1. And is that "ES2.x"? =) How about: * On ES2.x and newer, PLL1 is selected by setting bit * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as * 0. On H3 ES1.x, in addition to setting bit 21, also * bit 20 has to be set to select PLL1 as the clock source. >> + */ >> + if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL) >> + dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL; >> + } >> >> rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr); >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> index ba2e069fc0f7..d689f2510081 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c >> @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = { >> | RCAR_DU_FEATURE_VSP1_SOURCE >> | RCAR_DU_FEATURE_INTERLACED >> | RCAR_DU_FEATURE_TVM_SYNC, >> - .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, >> + .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY >> + | RCAR_DU_QUIRK_H3_ES1_PLL, >> .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0), >> .routes = { >> /* >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h >> index df87ccab146f..acc3673fefe1 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h >> @@ -35,6 +35,7 @@ struct rcar_du_device; >> >> #define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */ >> #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1) /* H3 ES1 has pclk stability issue */ >> +#define RCAR_DU_QUIRK_H3_ES1_PLL BIT(2) /* H3 ES1 PLL setup differs from non-ES1 */ >> >> enum rcar_du_output { >> RCAR_DU_OUTPUT_DPAD0, >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h >> index c1bcb0e8b5b4..94d913f66c8f 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h >> @@ -288,7 +288,8 @@ >> * isn't implemented by other SoC in the Gen3 family it can safely be set >> * unconditionally. >> */ > > This comment should be dropped. Ah, true. >> -#define DPLLCR_PLCS0 (3 << 20) >> +#define DPLLCR_PLCS0 (1 << 21) >> +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL (1 << 20) > > Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit > 20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and > newer, PLL1 is selected unconditionally. Could we name the two bits > accodingly ? Maybe > > #define DPLLCR_PLCS0_PLL (1 << 21) > #define DPLLCR_PLCS0_PLL1 (1 << 20) I'm fine with DPLLCR_PLCS0_PLL, but I do like to make it a bit more obvious that a bit is only for a particular ES/SoC version if it's simple to do. Especially here, as "DPLLCR_PLCS0_PLL1" sounds like you need to set it to use PLL1. Would you be ok with "DPLLCR_PLCS0_H3ES1X_PLL1"? Tomi
Hi Tomi, On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote: > On 18/01/2023 23:38, Laurent Pinchart wrote: > > On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote: > >> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock > > > > s/ES1/ES1.x/ > > > > Same below. > > Ok. But I do wonder, is there a difference? What's the case when ES1 > could be mistaken to mean something else? It's just for consistency I suppose. No big deal. > >> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On > >> non-ES1, only the higher bits are used (bits 21 and 23), and the lower > >> bits are reserved and should be set to 0 (or not set at all). > > > > How do you not set a bit ? :-) > > By leaving it to the value the register already has. But as we don't > read the register as a base value here, I guess that comment is a bit > misleading. > > >> The current code always sets the lower bits, even on non-ES1. > > > > I think that's harmless, and not worth making the driver more complex, > > but I'll stop fighting. > > > >> For both DU1 and DU2, on all SoC versions, when writing zeroes to those > >> bits the input clock is DCLKIN, and thus there's no difference between > >> ES1 and non-ES1. > >> > >> For DU1, writing 0b10 to the bits (or only writing the higher bit) > >> results in using PLL0 as the input clock, so in this case there's also > >> no difference between ES1 and non-ES1. > >> > >> However, for DU2, writing 0b10 to the bits results in using PLL0 as the > >> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 > >> you need to write 0b11 to select PLL1. > >> > >> The current code always writes 0b11 to PLCS0 field to select PLL1 on all > >> SoC versions, which works but causes an illegal (in the sense of not > >> allowed by the documentation) write to a reserved bit field. > >> > >> To remove the illegal bit write on PLSC0 we need to handle the input dot > >> clock selection differently for ES1 and non-ES1. > >> > >> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new > >> rcar_du_device_info entry for the ES1 SoC. Using these, we can always > > > > The new entry was added in the previous patch already. > > Indeed. > > >> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and > >> additionally set the bit 20 when on ES1. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- > >> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- > >> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + > >> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- > >> 4 files changed, 15 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 f2d3266509cc..8d660a6141bf 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) > >> | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) > >> | DPLLCR_STBY; > >> > >> - if (rcrtc->index == 1) > >> + if (rcrtc->index == 1) { > >> dpllcr |= DPLLCR_PLCS1 > >> | DPLLCR_INCS_DOTCLKIN1; > >> - else > >> + } else { > >> dpllcr |= DPLLCR_PLCS0 > >> | DPLLCR_INCS_DOTCLKIN0; > >> + /* > >> + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), > >> + * also bit 20 has to be set to select PLL1 as the > >> + * clock source. > > > > I'd add "On ES2 and newer, PLL1 is selected unconditionally.". > > It's not selected unconditionally, we need to set bit 21. And possibly > we need to set bit 20 to 0, although it's not documented what bit 20 > would do when set to 1. We currently set bit 20 to 1 and it works, so I concluded that bit 20 is ignored. That's what I meant by PLL1 being selected automatically, between PLL0 and PLL1. We still need to select PLL instead of DCLKIN with bit 21. > And is that "ES2.x"? =) Good point :-) > How about: > > * On ES2.x and newer, PLL1 is selected by setting bit > * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as > * 0. On H3 ES1.x, in addition to setting bit 21, also > * bit 20 has to be set to select PLL1 as the clock source. What I'd like to capture in the comment is that the clock topology is bit 20 | bit 21 v | |\ v PLL0 --> |0| |\ PLL1 --> |1| --> |1| --> |/ /-> |0| | |/ DCLKIN ------/ on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first mux is hardcoded to PLL1. > >> + */ > >> + if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL) > >> + dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL; > >> + } > >> > >> rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr); > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> index ba2e069fc0f7..d689f2510081 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c > >> @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = { > >> | RCAR_DU_FEATURE_VSP1_SOURCE > >> | RCAR_DU_FEATURE_INTERLACED > >> | RCAR_DU_FEATURE_TVM_SYNC, > >> - .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, > >> + .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY > >> + | RCAR_DU_QUIRK_H3_ES1_PLL, > >> .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0), > >> .routes = { > >> /* > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > >> index df87ccab146f..acc3673fefe1 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h > >> @@ -35,6 +35,7 @@ struct rcar_du_device; > >> > >> #define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */ > >> #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1) /* H3 ES1 has pclk stability issue */ > >> +#define RCAR_DU_QUIRK_H3_ES1_PLL BIT(2) /* H3 ES1 PLL setup differs from non-ES1 */ > >> > >> enum rcar_du_output { > >> RCAR_DU_OUTPUT_DPAD0, > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h > >> index c1bcb0e8b5b4..94d913f66c8f 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h > >> @@ -288,7 +288,8 @@ > >> * isn't implemented by other SoC in the Gen3 family it can safely be set > >> * unconditionally. > >> */ > > > > This comment should be dropped. > > Ah, true. > > >> -#define DPLLCR_PLCS0 (3 << 20) > >> +#define DPLLCR_PLCS0 (1 << 21) > >> +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL (1 << 20) > > > > Bit 21 selects between DCLKIN (when 0) and PLL (when 1). On ES1.x, bit > > 20 selects between PLL0 (when 0) and PLL1 (when 1), while on ES2 and > > newer, PLL1 is selected unconditionally. Could we name the two bits > > accodingly ? Maybe > > > > #define DPLLCR_PLCS0_PLL (1 << 21) > > #define DPLLCR_PLCS0_PLL1 (1 << 20) > > I'm fine with DPLLCR_PLCS0_PLL, but I do like to make it a bit more > obvious that a bit is only for a particular ES/SoC version if it's > simple to do. Especially here, as "DPLLCR_PLCS0_PLL1" sounds like you > need to set it to use PLL1. > > Would you be ok with "DPLLCR_PLCS0_H3ES1X_PLL1"? A bit long but I suppose I can live with that.
On 19/01/2023 11:40, Laurent Pinchart wrote: > Hi Tomi, > > On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote: >> On 18/01/2023 23:38, Laurent Pinchart wrote: >>> On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote: >>>> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock >>> >>> s/ES1/ES1.x/ >>> >>> Same below. >> >> Ok. But I do wonder, is there a difference? What's the case when ES1 >> could be mistaken to mean something else? > > It's just for consistency I suppose. No big deal. > >>>> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On >>>> non-ES1, only the higher bits are used (bits 21 and 23), and the lower >>>> bits are reserved and should be set to 0 (or not set at all). >>> >>> How do you not set a bit ? :-) >> >> By leaving it to the value the register already has. But as we don't >> read the register as a base value here, I guess that comment is a bit >> misleading. >> >>>> The current code always sets the lower bits, even on non-ES1. >>> >>> I think that's harmless, and not worth making the driver more complex, >>> but I'll stop fighting. >>> >>>> For both DU1 and DU2, on all SoC versions, when writing zeroes to those >>>> bits the input clock is DCLKIN, and thus there's no difference between >>>> ES1 and non-ES1. >>>> >>>> For DU1, writing 0b10 to the bits (or only writing the higher bit) >>>> results in using PLL0 as the input clock, so in this case there's also >>>> no difference between ES1 and non-ES1. >>>> >>>> However, for DU2, writing 0b10 to the bits results in using PLL0 as the >>>> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 >>>> you need to write 0b11 to select PLL1. >>>> >>>> The current code always writes 0b11 to PLCS0 field to select PLL1 on all >>>> SoC versions, which works but causes an illegal (in the sense of not >>>> allowed by the documentation) write to a reserved bit field. >>>> >>>> To remove the illegal bit write on PLSC0 we need to handle the input dot >>>> clock selection differently for ES1 and non-ES1. >>>> >>>> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new >>>> rcar_du_device_info entry for the ES1 SoC. Using these, we can always >>> >>> The new entry was added in the previous patch already. >> >> Indeed. >> >>>> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and >>>> additionally set the bit 20 when on ES1. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >>>> --- >>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + >>>> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- >>>> 4 files changed, 15 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 f2d3266509cc..8d660a6141bf 100644 >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c >>>> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) >>>> | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) >>>> | DPLLCR_STBY; >>>> >>>> - if (rcrtc->index == 1) >>>> + if (rcrtc->index == 1) { >>>> dpllcr |= DPLLCR_PLCS1 >>>> | DPLLCR_INCS_DOTCLKIN1; >>>> - else >>>> + } else { >>>> dpllcr |= DPLLCR_PLCS0 >>>> | DPLLCR_INCS_DOTCLKIN0; >>>> + /* >>>> + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), >>>> + * also bit 20 has to be set to select PLL1 as the >>>> + * clock source. >>> >>> I'd add "On ES2 and newer, PLL1 is selected unconditionally.". >> >> It's not selected unconditionally, we need to set bit 21. And possibly >> we need to set bit 20 to 0, although it's not documented what bit 20 >> would do when set to 1. > > We currently set bit 20 to 1 and it works, so I concluded that bit 20 is Ah, right, we do set it to 1. > ignored. That's what I meant by PLL1 being selected automatically, > between PLL0 and PLL1. We still need to select PLL instead of DCLKIN > with bit 21. > >> And is that "ES2.x"? =) > > Good point :-) > >> How about: >> >> * On ES2.x and newer, PLL1 is selected by setting bit >> * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as >> * 0. On H3 ES1.x, in addition to setting bit 21, also >> * bit 20 has to be set to select PLL1 as the clock source. > > What I'd like to capture in the comment is that the clock topology is > > bit 20 > | bit 21 > v | > |\ v > PLL0 --> |0| |\ > PLL1 --> |1| --> |1| --> > |/ /-> |0| > | |/ > DCLKIN ------/ > > on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first > mux is hardcoded to PLL1. Isn't that the stuff that's supposed to be found by reading the manual? I could, of course, copy the above picture, and draw a similar one for ES2+, but that feels like just useless copying of the manual... Doesn't the above comment already describe why we set the bits as we do? I could change the comment to talk about muxes, if you think that's a better approach. But, if I recall right, the manuals don't talk about muxes, just "set this to X to use PLL1", so my comment above approaches from that direction. Maybe something in this direction, then: On ES2.x we have a single mux controlled via bit 21, which selects between DCLKIN source (bit 21 = 0) and a PLL source (bit 21 = 1), where the PLL is always PLL1. On ES1.x we have an additional mux, controlled via bit 20, for choosing between PLL0 (bit 20 = 0) and PLL1 (bit 20 = 1). We always want to use PLL1, so on ES1.x, in addition to setting bit 21, we need to set the bit 20. Tomi
Hi Tomi, On Thu, Jan 19, 2023 at 12:24:48PM +0200, Tomi Valkeinen wrote: > On 19/01/2023 11:40, Laurent Pinchart wrote: > > On Thu, Jan 19, 2023 at 11:17:58AM +0200, Tomi Valkeinen wrote: > >> On 18/01/2023 23:38, Laurent Pinchart wrote: > >>> On Tue, Jan 17, 2023 at 03:51:53PM +0200, Tomi Valkeinen wrote: > >>>> On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock > >>> > >>> s/ES1/ES1.x/ > >>> > >>> Same below. > >> > >> Ok. But I do wonder, is there a difference? What's the case when ES1 > >> could be mistaken to mean something else? > > > > It's just for consistency I suppose. No big deal. > > > >>>> source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On > >>>> non-ES1, only the higher bits are used (bits 21 and 23), and the lower > >>>> bits are reserved and should be set to 0 (or not set at all). > >>> > >>> How do you not set a bit ? :-) > >> > >> By leaving it to the value the register already has. But as we don't > >> read the register as a base value here, I guess that comment is a bit > >> misleading. > >> > >>>> The current code always sets the lower bits, even on non-ES1. > >>> > >>> I think that's harmless, and not worth making the driver more complex, > >>> but I'll stop fighting. > >>> > >>>> For both DU1 and DU2, on all SoC versions, when writing zeroes to those > >>>> bits the input clock is DCLKIN, and thus there's no difference between > >>>> ES1 and non-ES1. > >>>> > >>>> For DU1, writing 0b10 to the bits (or only writing the higher bit) > >>>> results in using PLL0 as the input clock, so in this case there's also > >>>> no difference between ES1 and non-ES1. > >>>> > >>>> However, for DU2, writing 0b10 to the bits results in using PLL0 as the > >>>> input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 > >>>> you need to write 0b11 to select PLL1. > >>>> > >>>> The current code always writes 0b11 to PLCS0 field to select PLL1 on all > >>>> SoC versions, which works but causes an illegal (in the sense of not > >>>> allowed by the documentation) write to a reserved bit field. > >>>> > >>>> To remove the illegal bit write on PLSC0 we need to handle the input dot > >>>> clock selection differently for ES1 and non-ES1. > >>>> > >>>> Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new > >>>> rcar_du_device_info entry for the ES1 SoC. Using these, we can always > >>> > >>> The new entry was added in the previous patch already. > >> > >> Indeed. > >> > >>>> set the bit 21 on PLSC0 when choosing the PLL as the source clock, and > >>>> additionally set the bit 20 when on ES1. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >>>> --- > >>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- > >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- > >>>> drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + > >>>> drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- > >>>> 4 files changed, 15 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 f2d3266509cc..8d660a6141bf 100644 > >>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > >>>> @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) > >>>> | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) > >>>> | DPLLCR_STBY; > >>>> > >>>> - if (rcrtc->index == 1) > >>>> + if (rcrtc->index == 1) { > >>>> dpllcr |= DPLLCR_PLCS1 > >>>> | DPLLCR_INCS_DOTCLKIN1; > >>>> - else > >>>> + } else { > >>>> dpllcr |= DPLLCR_PLCS0 > >>>> | DPLLCR_INCS_DOTCLKIN0; > >>>> + /* > >>>> + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), > >>>> + * also bit 20 has to be set to select PLL1 as the > >>>> + * clock source. > >>> > >>> I'd add "On ES2 and newer, PLL1 is selected unconditionally.". > >> > >> It's not selected unconditionally, we need to set bit 21. And possibly > >> we need to set bit 20 to 0, although it's not documented what bit 20 > >> would do when set to 1. > > > > We currently set bit 20 to 1 and it works, so I concluded that bit 20 is > > Ah, right, we do set it to 1. > > > ignored. That's what I meant by PLL1 being selected automatically, > > between PLL0 and PLL1. We still need to select PLL instead of DCLKIN > > with bit 21. > > > >> And is that "ES2.x"? =) > > > > Good point :-) > > > >> How about: > >> > >> * On ES2.x and newer, PLL1 is selected by setting bit > >> * 21 (PLCS0) to 1 and keeping the (reserved) bit 20 as > >> * 0. On H3 ES1.x, in addition to setting bit 21, also > >> * bit 20 has to be set to select PLL1 as the clock source. > > > > What I'd like to capture in the comment is that the clock topology is > > > > bit 20 > > | bit 21 > > v | > > |\ v > > PLL0 --> |0| |\ > > PLL1 --> |1| --> |1| --> > > |/ /-> |0| > > | |/ > > DCLKIN ------/ > > > > on H3 ES1.x, while on newer revisions, bit 20 is ignored and the first > > mux is hardcoded to PLL1. > > Isn't that the stuff that's supposed to be found by reading the manual? > I could, of course, copy the above picture, and draw a similar one for > ES2+, but that feels like just useless copying of the manual... It would, if it was in the documentation :-) The above diagram is what I deduced from the documentation, but it's certainly not explicit, and the two bits 20 and 21 are not named separately. We don't need to capture this in ascii art, but some kind of summary would be nice. > Doesn't the above comment already describe why we set the bits as we do? > I could change the comment to talk about muxes, if you think that's a > better approach. But, if I recall right, the manuals don't talk about > muxes, just "set this to X to use PLL1", so my comment above approaches > from that direction. > > Maybe something in this direction, then: > > On ES2.x we have a single mux controlled via bit 21, which selects > between DCLKIN source (bit 21 = 0) and a PLL source (bit 21 = 1), where > the PLL is always PLL1. On ES1.x we have an additional mux, controlled > via bit 20, for choosing between PLL0 (bit 20 = 0) and PLL1 (bit 20 = > 1). We always want to use PLL1, so on ES1.x, in addition to setting bit > 21, we need to set the bit 20. Seems good to me.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index f2d3266509cc..8d660a6141bf 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -245,12 +245,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) | DPLLCR_N(dpll.n) | DPLLCR_M(dpll.m) | DPLLCR_STBY; - if (rcrtc->index == 1) + if (rcrtc->index == 1) { dpllcr |= DPLLCR_PLCS1 | DPLLCR_INCS_DOTCLKIN1; - else + } else { dpllcr |= DPLLCR_PLCS0 | DPLLCR_INCS_DOTCLKIN0; + /* + * On H3 ES1.x, in addition to setting bit 21 (PLCS0), + * also bit 20 has to be set to select PLL1 as the + * clock source. + */ + if (rcdu->info->quirks & RCAR_DU_QUIRK_H3_ES1_PLL) + dpllcr |= DPLLCR_PLCS0_H3ES1X_PLL1_SEL; + } rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ba2e069fc0f7..d689f2510081 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -394,7 +394,8 @@ static const struct rcar_du_device_info rcar_du_r8a7795_es1_info = { | RCAR_DU_FEATURE_VSP1_SOURCE | RCAR_DU_FEATURE_INTERLACED | RCAR_DU_FEATURE_TVM_SYNC, - .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY, + .quirks = RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY + | RCAR_DU_QUIRK_H3_ES1_PLL, .channels_mask = BIT(3) | BIT(2) | BIT(1) | BIT(0), .routes = { /* diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index df87ccab146f..acc3673fefe1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -35,6 +35,7 @@ struct rcar_du_device; #define RCAR_DU_QUIRK_ALIGN_128B BIT(0) /* Align pitches to 128 bytes */ #define RCAR_DU_QUIRK_H3_ES1_PCLK_STABILITY BIT(1) /* H3 ES1 has pclk stability issue */ +#define RCAR_DU_QUIRK_H3_ES1_PLL BIT(2) /* H3 ES1 PLL setup differs from non-ES1 */ enum rcar_du_output { RCAR_DU_OUTPUT_DPAD0, diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h index c1bcb0e8b5b4..94d913f66c8f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h @@ -288,7 +288,8 @@ * isn't implemented by other SoC in the Gen3 family it can safely be set * unconditionally. */ -#define DPLLCR_PLCS0 (3 << 20) +#define DPLLCR_PLCS0 (1 << 21) +#define DPLLCR_PLCS0_H3ES1X_PLL1_SEL (1 << 20) #define DPLLCR_CLKE (1 << 18) #define DPLLCR_FDPLL(n) ((n) << 12) #define DPLLCR_N(n) ((n) << 5)
On H3 ES1 two bits in DPLLCR are used to select the DU input dot clock source. These are bits 20 and 21 for DU2, and bits 22 and 23 for DU1. On non-ES1, only the higher bits are used (bits 21 and 23), and the lower bits are reserved and should be set to 0 (or not set at all). The current code always sets the lower bits, even on non-ES1. For both DU1 and DU2, on all SoC versions, when writing zeroes to those bits the input clock is DCLKIN, and thus there's no difference between ES1 and non-ES1. For DU1, writing 0b10 to the bits (or only writing the higher bit) results in using PLL0 as the input clock, so in this case there's also no difference between ES1 and non-ES1. However, for DU2, writing 0b10 to the bits results in using PLL0 as the input clock on ES1, whereas on non-ES1 it results in using PLL1. On ES1 you need to write 0b11 to select PLL1. The current code always writes 0b11 to PLCS0 field to select PLL1 on all SoC versions, which works but causes an illegal (in the sense of not allowed by the documentation) write to a reserved bit field. To remove the illegal bit write on PLSC0 we need to handle the input dot clock selection differently for ES1 and non-ES1. Add a new quirk, RCAR_DU_QUIRK_H3_ES1_PLL, for this, and a new rcar_du_device_info entry for the ES1 SoC. Using these, we can always set the bit 21 on PLSC0 when choosing the PLL as the source clock, and additionally set the bit 20 when on ES1. Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 12 ++++++++++-- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 + drivers/gpu/drm/rcar-du/rcar_du_regs.h | 3 ++- 4 files changed, 15 insertions(+), 4 deletions(-)