Patchwork [U-Boot] fix IDE_BUS(dev) macro

login
register
mail settings
Submitter Luka Perkov
Date April 17, 2012, 7:06 p.m.
Message ID <20120417190649.GA22913@w500.iskon.local>
Download mbox | patch
Permalink /patch/153291/
State Superseded
Headers show

Comments

Luka Perkov - April 17, 2012, 7:06 p.m.
IDE_BUS assumes that each bus has two devices and thus returns the first
bus even when the second one should be probed.

Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
Tested-by: Luka Perkov <uboot@lukaperkov.net>
---

Simon discovered this while adding support for new board IB NAS6210.

More info can be found here:

http://lists.denx.de/pipermail/u-boot/2012-April/122525.html

When this is commited I will do a coding style cleanup. There are tabs
after few "#define" parts in include/ide.h.

 include/ide.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
David Purdy - April 18, 2012, 9:37 p.m.
On Tue, Apr 17, 2012 at 2:06 PM, Luka Perkov <uboot@lukaperkov.net> wrote:
> IDE_BUS assumes that each bus has two devices and thus returns the first
> bus even when the second one should be probed.
>
> Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> Tested-by: Luka Perkov <uboot@lukaperkov.net>
> ---
>
> Simon discovered this while adding support for new board IB NAS6210.
>
> More info can be found here:
>
> http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
>
> When this is commited I will do a coding style cleanup. There are tabs
> after few "#define" parts in include/ide.h.
>
>  include/ide.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/ide.h b/include/ide.h
> index 8ecc9dd..385e909 100644
> --- a/include/ide.h
> +++ b/include/ide.h
> @@ -24,7 +24,7 @@
>  #ifndef        _IDE_H

Simon, Luka, Prafulla, etal,

I've tested this out on the Seagate GoFlex Net (Kirkwood) device,
which also has dual SATA ports, and it resolved the long-standing (and
irritating) bug/unwanted-feature that required us to boot from _only_
the right side port.

I would guess that other Kirkwood dual-SATA-port boxes would be
happier because of this, as well.

Nice catch, Simon.

regards,

Dave
Albert ARIBAUD - April 19, 2012, 6:38 a.m.
Hi Dave,

Le 18/04/2012 23:37, David Purdy a écrit :
> On Tue, Apr 17, 2012 at 2:06 PM, Luka Perkov<uboot@lukaperkov.net>  wrote:
>> IDE_BUS assumes that each bus has two devices and thus returns the first
>> bus even when the second one should be probed.
>>
>> Signed-off-by: Simon Baatz<gmbnomis@gmail.com>
>> Tested-by: Luka Perkov<uboot@lukaperkov.net>
>> ---
>>
>> Simon discovered this while adding support for new board IB NAS6210.
>>
>> More info can be found here:
>>
>> http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
>>
>> When this is commited I will do a coding style cleanup. There are tabs
>> after few "#define" parts in include/ide.h.
>>
>>   include/ide.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/ide.h b/include/ide.h
>> index 8ecc9dd..385e909 100644
>> --- a/include/ide.h
>> +++ b/include/ide.h
>> @@ -24,7 +24,7 @@
>>   #ifndef        _IDE_H
>
> Simon, Luka, Prafulla, etal,
>
> I've tested this out on the Seagate GoFlex Net (Kirkwood) device,
> which also has dual SATA ports, and it resolved the long-standing (and
> irritating) bug/unwanted-feature that required us to boot from _only_
> the right side port.
>
> I would guess that other Kirkwood dual-SATA-port boxes would be
> happier because of this, as well.
>
> Nice catch, Simon.

Not my main area of expertise here, but I am not sure how this plays on 
Marvell non-kirkwood platforms (e.g., orion5x).

ISTR it is not the first time we deal with the whole IDE number of bus / 
number of ports [ / useable ports ] issue, and we may be running in 
circles here, fixing one platform and breaking another.

I'll try this on EDMiniV2 in the coming days, and let people know the 
results in this thread.

> regards,
>
> Dave

