* Re: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems
[not found] <200901130343.56020.yur@emcraft.com>
@ 2009-01-13 2:23 ` David Gibson
2009-01-16 9:03 ` Re[2]: " Yuri Tikhonov
2009-01-15 2:24 ` Anton Vorontsov
1 sibling, 1 reply; 4+ messages in thread
From: David Gibson @ 2009-01-13 2:23 UTC (permalink / raw)
To: Yuri Tikhonov; +Cc: linux-raid, linuxppc-dev, dan.j.williams, wd, dzu, yanok
On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:
> Adds the platform device definitions and the architecture specific support
> routines for the ppc440spe adma driver.
>
> Any board equipped with PPC440SP(e) controller may utilize this driver.
>
> diff --git a/arch/powerpc/boot/dts/katmai.dts b/arch/powerpc/boot/dts/katmai.dts
> index 077819b..f2f77c8 100644
> --- a/arch/powerpc/boot/dts/katmai.dts
> +++ b/arch/powerpc/boot/dts/katmai.dts
> @@ -16,7 +16,7 @@
>
> / {
> #address-cells = <2>;
> - #size-cells = <1>;
> + #size-cells = <2>;
You've changed the root level size-cells, but haven't updated the
sub-nodes (such as /memory) accordingly.
> model = "amcc,katmai";
> compatible = "amcc,katmai";
> dcr-parent = <&{/cpus/cpu@0}>;
> @@ -392,6 +392,30 @@
> 0x0 0x0 0x0 0x3 &UIC3 0xa 0x4 /* swizzled int C */
> 0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled int D */>;
> };
> + DMA0: dma0 {
No 'compatible' property, which seems dubious.
> + interrupt-parent = <&DMA0>;
> + interrupts = <0 1>;
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
> + #size-cells = <0>;
> + interrupt-map = <
> + 0 &UIC0 0x14 4
> + 1 &UIC1 0x16 4>;
> + };
> + DMA1: dma1 {
> + interrupt-parent = <&DMA1>;
> + interrupts = <0 1>;
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
> + #size-cells = <0>;
> + interrupt-map = <
> + 0 &UIC0 0x16 4
> + 1 &UIC1 0x16 4>;
Are these interrupt-maps correct? The second interrupt from both dma
controllers is routed to the same line on UIC1?
> + };
> + xor {
> + interrupt-parent = <&UIC1>;
> + interrupts = <0x1f 4>;
What the hell is this thing? No compatible property, nor even a
meaningful name.
> + };
> };
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems
[not found] <200901130343.56020.yur@emcraft.com>
2009-01-13 2:23 ` [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems David Gibson
@ 2009-01-15 2:24 ` Anton Vorontsov
2009-01-16 12:13 ` Re[2]: " Yuri Tikhonov
1 sibling, 1 reply; 4+ messages in thread
From: Anton Vorontsov @ 2009-01-15 2:24 UTC (permalink / raw)
To: Yuri Tikhonov; +Cc: linux-raid, linuxppc-dev, dan.j.williams, wd, dzu, yanok
Hello Yuri,
On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:
> Adds the platform device definitions and the architecture specific support
> routines for the ppc440spe adma driver.
>
> Any board equipped with PPC440SP(e) controller may utilize this driver.
>
> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
> Signed-off-by: Ilya Yanok <yanok@emcraft.com>
> ---
Quite complex and interesting driver, I must say.
Have you thought about splitting ppc440spe-adma.c into multiple
files, btw?
A few comments down below...
[...]
> +typedef struct ppc440spe_adma_device {
Please avoid typedefs.
[...]
> +/*
> + * Descriptor of allocated CDB
> + */
> +typedef struct {
> + dma_cdb_t *vaddr; /* virtual address of CDB */
> + dma_addr_t paddr; /* physical address of CDB */
> + /*
> + * Additional fields
> + */
> + struct list_head link; /* link in processing list */
> + u32 status; /* status of the CDB */
> + /* status bits: */
> + #define DMA_CDB_DONE (1<<0) /* CDB processing competed */
> + #define DMA_CDB_CANCEL (1<<1) /* waiting thread was interrupted */
> +} dma_cdbd_t;
It seems there are no users of this struct.
[...]
> +typedef struct {
> + xor_cb_t *vaddr;
> + dma_addr_t paddr;
> +
> + /*
> + * Additional fields
> + */
> + struct list_head link; /* link to processing CBs */
> + u32 status; /* status of the CB */
> + /* status bits: */
> + #define XOR_CB_DONE (1<<0) /* CB processing competed */
> + #define XOR_CB_CANCEL (1<<1) /* waiting thread was interrupted */
> +#if 0
> + #define XOR_CB_STALLOC (1<<2) /* CB allocated statically */
> +#endif
Dead code.
[...]
> +static void ppc440spe_configure_raid_devices(void)
> +{
> + void *fifo_buf;
> + volatile i2o_regs_t *i2o_reg;
> + volatile dma_regs_t *dma_reg0, *dma_reg1;
> + volatile xor_regs_t *xor_reg;
> + u32 mask;
> +
> + /*
> + * Map registers and allocate fifo buffer
> + */
> + if (!(i2o_reg = ioremap(I2O_MMAP_BASE, I2O_MMAP_SIZE))) {
> + printk(KERN_ERR "I2O registers mapping failed.\n");
> + return;
> + }
i2o_reg = ioremap(I2O_MMAP_BASE, I2O_MMAP_SIZE);
if (!i20_reg)
...;
would look better.
> + if (!(dma_reg0 = ioremap(DMA0_MMAP_BASE, DMA_MMAP_SIZE))) {
> + printk(KERN_ERR "DMA0 registers mapping failed.\n");
> + goto err1;
> + }
> + if (!(dma_reg1 = ioremap(DMA1_MMAP_BASE, DMA_MMAP_SIZE))) {
> + printk(KERN_ERR "DMA1 registers mapping failed.\n");
> + goto err2;
> + }
> + if (!(xor_reg = ioremap(XOR_MMAP_BASE,XOR_MMAP_SIZE))) {
> + printk(KERN_ERR "XOR registers mapping failed.\n");
> + goto err3;
> + }
[...]
> +static struct platform_device *ppc440spe_devs[] __initdata = {
> + &ppc440spe_dma_0_channel,
> + &ppc440spe_dma_1_channel,
> + &ppc440spe_xor_channel,
> +};
> +
> +static int __init ppc440spe_register_raid_devices(void)
> +{
> + ppc440spe_configure_raid_devices();
> + platform_add_devices(ppc440spe_devs, ARRAY_SIZE(ppc440spe_devs));
> +
> + return 0;
> +}
> +
> +arch_initcall(ppc440spe_register_raid_devices);
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index e34b064..19df08c 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -62,6 +62,19 @@ config MV_XOR
> ---help---
> Enable support for the Marvell XOR engine.
>
> +config AMCC_PPC440SPE_ADMA
> + tristate "AMCC PPC440SPe ADMA support"
It's a tristate, but module_exit() disabled with #if 0...
[...]
> +/******************************************************************************
> + * Command (Descriptor) Blocks low-level routines
> + ******************************************************************************/
> +/**
> + * ppc440spe_desc_init_interrupt - initialize the descriptor for INTERRUPT
> + * pseudo operation
> + */
> +static inline void ppc440spe_desc_init_interrupt (ppc440spe_desc_t *desc,
> + ppc440spe_ch_t *chan)
> +{
Isn't this function way too big for inline?
> + xor_cb_t *p;
> +
> + switch (chan->device->id) {
> + case PPC440SPE_XOR_ID:
> + p = desc->hw_desc;
> + memset (desc->hw_desc, 0, sizeof(xor_cb_t));
> + /* NOP with Command Block Complete Enable */
> + p->cbc = XOR_CBCR_CBCE_BIT;
> + break;
> + case PPC440SPE_DMA0_ID:
> + case PPC440SPE_DMA1_ID:
> + memset (desc->hw_desc, 0, sizeof(dma_cdb_t));
> + /* NOP with interrupt */
> + set_bit(PPC440SPE_DESC_INT, &desc->flags);
> + break;
> + default:
> + printk(KERN_ERR "Unsupported id %d in %s\n", chan->device->id,
> + __func__);
> + break;
> + }
> +}
> +
> +/**
> + * ppc440spe_desc_init_null_xor - initialize the descriptor for NULL XOR
> + * pseudo operation
> + */
> +static inline void ppc440spe_desc_init_null_xor(ppc440spe_desc_t *desc)
> +{
I'd drop the inline.
> + memset (desc->hw_desc, 0, sizeof(xor_cb_t));
> + desc->hw_next = NULL;
> + desc->src_cnt = 0;
> + desc->dst_cnt = 1;
> +}
> +
> +/**
> + * ppc440spe_desc_init_xor - initialize the descriptor for XOR operation
> + */
> +static inline void ppc440spe_desc_init_xor(ppc440spe_desc_t *desc, int src_cnt,
> + unsigned long flags)
> +{
Ditto.
> + xor_cb_t *hw_desc = desc->hw_desc;
> +
> + memset (desc->hw_desc, 0, sizeof(xor_cb_t));
> + desc->hw_next = NULL;
> + desc->src_cnt = src_cnt;
> + desc->dst_cnt = 1;
> +
> + hw_desc->cbc = XOR_CBCR_TGT_BIT | src_cnt;
> + if (flags & DMA_PREP_INTERRUPT)
> + /* Enable interrupt on complete */
> + hw_desc->cbc |= XOR_CBCR_CBCE_BIT;
> +}
> +
> +/**
> + * ppc440spe_desc_init_dma2pq - initialize the descriptor for PQ
> + * operation in DMA2 controller
> + */
> +static inline void ppc440spe_desc_init_dma2pq(ppc440spe_desc_t *desc,
> + int dst_cnt, int src_cnt, unsigned long flags)
> +{
Ditto.
> + xor_cb_t *hw_desc = desc->hw_desc;
> +
> + memset (desc->hw_desc, 0, sizeof(xor_cb_t));
> + desc->hw_next = NULL;
> + desc->src_cnt = src_cnt;
> + desc->dst_cnt = dst_cnt;
> + memset (desc->reverse_flags, 0, sizeof (desc->reverse_flags));
> + desc->descs_per_op = 0;
> +
> + hw_desc->cbc = XOR_CBCR_TGT_BIT;
> + if (flags & DMA_PREP_INTERRUPT)
> + /* Enable interrupt on complete */
> + hw_desc->cbc |= XOR_CBCR_CBCE_BIT;
> +}
> +
> +/**
> + * ppc440spe_desc_init_dma01pq - initialize the descriptors for PQ operation
> + * qith DMA0/1
> + */
> +static inline void ppc440spe_desc_init_dma01pq(ppc440spe_desc_t *desc,
> + int dst_cnt, int src_cnt, unsigned long flags,
> + unsigned long op)
> +{
Way to big for inline. The same for all the inlines.
Btw, ppc_async_tx_find_best_channel() looks too big for inline
and also too big to be in a .h file.
> + dma_cdb_t *hw_desc;
> + ppc440spe_desc_t *iter;
> + u8 dopc;
> +
> + /* Common initialization of a PQ descriptors chain */
> + set_bits(op, &desc->flags);
> + desc->src_cnt = src_cnt;
> + desc->dst_cnt = dst_cnt;
> +
> + /* WXOR MULTICAST if both P and Q are being computed
> + * MV_SG1_SG2 if Q only
> + */
> + dopc = (desc->dst_cnt == DMA_DEST_MAX_NUM) ?
> + DMA_CDB_OPC_MULTICAST : DMA_CDB_OPC_MV_SG1_SG2;
> +
> + list_for_each_entry(iter, &desc->group_list, chain_node) {
> + hw_desc = iter->hw_desc;
> + memset (iter->hw_desc, 0, sizeof(dma_cdb_t));
> +
> + if (likely(!list_is_last(&iter->chain_node,
> + &desc->group_list))) {
> + /* set 'next' pointer */
> + iter->hw_next = list_entry(iter->chain_node.next,
> + ppc440spe_desc_t, chain_node);
> + clear_bit(PPC440SPE_DESC_INT, &iter->flags);
> + } else {
> + /* this is the last descriptor.
> + * this slot will be pasted from ADMA level
> + * each time it wants to configure parameters
> + * of the transaction (src, dst, ...)
> + */
> + iter->hw_next = NULL;
> + if (flags & DMA_PREP_INTERRUPT)
> + set_bit(PPC440SPE_DESC_INT, &iter->flags);
> + else
> + clear_bit(PPC440SPE_DESC_INT, &iter->flags);
> + }
> + }
> +
> + /* Set OPS depending on WXOR/RXOR type of operation */
> + if (!test_bit(PPC440SPE_DESC_RXOR, &desc->flags)) {
> + /* This is a WXOR only chain:
> + * - first descriptors are for zeroing destinations
> + * if PPC440SPE_ZERO_P/Q set;
> + * - descriptors remained are for GF-XOR operations.
> + */
> + iter = list_first_entry(&desc->group_list,
> + ppc440spe_desc_t, chain_node);
> +
> + if (test_bit(PPC440SPE_ZERO_P, &desc->flags)) {
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
> + iter = list_first_entry(&iter->chain_node,
> + ppc440spe_desc_t, chain_node);
> + }
> +
> + if (test_bit(PPC440SPE_ZERO_Q, &desc->flags)) {
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
> + iter = list_first_entry(&iter->chain_node,
> + ppc440spe_desc_t, chain_node);
> + }
> +
> + list_for_each_entry_from(iter, &desc->group_list, chain_node) {
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = dopc;
> + }
> + } else {
> + /* This is either RXOR-only or mixed RXOR/WXOR */
> +
> + /* The first 1 or 2 slots in chain are always RXOR,
> + * if need to calculate P & Q, then there are two
> + * RXOR slots; if only P or only Q, then there is one
> + */
> + iter = list_first_entry(&desc->group_list,
> + ppc440spe_desc_t, chain_node);
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
> +
> + if (desc->dst_cnt == DMA_DEST_MAX_NUM) {
> + iter = list_first_entry(&iter->chain_node,
> + ppc440spe_desc_t, chain_node);
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
> + }
> +
> + /* The remain descs (if any) are WXORs */
> + if (test_bit(PPC440SPE_DESC_WXOR, &desc->flags)) {
> + iter = list_first_entry(&iter->chain_node,
> + ppc440spe_desc_t, chain_node);
> + list_for_each_entry_from(iter, &desc->group_list,
> + chain_node) {
> + hw_desc = iter->hw_desc;
> + hw_desc->opc = dopc;
> + }
> + }
> + }
> +}
[...]
> +/**
> + * ppc440spe_chan_xor_slot_count - get the number of slots necessary for
> + * XOR operation
IIRC kerneldoc does not permit multiline short descriptions. Kdoc tools
will warn about it, I presume.
[...]
> +static int ppc440spe_test_raid6 (ppc440spe_ch_t *chan)
> +{
> + ppc440spe_desc_t *sw_desc, *iter;
> + struct page *pg;
> + char *a;
> + dma_addr_t dma_addr, addrs[2];
> + unsigned long op = 0;
> + int rval = 0;
> +
> + /*FIXME*/
?
> +
> + set_bit(PPC440SPE_DESC_WXOR, &op);
> +
> + pg = alloc_page(GFP_KERNEL);
> + if (!pg)
> + return -ENOMEM;
> +
> +
> +/**
> + * ppc440spe_adma_probe - probe the asynch device
> + */
> +static int __devinit ppc440spe_adma_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
Why is this a platform driver? What's the point of describing
DMA nodes in the device tree w/o actually using them (don't count
interrupts)? There are a lot of hard-coded addresses in the code...
:-/
And arch/powerpc/platforms/44x/ppc440spe_dma_engines.c file
reminds me arch/ppc-style bindings. ;-)
> + int ret=0, irq1, irq2, initcode = PPC_ADMA_INIT_OK;
> + void *regs;
> + ppc440spe_dev_t *adev;
> + ppc440spe_ch_t *chan;
> + ppc440spe_aplat_t *plat_data;
> + struct ppc_dma_chan_ref *ref;
> + struct device_node *dp;
> + char s[10];
> +
[...]
> +static int __init ppc440spe_adma_init (void)
> +{
> + int rval, i;
> + struct proc_dir_entry *p;
> +
> + for (i = 0; i < PPC440SPE_ADMA_ENGINES_NUM; i++)
> + ppc_adma_devices[i] = -1;
> +
> + rval = platform_driver_register(&ppc440spe_adma_driver);
> +
> + if (rval == 0) {
> + /* Create /proc entries */
> + ppc440spe_proot = proc_mkdir(PPC440SPE_R6_PROC_ROOT, NULL);
> + if (!ppc440spe_proot) {
> + printk(KERN_ERR "%s: failed to create %s proc "
> + "directory\n",__func__,PPC440SPE_R6_PROC_ROOT);
> + /* User will not be able to enable h/w RAID-6 */
> + return rval;
> + }
/proc? Why /proc? The driver has nothing to do with Linux VM subsystem
or processes. I think /sys/ interface would suit better for this, no?
Either way, userspace interfaces should be documented somehow
(probably Documentation/ABI/, or at least some comments in the
code).
> + /* GF polynome to use */
> + p = create_proc_entry("poly", 0, ppc440spe_proot);
> + if (p) {
> + p->read_proc = ppc440spe_poly_read;
> + p->write_proc = ppc440spe_poly_write;
> + }
> +
> + /* RAID-6 h/w enable entry */
> + p = create_proc_entry("enable", 0, ppc440spe_proot);
> + if (p) {
> + p->read_proc = ppc440spe_r6ena_read;
> + p->write_proc = ppc440spe_r6ena_write;
> + }
> +
> + /* initialization status */
> + p = create_proc_entry("devices", 0, ppc440spe_proot);
> + if (p) {
> + p->read_proc = ppc440spe_status_read;
> + }
> + }
> + return rval;
> +}
[...]
> +#if 0
> +static void __exit ppc440spe_adma_exit (void)
> +{
> + platform_driver_unregister(&ppc440spe_adma_driver);
> + return;
> +}
> +module_exit(ppc440spe_adma_exit);
> +#endif
Dead code.
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
`irc://irc.freenode.net/bd2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re[2]: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems
2009-01-13 2:23 ` [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems David Gibson
@ 2009-01-16 9:03 ` Yuri Tikhonov
0 siblings, 0 replies; 4+ messages in thread
From: Yuri Tikhonov @ 2009-01-16 9:03 UTC (permalink / raw)
To: David Gibson; +Cc: linux-raid, linuxppc-dev, dan.j.williams, wd, dzu, yanok
Hello David,
Thanks a lot for review.
The general note to be made here is that the changes to the DTS file
made by this patch are necessary for a ppc440spe ADMA driver, which is
a not-completed arch/powerpc port from the arch/ppc branch, and which
uses DT (well, incorrectly) just to get interrupts. Otherwise, it's
just a platform device driver.
We provided this ADMA driver just as the reference of driver, which
implements the RAID-6 related low-level stuff. ppc440spe ADMA in its
current state is far from ready for merging. We'll elaborate on its
cleaning up then (surely, taking into account all the comments made
from community). But, even now, the driver works, so we publish this
so interested people could use and test it.
Some comments mixed in below.
On Tuesday, January 13, 2009 you wrote:
> On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:
>> Adds the platform device definitions and the architecture specific support
>> routines for the ppc440spe adma driver.
>>
>> Any board equipped with PPC440SP(e) controller may utilize this driver.
>>
>> diff --git a/arch/powerpc/boot/dts/katmai.dts b/arch/powerpc/boot/dts/katmai.dts
>> index 077819b..f2f77c8 100644
>> --- a/arch/powerpc/boot/dts/katmai.dts
>> +++ b/arch/powerpc/boot/dts/katmai.dts
>> @@ -16,7 +16,7 @@
>>
>> / {
>> #address-cells = <2>;
>> - #size-cells = <1>;
>> + #size-cells = <2>;
> You've changed the root level size-cells, but haven't updated the
> sub-nodes (such as /memory) accordingly.
Thanks, we'll fix this in the next version of this patch.
>> model = "amcc,katmai";
>> compatible = "amcc,katmai";
>> dcr-parent = <&{/cpus/cpu@0}>;
>> @@ -392,6 +392,30 @@
>> 0x0 0x0 0x0 0x3 &UIC3 0xa 0x4 /* swizzled int C */
>> 0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /* swizzled int D */>;
>> };
>> + DMA0: dma0 {
> No 'compatible' property, which seems dubious.
OK, we'll fix.
>> + interrupt-parent = <&DMA0>;
>> + interrupts = <0 1>;
>> + #interrupt-cells = <1>;
>> + #address-cells = <0>;
>> + #size-cells = <0>;
>> + interrupt-map = <
>> + 0 &UIC0 0x14 4
>> + 1 &UIC1 0x16 4>;
>> + };
>> + DMA1: dma1 {
>> + interrupt-parent = <&DMA1>;
>> + interrupts = <0 1>;
>> + #interrupt-cells = <1>;
>> + #address-cells = <0>;
>> + #size-cells = <0>;
>> + interrupt-map = <
>> + 0 &UIC0 0x16 4
>> + 1 &UIC1 0x16 4>;
> Are these interrupt-maps correct? The second interrupt from both dma
> controllers is routed to the same line on UIC1?
The map is correct:
- first interrupts are 'DMAx Command Status FIFO Needs Service';
- second interrupt is 'DMA Error', both DMA engines share common error IRQ.
>> + };
>> + xor {
>> + interrupt-parent = <&UIC1>;
>> + interrupts = <0x1f 4>;
> What the hell is this thing? No compatible property, nor even a
> meaningful name.
This is the XOR accelerator, the dedicated DMA engine of ppc440spe
equipped with the ability to do XOR operations in h/w. I guess, it
could be named like DMA2.
Regards, Yuri
--
Yuri Tikhonov, Senior Software Engineer
Emcraft Systems, www.emcraft.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re[2]: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems
2009-01-15 2:24 ` Anton Vorontsov
@ 2009-01-16 12:13 ` Yuri Tikhonov
0 siblings, 0 replies; 4+ messages in thread
From: Yuri Tikhonov @ 2009-01-16 12:13 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: linux-raid, linuxppc-dev, dan.j.williams, wd, dzu, yanok
Hello Anton,
Thanks for review. Please note the general note I made in "Re[2]:
[PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e)
systems".
All your comments make sense, so we'll try to address these in the
next version of the driver. Some comments below.
On Thursday, January 15, 2009 you wrote:
> Hello Yuri,
> On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:
>> Adds the platform device definitions and the architecture specific support
>> routines for the ppc440spe adma driver.
>>
>> Any board equipped with PPC440SP(e) controller may utilize this driver.
>>
>> Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
>> Signed-off-by: Ilya Yanok <yanok@emcraft.com>
>> ---
> Quite complex and interesting driver, I must say.
> Have you thought about splitting ppc440spe-adma.c into multiple
> files, btw?
Admittedly, no. But I guess this makes sense. The driver supports two
different types of DMA devices of ppc440spe: DMA0,1 and DMA2[XOR
engine]. So, we could split the driver at least in two, which would
definitely simplified the code.
> A few comments down below...
> [...]
>> +typedef struct ppc440spe_adma_device {
> Please avoid typedefs.
OK.
> [...]
>> +/*
>> + * Descriptor of allocated CDB
>> + */
>> +typedef struct {
>> + dma_cdb_t *vaddr; /* virtual address of CDB */
>> + dma_addr_t paddr; /* physical address of CDB */
>> + /*
>> + * Additional fields
>> + */
>> + struct list_head link; /* link in processing list */
>> + u32 status; /* status of the CDB */
>> + /* status bits: */
>> + #define DMA_CDB_DONE (1<<0) /* CDB processing competed */
>> + #define DMA_CDB_CANCEL (1<<1) /* waiting thread was interrupted */
>> +} dma_cdbd_t;
> It seems there are no users of this struct.
Indeed. This is an useless inheritance of some old version of the
driver. Will remove this in the next patch.
[..]
>> +/**
>> + * ppc440spe_desc_init_dma01pq - initialize the descriptors for PQ operation
>> + * qith DMA0/1
>> + */
>> +static inline void ppc440spe_desc_init_dma01pq(ppc440spe_desc_t *desc,
>> + int dst_cnt, int src_cnt, unsigned long flags,
>> + unsigned long op)
>> +{
> Way to big for inline. The same for all the inlines.
> Btw, ppc_async_tx_find_best_channel() looks too big for inline
> and also too big to be in a .h file.
OK, will be moved to the appropriate .c.
[..]
> [...]
>> +static int ppc440spe_test_raid6 (ppc440spe_ch_t *chan)
>> +{
>> + ppc440spe_desc_t *sw_desc, *iter;
>> + struct page *pg;
>> + char *a;
>> + dma_addr_t dma_addr, addrs[2];
>> + unsigned long op = 0;
>> + int rval = 0;
>> +
>> + /*FIXME*/
> ?
>> +
>> + set_bit(PPC440SPE_DESC_WXOR, &op);
>> +
>> + pg = alloc_page(GFP_KERNEL);
>> + if (!pg)
>> + return -ENOMEM;
>> +
>> +
>> +/**
>> + * ppc440spe_adma_probe - probe the asynch device
>> + */
>> +static int __devinit ppc440spe_adma_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
> Why is this a platform driver? What's the point of describing
> DMA nodes in the device tree w/o actually using them (don't count
> interrupts)? There are a lot of hard-coded addresses in the code...
> :-/
> And arch/powerpc/platforms/44x/ppc440spe_dma_engines.c file
> reminds me arch/ppc-style bindings. ;-)
Right. This driver is a not-completed port from the arch/ppc branch.
>> + int ret=0, irq1, irq2, initcode = PPC_ADMA_INIT_OK;
>> + void *regs;
>> + ppc440spe_dev_t *adev;
>> + ppc440spe_ch_t *chan;
>> + ppc440spe_aplat_t *plat_data;
>> + struct ppc_dma_chan_ref *ref;
>> + struct device_node *dp;
>> + char s[10];
>> +
> [...]
>> +static int __init ppc440spe_adma_init (void)
>> +{
>> + int rval, i;
>> + struct proc_dir_entry *p;
>> +
>> + for (i = 0; i < PPC440SPE_ADMA_ENGINES_NUM; i++)
>> + ppc_adma_devices[i] = -1;
>> +
>> + rval = platform_driver_register(&ppc440spe_adma_driver);
>> +
>> + if (rval == 0) {
>> + /* Create /proc entries */
>> + ppc440spe_proot = proc_mkdir(PPC440SPE_R6_PROC_ROOT, NULL);
>> + if (!ppc440spe_proot) {
>> + printk(KERN_ERR "%s: failed to create %s proc "
>> + "directory\n",__func__,PPC440SPE_R6_PROC_ROOT);
>> + /* User will not be able to enable h/w RAID-6 */
>> + return rval;
>> + }
> /proc? Why /proc? The driver has nothing to do with Linux VM subsystem
> or processes. I think /sys/ interface would suit better for this, no?
> Either way, userspace interfaces should be documented somehow
> (probably Documentation/ABI/, or at least some comments in the
> code).
Agree, we'll fix this.
Regards, Yuri
--
Yuri Tikhonov, Senior Software Engineer
Emcraft Systems, www.emcraft.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-01-16 12:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200901130343.56020.yur@emcraft.com>
2009-01-13 2:23 ` [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems David Gibson
2009-01-16 9:03 ` Re[2]: " Yuri Tikhonov
2009-01-15 2:24 ` Anton Vorontsov
2009-01-16 12:13 ` Re[2]: " Yuri Tikhonov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).