Rockbox

Tasklist

FS#10267 - Sansa AMS FM Radio fix

Attached to Project: Rockbox
Opened by Michael Chicoine (mc2739) - Sunday, 31 May 2009, 23:55 GMT
Last edited by Bertrik Sikken (bertrik) - Saturday, 20 June 2009, 22:01 GMT
Task Type Patches
Category FM Tuner
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch fixes the FM radio problems that started with r21088.
This task depends upon

Closed by  Bertrik Sikken (bertrik)
Saturday, 20 June 2009, 22:01 GMT
Reason for closing:  Fixed
Additional comments about closing:  Fixed in svn r21416
Comment by Rafaël Carré (funman) - Monday, 01 June 2009, 00:15 GMT
i'm curious : have you tried this in combination with  FS#10048  ?
Comment by Michael Chicoine (mc2739) - Monday, 01 June 2009, 00:44 GMT
No, I have not had a chance to try  FS#10048  at all up to this point.
Comment by Bertrik Sikken (bertrik) - Monday, 01 June 2009, 07:50 GMT
First, I think a good thing would be to use some kind of mechanism to ensure that the delays used in the fmradio i2c driver are always about the same length. This can be done with a timer but unfortunately both timers in the as3525 are already used (TIMER1 as a general timer and TIMER2 to call the tick tasks). How about using the watchdog timer as a delay timer for delays on the order of a microsecond? This would of course mean that we lose the normal functionality of the watchdog timer, but I think we're not using it now anyway.

Second, I'd like to simplify the "generic i2c" driver. We adapted the "generic i2c" driver for the ams targets, we are currently the only users of that driver but it's not doing exactly what the OF does. In my opinion this driver can be made much simpler, for example scl_hi and scl_lo can be merged into a single scl_set function, scl_input and scl_output can be merged into a single scl_dir function, also the 6 different kind of delays can be merged into a single delay that takes an argument (e.g. number of us to wait, or number of half-bit units to wait)
Comment by Rafaël Carré (funman) - Monday, 01 June 2009, 10:35 GMT
The watchdog peripheral is already used for resetting the devices.

How about writing a usleep() function using TIMER2_VALUE register, since this timer is aways running ?

I believe that would also make FM radio work in  FS#10048  as well.
Comment by Bertrik Sikken (bertrik) - Monday, 01 June 2009, 11:48 GMT
Huh? As far as I can see TIMER2 is in periodic mode (as opposed to free running mode) so it gets reset regularly. The watchdog is not used, except just before reboot. Anyway I noticed that the watchdog runs at 1/256 of PCLK. so max. resolution is only 4 us.
Comment by Rafaël Carré (funman) - Monday, 01 June 2009, 12:12 GMT
We know the timer frequency, and we also know to which value it is reset (since there is only one call to tick_start(1000/HZ) in kernel.c), so we can detect timer wraps.

Is 4us (250kHz) too large, do we need 400kHz ?
Comment by Bertrik Sikken (bertrik) - Monday, 01 June 2009, 17:06 GMT
If your radio didn't work, can you try this patch?
A look in the disassembly and a quick calculation shows that the delay is probably already long enough already (6 instructions per loop).
The most important fix is an extra delay in i2c_start that seemed to be missing. Also it removes some things that seem to be unnecessary.
Comment by Michael Chicoine (mc2739) - Tuesday, 02 June 2009, 01:22 GMT
tested this patch with r21151 svn

Still hangs when going to the FM Radio screen. Changing the fm_delay() still seems to correct the problem.
Comment by Bertrik Sikken (bertrik) - Wednesday, 03 June 2009, 19:30 GMT
I committed the most important part of my previous patch (there seemed to be an actual bug in i2c_start). What kind of delay do you need roughly to make radio work (while boosted)?

I looked up the datasheet of the radio chip and it seems to support 400 kHz i2c. I'm a bit surprised that apparently a larger delay is needed, maybe there's another bug in the generic i2c driver somewhere. A dissassembly of the current code in SVN shows 6 cycles for the delay loop, so even when no pipeline stalls occur due to branching, the default delay of 100 loops should take at least 2.4 us at 248 MHz CPU clock, which should be more than enough.
Comment by Michael Chicoine (mc2739) - Wednesday, 03 June 2009, 21:41 GMT
With svn, I had to increase the number of loops to 400 to get it to work (399 still failed). The patch increased to 500 just to be safe.

Also, the failure is not a deadlock, as the backlight still goes off at its normal timeout and a keypress will turn it back on. The dap will turn off as normal with the power button.

I have also noticed that if I made the delay is too long, it will hang with the logo on the screen and never get to the main menu. I guess that the problem here is with the test for the FM Radio hardware on startup.

