[1/1] Support LVDS output on Allwinner A20
diff mbox series

Message ID 20200210195633.GA21832@kedthinkpad
State New
Headers show
Series
  • [1/1] Support LVDS output on Allwinner A20
Related show

Commit Message

Andrey Lebedev Feb. 10, 2020, 7:56 p.m. UTC
A20 SoC (found in Cubieboard 2 among others) requires different LVDS set
up procedure than A33. Timing controller (tcon) driver only implements
sun6i-style procedure, that doesn't work on A20 (sun7i).

The support for such procedure is ported from u-boot and follows u-boot
naming convention: SUN6I* for sun6i-style procedure, and SUN4I for other
(which happens to be compatible with A20).
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 91 ++++++++++++++++++++----------
 drivers/gpu/drm/sun4i/sun4i_tcon.h | 12 ++++
 2 files changed, 73 insertions(+), 30 deletions(-)

Comments

Maxime Ripard Feb. 11, 2020, 7:20 a.m. UTC | #1
Hi,

On Mon, Feb 10, 2020 at 09:56:33PM +0200, Andrey Lebedev wrote:
> A20 SoC (found in Cubieboard 2 among others) requires different LVDS set
> up procedure than A33. Timing controller (tcon) driver only implements
> sun6i-style procedure, that doesn't work on A20 (sun7i).

You're missing your Signed-off-by here.

> The support for such procedure is ported from u-boot and follows u-boot
> naming convention: SUN6I* for sun6i-style procedure, and SUN4I for other
> (which happens to be compatible with A20).

A commit log is mostly about why you're doing a change, this part can
be left out.

> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 91 ++++++++++++++++++++----------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h | 12 ++++
>  2 files changed, 73 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c81cdce6ed55..78896e907ca9 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -114,46 +114,74 @@ static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
>  	}
>  }
>
> +static void sun4i_tcon_lvds_sun6i_enable(struct sun4i_tcon *tcon,
> +					 const struct drm_encoder *encoder) {

This doesn't match the kernel coding style, make sure to run checkpatch.

Also, using something like sun6i_tcon_setup_lvds_phy would be more fit
to what this function is doing.

> +	u8 val;
> +	regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> +		     SUN6I_TCON0_LVDS_ANA0_C(2) |
> +		     SUN6I_TCON0_LVDS_ANA0_V(3) |
> +		     SUN6I_TCON0_LVDS_ANA0_PD(2) |
> +		     SUN6I_TCON0_LVDS_ANA0_EN_LDO);
> +	udelay(2);
> +
> +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> +			   SUN6I_TCON0_LVDS_ANA0_EN_MB,
> +			   SUN6I_TCON0_LVDS_ANA0_EN_MB);
> +	udelay(2);
> +
> +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> +			   SUN6I_TCON0_LVDS_ANA0_EN_DRVC,
> +			   SUN6I_TCON0_LVDS_ANA0_EN_DRVC);
> +
> +	if (sun4i_tcon_get_pixel_depth(encoder) == 18)
> +		val = 7;
> +	else
> +		val = 0xf;
> +
> +	regmap_write_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> +			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(0xf),
> +			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
> +
> +}
> +
> +static void sun4i_tcon_lvds_sun4i_enable(struct sun4i_tcon *tcon) {

And sun4i_tcon_setup_lvds_phy.

> +	regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> +		     SUN4I_TCON0_LVDS_ANA0_CK_EN |
> +		     SUN4I_TCON0_LVDS_ANA0_REG_V |
> +		     SUN4I_TCON0_LVDS_ANA0_REG_C |
> +		     SUN4I_TCON0_LVDS_ANA0_EN_MB |
> +		     SUN4I_TCON0_LVDS_ANA0_PD |
> +		     SUN4I_TCON0_LVDS_ANA0_DCHS);
> +
> +	udelay(2); /* delay at least 1200 ns */
> +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
> +			   SUN4I_TCON0_LVDS_ANA1_INIT,
> +			   SUN4I_TCON0_LVDS_ANA1_INIT);
> +	udelay(1); /* delay at least 1200 ns */

The delay and your comment don't match.

> +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
> +			   SUN4I_TCON0_LVDS_ANA1_UPDATE,
> +			   SUN4I_TCON0_LVDS_ANA1_UPDATE);

You refer to U-Boot in your commit log, but the sequence is not quite
the same, why did you change it?

