LinuxLists.cc - [git patches] IDE updates (part 2)

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Saturday 13 October 2007, Alan Cox wrote:
> On Sat, 13 Oct 2007 18:25:24 +0200
> Bartlomiej Zolnierkiewicz <[email protected]> wrote:
>
> >
> > Hi,
> >
> > highlights of this update:
> >
> > * Rework of IDE PMAC host driver: bugfixes, removal of the code
> > duplicated from the IDE core and conversion to use the generic
> > DMA tuning code path (the rework cuts ide-pmac.c by ~200 LOC).
>
> Reading the current driver from the git tree I don't see how the PCI
> driver ever sets the ctl register base. It seems to always be set to zero
> which means you can't issue SRST and reset sequences ?

Hi Alan,

Comment in pmac_ide_init_hwif_ports() is highly misleading as this function
returns early only for "normal" IDE PCI devices (pmac_ide_init_hwif_ports()
can be called outside ide-pmac driver through ppc_ide_md).

static int __devinit
pmac_ide_pci_attach(struct pci_dev *pdev, const struct pci_device_id *id)
{
...
pmif = &pmac_ide[i];
...
pmif->regbase = (unsigned long) base + 0x2000;
...
rc = pmac_ide_setup_device(pmif, hwif);
...
}

static int
pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
{
...
pmac_ide_init_hwif_ports(&hwif->hw, pmif->regbase, 0, &hwif->irq);
...
}

void
pmac_ide_init_hwif_ports(hw_regs_t *hw,
unsigned long data_port, unsigned long ctrl_port,
int *irq)
{
...
for (ix = 0; ix < MAX_HWIFS; ++ix)
if (data_port == pmac_ide[ix].regbase)
break;
---> since pmif->regbase was set earlier ix will be < MAX_HWIFS
if (ix >= MAX_HWIFS) {
/* Probably a PCI interface... */
for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
return;
}

for (i = 0; i < 8; ++i)
hw->io_ports[i] = data_port + i * 0x10;
---> ctl register base will be set here
hw->io_ports[8] = data_port + 0x160;
...
}

Bart

2007-10-13 21:40:50

by Alan

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

> Comment in pmac_ide_init_hwif_ports() is highly misleading as this function
> returns early only for "normal" IDE PCI devices (pmac_ide_init_hwif_ports()
> can be called outside ide-pmac driver through ppc_ide_md).

Follow the code you pasted

> pmif->regbase = (unsigned long) base + 0x2000;
> ...
> rc = pmac_ide_setup_device(pmif, hwif);

Ok so regbase is set

