[PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Bartlomiej Zolnierkiewicz]

Hi,

This patchset adds m68k/Atari Falcon PATA support to libata.
The major difference in the new libata's pata_falcon host
driver when compared to legacy IDE's falconide host driver is
that we are using polled PIO mode and thus avoiding the need
for STDMA locking magic altogether.

Tested under ARAnyM emulator.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


Bartlomiej Zolnierkiewicz (3):
  ata: allow subsystem to be used on m68k arch
  ata: pass queued command to ->sff_data_xfer method
  ata: add Atari Falcon PATA controller driver

 drivers/ata/Kconfig           |  11 ++-
 drivers/ata/Makefile          |   1 +
 drivers/ata/libata-sff.c      |  29 +++----
 drivers/ata/pata_at91.c       |   6 +-
 drivers/ata/pata_bf54x.c      |   7 +-
 drivers/ata/pata_ep93xx.c     |   4 +-
 drivers/ata/pata_falcon.c     | 184 ++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/pata_ixp4xx_cf.c  |   4 +-
 drivers/ata/pata_legacy.c     |  15 ++--
 drivers/ata/pata_octeon_cf.c  |  12 +--
 drivers/ata/pata_pcmcia.c     |   6 +-
 drivers/ata/pata_samsung_cf.c |   4 +-
 drivers/ata/sata_rcar.c       |   4 +-
 include/linux/libata.h        |   8 +-
 14 files changed, 247 insertions(+), 48 deletions(-)
 create mode 100644 drivers/ata/pata_falcon.c

-- 
1.9.1

[PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Bartlomiej Zolnierkiewicz]

When libata was merged m68k lacked IOMAP support.  This has not been
true for a long time now so allow subsystem to be used on m68k.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/ata/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 2c8be74..da19fc9 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -14,7 +14,7 @@ menuconfig ATA
 	tristate "Serial ATA and Parallel ATA drivers (libata)"
 	depends on HAS_IOMEM
 	depends on BLOCK
-	depends on !(M32R || M68K || S390) || BROKEN
+	depends on !(M32R || S390) || BROKEN
 	select SCSI
 	select GLOB
 	---help---
-- 
1.9.1

Re: [PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Christoph Hellwig]

On Fri, Dec 30, 2016 at 03:01:16PM +0100, Bartlomiej Zolnierkiewicz wrote:
> When libata was merged m68k lacked IOMAP support.  This has not been
> true for a long time now so allow subsystem to be used on m68k.

Is that dependency needed at all anymore?  What exact functionality
is missing on m32r and s390?

Re: [PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, December 30, 2016 06:12:49 AM Christoph Hellwig wrote:
> On Fri, Dec 30, 2016 at 03:01:16PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > When libata was merged m68k lacked IOMAP support.  This has not been
> > true for a long time now so allow subsystem to be used on m68k.
> 
> Is that dependency needed at all anymore?  What exact functionality
> is missing on m32r and s390?

* m32r: I don't know why it was restricted but it builds fine nowadays
  (I will fix it later in incremental patch)

* s390: no PATA hardware, legacy IDE subsystem is also not supported

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Christoph Hellwig]

On Fri, Dec 30, 2016 at 06:14:36PM +0100, Bartlomiej Zolnierkiewicz wrote:
> * m32r: I don't know why it was restricted but it builds fine nowadays
>   (I will fix it later in incremental patch)
> 
> * s390: no PATA hardware, legacy IDE subsystem is also not supported

s390 now has PCIe support, so PATA could be attached through a PCIe to
PCI bridge in theory.  And while that is very theoretical not having
arch ifdefs is still something we should strive for, so I'd suggest
removing the condition entirely.

Re: [PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Bartlomiej Zolnierkiewicz]

Hi,

On Sunday, January 08, 2017 02:08:23 AM Christoph Hellwig wrote:
> On Fri, Dec 30, 2016 at 06:14:36PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > * m32r: I don't know why it was restricted but it builds fine nowadays
> >   (I will fix it later in incremental patch)
> > 
> > * s390: no PATA hardware, legacy IDE subsystem is also not supported
> 
> s390 now has PCIe support, so PATA could be attached through a PCIe to
> PCI bridge in theory.  And while that is very theoretical not having
> arch ifdefs is still something we should strive for, so I'd suggest
> removing the condition entirely.

I agree.  I will do it later unless someone beats me to it.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 1/3] ata: allow subsystem to be used on m68k arch

[Geert Uytterhoeven]

On Fri, Dec 30, 2016 at 3:01 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> When libata was merged m68k lacked IOMAP support.  This has not been
> true for a long time now so allow subsystem to be used on m68k.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/3] ata: pass queued command to ->sff_data_xfer method

[Bartlomiej Zolnierkiewicz]

For Atari Falcon PATA support we need to check the current command
in its ->sff_data_xfer method.  Update core code and all users
accordingly.