> +}
> +
> +
>  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
>  				       const struct drm_encoder *encoder,
>  				       bool enabled)
>  {
>  	if (enabled) {
> -		u8 val;
> -
> +		// Enable LVDS interface

There's no need for that comment, it's simple enough :)

>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
>  				   SUN4I_TCON0_LVDS_IF_EN,
>  				   SUN4I_TCON0_LVDS_IF_EN);
>
> -		/*
> -		 * As their name suggest, these values only apply to the A31
> -		 * and later SoCs. We'll have to rework this when merging
> -		 * support for the older SoCs.
> -		 */
> -		regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> -			     SUN6I_TCON0_LVDS_ANA0_C(2) |
> -			     SUN6I_TCON0_LVDS_ANA0_V(3) |
> -			     SUN6I_TCON0_LVDS_ANA0_PD(2) |
> -			     SUN6I_TCON0_LVDS_ANA0_EN_LDO);
> -		udelay(2);
> -
> -		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> -				   SUN6I_TCON0_LVDS_ANA0_EN_MB,
> -				   SUN6I_TCON0_LVDS_ANA0_EN_MB);
> -		udelay(2);
> -
> -		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> -				   SUN6I_TCON0_LVDS_ANA0_EN_DRVC,
> -				   SUN6I_TCON0_LVDS_ANA0_EN_DRVC);
> -
> -		if (sun4i_tcon_get_pixel_depth(encoder) == 18)
> -			val = 7;
> -		else
> -			val = 0xf;
> +		// Perform SoC-specific setup procedure

Ditto.

