Hi Alexander,

welcome to the mailing list!

After reading your email, my first intention would be to check the GPIO configuration of the Pandaboard. From page 40 of the Pandaboard manual [1], I gather that two GPIO pins of the OMAP chip are directly connected to the USB-OTG connector, presumably for the power supply. In your code, I do not find the initialization of the corresponding GPIO pins 179 and 180. I expected this code to be present in 'dde_linux/src/drivers/usb/arm/platform/platform.cc'.

As another note, I have the impression that OTG is actutally unrelated to the existing USB device driver. It is different device. Is this correct? If yes, wouldn't it be sensible to put the OTG driver into a separate driver component instead of co-locating it with the existing USB driver? This would avoid the complexities of the existing USB code when looking at the OTG issue.

[1] Pandaboard manual

http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/pa...

Cheers Norman

On 10/05/2012 01:15 PM, Alexander Tarasikov wrote:

Hi, Genode crowd!

I've been porting the MUSB OTG driver for the OMAP4 platform to the Genode Framework.

I have implemented the following:

• some linux routines for platform devices (to manage resources)
• added code for some stubs in dde kit (strings, mutexes, spin locks)
• the I2C driver class for Genode and ported the OMAP4 I2C driver
• the TWL controller routines to read/write registers
• misc omap4 phy init (clocks and other registers)

This has allowed me to compile and run the musb driver and the gadget drivers (zero, ethernet) from the linux kernel. However, I've been desperately trying to figure out why it is not working.

I have enabled all the levels of debugging in the usb drivers and have dumped the MUSB FIFO in both linux and genode. The good thing is that the data is not all zeroes or all ones, but correct usb requests starting with 0x80. I think this indicates memory IO is working.

The bad thing is that under linux I get the correct usb requests (80 06, that is, GET_DESCRIPTOR). Under genode I either get SET_DESCRIPTOR or SET_ADDRESS all the time and the host (desktop) says "device descriptor read/64, error -32".

Does anyone have any idea what may have gone wrong? IO and register access seems to work so I'm lost here.

I have pushed the code to my github if you may want to try it out yourself git://github.com/astarasikov/genode.git The branch is omap4-otg-dirty. I have also pushed the 'contrib' directory because I was lazy to prepare the patches and fight Makefiles until the driver is working. My run script is called "panda_otg.run" (in my personal directory, astarasikov/run).

I have also made the kernel message dumps in linux and genode so that it's possible to compare how usb behaves. I have also disabled L2 cache in uboot since it caused a lot of trouble with framebuffer previously.

http://pastebin.ca/2239708 http://pastebin.ca/2239709

Note that I have the commented out the setting up interrupts part for the omap4 twl. Actually, zero is returned as the default irq number which is timer. I have tested with both the correct irq number and the zero interrupt, it does not affect anything - in both cases the device does detect when the cable is plugged, but fails to set up the connection.

Hi Alexander,

Sorry, I have not studied the whole source code yet, but I have the impression that after mapping memory via attaching mmio dataspace via rm reading registers by the pointer and via Register c++ template class gives different results. I had the trouble - my I2C driver from uboot didn't work when I just mapped the memory, but Ivan Loskutov has reworked it to use the Register class and it worked from now on.

maybe the trouble was related to the bug report we discussed yesterday? See:

http://sourceforge.net/mailarchive/forum.php?thread_name=506D7D02.2040904%40...

(you will have to click on "Attachments: Message as HTML" of the first message)

Could you tell if I'm digging in the correct direction? I think both IO memory and buffers must be mapped uncached, unbuffered via arm mmu, but, again, I've not yet read all the code of genode and fiasco.

You are definitely on the right track!

If you're interested, here is what I suggest (and want to do later), but I'm planning to write a more detailed blog entry later and keep you updated.

• replacing most panda stuff with omap4 name

(i.e., move headers from include/board/panda to include/cpu/omap4)

That's a good idea. Maybe we could even think about introducing another indirection 'soc'. ARMv7 is a 'cpu', OMAP4 is a 'soc', Pandaboard is a 'board'.

• Implementing abstractions for various device classes (like, gpio,

voltage regulator, led in linux, but in C++)

I agree. The corresponding session interfaces implemented by Ivan are a good start. So far, we haven't included them in our upstream tree because there was no pull request or topic in our issue tracker to discuss it. If you are interested, I would appreciate you opening a new topic for each device class. So we can take the discussion over there.

• splitting dde kit's common stuff (lx_emul, platform) and usb driver.

Ideally, it should be easy to add new drivers to the makefile and build each linux driver as a standalone binary

That is completely in line with our intentions. We planned to split those parts into reusable pieces as soon as we reach a certain diversity of drivers within dde_linux. Maybe, we are just reaching this point now? ;-)

Cheers Norman

Hi Alexander,

The first thing I did was to disable caching and buffering in L4.. And it was a fiasco. The good news is that reading IO memory via the DS attached pointer and via the mmio Register class seems to bring identical results.

it would be interesting to know if your current working version works with caches enabled. Just to see if the issue is related with caches.

