[U-Boot] Raspberry Pi with driver model

[U-Boot] Raspberry Pi with driver model

[Simon Glass]

Hi Stephen,

There was another thread where you reported a hang when booting
Raspberry Pi with driver model. It can be repeated by applying this
patch and trying to boot with syslinux.

http://patchwork.ozlabs.org/patch/392180/

When it hangs, the HDMI display displays lots of spaces, then lots of
dots, scrolling forever. The serial console displays nothing further.

I narrowed the problem down to writing to address 0. So it is easy to
repeat with something like 'mw 0 0'.

However I'm really not sure what is going on. Something with driver
model seems to make address 0 sensitive to writes. I can't see what
that might be, but I don't understand the platform very well. Do you
have any ideas?

Regards,
Simon

[U-Boot] Raspberry Pi with driver model

[Stephen Warren]

On 11/24/2014 08:58 AM, Simon Glass wrote:
> Hi Stephen,
> 
> There was another thread where you reported a hang when booting
> Raspberry Pi with driver model. It can be repeated by applying this
> patch and trying to boot with syslinux.
> 
> http://patchwork.ozlabs.org/patch/392180/
> 
> When it hangs, the HDMI display displays lots of spaces, then lots of
> dots, scrolling forever. The serial console displays nothing further.
> 
> I narrowed the problem down to writing to address 0. So it is easy to
> repeat with something like 'mw 0 0'.
> 
> However I'm really not sure what is going on. Something with driver
> model seems to make address 0 sensitive to writes. I can't see what
> that might be, but I don't understand the platform very well. Do you
> have any ideas?

The problem seems to be that inside pl01x_serial_probe() (well, I assume
all serial functions), dev_get_priv() returns 0, which ends up using
address 0 as the UART address since that's the first field in struct
pl01x_priv.

I found this by amending pl01x_serial_probe() as follows, and dumping
the data after boot:

uint32_t *log = (uint32_t *)(128*1024*1024);
uint32_t log_i = 1;

static int pl01x_serial_probe(struct udevice *dev)
{
	struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
	struct pl01x_priv *priv = dev_get_priv(dev);

        log[log_i++] = (uint32_t)dev;
        log[log_i++] = (uint32_t)plat;
        log[log_i++] = (uint32_t)priv;
        log[0] = log_i;
	priv->regs = (struct pl01x_regs *)plat->base;
	priv->type = plat->type;
	return pl01x_generic_serial_init(priv->regs, priv->type);
}

U-Boot> md.l 0x08000000
08000000: 00000007 07fffaf8 0003972c 00000000    ........,.......
08000010: 1db4e108 1df8372c 00000000 00000000    ....,7..........

It seems a bit odd that pl01x_serial_probe() is called twice, but
perhaps that's expected?

[U-Boot] Raspberry Pi with driver model

[Simon Glass]

Hi Stephen,

On 24 November 2014 at 21:21, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/24/2014 08:58 AM, Simon Glass wrote:
>> Hi Stephen,
>>
>> There was another thread where you reported a hang when booting
>> Raspberry Pi with driver model. It can be repeated by applying this
>> patch and trying to boot with syslinux.
>>
>> http://patchwork.ozlabs.org/patch/392180/
>>
>> When it hangs, the HDMI display displays lots of spaces, then lots of
>> dots, scrolling forever. The serial console displays nothing further.
>>
>> I narrowed the problem down to writing to address 0. So it is easy to
>> repeat with something like 'mw 0 0'.
>>
>> However I'm really not sure what is going on. Something with driver
>> model seems to make address 0 sensitive to writes. I can't see what
>> that might be, but I don't understand the platform very well. Do you
>> have any ideas?
>
> The problem seems to be that inside pl01x_serial_probe() (well, I assume
> all serial functions), dev_get_priv() returns 0, which ends up using
> address 0 as the UART address since that's the first field in struct
> pl01x_priv.
>
> I found this by amending pl01x_serial_probe() as follows, and dumping
> the data after boot:
>
> uint32_t *log = (uint32_t *)(128*1024*1024);
> uint32_t log_i = 1;
>
> static int pl01x_serial_probe(struct udevice *dev)
> {
>         struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>         struct pl01x_priv *priv = dev_get_priv(dev);
>
>         log[log_i++] = (uint32_t)dev;
>         log[log_i++] = (uint32_t)plat;
>         log[log_i++] = (uint32_t)priv;
>         log[0] = log_i;
>         priv->regs = (struct pl01x_regs *)plat->base;
>         priv->type = plat->type;
>         return pl01x_generic_serial_init(priv->regs, priv->type);
> }
>
> U-Boot> md.l 0x08000000
> 08000000: 00000007 07fffaf8 0003972c 00000000    ........,.......
> 08000010: 1db4e108 1df8372c 00000000 00000000    ....,7..........
>
> It seems a bit odd that pl01x_serial_probe() is called twice, but
> perhaps that's expected?

Thanks for finding that! It just needs this in the U_BOOT_DRIVER()
declaration in the serial driver:

.priv_auto_alloc_size = sizeof(struct pl01x_priv),

Then it all works. I'll update the series and resend. Thanks again.

Yes it will call the serial probe once before relocation, once after.

Regards,
Simon

[U-Boot] Raspberry Pi with driver model

[Stephen Warren]