There should be no functional changes caused by this patch.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/ata/libata-sff.c      | 29 +++++++++++++++--------------
 drivers/ata/pata_at91.c       |  6 +++---
 drivers/ata/pata_bf54x.c      |  7 ++++---
 drivers/ata/pata_ep93xx.c     |  4 ++--
 drivers/ata/pata_ixp4xx_cf.c  |  4 ++--
 drivers/ata/pata_legacy.c     | 15 +++++++++------
 drivers/ata/pata_octeon_cf.c  | 12 ++++++------
 drivers/ata/pata_pcmcia.c     |  6 +++---
 drivers/ata/pata_samsung_cf.c |  4 ++--
 drivers/ata/sata_rcar.c       |  4 ++--
 include/linux/libata.h        |  8 ++++----
 11 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 051b615..4441b5c 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -542,7 +542,7 @@ static inline void ata_tf_to_host(struct ata_port *ap,
 
 /**
  *	ata_sff_data_xfer - Transfer data by PIO
- *	@dev: device to target
+ *	@qc: queued command
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
@@ -555,10 +555,10 @@ static inline void ata_tf_to_host(struct ata_port *ap,
  *	RETURNS:
  *	Bytes consumed.
  */
-unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
+unsigned int ata_sff_data_xfer(struct ata_queued_cmd *qc, unsigned char *buf,
 			       unsigned int buflen, int rw)
 {
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	unsigned int words = buflen >> 1;
 
@@ -595,7 +595,7 @@ unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
 
 /**
  *	ata_sff_data_xfer32 - Transfer data by PIO
- *	@dev: device to target
+ *	@qc: queued command
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
@@ -610,16 +610,17 @@ unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
  *	Bytes consumed.
  */
 
-unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
+unsigned int ata_sff_data_xfer32(struct ata_queued_cmd *qc, unsigned char *buf,
 			       unsigned int buflen, int rw)
 {
+	struct ata_device *dev = qc->dev;
 	struct ata_port *ap = dev->link->ap;
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	unsigned int words = buflen >> 2;
 	int slop = buflen & 3;
 
 	if (!(ap->pflags & ATA_PFLAG_PIO32))
-		return ata_sff_data_xfer(dev, buf, buflen, rw);
+		return ata_sff_data_xfer(qc, buf, buflen, rw);
 
 	/* Transfer multiple of 4 bytes */
 	if (rw == READ)
@@ -658,7 +659,7 @@ unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
 
 /**
  *	ata_sff_data_xfer_noirq - Transfer data by PIO
- *	@dev: device to target
+ *	@qc: queued command
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
@@ -672,14 +673,14 @@ unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
  *	RETURNS:
  *	Bytes consumed.
  */
-unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
+unsigned int ata_sff_data_xfer_noirq(struct ata_queued_cmd *qc, unsigned char *buf,
 				     unsigned int buflen, int rw)
 {
 	unsigned long flags;
 	unsigned int consumed;
 
 	local_irq_save(flags);
-	consumed = ata_sff_data_xfer32(dev, buf, buflen, rw);
+	consumed = ata_sff_data_xfer32(qc, buf, buflen, rw);
 	local_irq_restore(flags);
 
 	return consumed;
@@ -723,14 +724,14 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
 		buf = kmap_atomic(page);
 
 		/* do the actual data transfer */
-		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
+		ap->ops->sff_data_xfer(qc, buf + offset, qc->sect_size,
 				       do_write);
 
 		kunmap_atomic(buf);
 		local_irq_restore(flags);
 	} else {
 		buf = page_address(page);
-		ap->ops->sff_data_xfer(qc->dev, buf + offset, qc->sect_size,
+		ap->ops->sff_data_xfer(qc, buf + offset, qc->sect_size,
 				       do_write);
 	}
 
@@ -791,7 +792,7 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
 	DPRINTK("send cdb\n");
 	WARN_ON_ONCE(qc->dev->cdb_len < 12);
 
-	ap->ops->sff_data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
+	ap->ops->sff_data_xfer(qc, qc->cdb, qc->dev->cdb_len, 1);
 	ata_sff_sync(ap);
 	/* FIXME: If the CDB is for DMA do we need to do the transition delay
 	   or is bmdma_start guaranteed to do it ? */
@@ -868,14 +869,14 @@ static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
 		buf = kmap_atomic(page);
 
 		/* do the actual data transfer */
-		consumed = ap->ops->sff_data_xfer(dev,  buf + offset,
+		consumed = ap->ops->sff_data_xfer(qc, buf + offset,
 								count, rw);
 
 		kunmap_atomic(buf);
 		local_irq_restore(flags);
 	} else {
 		buf = page_address(page);
-		consumed = ap->ops->sff_data_xfer(dev,  buf + offset,
+		consumed = ap->ops->sff_data_xfer(qc, buf + offset,
 								count, rw);
 	}
 
diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
index 1611e0e..fd5b34f 100644
--- a/drivers/ata/pata_at91.c
+++ b/drivers/ata/pata_at91.c
@@ -286,10 +286,10 @@ static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	set_smc_timing(ap->dev, adev, info, &timing);
 }
 
-static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev,
+static unsigned int pata_at91_data_xfer_noirq(struct ata_queued_cmd *qc,
 		unsigned char *buf, unsigned int buflen, int rw)
 {
-	struct at91_ide_info *info = dev->link->ap->host->private_data;
+	struct at91_ide_info *info = qc->dev->link->ap->host->private_data;
 	unsigned int consumed;
 	unsigned int mode;
 	unsigned long flags;
@@ -301,7 +301,7 @@ static unsigned int pata_at91_data_xfer_noirq(struct ata_device *dev,
 	regmap_fields_write(fields.mode, info->cs, (mode & ~AT91_SMC_DBW) |
 			    AT91_SMC_DBW_16);
 
-	consumed = ata_sff_data_xfer(dev, buf, buflen, rw);
+	consumed = ata_sff_data_xfer(qc, buf, buflen, rw);
 
 	/* restore 8bit mode after data is written */
 	regmap_fields_write(fields.mode, info->cs, (mode & ~AT91_SMC_DBW) |
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index ec748d3..9c5780a 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -1143,7 +1143,7 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
 
 /**
  *	bfin_data_xfer - Transfer data by PIO
- *	@adev: device for this I/O
+ *	@qc: queued command
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@write_data: read/write
@@ -1151,10 +1151,11 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
  *	Note: Original code is ata_sff_data_xfer().
  */
 
-static unsigned int bfin_data_xfer(struct ata_device *dev, unsigned char *buf,
+static unsigned int bfin_data_xfer(struct ata_queued_cmd *qc,
+				   unsigned char *buf,
 				   unsigned int buflen, int rw)
 {
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
 	unsigned int words = buflen >> 1;
 	unsigned short *buf16 = (u16 *)buf;
diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index bd6b089..bf1b910 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -474,11 +474,11 @@ static void ep93xx_pata_set_devctl(struct ata_port *ap, u8 ctl)
 }
 
 /* Note: original code is ata_sff_data_xfer */
-static unsigned int ep93xx_pata_data_xfer(struct ata_device *adev,
+static unsigned int ep93xx_pata_data_xfer(struct ata_queued_cmd *qc,
 					  unsigned char *buf,
 					  unsigned int buflen, int rw)
 {
-	struct ata_port *ap = adev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	struct ep93xx_pata_data *drv_data = ap->host->private_data;
 	u16 *data = (u16 *)buf;
 	unsigned int words = buflen >> 1;
diff --git a/drivers/ata/pata_ixp4xx_cf.c b/drivers/ata/pata_ixp4xx_cf.c
index abda441..0b0d930 100644
--- a/drivers/ata/pata_ixp4xx_cf.c
+++ b/drivers/ata/pata_ixp4xx_cf.c
@@ -40,13 +40,13 @@ static int ixp4xx_set_mode(struct ata_link *link, struct ata_device **error)
 	return 0;
 }
 
-static unsigned int ixp4xx_mmio_data_xfer(struct ata_device *dev,
+static unsigned int ixp4xx_mmio_data_xfer(struct ata_queued_cmd *qc,
 				unsigned char *buf, unsigned int buflen, int rw)
 {
 	unsigned int i;
 	unsigned int words = buflen >> 1;
 	u16 *buf16 = (u16 *) buf;
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	void __iomem *mmio = ap->ioaddr.data_addr;
 	struct ixp4xx_pata_data *data = dev_get_platdata(ap->host->dev);
 
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index bce2a8c..53828b6c 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -303,11 +303,12 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
 
 }
 
-static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
+static unsigned int pdc_data_xfer_vlb(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw)
 {
-	int slop = buflen & 3;
+	struct ata_device *dev = qc->dev;
 	struct ata_port *ap = dev->link->ap;
+	int slop = buflen & 3;
 
 	/* 32bit I/O capable *and* we need to write a whole number of dwords */
 	if (ata_id_has_dword_io(dev->id) && (slop == 0 || slop == 3)
@@ -340,7 +341,7 @@ static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
 		}
 		local_irq_restore(flags);
 	} else
-		buflen = ata_sff_data_xfer_noirq(dev, buf, buflen, rw);
+		buflen = ata_sff_data_xfer_noirq(qc, buf, buflen, rw);
 
 	return buflen;
 }
@@ -702,9 +703,11 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
 	return ata_sff_qc_issue(qc);
 }
 
-static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
-					unsigned int buflen, int rw)
+static unsigned int vlb32_data_xfer(struct ata_queued_cmd *qc,
+				    unsigned char *buf,
+				    unsigned int buflen, int rw)
 {
+	struct ata_device *adev = qc->dev;
 	struct ata_port *ap = adev->link->ap;
 	int slop = buflen & 3;
 
@@ -727,7 +730,7 @@ static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
 		}
 		return (buflen + 3) & ~3;
 	} else
-		return ata_sff_data_xfer(adev, buf, buflen, rw);
+		return ata_sff_data_xfer(qc, buf, buflen, rw);
 }
 
 static int qdi_port(struct platform_device *dev,
diff --git a/drivers/ata/pata_octeon_cf.c b/drivers/ata/pata_octeon_cf.c
index 475a006..e94e7ca 100644
--- a/drivers/ata/pata_octeon_cf.c
+++ b/drivers/ata/pata_octeon_cf.c
@@ -293,17 +293,17 @@ static void octeon_cf_set_dmamode(struct ata_port *ap, struct ata_device *dev)
 /**
  * Handle an 8 bit I/O request.
  *
- * @dev:        Device to access
+ * @qc:         Queued command
  * @buffer:     Data buffer
  * @buflen:     Length of the buffer.
  * @rw:         True to write.
  */
-static unsigned int octeon_cf_data_xfer8(struct ata_device *dev,
+static unsigned int octeon_cf_data_xfer8(struct ata_queued_cmd *qc,
 					 unsigned char *buffer,
 					 unsigned int buflen,
 					 int rw)
 {
-	struct ata_port *ap		= dev->link->ap;
+	struct ata_port *ap		= qc->dev->link->ap;
 	void __iomem *data_addr		= ap->ioaddr.data_addr;
 	unsigned long words;
 	int count;
@@ -332,17 +332,17 @@ static unsigned int octeon_cf_data_xfer8(struct ata_device *dev,
 /**
  * Handle a 16 bit I/O request.
  *
- * @dev:        Device to access
+ * @qc:         Queued command
  * @buffer:     Data buffer
  * @buflen:     Length of the buffer.
  * @rw:         True to write.
  */
-static unsigned int octeon_cf_data_xfer16(struct ata_device *dev,
+static unsigned int octeon_cf_data_xfer16(struct ata_queued_cmd *qc,
 					  unsigned char *buffer,
 					  unsigned int buflen,
 					  int rw)
 {
-	struct ata_port *ap		= dev->link->ap;
+	struct ata_port *ap		= qc->dev->link->ap;
 	void __iomem *data_addr		= ap->ioaddr.data_addr;
 	unsigned long words;
 	int count;
diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index bcc4b96..a541eac 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -90,7 +90,7 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
 
 /**
  *	ata_data_xfer_8bit	 -	Transfer data by 8bit PIO
- *	@dev: device to target
+ *	@qc: queued command
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
@@ -101,10 +101,10 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
  *	Inherited from caller.
  */
 
-static unsigned int ata_data_xfer_8bit(struct ata_device *dev,
+static unsigned int ata_data_xfer_8bit(struct ata_queued_cmd *qc,
 				unsigned char *buf, unsigned int buflen, int rw)
 {
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 
 	if (rw == READ)
 		ioread8_rep(ap->ioaddr.data_addr, buf, buflen);
diff --git a/drivers/ata/pata_samsung_cf.c b/drivers/ata/pata_samsung_cf.c
index f6facd6..431c7de 100644
--- a/drivers/ata/pata_samsung_cf.c
+++ b/drivers/ata/pata_samsung_cf.c
@@ -263,10 +263,10 @@ static u8 pata_s3c_check_altstatus(struct ata_port *ap)
 /*
  * pata_s3c_data_xfer - Transfer data by PIO
  */
-static unsigned int pata_s3c_data_xfer(struct ata_device *dev,
+static unsigned int pata_s3c_data_xfer(struct ata_queued_cmd *qc,
 				unsigned char *buf, unsigned int buflen, int rw)
 {
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	struct s3c_ide_info *info = ap->host->private_data;
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	unsigned int words = buflen >> 1, i;
diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index f72d601..5d38245 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -447,11 +447,11 @@ static void sata_rcar_exec_command(struct ata_port *ap,
 	ata_sff_pause(ap);
 }
 
-static unsigned int sata_rcar_data_xfer(struct ata_device *dev,
+static unsigned int sata_rcar_data_xfer(struct ata_queued_cmd *qc,
 					      unsigned char *buf,
 					      unsigned int buflen, int rw)
 {
-	struct ata_port *ap = dev->link->ap;
+	struct ata_port *ap = qc->dev->link->ap;
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	unsigned int words = buflen >> 1;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c170be5..0e8a800 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -968,7 +968,7 @@ struct ata_port_operations {
 	void (*sff_tf_read)(struct ata_port *ap, struct ata_taskfile *tf);
 	void (*sff_exec_command)(struct ata_port *ap,
 				 const struct ata_taskfile *tf);
-	unsigned int (*sff_data_xfer)(struct ata_device *dev,
+	unsigned int (*sff_data_xfer)(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw);
 	void (*sff_irq_on)(struct ata_port *);
 	bool (*sff_irq_check)(struct ata_port *);
@@ -1823,11 +1823,11 @@ extern int ata_sff_busy_sleep(struct ata_port *ap,
 extern void ata_sff_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 extern void ata_sff_exec_command(struct ata_port *ap,
 				 const struct ata_taskfile *tf);
-extern unsigned int ata_sff_data_xfer(struct ata_device *dev,
+extern unsigned int ata_sff_data_xfer(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw);
-extern unsigned int ata_sff_data_xfer32(struct ata_device *dev,
+extern unsigned int ata_sff_data_xfer32(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw);
-extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
+extern unsigned int ata_sff_data_xfer_noirq(struct ata_queued_cmd *qc,
 			unsigned char *buf, unsigned int buflen, int rw);
 extern void ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
-- 
1.9.1

[PATCH 3/3] ata: add Atari Falcon PATA controller driver

[Bartlomiej Zolnierkiewicz]

Add Atari Falcon PATA controller driver.  The major difference
when compared to legacy IDE's falconide host driver is that we
are using polled PIO mode and thus avoiding the need for STDMA
locking magic altogether.

Tested under ARAnyM emulator.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/ata/Kconfig       |   9 +++
 drivers/ata/Makefile      |   1 +
 drivers/ata/pata_falcon.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/ata/pata_falcon.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index da19fc9..cbd1019 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -895,6 +895,15 @@ config PATA_CMD640_PCI
 
 	  If unsure, say N.
 
+config PATA_FALCON
+	tristate "Atari Falcon PATA support"
+	depends on M68K && ATARI
+	help
+	  This option enables support for the on-board IDE
+	  interface on the Atari Falcon.
+
+	  If unsure, say N.
+
 config PATA_ISAPNP
 	tristate "ISA Plug and Play PATA support"
 	depends on ISAPNP
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index a46e6b7..89a0a19 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_PATA_WINBOND)	+= pata_sl82c105.o
 obj-$(CONFIG_PATA_AT32)		+= pata_at32.o
 obj-$(CONFIG_PATA_AT91)		+= pata_at91.o
 obj-$(CONFIG_PATA_CMD640_PCI)	+= pata_cmd640.o
+obj-$(CONFIG_PATA_FALCON)	+= pata_falcon.o
 obj-$(CONFIG_PATA_ISAPNP)	+= pata_isapnp.o
 obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_MPIIX)	+= pata_mpiix.o
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
new file mode 100644
index 0000000..7826408
--- /dev/null
+++ b/drivers/ata/pata_falcon.c
@@ -0,0 +1,184 @@
+/*
+ * Atari Falcon PATA controller driver
+ *
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Based on falconide.c:
+ *
+ *     Created 12 Jul 1997 by Geert Uytterhoeven
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_cmnd.h>
+#include <linux/ata.h>
+#include <linux/libata.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#include <asm/setup.h>
+#include <asm/atarihw.h>
+#include <asm/atariints.h>
+#include <asm/atari_stdma.h>
+#include <asm/ide.h>
+
+#define DRV_NAME "pata_falcon"
+#define DRV_VERSION "0.1.0"
+
+#define ATA_HD_BASE	0xfff00000
+#define ATA_HD_CONTROL	0x39
+
+static struct scsi_host_template pata_falcon_sht = {
+	ATA_PIO_SHT(DRV_NAME),
+};
+
+static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
+					  unsigned char *buf,
+					  unsigned int buflen, int rw)
+{
+	struct ata_device *dev = qc->dev;
+	struct ata_port *ap = dev->link->ap;
+	void __iomem *data_addr = ap->ioaddr.data_addr;
+	unsigned int words = buflen >> 1;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	bool swap = 1;
+
+	if (dev->class == ATA_DEV_ATA && cmd && cmd->request &&
+	    cmd->request->cmd_type == REQ_TYPE_FS)
+		swap = 0;
+
+	/* Transfer multiple of 2 bytes */
+	if (rw == READ) {
+		if (swap)
+			raw_insw_swapw((u16 *)data_addr, (u16 *)buf, words);
+		else
+			raw_insw((u16 *)data_addr, (u16 *)buf, words);
+	} else {
+		if (swap)
+			raw_outsw_swapw((u16 *)data_addr, (u16 *)buf, words);
+		else
+			raw_outsw((u16 *)data_addr, (u16 *)buf, words);
+	}
+
+	/* Transfer trailing byte, if any. */
+	if (unlikely(buflen & 0x01)) {
+		unsigned char pad[2] = { };
+
+		/* Point buf to the tail of buffer */
+		buf += buflen - 1;
+
+		if (rw == READ) {
+			if (swap)
+				raw_insw_swapw((u16 *)data_addr, (u16 *)pad, 1);
+			else
+				raw_insw((u16 *)data_addr, (u16 *)pad, 1);
+			*buf = pad[0];
+		} else {
+			pad[0] = *buf;
+			if (swap)
+				raw_outsw_swapw((u16 *)data_addr, (u16 *)pad, 1);
+			else
+				raw_outsw((u16 *)data_addr, (u16 *)pad, 1);
+		}
+		words++;
+	}
+
+	return words << 1;
+}
+
+/*
+ * Provide our own set_mode() as we don't want to change anything that has
+ * already been configured..
+ */
+static int pata_falcon_set_mode(struct ata_link *link,
+				struct ata_device **unused)
+{
+	struct ata_device *dev;
+
+	ata_for_each_dev(dev, link, ENABLED) {
+		/* We don't really care */
+		dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		dev->flags |= ATA_DFLAG_PIO;
+		ata_dev_info(dev, "configured for PIO\n");
+	}
+	return 0;
+}
+
+static struct ata_port_operations pata_falcon_ops = {
+	.inherits	= &ata_sff_port_ops,
+	.sff_data_xfer	= pata_falcon_data_xfer,
+	.cable_detect	= ata_cable_unknown,
+	.set_mode	= pata_falcon_set_mode,
+};
+
+static int pata_falcon_init_one(void)
+{
+	struct ata_host *host;
+	struct ata_port *ap;
+	struct platform_device *pdev;
+	void __iomem *base;
+
+	if (!MACH_IS_ATARI || !ATARIHW_PRESENT(IDE))
+		return -ENODEV;
+
+	pr_info(DRV_NAME ": Atari Falcon PATA controller\n");
+
+	pdev = platform_device_register_simple(DRV_NAME, 0, NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	if (!devm_request_mem_region(&pdev->dev, ATA_HD_BASE, 0x40, DRV_NAME)) {
+		pr_err(DRV_NAME ": resources busy\n");
+		return -EBUSY;
+	}
+
+	/* allocate host */
+	host = ata_host_alloc(&pdev->dev, 1);
+	if (!host)
+		return -ENOMEM;
+	ap = host->ports[0];
+
+	ap->ops = &pata_falcon_ops;
+	ap->pio_mask = ATA_PIO4;
+	ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
+	ap->flags |= ATA_FLAG_PIO_POLLING;
+
+	base = (void __iomem *)ATA_HD_BASE;
+	ap->ioaddr.data_addr		= base;
+	ap->ioaddr.error_addr		= base + 1 + 1 * 4;
+	ap->ioaddr.feature_addr		= base + 1 + 1 * 4;
+	ap->ioaddr.nsect_addr		= base + 1 + 2 * 4;
+	ap->ioaddr.lbal_addr		= base + 1 + 3 * 4;
+	ap->ioaddr.lbam_addr		= base + 1 + 4 * 4;
+	ap->ioaddr.lbah_addr		= base + 1 + 5 * 4;
+	ap->ioaddr.device_addr		= base + 1 + 6 * 4;
+	ap->ioaddr.status_addr		= base + 1 + 7 * 4;
+	ap->ioaddr.command_addr		= base + 1 + 7 * 4;
+
+	ap->ioaddr.altstatus_addr	= base + ATA_HD_CONTROL;
+	ap->ioaddr.ctl_addr		= base + ATA_HD_CONTROL;
+
+	ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", (unsigned long)base,
+		      (unsigned long)base + ATA_HD_CONTROL);
+
+	/* activate */
+	return ata_host_activate(host, 0, NULL, 0, &pata_falcon_sht);
+}
+
+module_init(pata_falcon_init_one);
+
+MODULE_AUTHOR("Bartlomiej Zolnierkiewicz");
+MODULE_DESCRIPTION("low-level driver for Atari Falcon PATA");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
-- 
1.9.1

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Geert Uytterhoeven]

Hi Bartlomiej,

On Fri, Dec 30, 2016 at 3:01 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> This patchset adds m68k/Atari Falcon PATA support to libata.

Thanks for your series!

That leaves us with 4 to go ;-)

    CONFIG_BLK_DEV_GAYLE
    CONFIG_BLK_DEV_BUDDHA
    CONFIG_BLK_DEV_MAC_IDE
    CONFIG_BLK_DEV_Q40IDE

Note that using libata instead of the legacy IDE driver increases kernel size.

After enabling libata:

    CONFIG_ATA=y
    CONFIG_ATA_VERBOSE_ERROR=y
    CONFIG_ATA_SFF=y
    CONFIG_PATA_FALCON=y

an atari_defconfig kernel grew by:

    add/remove: 775/0 grow/shrink: 753/41 up/down: 98999/-242 (98757)

After disabling CONFIG_IDE:

    add/remove: 0/589 grow/shrink: 0/12 up/down: 0/-62835 (-62835)

So the net result is:

    add/remove: 775/589 grow/shrink: 749/51 up/down: 98886/-62964 (35922)

Disabling CONFIG_ATA_VERBOSE_ERROR saved 1380 bytes, which is less than the
value advertised by Kconfig (6KB).

> The major difference in the new libata's pata_falcon host
> driver when compared to legacy IDE's falconide host driver is
> that we are using polled PIO mode and thus avoiding the need
> for STDMA locking magic altogether.

I'll let the Atari experts (Michael?) comment on that...

> Tested under ARAnyM emulator.

Works indeed fine on ARAnyM.
Unfortunately I can't test it on real hardware...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, January 03, 2017 11:49:16 AM Geert Uytterhoeven wrote:
> Hi Bartlomiej,
> 
> On Fri, Dec 30, 2016 at 3:01 PM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
> > This patchset adds m68k/Atari Falcon PATA support to libata.
> 
> Thanks for your series!
> 
> That leaves us with 4 to go ;-)
> 
>     CONFIG_BLK_DEV_GAYLE
>     CONFIG_BLK_DEV_BUDDHA
>     CONFIG_BLK_DEV_MAC_IDE
>     CONFIG_BLK_DEV_Q40IDE

They should be easy to port to libata, one just needs to remember to
use ->qc_defer for ports that require serialization.

> Note that using libata instead of the legacy IDE driver increases kernel size.
> 
> After enabling libata:
> 
>     CONFIG_ATA=y
>     CONFIG_ATA_VERBOSE_ERROR=y
>     CONFIG_ATA_SFF=y
>     CONFIG_PATA_FALCON=y
> 
> an atari_defconfig kernel grew by:
> 
>     add/remove: 775/0 grow/shrink: 753/41 up/down: 98999/-242 (98757)
> 
> After disabling CONFIG_IDE:
> 
>     add/remove: 0/589 grow/shrink: 0/12 up/down: 0/-62835 (-62835)
> 
> So the net result is:
> 
>     add/remove: 775/589 grow/shrink: 749/51 up/down: 98886/-62964 (35922)

SATA-specific code is not needed on legacy PATA systems so it should be
possible to reduce the size by adding new config option (i.e. SATA_HOST) and:

- covering SATA only code with CONFIG_SATA_HOST ifdefs

- making SATA_HOST to be selected by SATA host drivers in drivers/ata/KConfig

> Disabling CONFIG_ATA_VERBOSE_ERROR saved 1380 bytes, which is less than the
> value advertised by Kconfig (6KB).

If it is only ~1kB nowadays I would vote for making the error logging always
verbose and just removing CONFIG_ATA_VERBOSE_ERROR, Tejun?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Tejun Heo]

Hello,

On Mon, Jan 09, 2017 at 05:11:12PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > Disabling CONFIG_ATA_VERBOSE_ERROR saved 1380 bytes, which is less than the
> > value advertised by Kconfig (6KB).
> 
> If it is only ~1kB nowadays I would vote for making the error logging always
> verbose and just removing CONFIG_ATA_VERBOSE_ERROR, Tejun?

No objection from me.

Thanks.

-- 
tejun

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Bartlomiej,

thanks for caring to support our legacy PATA systems!

On Sat, Dec 31, 2016 at 3:01 AM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> Hi,
>
> This patchset adds m68k/Atari Falcon PATA support to libata.
> The major difference in the new libata's pata_falcon host
> driver when compared to legacy IDE's falconide host driver is
> that we are using polled PIO mode and thus avoiding the need
> for STDMA locking magic altogether.

I don't suppose this is the default libata mode for PIO?

How is polling implemented in libata? Sleeping for something
approximating the average seek latency shouldn't hurt but spinning
wont be acceptable for a low performance single CPU architecture like
the Falcon.

> Tested under ARAnyM emulator.

Not sure that the emulator is really feature complete enough - I'll
get this tested on my Falcon in the next few weeks. I'm a bit worried
about IDE still generating interrupts on seek completion (did you spot
anything like that, Geert?).

Cheers,

  Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, January 06, 2017 10:01:49 AM Michael Schmitz wrote:
> Hi Bartlomiej,
> 
> thanks for caring to support our legacy PATA systems!
> 
> On Sat, Dec 31, 2016 at 3:01 AM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
> > Hi,
> >
> > This patchset adds m68k/Atari Falcon PATA support to libata.
> > The major difference in the new libata's pata_falcon host
> > driver when compared to legacy IDE's falconide host driver is
> > that we are using polled PIO mode and thus avoiding the need
> > for STDMA locking magic altogether.
> 
> I don't suppose this is the default libata mode for PIO?

No, by default it is used only for some commands (i.e. IDENTIFY,
SET FEATURES - XFER MODE).

> How is polling implemented in libata? Sleeping for something
> approximating the average seek latency shouldn't hurt but spinning
> wont be acceptable for a low performance single CPU architecture like
> the Falcon.

You can find actual implementation in libata-sff.c.

Please see ata_sff_pio_task() for the main polling logic:

fsm_start:
	WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);

	/*
	 * This is purely heuristic.  This is a fast path.
	 * Sometimes when we enter, BSY will be cleared in
	 * a chk-status or two.  If not, the drive is probably seeking
	 * or something.  Snooze for a couple msecs, then
	 * chk-status again.  If still busy, queue delayed work.
	 */
	status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
	if (status & ATA_BUSY) {
		spin_unlock_irq(ap->lock);
		ata_msleep(ap, 2);
		spin_lock_irq(ap->lock);

		status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
		if (status & ATA_BUSY) {
			ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
			goto out_unlock;
		}
	}

	/*
	 * hsm_move() may trigger another command to be processed.
	 * clean the link beforehand.
	 */
	ap->sff_pio_task_link = NULL;
	/* move the HSM */
	poll_next = ata_sff_hsm_move(ap, qc, status, 1);

	/* another command or interrupt handler
	 * may be running at this point.
	 */
	if (poll_next)
		goto fsm_start;
out_unlock:


ata_sff_busy_wait()'s last argument is number of 10uS waits so
first check (50uS) should be quite quick.  If device is still
busy we sleep for 2ms.  Then we quickly (100uS) busy wait and
if needed  queue delayed work (with ATA_SHORT_PAUSE == 16 ms
delay).

Overall the current heuristic looks fine and spinning should be
minimal.

> > Tested under ARAnyM emulator.
> 
> Not sure that the emulator is really feature complete enough - I'll
> get this tested on my Falcon in the next few weeks. I'm a bit worried

Great!

> about IDE still generating interrupts on seek completion (did you spot
> anything like that, Geert?).
> 
> Cheers,
> 
>   Michael

BTW according to comment in arch/m68k/atari/stdma.c:

/* On the Falcon, the IDE bus uses just the ACSI/Floppy interrupt, but */
/* not the ST-DMA chip itself. So falhd.c needs not to lock the        */
/* chip. The interrupt is routed to falhd.c if IDE is configured, the  */
/* model is a Falcon and the interrupt was caused by the HD controller */
/* (can be determined by looking at its status register).              */

it should be okay to use IDE at the same time as SCSI/Floppy which
is what the new driver does (the old one is serialized operations by
ST-DMA related IRQ handling magic).

Also the comment itself may need some fixups as on Falcon it is SCSI
not ACSI (according to the earlier comment in same file) and the old
IDE host driver name is not falhd.c but falconide.c.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Bartlomiej,


>> How is polling implemented in libata? Sleeping for something
>> approximating the average seek latency shouldn't hurt but spinning
>> wont be acceptable for a low performance single CPU architecture like
>> the Falcon.
>
> You can find actual implementation in libata-sff.c.
>
> Please see ata_sff_pio_task() for the main polling logic:
>
> fsm_start:
>         WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
>
>         /*
>          * This is purely heuristic.  This is a fast path.
>          * Sometimes when we enter, BSY will be cleared in
>          * a chk-status or two.  If not, the drive is probably seeking
>          * or something.  Snooze for a couple msecs, then
>          * chk-status again.  If still busy, queue delayed work.
>          */
>         status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
>         if (status & ATA_BUSY) {
>                 spin_unlock_irq(ap->lock);
>                 ata_msleep(ap, 2);
>                 spin_lock_irq(ap->lock);
>
>                 status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
>                 if (status & ATA_BUSY) {
>                         ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
>                         goto out_unlock;
>                 }
>         }
>
>         /*
>          * hsm_move() may trigger another command to be processed.
>          * clean the link beforehand.
>          */
>         ap->sff_pio_task_link = NULL;
>         /* move the HSM */
>         poll_next = ata_sff_hsm_move(ap, qc, status, 1);
>
>         /* another command or interrupt handler
>          * may be running at this point.
>          */
>         if (poll_next)
>                 goto fsm_start;
> out_unlock:
>
>
> ata_sff_busy_wait()'s last argument is number of 10uS waits so
> first check (50uS) should be quite quick.  If device is still
> busy we sleep for 2ms.  Then we quickly (100uS) busy wait and
> if needed  queue delayed work (with ATA_SHORT_PAUSE == 16 ms
> delay).
>
> Overall the current heuristic looks fine and spinning should be
> minimal.

Thanks, that looks entirely reasonable.

>> > Tested under ARAnyM emulator.
>>
>> Not sure that the emulator is really feature complete enough - I'll
>> get this tested on my Falcon in the next few weeks. I'm a bit worried
>
> Great!
>
>> about IDE still generating interrupts on seek completion (did you spot
>> anything like that, Geert?).
>>
>> Cheers,
>>
>>   Michael
>
> BTW according to comment in arch/m68k/atari/stdma.c:
>
> /* On the Falcon, the IDE bus uses just the ACSI/Floppy interrupt, but */
> /* not the ST-DMA chip itself. So falhd.c needs not to lock the        */
> /* chip. The interrupt is routed to falhd.c if IDE is configured, the  */
> /* model is a Falcon and the interrupt was caused by the HD controller */
> /* (can be determined by looking at its status register).              */

That comment is probably incorrect in part. Blame Linus :-)

Seriously though - that comment dates back to the dark ages (when m68k
was an entirely separate port and the IDE driver was indeed named
falhd.c). That would have been even before 2.2 or 2.4 times. The
comment just never got updated.

What is still correct is that the IDE driver does use the interrupt
only, not the ST-DMA chip. And a single IDE interrupt can be correctly
assigned to IDE by looking at the status register.

With the SCSI (and IIRC also floppy) interrupts, we don't have direct
access to the status registers without disturbing the state of the DMA
though. Unless we know for definite that either chips have raised the
interrupt (and DMA ops are in flight), we must not touch the DMA chip
at all.

The case I'm worried about is both IDE and SCSI raising an interrupt.
We don't currently mask the IDE/ST-DMA interrupt so a stacked
interrupt must be processed in the same pass as the initial interrupt
or it will get dropped. We'd have to peek at the DMA registers to
check the SCSI or floppy interrupt status, and we just can't safely do
that. So races of this kind are currently prevented by including IDE
in the IRQ locking process.

Whether it's possible to mask the interrupt, do one pass, unmask and
process the second interrupt I don't know. Maybe Andreas does?

Cheers,

  Michael


> it should be okay to use IDE at the same time as SCSI/Floppy which
> is what the new driver does (the old one is serialized operations by
> ST-DMA related IRQ handling magic).
>
> Also the comment itself may need some fixups as on Falcon it is SCSI
> not ACSI (according to the earlier comment in same file) and the old
> IDE host driver name is not falhd.c but falconide.c.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Wed, 11 Jan 2017, Michael Schmitz wrote:

> What is still correct is that the IDE driver does use the interrupt 
> only, not the ST-DMA chip. And a single IDE interrupt can be correctly 
> assigned to IDE by looking at the status register.
> 
> With the SCSI (and IIRC also floppy) interrupts, we don't have direct 
> access to the status registers without disturbing the state of the DMA 
> though. Unless we know for definite that either chips have raised the 
> interrupt (and DMA ops are in flight), we must not touch the DMA chip at 
> all.
> 
> The case I'm worried about is both IDE and SCSI raising an interrupt. We 
> don't currently mask the IDE/ST-DMA interrupt so a stacked interrupt 
> must be processed in the same pass as the initial interrupt or it will 
> get dropped. We'd have to peek at the DMA registers to check the SCSI or 
> floppy interrupt status, and we just can't safely do that. So races of 
> this kind are currently prevented by including IDE in the IRQ locking 
> process.
> 
> Whether it's possible to mask the interrupt, do one pass, unmask and 
> process the second interrupt I don't know.

Would that require handling the SCSI DMA interrupt in the first pass? Or 
handling IDE first, and ensuring that the IDE handler does not access 
ST-DMA registers? What about FDC?

The atari_scsi handler accesses the ST-DMA registers; it can do so because 
it knows that any DMA must have completed -- it can infer this because a 
simultaneous pending interrupt from FDC or IDE is impossible due to 
stdma_lock().

Your suggestion would seem to allow other pending interrupts, hence the 
atari_scsi interrupt handler logic has to be tossed out. What logic would 
replace it?

If all else fails, perhaps we could inhibit DMA entirely when the new ATA 
driver is loaded. Then we can just dispatch the ST-DMA irq like a shared 
irq. I'm sure that atari_scsi can work without DMA. No idea about the FDC 
driver though (ataflop.c).

Another solution would be to dedicate the DMA function to atari_scsi, and 
then mask the FDC and IDE interrupts during each DMA transfer. But once 
again, this would mean changing the FDC driver to eliminate DMA, if that 
is possible. From the schematic it looks the the FDC chip, "AJAX", is 
another custom ...
http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf

Unfortunately my grasp of the ST hardware reflects my inability to read 
German; those who can may want to take a look at "ATARI Profibuch 
ST-STE-TT.pdf".

-- 

> Maybe Andreas does?
> 
> Cheers,
> 
>   Michael
> 
> 
> > it should be okay to use IDE at the same time as SCSI/Floppy which is 
> > what the new driver does (the old one is serialized operations by 
> > ST-DMA related IRQ handling magic).
> >
> > Also the comment itself may need some fixups as on Falcon it is SCSI 
> > not ACSI (according to the earlier comment in same file) and the old 
> > IDE host driver name is not falhd.c but falconide.c.
> >
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,

Am 13.01.2017 um 15:33 schrieb Finn Thain:

>> The case I'm worried about is both IDE and SCSI raising an interrupt. We 
>> don't currently mask the IDE/ST-DMA interrupt so a stacked interrupt 
>> must be processed in the same pass as the initial interrupt or it will 
>> get dropped. We'd have to peek at the DMA registers to check the SCSI or 
>> floppy interrupt status, and we just can't safely do that. So races of 
>> this kind are currently prevented by including IDE in the IRQ locking 
>> process.
>>
>> Whether it's possible to mask the interrupt, do one pass, unmask and 
>> process the second interrupt I don't know.
> 
> Would that require handling the SCSI DMA interrupt in the first pass? Or 
> handling IDE first, and ensuring that the IDE handler does not access 
> ST-DMA registers? What about FDC?

Handling the IDE interrupt first I think, then looking at the DMA (for
SCSI or FDC).

> The atari_scsi handler accesses the ST-DMA registers; it can do so because 
> it knows that any DMA must have completed -- it can infer this because a 
> simultaneous pending interrupt from FDC or IDE is impossible due to 
> stdma_lock().

libata dropped the locking (and does not use IDE interrupts at present
so it seems to be safe. Still testing - I've seen IO errors, and that's
a bit of a worry).

> Your suggestion would seem to allow other pending interrupts, hence the 
> atari_scsi interrupt handler logic has to be tossed out. What logic would 
> replace it?

I need to think about that some more - if no DMA is in progress we can
safely peek at the SCSI registers. So the logic could be changed to test
for DMA operation first, and just try and service the interrupt if DMA
wasn't active.

If DMA has been in progress, I'm not sure that we can figure out if it's
still active from looking at the status register (that is, whether bits
0 or 1 are set while DMA is ongoing). We'd have to peek at the DMA
status register (or DMA address registers) without first stopping DMA,
which the current driver does. The docs seem to advise against that.
If DMA was in progress, stopping it would likely leave us with residual
bytes to be transferred - we'd have to handle that transfer as we would
any other DMA error (from memory, probably best to retry the entire
command, or transfer the remaining bytes using PIO if we're sure no
bytes have been lost).

> If all else fails, perhaps we could inhibit DMA entirely when the new ATA 
> driver is loaded. Then we can just dispatch the ST-DMA irq like a shared 
> irq. I'm sure that atari_scsi can work without DMA. No idea about the FDC 
> driver though (ataflop.c).

Yes, SCSI can work using PIO but's it a real dog. Been there, done that
(about 20 years ago). I know nothing about the FDC chip though.

> Another solution would be to dedicate the DMA function to atari_scsi, and 
> then mask the FDC and IDE interrupts during each DMA transfer. But once 
> again, this would mean changing the FDC driver to eliminate DMA, if that 
> is possible. From the schematic it looks the the FDC chip, "AJAX", is 
> another custom ...
> http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf
> 
> Unfortunately my grasp of the ST hardware reflects my inability to read 
> German; those who can may want to take a look at "ATARI Profibuch 
> ST-STE-TT.pdf".

I'll reread the ST-DMA description (and the FDC one).

Let me know if you think this could work ...

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Sat, 14 Jan 2017, Michael Schmitz wrote:

> Am 13.01.2017 um 15:33 schrieb Finn Thain:
> 
> >> The case I'm worried about is both IDE and SCSI raising an interrupt. 
> >> We don't currently mask the IDE/ST-DMA interrupt so a stacked 
> >> interrupt must be processed in the same pass as the initial interrupt 
> >> or it will get dropped. We'd have to peek at the DMA registers to 
> >> check the SCSI or floppy interrupt status, and we just can't safely 
> >> do that. So races of this kind are currently prevented by including 
> >> IDE in the IRQ locking process.
> >>
> >> Whether it's possible to mask the interrupt, do one pass, unmask and 
> >> process the second interrupt I don't know.
> > 
> > Would that require handling the SCSI DMA interrupt in the first pass? 
> > Or handling IDE first, and ensuring that the IDE handler does not 
> > access ST-DMA registers? What about FDC?
> 
> Handling the IDE interrupt first I think, then looking at the DMA (for 
> SCSI or FDC).
> 

For the sake of discussion, I'll assume that the FDC driver will not be 
using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using the 
present locking mechanism, but it would not simplify things much: when IDE 
no longer participates in that locking mechanism then both FDC and SCSI 
drivers have to solve the same issues.)

> > The atari_scsi handler accesses the ST-DMA registers; it can do so 
> > because it knows that any DMA must have completed -- it can infer this 
> > because a simultaneous pending interrupt from FDC or IDE is impossible 
> > due to stdma_lock().
> 
> libata dropped the locking (and does not use IDE interrupts at present 
> so it seems to be safe. Still testing - I've seen IO errors, and that's 
> a bit of a worry).
> 

What compiler are you using, BTW? Are you still using the gcc-4.6.3 m68k 
cross-compiler from kernel.org? I had to abandon it in order to get my 
SCSI driver patches to work.

> > Your suggestion would seem to allow other pending interrupts, hence 
> > the atari_scsi interrupt handler logic has to be tossed out. What 
> > logic would replace it?
> 
> I need to think about that some more - if no DMA is in progress we can 
> safely peek at the SCSI registers. So the logic could be changed to test 
> for DMA operation first, and just try and service the interrupt if DMA 
> wasn't active.
> 

OK, so based on the above, we handle the possible IDE interrupt (without 
checking DMA registers), handle the possible FDC interrupt (again without 
checking DMA registers) and finally handle the possible SCSI interrupt.

The core 5380 driver knows whether or not it has started a DMA. The 
atari_scsi driver also knows that no other Falcon driver uses DMA. So the 
atari_scsi handler only has to figure out whether the interrupt was 
asserted by the ST-DMA chip or the 5380 chip, or neither. (The "neither" 
possility arises when IDE ditches the the stdma.c lock mechanism.)

Without the stdma.c lock, any or all of these interrupts could be asserted 
simultaneously, so the IDE and FDC drivers need to be able to do the right 
thing in the presence of the other interrupts and do so without accessing 
the ST-DMA chip. And the SCSI interrupt handler needs to do the right 
thing when there is no DMA interrupt, and yet a DMA is running.

Perhaps we could reverse the algorithm in scsi_falcon_intr(): if 
NCR5380_intr() completes with IRQ_HANDLED and the core 5380 driver is no 
longer in DMA, then check the ST-DMA registers for errors etc.

Alternatively, if NCR5380_intr() returns IRQ_NONE, then do nothing at all, 
on the basis that the interrupt was handled by FDC or IDE. In this 
situation, I gather that atari_scsi could miss out on a DMA completion 
interrupt from the ST-DMA chip, which could lead to command timeout?

> If DMA has been in progress, I'm not sure that we can figure out if it's 
> still active from looking at the status register (that is, whether bits 
> 0 or 1 are set while DMA is ongoing). We'd have to peek at the DMA 
> status register (or DMA address registers) without first stopping DMA, 
> which the current driver does. The docs seem to advise against that. If 
> DMA was in progress, stopping it would likely leave us with residual 
> bytes to be transferred -

I can't comment on the Profibuch doc or the ST-DMA chip details (Andreas?)

I suspect it has to be tried.

> we'd have to handle that transfer as we would any other DMA error (from 
> memory, probably best to retry the entire command, or transfer the 
> remaining bytes using PIO if we're sure no bytes have been lost).
> 

Allowing the command to fail should be fine so long as the 5380 driver 
sends the correct result code to the mid-layer. To attempt to complete the 
command after a failed DMA is needless complexity, and it's a trick that 
probably can't be pulled off reliably anyway.

-- 

> > If all else fails, perhaps we could inhibit DMA entirely when the new 
> > ATA driver is loaded. Then we can just dispatch the ST-DMA irq like a 
> > shared irq. I'm sure that atari_scsi can work without DMA. No idea 
> > about the FDC driver though (ataflop.c).
> 
> Yes, SCSI can work using PIO but's it a real dog. Been there, done that 
> (about 20 years ago). I know nothing about the FDC chip though.
> 
> > Another solution would be to dedicate the DMA function to atari_scsi, 
> > and then mask the FDC and IDE interrupts during each DMA transfer. But 
> > once again, this would mean changing the FDC driver to eliminate DMA, 
> > if that is possible. From the schematic it looks the the FDC chip, 
> > "AJAX", is another custom ... 
> > http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf
> > 
> > Unfortunately my grasp of the ST hardware reflects my inability to 
> > read German; those who can may want to take a look at "ATARI Profibuch 
> > ST-STE-TT.pdf".
> 
> I'll reread the ST-DMA description (and the FDC one).
> 
> Let me know if you think this could work ...
> 
> Cheers,
> 
> 	Michael
> 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,

Am 15.01.2017 um 12:47 schrieb Finn Thain:

> For the sake of discussion, I'll assume that the FDC driver will not
> be using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using
> the present locking mechanism, but it would not simplify things much:
> when IDE no longer participates in that locking mechanism then both
> FDC and SCSI drivers have to solve the same issues.)

Correct - IIRC the FDC registers are also accessible 'through' the
ST-DMA registers only so the same problem WRT DMA status arises.


> What compiler are you using, BTW? Are you still using the gcc-4.6.3
> m68k cross-compiler from kernel.org? I had to abandon it in order to
> get my SCSI driver patches to work.

4.6.3 is the version I still use. You had trouble with that one? I
recall some discussion on gcc versions on the m68k list a while back,
just never seemed to see any problems...

>> I need to think about that some more - if no DMA is in progress we
>> can safely peek at the SCSI registers. So the logic could be
>> changed to test for DMA operation first, and just try and service
>> the interrupt if DMA wasn't active.
>> 
> 
> OK, so based on the above, we handle the possible IDE interrupt
> (without checking DMA registers), handle the possible FDC interrupt
> (again without checking DMA registers) and finally handle the
> possible SCSI interrupt.

No, we can't check either FDC or SCSI interrupts (or indeed any chip
registers) without touching the ST-DMA. The moment we select a FDC or
SCSI register for read, DMA is terminated no questions asked.

> The core 5380 driver knows whether or not it has started a DMA. The 
> atari_scsi driver also knows that no other Falcon driver uses DMA. So
> the atari_scsi handler only has to figure out whether the interrupt
> was asserted by the ST-DMA chip or the 5380 chip, or neither. (The
> "neither" possility arises when IDE ditches the the stdma.c lock
> mechanism.)
> 
> Without the stdma.c lock, any or all of these interrupts could be
> asserted simultaneously, so the IDE and FDC drivers need to be able
> to do the right thing in the presence of the other interrupts and do
> so without accessing the ST-DMA chip. And the SCSI interrupt handler
> needs to do the right thing when there is no DMA interrupt, and yet a
> DMA is running.

Again, whenever DMA was running (and it might still be), we have to stop
it in order to look at FDC or SCSI registers. Utter braindamage, but

> Perhaps we could reverse the algorithm in scsi_falcon_intr(): if 
> NCR5380_intr() completes with IRQ_HANDLED and the core 5380 driver is
> no longer in DMA, then check the ST-DMA registers for errors etc.
> 
> Alternatively, if NCR5380_intr() returns IRQ_NONE, then do nothing at
> all, on the basis that the interrupt was handled by FDC or IDE.

We may only call NCR5380_intr() if DMA hadn't been active (or we are
sure it's completed, i.e. transfer address == end address. If that's
even possible). If the DMA is still ongoing, we have the choice of
punting (hoping for a command timeout to happen and clean up the mess),
or terminate DMA (by selecting the SCSI chip registers instead of the
DMA ones) and deal with the fallout.

> In this situation, I gather that atari_scsi could miss out on a DMA
> completion interrupt from the ST-DMA chip, which could lead to
> command timeout?

Doing nothing if DMA is enabled (and IDE had successfully handled an
interrupt!) would cause us to miss a stacked interrupt, yes. Timeout
would likely ensue. Not sure it's wise to kludge around that using a
watchdog timer activated in case we're not sure of the DMA completion
state...

> 
>> If DMA has been in progress, I'm not sure that we can figure out if
>> it's still active from looking at the status register (that is,
>> whether bits 0 or 1 are set while DMA is ongoing). We'd have to
>> peek at the DMA status register (or DMA address registers) without
>> first stopping DMA, which the current driver does. The docs seem to
>> advise against that. If DMA was in progress, stopping it would
>> likely leave us with residual bytes to be transferred -
> 
> I can't comment on the Profibuch doc or the ST-DMA chip details
> (Andreas?)
> 
> I suspect it has to be tried.

Yes, I fear I'll have to just try what happens if SCSI and IDE raise an
interrupt at the same time.

For now, polled IDE might be working well enough (haven't seen a huge
impact in IDE-only test workloads, I'll have to check impact on lots of
seeks across the whole disk a lot harder though). I need to recheck the
old IDE driver with my current combined test workload though (my second
4 GB Seagate disk has finally kicked the bucket, after the latest power
brownout).

> 
>> we'd have to handle that transfer as we would any other DMA error
>> (from memory, probably best to retry the entire command, or
>> transfer the remaining bytes using PIO if we're sure no bytes have
>> been lost).
>> 
> 
> Allowing the command to fail should be fine so long as the 5380
> driver sends the correct result code to the mid-layer. To attempt to
> complete the command after a failed DMA is needless complexity, and
> it's a trick that probably can't be pulled off reliably anyway.

Yep, we've been there before (when my CT60 caused SCSI DMA errors and
lost bytes). Not sure anymore what the correct result code was...

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Sun, 15 Jan 2017, Michael Schmitz wrote:

> Am 15.01.2017 um 12:47 schrieb Finn Thain:
> 
> > For the sake of discussion, I'll assume that the FDC driver will not 
> > be using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using 
> > the present locking mechanism, but it would not simplify things much: 
> > when IDE no longer participates in that locking mechanism then both 
> > FDC and SCSI drivers have to solve the same issues.)
> 
> Correct - IIRC the FDC registers are also accessible 'through' the 
> ST-DMA registers only so the same problem WRT DMA status arises.
> 

I had not considered that limitation.

> > What compiler are you using, BTW? Are you still using the gcc-4.6.3 
> > m68k cross-compiler from kernel.org? I had to abandon it in order to 
> > get my SCSI driver patches to work.
> 
> 4.6.3 is the version I still use. You had trouble with that one? I 
> recall some discussion on gcc versions on the m68k list a while back, 
> just never seemed to see any problems...
> 

... none that could be easily blamed on the compiler, anyway.

The gcc 4.6.3 issue affecting my builds was discussed back in 
November. There are alternative compilers available:
http://marc.info/?l=linux-m68k&m=147859596303294&w=2
http://marc.info/?l=linux-m68k&m=147859567903210&w=2

> >> I need to think about that some more - if no DMA is in progress we 
> >> can safely peek at the SCSI registers. So the logic could be changed 
> >> to test for DMA operation first, and just try and service the 
> >> interrupt if DMA wasn't active.
> >> 
> > 
> > OK, so based on the above, we handle the possible IDE interrupt 
> > (without checking DMA registers), handle the possible FDC interrupt 
> > (again without checking DMA registers) and finally handle the possible 
> > SCSI interrupt.
> 
> No, we can't check either FDC or SCSI interrupts (or indeed any chip 
> registers) without touching the ST-DMA. The moment we select a FDC or 
> SCSI register for read, DMA is terminated no questions asked.
> 

Perhaps we can convert DMA operations to PDMA (by polling with local irqs 
disabled) and avoid the whole problem of interrupt handlers executing 
during DMA transfers. The docs suggest that it is doable.

"Poll or service the Disk Driver Controller interrupt on the MK68901 MFP 
General Purpose I/O Register to detect the completion of a WD1772 FDC 
command. Do not poll the FDC Busy or DMA Sector Count Zero status bits." 
-- ST HW Spec, p. 36.
http://dev-docs.atariforge.org/files/ST_HW_Spec_1-7-1986.pdf

On page 18 there is an algorithm for floppy writes which is interesting.

I suspect that we will need to keep the FDC idle during SCSI transfers 
(and vice versa) much as the present stdma.c lock does.

"The interrupt outputs of the internal floppy disk controller and the 
external ACSI DMA port are logically OR'ed. The pin of the MFP GPIP will 
read as a '0' if either the FDC or a selected ACSI device controller is 
asserting its interrupt request."
-- ACSI/DMA Integration Guide, p.16.
http://dev-docs.atariforge.org/files/ACSI_DMA_Guide_6-28-1991.pdf

Polling the logically OR'ed interrupt sources to detect end-of-DMA will 
not be reliable unless we disable those sources that aren't relevant. 
Otherwise we access the DMA registers too early (which IIUC would kill the 
transfer). I'm afraid we shall have to expect that a few transfers will be 
interrupted by other devices in this way, and carefully check for this. 

For example, the 5380 SCSI bus reset interrupt is not maskable, which 
could affect FDC transfers. If this terminated the polling for DMA 
completion, the FDC driver then has to access the FDC registers and 
confirm that the transfer was not terminated early.

-- 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,

Am 15.01.2017 um 17:42 schrieb Finn Thain:

>> No, we can't check either FDC or SCSI interrupts (or indeed any chip 
>> registers) without touching the ST-DMA. The moment we select a FDC or 
>> SCSI register for read, DMA is terminated no questions asked.
>>
> 
> Perhaps we can convert DMA operations to PDMA (by polling with local irqs 
> disabled) and avoid the whole problem of interrupt handlers executing 
> during DMA transfers. The docs suggest that it is doable.
> 
> "Poll or service the Disk Driver Controller interrupt on the MK68901 MFP 
> General Purpose I/O Register to detect the completion of a WD1772 FDC 
> command. Do not poll the FDC Busy or DMA Sector Count Zero status bits." 
> -- ST HW Spec, p. 36.
> http://dev-docs.atariforge.org/files/ST_HW_Spec_1-7-1986.pdf

The MFP interrupt in question is the same as the one used by IDE
(wired-OR of IDE, FDC and SCSI), so we would still have to figure out
where the interrupt originated. Polling instead of taking the interrupt
does not change that fundamental problem (unless I'm missing something).

> 
> On page 18 there is an algorithm for floppy writes which is interesting.

That one (and the ACSI algorithm which would apply to SCSI for Falcon)
does suggest it won't be possible to peek at the sector count register
to detect end of DMA. The addendum (note 841017G) makes it clear that a
write to the DMA mode register is required to look at the status
register error bit (which might terminate DMA).

Maybe the DMA address register can be used to check for DMA completion
... it's used to check for residual or lost bytes anyway so that appears
to work. And the FDC driver does use the same strategy to check if
enough track data have been read.

Leaves the case where DMA hasn't completed but may have been aborted by
a NCR5380 interrupt. I suppose we can detect that by checking for any
change in the DMA address while repeatedly reading the DMA address
register. No change means the DMA has got stuck. Not exactly pretty but
all I can come up with.

> 
> I suspect that we will need to keep the FDC idle during SCSI transfers 
> (and vice versa) much as the present stdma.c lock does.
> 
> "The interrupt outputs of the internal floppy disk controller and the 
> external ACSI DMA port are logically OR'ed. The pin of the MFP GPIP will 
> read as a '0' if either the FDC or a selected ACSI device controller is 
> asserting its interrupt request."
> -- ACSI/DMA Integration Guide, p.16.
> http://dev-docs.atariforge.org/files/ACSI_DMA_Guide_6-28-1991.pdf

On Falcon, the IDE interrupt is also OR'ed to the above two interrupt
lines, hence the need for including IDE in the locking scheme there.

> 
> Polling the logically OR'ed interrupt sources to detect end-of-DMA will 
> not be reliable unless we disable those sources that aren't relevant. 
> Otherwise we access the DMA registers too early (which IIUC would kill the 
> transfer). I'm afraid we shall have to expect that a few transfers will be 
> interrupted by other devices in this way, and carefully check for this. 
> 
> For example, the 5380 SCSI bus reset interrupt is not maskable, which 
> could affect FDC transfers. If this terminated the polling for DMA 
> completion, the FDC driver then has to access the FDC registers and 
> confirm that the transfer was not terminated early.
> 

We'll have to make sure FDC and SCSI don't clash in their DMA and
interrupt use.

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Fri, 20 Jan 2017, Michael Schmitz wrote:

> Am 15.01.2017 um 17:42 schrieb Finn Thain:
> 
> >> No, we can't check either FDC or SCSI interrupts (or indeed any chip 
> >> registers) without touching the ST-DMA. The moment we select a FDC or 
> >> SCSI register for read, DMA is terminated no questions asked.
> >>
> > 
> > Perhaps we can convert DMA operations to PDMA (by polling with local 
> > irqs disabled) and avoid the whole problem of interrupt handlers 
> > executing during DMA transfers. The docs suggest that it is doable.
> > 
> > "Poll or service the Disk Driver Controller interrupt on the MK68901 
> > MFP General Purpose I/O Register to detect the completion of a WD1772 
> > FDC command. Do not poll the FDC Busy or DMA Sector Count Zero status 
> > bits." -- ST HW Spec, p. 36. 
> > http://dev-docs.atariforge.org/files/ST_HW_Spec_1-7-1986.pdf
> 
> The MFP interrupt in question is the same as the one used by IDE 
> (wired-OR of IDE, FDC and SCSI), so we would still have to figure out 
> where the interrupt originated.

I thought you said the driver you're testing does not use any interrupt -- 
I was assuming that only atari_scsi and ataflop drivers share the 
interupt.

> Polling instead of taking the interrupt does not change that fundamental 
> problem (unless I'm missing something).
> 

Actually, the fundamental problem you are describing is partly solved. By 
polling for DMA completion with local irqs disabled, we mostly avoid the 
need for the stdma.c "lock" because FDC/SCSI/IDE interrupt handlers can 
never interfere with a FDC/SCSI DMA process that might be underway.

> > 
> > On page 18 there is an algorithm for floppy writes which is 
> > interesting.
> 
> That one (and the ACSI algorithm which would apply to SCSI for Falcon) 
> does suggest it won't be possible to peek at the sector count register 
> to detect end of DMA. The addendum (note 841017G) makes it clear that a 
> write to the DMA mode register is required to look at the status 
> register error bit (which might terminate DMA).
> 
> Maybe the DMA address register can be used to check for DMA completion 
> ... it's used to check for residual or lost bytes anyway so that appears 
> to work. And the FDC driver does use the same strategy to check if 
> enough track data have been read.
> 
> Leaves the case where DMA hasn't completed but may have been aborted by 
> a NCR5380 interrupt. I suppose we can detect that by checking for any 
> change in the DMA address while repeatedly reading the DMA address 
> register. No change means the DMA has got stuck. Not exactly pretty but 
> all I can come up with.
> 

We don't have to poll any DMA registers (and I don't believe that it is 
viable to do so). I was talking about polling for end of DMA by polling 
for the interrupt (as per docs) but with local irqs disabled for the 
duration of the transfer (which provides exlusive access to the DMA chip).

> > 
> > I suspect that we will need to keep the FDC idle during SCSI transfers 
> > (and vice versa) much as the present stdma.c lock does.
> > 
> > "The interrupt outputs of the internal floppy disk controller and the 
> > external ACSI DMA port are logically OR'ed. The pin of the MFP GPIP 
> > will read as a '0' if either the FDC or a selected ACSI device 
> > controller is asserting its interrupt request." -- ACSI/DMA 
> > Integration Guide, p.16. 
> > http://dev-docs.atariforge.org/files/ACSI_DMA_Guide_6-28-1991.pdf
> 
> On Falcon, the IDE interrupt is also OR'ed to the above two interrupt 
> lines, hence the need for including IDE in the locking scheme there.
> 

I don't think the IDE/ATA driver needs to be included. atari_scsi and 
ataflop would though (if both drivers need DMA transfers).

> > 
> > Polling the logically OR'ed interrupt sources to detect end-of-DMA 
> > will not be reliable unless we disable those sources that aren't 
> > relevant. Otherwise we access the DMA registers too early (which IIUC 
> > would kill the transfer). I'm afraid we shall have to expect that a 
> > few transfers will be interrupted by other devices in this way, and 
> > carefully check for this.
> > 
> > For example, the 5380 SCSI bus reset interrupt is not maskable, which 
> > could affect FDC transfers. If this terminated the polling for DMA 
> > completion, the FDC driver then has to access the FDC registers and 
> > confirm that the transfer was not terminated early.
> > 
> 
> We'll have to make sure FDC and SCSI don't clash in their DMA and 
> interrupt use.
> 

The point I was trying to make above is that stdma lock only gets you so 
far: if SCSI or FDC generate an interrupt that can't be disabled, it could 
mess up the interrupt polling (and the interrupt polling is a necessary 
consequence of IDE operating without stdma lock). This would lead to a 
short transfer (which could be easily detected).

So the chips clash in their interrupt line use (rarely). The drivers need 
not clash at all.

Anyway, we seem to be talking past each other somewhat. I suggest we start 
coding and discuss actual patches ... unless you can convince me that this 
won't work ...

-- 

> Cheers,
> 
> 	Michael
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,

Am 21.01.2017 um 20:37 schrieb Finn Thain:

>> The MFP interrupt in question is the same as the one used by IDE 
>> (wired-OR of IDE, FDC and SCSI), so we would still have to figure out 
>> where the interrupt originated.
> 
> I thought you said the driver you're testing does not use any interrupt -- 
> I was assuming that only atari_scsi and ataflop drivers share the 
> interupt.

My mistake - I was considering options to allow IDE to share the
interrupt, without the complexities of the old locking scheme.

> 
>> Polling instead of taking the interrupt does not change that fundamental 
>> problem (unless I'm missing something).
>>
> 
> Actually, the fundamental problem you are describing is partly solved. By 
> polling for DMA completion with local irqs disabled, we mostly avoid the 
> need for the stdma.c "lock" because FDC/SCSI/IDE interrupt handlers can 
> never interfere with a FDC/SCSI DMA process that might be underway.

I hadn't considered that. Can PDMA for Falcon SCSI coexist with
interrupt-using DMA for TT SCSI in the same driver (i.e. as runtime
options)? How much overhead and latency would polling for DMA completion
add?

>> Maybe the DMA address register can be used to check for DMA completion 
>> ... it's used to check for residual or lost bytes anyway so that appears 
>> to work. And the FDC driver does use the same strategy to check if 
>> enough track data have been read.
>>
>> Leaves the case where DMA hasn't completed but may have been aborted by 
>> a NCR5380 interrupt. I suppose we can detect that by checking for any 
>> change in the DMA address while repeatedly reading the DMA address 
>> register. No change means the DMA has got stuck. Not exactly pretty but 
>> all I can come up with.
>>
> 
> We don't have to poll any DMA registers (and I don't believe that it is 
> viable to do so). I was talking about polling for end of DMA by polling 
> for the interrupt (as per docs) but with local irqs disabled for the 
> duration of the transfer (which provides exlusive access to the DMA chip).

atari_irq_pending(IRQ_MFP_FSCSI) should show the interrupt pending
condition if you want to poll for it. That's actually given me another
idea to pursue - if we can ensure the IDE interrupt handler is always
run first, and check whether the interrupt is still pending when the
SCSI or floppy interrupt handler runs and DMA has been in progress, we
should be able to avoid calling the respective handlers unnecessarily.

(The output of atari_irq_pending() does not directly reflect the status
of the MFP IRQ inputs - that would require testing bits in
st_mfp.par_dt_reg instead. )

> I don't think the IDE/ATA driver needs to be included. atari_scsi and 
> ataflop would though (if both drivers need DMA transfers).

If we manage to separate interrupt sharing from DMA access locking, IDE
would not need to take part in the locking. I'm assuming that IDE can
cope with spurious interrupts and won't get confused by a SCSI interrupt.

>>> For example, the 5380 SCSI bus reset interrupt is not maskable, which 
>>> could affect FDC transfers. If this terminated the polling for DMA 
>>> completion, the FDC driver then has to access the FDC registers and 
>>> confirm that the transfer was not terminated early.
>>>
>>
>> We'll have to make sure FDC and SCSI don't clash in their DMA and 
>> interrupt use.
>>
> 
> The point I was trying to make above is that stdma lock only gets you so 
> far: if SCSI or FDC generate an interrupt that can't be disabled, it could 
> mess up the interrupt polling (and the interrupt polling is a necessary 
> consequence of IDE operating without stdma lock). This would lead to a 
> short transfer (which could be easily detected).

Point taken - that problem still remains but is already being detected
(though not properly handled IIRC),

> So the chips clash in their interrupt line use (rarely). The drivers need 
> not clash at all.
> 
> Anyway, we seem to be talking past each other somewhat. I suggest we start 
> coding and discuss actual patches ... unless you can convince me that this 
> won't work ...

I think it could work both ways - polling for DMA completion or avoiding
to call the SCSI interrupt handler the interrupt was caused by IDE only.
But it's indeed time to put that to the test.

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Mon, 23 Jan 2017, Michael Schmitz wrote:

> 
> Am 21.01.2017 um 20:37 schrieb Finn Thain:
> 
> > 
> > Actually, the fundamental problem you are describing is partly solved. 
> > By polling for DMA completion with local irqs disabled, we mostly 
> > avoid the need for the stdma.c "lock" because FDC/SCSI/IDE interrupt 
> > handlers can never interfere with a FDC/SCSI DMA process that might be 
> > underway.
> 
> I hadn't considered that. Can PDMA for Falcon SCSI coexist with 
> interrupt-using DMA for TT SCSI in the same driver (i.e. as runtime 
> options)?

Sure, why not?

> How much overhead and latency would polling for DMA completion add?
> 

A polled DMA transfer should be faster than PDMA (i.e. mac_scsi, g_NCR5380 
etc). mac_scsi gets about 0.5 MBps from PDMA with sg_tablesize == 1, and I 
hope that DMA could get twice that (notwithstanding dumb hardware design). 

This would imply CPU overhead that is half of that which mac_scsi incurs. 
That's the best case, but I see no reason to expect worse performance than 
PDMA gets.

> atari_irq_pending(IRQ_MFP_FSCSI) should show the interrupt pending 
> condition if you want to poll for it.

The difficulty will be arranging for disabled FDC & IDE interrupt sources 
during SCSI DMA, and disabled SCSI & IDE interrupt sources during FDC DMA. 
(Not all 5380 interrupts can be disabled; no idea about the IDE device or 
WD1772 FDC.)

But if that is impossible, we just have to detect the short DMA that might 
result from an undesired interrupt.

> That's actually given me another idea to pursue - if we can ensure the 
> IDE interrupt handler is always run first,

There are no interrupts from the ATA driver you're testing, right? If you 
would re-introduce them, the whole polled DMA idea is moot.

> and check whether the interrupt is still pending when the SCSI or floppy 
> interrupt handler runs and DMA has been in progress, we should be able 
> to avoid calling the respective handlers unnecessarily.
> 
> (The output of atari_irq_pending() does not directly reflect the status 
> of the MFP IRQ inputs - that would require testing bits in 
> st_mfp.par_dt_reg instead. )
> 
> > I don't think the IDE/ATA driver needs to be included. atari_scsi and 
> > ataflop would though (if both drivers need DMA transfers).
> 
> If we manage to separate interrupt sharing from DMA access locking, IDE 
> would not need to take part in the locking. I'm assuming that IDE can 
> cope with spurious interrupts and won't get confused by a SCSI 
> interrupt.
> 

The ATA driver will never have to cope with a spurious interrupt under my 
simplifying assumptions discussed earlier, so the spurious interrupt 
question seems to belong to some alternative approach...

> I think it could work both ways - polling for DMA completion or avoiding 
> to call the SCSI interrupt handler the interrupt was caused by IDE only. 
> But it's indeed time to put that to the test.
> 

... "Both ways"? I don't follow. I don't see how IDE can share the FDC and 
SCSI interrupt line without sharing the stdma.c locking scheme. What is 
the alternative approach (i.e not polled DMA) that you alude to?

-- 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Geert Uytterhoeven]

Hi Finn,

On Thu, Jan 26, 2017 at 9:47 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> The difficulty will be arranging for disabled FDC & IDE interrupt sources
> during SCSI DMA, and disabled SCSI & IDE interrupt sources during FDC DMA.
> (Not all 5380 interrupts can be disabled; no idea about the IDE device or
> WD1772 FDC.)

IDE interrupts are disabled at the device level.
Unfortunately some hard drives (e.g. Western Digital Caviar) didn't honour
the ATA disable IRQ bit, so they caused an interrupt deadlock if you probed
for them on Amiga with the IDE interrupt enabled. The problem didn't show
up on PC because they had no shared interrupts, while on A4000 the IDE
interrupt is shared with Zorro Ethernet, which was still enabled.

That was fixed (in 1995 or 1996?) by disabling the IDE interrupt at the
IRQ controller level.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Thu, 26 Jan 2017, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Thu, Jan 26, 2017 at 9:47 AM, Finn Thain <fthain@telegraphics.com.au> 
> wrote:
> > The difficulty will be arranging for disabled FDC & IDE interrupt 
> > sources during SCSI DMA, and disabled SCSI & IDE interrupt sources 
> > during FDC DMA. (Not all 5380 interrupts can be disabled; no idea 
> > about the IDE device or WD1772 FDC.)
> 
> IDE interrupts are disabled at the device level. Unfortunately some hard 
> drives (e.g. Western Digital Caviar) didn't honour the ATA disable IRQ 
> bit, so they caused an interrupt deadlock if you probed for them on 
> Amiga with the IDE interrupt enabled. The problem didn't show up on PC 
> because they had no shared interrupts, while on A4000 the IDE interrupt 
> is shared with Zorro Ethernet, which was still enabled.
> 
> That was fixed (in 1995 or 1996?) by disabling the IDE interrupt at the 
> IRQ controller level.
> 

As I undersand it, masking these interrupts at the IRQ controller level 
won't work because these interrupt sources are all logically-OR'd together 
to generate the input that is to be polled during DMA. (And accessing the 
individual device registers is impossible during DMA!)

Anyway, the end result is that both IDE and SCSI have the potential to 
(occasionally) mess up the DMA polling during and FDC or SCSI transfer. 

For SCSI, I believe that we can detect this when it happens (by checking 
the sector count registers in the DMA chip) and return the SCSI command 
with an error result, so that the mid-layer will retry it.

I don't know how it would be handled in the case of an FDC transfer but I 
presume that a similar mechanism is available in the block layer.

-- 

> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,


Am 26.01.2017 um 21:47 schrieb Finn Thain:

>> I hadn't considered that. Can PDMA for Falcon SCSI coexist with 
>> interrupt-using DMA for TT SCSI in the same driver (i.e. as runtime 
>> options)?
> 
> Sure, why not?
> 
>> How much overhead and latency would polling for DMA completion add?
>>
> 
> A polled DMA transfer should be faster than PDMA (i.e. mac_scsi, g_NCR5380 
> etc). mac_scsi gets about 0.5 MBps from PDMA with sg_tablesize == 1, and I 
> hope that DMA could get twice that (notwithstanding dumb hardware design). 

DMA contends with the processor use of the data bus but that's true for
many DMA designs. Don't think Atari made any more dumb decisions (the
opaque DMA FIFO is probably the worst feature there but that could be
worked around at considerable expense by programming the DMA and the
SCSI bus transfer for one additional 512 byte block. Let's not go there.)

> This would imply CPU overhead that is half of that which mac_scsi incurs. 
> That's the best case, but I see no reason to expect worse performance than 
> PDMA gets.

But how much more overhead would we have compared to using the SCSI
interrupt to signal DMA completion?

>> atari_irq_pending(IRQ_MFP_FSCSI) should show the interrupt pending 
>> condition if you want to poll for it.
> 
> The difficulty will be arranging for disabled FDC & IDE interrupt sources 
> during SCSI DMA, and disabled SCSI & IDE interrupt sources during FDC DMA. 
> (Not all 5380 interrupts can be disabled; no idea about the IDE device or 
> WD1772 FDC.)
> 
> But if that is impossible, we just have to detect the short DMA that might 
> result from an undesired interrupt.

I think that's infeasible - IDE interrupts could be disabled at disk
level as Geert suggests but I don't think there is a kernel API for
other drivers to do so?

At the IRQ controller level, it's all or none due to the wired-OR design
as you pointed out in the reply to Geert's mail.

>> That's actually given me another idea to pursue - if we can ensure the 
>> IDE interrupt handler is always run first,
> 
> There are no interrupts from the ATA driver you're testing, right? If you 
> would re-introduce them, the whole polled DMA idea is moot.

The libata driver currently does disable the IDE interrupt and uses
polling, but I'd like to change that if at all possible. Sorry I didn't
make that clear.

As far as I could see during my testing, the current libata driver
coexists just fine with interrupt driven SCSI operation. I've once seen
a 'lost arbitration' message in the very first test when loading the
SCSI driver, but that's not been repeated. No problems seen otherwise.

>> If we manage to separate interrupt sharing from DMA access locking, IDE 
>> would not need to take part in the locking. I'm assuming that IDE can 
>> cope with spurious interrupts and won't get confused by a SCSI 
>> interrupt.
>>
> 
> The ATA driver will never have to cope with a spurious interrupt under my 
> simplifying assumptions discussed earlier, so the spurious interrupt 
> question seems to belong to some alternative approach...

I was assuming IDE could have interrupts reenabled in libata for that
discussion.

>> I think it could work both ways - polling for DMA completion or avoiding 
>> to call the SCSI interrupt handler the interrupt was caused by IDE only. 
>> But it's indeed time to put that to the test.
>>
> 
> ... "Both ways"? I don't follow. I don't see how IDE can share the FDC and 
> SCSI interrupt line without sharing the stdma.c locking scheme. What is 
> the alternative approach (i.e not polled DMA) that you alude to?

Since IDE does not use the ST-DMA and does not share any registers with
ST-DMA, peeking at the IDE status register in order to decide whether
the interrupt was raised by the IDE interface won't hurt the running DMA
process (regardless of whether FDC or SCSI started it). Nor will
servicing the IDE interrupt.

If at the end of the IDE interrupt processing the interrupt status is
cleared in the IDE interface, the interrupt line should go high again
before the IDE inthandler returns.

If we can ensure that the FDC/SCSI interrupt handler runs after the IDE
handler, we can then make that handler check the interrupt line status
and bail out if there's nothing to be done. (For the sake of simplicity,
this check can be done in stdma_int() since we need to retain mutual
locking of the DMA interface by SCSI and FDC anyway.) We can ensure the
IDE interrupt is called first by using a special interrupt controller to
register separate IDE and FDC/SCSI interrupts with (I've done that to
provide distinct interrupt numbers and handlers for the timer D
interrupt that's used to poll ethernet and USB interface status on the
ROM port).

That way, we can ensure IDE interrupts do not step on the ST-DMA state,
and all that remains are premature SCSI interrupts terminating DMA
transfer (which we already face anyway).

Am I missing a potential race here? Does IDE send the next request off
to the disk from inside the interrupt handler so we could see IDE
immediately raise the next interrupt?  In that case, we'd also need to
check the IDE interrupt status in the interface status register, and
bail out for another pass through the IDE/FDC/SCSI handlers until IDE is
no longer posting an interrupt...

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Finn Thain]

On Fri, 27 Jan 2017, Michael Schmitz wrote:

> Am 26.01.2017 um 21:47 schrieb Finn Thain:
> 
> > This would imply CPU overhead that is half of that which mac_scsi 
> > incurs. That's the best case, but I see no reason to expect worse 
> > performance than PDMA gets.
> 
> But how much more overhead would we have compared to using the SCSI 
> interrupt to signal DMA completion?
> 

I imagine that contention for the CPU bus would be a problem if we polled 
the interrupt flag without any delay between iterations. With a small 
delay I think the overhead would be comparable with PDMA and therefore 
tolerable.

> The libata driver currently does disable the IDE interrupt and uses 
> polling, but I'd like to change that if at all possible. Sorry I didn't 
> make that clear.
> 

[snip]

> Since IDE does not use the ST-DMA and does not share any registers with 
> ST-DMA, peeking at the IDE status register in order to decide whether 
> the interrupt was raised by the IDE interface won't hurt the running DMA 
> process (regardless of whether FDC or SCSI started it). Nor will 
> servicing the IDE interrupt.
> 

Maybe we can just call the IDE handler from the ST-DMA handler regardless 
of the status register. For a shared interrupt handler this should work 
okay. (BTW, where is the IDE status register found anyway?)

> If at the end of the IDE interrupt processing the interrupt status is 
> cleared in the IDE interface, the interrupt line should go high again 
> before the IDE inthandler returns.
> 

On page 2 of the schematic, MFP pin I5 is wired to the output of the 
logical OR combination of the IDEIRQ and XDISKINT signals (actually 
active-low signals fed into an AND gate). The pin is edge-triggered.

This is just like the wired-OR Nubus slot IRQs connected to the Mac's VIA 
pin. The handler must ack all asserted IRQs. Otherwise there will be no 
more edges and no more interrupts.

This means looping over the IDE, FDC/SCSI DMA handlers until they all 
return IRQ_NONE. (Or equivalently, looping over the IRQ flags in the 
device registers until they are all de-asserted.)

BTW, this makes me think that the stdma.c mechanism is already flawed, 
since stdma_int() can cause only one of IDEIRQ and XDISKINT to become 
inactive, but not both. That's fine as long as no device raises IRQ until 
it's driver acquires the stdma lock -- but we know this is not true for 
the 5380 bus reset interrupt and it isn't true for IDE devices either 
(based on Geert's email in this thread).

> If we can ensure that the FDC/SCSI interrupt handler runs after the IDE 
> handler, we can then make that handler check the interrupt line status 
> and bail out if there's nothing to be done. (For the sake of simplicity, 
> this check can be done in stdma_int() since we need to retain mutual 
> locking of the DMA interface by SCSI and FDC anyway.)
> 
> We can ensure the IDE interrupt is called first by using a special 
> interrupt controller to register separate IDE and FDC/SCSI interrupts 
> with (I've done that to provide distinct interrupt numbers and handlers 
> for the timer D interrupt that's used to poll ethernet and USB interface 
> status on the ROM port).
> 
> That way, we can ensure IDE interrupts do not step on the ST-DMA state, 
> and all that remains are premature SCSI interrupts terminating DMA 
> transfer (which we already face anyway).
> 
> Am I missing a potential race here? Does IDE send the next request off 
> to the disk from inside the interrupt handler so we could see IDE 
> immediately raise the next interrupt?  In that case, we'd also need to 
> check the IDE interrupt status in the interface status register, and 
> bail out for another pass through the IDE/FDC/SCSI handlers until IDE is 
> no longer posting an interrupt...
> 

I don't know anything about IDE so I can't comment on this particular 
scenario (IDE interrupt handler causing IDE interrupt). The race condition 
may be only theoretical.

What you seem to be aiming at is an algorithm to ensure that no DMA 
interrupt is handled whilst an IDE interrupt is pending. Taking into 
account the logical OR issue, one could imagine a handler for the 
IRQ_MFP_FSCSI interrupt something like the following. (This code is 
probably useless for implementing your interrupt controller, but I hope it 
illustrates some of the issues.)

	do {
		handled = ata_handler(irq, ata_dev);
		if (handled == IRQ_NONE && atari_irq_pending(IRQ_MFP_FSCSI))
			handled |= stdma_int(irq, stdma_dev);
	} while (handled != IRQ_NONE);

Clearly this is not free from race conditions. The other problem is the 
use of atari_irq_pending(IRQ_MFP_FSCSI). It tells us when an edge appears 
but doesn't tell us about the present state of the IRQ output pins on the 
NCR5380 or the WD1772. We can't access the device registers so 
st_mfp.par_dt_reg & BIT(5) must be used instead of 
atari_irq_pending(IRQ_MFP_FSCSI);

The simplest approach is to treat it like a shared interrupt, with a loop 
to account for the logical OR:

	do {
		handled = ata_handler(irq, ata_dev) | 
		          scsi_falcon_intr(irq, scsi_dev) |
		          fdc_handler(irq, fdc_dev);
	} while (handled != IRQ_NONE);

This should work fine with polled DMA (and might even allow the flawed 
stdma.c lock mechanism to be eliminated) but it can't work with your 
scheme because scsi_falcon_intr() assumes exclusive access to the IRQ; 
hence it must not be called unless there is an actual 5380 or DMA 
interrupt.

I don't know which scheme is better. Mine is simpler and probably free of 
race conditions but does burn some CPU time. Your scheme is more 
complicated.

-- 

> Cheers,
> 
> 	Michael
> 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Geert Uytterhoeven]