Turned out, the wrong data came from musb_read_fifo function. This function calls __raw_readl, __raw_readb, __raw_readw functions. I think I have implemented them correctly given that unaligned access works fine in genode. I would appreciate if someone takes a look at them.

https://github.com/astarasikov/genode/blob/omap4-otg-dirty/dde_linux/src/dri...

Your implementation looks ok. It is just a copy of the corresponding 'b' and 'l' functions with the type changed, isn't it?

Turns out, there's a single FIFO register and when you read from it or write to it, the controller immediately feeds it with the new data. So, maybe multiple unaligned access caused it to fail.

What I did was to comment out musb_{write,read}_fifo code and manually read the data. First I decided to read by 4-byte words, but that failed. To my surprise, reading by single bytes did the trick and the driver is now receiving the correct data and the host recognizes the device, albeit at the third attempt.

The 'musb_write_fifo' function is known to work on Linux. I am afraid that replacing this known-good code with a custom implementation will hide the symptom at best. I have a slight suspicion. Let's take another look at 'musb_write_fifo' again

http://lxr.linux.no/#linux+v3.6.1/drivers/usb/musb/musb_core.c#L232

The function uses different write-access functions for different alignments and length constrains. One particularly interesting case is the 32-aligned case for a len >= 4. Here, the 'writesl' function is used, which is just a wrapper around 'outsl'. Let's take a closer look to 'outsl':

http://lxr.linux.no/#linux+v3.6.1/include/asm-generic/io.h#L209

The function uses a loop to repeatedly call 'outl'. On x86, 'outl' performs an I/O port access. DDE Linux maps this function to the coresponding 'dde_kit_outl' implementation, which uses core's IO_PORT service to perform the access. On ARM, however, there are no I/O ports. Hence, the the out* function of the IO_PORT service in core are no-ops. You could validate the hypothesis by adding a bit of log output to the dummy functions at:

base/src/core/arm/io_port_session_component.c

If any of these functions is called on ARM, this is an error. If this is the case, the solution would be to implement the 'outl' function in your version of 'lx_emul.h' with an I/O memory access.

I must admit that I'm quite surprised that the 'outl' function is called on ARM at all. Of course, I recommend to check the other 'out*' functions as well.

And, btw, I've found a small bug in dde_linux. In the file dde_linux/src/drivers/usb/arm/platform/lx_emul.h the definition SZ_4K = 0x00001000a, seems clearly wrong to me. That 'a' at the end is probably a bad typo. When I figure out the musb issues, I'll submit a pull request for this if no one else does that before.

You are right. That is a typo. Thank you very much!

Cheers Norman

Hello Ivan,

On Thu, Oct 11, 2012 at 03:48:18PM +0400, Ivan Loskutov wrote:

We are planning to send pull-request with our commits in the near future. How to it make correct. I tried to make pull-request on Github, but I can't select required commits. How do you do it? Or can I open an issue with commits hash only?

Please have a look at

http://genode.org/community/contributions#Incorporating_your_work_into_the_m...

It describes our current daily work flow to incorporate contributions. The approach may induce a bit more work on your side as you have to rebase your work ontop of origin/master, but otherwise we had to do merges on our side with your new code we do not know as good as you do.

If you have further questions, do not hesitate to ask them here.

Greets

Hi Ivan,

great that you are preparing your work for the inclusion into our main development tree!

Once you created a branch based on current genodelabs/master and cherry-picked your changes on top of that branch, it's all set. No additional steps are needed. Just open an issue and mention the corresponding commit IDs or a link to the branch. Personally, I prefer opening plain issues over the pull request feature of GitHub.

It turned out to be a good practice to reference the issue number from the commit message. For example, by stating "Refers to issue #1234" (including the hash symbol), the commit ID will automatically appear in the discussion of issue #1234. If the commit message contains the magic words "Fixes #1234", Github will automatically close the issue as soon as the commit enters one of the genodelabs/staging or genodelabs/master branches. That's pretty convenient.

Cheers Norman

Hi Alexander,

excellent that you got the driver working. Congratulations! :-)

For now, I'll finish porting ethernet or rndis usb gadget and prepare the patches for the dde kit. Currently I would like to make usb otg a separate service inside the usb_drv (just as nic and input) for simplicity, but I would like to hear your opinion on how we should rework the makefile and dde_linux to make it possible to build drivers into separate binaries.

Merging the new driver into the existing usb_drv is a pragmatic starting point to reach a working state - just as you did. However, before incorporating these changes into mainline, I would like to see the otg driver to become a separate component that is independent from the usb driver. It is easy to add to existing code but it is hard to cut back code from the emulation environment once added. By adding the otg driver to the usb_drv, we would have a hard time to split those two drivers into two pieces (while aiming at having a minimal-complexity 'lx_emul.h' for each of them) at a later time.

