FS#10669 - AMS Sansa Reimplement Voltage Scaling

Attached to Project: Rockbox
Opened by Jack Halpin (FlynDice) - Saturday, 10 October 2009, 03:12 GMT
Last edited by Jack Halpin (FlynDice) - Thursday, 15 October 2009, 19:58 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch reimplements voltage scaling for the AMS Sansas. I have set the reduced voltage level at 1.05 V in the hope that the lower voltage will be more likely to expose any bugs that may show up. When actually implemented I would expect to use 1.10 as the reduced voltage. kugel and mc2739 came up with this solution several month ago I have simply massaged the implementation here a bit.

This patch uses cpu_boost() during sd card accesses when a microsd is present to raise the core voltage back to 1.20V. This means that for the clip, and also for the other targets when a microsd card is not present, it does not use cpu_boost() on card access and remains at the lower voltage.

I have run this at 1.05 on my e280v2 for several hours now both with and without a microsd with no problems.
This task depends upon

Closed by  Jack Halpin (FlynDice)
Thursday, 15 October 2009, 19:58 GMT
Reason for closing:  Accepted
Additional comments about closing:  r23193
Comment by Thomas Lloyd (thomaslloyd) - Saturday, 10 October 2009, 08:08 GMT
This breaks my microsd card access. The player when initially switched on will not read the card. This is when i access it through files form the main menu. If i pop the card out and in again it reads the contents and i can get access to play music from the card. After a while it crashes out with error.

Undefined instruction
at: 30800CF8

Side note, the the previous patch at 1.10v worked okay. I have an 8 GB class 2 Sandisk SDHC on an e260V2.
Comment by Rafaël Carré (funman) - Saturday, 10 October 2009, 09:53 GMT
Works fine on the Clip

The Fuze deadlocks when I remove the µSDHC (Transcend 4GB Class 6)
Comment by Michael Chicoine (mc2739) - Saturday, 10 October 2009, 18:20 GMT
Works fine on my e260v2. My card is a Sandisk 4GB class 2 SDHC.
Comment by Thomas Lloyd (thomaslloyd) - Sunday, 11 October 2009, 23:52 GMT
The error i am getting is directly related to the card. As we would expect. This does not seem related to cpu boost and i have inspected the buffer thread information. I have tried inserting a card and then just listening to the FM radio and the player will still crash with the same error.

I do not know how long before you drop the voltage back to the 1.05 after card insertion, maybe this coincides with the error. I have tried this with another 8 GB Sandisk Class 2 SDHC card this one with different disk info in the debug menu and very much the same results.

Comment by Thomas Lloyd (thomaslloyd) - Monday, 12 October 2009, 00:19 GMT
Modified my voltage back up to 1.10 and it is running okay now. I have been playing around with inserting and removing the card. Sometimes it is missing that i have taken the card out or put it in. This causes it to think there is an SD card inserted when i take it out and removes it when i am putting it in. I am not sure how you are detecting that the card has been ejected or put in but that currently can misread what is happening.
Comment by Jack Halpin (FlynDice) - Monday, 12 October 2009, 01:28 GMT
Since the card detection seems to be creating a problem I don't think it's worth the effort to try to sort it out. This patch simplifies things and uses cpu_boost() on anything that has a microsd slot(all except clip...)

The voltage is still at 1.05 but is easily changed to 1.10. If 1.05 gives you problems try changing line 370 in system-as3525 from CVDD_1_05 to CVDD_1_10.
Comment by Michael Chicoine (mc2739) - Monday, 12 October 2009, 02:28 GMT
This new patch still works fine for me.

Edit: I have noticed some blue pixels that I didn't see before applying this patch. They show up consistently on the Rockbox logo displayed by the main firmware (not bootloader). They are about half way down the display in one line. This is with r23123. I do not see them with a clean build.

Edit 2: I have reapplied the first patch with r23123 and do not see the blue pixels.
Comment by Jack Halpin (FlynDice) - Monday, 12 October 2009, 04:26 GMT
I can't duplicate the blue pixels here. I've got r23123 with this patch applied and I've tried rebooting a whole bunch of times with no luck.
I am puzzled why the first patch would not exhibit the same problem as it is basically the same process with a check to see if there is a card present.
And no, I am not doubting your results as you are the ultimate tester!