Amicalement,
Prafulla Wadaskar - April 20, 2012, 5:17 a.m.
> -----Original Message-----
> From: u-boot-bounces@lists.denx.de [mailto:u-boot-
> bounces@lists.denx.de] On Behalf Of Luka Perkov
> Sent: 18 April 2012 00:37
> To: u-boot@lists.denx.de
> Subject: [U-Boot] [PATCH] fix IDE_BUS(dev) macro
> 
> IDE_BUS assumes that each bus has two devices and thus returns the
> first
> bus even when the second one should be probed.
> 
> Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> Tested-by: Luka Perkov <uboot@lukaperkov.net>
> ---
> 
> Simon discovered this while adding support for new board IB NAS6210.
> 
> More info can be found here:
> 
> http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
> 
> When this is commited I will do a coding style cleanup. There are tabs
> after few "#define" parts in include/ide.h.
> 
>  include/ide.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/ide.h b/include/ide.h
> index 8ecc9dd..385e909 100644
> --- a/include/ide.h
> +++ b/include/ide.h
> @@ -24,7 +24,7 @@
>  #ifndef	_IDE_H
>  #define _IDE_H
> 
> -#define	IDE_BUS(dev)	(dev >> 1)
> +#define IDE_BUS(dev)	(dev / (CONFIG_SYS_IDE_MAXDEVICE /
> CONFIG_SYS_IDE_MAXBUS))
> 
>  #define	ATA_CURR_BASE(dev)
> 	(CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
>

Dear Wolfgang

This is generic patch to IDE framework, this patch is dependency for (being pulled) Kirkwood based boards.

Will you please kindly pull it? OR I should do the needful?

Regards..
Prafulla . . .
David Purdy - April 21, 2012, 7:38 p.m.
On Thu, Apr 19, 2012 at 1:38 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Dave,
>
>
>
>
> Not my main area of expertise here, but I am not sure how this plays on
> Marvell non-kirkwood platforms (e.g., orion5x).
>
> ISTR it is not the first time we deal with the whole IDE number of bus /
> number of ports [ / useable ports ] issue, and we may be running in circles
> here, fixing one platform and breaking another.

I, too, remember remember reading some discussions about changes to
that very line.


> I'll try this on EDMiniV2 in the coming days, and let people know the
> results in this thread.
>
>
>
> Amicalement,
> --
> Albert.

After further testing, I've found a possible (minor?) anomaly, but not
on a supported (official) Kirkwood device.  The device I tested it on
is a Seagate GoFlex Net (dual SATA).

The behavior I see is this:

With  Toshiba 160GB and Seagate 320GB hdd's in ports 0 and 1 (right &
left) respectively, I get perfect results.  I can access both drives
from U-Boot, and boot from both.

If I switch the drives around  (ie.  swap them) so the Toshiba is is
port 1 (left), I can no longer probe it.   It could very well be a
timing problem, or it could be simply a quirk that is simply
attributable to this hdd (which seems to check out completely
healthy).

If I place _just_ the Seagate drive in port 1 (left), it is still
correctly recognized.  This is true whether it is the lone hdd, or if
another hdd is in port 0.

Just thought I'd let you know.  Maybe this is a _specific_ behavior
subset that you all can test on your respective boxes.

My feeling is that the patch is nevertheless still valid  -  for what
I've seen on Kirkwood, it is an improvement over the previous
situation.  Just my 2 cents.

Dave
Luka Perkov - April 29, 2012, 8:19 p.m.
Hi Albert,

On Thu, Apr 19, 2012 at 08:38:19AM +0200, Albert ARIBAUD wrote:
> Not my main area of expertise here, but I am not sure how this plays
> on Marvell non-kirkwood platforms (e.g., orion5x).
> 
> ISTR it is not the first time we deal with the whole IDE number of
> bus / number of ports [ / useable ports ] issue, and we may be
> running in circles here, fixing one platform and breaking another.
> 
> I'll try this on EDMiniV2 in the coming days, and let people know
> the results in this thread.

I was just wondering did you have the time to test this patch on your
board?