> +		if (tcon->quirks->sun6i_lvds_init) {
> +			sun4i_tcon_lvds_sun6i_enable(tcon, encoder);
> +		}
> +		else {
> +			sun4i_tcon_lvds_sun4i_enable(tcon);
> +		}
>
> -		regmap_write_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> -				  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(0xf),
> -				  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
>  	} else {
>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
>  				   SUN4I_TCON0_LVDS_IF_EN, 0);
> @@ -1454,6 +1482,7 @@ static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
>  };
>
>  static const struct sun4i_tcon_quirks sun7i_a20_quirks = {
> +	.supports_lvds		= true,
>  	.has_channel_0		= true,
>  	.has_channel_1		= true,
>  	.dclk_min_div		= 4,
> @@ -1464,11 +1493,13 @@ static const struct sun4i_tcon_quirks sun7i_a20_quirks = {
>  static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
>  	.has_channel_0		= true,
>  	.has_lvds_alt		= true,
> +	.sun6i_lvds_init	= true,

Using a function pointer here (like we're doing with set_mux) would be
more future proof.

>  	.dclk_min_div		= 1,
>  };
>
>  static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = {
>  	.supports_lvds		= true,
> +	.sun6i_lvds_init	= true,
>  	.has_channel_0		= true,
>  	.dclk_min_div		= 1,
>  };
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index a62ec826ae71..973901c1bee5 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -193,6 +193,13 @@
>  #define SUN4I_TCON_MUX_CTRL_REG			0x200
>
>  #define SUN4I_TCON0_LVDS_ANA0_REG		0x220
> +#define SUN4I_TCON0_LVDS_ANA0_DCHS			BIT(16)
> +#define SUN4I_TCON0_LVDS_ANA0_PD			BIT(20) | BIT(21)
> +#define SUN4I_TCON0_LVDS_ANA0_EN_MB			BIT(22)
> +#define SUN4I_TCON0_LVDS_ANA0_REG_C			BIT(24) | BIT(25)
> +#define SUN4I_TCON0_LVDS_ANA0_REG_V			BIT(26) | BIT(27)
> +#define SUN4I_TCON0_LVDS_ANA0_CK_EN			BIT(29) | BIT(28)
> +
>  #define SUN6I_TCON0_LVDS_ANA0_EN_MB			BIT(31)
>  #define SUN6I_TCON0_LVDS_ANA0_EN_LDO			BIT(30)
>  #define SUN6I_TCON0_LVDS_ANA0_EN_DRVC			BIT(24)
> @@ -201,6 +208,10 @@
>  #define SUN6I_TCON0_LVDS_ANA0_V(x)			(((x) & 3) << 8)
>  #define SUN6I_TCON0_LVDS_ANA0_PD(x)			(((x) & 3) << 4)
>
> +#define SUN4I_TCON0_LVDS_ANA1_REG		0x224
> +#define SUN4I_TCON0_LVDS_ANA1_INIT			(0x1f << 26 | 0x1f << 10)
> +#define SUN4I_TCON0_LVDS_ANA1_UPDATE			(0x1f << 16 | 0x1f << 00)

Having proper defines for those fields would be great too.

Side question, this will need some DT changes too, right?

Maxime
Andrey Lebedev Feb. 11, 2020, 8:48 p.m. UTC | #2
Maxime, thanks for your comments. I'll update the patch, but meanwhile,
I have some remarks/questions, see below.

On Tue, Feb 11, 2020 at 08:20:04AM +0100, Maxime Ripard wrote:
> > +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
> > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE,
> > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE);
> 
> You refer to U-Boot in your commit log, but the sequence is not quite
> the same, why did you change it?

I actually had two reference implementations at my hand. One was u-boot
and another - an old (abandoned) branch of Priit Laes [1] (I took the
split-up of u-boot SUNXI_LCDC_LVDS_ANA0 constant from there).

This is an attempt to simplify the sequence, since I noticed that the
same bit was being set to the same register twice [2] and removing that
duplication didn't produce any observable regression. Priit
implementation didn't have that bit set in the end of the sequence
either, so I omitted it. That said, I agree that it could've been a bit
naive on my side, and I can get it back to match u-boot version, if you
feel that might be important.

For the reference the U-Boot code is here: [3]

[1] https://github.com/plaes/linux/commit/cc8c8bab2f2f2752ba6b11632dcd0f41bac249bc#diff-014a76a5007005a7a240825a972b8c7fR127
[2] setbits_le32(&lcdc->lvds_ana0, SUNXI_LCDC_LVDS_ANA0_UPDATE);
[3] https://github.com/ARM-software/u-boot/blob/master/drivers/video/sunxi/lcdc.c#L60

> > +#define SUN4I_TCON0_LVDS_ANA1_REG		0x224
> > +#define SUN4I_TCON0_LVDS_ANA1_INIT			(0x1f << 26 | 0x1f << 10)
> > +#define SUN4I_TCON0_LVDS_ANA1_UPDATE			(0x1f << 16 | 0x1f << 00)
> 
> Having proper defines for those fields would be great too.

If by "proper" you mean "split them up to individual bits", I would
agree, but I can't find any actual hardware reference documentation that
would mention the meaning of these registers.

In both places (u-boot and Priit) these constants are defined the same way. 

I took the liberty to rename ANA1_INIT1 to ANA1_INIT and ANA1_INIT2 to
ANA1_UPDATE to match Priit naming rather than u-boot, as I felt it was
more descriptive. I have no strong opinion here though. 

> Side question, this will need some DT changes too, right?

Hm, I agree. I think it would be reasonable to include LVDS0/1 pins and
sample (but disabled) lvds panel, connected to tcon to
arch/arm/boot/dts/sun7i-a20.dtsi. Does that make sense to you? Would you
expect dts changes in the same patch or separate?

P.S. This is my first patch to the linux kernel, please forgive me my
inexperience.
Maxime Ripard Feb. 12, 2020, 12:53 p.m. UTC | #3
Hi Andrey,

On Tue, Feb 11, 2020 at 10:48:28PM +0200, Andrey Lebedev wrote:
> Maxime, thanks for your comments. I'll update the patch, but meanwhile,
> I have some remarks/questions, see below.
>
> On Tue, Feb 11, 2020 at 08:20:04AM +0100, Maxime Ripard wrote:
> > > +	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
> > > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE,
> > > +			   SUN4I_TCON0_LVDS_ANA1_UPDATE);
> >
> > You refer to U-Boot in your commit log, but the sequence is not quite
> > the same, why did you change it?
>
> I actually had two reference implementations at my hand. One was u-boot
> and another - an old (abandoned) branch of Priit Laes [1] (I took the
> split-up of u-boot SUNXI_LCDC_LVDS_ANA0 constant from there).
>
> This is an attempt to simplify the sequence, since I noticed that the
> same bit was being set to the same register twice [2] and removing that
> duplication didn't produce any observable regression. Priit
> implementation didn't have that bit set in the end of the sequence
> either, so I omitted it. That said, I agree that it could've been a bit
> naive on my side, and I can get it back to match u-boot version, if you
> feel that might be important.
>
> For the reference the U-Boot code is here: [3]
>
> [1] https://github.com/plaes/linux/commit/cc8c8bab2f2f2752ba6b11632dcd0f41bac249bc#diff-014a76a5007005a7a240825a972b8c7fR127
> [2] setbits_le32(&lcdc->lvds_ana0, SUNXI_LCDC_LVDS_ANA0_UPDATE);
> [3] https://github.com/ARM-software/u-boot/blob/master/drivers/video/sunxi/lcdc.c#L60

