Hello all,
I've got an ide, and an idepnp question... (for 2.4)
I'm working on refining a patch sent previously
(http://groups.google.com/groups?selm=20021108061020.A14168%40localhost) to
be less intrusive. I'll be refering to things done in that patch...
The short of it is, this sb16 pnpide interface apparently cannot use
ALTSTATUS at a certain point. (I'm no ide whiz, I'm just simplifying the
code that David Meybohm wrote, so maybe I'm off a bit) at any rate, this
seems to require a new flag be listed along with the hardware information.
His solution was to add
+ int no_passive; /* no passive status tests */
to hw_reg_s in ide.h and check that flag in drive_is_ready()
I *think* it's out of place. It seems to me it'd be more appropriate to add
+ unsigned no_passive : 1; /* no passive status tests */
to hwif_s in ide.h. Right next to a few other bitfields
Which is better? or is there a different, even better spot?
As for the idepnp part, he added a "dev = NULL" into the loop, and was
unsure of whether or not this was a good idea. I have the same question.
Or perhaps this smells of a seperate patch?
I'd rather ask these question in the form of my own patch, but... I'm a bit
short on time, atm. sorry.
Thanks in advace,
-Ted
Ted,
Somebody asked me to poke my nose in here, so here goes.
The difference in the two locations has to do with early initalization.
One the issues of concern in the patch, is the usage of "passive".
A stronger position for setup would have a hwif->intq_mode operator.
Regardless if it is a bit field or not.
This would force ide-probe to initialize the hwif_intr properly.
Next the mask of the field would provide a method for poking the
drive_is_ready().
This would remove several issue.
One the config option for share or not interrupts goes away.
The list is short and obvious.
Cheers,
Andre Hedrick
LAD Storage Consulting Group
On Wed, 11 Dec 2002, Ted Kaminski wrote:
> Hello all,
>
> I've got an ide, and an idepnp question... (for 2.4)
>
> I'm working on refining a patch sent previously
> (http://groups.google.com/groups?selm=20021108061020.A14168%40localhost) to
> be less intrusive. I'll be refering to things done in that patch...
>
> The short of it is, this sb16 pnpide interface apparently cannot use
> ALTSTATUS at a certain point. (I'm no ide whiz, I'm just simplifying the
> code that David Meybohm wrote, so maybe I'm off a bit) at any rate, this
> seems to require a new flag be listed along with the hardware information.
>
> His solution was to add
> + int no_passive; /* no passive status tests */
> to hw_reg_s in ide.h and check that flag in drive_is_ready()
>
> I *think* it's out of place. It seems to me it'd be more appropriate to add
> + unsigned no_passive : 1; /* no passive status tests */
> to hwif_s in ide.h. Right next to a few other bitfields
>
> Which is better? or is there a different, even better spot?
>
> As for the idepnp part, he added a "dev = NULL" into the loop, and was
> unsure of whether or not this was a good idea. I have the same question.
> Or perhaps this smells of a seperate patch?
>
> I'd rather ask these question in the form of my own patch, but... I'm a bit
> short on time, atm. sorry.
>
> Thanks in advace,
> -Ted
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hi,
On Thu, Dec 12, 2002 at 12:33:01AM -0800, Andre Hedrick wrote:
>
> The difference in the two locations has to do with early initalization.
> One the issues of concern in the patch, is the usage of "passive".
> A stronger position for setup would have a hwif->intq_mode operator.
> Regardless if it is a bit field or not.
Is this patch any better? This patch also fixes the SB16 pnpide, and
is against 2.4.21-pre2.
> This would force ide-probe to initialize the hwif_intr properly.
> Next the mask of the field would provide a method for poking the
> drive_is_ready().
i was wondering if it would be reliable to choose STATUS or ALTSTATUS
in drive_is_ready() based on which one actual_try_to_identify() uses
for probing. Is this what you mean by "initialize the hwif_intr
properly", or something else?
> One the config option for share or not interrupts goes away.
I didn't remove CONFIG_IDEPCI_SHARE_IRQ completely because some
architectures have if off by default. So I set the default poking
method based on CONFIG_IDEPCI_SHARE_IRQ.
Thanks,
David
drivers/ide/ide-iops.c | 4 +---
drivers/ide/ide-pnp.c | 34 ++++++++++++++++++++++++++++------
drivers/ide/ide-probe.c | 5 ++---
drivers/ide/ide.c | 3 +++
include/linux/ide.h | 1 +
5 files changed, 35 insertions, 12 deletions
--- v2.4.21-pre2/drivers/ide/ide.c~sb16fix-pre2 Fri Dec 20 16:08:37 2002
+++ v2.4.21-pre2-sb16fix-pre2/drivers/ide/ide.c Fri Dec 20 16:08:37 2002
@@ -240,6 +240,9 @@ static void init_hwif_data (unsigned int
if (hwif->io_ports[IDE_DATA_OFFSET] == HD_DATA)
hwif->noprobe = 1; /* may be overridden by ide_setup() */
#endif /* CONFIG_BLK_DEV_HD */
+#ifndef CONFIG_IDEPCI_SHARE_IRQ
+ hwif->no_altstat = 1;
+#endif /* !CONFIG_IDEPCI_SHARE_IRQ */
hwif->major = ide_hwif_to_major[index];
hwif->name[0] = 'i';
hwif->name[1] = 'd';
--- v2.4.21-pre2/drivers/ide/ide-iops.c~sb16fix-pre2 Fri Dec 20 16:08:37 2002
+++ v2.4.21-pre2-sb16fix-pre2/drivers/ide/ide-iops.c Fri Dec 20 16:08:37 2002
@@ -473,17 +473,15 @@ int drive_is_ready (ide_drive_t *drive)
udelay(1);
#endif
-#ifdef CONFIG_IDEPCI_SHARE_IRQ
/*
* We do a passive status test under shared PCI interrupts on
* cards that truly share the ATA side interrupt, but may also share
* an interrupt with another pci card/device. We make no assumptions
* about possible isa-pnp and pci-pnp issues yet.
*/
- if (IDE_CONTROL_REG)
+ if (IDE_CONTROL_REG && !hwif->no_altstat)
stat = hwif->INB(IDE_ALTSTATUS_REG);
else
-#endif /* CONFIG_IDEPCI_SHARE_IRQ */
/* Note: this may clear a pending IRQ!! */
stat = hwif->INB(IDE_STATUS_REG);
--- v2.4.21-pre2/drivers/ide/ide-pnp.c~sb16fix-pre2 Fri Dec 20 16:08:37 2002
+++ v2.4.21-pre2-sb16fix-pre2/drivers/ide/ide-pnp.c Fri Dec 20 16:08:37 2002
@@ -58,7 +58,7 @@ static int __init pnpide_generic_init(st
return 0;
if (!(DEV_IO(dev, 0) && DEV_IO(dev, 1) && DEV_IRQ(dev, 0)))
- return 1;
+ return -1;
ide_setup_ports(&hw, (ide_ioreg_t) DEV_IO(dev, 0),
generic_ide_offsets,
@@ -69,16 +69,38 @@ static int __init pnpide_generic_init(st
index = ide_register_hw(&hw, NULL);
- if (index != -1) {
+ if (index != -1)
printk(KERN_INFO "ide%d: %s IDE interface\n", index, DEV_NAME(dev));
+ return index;
+}
+
+
+/* Initialisation function for SB16 ISA PnP IDE interface */
+static int __init pnpide_sb16_init(struct pci_dev *dev, int enable)
+{
+ int index;
+
+ if (!enable)
return 0;
- }
- return 1;
-}
+ index = pnpide_generic_init(dev, enable);
+ if (index == -1)
+ return index;
+
+ /*
+ * The SB16 IDE controller doesn't like ALTSTATUS.
+ */
+ ide_hwifs[index].no_altstat = 1;
+ return index;
+}
+
/* Add your devices here :)) */
struct pnp_dev_t idepnp_devices[] __initdata = {
+ /* SB16 PnP IDE controller */
+ { ISAPNP_ANY_ID, ISAPNP_ANY_ID,
+ ISAPNP_VENDOR('C', 'T', 'L'), ISAPNP_DEVICE(0x2011),
+ pnpide_sb16_init },
/* Generic ESDI/IDE/ATA compatible hard disk controller */
{ ISAPNP_ANY_ID, ISAPNP_ANY_ID,
ISAPNP_VENDOR('P', 'N', 'P'), ISAPNP_DEVICE(0x0600),
@@ -136,7 +158,7 @@ void __init pnpide_init(int enable)
}
/* Call device initialization function */
- if (dev_type->init_fn(dev, 1)) {
+ if (dev_type->init_fn(dev, 1) == -1) {
if (dev->deactivate(dev))
dev->deactivate(dev);
} else {
--- v2.4.21-pre2/drivers/ide/ide-probe.c~sb16fix-pre2 Fri Dec 20 16:08:37 2002
+++ v2.4.21-pre2-sb16fix-pre2/drivers/ide/ide-probe.c Fri Dec 20 16:08:37 2002
@@ -884,9 +884,8 @@ int init_irq (ide_hwif_t *hwif)
if (IDE_CHIPSET_IS_PCI(hwif->chipset)) {
sa = SA_SHIRQ;
-#ifndef CONFIG_IDEPCI_SHARE_IRQ
- sa |= SA_INTERRUPT;
-#endif /* CONFIG_IDEPCI_SHARE_IRQ */
+ if (hwif->no_altstat)
+ sa |= SA_INTERRUPT;
}
if (hwif->io_ports[IDE_CONTROL_OFFSET])
--- v2.4.21-pre2/include/linux/ide.h~sb16fix-pre2 Fri Dec 20 16:08:37 2002
+++ v2.4.21-pre2-sb16fix-pre2/include/linux/ide.h Fri Dec 20 16:14:36 2002
@@ -1009,6 +1009,7 @@ typedef struct hwif_s {
unsigned udma_four : 1; /* 1=ATA-66 capable, 0=default */
unsigned highmem : 1; /* can do full 32-bit dma */
unsigned no_dsc : 1; /* 0 default, 1 dsc_overlap disabled */
+ unsigned no_altstat : 1; /* 0 default, 1 don't use ALTSTATUS */
void *hwif_data; /* extra hwif data */
} ide_hwif_t;