Does raising the voltage to 1.10 have any effect?
Comment by Michael Chicoine (mc2739) - Monday, 12 October 2009, 06:01 GMT
No blue pixels with the voltage at 1.10V
Comment by Jack Halpin (FlynDice) - Wednesday, 14 October 2009, 02:40 GMT
Ok, I think this one should work on all players with no problems. I have not dropped the voltage on this one so it stays at 1.10.
It does check for the presence of a µSD card to boost, a much better way than the first patch, so if you don't have a µSD card installed, it should not boost.
I think this one is ready to commit if we don't see any problems.
Comment by Michael Chicoine (mc2739) - Wednesday, 14 October 2009, 03:41 GMT
Latest patch is working fine here.

No problems reading, writing, deleting, inserting, removing or playback on µSD card. No display anomalies.
Comment by Rafaël Carré (funman) - Wednesday, 14 October 2009, 12:41 GMT
Seems to work fine on Fuze (after modifying config-fuze.h)

I saw a freeze when removing the µSD while buffering, however I didn't test if the same freeze happen without this patch.

Removing µSD when idle or when audio has been buffered shows no problem

EDIT: I see you unboost unconditionally and this might be a problem since the counter could become negative
Comment by Jack Halpin (FlynDice) - Wednesday, 14 October 2009, 16:00 GMT
That figures, I actually unboosted unconditionally to avoid the problem of someone removing the µSD during the disk access and ending up stuck at boosted... But it seems you got a different problem.
The way i read cpu_boost(bool on_off) in system.c there is a safety check to reset the boost counter to 0 if it goes negative.

I will modify the config files and post another patch shortly.
Comment by Rafaël Carré (funman) - Wednesday, 14 October 2009, 16:03 GMT
Ah right I see the safety check, but you could accidentally cancel boosting when another thread needs it?

It should be possible to check for boosting state during SD removal with not much trouble
Comment by Jack Halpin (FlynDice) - Wednesday, 14 October 2009, 17:51 GMT
Ok, This one also unboosts if there is a µSD present.

I've edited the config files for e200v2, fuze, clip, and c200v2 to activate voltage scaling.

I tested removing the card while buffering and got lockups with both current svn and this patch so I am pretty sure this patch does not impact that issue.
Comment by Rafaël Carré (funman) - Wednesday, 14 October 2009, 19:16 GMT
you just forgot the m200v4 ;)

If all as3525 models use this, then I think this should be hardcoded (EDIT: I mean remove the ADJUST_CPU_VOLTAGE define)

The patch doesn't deal with µSD removal when an operation is in progress, but since this locks up the Sansa anyway I think you should just commit when you feel it's alright.
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 04:14 GMT
- Remove HAVE_ADJUSTABLE_CPU_VOLTAGE define and enable for all Sansa AMS
- Make CPU boosting depend on HAVE_MULTIDRIVE (unneeded for internal storage)
- Keep track of boosting status inside sd_enable() so we don't need to check for card removal

EDIT: tested on Fuze & Clip , battery_bench running on both
Comment by Jack Halpin (FlynDice) - Thursday, 15 October 2009, 04:39 GMT
I don't think you need the #ifdef HAVE_MULTIDRIVE unless the intent is to leave out that code on the clip. card_detect_target() should always return false for the clip.
cpu_boosted seems to take care of the card removal problem for us just fine....
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 05:05 GMT
I think it makes the code clearer to read.

But perhaps it should be conditional on HOTSWAP.

By the way the various
#if defined(SANSA_E200V2) || defined(SANSA_FUZE) || defined(SANSA_C200V2)
should be removed since they always are enclosed on MULTIDRIVE or HOTSWAP.

And I think most if not all the MULTIDRIVE should be changed to HOTSWAP (which means hotswappable µSD cards)
Because MULTIDRIVE could be true when using a ramdisk for example
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 07:19 GMT

My comments were addressed in r23180
Comment by Jack Halpin (FlynDice) - Thursday, 15 October 2009, 18:40 GMT
This runs fine and looks good to me(after I found amiconn's comments on irc). Do you want to wait on the battery benches before committing?
Comment by Rafaël Carré (funman) - Thursday, 15 October 2009, 19:30 GMT
The fuze bench just finished : 12h30 (I don't remember what SVN gives).
The clip freezed at playback though

No need to wait, please commit