Hi Finn,

On Wed, Feb 1, 2017 at 9:40 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> okay. (BTW, where is the IDE status register found anyway?)

In the IDE device.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,


Am 01.02.2017 um 21:40 schrieb Finn Thain:
> 
> On Fri, 27 Jan 2017, Michael Schmitz wrote:
> 
>> Am 26.01.2017 um 21:47 schrieb Finn Thain:
>>
>>> This would imply CPU overhead that is half of that which mac_scsi 
>>> incurs. That's the best case, but I see no reason to expect worse 
>>> performance than PDMA gets.
>>
>> But how much more overhead would we have compared to using the SCSI 
>> interrupt to signal DMA completion?
>>
> 
> I imagine that contention for the CPU bus would be a problem if we polled 
> the interrupt flag without any delay between iterations. With a small 
> delay I think the overhead would be comparable with PDMA and therefore 
> tolerable.

The first delay can even be quite long, estimated based on the transfer
size.

>> Since IDE does not use the ST-DMA and does not share any registers with 
>> ST-DMA, peeking at the IDE status register in order to decide whether 
>> the interrupt was raised by the IDE interface won't hurt the running DMA 
>> process (regardless of whether FDC or SCSI started it). Nor will 
>> servicing the IDE interrupt.
>>
> 
> Maybe we can just call the IDE handler from the ST-DMA handler regardless 
> of the status register. For a shared interrupt handler this should work 
> okay. (BTW, where is the IDE status register found anyway?)

