ide: New libata driver for OCTEON SOC Compact Flash interface.
PATCH 1/1 ide: New libata driver for OCTEON SOC Compact Flash interface.
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support on the CF interface so for these, only PIO is supported. Although if DMA is available, we do take advantage of it.
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Thanks,
---
drivers/ata/Kconfig | 9 + drivers/ata/Makefile | 1 + drivers/ata/pata_octeon_cf.c | 942 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 952 insertions(+), 0 deletions(-)
drivers/ata/Kconfig +0 -0
@@ -697,6 +697,15 @@ config PATA_IXP4XX_CF
If unsure, say N.
+config PATA_OCTEON_CF
+ tristate "OCTEON Boot Bus Compact Flash support"
+ depends on CPU_CAVIUM_OCTEON
+ help
+ This option enables a polled compact flash driver for use with
+ compact flash cards attached to the OCTEON boot bus.
+
+ If unsure, say N.
+
config PATA_SCC
tristate "Toshiba's Cell Reference Set IDE support" depends on PCI && PPC_CELLEB
drivers/ata/Makefile +0 -0
@@ -69,6 +69,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF) += pata_ixp4xx_cf.o
obj-$(CONFIG_PATA_SCC) += pata_scc.o
obj-$(CONFIG_PATA_SCH) += pata_sch.o
obj-$(CONFIG_PATA_BF54X) += pata_bf54x.o
+obj-$(CONFIG_PATA_OCTEON_CF) += pata_octeon_cf.o
obj-$(CONFIG_PATA_PLATFORM) += pata_platform.o
obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o
obj-$(CONFIG_PATA_ICSIDE) += pata_icside.o
drivers/ata/pata_octeon_cf.c new file +942 -0
942 lines not on this page · raw message
drivers/ata/pata_octeon_cf.c line 44
Quoted 6 lines
+ * Called to enable the use of DMA based on kernel command line.
+ */drivers/ata/pata_octeon_cf.c new file +4
-0+void octeon_cf_enable_dma (void ) +{ + use_cf_dma =1 ; +}
Why not use the standard module parameter interface ?
drivers/ata/pata_octeon_cf.c line 99
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+/* +* PIO modes 0-4 all allow the device to deassert IORDY to slow down +* the host. +*/
See ata_timing_compute() which also knows about master/slave timing, PIO5/6 rules and timing adjustments as well as doing bus clock quantisations and lengthenings for you.
drivers/ata/pata_octeon_cf.c line 100
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+ use_iordy =1 ;
This depends on the device as well and gets quite complicated. We have ata_pio_need_iordy() to do the work for you.
drivers/ata/pata_octeon_cf.c line 170
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ t1 = (t1 * clocks_us) /1000 /2 ; +if (t1) + t1--;
Even if you wanted to do it this way you could just use arrays and lookup tables as many other drivers do - ie
pio = dev->pio_mode - XFER_PIO_0; t1 = data[pio];
drivers/ata/pata_octeon_cf.c line 203
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+ mio_boot_reg_tim.s.wr_hld = t4;
What guarantees the computation results always fit the bit fields. This is one reason the kerne strongly favours masks - it is a lot easier to check such things.
drivers/ata/pata_octeon_cf.c line 247
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +8
-0+static void octeon_cf_set_dmamode (struct ata_port *ap,struct ata_device *dev) +{ +case XFER_MW_DMA_0 : + dma_ackh =20 ;/* Tj */ + To =480 ; + Td =215 ; + Tkr =50 ; +break ;
Same comments apply as to PIO
drivers/ata/pata_octeon_cf.c line 335
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 );
If you get a request for an odd length you should write an extra word containing the last byte and one other. See how the standard methods handle this.
drivers/ata/pata_octeon_cf.c line 336
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+if (ocd->is16bit) {
Or you could have two methods and two transfer routines defined
drivers/ata/pata_octeon_cf.c line 355
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+while (words--) { + *(uint16_t *)buffer =ioread16 (data_addr); + buffer +=sizeof (uint16_t );
By definition tht is 2. Do you have an ioread16_rep ?
drivers/ata/pata_octeon_cf.c line 566
Quoted 8 lines
drivers/ata/pata_octeon_cf.c new file +8
-0+ +/** +* Get ready for a dma operation. We do nothing, as all DMA +* operations are taken care of in octeon_cf_bmdma_start. +*/ +void octeon_cf_qc_prep (struct ata_queued_cmd *qc) +{ +}
ata_noop_qc_prep
drivers/ata/pata_octeon_cf.c line 707
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/** +* Check if any queued commands have more DMAs, if so start the next +* transfer, else do standard handling. +*/ +irqreturn_t octeon_cf_interrupt (int irq,void *dev_instance)
A lot of these functions could be static it seems
Review
Quoted 3 lines
(quoted diff) +2
-0+static void octeon_cf_delayed_irq(unsigned long data) +{
What stops the following occuring
ATA irq
BUSY still set
Queue tasklet
Other irq on same line
ATA busy clear
Handle command
Tasklet runs but command was sorted out
(or a reset of the ata controller in the gap)
drivers/ata/pata_octeon_cf.c line 860
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ base = cs0 + ocd->base_region_bias; +if (!ocd->is16bit) {
ata_std_
drivers/ata/pata_octeon_cf.c line 861
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+ ap->ioaddr.cmd_addr = base +ATA_REG_CMD ;
ata_sff_std_ports ? (at least for the 8bit case)
Alan
--
"Alan, I'm getting a bit worried about you."
-- Linus Torvalds
Quoted 9 lines · Alan Cox
drivers/ata/pata_octeon_cf.c line 44
Quoted 6 lines
+ * Called to enable the use of DMA based on kernel command line.
+ */drivers/ata/pata_octeon_cf.c new file +4
-0+void octeon_cf_enable_dma (void ) +{ + use_cf_dma =1 ; +}Why not use the standard module parameter interface ?
I will do that.
Quoted 10 lines
drivers/ata/pata_octeon_cf.c line 99
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+/* +* PIO modes 0-4 all allow the device to deassert IORDY to slow down +* the host. +*/ See
ata_timing_compute()which also knows about master/slave timing, PIO5/6 rules and timing adjustments as well as doing bus clock quantisations and lengthenings for you.
OK.
Quoted 12 lines
drivers/ata/pata_octeon_cf.c line 100
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+ use_iordy =1 ;This depends on the device as well and gets quite complicated. We have
ata_pio_need_iordy()to do the work for you.drivers/ata/pata_octeon_cf.c line 170
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ t1 = (t1 * clocks_us) /1000 /2 ; +if (t1) + t1--;Even if you wanted to do it this way you could just use arrays and lookup tables as many other drivers do - ie
Quoted 3 lines
pio = dev->pio_mode - XFER_PIO_0; t1 = data[pio];
The timing calculations are based on the CPU clock rate, It is difficult to encapsulate that in a table.
[...]
Quoted 10 lines
drivers/ata/pata_octeon_cf.c line 335
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 );If you get a request for an odd length you should write an extra word containing the last byte and one other. See how the standard methods handle this.
OK.
Quoted 5 lines
drivers/ata/pata_octeon_cf.c line 336
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +1
-0+if (ocd->is16bit) {Or you could have two methods and two transfer routines defined
Good idea.
Quoted 6 lines
drivers/ata/pata_octeon_cf.c line 355
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+while (words--) { + *(uint16_t *)buffer =ioread16 (data_addr); + buffer +=sizeof (uint16_t );By definition tht is 2. Do you have an
ioread16_rep?
It appears to be broken. One would expect ioread16 and ioread16_rep to do endian swapping in the same manner. On MIPS they do not. Perhaps it would be better to fix the problem at the source.
Quoted 12 lines
drivers/ata/pata_octeon_cf.c line 566
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +8
-0+ +/** +* Get ready for a dma operation. We do nothing, as all DMA +* operations are taken care of in octeon_cf_bmdma_start. +*/ +void octeon_cf_qc_prep (struct ata_queued_cmd *qc) +{ +}
ata_noop_qc_prep
OK.
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 707
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/** +* Check if any queued commands have more DMAs, if so start the next +* transfer, else do standard handling. +*/ +irqreturn_t octeon_cf_interrupt (int irq,void *dev_instance)A lot of these functions could be static it seems
An oversight on my part. I will make them all static.
Quoted 10 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+static void octeon_cf_delayed_irq(unsigned long data) +{What stops the following occuring
ATA irq
BUSY still set
Queue tasklet
Quoted 8 lines
Other irq on same line
ATA busy clear
Handle commandTasklet runs but command was sorted out
(or a reset of the ata controller in the gap)
Probably nothing. I will try to sort it out.
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 860
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ base = cs0 + ocd->base_region_bias; +if (!ocd->is16bit) {
ata_std_drivers/ata/pata_octeon_cf.c line 861
Quoted 1 line
drivers/ata/pata_octeon_cf.c new file +1
-0+ ap->ioaddr.cmd_addr = base +ATA_REG_CMD ;
ata_sff_std_ports? (at least for the 8bit case)
OK.
Thanks,
David Daney
Quoted 9 lines · David Daney
Quoted 8 lines
Quoted 1 line
+
use_iordy= 1;This depends on the device as well and gets quite complicated. We have
ata_pio_need_iordy()to do the work for you.Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ t1 = (t1 * clocks_us) /1000 /2 ; +if (t1) + t1--;
Quoted 7 lines
Quoted 5 lines
Even if you wanted to do it this way you could just use arrays and lookup tables as many other drivers do - ie
pio = dev->pio_mode - XFER_PIO_0; t1 = data[pio];The timing calculations are based on the CPU clock rate, It is difficult to encapsulate that in a table.
Nobody suggests that. You surely can put the timings in ns in a
structure/table.
Quoted 8 lines
[...]
Quoted 5 lines
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 );
Quoted 5 lines
Quoted 3 lines
If you get a request for an odd length you should write an extra word containing the last byte and one other. See how the standard methods handle this.
OK.
I don't think you need to bother doing that with CF.
Quoted 7 lines
Quoted 5 lines
drivers/ata/pata_octeon_cf.c line 355
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+while (words--) { + *(uint16_t *)buffer =ioread16 (data_addr); + buffer +=sizeof (uint16_t );By definition tht is 2. Do you have an
ioread16_rep?It appears to be broken. One would expect ioread16 and
ioread16_repto do endian swapping in the same manner. On MIPS they do not.
And that's actually good.
Quoted 1 line
Perhaps it would be better to fix the problem at the source.
I don't think there's a problem there. The string versions of the
functions treat memory as a string of bytes.
Quoted 5 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static void octeon_cf_delayed_irq(unsigned long data) +{What stops the following occuring
Quoted 7 lines
Quoted 7 lines
ATA irq
BUSY still set
Queue taskletOther irq on same line
ATA busy clear
Handle command
Is CF interrupt indeed shared with anything?
Quoted 5 lines
Quoted 3 lines
Tasklet runs but command was sorted out
(or a reset of the ata controller in the gap)
Probably nothing. I will try to sort it out.
It's the need for the tasklet that seems doubtful to me...
Quoted 2 lines
Thanks,
David Daney
MBR, Sergei
Quoted 1 line · Sergei Shtylyov
David Daney wrote:
[...]
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
What stops the following occuring
ATA irq
BUSY still set
Queue taskletOther irq on same line
ATA busy clear
Handle commandIs CF interrupt indeed shared with anything?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Tasklet runs but command was sorted out
(or a reset of the ata controller in the gap)
Probably nothing. I will try to sort it out.
It's the need for the tasklet that seems doubtful to me...
The interrupt occurs *before* the device de-asserts BUSY. A small pause is needed to allow the device to clear BUSY and allow libata to function normally.
Calling ata_sff_host_intr() while BUSY is asserted causes the driver to fail, as it is expecting to be called from a true IDE interrupt routine.
Delaying calling ata_sff_host_intr() until BUSY is clear (using the
tasklet) seemed to be the cleanest way to write the driver given that we are using an interrupt generated by DMA complete.
David Daney.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
If you get a request for an odd length you should write an extra word containing the last byte and one other. See how the standard methods handle this.
OK.
I don't think you need to bother doing that with CF.
Look at it the other way around is the best thing to do given an odd sized I/O (eg if a CF format ATAPI drive comes along) to crash the machine.
Quoted 4 lines
Quoted 1 line
Perhaps it would be better to fix the problem at the source.
I don't think there's a problem there. The string versions of the
functions treat memory as a string of bytes.
Double checking the docs you are right. How dumb, but dumb or not that is what it is specified to do.
Quoted 3 lines
Quoted 1 line
Probably nothing. I will try to sort it out.
It's the need for the tasklet that seems doubtful to me...
I was wondering but assumed it was some kind of hardware feature where the end of IRQ occurs too early to do some of the cleanup.
Quoted 9 lines
Quoted 6 lines
Even if you wanted to do it this way you could just use arrays and lookup tables as many other drivers do - ie
pio = dev->pio_mode - XFER_PIO_0; t1 = data[pio];The timing calculations are based on the CPU clock rate, It is difficult to encapsulate that in a table.
The lookup part of the switch you can however. If you can use the ata_timing interface then it will also do the clock adjusting for you and has a FIT() macro that can be quite handy for clipping.
Quoted 3 lines
It appears to be broken. One would expect ioread16 and
ioread16_repto do endian swapping in the same manner. On MIPS they do not. Perhaps it would be better to fix the problem at the source.
I think the MIPS tree needs fixing then.
Alan
Hello.
Quoted 5 lines · David Daney
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support
That latter excludes the variant of porting to the IDE core...
Quoted 6 lines
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Why a libata driver should be merged via the linus-mips tree?
Quoted 10 lines
Thanks,
Signed-off-by David Daney <[email protected]>drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@
[...]
drivers/ata/pata_octeon_cf.c line 22
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+#define DRV_NAME "pata_octeon_cf" +#define DRV_VERSION "2.1"
First version and already 2.1? :-)
drivers/ata/pata_octeon_cf.c line 49
Quoted 12 lines
drivers/ata/pata_octeon_cf.c new file +12
-0+/** +* Called to enable the use of DMA based on kernel command line. +*/ +void octeon_cf_enable_dma (void ) +{ + use_cf_dma =1 ; +} + +static unsigned int div_roundup (unsigned int n,unsigned int d) +{ +return (n + d -1 ) / d; +}
Why reinvent the wheel? There's DIV_ROUND_UP macro in <linux/kernel.h>...
Quoted 16 lines
+ * Called after libata determines the needed PIO mode. This
+ * function programs the Octeon bootbus regions to support the
+ * timing requirements of the PIO mode.
+ *
+ * @ap: ATA port information
+ * @dev: ATA device
+ */
+static voidocteon_cf_set_piomode(struct ata_port *ap, struct ata_device *dev)drivers/ata/pata_octeon_cf.c new file +7
-0+{ +struct octeon_cf_data *ocd = ap->dev->platform_data; +cvmx_mio_boot_reg_timx_t mio_boot_reg_tim; +unsigned int cs = ocd->base_region; + +int use_iordy;/* Non zero to monitor the IORDY signal */ +int clocks_us;/* Number of clock cycles per microsec */
drivers/ata/pata_octeon_cf.c line 98
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+/* These names are timing parameters from the ATA spec */ +int t1; +int t2; +int t2i; +int t4; +int t6; +int t6z; +/* +* PIO modes 0-4 all allow the device to deassert IORDY to slow down +* the host.
That's not so simple. Drive may support PIO mode up to 2 but not support
IORDY -- typical for low-end CF. There's ata_pio_need_iordy() for checking if the current PIO mode needs IORDY.
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+*/ + use_iordy =1 ;
Empty line needed after the declaration block.
drivers/ata/pata_octeon_cf.c line 105
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* Use the PIO mode to determine out timing parameters */ +switch (dev->pio_mode) { +case XFER_PIO_0 : + t1 =70 ; + t2 =165 ;
These 2 parameters (along with t2i) are returned by ata_timing_find_mode().
Quoted 1 line
+ t2i = 0;
You can't calclate the recoverly properly basing on 0 minimum recovery
timing. You must include the minimum cycle time into equation -- it's returned by ata_timing_find_mode() as well...
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ t4 =30 ; + t6 =5 ;
I don't see any sense in setting this timing separately for all modes with
it always being the same 5 ns.
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ t6z =30 ; +break ;
Most of these parameters are returned by ata_timing_find_mode().
drivers/ata/pata_octeon_cf.c line 145
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+case XFER_PIO_5 : +/* CF spec say IORDY should be ignored in PIO 5 and 6 */ + use_iordy =0 ;
Wow, that's something new! CF 2.1 that I have doesn't even say these modes
exist (though I know they do). The libata code surely doesn't follow this.
Quoted 1 line
+
clocks_us= (octeon_get_clock_rate()+ (1000000 - 1)) / (1000000);
Use DIV_ROUND_UP() here.
drivers/ata/pata_octeon_cf.c line 176
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ t1 = (t1 * clocks_us) /1000 /2 ; +if (t1) + t1--; + t2 = (t2 * clocks_us) /1000 /2 ; +if (t2) + t2--; + t2i = (t2i * clocks_us) /1000 /2 ; +if (t2i) + t2i--;
Calculation the recovery time isn't so simple for PIO modes 0 thru 2.
drivers/ata/pata_octeon_cf.c line 185
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ t4 = (t4 * clocks_us) /1000 /2 ; +if (t4) + t4--; + t6 = (t6 * clocks_us) /1000 /2 ; +if (t6) + t6--; + t6z = (t6z * clocks_us) /1000 /2 ; +if (t6z) + t6z--;
I think the above repetitive calculation needs to be factored out into a
function.
Quoted 3 lines
+static void
octeon_cf_set_dmamode(struct ata_port *ap, struct ata_device *dev)
+{
[...]
drivers/ata/pata_octeon_cf.c line 246
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+switch (dev->dma_mode) { +case XFER_MW_DMA_0 : + dma_ackh =20 ;/* Tj */ + To =480 ; + Td =215 ; + Tkr =50 ;
It's safer to use Tkw instead as it's 215 ns minimum in this mode (in
other modes Tkr and Tkw minimums are the same).
Again, To, Td, and Tkw are returned by ata_timing_find_mode().
drivers/ata/pata_octeon_cf.c line 276
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+/* Td (Seem to need more margin here.... */ + oe_a = Td +20 ;
Well, you shouldn't...
Quoted 22 lines
drivers/ata/pata_octeon_cf.c new file +22
-0+/** +* Handle an I/O request. +* +* @cf: Device to access +* @lba_sector: Starting sector +* @num_sectors: +* Number of sectors to transfer +* @buffer: Data buffer +* @write: Is the a write. Default to a read +*/ +static unsigned int octeon_cf_data_xfer (struct ata_device *dev, +unsigned char *buffer, +unsigned int buflen, +int rw) +{ +struct ata_port *ap = dev->link->ap; +struct octeon_cf_data *ocd = ap->dev->platform_data; +void __iomem *data_addr = ap->ioaddr.data_addr; +unsigned int words; +unsigned int count; + +/*
drivers/ata/pata_octeon_cf.c line 341
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 ); +if (ocd->is16bit) { + words = buflen /2 ; +if (rw) { + count =16 ; +while (words--) { +iowrite16 (*(uint16_t *)buffer, data_addr);
Not seeing the reason to use iowrite16() and not writew() as registers are
always memory mapped...
drivers/ata/pata_octeon_cf.c line 350
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ buffer +=sizeof (uint16_t ); +/* +* Every 16 writes do a read so the +* bootbus FIFO doesn't fill up. +*/ +if (--count ==0 ) { +ioread8 (ap->ioaddr.altstatus_addr); + count =16 ; + }
This is a strange/slow way of doing anything every 16 data writes. Why not
use iowrite16_rep()?
drivers/ata/pata_octeon_cf.c line 355
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+ } + }else { +while (words--) { + *(uint16_t *)buffer =ioread16 (data_addr); + buffer +=sizeof (uint16_t );
It would have been handier to use a local pointer variable instead...
Quoted 2 lines
+/**
+ * Read the taskfile for 16bit non-true_ide only.
It's called TrueIDE, not true_ide. :-)
drivers/ata/pata_octeon_cf.c line 392
Quoted 10 lines
+ */
+static voidocteon_cf_tf_read16(struct ata_port *ap, struct ata_taskfile *tf)drivers/ata/pata_octeon_cf.c new file +7
-0+{ +u16 blob; +/* The base of the registers is at ioaddr.data_addr. */ +void __iomem *base = ap->ioaddr.data_addr; + + blob =__raw_readw (base +0xc ); + tf->feature = blob >>8 ;
How come the error register gets mapped at offset 0xC? (Well, that would explain alike part in another CF driver.)
drivers/ata/pata_octeon_cf.c line 407
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+if (tf->flags &ATA_TFLAG_LBA48 ) { +if (likely (ap->ioaddr.ctl_addr)) {
You set it yourself, what's the point of checking it?
drivers/ata/pata_octeon_cf.c line 431
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+static u8 octeon_cf_check_status16 (struct ata_port *ap) +{ +u16 blob; +void __iomem *base = ap->ioaddr.data_addr;
You hardly need these variables...
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ blob =__raw_readw (base +6 ); +return blob >>8 ; +} +/** +* Issue ATA command to host controller. The device_addr is also sent +* as it must be written in a combined write with the command. +*/ +static void octeon_cf_exec_command16 (struct ata_port *ap, +const struct ata_taskfile *tf)
drivers/ata/pata_octeon_cf.c line 527
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+{ +/* The base of the registers is at ioaddr.data_addr. */ +void __iomem *base = ap->ioaddr.data_addr; +u16 blob; + +if (tf->flags &ATA_TFLAG_DEVICE ) { +VPRINTK ("device 0x%X\n" , tf->device); + blob = tf->device; + }else + blob =0 ;
I'd have written 0xA0 in this case...
drivers/ata/pata_octeon_cf.c line 530
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+DPRINTK ("ata%u: cmd 0x%X\n" , ap->print_id, tf->command); + blob |= (tf->command <<8 );
Unneeded parens.
Quoted 3 lines
+/**
+ * Get the status of the DMA engine. The results of this function
+ * must emulate the BMDMA engine expected by libata.
Ugh... At least the IDE core is not so retarded.
drivers/ata/pata_octeon_cf.c line 707
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/** +* Check if any queued commands have more DMAs, if so start the next +* transfer, else do standard handling. +*/ +irqreturn_t octeon_cf_interrupt (int irq,void *dev_instance)
Is that a normal IDE interrupt, or some Octeon specific interrupt?
drivers/ata/pata_octeon_cf.c line 718
Quoted 11 lines
drivers/ata/pata_octeon_cf.c new file +11
-0+{ +struct ata_host *host = dev_instance; +struct octeon_cf_port *cf_port; +int i; +unsigned int handled =0 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); + +DPRINTK ("ENTER\n" ); +for (i =0 ; i < host->n_ports; i++) {
Oh, you can have several ports?
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+struct ata_port *ap; +struct ata_queued_cmd *qc; + + ap = host->ports[i]; +if (!ap || (ap->flags &ATA_FLAG_DISABLED )) +continue ; + + qc =ata_qc_from_tag (ap, ap->link.active_tag); +if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && + (qc->flags &ATA_QCFLAG_ACTIVE )) {
drivers/ata/pata_octeon_cf.c line 738
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+u8 status =octeon_cf_bmdma_status (ap); +if ((status &ATA_DMA_INTR ) + && !(status &ATA_DMA_ACTIVE ) + && !sg_is_last (qc->cursg)) { + qc->cursg =sg_next (qc->cursg); + handled =1 ; +octeon_cf_bmdma_start (qc); + }else { + status =ioread8 (ap->ioaddr.altstatus_addr); +if (status &ATA_BUSY ) {
If it's normal IDE interrupt, BSY bit will always be 0 (unless the
interrupt is shared with some other device)...
drivers/ata/pata_octeon_cf.c line 748
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+/* +* We are busy, try to handle +* it later. This is the DMA +* finished interrupt, and it +* could take a little while +* for the card to be ready +* for more commands. +*/ + cf_port = (struct octeon_cf_port *)ap->private_data; +tasklet_schedule (&cf_port->delayed_irq_tasklet);
... in which case the tasklet would seem pointless.
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+static int __devinit octeon_cf_probe (struct platform_device *pdev) +{ +struct resource *res_cs0, *res_cs1; + +void __iomem *cs0; +void __iomem *cs1;
[...]
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ + res_cs0 =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + res_cs1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 );
[...]
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ base = cs0 + ocd->base_region_bias; +if (!ocd->is16bit) {
[...]
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ ap->ioaddr.altstatus_addr = base +0xe ; + ap->ioaddr.ctl_addr = base +0xe ;
Wait, why have 2 MMIO resources then?
Quoted 1 line
+ } else if (
is_true_ide) {
[...]
drivers/ata/pata_octeon_cf.c line 891
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+if (use_cf_dma) { + ap->mwdma_mask =0x1f ;/* Support Multiword DMA 0-4 */ + irq =platform_get_irq (pdev,0 ); + irq_handler = octeon_cf_interrupt;
So, what kind of interrupt this is?
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+ } + }else { +/* 16 bit but not true_ide */ + octeon_cf_ops.softreset = octeon_cf_softreset16;
drivers/ata/pata_octeon_cf.c line 905
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+ octeon_cf_ops.sff_check_status = octeon_cf_check_status16; + octeon_cf_ops.sff_tf_read = octeon_cf_tf_read16; + octeon_cf_ops.sff_tf_load = octeon_cf_tf_load16; + octeon_cf_ops.sff_exec_command = octeon_cf_exec_command16; + + ap->ioaddr.data_addr = base +ATA_REG_DATA ; + ap->ioaddr.nsect_addr = base +ATA_REG_NSECT ; + ap->ioaddr.lbal_addr = base +ATA_REG_LBAL ; + ap->ioaddr.ctl_addr = base +0xe ; + ap->ioaddr.altstatus_addr = base +0xe ;
Why have 2 MMIO resources then?
WBR, Sergei
Thanks for the feedback...
Quoted 11 lines · Sergei Shtylyov
Hello.
Quoted 5 lines · David Daney
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support
That latter excludes the variant of porting to the IDE core...
That's right.
Quoted 10 lines
Quoted 7 lines
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Why a libata driver should be merged via the linus-mips tree?
I don't have a strong preference. The driver depends on the processor support *and* it is only relevant to OCTEON based boards. If the linux-ide maintainers would prefer to merge it via their tree that is fine with me.
Quoted 11 lines
Quoted 10 lines
Thanks,
Signed-off-by David Daney <[email protected]>drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@[...]
Quoted 4 lines
drivers/ata/pata_octeon_cf.c line 22
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+#define DRV_NAME "pata_octeon_cf" +#define DRV_VERSION "2.1"First version and already 2.1? :-)
No, versions 1.0, 1.1, and 2.0 exist, but are living a decadent out-of-tree life.
[...]
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 49
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+static unsigned int div_roundup (unsigned int n,unsigned int d) +{ +return (n + d -1 ) / d; +}Why reinvent the wheel? There's
DIV_ROUND_UPmacro in
<linux/kernel.h>...
OK.
Quoted 21 lines
Quoted 21 lines
+ * Called after libata determines the needed PIO mode. This
+ * function programs the Octeon bootbus regions to support the
+ * timing requirements of the PIO mode.
+ *
+ * @ap: ATA port information
+ * @dev: ATA device
+ */
+static voidocteon_cf_set_piomode(struct ata_port *ap, struct ata_device *dev)drivers/ata/pata_octeon_cf.c new file +12
-0+{ +struct octeon_cf_data *ocd = ap->dev->platform_data; +cvmx_mio_boot_reg_timx_t mio_boot_reg_tim; +unsigned int cs = ocd->base_region; + +int use_iordy;/* Non zero to monitor the IORDY signal */ +int clocks_us;/* Number of clock cycles per microsec */ +/* These names are timing parameters from the ATA spec */ +int t1; +int t2; +int t2i; +int t4;
Quoted 9 lines
drivers/ata/pata_octeon_cf.c line 98
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+int t6; +int t6z; +/* +* PIO modes 0-4 all allow the device to deassert IORDY to slow down +* the host. That's not so simple. Drive may support PIO mode up to 2 but not
support IORDY -- typical for low-end CF. There'sata_pio_need_iordy()for checking if the current PIO mode needs IORDY.
OK.
Quoted 5 lines
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+*/ + use_iordy =1 ;Empty line needed after the declaration block.
Quoted 9 lines
drivers/ata/pata_octeon_cf.c line 105
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* Use the PIO mode to determine out timing parameters */ +switch (dev->pio_mode) { +case XFER_PIO_0 : + t1 =70 ; + t2 =165 ;These 2 parameters (along with t2i) are returned by
ata_timing_find_mode().
OK.
[...]
Quoted 10 lines
drivers/ata/pata_octeon_cf.c line 185
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ t4 = (t4 * clocks_us) /1000 /2 ; +if (t4) + t4--; + t6 = (t6 * clocks_us) /1000 /2 ; +if (t6) + t6--; + t6z = (t6z * clocks_us) /1000 /2 ; +if (t6z) + t6z--;
Quoted 3 lines
I think the above repetitive calculation needs to be factored out
into a function.
Yep.
Quoted 11 lines
Quoted 3 lines
+static void
octeon_cf_set_dmamode(struct ata_port *ap, struct ata_device *dev)
+{[...]
drivers/ata/pata_octeon_cf.c line 246
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+switch (dev->dma_mode) { +case XFER_MW_DMA_0 : + dma_ackh =20 ;/* Tj */ + To =480 ; + Td =215 ; + Tkr =50 ;
Quoted 9 lines
It's safer to use Tkw instead as it's 215 ns minimum in this mode (in
other modes Tkr and Tkw minimums are the same).
Again, To, Td, and Tkw are returned byata_timing_find_mode().drivers/ata/pata_octeon_cf.c line 276
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+/* Td (Seem to need more margin here.... */ + oe_a = Td +20 ;Well, you shouldn't...
Hmm...
Quoted 27 lines
Quoted 27 lines
drivers/ata/pata_octeon_cf.c new file +27
-0+/** +* Handle an I/O request. +* +* @cf: Device to access +* @lba_sector: Starting sector +* @num_sectors: +* Number of sectors to transfer +* @buffer: Data buffer +* @write: Is the a write. Default to a read +*/ +static unsigned int octeon_cf_data_xfer (struct ata_device *dev, +unsigned char *buffer, +unsigned int buflen, +int rw) +{ +struct ata_port *ap = dev->link->ap; +struct octeon_cf_data *ocd = ap->dev->platform_data; +void __iomem *data_addr = ap->ioaddr.data_addr; +unsigned int words; +unsigned int count; + +/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 ); +if (ocd->is16bit) {
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 341
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+ words = buflen /2 ; +if (rw) { + count =16 ; +while (words--) { +iowrite16 (*(uint16_t *)buffer, data_addr);Not seeing the reason to use
iowrite16()and notwritew()as
registers are always memory mapped...
I will consider that.
Quoted 11 lines
drivers/ata/pata_octeon_cf.c line 350
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ buffer +=sizeof (uint16_t ); +/* +* Every 16 writes do a read so the +* bootbus FIFO doesn't fill up. +*/ +if (--count ==0 ) { +ioread8 (ap->ioaddr.altstatus_addr); + count =16 ; + }
Quoted 1 line
This is a strange/slow way of doing anything every 16 data writes.
Oh, how would you do something every 16 writes?
Quoted 1 line
Why not use
iowrite16_rep()?
Two reasons. It is broken, and we need to do something every 16 writes.
We need to be careful not to be anti-social by filling the FIFO. Once the FIFO is filled other cores will be starved of I/O resources.
[...]
Quoted 4 lines
Quoted 4 lines
+static void
octeon_cf_tf_read16(struct ata_port *ap, struct ata_taskfile *tf)drivers/ata/pata_octeon_cf.c new file +2
-0+{ +u16 blob;
Quoted 9 lines
drivers/ata/pata_octeon_cf.c line 392
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* The base of the registers is at ioaddr.data_addr. */ +void __iomem *base = ap->ioaddr.data_addr; + + blob =__raw_readw (base +0xc ); + tf->feature = blob >>8 ;How come the error register gets mapped at offset
0xC? (Well, that would explain alike part in another CF driver.)
In 16 bit mode it overlaps the data register, so it must be accessed via the 0xc address.
[...]
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 707
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/** +* Check if any queued commands have more DMAs, if so start the next +* transfer, else do standard handling. +*/ +irqreturn_t octeon_cf_interrupt (int irq,void *dev_instance)Is that a normal IDE interrupt, or some Octeon specific interrupt?
No, It is OCTEON specific. It fires when a DMA operation is complete. As I mentioned earlier, the normal IDE interrupt is not connected to anything on most boards.
Quoted 5 lines
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+{ +struct ata_host *host = dev_instance; +struct octeon_cf_port *cf_port; +int i; +unsigned int handled =0 ;
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 718
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); + +DPRINTK ("ENTER\n" ); +for (i =0 ; i < host->n_ports; i++) {Oh, you can have several ports?
In theory it could happen...
Quoted 15 lines
Quoted 15 lines
drivers/ata/pata_octeon_cf.c new file +14
-0+struct ata_port *ap; +struct ata_queued_cmd *qc; + + ap = host->ports[i]; +if (!ap || (ap->flags &ATA_FLAG_DISABLED )) +continue ; + + qc =ata_qc_from_tag (ap, ap->link.active_tag); +if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && + (qc->flags &ATA_QCFLAG_ACTIVE )) { +u8 status =octeon_cf_bmdma_status (ap); +if ((status &ATA_DMA_INTR ) + && !(status &ATA_DMA_ACTIVE ) + && !sg_is_last (qc->cursg)) {
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 738
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+ qc->cursg =sg_next (qc->cursg); + handled =1 ; +octeon_cf_bmdma_start (qc); + }else { + status =ioread8 (ap->ioaddr.altstatus_addr); +if (status &ATA_BUSY ) {If it's normal IDE interrupt,
Which it is not. So...
Quoted 1 line
BSY bit will always be 0
...this doesn't hold, thus all this code.
Quoted 13 lines
(unless the interrupt is shared with some other device)...
drivers/ata/pata_octeon_cf.c line 748
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+/* +* We are busy, try to handle +* it later. This is the DMA +* finished interrupt, and it +* could take a little while +* for the card to be ready +* for more commands. +*/ + cf_port = (struct octeon_cf_port *)ap->private_data; +tasklet_schedule (&cf_port->delayed_irq_tasklet);
Quoted 1 line
... in which case the tasklet would seem pointless.
See above.
Quoted 14 lines
Quoted 7 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+static int __devinit octeon_cf_probe (struct platform_device *pdev) +{ +struct resource *res_cs0, *res_cs1; + +void __iomem *cs0; +void __iomem *cs1;[...]
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ + res_cs0 =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + res_cs1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 );[...]
Quoted 7 lines
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ base = cs0 + ocd->base_region_bias; +if (!ocd->is16bit) {[...]
drivers/ata/pata_octeon_cf.c
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ ap->ioaddr.altstatus_addr = base +0xe ; + ap->ioaddr.ctl_addr = base +0xe ;Wait, why have 2 MMIO resources then?
The boards that are 'TrueIDE' use two resources, others only one.
Quoted 10 lines
Quoted 2 lines
+ } else if (
is_true_ide) {[...]
drivers/ata/pata_octeon_cf.c line 891
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+if (use_cf_dma) { + ap->mwdma_mask =0x1f ;/* Support Multiword DMA 0-4 */ + irq =platform_get_irq (pdev,0 ); + irq_handler = octeon_cf_interrupt;So, what kind of interrupt this is?
DMA completion, see above.
[...]
Thanks,
David Daney
Hello.
Quoted 10 lines · David Daney
Quoted 8 lines
Quoted 6 lines
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Why a libata driver should be merged via the linus-mips tree?
I don't have a strong preference. The driver depends on the processor support *and* it is only relevant to OCTEON based boards. If the
Most SoC IDE drivers are like that.
Quoted 23 lines
linux-ide maintainers would prefer to merge it via their tree that is fine with me.
Quoted 20 lines
Quoted 7 lines
Thanks,
Signed-off-by David Daney <[email protected]>+static void
octeon_cf_set_dmamode(struct ata_port *ap, struct ata_device *dev)
+{[...]
drivers/ata/pata_octeon_cf.c line 246
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+switch (dev->dma_mode) { +case XFER_MW_DMA_0 : + dma_ackh =20 ;/* Tj */ + To =480 ; + Td =215 ; + Tkr =50 ;It's safer to use Tkw instead as it's 215 ns minimum in this mode
(in other modes Tkr and Tkw minimums are the same).
Again, To, Td, and Tkw are returned byata_timing_find_mode().
Quoted 6 lines
Quoted 4 lines
drivers/ata/pata_octeon_cf.c line 276
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+/* Td (Seem to need more margin here.... */ + oe_a = Td +20 ;Well, you shouldn't...
Hmm...
Shouldn't need more margin -- that why those a minimum timings.
Quoted 37 lines
Quoted 28 lines
Quoted 28 lines
drivers/ata/pata_octeon_cf.c new file +28
-0+/** +* Handle an I/O request. +* +* @cf: Device to access +* @lba_sector: Starting sector +* @num_sectors: +* Number of sectors to transfer +* @buffer: Data buffer +* @write: Is the a write. Default to a read +*/ +static unsigned int octeon_cf_data_xfer (struct ata_device *dev, +unsigned char *buffer, +unsigned int buflen, +int rw) +{ +struct ata_port *ap = dev->link->ap; +struct octeon_cf_data *ocd = ap->dev->platform_data; +void __iomem *data_addr = ap->ioaddr.data_addr; +unsigned int words; +unsigned int count; + +/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 ); +if (ocd->is16bit) { + words = buflen /2 ;Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 341
Quoted 4 lines
drivers/ata/pata_octeon_cf.c new file +4
-0+if (rw) { + count =16 ; +while (words--) { +iowrite16 (*(uint16_t *)buffer, data_addr);Not seeing the reason to use
iowrite16()and notwritew()as
registers are always memory mapped...
Quoted 1 line
I will consider that.
Now that I think again, neither of those is correct. We must treat the
data as a stream of bytes.
Quoted 10 lines
Quoted 9 lines
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+ buffer +=sizeof (uint16_t ); +/* +* Every 16 writes do a read so the +* bootbus FIFO doesn't fill up. +*/ +if (--count ==0 ) { +ioread8 (ap->ioaddr.altstatus_addr); + count =16 ; + }
Quoted 3 lines
Quoted 1 line
This is a strange/slow way of doing anything every 16 data writes.
Oh, how would you do something every 16 writes?
count = words > 16 ? 16 : words; words -= count; iowrite16_rep(ptr, data_addr, count); ioread(ap->ioaddr.altstatus_addr);
Quoted 3 lines
Quoted 1 line
Why not use
iowrite16_rep()?Two reasons. It is broken, and we need to do something every 16 writes.
It's not broken -- using iowrite16() is wrong.
Quoted 1 line
We need to be careful not to be anti-social by filling the FIFO. Once the FIFO is filled other cores will be starved of I/O resources.
It's not the first time I'm seeing this driver, so I'm familiar with the
trick. Altho I didn't realize that this abuses the shared write buffer.
Quoted 15 lines
[...]
Quoted 4 lines
Quoted 4 lines
+static void
octeon_cf_tf_read16(struct ata_port *ap, struct ata_taskfile *tf)(quoted diff) +2
-0+{ + u16 blob;Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 392
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/* The base of the registers is at ioaddr.data_addr. */ +void __iomem *base = ap->ioaddr.data_addr; + + blob =__raw_readw (base +0xc ); + tf->feature = blob >>8 ;How come the error register gets mapped at offset
0xC? (Well, that would explain alike part in another CF driver.)
Quoted 2 lines
In 16 bit mode it overlaps the data register, so it must be accessed via the
0xcaddress.
Actually, these register *always* overlap. That's the way that IDE thing
works. :-)
But I'm getting it now -- no way to differ the data read/write from the
error/feature register read/write in 16-bit mode.
Quoted 11 lines
Quoted 7 lines
drivers/ata/pata_octeon_cf.c line 707
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+/** +* Check if any queued commands have more DMAs, if so start the next +* transfer, else do standard handling. +*/ +irqreturn_t octeon_cf_interrupt (int irq,void *dev_instance)Is that a normal IDE interrupt, or some Octeon specific interrupt?
No, It is OCTEON specific. It fires when a DMA operation is complete.
As I mentioned earlier, the normal IDE interrupt is not connected to anything on most boards.
That's not exactly a good design desicion...
Quoted 10 lines
Quoted 10 lines
drivers/ata/pata_octeon_cf.c line 717
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+{ +struct ata_host *host = dev_instance; +struct octeon_cf_port *cf_port; +int i; +unsigned int handled =0 ; +unsigned long flags; + +spin_lock_irqsave (&host->lock, flags); + +DPRINTK ("ENTER\n" );
"ENTER" needs "EXIT", no?
Quoted 24 lines
Quoted 15 lines
Quoted 15 lines
drivers/ata/pata_octeon_cf.c new file +15
-0+for (i =0 ; i < host->n_ports; i++) { +struct ata_port *ap; +struct ata_queued_cmd *qc; + + ap = host->ports[i]; +if (!ap || (ap->flags &ATA_FLAG_DISABLED )) +continue ; + + qc =ata_qc_from_tag (ap, ap->link.active_tag); +if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && + (qc->flags &ATA_QCFLAG_ACTIVE )) { +u8 status =octeon_cf_bmdma_status (ap); +if ((status &ATA_DMA_INTR ) + && !(status &ATA_DMA_ACTIVE ) + && !sg_is_last (qc->cursg)) {Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 738
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+ qc->cursg =sg_next (qc->cursg); + handled =1 ; +octeon_cf_bmdma_start (qc); + }else { + status =ioread8 (ap->ioaddr.altstatus_addr); +if (status &ATA_BUSY ) {If it's normal IDE interrupt,
Quoted 5 lines
Which it is not. So...
Quoted 1 line
BSY bit will always be 0
...this doesn't hold, thus all this code.
Not necessarily. Depends on when the DMA interrupt occurs. Although it
probably predates the IDE interrupt...
Quoted 18 lines
Quoted 17 lines
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +6
-0+static int __devinit octeon_cf_probe (struct platform_device *pdev) +{ +struct resource *res_cs0, *res_cs1; + +void __iomem *cs0; +void __iomem *cs1;[...]
Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +3
-0+ + res_cs0 =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + res_cs1 =platform_get_resource (pdev,IORESOURCE_MEM ,1 );[...]
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+ base = cs0 + ocd->base_region_bias; +if (!ocd->is16bit) {
Quoted 8 lines
Quoted 6 lines
[...]
Review
Quoted 2 lines
(quoted diff) +2
-0+ ap->ioaddr.altstatus_addr = base + 0xe; + ap->ioaddr.ctl_addr = base + 0xe;Wait, why have 2 MMIO resources then?
The boards that are 'TrueIDE' use two resources, others only one.
But your probe will abort seeing only one resource. I think this needs to
be changed.
MBR, Sergei
Quoted 5 lines · Sergei Shtylyov
Quoted 2 lines
We need to be careful not to be anti-social by filling the FIFO. Once the FIFO is filled other cores will be starved of I/O resources.
It's not the first time I'm seeing this driver, so I'm familiar with the
trick. Altho I didn't realize that this abuses the shared write buffer.
There is a FIFO between the bootbus and the cores separate from the normal Octeon per core write buffer. This fifo is shared will all other cores and IO units for bootbus access. If the fifo overflows then IOs other than the bootbus are affected.
Chad
Quoted 4 lines
Quoted 1 line
+
iowrite16(*(uint16_t *)buffer, data_addr);Not seeing the reason to use
iowrite16()and notwritew()as registers are
always memory mapped...
iomap uses iowrite/ioread, writew is for the older ioremap stuff. Mixing them up is not guaranteed to work for all ports.
Quoted 4 lines
Quoted 2 lines
+ * Get the status of the DMA engine. The results of this function
+ * must emulate the BMDMA engine expected by libata.Ugh... At least the IDE core is not so retarded.
Nor is libata, but he is piggybacking on the SFF code hence this.
Alan
Hello.
Quoted 25 lines · David Daney
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support on the CF interface so for these, only PIO is supported. Although if DMA is available, we do take advantage of it.
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Thanks,
Signed-off-by David Daney <[email protected]>drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@
[...]
Quoted 45 lines
drivers/ata/pata_octeon_cf.c new file +45
-0+/** +* Handle an I/O request. +* +* @cf: Device to access +* @lba_sector: Starting sector +* @num_sectors: +* Number of sectors to transfer +* @buffer: Data buffer +* @write: Is the a write. Default to a read +*/ +static unsigned int octeon_cf_data_xfer (struct ata_device *dev, +unsigned char *buffer, +unsigned int buflen, +int rw) +{ +struct ata_port *ap = dev->link->ap; +struct octeon_cf_data *ocd = ap->dev->platform_data; +void __iomem *data_addr = ap->ioaddr.data_addr; +unsigned int words; +unsigned int count; + +/* +* Odd lengths are not supported. We should always be a +* multiple of 512. +*/ +BUG_ON (buflen &1 ); +if (ocd->is16bit) { + words = buflen /2 ; +if (rw) { + count =16 ; +while (words--) { +iowrite16 (*(uint16_t *)buffer, data_addr); + buffer +=sizeof (uint16_t ); +/* +* Every 16 writes do a read so the +* bootbus FIFO doesn't fill up. +*/ +if (--count ==0 ) { +ioread8 (ap->ioaddr.altstatus_addr); + count =16 ; + } + } + }else { +while (words--) { + *(uint16_t *)buffer =ioread16 (data_addr);
drivers/ata/pata_octeon_cf.c line 364
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+ buffer +=sizeof (uint16_t ); + } + } + }else { +/* 8-bit */ + words = buflen; +if (rw) { + count =16 ; +while (words--) { +iowrite8 (*buffer, data_addr);
About the 8-bit mode: you need to issue the Set Features command with
opcode 1 to enable that mode -- libata currently doesn't do that, so it won't work I suppose...
MBR, Sergei
Quoted 3 lines
About the 8-bit mode: you need to issue the Set Features command with
opcode 1 to enable that mode -- libata currently doesn't do that, so it won't work I suppose...
That depends on the hardware interface. I have worked with at least one board that is 8bit to the CPU for data transfers and 16bit to the disk. Also Set Features 8bit is strictly for TrueIDE mode (see CF 4.0 spec pg 150).
Alan
Hello.
Quoted 18 lines · David Daney
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support on the CF interface so for these, only PIO is supported. Although if DMA is available, we do take advantage of it.
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Thanks,
Signed-off-by David Daney <[email protected]>
[...]
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@
[...]
Quoted 9 lines
drivers/ata/pata_octeon_cf.c new file +9
-0+/** +* Get the status of the DMA engine. The results of this function +* must emulate the BMDMA engine expected by libata. +* +* @ap: ATA port to check status on +* +* Returns BMDMA status bits +*/ +static uint8_t octeon_cf_bmdma_status (struct ata_port *ap)
drivers/ata/pata_octeon_cf.c line 655
Quoted 10 lines
drivers/ata/pata_octeon_cf.c new file +10
-0+{ +struct octeon_cf_data *ocd = ap->dev->platform_data; +cvmx_mio_boot_dma_intx_t mio_boot_dma_int; +cvmx_mio_boot_dma_cfgx_t mio_boot_dma_cfg; +uint8_t result =0 ; + + mio_boot_dma_int.u64 = +cvmx_read_csr (CVMX_MIO_BOOT_DMA_INTX (ocd->dma_engine)); +if (mio_boot_dma_int.s.done) + result |=ATA_DMA_INTR ;
But if you're saying that there is only DMA completion inetrrupt, you
*cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.
MBR, Sergei
Quoted 4 lines
But if you're saying that there is only DMA completion inetrrupt, you
*cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.
I don't see whats wrong with treating it that way if it keeps the amount of code needed down.
Hello.
Quoted 6 lines · Alan Cox
Quoted 3 lines
But if you're saying that there is only DMA completion inetrrupt, you
*cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.I don't see whats wrong with treating it that way if it keeps the amount of code needed down.
Well, as ata_sff_host_intr() doesn't get called, probably nothing
indeed...
MBR, Sergei
Quoted 9 lines · Hello, I wrote:
Quoted 6 lines
Quoted 3 lines
But if you're saying that there is only DMA completion inetrrupt,
you *cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.I don't see whats wrong with treating it that way if it keeps the amount of code needed down.
Well, as
ata_sff_host_intr()doesn't get called, probably nothing
indeed...
Hm, it does get called but only with BSY=0, so all this seems more
tricky than I thought...
MBR, Sergei
Quoted 6 lines · Alan Cox
Quoted 2 lines
But if you're saying that there is only DMA completion inetrrupt, you
*cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.I don't see whats wrong with treating it that way if it keeps the amount of code needed down.
For exactly that reason, I intend to continue to try to emulate it.
David Daney
Hello.
Quoted 8 lines · David Daney
Quoted 6 lines
Quoted 2 lines
But if you're saying that there is only DMA completion inetrrupt,
you *cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.I don't see whats wrong with treating it that way if it keeps the amount of code needed down.
For exactly that reason, I intend to continue to try to emulate it.
"You do or you do not. There is no try." :-D
Quoted 1 line
David Daney
MBR, Sergei
Quoted 40 lines · Hello, I wrote:
Quoted 18 lines
As part of our efforts to get the Cavium OCTEON processor support merged (see: http://marc.info/?l=linux-mips&m=122704699515601), we have this CF driver for your consideration.
Most OCTEON variants have *no* DMA or interrupt support on the CF interface so for these, only PIO is supported. Although if DMA is available, we do take advantage of it.
The register definitions are part of the chip support patch set mentioned above, and are not included here.
At this point I would like to get feedback on the patch and would expect that it would merge via the linux-mips tree along with the rest of the chip support.
Thanks,
Signed-off-by David Daney <[email protected]>[...]
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@[...]
Quoted 14 lines
drivers/ata/pata_octeon_cf.c new file +14
-0+/** +* Get the status of the DMA engine. The results of this function +* must emulate the BMDMA engine expected by libata. +* +* @ap: ATA port to check status on +* +* Returns BMDMA status bits +*/ +static uint8_t octeon_cf_bmdma_status (struct ata_port *ap) +{ +struct octeon_cf_data *ocd = ap->dev->platform_data; +cvmx_mio_boot_dma_intx_t mio_boot_dma_int; +cvmx_mio_boot_dma_cfgx_t mio_boot_dma_cfg; +uint8_t result =0 ;
Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 655
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+ + mio_boot_dma_int.u64 = +cvmx_read_csr (CVMX_MIO_BOOT_DMA_INTX (ocd->dma_engine)); +if (mio_boot_dma_int.s.done) + result |=ATA_DMA_INTR ;But if you're saying that there is only DMA completion inetrrupt,
you *cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.
My suggestion then is not to emulate the ATA_DMA_INTR bit (always
returning it as 0) and use:
mio_boot_dma_int.u64 =
cvmx_read_csr(CVMX_MIO_BOOT_DMA_INTX(ocd->dma_engine));
if (mio_boot_dma_int.s.done)
directly in octeon_cf_interrupt().
Also, this fragment of octeon_cf_bmdma_status() looks doubtful to me:
drivers/ata/pata_octeon_cf.c line 662
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+else if (mio_boot_dma_cfg.s.size !=0xfffff ) + result |=ATA_DMA_ERR ;
I suppose this only makes sense when DMA interrupt is active. What
does this bitfield mean?
MBR, Sergei
Quoted 7 lines · Sergei Shtylyov
Also, this fragment of
octeon_cf_bmdma_status()looks doubtful to me:drivers/ata/pata_octeon_cf.c line 662
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+else if (mio_boot_dma_cfg.s.size !=0xfffff ) + result |=ATA_DMA_ERR ;I suppose this only makes sense when DMA interrupt is active. What
does this bitfield mean?
When you start the Octeon DMA engine, you program mio_boot_dma_cfg.s.size with the number of 16bit words to transfer. As the DMA runs, the hardware decrements this field. At the end it ends up decrementing past 0 to -1. The above check is simply checking if the DMA completed. Since this specific interrupt can only be generated by the DMA engine, it must have been caused by an error condition if the size is not -1.
Chad
Hello.
Quoted 4 lines · Chad Reese
drivers/ata/pata_octeon_cf.c new file +3
-0+/* +* Don't stop the DMA if the device deasserts DMARQ. Many compact +* flashes deassert DMARQ for a short time between sectors. Instead of
It's perfectly legal to do for any IDE device -- even not on the
sector boundaries.
Quoted 2 lines
+ * stoppng and restarting the DMA, we'll let the hardware do it.
If the
stopping
drivers/ata/pata_octeon_cf.c line 617
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+* DMA is really stopped early due to an error condition, a later +* timeout will force us to stop.
Sigh... So any command error will result in a timeout. I wonder what
hardware genius decided that CF doesn't need an IRQ...
Quoted 5 lines
Quoted 5 lines
Also, this fragment of
octeon_cf_bmdma_status()looks doubtful to me:Quoted 3 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+else if (mio_boot_dma_cfg.s.size !=0xfffff ) + result |=ATA_DMA_ERR ;
Quoted 9 lines
Quoted 3 lines
I suppose this only makes sense when DMA interrupt is active. What
does this bitfield mean?When you start the Octeon DMA engine, you program mio_boot_dma_cfg.s.size with the number of 16bit words to transfer. As the DMA runs, the hardware decrements this field. At the end it ends up decrementing past 0 to -1. The above check is simply checking if the DMA
What warrants that 0xfffff doesn't mean 2 MB transfer?
Quoted 4 lines
completed. Since this specific interrupt can only be generated by the DMA engine, it must have been caused by an error condition if the size is not -1.
Note that In the real SFF-8038i BMDMA, error bit doesn't cause an
interrupt...
Quoted 2 lines
Chad
MBR, Sergei
Quoted 6 lines · I just
Quoted 2 lines
+ * stoppng and restarting the DMA, we'll let the hardware do it.
If thestopping
Quoted 1 line
+ * DMA is really stopped early due to an error condition, a later
I'm not sure which error condition is meant here: ERR=1 in the status
register, some internal DMA error, both?
Quoted 4 lines
Quoted 1 line
+ * timeout will force us to stop.
Sigh... So any command error will result in a timeout. I wonder what
hardware genius decided that CF doesn't need an IRQ...
All this makes me think that since the DMA error condition cannot be
reliably detected, you'd better not try to emulate the BMDMA error bit as well...
Quoted 15 lines
Quoted 11 lines
Quoted 5 lines
drivers/ata/pata_octeon_cf.c line 662
Quoted 2 lines
drivers/ata/pata_octeon_cf.c new file +2
-0+else if (mio_boot_dma_cfg.s.size !=0xfffff ) + result |=ATA_DMA_ERR ;I suppose this only makes sense when DMA interrupt is active.
What does this bitfield mean?When you start the Octeon DMA engine, you program mio_boot_dma_cfg.s.size with the number of 16bit words to transfer. As the DMA runs, the hardware decrements this field. At the end it ends up decrementing past 0 to -1. The above check is simply checking if the DMA
Also, this fragment of
octeon_cf_bmdma_status()looks doubtful to me:What warrants that
0xfffffdoesn't mean 2 MB transfer?
Quoted 7 lines
Quoted 4 lines
completed. Since this specific interrupt can only be generated by the DMA engine, it must have been caused by an error condition if the size is not -1.
Note that In the real SFF-8038i BMDMA, error bit doesn't cause an
interrupt...
So, Octeon DMA can actually generate an error interrupt?
MBR, Sergei
Quoted 14 lines · Hello, I wrote:
Quoted 6 lines
Quoted 2 lines
+ * stoppng and restarting the DMA, we'll let the hardware do
it. If thestopping
Quoted 1 line
+ * DMA is really stopped early due to an error condition, a later
I'm not sure which error condition is meant here: ERR=1 in the
status register, some internal DMA error, both?Quoted 4 lines
Quoted 1 line
+ * timeout will force us to stop.
Sigh... So any command error will result in a timeout. I wonder
what hardware genius decided that CF doesn't need an IRQ...
Ah, I forgot thet libata should be supporting polling mode, so should
handle ERR=1 case. In this case the comment about the timeout contradicts your words about an interrupt being generated on DMA error.
Quoted 1 line
So, Octeon DMA can actually generate an error interrupt?
MBR, Sergei
Quoted 29 lines · Hello, I wrote:
Quoted 21 lines
Quoted 6 lines
drivers/ata/pata_octeon_cf.c new file +0
-0@@ -0,0 +1,942 @@[...]
Quoted 14 lines
drivers/ata/pata_octeon_cf.c new file +14
-0+/** +* Get the status of the DMA engine. The results of this function +* must emulate the BMDMA engine expected by libata. +* +* @ap: ATA port to check status on +* +* Returns BMDMA status bits +*/ +static uint8_t octeon_cf_bmdma_status (struct ata_port *ap) +{ +struct octeon_cf_data *ocd = ap->dev->platform_data; +cvmx_mio_boot_dma_intx_t mio_boot_dma_int; +cvmx_mio_boot_dma_cfgx_t mio_boot_dma_cfg; +uint8_t result =0 ;Quoted 8 lines
drivers/ata/pata_octeon_cf.c line 655
Quoted 5 lines
drivers/ata/pata_octeon_cf.c new file +5
-0+ + mio_boot_dma_int.u64 = +cvmx_read_csr (CVMX_MIO_BOOT_DMA_INTX (ocd->dma_engine)); +if (mio_boot_dma_int.s.done) + result |=ATA_DMA_INTR ;But if you're saying that there is only DMA completion inetrrupt,
you *cannot* completely emulate SFF-8038i BMDMA since its interrupt status is the (delayed) IDE INTRQ status. I suggest that you move away from the emulation -- Alan has said it's possible.
Quoted 4 lines
As part of our efforts to get the Cavium OCTEON processor support
My suggestion then is not to emulate the
ATA_DMA_INTRbit (always
returning it as 0)
No, this is not an option because you chain to ata_sff_host_intr().
However, DMA interrupt status is *not* a substitute for SFF-8038i interrupt status (as it gets set on the S/G segment boundaries too), so you should employ the pure software emulation for this bit, only triggering before the control gets passed to ata_sff_host_intr().
Quoted 2 lines
directly in
octeon_cf_interrupt().
MBR, Sergei