The U-Boot code has been here for a while and we know it's robust by
now, so I'd prefer to be conservative and use it here.

> > > +#define SUN4I_TCON0_LVDS_ANA1_REG		0x224
> > > +#define SUN4I_TCON0_LVDS_ANA1_INIT			(0x1f << 26 | 0x1f << 10)
> > > +#define SUN4I_TCON0_LVDS_ANA1_UPDATE			(0x1f << 16 | 0x1f << 00)
> >
> > Having proper defines for those fields would be great too.
>
> If by "proper" you mean "split them up to individual bits", I would
> agree, but I can't find any actual hardware reference documentation that
> would mention the meaning of these registers.

Of course we don't.. :)

It's fine to leave them as is then

> In both places (u-boot and Priit) these constants are defined the same way.
>
> I took the liberty to rename ANA1_INIT1 to ANA1_INIT and ANA1_INIT2 to
> ANA1_UPDATE to match Priit naming rather than u-boot, as I felt it was
> more descriptive. I have no strong opinion here though.
>
> > Side question, this will need some DT changes too, right?
>
> Hm, I agree. I think it would be reasonable to include LVDS0/1 pins

That, but most importantly, the reset and clocks for the LVDS
block. Also from looking at it, I'm not entirely sure that the TCON1
has a LVDS output, do you have a board when you have been able to test
it?

> and sample (but disabled) lvds panel,

That's good for the sake of the example, but it shouldn't be in the
same patch, it won't be merged.

> connected to tcon to arch/arm/boot/dts/sun7i-a20.dtsi. Does that
> make sense to you? Would you expect dts changes in the same patch or
> separate?
>
> P.S. This is my first patch to the linux kernel, please forgive me my
> inexperience.

You're doing fine so far :)
Maxime
Andrey Lebedev Feb. 12, 2020, 10:46 p.m. UTC | #4
On Wed, Feb 12, 2020 at 01:53:45PM +0100, Maxime Ripard wrote:
> > > Side question, this will need some DT changes too, right?
> >
> > Hm, I agree. I think it would be reasonable to include LVDS0/1 pins
> 
> That, but most importantly, the reset and clocks for the LVDS
> block. Also from looking at it, I'm not entirely sure that the TCON1
> has a LVDS output

I also have impression that LVDS is only supported on TCON0, but that's
mostly from this comment in sun4i_lvds.c:

	/* The LVDS encoder can only work with the TCON channel 0 */

> do you have a board when you have been able to test it?

Yes, I have the hardware (Cubieboard 2) at hand, but I cannot change the
any physical connections on it. FWIW, it is https://openvario.org, the
device we are (painfully) trying to upgrade from old kernel-3.4 with
proprietary mali drivers to contemporary software.

> > and sample (but disabled) lvds panel,
> 
> That's good for the sake of the example, but it shouldn't be in the
> same patch, it won't be merged.

I jave just submitted version 2 of the patches - set of 2 patches this
time. Addressed your comments, please take a look.
Maxime Ripard Feb. 13, 2020, 9:24 a.m. UTC | #5
On Thu, Feb 13, 2020 at 12:46:53AM +0200, Andrey Lebedev wrote:
> On Wed, Feb 12, 2020 at 01:53:45PM +0100, Maxime Ripard wrote:
> > > > Side question, this will need some DT changes too, right?
> > >
> > > Hm, I agree. I think it would be reasonable to include LVDS0/1 pins
> >
> > That, but most importantly, the reset and clocks for the LVDS
> > block. Also from looking at it, I'm not entirely sure that the TCON1
> > has a LVDS output
>
> I also have impression that LVDS is only supported on TCON0, but that's
> mostly from this comment in sun4i_lvds.c:
>
> 	/* The LVDS encoder can only work with the TCON channel 0 */

No, that's a separate thing.

Internally the TCON has two channels, one connected to panels type of
display (LVDS, Parallel, etc), the second one connected to TV outputs
(HDMI, composite).