We could do that as well, true. Easier to implement on the quick.

>> If at the end of the IDE interrupt processing the interrupt status is 
>> cleared in the IDE interface, the interrupt line should go high again 
>> before the IDE inthandler returns.
>>
> 
> On page 2 of the schematic, MFP pin I5 is wired to the output of the 
> logical OR combination of the IDEIRQ and XDISKINT signals (actually 
> active-low signals fed into an AND gate). The pin is edge-triggered.
> 
> This is just like the wired-OR Nubus slot IRQs connected to the Mac's VIA 
> pin. The handler must ack all asserted IRQs. Otherwise there will be no 
> more edges and no more interrupts.

Quite right - that's why I mentioned monitoring the IRQ signal status in
the GPIO register.

> This means looping over the IDE, FDC/SCSI DMA handlers until they all 
> return IRQ_NONE. (Or equivalently, looping over the IRQ flags in the 
> device registers until they are all de-asserted.)

Looping over the handlers risks stopping the DMA without need (except
for IDE).

> BTW, this makes me think that the stdma.c mechanism is already flawed, 
> since stdma_int() can cause only one of IDEIRQ and XDISKINT to become 
> inactive, but not both. That's fine as long as no device raises IRQ until 
> it's driver acquires the stdma lock -- but we know this is not true for 
> the 5380 bus reset interrupt and it isn't true for IDE devices either 
> (based on Geert's email in this thread).

The initial bus reset code had safeguards against raising an interrupt -
the IRQ was 'turned off' while the bus reset was executed. Maybe we need
something like that, at least in the case where SCSI does not hold the
ST-DMA lock.

>> If we can ensure that the FDC/SCSI interrupt handler runs after the IDE 
>> handler, we can then make that handler check the interrupt line status 
>> and bail out if there's nothing to be done. (For the sake of simplicity, 
>> this check can be done in stdma_int() since we need to retain mutual 
>> locking of the DMA interface by SCSI and FDC anyway.)
>>
>> We can ensure the IDE interrupt is called first by using a special 
>> interrupt controller to register separate IDE and FDC/SCSI interrupts 
>> with (I've done that to provide distinct interrupt numbers and handlers 
>> for the timer D interrupt that's used to poll ethernet and USB interface 
>> status on the ROM port).
>>
>> That way, we can ensure IDE interrupts do not step on the ST-DMA state, 
>> and all that remains are premature SCSI interrupts terminating DMA 
>> transfer (which we already face anyway).
>>
>> Am I missing a potential race here? Does IDE send the next request off 
>> to the disk from inside the interrupt handler so we could see IDE 
>> immediately raise the next interrupt?  In that case, we'd also need to 
>> check the IDE interrupt status in the interface status register, and 
>> bail out for another pass through the IDE/FDC/SCSI handlers until IDE is 
>> no longer posting an interrupt...
>>
> 
> I don't know anything about IDE so I can't comment on this particular 
> scenario (IDE interrupt handler causing IDE interrupt). The race condition 
> may be only theoretical.
> 
> What you seem to be aiming at is an algorithm to ensure that no DMA 
> interrupt is handled whilst an IDE interrupt is pending. Taking into 