> ...
> }
>
> static int
> pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
> {
> ...
> pmac_ide_init_hwif_ports(&hwif->hw, pmif->regbase, 0, &hwif->irq);

Now we pass a honking great zero for the ctrl_port
> ...
> }
>
> void
> pmac_ide_init_hwif_ports(hw_regs_t *hw,
> unsigned long data_port, unsigned long ctrl_port,
> int *irq)
> {
> ...
> for (ix = 0; ix < MAX_HWIFS; ++ix)
> if (data_port == pmac_ide[ix].regbase)
> break;
> ---> since pmif->regbase was set earlier ix will be < MAX_HWIFS

> if (ix >= MAX_HWIFS) {
> /* Probably a PCI interface... */
> for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;

Which is zero..

See the problem ?

Alan

2007-10-13 22:05:24

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Saturday 13 October 2007, Alan Cox wrote:
> > Comment in pmac_ide_init_hwif_ports() is highly misleading as this function
> > returns early only for "normal" IDE PCI devices (pmac_ide_init_hwif_ports()
> > can be called outside ide-pmac driver through ppc_ide_md).
>
> Follow the code you pasted
>
> > pmif->regbase = (unsigned long) base + 0x2000;
> > ...
> > rc = pmac_ide_setup_device(pmif, hwif);
>
> Ok so regbase is set
>
> > ...
> > }
> >
> > static int
> > pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
> > {
> > ...
> > pmac_ide_init_hwif_ports(&hwif->hw, pmif->regbase, 0, &hwif->irq);
>
> Now we pass a honking great zero for the ctrl_port
> > ...
> > }
> >
> > void
> > pmac_ide_init_hwif_ports(hw_regs_t *hw,
> > unsigned long data_port, unsigned long ctrl_port,
> > int *irq)
> > {
> > ...
> > for (ix = 0; ix < MAX_HWIFS; ++ix)

this loop is a tricky part, regbase is set so we break out early

> > if (data_port == pmac_ide[ix].regbase)
> > break;
> > ---> since pmif->regbase was set earlier ix will be < MAX_HWIFS
^^^^^^^^^^^^^^^^^^^^^^
> > if (ix >= MAX_HWIFS) {
^^^^^^^^^^^^^^^
> > /* Probably a PCI interface... */
> > for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> > hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> > hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
>
> Which is zero..
>
>
> See the problem ?

2007-10-13 22:25:23

by Alan

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

> > > /* Probably a PCI interface... */
> > > for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> > > hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> > > hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;

Ok so in actual fact

- The piece of code above can't be executed anyway
- The ctrl_port argument is not needed ?

Alan

2007-10-13 22:36:26

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sunday 14 October 2007, Alan Cox wrote:
> > > > /* Probably a PCI interface... */
> > > > for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> > > > hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> > > > hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
>
> Ok so in actual fact
>
> - The piece of code above can't be executed anyway
> - The ctrl_port argument is not needed ?

pmac_ide_init_hwif_ports() is also called by ide_init_hwif_ports()
through ppc_ide_md.init_hwif.

Bart

2007-10-14 00:08:32

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sun, 2007-10-14 at 00:41 +0200, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 14 October 2007, Alan Cox wrote:
> > > > > /* Probably a PCI interface... */
> > > > > for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> > > > > hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> > > > > hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
> >
> > Ok so in actual fact
> >
> > - The piece of code above can't be executed anyway
> > - The ctrl_port argument is not needed ?
>
> pmac_ide_init_hwif_ports() is also called by ide_init_hwif_ports()
> through ppc_ide_md.init_hwif.

In which case it should be called with a ctrl_port right ?

Ben.

2007-10-14 00:12:59

by Al Viro

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

Proposed addition to icside part, provided that ARM folks ACK it - gets
icside to build and AFAICS it's correct:

diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index be30923..842fe08 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -332,12 +332,13 @@ static void ata_dummy_noret(struct ata_port *port)
{
}

-static void pata_icside_postreset(struct ata_port *ap, unsigned int *classes)
+static void pata_icside_postreset(struct ata_link *link, unsigned int *classes)
{
+ struct ata_port *ap = link->ap;
struct pata_icside_state *state = ap->host->private_data;

if (classes[0] != ATA_DEV_NONE || classes[1] != ATA_DEV_NONE)
- return ata_std_postreset(ap, classes);
+ return ata_std_postreset(link, classes);

state->port[ap->port_no].disabled = 1;

@@ -395,29 +396,30 @@ static struct ata_port_operations pata_icside_port_ops = {

static void __devinit
pata_icside_setup_ioaddr(struct ata_port *ap, void __iomem *base,
- const struct portinfo *info)
+ struct pata_icside_info *info,
+ const struct portinfo *port)
{
struct ata_ioports *ioaddr = &ap->ioaddr;
- void __iomem *cmd = base + info->dataoffset;
+ void __iomem *cmd = base + port->dataoffset;

ioaddr->cmd_addr = cmd;
- ioaddr->data_addr = cmd + (ATA_REG_DATA << info->stepping);
- ioaddr->error_addr = cmd + (ATA_REG_ERR << info->stepping);
- ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << info->stepping);
- ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << info->stepping);
- ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << info->stepping);
- ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << info->stepping);
- ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << info->stepping);
- ioaddr->device_addr = cmd + (ATA_REG_DEVICE << info->stepping);
- ioaddr->status_addr = cmd + (ATA_REG_STATUS << info->stepping);
- ioaddr->command_addr = cmd + (ATA_REG_CMD << info->stepping);
-
- ioaddr->ctl_addr = base + info->ctrloffset;
+ ioaddr->data_addr = cmd + (ATA_REG_DATA << port->stepping);
+ ioaddr->error_addr = cmd + (ATA_REG_ERR << port->stepping);
+ ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << port->stepping);
+ ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << port->stepping);
+ ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << port->stepping);
+ ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << port->stepping);
+ ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << port->stepping);
+ ioaddr->device_addr = cmd + (ATA_REG_DEVICE << port->stepping);
+ ioaddr->status_addr = cmd + (ATA_REG_STATUS << port->stepping);
+ ioaddr->command_addr = cmd + (ATA_REG_CMD << port->stepping);
+
+ ioaddr->ctl_addr = base + port->ctrloffset;
ioaddr->altstatus_addr = ioaddr->ctl_addr;

ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
- info->raw_base + info->dataoffset,
- info->raw_base + info->ctrloffset);
+ info->raw_base + port->dataoffset,
+ info->raw_base + port->ctrloffset);