But then, on some SoCs like the A20, you have two TCON's too. As far
as I could see, only the first TCON can use LVDS, but I'm not
definitive.

Allwinner seems to allow panels to only be tied to TCON0 in the BSP,
so I guess we can assume that.

> > do you have a board when you have been able to test it?
>
> Yes, I have the hardware (Cubieboard 2) at hand, but I cannot change the
> any physical connections on it. FWIW, it is https://openvario.org, the
> device we are (painfully) trying to upgrade from old kernel-3.4 with
> proprietary mali drivers to contemporary software.

What painpoints do you have?

> > > and sample (but disabled) lvds panel,
> >
> > That's good for the sake of the example, but it shouldn't be in the
> > same patch, it won't be merged.
>
> I jave just submitted version 2 of the patches - set of 2 patches this
> time. Addressed your comments, please take a look.

I will, thanks!
Maxime
Andrey Lebedev Feb. 13, 2020, 6:11 p.m. UTC | #6
On Thu, Feb 13, 2020 at 10:24:33AM +0100, Maxime Ripard wrote:
> > > do you have a board when you have been able to test it?
> >
> > Yes, I have the hardware (Cubieboard 2) at hand, but I cannot change the
> > any physical connections on it. FWIW, it is https://openvario.org, the
> > device we are (painfully) trying to upgrade from old kernel-3.4 with
> > proprietary mali drivers to contemporary software.
> 
> What painpoints do you have?

So, even though proprietary mali drivers worked well for us, they seem
to hold us back to old kernel-3.4, and it started to get harder to avoid
various compatibility issues with newer software. Since there seemed to
be a good progress with OSS lima driver lately, we decided to try to
replace mali with lima. And that naturally pulled to switch to DRM/KMS.

The first painpoint is this LVDS support problem: after a week of trial
and error, I discovered that support was simply not there. But it seemed
that not that much was actually missing and after couple of evenings
deep into debugging, here we are.

Another one is the screen rotation: the device is installed into the
glider' instrument panel, and some pilots prefer it in portrait
orientation.  Under old mali setup, all that (seemingly) was required
was setting "fbcon=rotate:n" boot arg, and both linux console and
graphical app (https://xcsoar.org/) rotated "automagically". With new
DRM/KMS setup, only console is rotated, all graphical apps seem to see
the screen in its "natural" landscape orientation. Do you know if there
is any way to leverage DMS/KMS infrastructure to make screen appear
rotated for userspace apps (writing to /sys/class/graphics/fb0/rotate
didn't work)?  There's of course a plan B to teach the app to rotate its
output, but that leads to problem number 3.

And the 3rd outstanding problem is that app doesn't really work under
lima module and lima mesa driver. It starts, but renders a black window.
I haven't dug deeply into this yet, but it is possible that some opengl
API isn't supported in OSS drivers yet (even though app only renders 2d
graphics).

Hopefully that wasn't too much complaining for you :) If you have any
insight on possible causes or attack vectors on any of these, that would
help a lot. Also, please feel free to correct any of false assumptions I
make above, I'm happy to learn more about this.
Maxime Ripard Feb. 14, 2020, 7:58 a.m. UTC | #7
On Thu, Feb 13, 2020 at 08:11:10PM +0200, Andrey Lebedev wrote:
> On Thu, Feb 13, 2020 at 10:24:33AM +0100, Maxime Ripard wrote:
> > > > do you have a board when you have been able to test it?
> > >
> > > Yes, I have the hardware (Cubieboard 2) at hand, but I cannot change the
> > > any physical connections on it. FWIW, it is https://openvario.org, the
> > > device we are (painfully) trying to upgrade from old kernel-3.4 with
> > > proprietary mali drivers to contemporary software.
> >
> > What painpoints do you have?
>
> So, even though proprietary mali drivers worked well for us, they seem
> to hold us back to old kernel-3.4, and it started to get harder to avoid
> various compatibility issues with newer software. Since there seemed to
> be a good progress with OSS lima driver lately, we decided to try to
> replace mali with lima. And that naturally pulled to switch to DRM/KMS.

You can use the proprietary mali drivers with mainline too, but yeah
it's in maintainance mode these days and you should go for lima if
you're (re)starting from scratch.

> The first painpoint is this LVDS support problem: after a week of trial
> and error, I discovered that support was simply not there. But it seemed
> that not that much was actually missing and after couple of evenings
> deep into debugging, here we are.
>
> Another one is the screen rotation: the device is installed into the
> glider' instrument panel, and some pilots prefer it in portrait
> orientation.  Under old mali setup, all that (seemingly) was required
> was setting "fbcon=rotate:n" boot arg, and both linux console and
> graphical app (https://xcsoar.org/) rotated "automagically". With new
> DRM/KMS setup, only console is rotated, all graphical apps seem to see
> the screen in its "natural" landscape orientation. Do you know if there
> is any way to leverage DMS/KMS infrastructure to make screen appear
> rotated for userspace apps (writing to /sys/class/graphics/fb0/rotate
> didn't work)?  There's of course a plan B to teach the app to rotate its
> output, but that leads to problem number 3.

There's a rotation property that can be set by whatever you're using
on top of KMS, DRM_MODE_ROTATE_*, but I'm not sure the driver supports
it at the moment.

> And the 3rd outstanding problem is that app doesn't really work under
> lima module and lima mesa driver. It starts, but renders a black window.
> I haven't dug deeply into this yet, but it is possible that some opengl
> API isn't supported in OSS drivers yet (even though app only renders 2d
> graphics).

I have no idea on this one though, sorry :/

Maxime
Andrey Lebedev Feb. 19, 2020, 6:08 p.m. UTC | #8
Address all outstanding review comments.

Maxime, please confirm I've got "document the new
compatibles" part right.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c81cdce6ed55..78896e907ca9 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -114,46 +114,74 @@  static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
 	}
 }
 