Whilst the IDE interrupt is the only one pending, to be precise.

> account the logical OR issue, one could imagine a handler for the 
> IRQ_MFP_FSCSI interrupt something like the following. (This code is 
> probably useless for implementing your interrupt controller, but I hope it 
> illustrates some of the issues.)
> 
> 	do {
> 		handled = ata_handler(irq, ata_dev);
> 		if (handled == IRQ_NONE && atari_irq_pending(IRQ_MFP_FSCSI))
> 			handled |= stdma_int(irq, stdma_dev);
> 	} while (handled != IRQ_NONE);

Not quite - we can't be certain that there aren't actually two
interrupts pending (say IDE interrupts first, and while we service that
interrupt, SCSI interrupts next but since the IRQ signal remains low, we
don't trigger another interrupt). So we must run the loop for both IDE
and stdma_int for as long as the IRQ signal remains low.

I had missed the case where SCSI interrupts first and the IDE interrupt
only comes in while the SCSI inthandler is running - need to think a bit
more about this.

> Clearly this is not free from race conditions. The other problem is the 
> use of atari_irq_pending(IRQ_MFP_FSCSI). It tells us when an edge appears 
> but doesn't tell us about the present state of the IRQ output pins on the 
> NCR5380 or the WD1772. We can't access the device registers so 
> st_mfp.par_dt_reg & BIT(5) must be used instead of 
> atari_irq_pending(IRQ_MFP_FSCSI);

I've written a function to that effect, and tested it in the current
Falcon inthandler (without sharing the interrupt truly yet).

> The simplest approach is to treat it like a shared interrupt, with a loop 
> to account for the logical OR:
> 
> 	do {
> 		handled = ata_handler(irq, ata_dev) | 
> 		          scsi_falcon_intr(irq, scsi_dev) |
> 		          fdc_handler(irq, fdc_dev);
> 	} while (handled != IRQ_NONE);

We can never have both SCSI and FDC interrupt at the same time (both
register access and DMA setup are shared between the two). Best retain
stdma_lock() for these two.

> This should work fine with polled DMA (and might even allow the flawed 
> stdma.c lock mechanism to be eliminated) but it can't work with your 
> scheme because scsi_falcon_intr() assumes exclusive access to the IRQ; 
> hence it must not be called unless there is an actual 5380 or DMA 
> interrupt.

Correct - polled DMA might be easier to handle here.

> I don't know which scheme is better. Mine is simpler and probably free of 
> race conditions but does burn some CPU time. Your scheme is more 
> complicated.

Meaning likely to race...

I'll have to test this with IDE removed from the locking scheme, and
we'll hopefully see races pretty quick.

Got a few new ideas to try now, thanks!

Cheers,

	Michael

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Tejun Heo]

