Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jshFlashRead - restart read command also if someone pulled CS pin high #1943

Merged
merged 3 commits into from Oct 12, 2020

Conversation

@fanoush
Copy link
Contributor

@fanoush fanoush commented Oct 11, 2020

This makes the code a bit more robust - if CS is already high for some reason it should for sure start another read command.

Also it helps another code to work on device with shared SPI pins. With this I can have SPI display driver working on P8 and other DaFit watches like Pinetime and still have storage in SPI flash. In the LCD driver I pull flash CS high before puling LCD CS low and then keep flash CS high when exiting from driver back into javascript. This patch allows espruino to restart and continue executing code from SPI flash.

My driver is in InlineC so setting spiFlashLastAddress to 0 instead would be very tricky but even if it was fully native in libs/graphics it would still be not so nice to access jshardware.c internal variable from other unrelated code.

Maybe with this patch similar code in jshFlash write and erase methods could also just leave CS pin high so the test and setting this variable to zero could be removed from all places.

EDIT:
If you think using just CS pin is way to go I can update the patch to remove spiFlashLastAddress = 0; in jshFlashErase and jshFlashWrite and also the extra test for zero here. then it will really just test the CS pin to know that CS pin is high :-)

helps when other code pulls CS high (e.g. display driver sharing same SPI pins)
@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Oct 12, 2020

I really don't think this is a good idea - perhaps for those other watches we could instead have an #ifndef DISABLE_SPIFLASH_CS_OPTIMISE or something and remove the spiFlashLastAddress?

Or we could just make the check itself behind an #ifdef?

Or.. it sounds like if you're adding SPIFLASH storage to the watch you're building your own firmware, so you could just build your Inline C code directly into the firmware and poke spiFlashLastAddress?

The problem is, this code ends up getting absolutely thrashed when executing JS from Flash - it's called every 16(or 32? I forget) characters of JS executed. This patch adds a read from the IO bus (which I think only runs at 16Mhz?), not to mention the shift and so on.

I'd have thought it would slow down JS execution when on other devices there's no need as the SPI bus isn't shared. I feel like there's really no point 'adding robustness' in those cases when actually it's perfectly robust anyway.

@fanoush
Copy link
Contributor Author

@fanoush fanoush commented Oct 12, 2020

it sounds like if you're adding SPIFLASH storage to the watch you're building your own firmware, so you could just build your Inline C code directly into the firmware and poke spiFlashLastAddress?

Yes I addressed that in first comment 'even if it was fully native in libs/graphics it would still be not so nice to access jshardware.c internal variable from other unrelated code.' As you say I am building custom firmware so I can do it in any way.

I was not expecting this would be an issue since in best case this calls spiFlashRead
which does similar access 1+24*length times (3*8*len+setting mosi) so we are talking about 1/(24*(16 or 32)) slowdown of I/o bus operations (in addition to all other code it does + subroutine call).

BTW, this is reading OUT register (not IN) so do you think it really goes down to 16Mhz to read back value previously written there by CPU? Are all GPIO registers slower too i.e. all the CNF ones? I would guess 16Mhz relates only how the changes propagate between real wires and those registers so e.g. reading IN at 64Mhz would give you same value 4 times due to slower input but I wouldn't guess it would slow down the actual ARM read from 0x50000000+0x504. But I see I am maybe wrong on this. So any access to 0x50000000 area waits for some 16MHz bus(?)

So then you may possibly also remove clearing MOSI pin in spiFlashRead to save one such cycle as the flash ignores data on read anyway(?).

Configuring out this optimization would make no sense even for my case with shared display as this would be performance hit all the time.

So I guess if you really see it as performance issue then hiding this behind something like #ifdef SPIFLASH_SHARED_SPI is still worth the effort since this could be moved to board files instead of local modification. Apart from DaFit watches this is also issue on another watch - ID107HR. So there are at least two persons that would possibly build with that flag. So I'll add the ifdef around it.

fanoush added 2 commits Oct 12, 2020
… hit if not needed
@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Oct 12, 2020

Thanks! I was pretty sure IO accesses to the GPIO were slower - at least that was always the case on STM32, and I'd imagined nRF52 would be similar.

However I stand corrected:

var c = E.compiledC(`
// int testa()
// int testb()
typedef volatile unsigned int uint32_t;
int testa(){
  uint32_t tStart = *((uint32_t*)0xE000E018); // systick
  volatile uint32_t z = 0;
  for (int i=0;i<100000;i++)
    z+=*((uint32_t*)0x20000000);
  uint32_t tEnd = *((uint32_t*)0xE000E018); // systick
  return (z&1)+(tStart-tEnd)&0xFFFFFF;
}

int testb(){
  uint32_t tStart = *((uint32_t*)0xE000E018); // systick
  volatile uint32_t z = 0;
  for (int i=0;i<100000;i++)
    z+=*((uint32_t*)0x50000504);
  uint32_t tEnd = *((uint32_t*)0xE000E018); // systick
  return (z&1)+(tStart-tEnd)&0xFFFFFF;
}

`);

print(c.testa()) // 1240839 RAM (flash is similar)
print(c.testb()) // 1057747 GPIO

GPIO is actually faster!

However I still think the original code will be faster because spiFlashLastAddress is already in a register. I know the hit is pretty minor and it seems petty, but I feel like if I'm integrating stuff that is done to make life easier for a few folks using Espruino on their own devices, it really shouldn't negatively impact users of official devices at all.

So then you may possibly also remove clearing MOSI pin in spiFlashRead to save one such cycle as the flash ignores data on read anyway(?)

I'd be up for that - I know it's minor, but everything helps.

@gfwilliams gfwilliams merged commit fff2046 into espruino:master Oct 12, 2020
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.