+static void sun4i_tcon_lvds_sun6i_enable(struct sun4i_tcon *tcon,
+					 const struct drm_encoder *encoder) {
+	u8 val;
+	regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
+		     SUN6I_TCON0_LVDS_ANA0_C(2) |
+		     SUN6I_TCON0_LVDS_ANA0_V(3) |
+		     SUN6I_TCON0_LVDS_ANA0_PD(2) |
+		     SUN6I_TCON0_LVDS_ANA0_EN_LDO);
+	udelay(2);
+
+	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
+			   SUN6I_TCON0_LVDS_ANA0_EN_MB,
+			   SUN6I_TCON0_LVDS_ANA0_EN_MB);
+	udelay(2);
+
+	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
+			   SUN6I_TCON0_LVDS_ANA0_EN_DRVC,
+			   SUN6I_TCON0_LVDS_ANA0_EN_DRVC);
+
+	if (sun4i_tcon_get_pixel_depth(encoder) == 18)
+		val = 7;
+	else
+		val = 0xf;
+
+	regmap_write_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
+			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(0xf),
+			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
+
+}
+
+static void sun4i_tcon_lvds_sun4i_enable(struct sun4i_tcon *tcon) {
+	regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
+		     SUN4I_TCON0_LVDS_ANA0_CK_EN |
+		     SUN4I_TCON0_LVDS_ANA0_REG_V |
+		     SUN4I_TCON0_LVDS_ANA0_REG_C |
+		     SUN4I_TCON0_LVDS_ANA0_EN_MB |
+		     SUN4I_TCON0_LVDS_ANA0_PD |
+		     SUN4I_TCON0_LVDS_ANA0_DCHS);
+
+	udelay(2); /* delay at least 1200 ns */
+	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
+			   SUN4I_TCON0_LVDS_ANA1_INIT,
+			   SUN4I_TCON0_LVDS_ANA1_INIT);
+	udelay(1); /* delay at least 1200 ns */
+	regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA1_REG,
+			   SUN4I_TCON0_LVDS_ANA1_UPDATE,
+			   SUN4I_TCON0_LVDS_ANA1_UPDATE);
+}
+
+
 static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
 				       const struct drm_encoder *encoder,
 				       bool enabled)
 {
 	if (enabled) {
-		u8 val;
-
+		// Enable LVDS interface
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
 				   SUN4I_TCON0_LVDS_IF_EN,
 				   SUN4I_TCON0_LVDS_IF_EN);
 
-		/*
-		 * As their name suggest, these values only apply to the A31
-		 * and later SoCs. We'll have to rework this when merging
-		 * support for the older SoCs.
-		 */
-		regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
-			     SUN6I_TCON0_LVDS_ANA0_C(2) |
-			     SUN6I_TCON0_LVDS_ANA0_V(3) |
-			     SUN6I_TCON0_LVDS_ANA0_PD(2) |
-			     SUN6I_TCON0_LVDS_ANA0_EN_LDO);
-		udelay(2);
-
-		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
-				   SUN6I_TCON0_LVDS_ANA0_EN_MB,
-				   SUN6I_TCON0_LVDS_ANA0_EN_MB);
-		udelay(2);
-
-		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
-				   SUN6I_TCON0_LVDS_ANA0_EN_DRVC,
-				   SUN6I_TCON0_LVDS_ANA0_EN_DRVC);
-
-		if (sun4i_tcon_get_pixel_depth(encoder) == 18)
-			val = 7;
-		else
-			val = 0xf;
+		// Perform SoC-specific setup procedure
+		if (tcon->quirks->sun6i_lvds_init) {
+			sun4i_tcon_lvds_sun6i_enable(tcon, encoder);
+		}
+		else {
+			sun4i_tcon_lvds_sun4i_enable(tcon);
+		}
 