if (info->raw_ioc_base)
ata_port_desc(ap, "iocbase 0x%lx", info->raw_ioc_base);
@@ -441,7 +443,7 @@ static int __devinit pata_icside_register_v5(struct pata_icside_info *info)
info->nr_ports = 1;
info->port[0] = &pata_icside_portinfo_v5;

- info->raw_base = ecard_resource_start(ec, ECARD_RES_MEMC);
+ info->raw_base = ecard_resource_start(info->ec, ECARD_RES_MEMC);

return 0;
}
@@ -522,7 +524,7 @@ static int __devinit pata_icside_add_ports(struct pata_icside_info *info)
ap->flags |= ATA_FLAG_SLAVE_POSS;
ap->ops = &pata_icside_port_ops;

- pata_icside_setup_ioaddr(ap, info->base, info->port[i]);
+ pata_icside_setup_ioaddr(ap, info->base, info, info->port[i]);
}

return ata_host_activate(host, ec->irq, ata_interrupt, 0,

2007-10-14 16:16:56

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sunday 14 October 2007, Benjamin Herrenschmidt wrote:
>
> On Sun, 2007-10-14 at 00:41 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 14 October 2007, Alan Cox wrote:
> > > > > > /* Probably a PCI interface... */
> > > > > > for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> > > > > > hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> > > > > > hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
> > >
> > > Ok so in actual fact
> > >
> > > - The piece of code above can't be executed anyway
> > > - The ctrl_port argument is not needed ?
> >
> > pmac_ide_init_hwif_ports() is also called by ide_init_hwif_ports()
> > through ppc_ide_md.init_hwif.
>
> In which case it should be called with a ctrl_port right ?

Yep.

How's about this patch?

[PATCH] ide-pmac: fix pmac_ide_init_hwif_ports()

* pmac_ide_init_hwif_ports() can be called by ide_init_hwif_ports()
(through ppc_ide_md.ide_init_hwif hook) for non IDE PMAC interfaces.
If this is the case the hw->io_ports[] should be already setup by
ide_init_hwif_ports()->ide_std_init_ports() so remove redundant code
from pmac_ide_init_hwif_ports().

As side-effect this change fixes ctl_addr == 0 special handling in
ide_init_hwif_ports().

* Fix misleading comment while at it.

Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Alan Cox <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
drivers/ide/ppc/pmac.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

Index: b/drivers/ide/ppc/pmac.c
===================================================================
--- a/drivers/ide/ppc/pmac.c
+++ b/drivers/ide/ppc/pmac.c
@@ -438,13 +438,8 @@ pmac_ide_init_hwif_ports(hw_regs_t *hw,
if (data_port == pmac_ide[ix].regbase)
break;

- if (ix >= MAX_HWIFS) {
- /* Probably a PCI interface... */
- for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
- hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
- hw->io_ports[IDE_CONTROL_OFFSET] = ctrl_port;
- return;
- }
+ if (ix >= MAX_HWIFS)
+ return; /* not an IDE PMAC interface */

for (i = 0; i < 8; ++i)
hw->io_ports[i] = data_port + i * 0x10;

2007-10-14 16:17:25

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sunday 14 October 2007, Al Viro wrote:
> Proposed addition to icside part, provided that ARM folks ACK it - gets
> icside to build and AFAICS it's correct:

the patch looks good and gets my ACK but it is for Jeff ;)

> diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
> index be30923..842fe08 100644
> --- a/drivers/ata/pata_icside.c
> +++ b/drivers/ata/pata_icside.c
> @@ -332,12 +332,13 @@ static void ata_dummy_noret(struct ata_port *port)
> {
> }
>
> -static void pata_icside_postreset(struct ata_port *ap, unsigned int *classes)
> +static void pata_icside_postreset(struct ata_link *link, unsigned int *classes)
> {
> + struct ata_port *ap = link->ap;
> struct pata_icside_state *state = ap->host->private_data;
>
> if (classes[0] != ATA_DEV_NONE || classes[1] != ATA_DEV_NONE)
> - return ata_std_postreset(ap, classes);
> + return ata_std_postreset(link, classes);
>
> state->port[ap->port_no].disabled = 1;
>
> @@ -395,29 +396,30 @@ static struct ata_port_operations pata_icside_port_ops = {
>
> static void __devinit
> pata_icside_setup_ioaddr(struct ata_port *ap, void __iomem *base,
> - const struct portinfo *info)
> + struct pata_icside_info *info,
> + const struct portinfo *port)
> {
> struct ata_ioports *ioaddr = &ap->ioaddr;
> - void __iomem *cmd = base + info->dataoffset;
> + void __iomem *cmd = base + port->dataoffset;
>
> ioaddr->cmd_addr = cmd;
> - ioaddr->data_addr = cmd + (ATA_REG_DATA << info->stepping);
> - ioaddr->error_addr = cmd + (ATA_REG_ERR << info->stepping);
> - ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << info->stepping);
> - ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << info->stepping);
> - ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << info->stepping);
> - ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << info->stepping);
> - ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << info->stepping);
> - ioaddr->device_addr = cmd + (ATA_REG_DEVICE << info->stepping);
> - ioaddr->status_addr = cmd + (ATA_REG_STATUS << info->stepping);
> - ioaddr->command_addr = cmd + (ATA_REG_CMD << info->stepping);
> -
> - ioaddr->ctl_addr = base + info->ctrloffset;
> + ioaddr->data_addr = cmd + (ATA_REG_DATA << port->stepping);
> + ioaddr->error_addr = cmd + (ATA_REG_ERR << port->stepping);
> + ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << port->stepping);
> + ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << port->stepping);
> + ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << port->stepping);
> + ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << port->stepping);
> + ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << port->stepping);
> + ioaddr->device_addr = cmd + (ATA_REG_DEVICE << port->stepping);
> + ioaddr->status_addr = cmd + (ATA_REG_STATUS << port->stepping);
> + ioaddr->command_addr = cmd + (ATA_REG_CMD << port->stepping);
> +
> + ioaddr->ctl_addr = base + port->ctrloffset;
> ioaddr->altstatus_addr = ioaddr->ctl_addr;
>
> ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> - info->raw_base + info->dataoffset,
> - info->raw_base + info->ctrloffset);
> + info->raw_base + port->dataoffset,
> + info->raw_base + port->ctrloffset);
>
> if (info->raw_ioc_base)
> ata_port_desc(ap, "iocbase 0x%lx", info->raw_ioc_base);
> @@ -441,7 +443,7 @@ static int __devinit pata_icside_register_v5(struct pata_icside_info *info)
> info->nr_ports = 1;
> info->port[0] = &pata_icside_portinfo_v5;
>
> - info->raw_base = ecard_resource_start(ec, ECARD_RES_MEMC);
> + info->raw_base = ecard_resource_start(info->ec, ECARD_RES_MEMC);
>
> return 0;
> }
> @@ -522,7 +524,7 @@ static int __devinit pata_icside_add_ports(struct pata_icside_info *info)
> ap->flags |= ATA_FLAG_SLAVE_POSS;
> ap->ops = &pata_icside_port_ops;
>
> - pata_icside_setup_ioaddr(ap, info->base, info->port[i]);
> + pata_icside_setup_ioaddr(ap, info->base, info, info->port[i]);
> }
>
> return ata_host_activate(host, ec->irq, ata_interrupt, 0,
>

2007-10-14 21:15:41

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

> How's about this patch?
>
> [PATCH] ide-pmac: fix pmac_ide_init_hwif_ports()
>
> * pmac_ide_init_hwif_ports() can be called by ide_init_hwif_ports()
> (through ppc_ide_md.ide_init_hwif hook) for non IDE PMAC interfaces.
> If this is the case the hw->io_ports[] should be already setup by
> ide_init_hwif_ports()->ide_std_init_ports() so remove redundant code
> from pmac_ide_init_hwif_ports().
>
> As side-effect this change fixes ctl_addr == 0 special handling in
> ide_init_hwif_ports().
>
> * Fix misleading comment while at it.

I would have to try it. Problem is, I don't actually have any powermac
with a PCI IDE controller at hand.. ouch. I'll see what I can find.

Ben.

2007-10-15 19:54:39

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

Al Viro wrote:
> Proposed addition to icside part, provided that ARM folks ACK it - gets
> icside to build and AFAICS it's correct:

ACK from me too... ARM folks?

2007-10-15 20:03:44

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
> Al Viro wrote:
> >Proposed addition to icside part, provided that ARM folks ACK it - gets
> >icside to build and AFAICS it's correct:
>
> ACK from me too... ARM folks?

Will test tomorrow/Wednesday.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-10-15 20:08:19

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

Russell King wrote:
> On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
>> Al Viro wrote:
>>> Proposed addition to icside part, provided that ARM folks ACK it - gets
>>> icside to build and AFAICS it's correct:
>> ACK from me too... ARM folks?
>
> Will test tomorrow/Wednesday.

Thanks. Sorry about missing your CC on the libata-wide ata_link
changes. We should have poked the maintainers on that, for the drivers
we cannot build ourselves.

2007-10-16 16:00:49

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sun, Oct 14, 2007 at 01:12:39AM +0100, Al Viro wrote:
> Proposed addition to icside part, provided that ARM folks ACK it - gets
> icside to build and AFAICS it's correct:

Booted and tested as working.

Whatever-tag-that-is: Russell King <[email protected]>

> diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
> index be30923..842fe08 100644
> --- a/drivers/ata/pata_icside.c
> +++ b/drivers/ata/pata_icside.c
> @@ -332,12 +332,13 @@ static void ata_dummy_noret(struct ata_port *port)
> {
> }
>
> -static void pata_icside_postreset(struct ata_port *ap, unsigned int *classes)
> +static void pata_icside_postreset(struct ata_link *link, unsigned int *classes)
> {
> + struct ata_port *ap = link->ap;
> struct pata_icside_state *state = ap->host->private_data;
>
> if (classes[0] != ATA_DEV_NONE || classes[1] != ATA_DEV_NONE)
> - return ata_std_postreset(ap, classes);
> + return ata_std_postreset(link, classes);
>
> state->port[ap->port_no].disabled = 1;
>
> @@ -395,29 +396,30 @@ static struct ata_port_operations pata_icside_port_ops = {
>
> static void __devinit
> pata_icside_setup_ioaddr(struct ata_port *ap, void __iomem *base,
> - const struct portinfo *info)
> + struct pata_icside_info *info,
> + const struct portinfo *port)
> {
> struct ata_ioports *ioaddr = &ap->ioaddr;
> - void __iomem *cmd = base + info->dataoffset;
> + void __iomem *cmd = base + port->dataoffset;
>
> ioaddr->cmd_addr = cmd;
> - ioaddr->data_addr = cmd + (ATA_REG_DATA << info->stepping);
> - ioaddr->error_addr = cmd + (ATA_REG_ERR << info->stepping);
> - ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << info->stepping);
> - ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << info->stepping);
> - ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << info->stepping);
> - ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << info->stepping);
> - ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << info->stepping);
> - ioaddr->device_addr = cmd + (ATA_REG_DEVICE << info->stepping);
> - ioaddr->status_addr = cmd + (ATA_REG_STATUS << info->stepping);
> - ioaddr->command_addr = cmd + (ATA_REG_CMD << info->stepping);
> -
> - ioaddr->ctl_addr = base + info->ctrloffset;
> + ioaddr->data_addr = cmd + (ATA_REG_DATA << port->stepping);
> + ioaddr->error_addr = cmd + (ATA_REG_ERR << port->stepping);
> + ioaddr->feature_addr = cmd + (ATA_REG_FEATURE << port->stepping);
> + ioaddr->nsect_addr = cmd + (ATA_REG_NSECT << port->stepping);
> + ioaddr->lbal_addr = cmd + (ATA_REG_LBAL << port->stepping);
> + ioaddr->lbam_addr = cmd + (ATA_REG_LBAM << port->stepping);
> + ioaddr->lbah_addr = cmd + (ATA_REG_LBAH << port->stepping);
> + ioaddr->device_addr = cmd + (ATA_REG_DEVICE << port->stepping);
> + ioaddr->status_addr = cmd + (ATA_REG_STATUS << port->stepping);
> + ioaddr->command_addr = cmd + (ATA_REG_CMD << port->stepping);
> +
> + ioaddr->ctl_addr = base + port->ctrloffset;
> ioaddr->altstatus_addr = ioaddr->ctl_addr;
>
> ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
> - info->raw_base + info->dataoffset,
> - info->raw_base + info->ctrloffset);
> + info->raw_base + port->dataoffset,
> + info->raw_base + port->ctrloffset);
>
> if (info->raw_ioc_base)
> ata_port_desc(ap, "iocbase 0x%lx", info->raw_ioc_base);
> @@ -441,7 +443,7 @@ static int __devinit pata_icside_register_v5(struct pata_icside_info *info)
> info->nr_ports = 1;
> info->port[0] = &pata_icside_portinfo_v5;
>
> - info->raw_base = ecard_resource_start(ec, ECARD_RES_MEMC);
> + info->raw_base = ecard_resource_start(info->ec, ECARD_RES_MEMC);
>
> return 0;
> }
> @@ -522,7 +524,7 @@ static int __devinit pata_icside_add_ports(struct pata_icside_info *info)
> ap->flags |= ATA_FLAG_SLAVE_POSS;
> ap->ops = &pata_icside_port_ops;
>
> - pata_icside_setup_ioaddr(ap, info->base, info->port[i]);
> + pata_icside_setup_ioaddr(ap, info->base, info, info->port[i]);
> }
>
> return ata_host_activate(host, ec->irq, ata_interrupt, 0,

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-10-16 20:51:07

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Sunday 14 October 2007, Benjamin Herrenschmidt wrote:
>
> > How's about this patch?
> >
> > [PATCH] ide-pmac: fix pmac_ide_init_hwif_ports()
> >
> > * pmac_ide_init_hwif_ports() can be called by ide_init_hwif_ports()
> > (through ppc_ide_md.ide_init_hwif hook) for non IDE PMAC interfaces.
> > If this is the case the hw->io_ports[] should be already setup by
> > ide_init_hwif_ports()->ide_std_init_ports() so remove redundant code
> > from pmac_ide_init_hwif_ports().
> >
> > As side-effect this change fixes ctl_addr == 0 special handling in
> > ide_init_hwif_ports().
> >
> > * Fix misleading comment while at it.
>
> I would have to try it. Problem is, I don't actually have any powermac
> with a PCI IDE controller at hand.. ouch. I'll see what I can find.

Same problem here, would be great if somebody with PMAC could test
this patch (should apply cleanly to Linus' tree).

Well, since the change is pretty safe I'm adding the patch to IDE tree
(so it hopefully gets tested through -mm).

Thanks,
Bart

2007-10-24 12:44:33

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Mon, Oct 15, 2007 at 04:07:47PM -0400, Jeff Garzik wrote:
> Russell King wrote:
> >On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
> >>Al Viro wrote:
> >>>Proposed addition to icside part, provided that ARM folks ACK it - gets
> >>>icside to build and AFAICS it's correct:
> >>ACK from me too... ARM folks?
> >
> >Will test tomorrow/Wednesday.
>
> Thanks. Sorry about missing your CC on the libata-wide ata_link
> changes. We should have poked the maintainers on that, for the drivers
> we cannot build ourselves.

I think Viro's patch got an ack from everyone. Who's dealing with getting
this regression fix into mainline? It seems at the moment the patch is in
limbo.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-10-24 12:53:32

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

Russell King wrote:
> On Mon, Oct 15, 2007 at 04:07:47PM -0400, Jeff Garzik wrote:
>> Russell King wrote:
>>> On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
>>>> Al Viro wrote:
>>>>> Proposed addition to icside part, provided that ARM folks ACK it - gets
>>>>> icside to build and AFAICS it's correct:
>>>> ACK from me too... ARM folks?
>>> Will test tomorrow/Wednesday.
>> Thanks. Sorry about missing your CC on the libata-wide ata_link
>> changes. We should have poked the maintainers on that, for the drivers
>> we cannot build ourselves.
>
> I think Viro's patch got an ack from everyone. Who's dealing with getting
> this regression fix into mainline? It seems at the moment the patch is in
> limbo.

I was waiting on you, who last said "Will test tomorrow/Wednesday" (with
presumably an ACK forthcoming after test results)

If it now has your ACK, it's ready and waiting in my mbox queue to be
merged upstream.

Jeff

2007-10-24 13:18:16

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

On Wed, Oct 24, 2007 at 08:53:09AM -0400, Jeff Garzik wrote:
> Russell King wrote:
> >On Mon, Oct 15, 2007 at 04:07:47PM -0400, Jeff Garzik wrote:
> >>Russell King wrote:
> >>>On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
> >>>>Al Viro wrote:
> >>>>>Proposed addition to icside part, provided that ARM folks ACK it - gets
> >>>>>icside to build and AFAICS it's correct:
> >>>>ACK from me too... ARM folks?
> >>>Will test tomorrow/Wednesday.
> >>Thanks. Sorry about missing your CC on the libata-wide ata_link
> >>changes. We should have poked the maintainers on that, for the drivers
> >>we cannot build ourselves.
> >
> >I think Viro's patch got an ack from everyone. Who's dealing with getting
> >this regression fix into mainline? It seems at the moment the patch is in
> >limbo.
>
> I was waiting on you, who last said "Will test tomorrow/Wednesday" (with
> presumably an ACK forthcoming after test results)
>
> If it now has your ACK, it's ready and waiting in my mbox queue to be
> merged upstream.

Looks like it got my ack more than a week ago:

| Date: Tue, 16 Oct 2007 16:55:07 +0100
| From: Russell King <[email protected]>
| To: Al Viro <[email protected]>
| Cc: Bartlomiej Zolnierkiewicz <[email protected]>,
| Linus Torvalds <[email protected]>,
| [email protected], [email protected]
| Subject: Re: [git patches] IDE updates (part 2)
|
| On Sun, Oct 14, 2007 at 01:12:39AM +0100, Al Viro wrote:
| > Proposed addition to icside part, provided that ARM folks ACK it - gets
| > icside to build and AFAICS it's correct:
|
| Booted and tested as working.
|
| Whatever-tag-that-is: Russell King <[email protected]>

which I gave to the _initial_ message asking for my ack. And no, before
you whinge and moan, I didn't drop any CC's.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-10-24 13:20:36

[permalink] [raw]

Subject: Re: [git patches] IDE updates (part 2)

Russell King wrote:
> On Wed, Oct 24, 2007 at 08:53:09AM -0400, Jeff Garzik wrote:
>> Russell King wrote:
>>> On Mon, Oct 15, 2007 at 04:07:47PM -0400, Jeff Garzik wrote:
>>>> Russell King wrote:
>>>>> On Mon, Oct 15, 2007 at 03:54:16PM -0400, Jeff Garzik wrote:
>>>>>> Al Viro wrote:
>>>>>>> Proposed addition to icside part, provided that ARM folks ACK it - gets
>>>>>>> icside to build and AFAICS it's correct:
>>>>>> ACK from me too... ARM folks?
>>>>> Will test tomorrow/Wednesday.
>>>> Thanks. Sorry about missing your CC on the libata-wide ata_link
>>>> changes. We should have poked the maintainers on that, for the drivers
>>>> we cannot build ourselves.
>>> I think Viro's patch got an ack from everyone. Who's dealing with getting
>>> this regression fix into mainline? It seems at the moment the patch is in
>>> limbo.
>> I was waiting on you, who last said "Will test tomorrow/Wednesday" (with
>> presumably an ACK forthcoming after test results)
>>
>> If it now has your ACK, it's ready and waiting in my mbox queue to be
>> merged upstream.
>
> Looks like it got my ack more than a week ago:

Groovy.

Jeff

2007-10-25 06:06:07