On Fri, Dec 30, 2016 at 03:01:15PM +0100, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> This patchset adds m68k/Atari Falcon PATA support to libata.
> The major difference in the new libata's pata_falcon host
> driver when compared to legacy IDE's falconide host driver is
> that we are using polled PIO mode and thus avoiding the need
> for STDMA locking magic altogether.
> 
> Tested under ARAnyM emulator.

Applied 1-3 to libata/for-4.11.

Thanks.

-- 
tejun

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Geert Uytterhoeven]

On Fri, Dec 30, 2016 at 3:01 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> This patchset adds m68k/Atari Falcon PATA support to libata.
> The major difference in the new libata's pata_falcon host
> driver when compared to legacy IDE's falconide host driver is
> that we are using polled PIO mode and thus avoiding the need
> for STDMA locking magic altogether.
>
> Tested under ARAnyM emulator.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
> Bartlomiej Zolnierkiewicz (3):
>   ata: allow subsystem to be used on m68k arch
>   ata: pass queued command to ->sff_data_xfer method
>   ata: add Atari Falcon PATA controller driver

drivers/ata/pata_falcon.c:57:18: error: 'struct request' has no member
named 'cmd_type'
drivers/ata/pata_falcon.c:57:32: error: 'REQ_TYPE_FS' undeclared
(first use in this function)