I would suggest to create a new target at 'dde_linux/src/drivers/otg/target.mk' by taking the 'usb/target.mk' as blue print and removing all the parts that are not needed for otg. Based on your current version of 'lx_emul.h' (that contains the emulation environment for the combined usb+otg driver), the next step would be to minimize the emulation environment. When having reached a state where nothing can be removed, we can look for common parts between 'drivers/otg/lx_emul.h' and 'drivers/usb/lx_emul.h'. For each reasonably large block of common code, I would create a file 'drivers/lx_emul_<module>.h', which can then be included by both 'usb/lx_emul.h' and 'otg/lx_emul.h'. For example, the common (but only the common) types will go to 'lx_emul_types.h'. Maybe, we could use the name of the corresponding linux kernel headers as the <module> name?

As we proceed, we will get a nice library of 'lx_emul_*.h' snippets that can be easily reused to construct driver-specific emulation environments.

The 'target.mk' files will certainly have duplicated parts. Those parts should go to a common 'target.inc' file.

Also, in future it would be nice to add more granularity to the platform code in dde_kit as well as in other genode services because eventually we'll have to think of how to keep multiple boards with the same SoC in one tree.

As of now, 'dde_kit' is pretty platform agnostic. Where do you see the need for platform-specific additions to 'dde_kit'?

One thing we've found is that the at the header preprocessing stage genode makefile fails on the "musb_regs.file" at the following line: #include "tusb6010.h" /* Needed "only" for TUSB_EP0_CONF */ It is confused by the quotes there. If we remove them, it works fine. If we do not, a strace shows that an emtpy line appears somewhere, as if the opening quotation mark terminates a string somewhere in the preprocessing stage.

Thanks for reporting. This is a problem with the regular expression used in 'drivers/usb/target.mk' at line 56:

https://github.com/genodelabs/genode/blob/master/dde_linux/src/drivers/usb/t...

Does it work when changing the expression to the following (replacing '.*' by '[^>"]')?

"s/^#include *[<"]([^>"]*)[>"].*/\1/"

Btw, I've found some time to write down my thoughts about what I want to see in genode one day, but it had boiled down to what's being discussed here lately - power management and graceful service restart on errors (http://t.co/14KBWz7X)

I enjoyed your article and also your other rants. ;-) Thanks for the nice words about Genode! I think that your observations about the current shortcomings are spot-on. Each of the points would deserve attention. I suggest to discuss each of them separately. The question is how to prioritize those points among the many other current construction sites of the project?

When reading the part about languages, I got the impression that you would really appreciate the D programming language. Have you had a look into it?

Cheers Norman

Hi Alexander,

On 10/23/2012 12:16 PM, Alexander Tarasikov wrote:

Another interesting thing I've noticed is that if we disable LX_VERBOSE_DEBUG, usb fails to work at all due to timeouts. Turns out, some functions that are called around endpoint setup at gadget registration, cause usb to break if we disable debugging. The weird thing is that if we insert an mdelay or a printk, it still does not work, but if the dde_kit_printf is used with color escape sequences, that makes it work.

This may have something to do with https://github.com/genodelabs/genode/issues/332 which is still not completely fixed, there is a race between the interrupt thread creation in dde-kit and the driver. Can you try if the attached patch changes anything?

Greeting,

Sebastian

Hi Alexander,

For now, I'll keep working on our other projects (that includes rebasing and upstreaming our drivers and writing more omap4 support).

that sounds excellent!

One interesting question is how to handle gpio mux for various SoC. I mean, alternate pin functions. Ideally, we should be able to set up all the hardware from Genode, and most SoCs have their GPIO pins multiplexed to multiple functions. Probably the easiest thing is to use strings for pin names, or just integers? Like, "Gpio.set_mux(unsigned pin, string &name)" vs "Gpio.set_mux(unsigned pin, unsigned af)"

I am not sure that I fully comprehend the issue. The gpio driver just takes physical pins as arguments. But the meaning of the pins depends on the platform. So you'd like to use names instead of physical pin numbers in order to map the meaning of pins to physical pins? Just for the sake of an example, on one platform the 'hdmi_detect' signal is connected to gpio 17, and on another platform, the signal is connected to gpio 43. You'd like the HDMI driver to just specify 'hdmi_detect' as argument to the gpio driver instead of 17 or respectively 43?

I think, the mapping of names to physical resources (be it pins, physical addresses, or irqs) should have the form of enum values (or constants) in platform-specific header files (such as those files located at 'os/include/platform'. For example, the 'pl011_defs.h' file contains physical properties of the pl011 UART. There exists a file for the pbxa9 platform but also for vea9x4 platform. Depending on the platform of the build directory, either the 'include/platform/pbxa9' or 'include/platform/vea9x4' gets added to the include-search path. So the pl011 driver can always refer to the named enum values.

When going back to the HDMI driver example, there would be a 'include/platform/<platform-name>/hdmi_defs.h' file for each platform that has HDMI. On one platform this file may contain the line

enum { HDMI_DETECT_GPIO = 17 };

and for the other platform, the file would contain the line

enum { HDMI_DETECT_GPIO = 43 };

The HDMI driver would just include 'hdmi_defs.h' and will call the gpio driver with the named enum value 'HDMI_DETECT_GPIO' as argument.

I sincerely hope that I haven't missed the point of your question. I so, please get me on track. ;-)

Cheers Norman