Re: [PATCH 2/2 v3] sata_mv: Support SoC controllers
On Fri, Feb 01, 2008 at 11:53:30AM -0500, Jeff Garzik wrote:
[EMAIL PROTECTED] wrote:
--- /dev/null
+++ b/include/linux/sata_mv.h
@@ -0,0 +1,21 @@
+/*
+ * Marvell integrated SATA platfrom device data definition file.
+ *
+ * Saeed Bishara [EMAIL PROTECTED]
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_SATA_MV_H__
+#define __LINUX_SATA_MV_H__
+
+/*
+ * Sata private data
+ */
+struct mv_sata_platform_data {
+int n_ports; /* number of sata ports */
+};
+
+#endif
Overall, the patch is OK, but I fear adding way too many of these tiny
includes, for each platform.
Unless Paul M objects (pata_platform maintainer), I will rename
linux/pata_platform.h to linux/ata_platform.h, and we can put your
mv_sata_platform_data structure in there.
No objections from me, feel free to make the best use you can out of
what's there.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Thu, Dec 20, 2007 at 09:53:54PM +, Adrian McMenamin wrote: On 16/12/2007, Paul Mundt [EMAIL PROTECTED] wrote: Also, __devinit/__devexit annotations? Is there any difference between __init and __devint? Yes. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
Ok, I don't know anything about the CD-ROM layer, so I've just commented
on the general and SH-specific stuff. Hopefully someone with a clue
in this area (ie, not me) can offer input on the rest of the bits.
On Sun, Dec 16, 2007 at 12:21:21AM +, Adrian McMenamin wrote:
+/* GD Rom registers */
+#define GDROM_BASE_REG 0xA05F7000
+#define GDROM_ALTSTATUS_REG GDROM_BASE_REG + 0x18
+#define GDROM_DATA_REG GDROM_BASE_REG + 0x80
+#define GDROM_ERROR_REG GDROM_BASE_REG + 0x84
+#define GDROM_INTSEC_REG GDROM_BASE_REG + 0x88
+#define GDROM_SECNUM_REG GDROM_BASE_REG + 0x8C
+#define GDROM_BCL_REG GDROM_BASE_REG + 0x90
+#define GDROM_BCH_REG GDROM_BASE_REG + 0x94
+#define GDROM_DSEL_REG GDROM_BASE_REG + 0x98
+#define GDROM_STATUSCOMMAND_REG GDROM_BASE_REG + 0x9C
+#define GDROM_RESET_REG GDROM_BASE_REG + 0x4E4
+
+#define GDROM_DATA_REG_P0 0x005F7080
+
+#define GDROM_DMA_STARTADDR_REG GDROM_BASE_REG + 0x404
+#define GDROM_DMA_LENGTH_REG GDROM_BASE_REG + 0x408
+#define GDROM_DMA_DIRECTION_REG GDROM_BASE_REG + 0x40C
+#define GDROM_DMA_ENABLE_REG GDROM_BASE_REG + 0x414
+#define GDROM_DMA_STATUS_REG GDROM_BASE_REG + 0x418
+#define GDROM_DMA_WAIT_REG GDROM_BASE_REG + 0x4A0
+#define GDROM_DMA_ACCESS_CTRL_REG GDROM_BASE_REG + 0x4B8
+
These should all be encapsulated by brackets.
+static void wait_clrbusy(void)
+{
+ while (ctrl_inb(GDROM_ALTSTATUS_REG) 0x80)
+ schedule();
+}
+
+static void gdrom_wait_busy_sleeps(void)
+{
+ /* Wait to get busy first */
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x80) == 0)
+ schedule();
+ /* Now wait for busy to clear */
+ wait_clrbusy();
+}
+
Are you sure you can tolerate a schedule() in here, as opposed to a
cpu_relax()? If you're going to schedule away whilst polling a bit, you
may as well just use a wait queue and wait_on_bit() or so. This seems
like a lot of extra latency for this though.
+static void gdrom_spicommand(void *spi_string, int buflen)
+{
+ short *cmd = spi_string;
[snip]
+ wait_clrbusy();
+ ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
+ while ((ctrl_inb(GDROM_ALTSTATUS_REG) 0x88) != 8)
+ ; /* wait for DRQ to be set to 1 */
cpu_relax()
+static char gdrom_execute_diagnostic(void)
+{
+ int count;
+ /* Restart the GDROM */
+ ctrl_outl(0x1f, GDROM_RESET_REG);
+ for (count = 0xa000; count 0xa020; count += 4)
+ ctrl_inl(count);
Er? This ranged dummy reading of the P2 space needs some explanation. The
GD-ROM isn't even mapped in to this space, so this seems like a hack to
either work around a timing issue or a write ordering problem.
+static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
struct cdrom_multisession *ms_info)
+{
[snip]
+ }
+ }
+ else
Questionable placement of else.
+ printk(KERN_DEBUG Disk is GDROM\n);
+ if (gd.toc)
+ kfree(gd.toc);
Useless if.
+ gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
+ if (!gd.toc) {
+ err = -ENOMEM;
+ goto clean_tocB;
+ }
+ if (tocuse)
Broken indendation.
+static int gdrom_open(struct cdrom_device_info *cd_info, int purpose)
+{
+ int err;
+ /* spin up the disk */
+ err = gdrom_preparedisk_cmd();
+ if (err)
+ return -EIO;
+
+ return 0;
Perhaps gdrom_preparedisk_cmd() should just hand back -EIO in the error
case and you can just pass that on directly. If you have no other users
of it, then just move the work in to the gdrom_open() directly.
+static void gdrom_release(struct cdrom_device_info *cd_info)
+{
+}
+
Do you really need this? If this is some sort of driver model damage, a
comment to that extent would be helpful, otherwise just kill this off.
+static int gdrom_mediachanged(struct cdrom_device_info *cd_info, int ignore)
+{
+ /* check the sense key */
+ char sense = ctrl_inb(GDROM_ERROR_REG);
+ if ((sense 0xF0) == 0x60)
+ return 1;
+ return 0;
+}
+
Just return (ctrl_inb(GDROM_ERROR_REG) 0xf0) == 0x60 ?
+static int gdrom_hardreset(struct cdrom_device_info * cd_info)
+{
+ int count;
+ ctrl_outl(0x1f, GDROM_RESET_REG);
+ for (count = 0xa000; count 0xa020; count += 4)
+ ctrl_inl(count);
+ return 0;
+}
+
More strange P2 abuse. If this is the officially recommended reset
method in the GD-ROM errat^H^H^H^Hspecification, it paints a pretty good
picture of its commercial success.
+ if (bufstring){
+ memcpy(bufstring, sense[4], 2); /* return additional sense
data */
+ }
+
Useless braces.
+ if (sense_key 2)
+ return 0;
+ return -EIO;
+}
+
+static struct cdrom_device_ops gdrom_ops = {
+ .open = gdrom_open,
+ .release= gdrom_release,
+ .drive_status =
Re: [PATCH 1/3] [libata] pata_platform: make probe and remove functions device type neutral
On Fri, Dec 14, 2007 at 09:24:29PM +0300, Anton Vorontsov wrote:
Split pata_platform_{probe,remove} into two pieces:
1. pata_platform_{probe,remove} -- platform_device-dependant bits;
2. __ptata_platform_{probe,remove} -- device type neutral bits.
This is done to not duplicate code for the OF-platform driver.
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
Acked-by: Olof Johansson [EMAIL PROTECTED]
Looks fine to me now, thanks for cleaning it up Anton.
Acked-by: Paul Mundt [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 05:32:51PM +, Adrian McMenamin wrote: On Sun, 2007-12-16 at 18:50 +0900, Paul Mundt wrote: + for (count = 0xa000; count 0xa020; count += 4) + ctrl_inl(count); Er? This ranged dummy reading of the P2 space needs some explanation. The GD-ROM isn't even mapped in to this space, so this seems like a hack to either work around a timing issue or a write ordering problem. I'll confess to not knowing what it's up to here either, other than it looks like a mechanism to cause a G1 bus reset. This is a progressive read of the Boot ROM area and that is all under G1 control (as is the GD Rom obviously) Ok, then this should be moved out to a g1_bus_reset() or something similar with a comment explaining what it's doing, that way it can be trivially reworked if a saner method of forcing a G1 reset is discovered. While I realize that this is all undocumented and based entirely on reverse engineering, you should at least verify that that's precisely what is going on, and that this is not just a precaution for flushing posted writes. You can test that by removing the loop and doing a dummy read after your write (to the same register, rather than the entire ROM space). If it's a posting issue, then you will have to do your own read/write_reg routines that handle the flush. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Add GD-Rom support to the SEGA Dreamcast
On Sun, Dec 16, 2007 at 12:20:54AM +, Adrian McMenamin wrote: This device is electrically compatible with ATA-3 IDE CD drives but implements a proprietary packet interface. There have been previous Dreamcast CD drivers around but this is new code and uses DMA as opposed to PIO for reads. It also supports reading the proprietary GD Rom format disks. The driver will live as drivers/sh/gdrom/gdrom.c No, the driver will live in drivers/cdrom if that's the API that you depend on. The only things in drivers/sh are SH-specific bus support code, it is not a dumping ground for unrelated drivers. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] [libata] pata_of_platform: OF-Platform PATA device driver
On Tue, Dec 04, 2007 at 02:01:21PM -0600, Olof Johansson wrote: On Tue, Dec 04, 2007 at 10:49:21PM +0300, Anton Vorontsov wrote: tristate Generic platform device PATA support - depends on EMBEDDED || ARCH_RPC + depends on EMBEDDED || ARCH_PPC It needs to be || PPC, not || ARCH_PPC. Wrong. It needs to be EMBEDDED || ARCH_RPC || PPC. ARCH_RPC is not a typo, it's an ARM platform. Please grep first :-) - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] [libata] pata_platform: s/ioport_shift/reg_shift/g
On Tue, Dec 04, 2007 at 08:07:45PM +0300, Anton Vorontsov wrote: This patch renames ioport_shift member of pata_platform_info structure to reg_shift. Users of pata_platform are followed appropriately. Rationale of that change is: shifting applies to the whole memory mapped region, not only to the command block of the ATA registers, despite the fact that shifting is meaningless for ctl register. It's called ioport_shift because of the fact it shifts an ioport, namely a struct ata_ioport *. We could rename it, but I really don't see the point. If you don't like the choice of name on your platform, add a comment to your platform-specific code noting this particular outrage so it can be grepped for by future generations ;-) Nacked-by: Paul Mundt [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] OF-platform PATA driver
On Tue, Nov 27, 2007 at 06:37:08PM +0300, Anton Vorontsov wrote: Here is the second spin of the OF-platform PATA driver and related patches. So either the patches are missing, or I wasn't CC'ed on them. I'm going to go out on a limb and assume the latter. If you wish me to Ack them, I'm not going to start grovelling around list archives trying to find the relevant posts. This is now the second time this has happened, the first time I was only made aware of this work as Jeff forwarded them along. CC'ing the authors of code you are changing shouldn't be a cryptic requirement we have to spell out in SubmittingPatches or so, especially if you're soliciting Acked-bys. Please fix your development methodology, thanks. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 0/3] OF-platform PATA driver
On Mon, Nov 26, 2007 at 03:23:14AM +0300, Anton Vorontsov wrote: On Sat, Nov 24, 2007 at 04:26:13PM +0900, Paul Mundt wrote: On Fri, Nov 23, 2007 at 07:49:33PM -0500, Jeff Garzik wrote: Anton Vorontsov wrote: Here is the PATA Platform driver using OF infrastructure. Mostly it's just a wrapper around a bit modified pata_platform driver. Patches are well split for the easier review: First one factors out platform_device specific bits and modifies pata_platform to be a library-alike driver (with platform_device default binding). The only issue I have here is that this library-like version has subtle semantic changes that will break existing drivers. Actually I've tried to keep semantics intact: +static int __devinit pata_platform_probe(struct platform_device *pdev) [...] + /* +* And the IRQ +*/ + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (irq_res) + irq_res-flags = pp_info ? pp_info-irq_flags : 0; [...] So, I'm passing flags from the platform data. Did you overlook these bits, or I'm still changing semantics somewhere else? Oh, I overlooked that. Using irq_res-flags as a temporary for pp_info-irq_flags seems a bit hacky, but as long as pp_info-irq_flags semantics are intact, I'm not too violently opposed to this anyways. Incidentally, we already have an include/linux/pata_platform.h. If this is going to be library-like, through the prototypes in there, rather than splitting them up betewen include/linux and drivers/ata. We don't need two headers. My intention was: keep private declarations in the drivers/ata/ and public in the include/linux/ -- to not confuse pata_platform users by __pata_platform_* internal stuff. But okay, I'll merge them. I suppose that depends on whether the intent is that all pata_platform users will be stuck in drivers/ata or not. If this is treated as more of a library, implementations can simply bury themselves in arch/ land if they feel like it. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 0/3] OF-platform PATA driver
On Fri, Nov 23, 2007 at 07:49:33PM -0500, Jeff Garzik wrote: Anton Vorontsov wrote: Here is the PATA Platform driver using OF infrastructure. Mostly it's just a wrapper around a bit modified pata_platform driver. Patches are well split for the easier review: First one factors out platform_device specific bits and modifies pata_platform to be a library-alike driver (with platform_device default binding). The only issue I have here is that this library-like version has subtle semantic changes that will break existing drivers. irq_flags exists in struct pata_platform_info precisely for the IRQ resource IRQ flags (as opposed to the IORESOURCE flags, which are what the IRQ resource flags refer to instead). This change takes that for granted and just assumes we're going to be using the res-flags, which is both an invalid assumption, and will utterly break blackfin and others that depend on it. Incidentally, we already have an include/linux/pata_platform.h. If this is going to be library-like, through the prototypes in there, rather than splitting them up betewen include/linux and drivers/ata. We don't need two headers. These patches basically look fine to me otherwise, though it would be nice if the semantic-changing bits had been abstracted. So as long as the old irq_flags behaviour is maintained and that irq_res-flags stuff is ripped out, I'll add my Acked-by as well. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] libata: Support PIO polling-only hosts.
By default ata_host_activate() expects a valid IRQ in order to successfully register the host. This patch enables a special case for registering polling-only hosts that either don't have IRQs or have buggy IRQ generation (either in terms of handling or sensing), which otherwise work fine. Hosts that want to use polling mode can simply set ATA_FLAG_PIO_POLLING and pass in a NULL IRQ handler or invalid ( 0) IRQ. Signed-off-by: Paul Mundt [EMAIL PROTECTED] --- drivers/ata/libata-core.c |7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index ec3ce12..a0cd6bb 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -7178,6 +7178,9 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht) * request IRQ and register it. This helper takes necessasry * arguments and performs the three steps in one go. * + * A NULL @irq_handler or invalid IRQ skips the IRQ registration + * and expects the host to have set polling mode on the port. + * * LOCKING: * Inherited from calling layer (may sleep). * @@ -7194,6 +7197,10 @@ int ata_host_activate(struct ata_host *host, int irq, if (rc) return rc; + /* Special case for polling mode */ + if (!irq_handler || irq 0) + return ata_host_register(host, sht); + rc = devm_request_irq(host-dev, irq, irq_handler, irq_flags, dev_driver_string(host-dev), host); if (rc) - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] libata: pata_platform: Support polling-mode configuration.
Some SH boards (old R2D-1 boards) have generally not had working CF
under libata, due to both buswidth issues (handled by Aoi Shinkai
in 43f4b8c7578b928892b6f01d374346ae14e5eb70), and buggy interrupt
controllers. For these sorts of boards simply disabling the IRQ and
polling ends up working fine.
This conditionalizes the IRQ resource for pata_platform and lets
platforms that want to use polling mode simply omit the resource
entirely.
Signed-off-by: Paul Mundt [EMAIL PROTECTED]
---
drivers/ata/pata_platform.c | 33 ++---
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index fc72a96..6b2d731 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -1,7 +1,7 @@
/*
* Generic platform device PATA driver
*
- * Copyright (C) 2006 Paul Mundt
+ * Copyright (C) 2006 - 2007 Paul Mundt
*
* Based on pata_pcmcia:
*
@@ -22,7 +22,7 @@
#include linux/pata_platform.h
#define DRV_NAME pata_platform
-#define DRV_VERSION 1.1
+#define DRV_VERSION 1.2
static int pio_mask = 1;
@@ -120,15 +120,20 @@ static void pata_platform_setup_port(struct ata_ioports
*ioaddr,
* Register a platform bus IDE interface. Such interfaces are PIO and we
* assume do not support IRQ sharing.
*
- * Platform devices are expected to contain 3 resources per port:
+ * Platform devices are expected to contain at least 2 resources per port:
*
* - I/O Base (IORESOURCE_IO or IORESOURCE_MEM)
* - CTL Base (IORESOURCE_IO or IORESOURCE_MEM)
+ *
+ * and optionally:
+ *
* - IRQ (IORESOURCE_IRQ)
*
* If the base resources are both mem types, the ioremap() is handled
* here. For IORESOURCE_IO, it's assumed that there's no remapping
* necessary.
+ *
+ * If no IRQ resource is present, PIO polling mode is used instead.
*/
static int __devinit pata_platform_probe(struct platform_device *pdev)
{
@@ -137,11 +142,12 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
struct ata_port *ap;
struct pata_platform_info *pp_info;
unsigned int mmio;
+ int irq;
/*
* Simple resource validation ..
*/
- if (unlikely(pdev-num_resources != 3)) {
+ if ((pdev-num_resources != 3) (pdev-num_resources != 2)) {
dev_err(pdev-dev, invalid number of resources\n);
return -EINVAL;
}
@@ -173,6 +179,11 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
(ctl_res-flags == IORESOURCE_MEM));
/*
+* And the IRQ
+*/
+ irq = platform_get_irq(pdev, 0);
+
+ /*
* Now that that's out of the way, wire up the port..
*/
host = ata_host_alloc(pdev-dev, 1);
@@ -185,6 +196,14 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
ap-flags |= ATA_FLAG_SLAVE_POSS;
/*
+* Use polling mode if there's no IRQ
+*/
+ if (irq 0) {
+ ap-flags |= ATA_FLAG_PIO_POLLING;
+ ata_port_desc(ap, no IRQ, using PIO polling);
+ }
+
+ /*
* Handle the MMIO case
*/
if (mmio) {
@@ -213,9 +232,9 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
(unsigned long long)ctl_res-start);
/* activate */
- return ata_host_activate(host, platform_get_irq(pdev, 0),
-ata_interrupt, pp_info ? pp_info-irq_flags
-: 0, pata_platform_sht);
+ return ata_host_activate(host, irq, ata_interrupt,
+pp_info ? pp_info-irq_flags : 0,
+pata_platform_sht);
}
/**
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] libata: Support PIO polling-only hosts.
On Wed, Nov 07, 2007 at 09:09:30AM -0500, Mark Lord wrote: Paul Mundt wrote: On Wed, Nov 07, 2007 at 01:09:40PM +, Alan Cox wrote: On Wed, 7 Nov 2007 17:10:52 +0900 Paul Mundt [EMAIL PROTECTED] wrote: By default ata_host_activate() expects a valid IRQ in order to successfully register the host. This patch enables a special case for registering polling-only hosts that either don't have IRQs or have buggy IRQ generation (either in terms of handling or sensing), which otherwise work fine. Hosts that want to use polling mode can simply set ATA_FLAG_PIO_POLLING and pass in a NULL IRQ handler or invalid ( 0) IRQ. NAK Zero is no IRQ, please use that for polling not 0 However, platform_get_irq() will happily return IRQ#0, and it's a valid vector on plenty of machines. NO_IRQ is also 0 on at least FR-V, ARM, blackin, PA-RISC, some PowerPC, and even IDE. Too bad. The Penultimate Penguin wants zero to continue to mean no IRQ. Dig into the archives for multiple threads on this exact topic. The end result is that 0 means no IRQ. If your physical IRQ actually is the number 0, then reencode it to some other value for this purpose. I've read the threads, but this does little to do with the fact it's still a perfectly valid vector, and I'm not about to force every IRQ vector on my platform off-by-1 in order to satisfy a religious point of view with zero reflection on what the hardware actually looks like. So I'll change the check to IRQ#0 == invalid, but if that's to be enforced kernel-wide, then all of the existing NO_IRQ cases should be ripped out and set to 0. This way at least people are getting screwed consistently, rather than just in particular subsystems. Yes, a bit of pain, but that's how many parts of the kernel expect it, Just as many parts of the kernel make no such assumption. and in the end it's no more overall hassle than doing it differently might have been. Spoken like someone who doesn't have to contend with off-by-1 IRQ vectors as a result of an entirely cosmetic change. It's certainly easier to parrot a party line when you aren't being bitten by it. So again, I'll make the change, but it's utter nonsense. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] libata: Support PIO polling-only hosts.
On Wed, Nov 07, 2007 at 01:09:40PM +, Alan Cox wrote: On Wed, 7 Nov 2007 17:10:52 +0900 Paul Mundt [EMAIL PROTECTED] wrote: By default ata_host_activate() expects a valid IRQ in order to successfully register the host. This patch enables a special case for registering polling-only hosts that either don't have IRQs or have buggy IRQ generation (either in terms of handling or sensing), which otherwise work fine. Hosts that want to use polling mode can simply set ATA_FLAG_PIO_POLLING and pass in a NULL IRQ handler or invalid ( 0) IRQ. NAK Zero is no IRQ, please use that for polling not 0 However, platform_get_irq() will happily return IRQ#0, and it's a valid vector on plenty of machines. NO_IRQ is also 0 on at least FR-V, ARM, blackin, PA-RISC, some PowerPC, and even IDE. We do have some devices that are physically on IRQ#0 that otherwise work fine, they aren't ATA devices mind you, but to claim that IRQ#0 isn't a valid vector is not in line with what hardware actually does, whether it's a good idea or not. In our case the IRQ vector maps to an exception offset, which we bump down to zero. We could force an off-by-1 there so that the math that indexes IRQ#0 is bumped up one, but that entails fixing up every one of our IRQ numbers for no obvious gain. I don't really see any value in purposely crippling the range of allowable vectors for these machines. Though I don't mind switching to a NO_IRQ comparison instead of the 0 case, so both can be handled. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2 take #2] libata: pata_platform: Support polling-mode configuration.
Some SH boards (old R2D-1 boards) have generally not had working CF
under libata, due to both buswidth issues (handled by Aoi Shinkai
in 43f4b8c7578b928892b6f01d374346ae14e5eb70), and buggy interrupt
controllers. For these sorts of boards simply disabling the IRQ and
polling ends up working fine.
This conditionalizes the IRQ resource for pata_platform and lets
platforms that want to use polling mode simply omit the resource
entirely.
Signed-off-by: Paul Mundt [EMAIL PROTECTED]
---
drivers/ata/pata_platform.c | 35 ---
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index fc72a96..ac03a90 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -1,7 +1,7 @@
/*
* Generic platform device PATA driver
*
- * Copyright (C) 2006 Paul Mundt
+ * Copyright (C) 2006 - 2007 Paul Mundt
*
* Based on pata_pcmcia:
*
@@ -22,7 +22,7 @@
#include linux/pata_platform.h
#define DRV_NAME pata_platform
-#define DRV_VERSION 1.1
+#define DRV_VERSION 1.2
static int pio_mask = 1;
@@ -120,15 +120,20 @@ static void pata_platform_setup_port(struct ata_ioports
*ioaddr,
* Register a platform bus IDE interface. Such interfaces are PIO and we
* assume do not support IRQ sharing.
*
- * Platform devices are expected to contain 3 resources per port:
+ * Platform devices are expected to contain at least 2 resources per port:
*
* - I/O Base (IORESOURCE_IO or IORESOURCE_MEM)
* - CTL Base (IORESOURCE_IO or IORESOURCE_MEM)
+ *
+ * and optionally:
+ *
* - IRQ (IORESOURCE_IRQ)
*
* If the base resources are both mem types, the ioremap() is handled
* here. For IORESOURCE_IO, it's assumed that there's no remapping
* necessary.
+ *
+ * If no IRQ resource is present, PIO polling mode is used instead.
*/
static int __devinit pata_platform_probe(struct platform_device *pdev)
{
@@ -137,11 +142,12 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
struct ata_port *ap;
struct pata_platform_info *pp_info;
unsigned int mmio;
+ int irq;
/*
* Simple resource validation ..
*/
- if (unlikely(pdev-num_resources != 3)) {
+ if ((pdev-num_resources != 3) (pdev-num_resources != 2)) {
dev_err(pdev-dev, invalid number of resources\n);
return -EINVAL;
}
@@ -173,6 +179,13 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
(ctl_res-flags == IORESOURCE_MEM));
/*
+* And the IRQ
+*/
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0)
+ irq = 0;/* no irq */
+
+ /*
* Now that that's out of the way, wire up the port..
*/
host = ata_host_alloc(pdev-dev, 1);
@@ -185,6 +198,14 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
ap-flags |= ATA_FLAG_SLAVE_POSS;
/*
+* Use polling mode if there's no IRQ
+*/
+ if (!irq) {
+ ap-flags |= ATA_FLAG_PIO_POLLING;
+ ata_port_desc(ap, no IRQ, using PIO polling);
+ }
+
+ /*
* Handle the MMIO case
*/
if (mmio) {
@@ -213,9 +234,9 @@ static int __devinit pata_platform_probe(struct
platform_device *pdev)
(unsigned long long)ctl_res-start);
/* activate */
- return ata_host_activate(host, platform_get_irq(pdev, 0),
-ata_interrupt, pp_info ? pp_info-irq_flags
-: 0, pata_platform_sht);
+ return ata_host_activate(host, irq, irq ? ata_interrupt : NULL,
+pp_info ? pp_info-irq_flags : 0,
+pata_platform_sht);
}
/**
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2 take #2] libata: Support PIO polling-only hosts.
By default ata_host_activate() expects a valid IRQ in order to
successfully register the host. This patch enables a special case
for registering polling-only hosts that either don't have IRQs
or have buggy IRQ generation (either in terms of handling or
sensing), which otherwise work fine.
Hosts that want to use polling mode can simply set ATA_FLAG_PIO_POLLING
and pass in an invalid IRQ.
Signed-off-by: Paul Mundt [EMAIL PROTECTED]
---
drivers/ata/libata-core.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ec3ce12..89fd0e9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -7178,6 +7178,10 @@ int ata_host_register(struct ata_host *host, struct
scsi_host_template *sht)
* request IRQ and register it. This helper takes necessasry
* arguments and performs the three steps in one go.
*
+ * An invalid IRQ skips the IRQ registration and expects the host to
+ * have set polling mode on the port. In this case, @irq_handler
+ * should be NULL.
+ *
* LOCKING:
* Inherited from calling layer (may sleep).
*
@@ -7194,6 +7198,12 @@ int ata_host_activate(struct ata_host *host, int irq,
if (rc)
return rc;
+ /* Special case for polling mode */
+ if (!irq) {
+ WARN_ON(irq_handler);
+ return ata_host_register(host, sht);
+ }
+
rc = devm_request_irq(host-dev, irq, irq_handler, irq_flags,
dev_driver_string(host-dev), host);
if (rc)
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pata_platform: Fix NULL pointer dereference
On Tue, Jul 17, 2007 at 06:24:53AM -0400, Jeff Garzik wrote: It's just reality that there are never enough people to verify every patch before it goes in. We just support way too much hardware for that to be remotely feasible. Sure we -try- to keep the driver maintainer CC'd, but alas we are human. Try? I've never _once_ received a CC from you regarding a change to pata_platform. The only time I've been CC'ed at all is when someone submitting the patch had the common courtesy to do so. Whereas the number of times I've had to clean up after something merged behind my back has been higher than the number of times I've been CC'ed on any of these changes. Your thinking is flawed if you think I'm going to hold up every patch until it's verified on each of 63 ATA controller drivers, when I make a core change, for example. Not scalable. We scale in another way: This has nothing to do with core changes that impact every driver, this has to do with isolated changes to a _single_ driver that you're obviously not in a position to test. That's a very different situation. Again, if you can't be bothered to CC people on changes to their drivers that aren't libata-wide, don't apply them in the first place. I would much rather play API catch up with my driver than have to backtrack and figure out what went wrong when suddenly all of my boards stop booting. Your merge first and hope someone else notices and fixes it later approach is insane. We've never done kernel development like that, libata isn't special. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ata: pata_platform: Disable prereset logic.
On a number of boards the current prereset logic seems to misbehave:
scsi0 : pata_platform
ata1: PATA max PIO0 cmd 0xb06001f0 ctl 0xb06003f6 bmdma 0x irq 0
ata1: device not ready (errno=-19), forcing hardreset
ata1: BUG: prereset() requested invalid reset type
This triggers when there is no card inserted in the slot.
Simply disabling the prereset gets rid of this, and doesn't seem to cause
any problems for either PCMCIA or CF cards when they're actually present.
Signed-off-by: Paul Mundt [EMAIL PROTECTED]
--
drivers/ata/pata_platform.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index cbb7866..8a569c7 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -1,7 +1,7 @@
/*
* Generic platform device PATA driver
*
- * Copyright (C) 2006 Paul Mundt
+ * Copyright (C) 2006 - 2007 Paul Mundt
*
* Based on pata_pcmcia:
*
@@ -22,7 +22,7 @@
#include linux/pata_platform.h
#define DRV_NAME pata_platform
-#define DRV_VERSION 1.0
+#define DRV_VERSION 1.1
static int pio_mask = 1;
@@ -30,7 +30,8 @@ static int pio_mask = 1;
* Provide our own set_mode() as we don't want to change anything that has
* already been configured..
*/
-static int pata_platform_set_mode(struct ata_port *ap, struct ata_device
**unused)
+static int pata_platform_set_mode(struct ata_port *ap,
+ struct ata_device **unused)
{
int i;
@@ -48,6 +49,12 @@ static int pata_platform_set_mode(struct ata_port *ap,
struct ata_device **unuse
return 0;
}
+static void pata_platform_error_handler(struct ata_port *ap)
+{
+ ata_bmdma_drive_eh(ap, NULL, ata_std_softreset, NULL,
+ ata_std_postreset);
+}
+
static int ata_dummy_ret0(struct ata_port *ap) { return 0; }
static struct scsi_host_template pata_platform_sht = {
@@ -80,7 +87,7 @@ static struct ata_port_operations pata_platform_port_ops = {
.freeze = ata_bmdma_freeze,
.thaw = ata_bmdma_thaw,
- .error_handler = ata_bmdma_error_handler,
+ .error_handler = pata_platform_error_handler,
.post_internal_cmd = ata_bmdma_post_internal_cmd,
.cable_detect = ata_cable_unknown,
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ata: pata_platform: Disable prereset logic.
On Wed, May 23, 2007 at 10:07:08AM +0200, Tejun Heo wrote: Paul Mundt wrote: On a number of boards the current prereset logic seems to misbehave: scsi0 : pata_platform ata1: PATA max PIO0 cmd 0xb06001f0 ctl 0xb06003f6 bmdma 0x irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: BUG: prereset() requested invalid reset type This triggers when there is no card inserted in the slot. Simply disabling the prereset gets rid of this, and doesn't seem to cause any problems for either PCMCIA or CF cards when they're actually present. NACK. The BUG printking needs fixing but you can't just kill prereset(). Did it work properly on 2.6.21.1? Can you modify ata_wait_ready() such that it prints out the status value while waiting? ata_wait_ready() works fine, it reports 0xff immediately (and this is what I would expect when there is no card inserted). The board that exhibits this behaviour wasn't supported in the older kernels, I can backport and test if it will be useful, though. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ata: pata_platform: Disable prereset logic.
On Wed, May 23, 2007 at 11:29:37AM +0200, Tejun Heo wrote: Paul Mundt wrote: On Wed, May 23, 2007 at 10:07:08AM +0200, Tejun Heo wrote: Paul Mundt wrote: On a number of boards the current prereset logic seems to misbehave: scsi0 : pata_platform ata1: PATA max PIO0 cmd 0xb06001f0 ctl 0xb06003f6 bmdma 0x irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: BUG: prereset() requested invalid reset type This triggers when there is no card inserted in the slot. ata_wait_ready() works fine, it reports 0xff immediately (and this is what I would expect when there is no card inserted). Does the attached patch fix your problem? Yes, that works fine, thanks. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: implement ata_wait_after_reset()
On Sat, May 19, 2007 at 09:04:56PM +0200, Tejun Heo wrote: Tejun Heo wrote: Yeah, if SCR registers are accessible, 0xff doesn't indicate the device isn't there, so the whole skip-0xff logic probably shouldn't apply in such cases, but we can also achieve pretty good result by just making the first reset tries a bit more aggressive. So, here's the patch. Paul, can you please test this patch without the previous patch? Indan, this should reduce the resume delay. Please test. But you'll still feel some added delay compared to 2.6.20 due to the mentioned suspend/resume change. Seems to work ok: [0.977254] scsi0 : sata_sil [0.980243] scsi1 : sata_sil [0.983207] ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 [0.991183] ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 [2.578436] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) [2.586828] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.591596] ata1.00: ATA-5: HHD424020F7SV00, 00MLA0A5, max UDMA/100 [2.598094] ata1.00: 39070080 sectors, multi 0: LBA [2.603248] ata1.00: applying bridge limits [2.614710] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.619489] ata1.00: configured for UDMA/100 [2.933096] ata2: SATA link down (SStatus 0 SControl 310) [2.936265] scsi 0:0:0:0: Direct-Access ATA HHD424020F7SV00 00ML PQ: 0 ANSI: 5 [2.945002] sd 0:0:0:0: [sda] 39070080 512-byte hardware sectors (20004 MB) [2.951473] sd 0:0:0:0: [sda] Write Protect is off - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: implement ata_wait_after_reset()
On Wed, May 16, 2007 at 06:44:53PM +0200, Tejun Heo wrote: This patch is against the current libata-dev#upstream + pata_scc-fix-build-failure[1]. [1] http://article.gmane.org/gmane.linux.kernel/528405 Paul, please verify this fixes your problem. You can skip the pata_scc patch, it will cause pata_scc part to be rejected but doesn't matter. Yes, this does get iVDR detection working again. The only problem seems to be that every now and then I end up with this: scsi0 : sata_sil scsi1 : sata_sil ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 9 secs ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) So at least the drive detection works, but it would be nice not to trigger this 9-second retry. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: implement ata_wait_after_reset()
On Wed, May 16, 2007 at 06:44:53PM +0200, Tejun Heo wrote: + /* FIXME: GoVault needs 2s but we can't afford that without + * parallel probing. 800ms is enough for iVDR disk + * HHD424020F7SV00. Increase to 2secs when parallel probing + * is in place. + */ + ATA_TMOUT_FF_WAIT = 4 * HZ / 5, + Changing this to 4 * HZ / 4 gets rid of the occasional COMRESET failure. So it would seem that 800ms is good enough for the common case, but it seems to be cutting it pretty close.. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata reset-seq merge broke sata_sil on sh
On Fri, May 11, 2007 at 11:39:20AM +0200, Tejun Heo wrote: Paul Mundt wrote: Bumping the hardreset delay up does indeed fix it, I've had to bump it up to 1200 before it started working (at 600 it still fails): [0.967379] scsi0 : sata_sil [0.970425] scsi1 : sata_sil [0.973298] ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 [0.981331] ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 [1.299353] ata1: device not ready (errno=-19), forcing hardreset [2.817893] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) [2.826284] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.831052] ata1.00: ATA-5: HHD424020F7SV00, 00MLA0A5, max UDMA/100 [2.837548] ata1.00: 39070080 sectors, multi 0: LBA [2.842702] ata1.00: applying bridge limits [2.854162] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.858938] ata1.00: configured for UDMA/100 [3.172602] ata2: SATA link down (SStatus 0 SControl 310) [3.175736] scsi 0:0:0:0: Direct-Access ATA HHD424020F7SV00 00ML PQ: 0 ANSI: 5 I'm not sure if it matters or not, but this is an iVDR drive, so that might also have additional implications. Don't have the flimsiest idea what an iVDR drive is but I take it that it's some sort of special purpose thing. :-) http://www.ivdr.org The GoVault appears to be a similar device, in that they're both removeable cartridges. Gary, IIRC, the requirement for GoVault was 3secs, right? Paul, can you try to estimate the minimum required delay? Please go down by 100ms and report where it breaks. 800ms was the lowest it would work at, 700ms still breaks. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
libata reset-seq merge broke sata_sil on sh
Current git fails to boot via SATA on SH with the recent libata merge: sata_sil :00:01.0: cache line size not set. Driver may not function scsi0 : sata_sil scsi1 : sata_sil ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 10 secs ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 10 secs ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 35 secs ata1: COMRESET failed (errno=-19) ata1: reset failed, giving up ata2: SATA link down (SStatus 0 SControl 310) ... bisect points to the reset-seq merge as the bad change. Going back a bit further from this, these are where the problems first started showing up (but still managed to find the disk, which current git is not able to). At b8cffc6ad8c000410186815b7bcc6b76ef1bbb13 it's still working, even though it's started complaining about the reset.. sata_sil :00:01.0: version 2.2 sata_sil :00:01.0: cache line size not set. Driver may not function scsi0 : sata_sil scsi1 : sata_sil ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: COMRESET failed (errno=-19) ata1: hardreset failed, retrying in 5 secs ata1: COMRESET failed (errno=-19) ata1: hardreset failed, retrying in 5 secs ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 ata1.00: ATA-5: HHD424020F7SV00, 00MLA0A5, max UDMA/100 ata1.00: 39070080 sectors, multi 0: LBA ata1.00: applying bridge limits ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 ata1.00: configured for UDMA/100 ata2: SATA link down (SStatus 0 SControl 310) isa bounce pool size: 16 pages scsi 0:0:0:0: Direct-Access ATA HHD424020F7SV00 00ML PQ: 0 ANSI: 5 SCSI device sda: 39070080 512-byte hdwr sectors (20004 MB) sda: Write Protect is off sda: Mode Sense: 00 3a 00 00 SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA SCSI device sda: 39070080 512-byte hdwr sectors (20004 MB) sda: Write Protect is off sda: Mode Sense: 00 3a 00 00 SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA sda: sda1 sda2 sda3 sda4 sd 0:0:0:0: Attached scsi disk sda at 27c78b372d05e47bbd059c9bb003c6d716abff54 the retry time was changed to 10 seconds, which still manages to find the device.. sata_sil :00:01.0: cache line size not set. Driver may not function scsi0 : sata_sil scsi1 : sata_sil ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 ata1: device not ready (errno=-19), forcing hardreset ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 10 secs ata1: COMRESET failed (errno=-19) ata1: reset failed (errno=-19), retrying in 10 secs ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 ata1.00: ATA-5: HHD424020F7SV00, 00MLA0A5, max UDMA/100 ata1.00: 39070080 sectors, multi 0: LBA ata1.00: applying bridge limits ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 ata1.00: configured for UDMA/100 ata2: SATA link down (SStatus 0 SControl 310) isa bounce pool size: 16 pages scsi 0:0:0:0: Direct-Access ATA HHD424020F7SV00 00ML PQ: 0 ANSI: 5 SCSI device sda: 39070080 512-byte hdwr sectors (20004 MB) sda: Write Protect is off SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA SCSI device sda: 39070080 512-byte hdwr sectors (20004 MB) sda: Write Protect is off SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA sda: sda1 sda2 sda3 sda4 sd 0:0:0:0: Attached scsi disk sda However, if I go back before any of the reset changes were introduced, things were working fine, and there were no problems with waiting for the reset. Ideas? - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata reset-seq merge broke sata_sil on sh
On Thu, May 10, 2007 at 01:28:19PM +0200, Tejun Heo wrote: Paul Mundt wrote: [--snip, thanks a lot for the detailed report--] However, if I go back before any of the reset changes were introduced, things were working fine, and there were no problems with waiting for the reset. Ideas? * Does your drive start spun down when it boots? Can you post dmesg with printk timestamp turned on with kernel prior to reset-seq merge? Yes, it's spun down at boot. I'll get the logs with the timestamps and the results of the mdelay() incrementing in the morning when I've got the board handy again. On Thu, May 10, 2007 at 01:53:48PM +0200, Tejun Heo wrote: * According to the report, things still work till 27c78b37 - commit for the actual merge and there's no related change till the current master from there. So, which commit actually breaks detection? Or is detection just flaky after b8cffc6a? The detection is simply flaky after that point, however before the current master it never hit the 35 second point (and thus never implied that the link was down). I'll double check the bisect log to see if there was anything beyond that that may have caused it. The -ENODEV at least implies that the SRST fails, so at least that's a starting point. One other curious thing is that it seems to misreport the IRQ. In this case the log indicates 0, whereas it's actually on IRQ 66. When the system is booted, /proc/interrupts reflects that sata_sil is on the proper IRQ (even when it's reported as 0 in the boot printk()). - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata reset-seq merge broke sata_sil on sh
On Thu, May 10, 2007 at 03:08:59PM +0200, Tejun Heo wrote: Paul Mundt wrote: The detection is simply flaky after that point, however before the current master it never hit the 35 second point (and thus never implied that the link was down). I'll double check the bisect log to see if there was anything beyond that that may have caused it. The -ENODEV at least implies that the SRST fails, so at least that's a starting point. If prereset() fails to get the initial DRDY before 10secs, it assumes something went wrong and escalates to hardreset. sil family of controllers report 0xff status while the link is broken and it seems that your particular drive needs more than the current 150ms to recover phy link. It probably went unnoticed till now because the device was never hardreset before. If the diagnosis is correct, increasing the delay in hardreset should fix the problem. Well, let's see. :-) Bumping the hardreset delay up does indeed fix it, I've had to bump it up to 1200 before it started working (at 600 it still fails): [0.967379] scsi0 : sata_sil [0.970425] scsi1 : sata_sil [0.973298] ata1: SATA max UDMA/100 cmd 0xfd000280 ctl 0xfd00028a bmdma 0xfd000200 irq 0 [0.981331] ata2: SATA max UDMA/100 cmd 0xfd0002c0 ctl 0xfd0002ca bmdma 0xfd000208 irq 0 [1.299353] ata1: device not ready (errno=-19), forcing hardreset [2.817893] ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) [2.826284] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.831052] ata1.00: ATA-5: HHD424020F7SV00, 00MLA0A5, max UDMA/100 [2.837548] ata1.00: 39070080 sectors, multi 0: LBA [2.842702] ata1.00: applying bridge limits [2.854162] ata1.00: ata_hpa_resize 1: sectors = 39070080, hpa_sectors = 39070080 [2.858938] ata1.00: configured for UDMA/100 [3.172602] ata2: SATA link down (SStatus 0 SControl 310) [3.175736] scsi 0:0:0:0: Direct-Access ATA HHD424020F7SV00 00ML PQ: 0 ANSI: 5 I'm not sure if it matters or not, but this is an iVDR drive, so that might also have additional implications. -- diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 4595d1f..4dad3fd 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3518,7 +3518,7 @@ int sata_std_hardreset(struct ata_port *ap, unsigned int *class, } /* wait a while before checking status, see SRST for more info */ - msleep(150); + msleep(1200); rc = ata_wait_ready(ap, deadline); /* link occupied, -ENODEV too is an error */ - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
