| Message ID | 236762130.3394112.1556751009128.JavaMail.zimbra@raptorengineeringinc.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/3] drm/aspeed: Preserve DVO configuration bits during initialization | expand |
On Thu, 2 May 2019, at 08:20, Timothy Pearson wrote: > There appears to be a significant error in the pinmux table starting on > page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) > requirement is incorrectly applied to multiple digital video input (DVI) > muxed pins, and no DVI-specific condition is provided. This results in > the serial devices incorrectly overriding the DVO pinmuxes and disabling > the DVO pins. > > Create a new condition code (COND6) for DVI enable, and update the most > seriously affected pins to use the new condition code. > > Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > index 6f357a11e89a..676f90d3c5f3 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > @@ -29,6 +29,7 @@ > > #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } > #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } > +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } > > /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ > #define LHCR0 0x20 > @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); > > #define T1 89 > #define T1_DESC SIG_DESC_SET(SCU84, 17) > -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); > -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); > +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI condition) requires SCU90[5]=0b1, but your introduction of COND6 requires SCU90[5:4]=0b00 for the mux configuration to succeed. This can't work - bit 5 of SCU90 can not simultaneously take the values 1 and 0. Ryan: Can we just drop the COND2 requirement for function 2 of balls T1, U2, P4 and P3? I think that gets us where we need to be? Cheers, Andrew > MS_PIN_DECL(T1, GPIOL1, VPIDE, NDCD1); > FUNC_GROUP_DECL(NDCD1, T1); > > @@ -674,22 +675,22 @@ FUNC_GROUP_DECL(NDSR1, U1); > > #define U2 91 > #define U2_DESC SIG_DESC_SET(SCU84, 19) > -SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND2); > -SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND6); > +SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND6); > MS_PIN_DECL(U2, GPIOL3, VPIHS, NRI1); > FUNC_GROUP_DECL(NRI1, U2); > > #define P4 92 > #define P4_DESC SIG_DESC_SET(SCU84, 20) > -SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND2); > -SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND6); > +SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND6); > MS_PIN_DECL(P4, GPIOL4, VPIVS, NDTR1); > FUNC_GROUP_DECL(NDTR1, P4); > > #define P3 93 > #define P3_DESC SIG_DESC_SET(SCU84, 21) > -SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND2); > -SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND6); > +SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND6); > MS_PIN_DECL(P3, GPIOL5, VPICLK, NRTS1); > FUNC_GROUP_DECL(NRTS1, P3); > > -- > 2.11.0 >
> There appears to be a significant error in the pinmux table starting > on page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 > (DVO) requirement is incorrectly applied to multiple digital video > input (DVI) muxed pins, and no DVI-specific condition is provided. > This results in the serial devices incorrectly overriding the DVO > pinmuxes and disabling the DVO pins. > > Create a new condition code (COND6) for DVI enable, and update the > most seriously affected pins to use the new condition code. > > Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > index 6f357a11e89a..676f90d3c5f3 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > @@ -29,6 +29,7 @@ > > #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } > #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } > +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } > > /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ > #define LHCR0 0x20 > @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, > SIG_DESC_SET(SCU84, 16)); > > #define T1 89 > #define T1_DESC SIG_DESC_SET(SCU84, 17) > -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, > COND2); -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, > +COND6); SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); >>I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI condition) requires SCU90[5]=0b1, but your >>introduction of COND6 requires SCU90[5:4]=0b00 for the mux configuration to succeed. This can't work - bit 5 of SCU90 can not >>simultaneously take the values 1 and 0. >>Ryan: Can we just drop the COND2 requirement for function 2 of balls T1, U2, P4 and P3? >>I think that gets us where we need to be? No, that will have some impact, we don't know. > MS_PIN_DECL(T1, GPIOL1, VPIDE, NDCD1); FUNC_GROUP_DECL(NDCD1, T1); > > @@ -674,22 +675,22 @@ FUNC_GROUP_DECL(NDSR1, U1); > > #define U2 91 > #define U2_DESC SIG_DESC_SET(SCU84, 19) > -SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, > COND2); -SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, > +COND6); SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND6); > MS_PIN_DECL(U2, GPIOL3, VPIHS, NRI1); FUNC_GROUP_DECL(NRI1, U2); > > #define P4 92 > #define P4_DESC SIG_DESC_SET(SCU84, 20) > -SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, > COND2); -SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, > +COND6); SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND6); > MS_PIN_DECL(P4, GPIOL4, VPIVS, NDTR1); FUNC_GROUP_DECL(NDTR1, P4); > > #define P3 93 > #define P3_DESC SIG_DESC_SET(SCU84, 21) > -SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, > COND2); -SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND2); > +SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, > +COND6); SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND6); > MS_PIN_DECL(P3, GPIOL5, VPICLK, NRTS1); FUNC_GROUP_DECL(NRTS1, P3); > > -- > 2.11.0 >
----- Original Message ----- > From: "Andrew Jeffery" <andrew@aj.id.au> > To: "Timothy Pearson" <tpearson@raptorengineering.com>, "linux-aspeed" <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" > <ryan_chen@aspeedtech.com> > Sent: Thursday, May 2, 2019 12:51:00 AM > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs on AST2500 devices > On Thu, 2 May 2019, at 08:20, Timothy Pearson wrote: >> There appears to be a significant error in the pinmux table starting on >> page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) >> requirement is incorrectly applied to multiple digital video input (DVI) >> muxed pins, and no DVI-specific condition is provided. This results in >> the serial devices incorrectly overriding the DVO pinmuxes and disabling >> the DVO pins. >> >> Create a new condition code (COND6) for DVI enable, and update the most >> seriously affected pins to use the new condition code. >> >> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> >> --- >> drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> index 6f357a11e89a..676f90d3c5f3 100644 >> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> @@ -29,6 +29,7 @@ >> >> #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } >> #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } >> +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } >> >> /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ >> #define LHCR0 0x20 >> @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); >> >> #define T1 89 >> #define T1_DESC SIG_DESC_SET(SCU84, 17) >> -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); >> -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); >> +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); >> +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); > > I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI > condition) > requires SCU90[5]=0b1, but your introduction of COND6 requires SCU90[5:4]=0b00 > for > the mux configuration to succeed. This can't work - bit 5 of SCU90 can not > simultaneously take the values 1 and 0. That's correct -- I don't have hardware that supports DVI available to test with. > Ryan: Can we just drop the COND2 requirement for function 2 of balls T1, U2, P4 > and P3? > I think that gets us where we need to be? > > Cheers, > > Andrew > >> MS_PIN_DECL(T1, GPIOL1, VPIDE, NDCD1); >> FUNC_GROUP_DECL(NDCD1, T1); >> >> @@ -674,22 +675,22 @@ FUNC_GROUP_DECL(NDSR1, U1); >> >> #define U2 91 >> #define U2_DESC SIG_DESC_SET(SCU84, 19) >> -SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND2); >> -SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND2); >> +SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND6); >> +SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND6); >> MS_PIN_DECL(U2, GPIOL3, VPIHS, NRI1); >> FUNC_GROUP_DECL(NRI1, U2); >> >> #define P4 92 >> #define P4_DESC SIG_DESC_SET(SCU84, 20) >> -SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND2); >> -SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND2); >> +SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND6); >> +SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND6); >> MS_PIN_DECL(P4, GPIOL4, VPIVS, NDTR1); >> FUNC_GROUP_DECL(NDTR1, P4); >> >> #define P3 93 >> #define P3_DESC SIG_DESC_SET(SCU84, 21) >> -SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND2); >> -SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND2); >> +SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND6); >> +SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND6); >> MS_PIN_DECL(P3, GPIOL5, VPICLK, NRTS1); >> FUNC_GROUP_DECL(NRTS1, P3); >> >> -- >> 2.11.0
On Thu, 2 May 2019, at 16:03, Timothy Pearson wrote: > > > ----- Original Message ----- > > From: "Andrew Jeffery" <andrew@aj.id.au> > > To: "Timothy Pearson" <tpearson@raptorengineering.com>, "linux-aspeed" <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" > > <ryan_chen@aspeedtech.com> > > Sent: Thursday, May 2, 2019 12:51:00 AM > > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs on AST2500 devices > > > On Thu, 2 May 2019, at 08:20, Timothy Pearson wrote: > >> There appears to be a significant error in the pinmux table starting on > >> page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) > >> requirement is incorrectly applied to multiple digital video input (DVI) > >> muxed pins, and no DVI-specific condition is provided. This results in > >> the serial devices incorrectly overriding the DVO pinmuxes and disabling > >> the DVO pins. > >> > >> Create a new condition code (COND6) for DVI enable, and update the most > >> seriously affected pins to use the new condition code. > >> > >> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> > >> --- > >> drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- > >> 1 file changed, 9 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> index 6f357a11e89a..676f90d3c5f3 100644 > >> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> @@ -29,6 +29,7 @@ > >> > >> #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } > >> #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } > >> +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } > >> > >> /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ > >> #define LHCR0 0x20 > >> @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); > >> > >> #define T1 89 > >> #define T1_DESC SIG_DESC_SET(SCU84, 17) > >> -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); > >> -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); > >> +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); > >> +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); > > > > I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI > > condition) > > requires SCU90[5]=0b1, but your introduction of COND6 requires SCU90[5:4]=0b00 > > for > > the mux configuration to succeed. This can't work - bit 5 of SCU90 can not > > simultaneously take the values 1 and 0. > > That's correct -- I don't have hardware that supports DVI available to > test with. Okay. In that case I'm not prepared to ACK changes here, much less changes with that fail in this way. The current implementation at least follows what is dictated by the programming guide and is at least correct in theory. As Ryan is not confident there are no negative side-effects to not following the configuration dictated by the programming guide, changes like this have a real up-hill battle on their hands. Cheers, Andrew > > > Ryan: Can we just drop the COND2 requirement for function 2 of balls T1, U2, P4 > > and P3? > > I think that gets us where we need to be? > > > > Cheers, > > > > Andrew > > > >> MS_PIN_DECL(T1, GPIOL1, VPIDE, NDCD1); > >> FUNC_GROUP_DECL(NDCD1, T1); > >> > >> @@ -674,22 +675,22 @@ FUNC_GROUP_DECL(NDSR1, U1); > >> > >> #define U2 91 > >> #define U2_DESC SIG_DESC_SET(SCU84, 19) > >> -SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND2); > >> -SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND2); > >> +SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND6); > >> +SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND6); > >> MS_PIN_DECL(U2, GPIOL3, VPIHS, NRI1); > >> FUNC_GROUP_DECL(NRI1, U2); > >> > >> #define P4 92 > >> #define P4_DESC SIG_DESC_SET(SCU84, 20) > >> -SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND2); > >> -SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND2); > >> +SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND6); > >> +SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND6); > >> MS_PIN_DECL(P4, GPIOL4, VPIVS, NDTR1); > >> FUNC_GROUP_DECL(NDTR1, P4); > >> > >> #define P3 93 > >> #define P3_DESC SIG_DESC_SET(SCU84, 21) > >> -SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND2); > >> -SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND2); > >> +SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND6); > >> +SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND6); > >> MS_PIN_DECL(P3, GPIOL5, VPICLK, NRTS1); > >> FUNC_GROUP_DECL(NRTS1, P3); > >> > >> -- > >> 2.11.0 >
----- Original Message ----- > From: "Andrew Jeffery" <andrew@aj.id.au> > To: "Timothy Pearson" <tpearson@raptorengineering.com> > Cc: "linux-aspeed" <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" <ryan_chen@aspeedtech.com> > Sent: Thursday, May 2, 2019 1:40:39 AM > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs on AST2500 devices > On Thu, 2 May 2019, at 16:03, Timothy Pearson wrote: >> >> >> ----- Original Message ----- >> > From: "Andrew Jeffery" <andrew@aj.id.au> >> > To: "Timothy Pearson" <tpearson@raptorengineering.com>, "linux-aspeed" >> > <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" >> > <ryan_chen@aspeedtech.com> >> > Sent: Thursday, May 2, 2019 12:51:00 AM >> > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs >> > on AST2500 devices >> >> > On Thu, 2 May 2019, at 08:20, Timothy Pearson wrote: >> >> There appears to be a significant error in the pinmux table starting on >> >> page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) >> >> requirement is incorrectly applied to multiple digital video input (DVI) >> >> muxed pins, and no DVI-specific condition is provided. This results in >> >> the serial devices incorrectly overriding the DVO pinmuxes and disabling >> >> the DVO pins. >> >> >> >> Create a new condition code (COND6) for DVI enable, and update the most >> >> seriously affected pins to use the new condition code. >> >> >> >> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> >> >> --- >> >> drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- >> >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> >> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> >> index 6f357a11e89a..676f90d3c5f3 100644 >> >> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> >> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c >> >> @@ -29,6 +29,7 @@ >> >> >> >> #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } >> >> #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } >> >> +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } >> >> >> >> /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ >> >> #define LHCR0 0x20 >> >> @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); >> >> >> >> #define T1 89 >> >> #define T1_DESC SIG_DESC_SET(SCU84, 17) >> >> -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); >> >> -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); >> >> +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); >> >> +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); >> > >> > I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI >> > condition) >> > requires SCU90[5]=0b1, but your introduction of COND6 requires SCU90[5:4]=0b00 >> > for >> > the mux configuration to succeed. This can't work - bit 5 of SCU90 can not >> > simultaneously take the values 1 and 0. >> >> That's correct -- I don't have hardware that supports DVI available to >> test with. > > Okay. In that case I'm not prepared to ACK changes here, much less changes with > that fail in this way. The current implementation at least follows what is > dictated by > the programming guide and is at least correct in theory. > > As Ryan is not confident there are no negative side-effects to not following the > configuration dictated by the programming guide, changes like this have a real > up-hill battle on their hands. > > Cheers, > > Andrew There is a negative effect right now in that enabling either UART will force disable the DVO pinmuxes. While I agree the patch needs additional work, as it stands right now DVO will not function simultaneously with the UART without a patched kernel. From where I stand I am fairly confident in a documentation error, however I do not have access to the hardware required to prove this. Can someone at ASpeed look at the HDL and verify or correct the documentation? We have already caught one documentation error relating to COND2 and DVO, and verified that one in hardware. Thank you!
On Fri, 3 May 2019, at 05:07, Timothy Pearson wrote: > > > ----- Original Message ----- > > From: "Andrew Jeffery" <andrew@aj.id.au> > > To: "Timothy Pearson" <tpearson@raptorengineering.com> > > Cc: "linux-aspeed" <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" <ryan_chen@aspeedtech.com> > > Sent: Thursday, May 2, 2019 1:40:39 AM > > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs on AST2500 devices > > > On Thu, 2 May 2019, at 16:03, Timothy Pearson wrote: > >> > >> > >> ----- Original Message ----- > >> > From: "Andrew Jeffery" <andrew@aj.id.au> > >> > To: "Timothy Pearson" <tpearson@raptorengineering.com>, "linux-aspeed" > >> > <linux-aspeed@lists.ozlabs.org>, "Ryan Chen" > >> > <ryan_chen@aspeedtech.com> > >> > Sent: Thursday, May 2, 2019 12:51:00 AM > >> > Subject: Re: [PATCH 3/3] aspeed/pinctrl: Fix simultaneous DVO and serial outputs > >> > on AST2500 devices > >> > >> > On Thu, 2 May 2019, at 08:20, Timothy Pearson wrote: > >> >> There appears to be a significant error in the pinmux table starting on > >> >> page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) > >> >> requirement is incorrectly applied to multiple digital video input (DVI) > >> >> muxed pins, and no DVI-specific condition is provided. This results in > >> >> the serial devices incorrectly overriding the DVO pinmuxes and disabling > >> >> the DVO pins. > >> >> > >> >> Create a new condition code (COND6) for DVI enable, and update the most > >> >> seriously affected pins to use the new condition code. > >> >> > >> >> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> > >> >> --- > >> >> drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- > >> >> 1 file changed, 9 insertions(+), 8 deletions(-) > >> >> > >> >> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> >> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> >> index 6f357a11e89a..676f90d3c5f3 100644 > >> >> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> >> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > >> >> @@ -29,6 +29,7 @@ > >> >> > >> >> #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } > >> >> #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } > >> >> +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } > >> >> > >> >> /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ > >> >> #define LHCR0 0x20 > >> >> @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); > >> >> > >> >> #define T1 89 > >> >> #define T1_DESC SIG_DESC_SET(SCU84, 17) > >> >> -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); > >> >> -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); > >> >> +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); > >> >> +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); > >> > > >> > I feel like you didn't test this patch, because VPI_24_RSVD_DESC (the DVI > >> > condition) > >> > requires SCU90[5]=0b1, but your introduction of COND6 requires SCU90[5:4]=0b00 > >> > for > >> > the mux configuration to succeed. This can't work - bit 5 of SCU90 can not > >> > simultaneously take the values 1 and 0. > >> > >> That's correct -- I don't have hardware that supports DVI available to > >> test with. > > > > Okay. In that case I'm not prepared to ACK changes here, much less changes with > > that fail in this way. The current implementation at least follows what is > > dictated by > > the programming guide and is at least correct in theory. > > > > As Ryan is not confident there are no negative side-effects to not following the > > configuration dictated by the programming guide, changes like this have a real > > up-hill battle on their hands. > > > > Cheers, > > > > Andrew > > There is a negative effect right now in that enabling either UART will > force disable the DVO pinmuxes. While I agree the patch needs > additional work, as it stands right now DVO will not function > simultaneously with the UART without a patched kernel. > > From where I stand I am fairly confident in a documentation error, > however I do not have access to the hardware required to prove this. > Can someone at ASpeed look at the HDL and verify or correct the > documentation? We have already caught one documentation error relating > to COND2 and DVO, and verified that one in hardware. Right - it's odd that there's a dependency on COND2 when COND2 relates to VPO, but the pins in question are VPI pins, and you're not interested in VPI. Ryan and I have spoken about it but he's deferred to the designer's opinion which is that we should follow what's specified in the datasheet. Given the complexity of the pinmux I'm going to set the bar for accepting these patches at "you need to convince Aspeed to correct the programming guide". I understand that might be annoying, but I need to be conservative to cater to the stability of everyone's use-cases, and not just accept patches contrary to the datasheet to enable your "conflicting" design. I appreciate that your experiments indicate the datasheet is misleading in some circumstances, but let's get Aspeed on board with that. Andrew
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c index 6f357a11e89a..676f90d3c5f3 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c @@ -29,6 +29,7 @@ #define COND1 { ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 } #define COND2 { ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 } +#define COND6 { ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 } /* LHCR0 is offset from the end of the H8S/2168-compatible registers */ #define LHCR0 0x20 @@ -660,8 +661,8 @@ SSSF_PIN_DECL(T2, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16)); #define T1 89 #define T1_DESC SIG_DESC_SET(SCU84, 17) -SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND2); -SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND2); +SIG_EXPR_LIST_DECL_SINGLE(VPIDE, VPI24, VPI_24_RSVD_DESC, T1_DESC, COND6); +SIG_EXPR_LIST_DECL_SINGLE(NDCD1, NDCD1, T1_DESC, COND6); MS_PIN_DECL(T1, GPIOL1, VPIDE, NDCD1); FUNC_GROUP_DECL(NDCD1, T1); @@ -674,22 +675,22 @@ FUNC_GROUP_DECL(NDSR1, U1); #define U2 91 #define U2_DESC SIG_DESC_SET(SCU84, 19) -SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND2); -SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND2); +SIG_EXPR_LIST_DECL_SINGLE(VPIHS, VPI24, VPI_24_RSVD_DESC, U2_DESC, COND6); +SIG_EXPR_LIST_DECL_SINGLE(NRI1, NRI1, U2_DESC, COND6); MS_PIN_DECL(U2, GPIOL3, VPIHS, NRI1); FUNC_GROUP_DECL(NRI1, U2); #define P4 92 #define P4_DESC SIG_DESC_SET(SCU84, 20) -SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND2); -SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND2); +SIG_EXPR_LIST_DECL_SINGLE(VPIVS, VPI24, VPI_24_RSVD_DESC, P4_DESC, COND6); +SIG_EXPR_LIST_DECL_SINGLE(NDTR1, NDTR1, P4_DESC, COND6); MS_PIN_DECL(P4, GPIOL4, VPIVS, NDTR1); FUNC_GROUP_DECL(NDTR1, P4); #define P3 93 #define P3_DESC SIG_DESC_SET(SCU84, 21) -SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND2); -SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND2); +SIG_EXPR_LIST_DECL_SINGLE(VPICLK, VPI24, VPI_24_RSVD_DESC, P3_DESC, COND6); +SIG_EXPR_LIST_DECL_SINGLE(NRTS1, NRTS1, P3_DESC, COND6); MS_PIN_DECL(P3, GPIOL5, VPICLK, NRTS1); FUNC_GROUP_DECL(NRTS1, P3);
There appears to be a significant error in the pinmux table starting on page 127 of the AST2500 datasheet v1.6. Specifically, the COND2 (DVO) requirement is incorrectly applied to multiple digital video input (DVI) muxed pins, and no DVI-specific condition is provided. This results in the serial devices incorrectly overriding the DVO pinmuxes and disabling the DVO pins. Create a new condition code (COND6) for DVI enable, and update the most seriously affected pins to use the new condition code. Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com> --- drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)