RE: [PATCH v2]460EX on-chip SATA driverresubmisison
Hi Sergei,
Thanks for your suggestions.
I would look into them and submit a patch for style improvement some time
later.
Regards,
Rup
-Original Message-
From: Sergei Shtylyov [mailto:sshtyl...@mvista.com]
Sent: Saturday, July 17, 2010 9:21 PM
To: Rupjyoti Sarmah
Cc: linux-...@vger.kernel.org; linux-ker...@vger.kernel.org;
rsar...@apm.com; jgar...@pobox.com; s...@denx.de; linuxppc-...@ozlabs.org
Subject: Re: [PATCH v2]460EX on-chip SATA driverresubmisison
Hello.
Rupjyoti Sarmah wrote:
This patch enables the on-chip DWC SATA controller of the AppliedMicro
processor 460EX.
Too bad thius has already been applied but here's my (mostly
stylistic)
comments anyway:
Signed-off-by: Rupjyoti Sarmah rsar...@appliedmicro.com
Signed-off-by: Mark Miesfeld mmiesf...@appliedmicro.com
Signed-off-by: Prodyut Hazarika phazar...@appliedmicro.com
[...]
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
new file mode 100644
index 000..ea24c1e
--- /dev/null
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -0,0 +1,1756 @@
+/*
+ * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
+#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
+#define DEBUG
+#endif
+
+#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
+#define VERBOSE_DEBUG
+#define DEBUG_NCQ
+#endif
[...]
+/* SATA DMA driver Globals */
+#define DMA_NUM_CHANS1
+#define DMA_NUM_CHAN_REGS8
+
+/* SATA DMA Register definitions */
+#define AHB_DMA_BRST_DFLT64 /* 16 data items burst length*/
Please put a space before */.
+struct ahb_dma_regs {
+ struct dma_chan_regschan_regs[DMA_NUM_CHAN_REGS];
+ struct dma_interrupt_regs interrupt_raw;/* Raw Interrupt
*/
+ struct dma_interrupt_regs interrupt_status; /* Interrupt
Status */
+ struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask
*/
+ struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear
*/
+ struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
+#define DMA_CTL_BLK_TS(size)((size) 0x00FFF) /* Blk
Transfer size */
+#define DMA_CHANNEL(ch) (0x0001 (ch))/* Select
channel */
+ /* Enable channel */
+#define DMA_ENABLE_CHAN(ch) ((0x0001 (ch)) |
\
+ ((0x1 (ch)) 8))
+ /* Disable channel */
+#define DMA_DISABLE_CHAN(ch)(0x | ((0x1
(ch)) 8))
What's the point of OR'ing with zero?
+/*
+ * Commonly used DWC SATA driver Macros
+ */
+#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
+ (host)-private_data)
+#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
+ (ap)-host-private_data)
+#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
+ (ap)-private_data)
+#define HSDEV_FROM_QC(qc)((struct sata_dwc_device *)\
+ (qc)-ap-host-private_data)
+#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
+ (hsdevp)-hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
+struct sata_dwc_host_priv {
+ void__iomem *scr_addr_sstatus;
+ u32 sata_dwc_sactive_issued ;
+ u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
+static void sata_dwc_tf_dump(struct ata_taskfile *tf)
+{
+ dev_vdbg(host_pvt.dwc_dev, taskfile cmd: 0x%02x protocol: %s
flags:
+ 0x%lx device: %x\n, tf-command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
+/*
+ * Function: get_burst_length_encode
+ * arguments: datalength: length in bytes of data
+ * returns value to be programmed in register corrresponding to data
length
+ * This value is effectively the log(base 2) of the length
+ */
+static int get_burst_length_encode(int datalength)
+{
+ int items = datalength 2;/* div by 4 to get lword count */
+
+ if (items = 64)
+ return 5;
+
+ if (items = 32)
+ return 4;
+
+ if (items = 16)
+ return 3;
+
+ if (items = 8)
+ return 2;
+
+ if (items = 4)
+ return 1;
+
+ return 0;
+}
Hmm, there should be a function in the kernel to calculate 2^n order
from
size, something like get_count_order()...
+/*
+ * Function: dma_dwc_interrupt
+ * arguments: irq, dev_id, pt_regs
+ * returns channel number if available else -1
+ * Interrupt Handler for DW AHB SATA DMA
+ */
+static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
+{
+ int chan;
+ u32 tfr_reg, err_reg;
+ unsigned long flags;
+ struct sata_dwc_device *hsdev =
+ (struct sata_dwc_device *)hsdev_instance
Re: [PATCH v2]460EX on-chip SATA driverresubmisison
Hello.
Rupjyoti Sarmah wrote:
This patch enables the on-chip DWC SATA controller of the AppliedMicro
processor 460EX.
Too bad thius has already been applied but here's my (mostly stylistic)
comments anyway:
Signed-off-by: Rupjyoti Sarmah rsar...@appliedmicro.com
Signed-off-by: Mark Miesfeld mmiesf...@appliedmicro.com
Signed-off-by: Prodyut Hazarika phazar...@appliedmicro.com
[...]
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
new file mode 100644
index 000..ea24c1e
--- /dev/null
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -0,0 +1,1756 @@
+/*
+ * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
+#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
+#define DEBUG
+#endif
+
+#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
+#define VERBOSE_DEBUG
+#define DEBUG_NCQ
+#endif
[...]
+/* SATA DMA driver Globals */
+#define DMA_NUM_CHANS 1
+#define DMA_NUM_CHAN_REGS 8
+
+/* SATA DMA Register definitions */
+#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/
Please put a space before */.
+struct ahb_dma_regs {
+ struct dma_chan_regschan_regs[DMA_NUM_CHAN_REGS];
+ struct dma_interrupt_regs interrupt_raw;/* Raw Interrupt */
+ struct dma_interrupt_regs interrupt_status; /* Interrupt Status */
+ struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask */
+ struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear */
+ struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
+#defineDMA_CTL_BLK_TS(size)((size) 0x00FFF) /* Blk
Transfer size */
+#define DMA_CHANNEL(ch)(0x0001 (ch)) /* Select
channel */
+ /* Enable channel */
+#defineDMA_ENABLE_CHAN(ch) ((0x0001 (ch)) |
\
+((0x1 (ch)) 8))
+ /* Disable channel */
+#defineDMA_DISABLE_CHAN(ch)(0x | ((0x1 (ch))
8))
What's the point of OR'ing with zero?
+/*
+ * Commonly used DWC SATA driver Macros
+ */
+#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
+ (host)-private_data)
+#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
+ (ap)-host-private_data)
+#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
+ (ap)-private_data)
+#define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\
+ (qc)-ap-host-private_data)
+#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
+ (hsdevp)-hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
+struct sata_dwc_host_priv {
+ void__iomem *scr_addr_sstatus;
+ u32 sata_dwc_sactive_issued ;
+ u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
+static void sata_dwc_tf_dump(struct ata_taskfile *tf)
+{
+ dev_vdbg(host_pvt.dwc_dev, taskfile cmd: 0x%02x protocol: %s flags:
+ 0x%lx device: %x\n, tf-command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
+/*
+ * Function: get_burst_length_encode
+ * arguments: datalength: length in bytes of data
+ * returns value to be programmed in register corrresponding to data length
+ * This value is effectively the log(base 2) of the length
+ */
+static int get_burst_length_encode(int datalength)
+{
+ int items = datalength 2; /* div by 4 to get lword count */
+
+ if (items = 64)
+ return 5;
+
+ if (items = 32)
+ return 4;
+
+ if (items = 16)
+ return 3;
+
+ if (items = 8)
+ return 2;
+
+ if (items = 4)
+ return 1;
+
+ return 0;
+}
Hmm, there should be a function in the kernel to calculate 2^n order from
size, something like get_count_order()...
+/*
+ * Function: dma_dwc_interrupt
+ * arguments: irq, dev_id, pt_regs
+ * returns channel number if available else -1
+ * Interrupt Handler for DW AHB SATA DMA
+ */
+static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
+{
+ int chan;
+ u32 tfr_reg, err_reg;
+ unsigned long flags;
+ struct sata_dwc_device *hsdev =
+ (struct sata_dwc_device *)hsdev_instance;
+ struct ata_host *host = (struct ata_host *)hsdev-host;
+ struct ata_port *ap;
+ struct sata_dwc_device_port *hsdevp;
+ u8 tag = 0;
+ unsigned int port = 0;
+
+ spin_lock_irqsave(host-lock, flags);
+ ap = host-ports[port];
+ hsdevp = HSDEVP_FROM_AP(ap);
+ tag = ap-link.active_tag;
+
+ tfr_reg = in_le32((host_pvt.sata_dma_regs-interrupt_status.tfr\
There's no need to use
Re: [PATCH v2]460EX on-chip SATA driverresubmisison
On 07/06/2010 07:06 AM, Rupjyoti Sarmah wrote: This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX. Signed-off-by: Rupjyoti Sarmahrsar...@appliedmicro.com Signed-off-by: Mark Miesfeldmmiesf...@appliedmicro.com Signed-off-by: Prodyut Hazarikaphazar...@appliedmicro.com --- This patch incorporates the changes advised in the mailing list. The device tree changes were submitted as a seperate patch. Kconfig file is fixed in this version. drivers/ata/Kconfig |9 + drivers/ata/Makefile |1 + drivers/ata/sata_dwc_460ex.c | 1756 ++ 3 files changed, 1766 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/sata_dwc_460ex.c applied ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