-		regmap_write_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
-				  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(0xf),
-				  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
 	} else {
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
 				   SUN4I_TCON0_LVDS_IF_EN, 0);
@@ -1454,6 +1482,7 @@  static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
 };
 
 static const struct sun4i_tcon_quirks sun7i_a20_quirks = {
+	.supports_lvds		= true,
 	.has_channel_0		= true,
 	.has_channel_1		= true,
 	.dclk_min_div		= 4,
@@ -1464,11 +1493,13 @@  static const struct sun4i_tcon_quirks sun7i_a20_quirks = {
 static const struct sun4i_tcon_quirks sun8i_a33_quirks = {
 	.has_channel_0		= true,
 	.has_lvds_alt		= true,
+	.sun6i_lvds_init	= true,
 	.dclk_min_div		= 1,
 };
 
 static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = {
 	.supports_lvds		= true,
+	.sun6i_lvds_init	= true,
 	.has_channel_0		= true,
 	.dclk_min_div		= 1,
 };
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index a62ec826ae71..973901c1bee5 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -193,6 +193,13 @@ 
 #define SUN4I_TCON_MUX_CTRL_REG			0x200
 
 #define SUN4I_TCON0_LVDS_ANA0_REG		0x220
+#define SUN4I_TCON0_LVDS_ANA0_DCHS			BIT(16)
+#define SUN4I_TCON0_LVDS_ANA0_PD			BIT(20) | BIT(21)
+#define SUN4I_TCON0_LVDS_ANA0_EN_MB			BIT(22)
+#define SUN4I_TCON0_LVDS_ANA0_REG_C			BIT(24) | BIT(25)
+#define SUN4I_TCON0_LVDS_ANA0_REG_V			BIT(26) | BIT(27)
+#define SUN4I_TCON0_LVDS_ANA0_CK_EN			BIT(29) | BIT(28)
+
 #define SUN6I_TCON0_LVDS_ANA0_EN_MB			BIT(31)
 #define SUN6I_TCON0_LVDS_ANA0_EN_LDO			BIT(30)
 #define SUN6I_TCON0_LVDS_ANA0_EN_DRVC			BIT(24)
@@ -201,6 +208,10 @@ 
 #define SUN6I_TCON0_LVDS_ANA0_V(x)			(((x) & 3) << 8)
 #define SUN6I_TCON0_LVDS_ANA0_PD(x)			(((x) & 3) << 4)
 
+#define SUN4I_TCON0_LVDS_ANA1_REG		0x224
+#define SUN4I_TCON0_LVDS_ANA1_INIT			(0x1f << 26 | 0x1f << 10)
+#define SUN4I_TCON0_LVDS_ANA1_UPDATE			(0x1f << 16 | 0x1f << 00)
+
 #define SUN4I_TCON1_FILL_CTL_REG		0x300
 #define SUN4I_TCON1_FILL_BEG0_REG		0x304
 #define SUN4I_TCON1_FILL_END0_REG		0x308
@@ -224,6 +235,7 @@  struct sun4i_tcon_quirks {
 	bool	needs_de_be_mux; /* sun6i needs mux to select backend */
 	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
 	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
+	bool	sun6i_lvds_init; /* Requires sun6i lvds initialization? */
 	u8	dclk_min_div;	/* minimum divider for TCON0 DCLK */
 
 	/* callback to handle tcon muxing options */