Regards,
Luka
Rogan Dawes - June 3, 2012, 8:23 p.m.
On 19/04/2012 08:38, Albert ARIBAUD wrote:
> Hi Dave,
>
> Le 18/04/2012 23:37, David Purdy a écrit :
>> On Tue, Apr 17, 2012 at 2:06 PM, Luka Perkov<uboot@lukaperkov.net> wrote:
>>> IDE_BUS assumes that each bus has two devices and thus returns the first
>>> bus even when the second one should be probed.
>>>
>>> Signed-off-by: Simon Baatz<gmbnomis@gmail.com>
>>> Tested-by: Luka Perkov<uboot@lukaperkov.net>
>>> ---
>>>
>>> Simon discovered this while adding support for new board IB NAS6210.
>>>
>>> More info can be found here:
>>>
>>> http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
>>>
>>> When this is commited I will do a coding style cleanup. There are tabs
>>> after few "#define" parts in include/ide.h.
>>>
>>> include/ide.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/ide.h b/include/ide.h
>>> index 8ecc9dd..385e909 100644
>>> --- a/include/ide.h
>>> +++ b/include/ide.h
>>> @@ -24,7 +24,7 @@
>>> #ifndef _IDE_H
>>
>> Simon, Luka, Prafulla, etal,
>>
>> I've tested this out on the Seagate GoFlex Net (Kirkwood) device,
>> which also has dual SATA ports, and it resolved the long-standing (and
>> irritating) bug/unwanted-feature that required us to boot from _only_
>> the right side port.
>>
>> I would guess that other Kirkwood dual-SATA-port boxes would be
>> happier because of this, as well.
>>
>> Nice catch, Simon.
>
> Not my main area of expertise here, but I am not sure how this plays on
> Marvell non-kirkwood platforms (e.g., orion5x).
>
> ISTR it is not the first time we deal with the whole IDE number of bus /
> number of ports [ / useable ports ] issue, and we may be running in
> circles here, fixing one platform and breaking another.
>
> I'll try this on EDMiniV2 in the coming days, and let people know the
> results in this thread.
>
>> regards,
>>
>> Dave
>
> Amicalement,

I had something similar for my DNS323 port (orion5x-based), which other 
people may be remembering.

http://lists.denx.de/pipermail/u-boot/2010-August/075589.html

I seem to recall Wolfgang was not terribly enthused about it for some 
reason, though.

Rogan
Luka Perkov - June 3, 2012, 9:25 p.m.
Hi Rogan,

On Sun, Jun 03, 2012 at 10:23:15PM +0200, Rogan Dawes wrote:
> I had something similar for my DNS323 port (orion5x-based), which
> other people may be remembering.
> 
> http://lists.denx.de/pipermail/u-boot/2010-August/075589.html
> 
> I seem to recall Wolfgang was not terribly enthused about it for
> some reason, though.

I'm waiting for Albert to do testing on his boards. I was going to ping
him again about this one next week ;)

Regards,
Luka
Luka Perkov - Oct. 7, 2012, 7:15 a.m.
Hi Tom, Albert, Prafulla and others,

On Tue, Apr 17, 2012 at 09:06:49PM +0200, Luka Perkov wrote:
> IDE_BUS assumes that each bus has two devices and thus returns the first
> bus even when the second one should be probed.
> 
> Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> Tested-by: Luka Perkov <uboot@lukaperkov.net>
> ---
> 
> Simon discovered this while adding support for new board IB NAS6210.
> 
> More info can be found here:
> 
> http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
> 
> When this is commited I will do a coding style cleanup. There are tabs
> after few "#define" parts in include/ide.h.
> 
>  include/ide.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/ide.h b/include/ide.h
> index 8ecc9dd..385e909 100644
> --- a/include/ide.h
> +++ b/include/ide.h
> @@ -24,7 +24,7 @@
>  #ifndef	_IDE_H
>  #define _IDE_H
>  
> -#define	IDE_BUS(dev)	(dev >> 1)
> +#define IDE_BUS(dev)	(dev / (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS))
>  
>  #define	ATA_CURR_BASE(dev)	(CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
>  

I have assigned this patch to Tom in patchwork here:

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

From when it was submitted we have got confirmations that this patch
does the job for kirkwood boards on the u-boot mailing list:

http://lists.denx.de/pipermail/u-boot/2012-April/122684.html
http://lists.denx.de/pipermail/u-boot/2012-June/125658.html

It would be nice if this patch would enter v2012.10 release. At least
someone could pull it into his repo so it can find it's way into master
eventually.

Regards,
Luka
Albert ARIBAUD - Oct. 7, 2012, 9:28 a.m.
Hi uboot@lukaperkov.net,

On Sun, 7 Oct 2012 09:15:52 +0200, uboot@lukaperkov.net wrote:

> Hi Tom, Albert, Prafulla and others,
> 
> On Tue, Apr 17, 2012 at 09:06:49PM +0200, Luka Perkov wrote:
> > IDE_BUS assumes that each bus has two devices and thus returns the first
> > bus even when the second one should be probed.
> > 
> > Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
> > Tested-by: Luka Perkov <uboot@lukaperkov.net>
> > ---
> > 
> > Simon discovered this while adding support for new board IB NAS6210.
> > 
> > More info can be found here:
> > 
> > http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
> > 
> > When this is commited I will do a coding style cleanup. There are tabs
> > after few "#define" parts in include/ide.h.
> > 
> >  include/ide.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/ide.h b/include/ide.h
> > index 8ecc9dd..385e909 100644
> > --- a/include/ide.h
> > +++ b/include/ide.h
> > @@ -24,7 +24,7 @@
> >  #ifndef	_IDE_H
> >  #define _IDE_H
> >  
> > -#define	IDE_BUS(dev)	(dev >> 1)
> > +#define IDE_BUS(dev)	(dev / (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS))
> >  
> >  #define	ATA_CURR_BASE(dev)	(CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
> >  
> 
> I have assigned this patch to Tom in patchwork here:
> 
> http://patchwork.ozlabs.org/patch/159129/
> 
> From when it was submitted we have got confirmations that this patch
> does the job for kirkwood boards on the u-boot mailing list:
> 
> http://lists.denx.de/pipermail/u-boot/2012-April/122684.html
> http://lists.denx.de/pipermail/u-boot/2012-June/125658.html
> 
> It would be nice if this patch would enter v2012.10 release. At least
> someone could pull it into his repo so it can find it's way into master
> eventually.

As discussed on IRC, there are tests that actually cover the plaforms I
should have tested myself, so I personally think this can go in 2012.10.

> Regards,
> Luka

Amicalement,
Tom Rini - Oct. 7, 2012, 2:40 p.m.
On Sun, Oct 7, 2012 at 2:28 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi uboot@lukaperkov.net,
>
> On Sun, 7 Oct 2012 09:15:52 +0200, uboot@lukaperkov.net wrote:
>
>> Hi Tom, Albert, Prafulla and others,
>>
>> On Tue, Apr 17, 2012 at 09:06:49PM +0200, Luka Perkov wrote:
>> > IDE_BUS assumes that each bus has two devices and thus returns the first
>> > bus even when the second one should be probed.
>> >
>> > Signed-off-by: Simon Baatz <gmbnomis@gmail.com>
>> > Tested-by: Luka Perkov <uboot@lukaperkov.net>
>> > ---
>> >
>> > Simon discovered this while adding support for new board IB NAS6210.
>> >
>> > More info can be found here:
>> >
>> > http://lists.denx.de/pipermail/u-boot/2012-April/122525.html
>> >
>> > When this is commited I will do a coding style cleanup. There are tabs
>> > after few "#define" parts in include/ide.h.
>> >
>> >  include/ide.h |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/include/ide.h b/include/ide.h
>> > index 8ecc9dd..385e909 100644
>> > --- a/include/ide.h
>> > +++ b/include/ide.h
>> > @@ -24,7 +24,7 @@
>> >  #ifndef    _IDE_H
>> >  #define _IDE_H
>> >
>> > -#define    IDE_BUS(dev)    (dev >> 1)
>> > +#define IDE_BUS(dev)       (dev / (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS))
>> >
>> >  #define    ATA_CURR_BASE(dev)      (CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])
>> >
>>
>> I have assigned this patch to Tom in patchwork here:
>>
>> http://patchwork.ozlabs.org/patch/159129/
>>
>> From when it was submitted we have got confirmations that this patch
>> does the job for kirkwood boards on the u-boot mailing list:
>>
>> http://lists.denx.de/pipermail/u-boot/2012-April/122684.html
>> http://lists.denx.de/pipermail/u-boot/2012-June/125658.html
>>
>> It would be nice if this patch would enter v2012.10 release. At least
>> someone could pull it into his repo so it can find it's way into master
>> eventually.
>
> As discussed on IRC, there are tests that actually cover the plaforms I
> should have tested myself, so I personally think this can go in 2012.10.

OK, I'll pick this up Monday, thanks.

Patch

diff --git a/include/ide.h b/include/ide.h
index 8ecc9dd..385e909 100644
--- a/include/ide.h
+++ b/include/ide.h
@@ -24,7 +24,7 @@ 
 #ifndef	_IDE_H
 #define _IDE_H
 
-#define	IDE_BUS(dev)	(dev >> 1)
+#define IDE_BUS(dev)	(dev / (CONFIG_SYS_IDE_MAXDEVICE / CONFIG_SYS_IDE_MAXBUS))
 
 #define	ATA_CURR_BASE(dev)	(CONFIG_SYS_ATA_BASE_ADDR+ide_bus_offset[IDE_BUS(dev)])