On 11/24/2014 09:32 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 24 November 2014 at 21:21, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/24/2014 08:58 AM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> There was another thread where you reported a hang when booting
>>> Raspberry Pi with driver model. It can be repeated by applying this
>>> patch and trying to boot with syslinux.
>>>
>>> http://patchwork.ozlabs.org/patch/392180/
>>>
>>> When it hangs, the HDMI display displays lots of spaces, then lots of
>>> dots, scrolling forever. The serial console displays nothing further.
>>>
>>> I narrowed the problem down to writing to address 0. So it is easy to
>>> repeat with something like 'mw 0 0'.
>>>
>>> However I'm really not sure what is going on. Something with driver
>>> model seems to make address 0 sensitive to writes. I can't see what
>>> that might be, but I don't understand the platform very well. Do you
>>> have any ideas?
>>
>> The problem seems to be that inside pl01x_serial_probe() (well, I assume
>> all serial functions), dev_get_priv() returns 0, which ends up using
>> address 0 as the UART address since that's the first field in struct
>> pl01x_priv.
>>
>> I found this by amending pl01x_serial_probe() as follows, and dumping
>> the data after boot:
>>
>> uint32_t *log = (uint32_t *)(128*1024*1024);
>> uint32_t log_i = 1;
>>
>> static int pl01x_serial_probe(struct udevice *dev)
>> {
>>          struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>>          struct pl01x_priv *priv = dev_get_priv(dev);
>>
>>          log[log_i++] = (uint32_t)dev;
>>          log[log_i++] = (uint32_t)plat;
>>          log[log_i++] = (uint32_t)priv;
>>          log[0] = log_i;
>>          priv->regs = (struct pl01x_regs *)plat->base;
>>          priv->type = plat->type;
>>          return pl01x_generic_serial_init(priv->regs, priv->type);
>> }
>>
>> U-Boot> md.l 0x08000000
>> 08000000: 00000007 07fffaf8 0003972c 00000000    ........,.......
>> 08000010: 1db4e108 1df8372c 00000000 00000000    ....,7..........
>>
>> It seems a bit odd that pl01x_serial_probe() is called twice, but
>> perhaps that's expected?
>
> Thanks for finding that! It just needs this in the U_BOOT_DRIVER()
> declaration in the serial driver:
>
> .priv_auto_alloc_size = sizeof(struct pl01x_priv),
>
> Then it all works. I'll update the series and resend. Thanks again.

Does it make sense to add an error check for one or more of the 
following to the driver core:

a) Drivers/devices with .priv_auto_alloc_size == 0.

b) Devices with NULL priv fields, when the core is about to call the 
driver-specific probe() function.

[U-Boot] Raspberry Pi with driver model

[Simon Glass]

Hi Stephen,

On 25 November 2014 at 09:44, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/24/2014 09:32 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 24 November 2014 at 21:21, Stephen Warren <swarren@nvidia.com> wrote:
>>>
>>> On 11/24/2014 08:58 AM, Simon Glass wrote:
>>>>
>>>> Hi Stephen,
>>>>
>>>> There was another thread where you reported a hang when booting
>>>> Raspberry Pi with driver model. It can be repeated by applying this
>>>> patch and trying to boot with syslinux.
>>>>
>>>> http://patchwork.ozlabs.org/patch/392180/
>>>>
>>>> When it hangs, the HDMI display displays lots of spaces, then lots of
>>>> dots, scrolling forever. The serial console displays nothing further.
>>>>
>>>> I narrowed the problem down to writing to address 0. So it is easy to
>>>> repeat with something like 'mw 0 0'.
>>>>
>>>> However I'm really not sure what is going on. Something with driver
>>>> model seems to make address 0 sensitive to writes. I can't see what
>>>> that might be, but I don't understand the platform very well. Do you
>>>> have any ideas?
>>>
>>>
>>> The problem seems to be that inside pl01x_serial_probe() (well, I assume
>>> all serial functions), dev_get_priv() returns 0, which ends up using
>>> address 0 as the UART address since that's the first field in struct
>>> pl01x_priv.
>>>
>>> I found this by amending pl01x_serial_probe() as follows, and dumping
>>> the data after boot:
>>>
>>> uint32_t *log = (uint32_t *)(128*1024*1024);
>>> uint32_t log_i = 1;
>>>
>>> static int pl01x_serial_probe(struct udevice *dev)
>>> {
>>>          struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>>>          struct pl01x_priv *priv = dev_get_priv(dev);
>>>
>>>          log[log_i++] = (uint32_t)dev;
>>>          log[log_i++] = (uint32_t)plat;
>>>          log[log_i++] = (uint32_t)priv;
>>>          log[0] = log_i;
>>>          priv->regs = (struct pl01x_regs *)plat->base;
>>>          priv->type = plat->type;
>>>          return pl01x_generic_serial_init(priv->regs, priv->type);
>>> }
>>>
>>> U-Boot> md.l 0x08000000
>>> 08000000: 00000007 07fffaf8 0003972c 00000000    ........,.......
>>> 08000010: 1db4e108 1df8372c 00000000 00000000    ....,7..........
>>>
>>> It seems a bit odd that pl01x_serial_probe() is called twice, but
>>> perhaps that's expected?
>>
>>
>> Thanks for finding that! It just needs this in the U_BOOT_DRIVER()
>> declaration in the serial driver:
>>
>> .priv_auto_alloc_size = sizeof(struct pl01x_priv),
>>
>> Then it all works. I'll update the series and resend. Thanks again.
>
>
> Does it make sense to add an error check for one or more of the following to
> the driver core:
>
> a) Drivers/devices with .priv_auto_alloc_size == 0.

That is perfectly valid if drivers want to alloc their own space.

>
> b) Devices with NULL priv fields, when the core is about to call the
> driver-specific probe() function.

Also valid :-(

I did write a longer screed but my plan is for dev_get_priv() to have
checks as to what it is actually returning. I've done this a few times
in life and it's not very hard. That's why I'm trying to access things
through functions.

Regards,
Simon