Using patches from  FS#10048 , I have noticed that the radio will sometimes work with the additional delay and sometimes not. Using this patch: http://www.rockbox.org/tracker/task/10048#comment30627 I was not able to get the radio working with any delay setting. With the previous patch, the radio worked with the 500 loop delay.
Comment by Bertrik Sikken (bertrik) - Thursday, 04 June 2009, 21:25 GMT
Another go at some generic_i2c fixes. I looked at the i2c spec and the OF code again and it seems some more delays are missing. This patches adds those delays. Can you try this patch without increasing fm_delay?

On my clip, with  FS#10048 , with fm_delay actually intentionally lowered down to 20 cycles and with CPU boosted, this still works.
Comment by Michael Chicoine (mc2739) - Friday, 05 June 2009, 00:53 GMT
tested with r21192

Still hangs before entering the FM Radio screen. Increasing fm_delay() loops allows proper operation.
Comment by Bertrik Sikken (bertrik) - Saturday, 06 June 2009, 06:47 GMT
Hm, still weird. I'm surprised such a big delay is needed while it seems to work with just 20 cycles on my clip. This makes me think there's something else going on. Ideas:
* perhaps i2c bus method selection fails in some way (the fm chip can use both i2c and some other 3-wire protocol). This "just worked" so far and has not really been looked into, but maybe it's giving trouble now at higher CPU speeds.
* perhaps there's still some bug in the generic_i2c driver. I think there's a tiny time window where a short circuit can exist on the SDA line between the fm radio and the as3525 controller: just after sending a byte to the fmradio, we expect an ACK bit back from the fmradio but the SDA line is still in output mode. However the OF seems to do it like this as well.
* perhaps the firmware version of the fmradio chip has an influence on behaviour. The si4702 comes in two firmware versions, 16 and 19. Can you check the chip ids (first two values in the fmradio debug screen)? The radio has to be enabled for this debug screen to work. My clip shows 0x1242 0x1053 indicating an si4702 with firmware version 19.
Comment by Dustin Skoracki (sko) - Saturday, 06 June 2009, 08:58 GMT
- "Can you check the chip ids (first two values in the fmradio debug screen)"

Uhm... fmradio debug screen shows only zeros on my e250v2 (see screendump)
Comment by Bertrik Sikken (bertrik) - Saturday, 06 June 2009, 09:04 GMT
Can you have a look in the fmradio debug screen, while the radio is playing?
Comment by Dustin Skoracki (sko) - Saturday, 06 June 2009, 11:27 GMT
I can't get the radio to work... even with this patch (one of them or both).
Comment by Michael Chicoine (mc2739) - Saturday, 06 June 2009, 13:25 GMT
Chip ids on my e260v2 are 0x1242 0x0850
Comment by Rafaël Carré (funman) - Saturday, 06 June 2009, 13:37 GMT
That means Chip version 4, si 4702, Firmware version 16
and bertrik's Clip is version 8, si 4702, Firmware version 19

The datasheet I have is for firmware version 19, we should get a copy of version 16 and see if there is any differences.
Comment by Bertrik Sikken (bertrik) - Saturday, 06 June 2009, 14:07 GMT
I have the datasheet for the si4700/4701 revision B, firmware version 15. The bit mapping for the REV, DEV and FIRMWARE fields is a bit different for the 4700/4701, but it does mention 0x4XX and 0x8XX as possible values (XX = firmware version). So this could be an older chip that behaves a little differently.
Comment by Jack Halpin (FlynDice) - Saturday, 06 June 2009, 23:11 GMT
I have radio working on my e280v2 with svn patched with  FS#10048 . The only changes I made were to run in fastbus only mode at 62 MHz boosted and 31MHz unboosted. I will try to test higher frequencies and find the failure point. I have the same chip ids as mc2739.
Comment by Jack Halpin (FlynDice) - Monday, 08 June 2009, 05:28 GMT
I don't know if this will help but I tried several higher FCLK frequencies and found that at FCLK 124MHz the radio is still functional and it is not functional at the next lowest FCLK I could get to boot which was 186 MHz.
Comment by Graham Hawkins (daytona955) - Monday, 08 June 2009, 08:05 GMT
For me (e260v2), the radio stops working at vanilla SVN r21088 - the otherwise excellent FlynDice clocking rationalisation patch ;)
Changing fm_delay to 500 does not help.

However changing DBOP clock to:
#define AS3525_DBOP_FREQ (AS3525_PCLK_FREQ/2)
in an otherwise vanilla r21088 gets the radio going.

This comes from experimenting, not from any knowledge of how or why DBOP clock affects the FM chip.

I have not yet tried the generic_i2c.c patch.

FTR the chip ID is 0x1242 0x0850
Comment by Graham Hawkins (daytona955) - Monday, 08 June 2009, 10:51 GMT
The same change to DBOP clock also gets the radio working on r21184+FS10048+FS10284.
The UI seems reasonably responsive with this setup - test_fps says 44.7 fps unboosted, spacerocks is playable.
Comment by Bertrik Sikken (bertrik) - Monday, 08 June 2009, 17:00 GMT
Weird...

