MPC5200 Bestcomm platform driver
cover MPC5200 Bestcomm platform driver
Hi Sylvain
This is first part of "platformizied" bestcomm/fec drivers.
Comments/Commit?
--
Regards
Andrey Volkov
arch/ppc/Kconfig +5 -0
@@ -733,6 +733,11 @@ config EMBEDDEDBOOT config PPC_MPC52xx bool +config PPC_BESTCOMM + bool + depends on PPC_MPC52xx + default y + config 8260 bool "CPM2 Support" if WILLOW depends on 6xx
arch/ppc/syslib/Makefile +1 -0
@@ -116,3 +116,4 @@ obj-$(CONFIG_PPC_MPC52xx) += mpc52xx_set ifeq ($(CONFIG_PPC_MPC52xx),y) obj-$(CONFIG_PCI) += mpc52xx_pci.o endif +obj-$(CONFIG_PPC_BESTCOMM) += bestcomm/
arch/ppc/syslib/bestcomm/Makefile new file +2 -0
@@ -0,0 +1,2 @@ +obj-$(CONFIG_PPC_BESTCOMM) += bestcomm.o +obj-$(CONFIG_FEC_MPC52xx) += sdma_fec_rx_task.o sdma_fec_tx_task.o fec.o
arch/ppc/syslib/bestcomm/bestcomm.c new file +408 -0
408 lines not on this page · raw message
arch/ppc/syslib/bestcomm/bestcomm.h new file +473 -0
473 lines not on this page · raw message
arch/ppc/syslib/bestcomm/fec.c new file +174 -0
174 lines not on this page · raw message
arch/ppc/syslib/bestcomm/fec.h new file +71 -0
71 lines not on this page · raw message
arch/ppc/syslib/bestcomm/sdma_fec_rx_task.c new file +71 -0
71 lines not on this page · raw message
arch/ppc/syslib/bestcomm/sdma_fec_tx_task.c new file +84 -0
84 lines not on this page · raw message
arch/ppc/syslib/mpc52xx_devices.c +22 -0
@@ -34,6 +34,28 @@ static struct fsl_i2c_platform_data mpc5possibly using IORESOURCE_DMA. But that's when BestComm is ready ... */ struct platform_device ppc_sys_platform_devices[] = { + [MPC52xx_SDMA] = { + .name ="mpc52xx-sdma" , + .id =-1 , + .num_resources =3 , + .resource = (struct resource []) { + { + .start =0x1200 , + .end =0x12ff , + .flags =IORESOURCE_MEM , + }, + { + .start =0x8000 , + .end =0xbfff , + .flags =IORESOURCE_MEM , + }, + { + .start = MPC52xx_SDMA_IRQ, + .end = MPC52xx_SDMA_IRQ, + .flags =IORESOURCE_IRQ , + }, + }, + }, [MPC52xx_MSCAN1] = { .name ="mpc52xx-mscan" , .id =0 ,
arch/ppc/syslib/mpc52xx_sys.c +2 -2
@@ -21,10 +21,10 @@ struct ppc_sys_spec ppc_sys_specs[] = { .ppc_sys_name = "5200", .mask = 0xffff0000, .value = 0x80110000, - .num_devices = 15, + .num_devices = MPC52xx_NUM_DEVICES, .device_list = (enum ppc_sys_devices []) { - MPC52xx_MSCAN1, MPC52xx_MSCAN2, MPC52xx_SPI, + MPC52xx_SDMA, MPC52xx_MSCAN1, MPC52xx_MSCAN2, MPC52xx_SPI, MPC52xx_USB, MPC52xx_BDLC, MPC52xx_PSC1, MPC52xx_PSC2, MPC52xx_PSC3, MPC52xx_PSC4, MPC52xx_PSC5, MPC52xx_PSC6, MPC52xx_FEC, MPC52xx_ATA, MPC52xx_I2C1, MPC52xx_I2C2,
include/asm-ppc/mpc52xx.h +3 -0
@@ -34,6 +34,8 @@ struct pt_regs;/* ======================================================================== */ enum ppc_sys_devices { + MPC52xx_SDMA,/* Must be first device to probe, +else FEC/ATA will failed */ MPC52xx_MSCAN1, MPC52xx_MSCAN2, MPC52xx_SPI,
@@ -49,6 +51,7 @@ enum ppc_sys_devices { MPC52xx_ATA, MPC52xx_I2C1, MPC52xx_I2C2, + MPC52xx_NUM_DEVICES };
Hi Andrey,
Quoted 5 lines · Andrey Volkov
Hi Sylvain
This is first part of "platformizied" bestcomm/fec drivers.
Comments/Commit?
Obviously I haven't yet had the time to review all the code but the glance I had looked good ! I'll review it deeper and test it and come back to you asap.
Still, some "preliminary" comments :
- I never really liked to have multiple "type" of buffer descriptors
depending of the number of pointers in them. "standard" task have either 1 or 2 pointers true but I have custom tasks with 3 so I need a subtmitbuffer3 ... Not very extensible imho. I think there is no problem as defining the descriptor structure with an array of pointer and then just allocate the good size at init. Whoever use them must anyway know the number of pointer to fill.
- When I started to clean up bescomm a while ago, the only thing I
really got done was a rewrite of the SRAM allocator that supports the freeing of block at little overcost. I'll try to find it and send it to you.
- I like the separation of phys/virt ;)
- sdma_clear_irq(struct sdma *s) is useless, interrupt acking for the
SDMA is already done in mpc52xx_irq.c
- I thought of separating bestcomm.h in two headers : one public for
the drivers that use the SDMA like the fec. one private for the bestcomm.c and the tasks implementation. I think it makes sense but I never deeply looked it one wouldn't end up almost empty.
... to be continued ;)
Sylvain
Quoted 3 lines · Sylvain Munaut
Obviously I haven't yet had the time to review all the code but the glance I had looked good ! I'll review it deeper and test it and come back to you asap.
As you could see it was only first step (I hope at not so long way :)):
bring Dale's code to the current kernel, and make point from which everyone could start dance.
Quoted 10 lines
Still, some "preliminary" comments :
- I never really liked to have multiple "type" of buffer descriptors
depending of the number of pointers in them. "standard" task have either 1 or 2 pointers true but I have custom tasks with 3 so I need a subtmitbuffer3 ... Not very extensible imho. I think there is no problem as defining the descriptor structure with an array of pointer and then just allocate the good size at init. Whoever use them must anyway know the number of pointer to fill.
Accepted. But I think it will be better to start from another end:
allow everyone to create him/here self task (variable buffers number will consequence of that).
Quoted 4 lines
- When I started to clean up bescomm a while ago, the only thing I
really got done was a rewrite of the SRAM allocator that supports the freeing of block at little overcost. I'll try to find it and send it to you.
I agree with you, sram_free is required function, especially if sdma-depended drivers will loaded/unloaded as modules. But I also agree with Dale's comments: What to do with fragmentations? I could lightly imagine situation, when after some load/unload iterations we receive ENOMEM :(.
Quoted 2 lines
- I like the separation of phys/virt ;)
I'd like it too :), but I had a pa/va headache when setting up task/var/inc tables, so everyone, who will write sdma related code must be very careful.
Quoted 3 lines
-
sdma_clear_irq(struct sdma *s)is useless, interrupt acking for the
SDMA is already done inmpc52xx_irq.c
Ok, will be removed.
Quoted 5 lines
- I thought of separating
bestcomm.hin two headers : one public for
the drivers that use the SDMA like the fec. one private for thebestcomm.cand the tasks implementation. I think it makes sense but I never deeply looked it one wouldn't end up almost empty.
I review code. It will be done bloodless, I hope :).
Quoted 2 lines
... to be continued ;)
Will wait impatiently ;).
Quoted 1 line
Signature · 2 lines
Regards Andrey Volkov
Quoted 7 lines · Andrey Volkov
Quoted 3 lines · Sylvain Munaut
Obviously I haven't yet had the time to review all the code but the glance I had looked good ! I'll review it deeper and test it and come back to you asap.
As you could see it was only first step (I hope at not so long way :)):
bring Dale's code to the current kernel, and make point from which everyone could start dance.
Andrey, thanks for keeping this alive.
I agree with Sylvain's other comments.
The main thing I would like to see happen to this driver (other than getting it in mainline :-) is an attempt to replace the ugly phy code with calls to the new phy abstraction layer. On the other hand, I'm glad to see progress on any front.
-Dale
Quoted 13 lines · Dale Farnsworth
Quoted 9 lines · Andrey Volkov
Quoted 3 lines · Sylvain Munaut
Obviously I haven't yet had the time to review all the code but the glance I had looked good ! I'll review it deeper and test it and come back to you asap.
As you could see it was only first step (I hope at not so long way :)):
bring Dale's code to the current kernel, and make point from which everyone could start dance.
Quoted 8 lines
Andrey, thanks for keeping this alive.
I agree with Sylvain's other comments.
The main thing I would like to see happen to this driver (other than getting it in mainline :-) is an attempt to replace the ugly phy code with calls to the new phy abstraction layer. On the other hand, I'm glad to see progress on any front.
Ok. But if smb, wish do it now -- welcome.
Because I planned do it only after bestcomm will be accepted.
Signature · 2 lines
Regards Andrey Volkov
Quoted 11 lines · Andrey Volkov
Quoted 7 lines
- I never really liked to have multiple "type" of buffer descriptors depending of the number of pointers in them. "standard" task have either 1 or 2 pointers true but I have custom tasks with 3 so I need a subtmitbuffer3 ... Not very extensible imho. I think there is no problem as defining the descriptor structure with an array of pointer and then just allocate the good size at init. Whoever use them must anyway know the number of pointer to fill.
Accepted. But I think it will be better to start from another end:
allow everyone to create him/here self task (variable buffers number will consequence of that).
Sure, see my comments about your bestcomm microcode doc in my preceing mail.
Quoted 9 lines
Quoted 3 lines
- When I started to clean up bescomm a while ago, the only thing I really got done was a rewrite of the SRAM allocator that supports the freeing of block at little overcost. I'll try to find it and send it to you.
I agree with you,
sram_freeis required function, especially if sdma-depended drivers will loaded/unloaded as modules. But I also agree with Dale's comments: What to do with fragmentations? I could lightly imagine situation, when after some load/unload iterations we receive ENOMEM :(.
Sure, fragmentation is a problem but there is no 'easy' way to prevent that ... It's not perfet but better than nothing IMHO.
Quoted 5 lines
Quoted 1 line
- I like the separation of phys/virt ;)
I'd like it too :), but I had a pa/va headache when setting up task/var/inc tables, so everyone, who will write sdma related code must be very careful.
Anyway, they must be careful ;)
Bestcomm is like a very fragile balance between numerous task competing for some dma transfers. Without caution, one will starve the others ...
Sylvain