http://kisskb.ellerman.id.au/kisskb/buildresult/12936876/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, February 15, 2017 09:45:53 AM Geert Uytterhoeven wrote:
> On Fri, Dec 30, 2016 at 3:01 PM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
> > This patchset adds m68k/Atari Falcon PATA support to libata.
> > The major difference in the new libata's pata_falcon host
> > driver when compared to legacy IDE's falconide host driver is
> > that we are using polled PIO mode and thus avoiding the need
> > for STDMA locking magic altogether.
> >
> > Tested under ARAnyM emulator.
> >
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> >
> > Bartlomiej Zolnierkiewicz (3):
> >   ata: allow subsystem to be used on m68k arch
> >   ata: pass queued command to ->sff_data_xfer method
> >   ata: add Atari Falcon PATA controller driver
> 
> drivers/ata/pata_falcon.c:57:18: error: 'struct request' has no member
> named 'cmd_type'
> drivers/ata/pata_falcon.c:57:32: error: 'REQ_TYPE_FS' undeclared
> (first use in this function)
> 
> http://kisskb.ellerman.id.au/kisskb/buildresult/12936876/

From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: [PATCH] pata_falcon: build fix for block layer changes

commit aebf526b53ae ("block: fold cmd_type into the REQ_OP_
space") from the block tree removes cmd_type so pata_falcon
needs the following trivial update to make it build again.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
Tejun, I guess that you may need to fold this fix into
your pull request for 4.11 (block layer pull request has
been already sent by Jens so it will be most likely merged
first).

 drivers/ata/pata_falcon.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/drivers/ata/pata_falcon.c
===================================================================
--- a/drivers/ata/pata_falcon.c	2017-02-20 18:40:04.174989455 +0100
+++ b/drivers/ata/pata_falcon.c	2017-02-20 18:42:59.482993870 +0100
@@ -54,7 +54,7 @@ static unsigned int pata_falcon_data_xfe
 	bool swap = 1;
 
 	if (dev->class == ATA_DEV_ATA && cmd && cmd->request &&
-	    cmd->request->cmd_type == REQ_TYPE_FS)
+	    !blk_rq_is_passthrough(cmd->request))
 		swap = 0;
 
 	/* Transfer multiple of 2 bytes */

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Tejun Heo]

On Mon, Feb 20, 2017 at 07:15:34PM +0100, Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Subject: [PATCH] pata_falcon: build fix for block layer changes
> 
> commit aebf526b53ae ("block: fold cmd_type into the REQ_OP_
> space") from the block tree removes cmd_type so pata_falcon
> needs the following trivial update to make it build again.
> 
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> Tejun, I guess that you may need to fold this fix into
> your pull request for 4.11 (block layer pull request has
> been already sent by Jens so it will be most likely merged
> first).
> 
>  drivers/ata/pata_falcon.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/drivers/ata/pata_falcon.c
> ===================================================================
> --- a/drivers/ata/pata_falcon.c	2017-02-20 18:40:04.174989455 +0100
> +++ b/drivers/ata/pata_falcon.c	2017-02-20 18:42:59.482993870 +0100
> @@ -54,7 +54,7 @@ static unsigned int pata_falcon_data_xfe
>  	bool swap = 1;
>  
>  	if (dev->class == ATA_DEV_ATA && cmd && cmd->request &&
> -	    cmd->request->cmd_type == REQ_TYPE_FS)
> +	    !blk_rq_is_passthrough(cmd->request))

Sent it along with the pull request.

Thanks.

-- 
tejun