I looked a bit at the fuze and e200v2 display driver and noticed that the fuze display driver configures GPIO A7 as output and sets it low, this pin is also used for FM radio I2C. The e200v2 display driver doesn't do this. I hope there isn't really any dual-use for this pin so this could simply be a bug, but maybe it has a side-effect of making the FM radio work. Can someone with a fuze try to remove the GPIO A7 stuff (both direction setting and level setting) from the fuze lcd driver and see if this causes the radio to stop working?

I also looked a bit closer at the e200v2 OF. It actually uses a really tiny delay, just 3 loops of 4 instructions. One thing in the e200v2 OF that I didn't find in the Clip OF is that the SDA and SCL I2C lines are both set to 1 for some time before sending an I2C start condition.
Comment by Rafaël Carré (funman) - Monday, 08 June 2009, 17:08 GMT
Radio (and LCD) still works on the Fuze, with this modification.
Comment by Bertrik Sikken (bertrik) - Wednesday, 10 June 2009, 20:16 GMT
Attached are a couple of patches to experiment with, can you apply each and see if it works? Please use a clean tree from current SVN before applying each patch.
fix1: increase si4700 crystal oscillator startup delay
fix2: wait some time after setting i2c SCL signal as output
fix3: more extensive i2c initialisation
Comment by Michael Chicoine (mc2739) - Wednesday, 10 June 2009, 23:18 GMT
Tested against r21245 clean tree

fix1: radio does not work, screen cleared except for backdrop and statusbar, not deadlocked (backlight goes off, keypress turns backlight on)
fix2: hangs at Rockbox logo, not deadlocked (backlight goes off, keypress turns backlight on)
fix3: radio does not work, screen cleared except for backdrop and statusbar, not deadlocked (backlight goes off, keypress turns backlight on)
Comment by Bertrik Sikken (bertrik) - Thursday, 11 June 2009, 07:03 GMT
Hmm, too bad. Attached is another attempt. This time a small modification in the generic i2c driver, can you try this?
Comment by Graham Hawkins (daytona955) - Thursday, 11 June 2009, 07:53 GMT
Fix4 on r21245 - radio does not work, screen cleared except for backdrop and statusbar, not deadlocked (backlight goes off, keypress turns backlight on)
I assume the patches are intended to be applied separately, not all at once to the same source tree?
Comment by Bertrik Sikken (bertrik) - Thursday, 11 June 2009, 16:06 GMT
Another experiment: the following patch works around a while-loop in the si4700 driver that may be hanging, can you try if this works better? The radio may not get tuned correctly but there's at least a chance to see the actual radio screen and it might gives us more clues.
Comment by Ralf Erbel (jago) - Thursday, 11 June 2009, 21:27 GMT
Tried the patch. Now it showed at least the normal radio screen. The handling (channel,volume e.g.) seems normal but there is no sound.
Like you suspected it was waiting in the loop for the ready signal and it never arrives.
Comment by Bertrik Sikken (bertrik) - Thursday, 11 June 2009, 22:28 GMT
Ah, now we're getting somewhere, can you get a screenshot/screendump of the FM radio debug screen after visiting the FM radio screen?
Comment by Michael Chicoine (mc2739) - Thursday, 11 June 2009, 22:56 GMT
Tested against r21253 clean tree

Same results as jago

Attached screenshot of FM radio debug screen
Comment by Michael Chicoine (mc2739) - Friday, 12 June 2009, 01:25 GMT
For comparison, I thought you might also like to have a screenshot of the FM radio debug screen from a working build. This is from r21171 with the increased fm_delay()
Comment by Bertrik Sikken (bertrik) - Friday, 12 June 2009, 06:14 GMT
Thanks, those screendumps are very useful. It looks like the i2c communication with the radio chip is actually fine. The problem seems to be that the fm radio chip hasn't fully or properly been powered up. For example the lower part of the chip id register (second number) is different, which is consistent with what the datasheet says.

Perhaps it simply needs more time than expected to power up. Can you try attached patch?
Comment by Ralf Erbel (jago) - Friday, 12 June 2009, 15:30 GMT
Unfortunately the patch has no effect. Testet against r21246.
Comment by Bertrik Sikken (bertrik) - Friday, 12 June 2009, 22:35 GMT
Can you try the combination of e200_radio_fix1.patch, e200_radio_fix5.patch and si4700_enable_delay.patch?
Comment by Michael Chicoine (mc2739) - Friday, 12 June 2009, 23:10 GMT
Tested with r21268

Radio still not working with e200_radio_fix1.patch, e200_radio_fix5.patch and si4700_enable_delay.patch
Comment by Rafaël Carré (funman) - Thursday, 18 June 2009, 18:22 GMT
the c200v2 seems to have the same problem (using si4700 as well)
Comment by Bertrik Sikken (bertrik) - Saturday, 20 June 2009, 22:01 GMT
The problem with the e200v2 radio turned out to be a problem with the initialisation sequence of the Si4702 with firmware version 16, that appears to be slightly different from Si4702 radios with firmware version 19.

Loading...