Re: [U-Boot-Users] reg ISP 1561 integration with u-boot1.1.6
Hi,
it's working. I modified the testing-USB branch to run against a PCI OHCI controller (Philips ISP1561) on the APC405 board (PowerPC PPC405GPr).
Enumeration and USB storage is working. I do not dare to say 'working well'.
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset to access the system RAM by the busmastering OHCI controller. I called this constant CFG_PCIRAM_BASE. But the name is a little bit confusion, isn't it?
Here are my (first) questions: 1) What do I have to do in usb_board_stop() and usb_board_init_fail()? My current code can only initialize the OHCI controller once. A 2nd 'usb start' does not find any devices.
2) I had to disable the 'return -1' statement for an unfinished urb in sohci_submit_job(). Why isn't it finished?
Please do not blame me for the board dependent code still in the usb_ohci.c file. I will move it later.
The modified code is not tested on any other board (e.g. with on-chip OHCI controllers). I might break other boards.
Matthias
On Friday 05 January 2007 06:43, mahendra varman wrote:
Hi,
Have u successfully enumerated the usb devices thru ISP1561 ? Iam facing problem even in enumeration ....
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
it's working. I modified the testing-USB branch to run against a PCI OHCI controller (Philips ISP1561) on the APC405 board (PowerPC PPC405GPr).
Enumeration and USB storage is working. I do not dare to say 'working well'.
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
to access the system RAM by the busmastering OHCI controller. I called this constant CFG_PCIRAM_BASE. But the name is a little bit confusion, isn't it?
Here are my (first) questions:
- What do I have to do in usb_board_stop() and usb_board_init_fail()?
My current code can only initialize the OHCI controller once. A 2nd 'usb start' does not find any devices.
You might not have to do anything, but maybe you'll need to do some board dependant cleanup for example. If not just leave them empty.
- I had to disable the 'return -1' statement for an unfinished urb in
sohci_submit_job(). Why isn't it finished?
I really don't know, I guess only you can answer this!
Please do not blame me for the board dependent code still in the usb_ohci.c file. I will move it later.
Who else shall we blame then ;-) ?
Regards
Markus Klotzbuecher
On Wednesday 10 January 2007 23:01, Markus Klotzbücher wrote:
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
it's working. I modified the testing-USB branch to run against a PCI OHCI controller (Philips ISP1561) on the APC405 board (PowerPC PPC405GPr).
Enumeration and USB storage is working. I do not dare to say 'working well'.
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
to access the system RAM by the busmastering OHCI controller. I called this constant CFG_PCIRAM_BASE. But the name is a little bit confusion, isn't it?
Here are my (first) questions:
- What do I have to do in usb_board_stop() and usb_board_init_fail()?
My current code can only initialize the OHCI controller once. A 2nd 'usb start' does not find any devices.
You might not have to do anything, but maybe you'll need to do some board dependant cleanup for example. If not just leave them empty.
I thought that I have to do something here because stopping and restarting the usb stuff does not work at the moment on my system. Does it work on other systems? I do not have any target with onchip USB at the moment (...but comming soon :-). Did anyone test the merged code on a 440EP so far?
- I had to disable the 'return -1' statement for an unfinished urb in
sohci_submit_job(). Why isn't it finished?
I really don't know, I guess only you can answer this!
This answer helps me a lot. Why is the urb_finished stuff needed? I go better without :-)
Please do not blame me for the board dependent code still in the usb_ohci.c file. I will move it later.
Who else shall we blame then ;-) ?
So please blame be. I just though about leaving the generic PCI OHCI code in the file. When it has been made fully generic, it is not CPU or even board dependant. PCI device identification constants (better classcode) could be defined in the board config header and that's it for CONFIG_USB_OHCI_PCI.
Matthias
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
On Wednesday 10 January 2007 23:01, Markus Klotzbücher wrote:
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
I get it. So in the end we have four cases: byte swapping register access or not _and_ byte swapping data or not. Right? Doesn't sound too complicated.
Here are my (first) questions:
- What do I have to do in usb_board_stop() and usb_board_init_fail()?
My current code can only initialize the OHCI controller once. A 2nd 'usb start' does not find any devices.
You might not have to do anything, but maybe you'll need to do some board dependant cleanup for example. If not just leave them empty.
I thought that I have to do something here because stopping and restarting the usb stuff does not work at the moment on my system. Does it work on other systems? I do not have any target with onchip USB at the moment (...but comming soon :-). Did anyone test the merged code on a 440EP so far?
Yes, I did test it successfully.
- I had to disable the 'return -1' statement for an unfinished urb in
sohci_submit_job(). Why isn't it finished?
I really don't know, I guess only you can answer this!
This answer helps me a lot. Why is the urb_finished stuff needed? I go better without :-)
Did you see the comment in usb_ohci.c:1355? It's to make sure a transaction has completed before a new one is started. I'm not insisting to keep this if it's useless, but we need to prove that first. Obviously it was put there for some reason.
Please do not blame me for the board dependent code still in the usb_ohci.c file. I will move it later.
Who else shall we blame then ;-) ?
So please blame be. I just though about leaving the generic PCI OHCI code in the file. When it has been made fully generic, it is not CPU or even board dependant. PCI device identification constants (better classcode) could be defined in the board config header and that's it for CONFIG_USB_OHCI_PCI.
That would be great!
Regards
Markus Klotzbuecher
Hi Markus,
On Thursday 11 January 2007 11:17, Markus Klotzbücher wrote:
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
On Wednesday 10 January 2007 23:01, Markus Klotzbücher wrote:
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
I get it. So in the end we have four cases: byte swapping register access or not _and_ byte swapping data or not. Right? Doesn't sound too complicated.
What about taking some code / macros from the linux kernel (usb/host/ohci.h)? It seems that all we need is the CONFIG_USB_OHCI_BIG_ENDIAN define.
- I had to disable the 'return -1' statement for an unfinished urb in
sohci_submit_job(). Why isn't it finished?
I really don't know, I guess only you can answer this!
This answer helps me a lot. Why is the urb_finished stuff needed? I go better without :-)
Did you see the comment in usb_ohci.c:1355? It's to make sure a transaction has completed before a new one is started. I'm not insisting to keep this if it's useless, but we need to prove that first. Obviously it was put there for some reason.
The code makes sense. But it does not work for me. I will investigate some more time on this and figure it out.
So please blame be. I just though about leaving the generic PCI OHCI code in the file. When it has been made fully generic, it is not CPU or even board dependant. PCI device identification constants (better classcode) could be defined in the board config header and that's it for CONFIG_USB_OHCI_PCI.
That would be great!
Regards Matthias
In message 200701111339.01196.matthias.fuchs@esd-electronics.com you wrote:
What about taking some code / macros from the linux kernel (usb/host/ohci.h)? It seems that all we need is the CONFIG_USB_OHCI_BIG_ENDIAN define.
No, this is not sufficient. See my previous message.
Also be aware that the U-Boot USB code is much more restrictive, so watch for incompatibilities when copying from Linux sources.
Best regards,
Wolfgang Denk
On Thursday 11 January 2007 14:15, Wolfgang Denk wrote:
In message 200701111339.01196.matthias.fuchs@esd-electronics.com you
wrote:
What about taking some code / macros from the linux kernel
(usb/host/ohci.h)?
It seems that all we need is the CONFIG_USB_OHCI_BIG_ENDIAN define.
No, this is not sufficient. See my previous message.
Also be aware that the U-Boot USB code is much more restrictive, so watch for incompatibilities when copying from Linux sources.
My idea is not to copy the ohci implementation, but the access macros and the naming convention in order to get a little closer to the kernel's implementation.
Matthias
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
On Thursday 11 January 2007 11:17, Markus Klotzbücher wrote:
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
On Wednesday 10 January 2007 23:01, Markus Klotzbücher wrote:
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
The main changes concern endianess fixes. The former code only supports OHCI controller with the same endianess as the CPU. The other fix allows a offset
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
I get it. So in the end we have four cases: byte swapping register access or not _and_ byte swapping data or not. Right? Doesn't sound too complicated.
What about taking some code / macros from the linux kernel (usb/host/ohci.h)? It seems that all we need is the CONFIG_USB_OHCI_BIG_ENDIAN define.
Seems reasonable. So we'll use these for register accesses, and the rest stays as is doing CPU dependant byteswapping based on the LITTLEENDIAN define (include/usb.h). Ok?
- I had to disable the 'return -1' statement for an unfinished urb in
sohci_submit_job(). Why isn't it finished?
I really don't know, I guess only you can answer this!
This answer helps me a lot. Why is the urb_finished stuff needed? I go better without :-)
Did you see the comment in usb_ohci.c:1355? It's to make sure a transaction has completed before a new one is started. I'm not insisting to keep this if it's useless, but we need to prove that first. Obviously it was put there for some reason.
The code makes sense. But it does not work for me. I will investigate some more time on this and figure it out.
Thanks!
Regards
Markus Klotzbuecher
In message 877ivtu003.fsf@denx.de you wrote:
Seems reasonable. So we'll use these for register accesses, and the rest stays as is doing CPU dependant byteswapping based on the LITTLEENDIAN define (include/usb.h). Ok?
Please don't.
Please keep in mind that mixed configurations exist, and it would be nice to support these, too.
Best regards,
Wolfgang Denk
Hi Markus,
On Thursday 11 January 2007 15:11, Markus Klotzbücher wrote:
IIRC same problem with the 440EP and MPC5200, thats what the extra #ifdef in drivers/usb_ohci.c:103 is for.
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
I get it. So in the end we have four cases: byte swapping register access or not _and_ byte swapping data or not. Right? Doesn't sound too complicated.
What about taking some code / macros from the linux kernel (usb/host/ohci.h)? It seems that all we need is the CONFIG_USB_OHCI_BIG_ENDIAN define.
Seems reasonable. So we'll use these for register accesses, and the rest stays as is doing CPU dependant byteswapping based on the LITTLEENDIAN define (include/usb.h). Ok?
Sounds reasonable to me. We still need a more pretty way to handle the +/- CFG_PCIRAM_BASE. What about some cpu_to_bus/bus_to_cpu macros?
Matthias
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
Sounds reasonable to me. We still need a more pretty way to handle the +/- CFG_PCIRAM_BASE. What about some cpu_to_bus/bus_to_cpu macros?
Hmm, that sounds a bit confusing too if you're only accessing a on chip controller. How about just renaming the define to something more generic such as CFG_OHCI_RAM_OFFSET and adding a nice explanation in the README? That way it wouldn't seem so strongly related to pci.
Regards
Markus
Hi Markus,
On Friday 12 January 2007 12:07, Markus Klotzbücher wrote:
Hi Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com writes:
Sounds reasonable to me. We still need a more pretty way to handle the +/- CFG_PCIRAM_BASE. What about some cpu_to_bus/bus_to_cpu macros?
Hmm, that sounds a bit confusing too if you're only accessing a on chip controller. How about just renaming the define to something more generic such as CFG_OHCI_RAM_OFFSET and adding a nice explanation in the README? That way it wouldn't seem so strongly related to pci.
When dealing with a pci OHCI controller the offset if pci related. But you are right, when not running on a PCI platform your name ist better.
BTW, considering Wolfgang's desire for multi ohci controller support, the constant will change into a field in the ohci struct.
Matthias
In message 873b6hna01.fsf@denx.de you wrote:
I get it. So in the end we have four cases: byte swapping register access or not _and_ byte swapping data or not. Right? Doesn't sound too complicated.
...or all of this combined.
Did you see the comment in usb_ohci.c:1355? It's to make sure a transaction has completed before a new one is started. I'm not insisting to keep this if it's useless, but we need to prove that first. Obviously it was put there for some reason.
Check the changelogs. IIRC it *is* there for a purpose.
Best regards,
Wolfgang Denk
In message 200701110950.08710.matthias.fuchs@esd-electronics.com you wrote:
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code never swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the host controller indirectly.
Be careful! Keep in mind that you may need both cases in one system, like using a PCI OHCI controller on a MPC5200 board where you have to support USB both on the internal USB controllers and on the PCI card.
See our linuxppc_2_4_devel tree for one possible implementation.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Thursday 11 January 2007 14:13, Wolfgang Denk wrote:
These are different issues! usb_ohci.c uses readl and writel to access the controller's registers from the CPU (e.g. ohci.regs). The original code
never
swaps here. But a PCI OHCI controller on a PowerPC needs it. The mXX_swap macros are used to swap data fields in structures that are passed to the
host
controller indirectly.
Be careful! Keep in mind that you may need both cases in one system, like using a PCI OHCI controller on a MPC5200 board where you have to support USB both on the internal USB controllers and on the PCI card.
See our linuxppc_2_4_devel tree for one possible implementation.
Now it's getting ugly. But I will give it a glace.
Matthias
participants (3)
-
Markus Klotzbücher -
Matthias Fuchs -
Wolfgang Denk