[v2,12/19] video: sunxi: de2: switch to DT probing

Currently DE2 driver is probed via driver info. Switch probing to device
tree compatible string method.

Display is now searched via driver name which has same limitation as
previous method. This can be improved only when all drivers in chain are
probed via device tree compatible strings.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/video/sunxi/sunxi_de2.c | 88 +++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 36 deletions(-)

On Sat,  6 Mar 2021 20:54:30 +0100
Jernej Skrabec <jernej.skrabec@siol.net> wrote:

Hi Jernej,

(CC:ing Simon for some DM issues below)

> Currently DE2 driver is probed via driver info. Switch probing to device
> tree compatible string method.
> 
> Display is now searched via driver name which has same limitation as
> previous method. This can be improved only when all drivers in chain are
> probed via device tree compatible strings.

So on a first glance this looks alright (also the fixed version on your
github). However it doesn't work on the Pine64-LTS (or A64 in general),
and I identified at least two problems below:

> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/video/sunxi/sunxi_de2.c | 88 +++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c
> index e02d359cd259..81576e45e9ef 100644
> --- a/drivers/video/sunxi/sunxi_de2.c
> +++ b/drivers/video/sunxi/sunxi_de2.c
> @@ -31,6 +31,11 @@ enum {
>  	LCD_MAX_LOG2_BPP	= VIDEO_BPP32,
>  };
>  
> +struct sunxi_de2_data {
> +	int id;

nit: can you rename this to something less general, like mux or
pipeline_id?

> +	const char *disp_drv_name;
> +};
> +
>  static void sunxi_de2_composer_init(void)
>  {
>  	struct sunxi_ccm_reg * const ccm =
> @@ -228,51 +233,34 @@ static int sunxi_de2_init(struct udevice *dev, ulong fbbase,
>  
>  static int sunxi_de2_probe(struct udevice *dev)
>  {
> +	const struct sunxi_de2_data *data =
> +		(const struct sunxi_de2_data *)dev_get_driver_data(dev);
>  	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
>  	struct udevice *disp;
> -	int ret;
> +	int ret, index = 0;
>  
>  	/* Before relocation we don't need to do anything */
>  	if (!(gd->flags & GD_FLG_RELOC))
>  		return 0;
>  
> -	ret = uclass_get_device_by_driver(UCLASS_DISPLAY,
> -					  DM_DRIVER_GET(sunxi_lcd), &disp);
> -	if (!ret) {
> -		int mux;
> +	while (!(ret = uclass_get_device(UCLASS_DISPLAY, index++, &disp))) {

So this call tries to enumerate all devices registered as
UCLASS_DISPLAY. For the A64 the sunxi_lcd driver provides one device,
however it only probes correctly when there is bridge configured (for
the Pinebook, for instance). There is code to read timings from DT,
but I don't find any of the required properties (simple-panel) in any
Allwinner DTs (using DE2, at least).
As a consequence the probe in sunxi_lcd.c fails (except for the
Pinebook), and it returns -ENODEV. This is unfortunately the same error
code that uclass_get_device_by_driver() returns when there are no
(more) devices providing UCLASS_DISPLAY, so the while loop stops here,
before having considered the HDMI devices (which are enumerated *after*
the LCD device). This is the same problem with your change to use
uclass_id_foreach_dev(), btw.

Not sure how to best solve this, it sounds like a general DM related
problem (failing probe() ending iterating all devices in one class).

I hacked this a bit (using a different error code in sunxi_lcd_probe()
and catching/translating this here), but this revealed another problem:
I only ever see the mixer0 device being probed (actually multiple
times). I could debug that both mixers are detected in the DT, created
as two devices and bound (using the right .id value and the right DT
offset), but only the first one is ever probed through this function
here. Unfortunately this is the wrong one (HDMI is on mixer1), so
de2_probe() fails :-(
Disabling mixer0 in the DT makes it work (on top of the hack above).

No real clue about this second problem, but I will debug further.

Cheers,
Andre


> +		if (strcmp(disp->driver->name, data->disp_drv_name))
> +			continue;
>  
> -		mux = 0;
> +		ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp,
> +				     data->id, false);
> +		if (ret)
> +			return ret;
>  
> -		ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
> -				     false);
> -		if (!ret) {
> -			video_set_flush_dcache(dev, 1);
> -			return 0;
> -		}
> -	}
> -
> -	debug("%s: lcd display not found (ret=%d)\n", __func__, ret);
> -
> -	ret = uclass_get_device_by_driver(UCLASS_DISPLAY,
> -					  DM_DRIVER_GET(sunxi_dw_hdmi), &disp);
> -	if (!ret) {
> -		int mux;
> -		if (IS_ENABLED(CONFIG_MACH_SUNXI_H3_H5))
> -			mux = 0;
> -		else
> -			mux = 1;
> +		video_set_flush_dcache(dev, 1);
>  
> -		ret = sunxi_de2_init(dev, plat->base, VIDEO_BPP32, disp, mux,
> -				     false);
> -		if (!ret) {
> -			video_set_flush_dcache(dev, 1);
> -			return 0;
> -		}
> +		return 0;
>  	}
>  
> -	debug("%s: hdmi display not found (ret=%d)\n", __func__, ret);
> +	debug("%s: %s not found (ret=%d)\n", __func__,
> +	      data->disp_drv_name, ret);
>  
> -	return -ENODEV;
> +	return ret;
>  }
>  
>  static int sunxi_de2_bind(struct udevice *dev)
> @@ -285,22 +273,50 @@ static int sunxi_de2_bind(struct udevice *dev)
>  	return 0;
>  }
>  
> +const struct sunxi_de2_data h3_mixer_0 = {
> +	.id = 0,
> +	.disp_drv_name = "sunxi_dw_hdmi",
> +};
> +
> +const struct sunxi_de2_data a64_mixer_0 = {
> +	.id = 0,
> +	.disp_drv_name = "sunxi_lcd",
> +};
> +
> +const struct sunxi_de2_data a64_mixer_1 = {
> +	.id = 1,
> +	.disp_drv_name = "sunxi_dw_hdmi",
> +};
> +
> +static const struct udevice_id sunxi_de2_ids[] = {
> +	{
> +		.compatible = "allwinner,sun8i-h3-de2-mixer-0",
> +		.data = (ulong)&h3_mixer_0,
> +	},
> +	{
> +		.compatible = "allwinner,sun50i-a64-de2-mixer-0",
> +		.data = (ulong)&a64_mixer_0,
> +	},
> +	{
> +		.compatible = "allwinner,sun50i-a64-de2-mixer-1",
> +		.data = (ulong)&a64_mixer_1,
> +	},
> +	{ }
> +};
> +
>  static const struct video_ops sunxi_de2_ops = {
>  };
>  
>  U_BOOT_DRIVER(sunxi_de2) = {
>  	.name	= "sunxi_de2",
>  	.id	= UCLASS_VIDEO,
> +	.of_match = sunxi_de2_ids,
>  	.ops	= &sunxi_de2_ops,
>  	.bind	= sunxi_de2_bind,
>  	.probe	= sunxi_de2_probe,
>  	.flags	= DM_FLAG_PRE_RELOC,
>  };
>  
> -U_BOOT_DRVINFO(sunxi_de2) = {
> -	.name = "sunxi_de2"
> -};
> -
>  /*
>   * Simplefb support.
>   */

Hi Andre,

On Tue, 6 Apr 2021 at 13:09, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sat,  6 Mar 2021 20:54:30 +0100
> Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> Hi Jernej,
>
> (CC:ing Simon for some DM issues below)
>
> > Currently DE2 driver is probed via driver info. Switch probing to device
> > tree compatible string method.
> >
> > Display is now searched via driver name which has same limitation as
> > previous method. This can be improved only when all drivers in chain are
> > probed via device tree compatible strings.
>
> So on a first glance this looks alright (also the fixed version on your
> github). However it doesn't work on the Pine64-LTS (or A64 in general),
> and I identified at least two problems below:
>
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >  drivers/video/sunxi/sunxi_de2.c | 88 +++++++++++++++++++--------------
> >  1 file changed, 52 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c
> > index e02d359cd259..81576e45e9ef 100644
> > --- a/drivers/video/sunxi/sunxi_de2.c
> > +++ b/drivers/video/sunxi/sunxi_de2.c
> > @@ -31,6 +31,11 @@ enum {
> >       LCD_MAX_LOG2_BPP        = VIDEO_BPP32,
> >  };
> >
> > +struct sunxi_de2_data {
> > +     int id;
>
> nit: can you rename this to something less general, like mux or
> pipeline_id?
>
> > +     const char *disp_drv_name;
> > +};
> > +
> >  static void sunxi_de2_composer_init(void)
> >  {
> >       struct sunxi_ccm_reg * const ccm =
> > @@ -228,51 +233,34 @@ static int sunxi_de2_init(struct udevice *dev, ulong fbbase,
> >
> >  static int sunxi_de2_probe(struct udevice *dev)
> >  {
> > +     const struct sunxi_de2_data *data =
> > +             (const struct sunxi_de2_data *)dev_get_driver_data(dev);
> >       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> >       struct udevice *disp;
> > -     int ret;
> > +     int ret, index = 0;
> >
> >       /* Before relocation we don't need to do anything */
> >       if (!(gd->flags & GD_FLG_RELOC))
> >               return 0;
> >
> > -     ret = uclass_get_device_by_driver(UCLASS_DISPLAY,
> > -                                       DM_DRIVER_GET(sunxi_lcd), &disp);
> > -     if (!ret) {
> > -             int mux;
> > +     while (!(ret = uclass_get_device(UCLASS_DISPLAY, index++, &disp))) {
>
> So this call tries to enumerate all devices registered as
> UCLASS_DISPLAY. For the A64 the sunxi_lcd driver provides one device,
> however it only probes correctly when there is bridge configured (for
> the Pinebook, for instance). There is code to read timings from DT,
> but I don't find any of the required properties (simple-panel) in any
> Allwinner DTs (using DE2, at least).
> As a consequence the probe in sunxi_lcd.c fails (except for the
> Pinebook), and it returns -ENODEV. This is unfortunately the same error
> code that uclass_get_device_by_driver() returns when there are no
> (more) devices providing UCLASS_DISPLAY, so the while loop stops here,
> before having considered the HDMI devices (which are enumerated *after*
> the LCD device). This is the same problem with your change to use
> uclass_id_foreach_dev(), btw.

Well, good to check the error to something different. The -ENODEV
error is reserved for driver model. It can be returned by the bind()
method, but not probe().

>
> Not sure how to best solve this, it sounds like a general DM related
> problem (failing probe() ending iterating all devices in one class).

That's intended though. I suppose we could change it. But you can
iterate through devices with uclass_first_device_err() if you want to
do them all..

Is this in core DM code or somewhere else?

>
> I hacked this a bit (using a different error code in sunxi_lcd_probe()
> and catching/translating this here), but this revealed another problem:
> I only ever see the mixer0 device being probed (actually multiple
> times). I could debug that both mixers are detected in the DT, created
> as two devices and bound (using the right .id value and the right DT
> offset), but only the first one is ever probed through this function
> here. Unfortunately this is the wrong one (HDMI is on mixer1), so
> de2_probe() fails :-(
> Disabling mixer0 in the DT makes it work (on top of the hack above).
>
> No real clue about this second problem, but I will debug further.
>
[..]

Regards,
Simon