Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
On Wed, Jul 2, 2008 at 6:30 PM, Grant Likely [EMAIL PROTECTED] wrote:
I know that only ATA uses this; but it is nice to have fixes to things
that are obviously wrong in existing code to be split into their own
patches. That way, even if the ATA patch gets backed out, the bug fix
will remain.
Ok, so I've split the patch up into two pieces now...
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc-ap;
+ struct mpc52xx_ata_priv *priv = ap-host-private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, pata_mpc52xx_ata_dma_lock) != 0)
+ ;
Need to be able to bail on timeout.
A deadlock can't occur within the PATA driver because you won't have
two DMA requests happening at once, so there is no point in adding a
timeout. And even if you do have a timeout, you'd have to drop the I/O
request somehow, so it's not really a good idea. If anything else
needs to touch the DMA lock, it should do so in a sensible fashion...
Thanks,
Tim
1) ata.h has dst_pa in the wrong place (needs to match what the BestComm
task microcode in bcom_ata_task.c expects); fix it.
2) The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
this fixes a deadlock issue I was experiencing when heavy DMA was
occuring on both the ATA and Ethernet BestComm tasks, e.g. when
downloading a large file over a LAN to disk.
3) The ATA BestComm driver uses bcom_ata_bd which is bigger than bcom_bd
and this causes problems because the various bcom_... functions do not
dereference the correct location. I've introduced bcom_get_bd which
uses bcom_task.bd_size and this fixes the problem.
Signed-off-by: Tim Yamin [EMAIL PROTECTED]
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h 2008-07-03 16:17:05.0 +0100
@@ -16,8 +16,8 @@
struct bcom_ata_bd {
u32 status;
- u32 dst_pa;
u32 src_pa;
+ u32 dst_pa;
};
extern struct bcom_task *
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-07-03 16:17:05.0 +0100
@@ -38,7 +38,7 @@ struct bcom_task {
unsigned int flags;
int irq;
- struct bcom_bd *bd;
+ void *bd;
phys_addr_t bd_pa;
void **cookie;
unsigned short index;
@@ -140,15 +140,29 @@ bcom_queue_full(struct bcom_task *tsk)
}
/**
+ * bcom_get_bd - Get a BD from the queue
+ * @tsk: The BestComm task structure
+ * index: Index of the BD to fetch
+ */
+static inline struct bcom_bd
+*bcom_get_bd(struct bcom_task *tsk, unsigned int index)
+{
+ return tsk-bd + index * tsk-bd_size;
+}
+
+/**
* bcom_buffer_done - Checks if a BestComm
* @tsk: The BestComm task structure
*/
static inline int
bcom_buffer_done(struct bcom_task *tsk)
{
+ struct bcom_bd *bd;
if (bcom_queue_empty(tsk))
return 0;
- return !(tsk-bd[tsk-outdex].status BCOM_BD_READY);
+
+ bd = bcom_get_bd(tsk, tsk-outdex);
+ return !(bd-status BCOM_BD_READY);
}
/**
@@ -160,16 +174,21 @@ bcom_buffer_done(struct bcom_task *tsk)
static inline struct bcom_bd *
bcom_prepare_next_buffer(struct bcom_task *tsk)
{
- tsk-bd[tsk-index].status = 0; /* cleanup last status */
- return tsk-bd[tsk-index];
+ struct bcom_bd *bd;
+
+ bd = bcom_get_bd(tsk, tsk-index);
+ bd-status = 0; /* cleanup last status */
+ return bd;
}
static inline void
bcom_submit_next_buffer(struct bcom_task *tsk, void *cookie)
{
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk-index);
+
tsk-cookie[tsk-index] = cookie;
mb(); /* ensure the bd is really up-to-date */
- tsk-bd[tsk-index].status |= BCOM_BD_READY;
+ bd-status |= BCOM_BD_READY;
tsk-index = _bcom_next_index(tsk);
if (tsk-flags BCOM_FLAGS_ENABLE_TASK)
bcom_enable(tsk);
@@ -179,10 +198,12 @@ static inline void *
bcom_retrieve_buffer(struct bcom_task *tsk, u32 *p_status, struct bcom_bd **p_bd)
{
void *cookie = tsk-cookie[tsk-outdex];
+ struct bcom_bd *bd = bcom_get_bd(tsk, tsk-outdex);
+
if (p_status)
- *p_status = tsk-bd[tsk-outdex].status;
+ *p_status = bd-status;
if (p_bd)
- *p_bd = tsk-bd[tsk-outdex];
+ *p_bd = bd;
tsk-outdex = _bcom_next_outdex(tsk);
return cookie;
}
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h 2008-07-03 16:17:05.0 +0100
@@ -198,8 +198,8 @@ struct bcom_task_header {
#define BCOM_IPR_SCTMR_1 2
#define BCOM_IPR_FEC_RX 6
#define
Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
On Thu, 2008-07-03 at 16:35 +0100, Tim Yamin wrote:
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc-ap;
+ struct mpc52xx_ata_priv *priv = ap-host-private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, pata_mpc52xx_ata_dma_lock) != 0)
+ ;
Need to be able to bail on timeout.
A deadlock can't occur within the PATA driver because you won't have
two DMA requests happening at once, so there is no point in adding a
timeout. And even if you do have a timeout, you'd have to drop the I/O
request somehow, so it's not really a good idea. If anything else
needs to touch the DMA lock, it should do so in a sensible fashion...
But why a hand-coded lock with bitops ? Why not a real spinlock then ?
The later is more efficient anyway.
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
On Fri, Jul 04, 2008 at 09:47:03AM +1000, Benjamin Herrenschmidt wrote:
On Thu, 2008-07-03 at 16:35 +0100, Tim Yamin wrote:
+static void
+mpc52xx_bmdma_start(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc-ap;
+ struct mpc52xx_ata_priv *priv = ap-host-private_data;
+
+ /* LocalBus lock */
+ while (test_and_set_bit(0, pata_mpc52xx_ata_dma_lock) != 0)
+ ;
Need to be able to bail on timeout.
A deadlock can't occur within the PATA driver because you won't have
two DMA requests happening at once, so there is no point in adding a
timeout. And even if you do have a timeout, you'd have to drop the I/O
request somehow, so it's not really a good idea. If anything else
needs to touch the DMA lock, it should do so in a sensible fashion...
But why a hand-coded lock with bitops ? Why not a real spinlock then ?
The later is more efficient anyway.
Umm, yeah (don't know why I didn't clue into that). This definitely
should be a spinlock.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
On Thu, Jul 03, 2008 at 04:35:27PM +0100, Tim Yamin wrote:
There's also what I believe to be a hardware bug if you have high levels
of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
the address bus seems to sometimes get corrupted with ATA commands while
the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
I've asked Freescale about this but have not received a reply yet -- if
anybody from Freescale has any ideas please contact me; I can supply some
analyzer traces if needed. Therefore, for now, do not enable DMA if you
need reliable LocalPlus Bus unless you do a fixup in your driver as
follows:
Locking example:
while (test_and_set_bit(0, pata_mpc52xx_ata_dma_lock) != 0)
{
struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
if(bcom_buffer_done_2(tsk))
return 1;
}
return 0;
(Save the return value to `flags`)
Unlocking example:
if(flags == 0)
clear_bit(0, pata_mpc52xx_ata_dma_lock);
One more thing. For this description to be of any use, it needs to be
documented where somebody will actually see it. The commit message is
great for describing *why* a change is needed, but it's not seen
so much after the change is merged.
This description should probably be added to the header comment block of
the ATA driver, and it should be talked about in the Kconfig help text
too.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
Hi Grant,
Thanks for the feedback. New version is attached.
Is this a bug fix? If so, please put it into a separate patch.
I suppose so, yes. If Ethernet has higher priority than ATA, you can
get a deadlock if you try and download a large file over a LAN to
disk, for example. But given that nothing other than this patch uses
BestComm for ATA do you have any specific reason to split it out into
another patch?
Good, it can be turned off. Do you think there is any risk to existing
ATA users with this patch applied if this is turned off?
With the new version, the only risk if this is turned off is the
change I've made to bestcomm.h. Other than that, you should get no
risk because none of the new
code is executed (mwdma_mask and udma_mask are set to 0 if the option
is turned off).
Can you find any way to avoid this? This could be a performance drain.
Previous code had this, so I kept it. Things do seem to work OK
without it, so I've removed it...
Is there any way to turn on/off DMA at runtime instead of CONFIG time?
You could use libata.dma=0 to force DMA off even if it's enabled at CONFIG time.
priv-ipb_period = 10 / (ipb_freq / 1000);
priv-ata_regs = ata_regs;
+ priv-ata_regs_pa = (struct mpc52xx_ata __iomem *) res_mem.start;
I'm not fond of this. First off, it is *not* __iomem. It is physical
address. It would be better to use the offset_of macro to add an offset
to the physical base address. Doing it this way forces you to cast and
sidestep the compile time checks for incorrect dereferences.
I'm afraid I'm not quite sure what you have in mind here, could you
please provide a pointer?
Thanks,
Tim
This patch adds MDMA/UDMA support (using BestComm for DMA) on the MPC5200
platform.
Based heavily on previous work by Freescale (Bernard Kuhn, John Rigby)
and Domen Puncer.
Using a SanDisk Extreme IV CF card I get read speeds of approximately
26.70 MB/sec.
The BestComm ATA task priority was changed to maximum in bestcomm_priv.h;
this fixes a deadlock issue I was experiencing when heavy DMA was
occuring on both the ATA and Ethernet BestComm tasks, e.g. when
downloading a large file over a LAN to disk.
There's also what I believe to be a hardware bug if you have high levels
of BestComm ATA DMA activity along with heavy LocalPlus Bus activity;
the address bus seems to sometimes get corrupted with ATA commands while
the LocalPlus Bus operation is still active (i.e. Chip Select is asserted).
I've asked Freescale about this but have not received a reply yet -- if
anybody from Freescale has any ideas please contact me; I can supply some
analyzer traces if needed. Therefore, for now, do not enable DMA if you
need reliable LocalPlus Bus unless you do a fixup in your driver as
follows:
Locking example:
while (test_and_set_bit(0, pata_mpc52xx_ata_dma_lock) != 0)
{
struct bcom_task_2 *tsk = pata_mpc52xx_ata_dma_task;
if(bcom_buffer_done_2(tsk))
return 1;
}
return 0;
(Save the return value to `flags`)
Unlocking example:
if(flags == 0)
clear_bit(0, pata_mpc52xx_ata_dma_lock);
Comments and testing would of course be very welcome.
Thanks,
Signed-off-by: Tim Yamin [EMAIL PROTECTED]
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/ata.h 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/ata.h 2008-07-02 12:48:14.0 +0100
@@ -16,8 +16,8 @@
struct bcom_ata_bd {
u32 status;
- u32 dst_pa;
u32 src_pa;
+ u32 dst_pa;
};
extern struct bcom_task *
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-07-02 12:48:14.0 +0100
@@ -330,11 +330,16 @@ bcom_engine_init(void)
/* Init 'always' initiator */
out_8(bcom_eng-regs-ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
+ /* If ATA DMA is enabled, always turn prefetch off (it breaks things) */
+#ifndef CONFIG_PATA_MPC52xx_DMA
/* Disable COMM Bus Prefetch on the original 5200; it's broken */
if ((mfspr(SPRN_SVR) MPC5200_SVR_MASK) == MPC5200_SVR) {
+#endif
regval = in_be16(bcom_eng-regs-PtdCntrl);
out_be16(bcom_eng-regs-PtdCntrl, regval | 1);
+#ifndef CONFIG_PATA_MPC52xx_DMA
}
+#endif
/* Init lock */
spin_lock_init(bcom_eng-lock);
diff -urp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-04-17 03:49:44.0 +0100
+++ linux-2.6.26-rc6-ata/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-07-02 12:48:14.0 +0100
@@ -140,15 +140,29 @@
Re: [PATCH]: [MPC5200] (v2) Add ATA DMA support
On Fri, Jun 27, 2008 at 01:44:08PM +0100, Tim Yamin wrote:
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c
linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.c 2008-03-18
15:49:53.0 +
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.c
2008-04-15 10:42:38.0 +0100
@@ -330,11 +330,10 @@
/* Init 'always' initiator */
out_8(bcom_eng-regs-ipr[BCOM_INITIATOR_ALWAYS], BCOM_IPR_ALWAYS);
- /* Disable COMM Bus Prefetch on the original 5200; it's broken */
- if ((mfspr(SPRN_SVR) MPC5200_SVR_MASK) == MPC5200_SVR) {
- regval = in_be16(bcom_eng-regs-PtdCntrl);
- out_be16(bcom_eng-regs-PtdCntrl, regval | 1);
- }
+ /* Disable COMM Bus Prefetch; ATA DMA does not work properly with it
+enabled. */
+ regval = in_be16(bcom_eng-regs-PtdCntrl);
+ out_be16(bcom_eng-regs-PtdCntrl, regval | 1);
This could have a performance impact on existing systems that don't
use/need ATA DMA.
Please run next version through checkpatch. There's lots of minor
cosmetic stuff, but there are also a few important errors.
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h
linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm.h 2008-03-18
15:49:53.0 +
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm.h
2008-04-15 10:42:38.0 +0100
@@ -17,6 +17,7 @@
#define __BESTCOMM_H__
struct bcom_bd; /* defined later on ... */
+struct bcom_bd_2;
/*
*/
@@ -49,6 +50,22 @@
void* priv;
};
+struct bcom_task_2 {
+ unsigned inttasknum;
+ unsigned intflags;
+ int irq;
+
+ struct bcom_bd_2*bd;
+ phys_addr_t bd_pa;
+ void**cookie;
+ unsigned short index;
+ unsigned short outdex;
+ unsigned intnum_bd;
+ unsigned intbd_size;
+
+ void* priv;
+};
+
Oh, ugly. The only difference seems to be that bcom_bd_2 is 1 word
larger than bcom_bd. There must be a better way to do this rather
than just duplicating all the functions and structures.. It would
probably be better to use bd_size to determine how to dereference an
index into the *bd list. It will require a bit of refactoring, but
it will be much cleaner and more maintainable that way.
diff -Nurp linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
--- linux-2.6.26-rc6/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
2008-03-18 15:49:53.0 +
+++ linux-2.6.26-rc6.new/arch/powerpc/sysdev/bestcomm/bestcomm_priv.h
2008-04-15 10:42:38.0 +0100
@@ -198,8 +198,8 @@ struct bcom_task_header {
#define BCOM_IPR_SCTMR_1 2
#define BCOM_IPR_FEC_RX 6
#define BCOM_IPR_FEC_TX 5
-#define BCOM_IPR_ATA_RX 4
-#define BCOM_IPR_ATA_TX 3
+#define BCOM_IPR_ATA_RX 7
+#define BCOM_IPR_ATA_TX 7
#define BCOM_IPR_SCPCI_RX2
#define BCOM_IPR_SCPCI_TX2
#define BCOM_IPR_PSC3_RX 2
Is this a bug fix? If so, please put it into a separate patch.
diff -Nurp linux-2.6.26-rc6/drivers/ata/Kconfig
linux-2.6.26-rc6.new/drivers/ata/Kconfig
--- linux-2.6.26-rc6/drivers/ata/Kconfig 2008-03-18 15:49:33.0
+
+++ linux-2.6.26-rc6.new/drivers/ata/Kconfig 2008-04-15 10:41:51.0
+0100
@@ -462,6 +462,15 @@
If unsure, say N.
+config PATA_MPC52xx_DMA
+ tristate Freescale MPC52xx SoC internal IDE DMA
+ depends on PATA_MPC52xx
+ help
+ This option enables support for DMA on the MPC52xx SoC PATA
+ controller.
+
+ If unsure, say N.
+
Good, it can be turned off. Do you think there is any risk to existing
ATA users with this patch applied if this is turned off?
config PATA_MPIIX
tristate Intel PATA MPIIX support
depends on PCI
diff -Nurp linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c
linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c
--- linux-2.6.26-rc6/drivers/ata/pata_mpc52xx.c 2008-03-18
15:49:33.0 +
+++ linux-2.6.26-rc6.new/drivers/ata/pata_mpc52xx.c 2008-04-15
10:41:49.0 +0100
@@ -6,6 +6,9 @@
* Copyright (C) 2006 Sylvain Munaut [EMAIL PROTECTED]
* Copyright (C) 2003 Mipsys - Benjamin Herrenschmidt
*
+ * UDMA support based on patches by Freescale (Bernard Kuhn, John Rigby),
+ * Domen Puncer and Tim Yamin.
+ *
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed as is without any warranty of any
* kind, whether express or implied.
@@ -17,28 +20,47 @@
#include
RE: [PATCH]: [MPC5200] (v2) Add ATA DMA support
Hi, Against which kernel is this patch against ? Regards, Daniel. Tim Yamin wrote: Changes from previous version: - Add FIFO status error checking code before a DMA transaction starts and after it is completed. - Fix an incorrect check in the previous patch causing spurious dma table too small errors. Tim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
