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

HW SSEL Support for SPI API #1028

Merged
merged 2 commits into from May 27, 2015
Merged

HW SSEL Support for SPI API #1028

merged 2 commits into from May 27, 2015

Conversation

sg-
Copy link
Contributor

@sg- sg- commented Apr 10, 2015

There was a lot of duplicate code and unclear use of the spi_init ssel parameter. This looked like 30 copy & pastes of the logic from the LPC1768 implementation. ssel in spi_init is now clearly (coded, still not documented) to be hw based SS rather than master slave determination. This behavior was derived from the constructor implementation since HAL documentation doesn't exist.

Update SPI API documentation for hardware driven SS. Modify all target spi_api.c implementations. spi_init ssel parameter was used incorrectly to determine master/slave mode rather than enable hardware driven SS (seems to be due to legacy copy paste). Remove duplicate copy paste code of initialization in spi_init that is done by constructor (SPI and SPISlave)

…t spi_api.c implementations. spi_init ssel parameter was used incorrectly to determine master/slave mode rather than enable hardware driven SS (seems to be due to legacy copy paste). Remove duplicate copy paste code of initialization in spi_init that is done by constructor (SPI and SPISlave)
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2015

It's not clear to me what happens for ST boards, in the following code:

    if (ssel != NC) {
        pinmap_pinout(ssel, PinMap_SPI_SSEL);
    }
    else {
         obj->nss = SPI_NSS_SOFT;
    }

Formating is wrong there for else statement :)

Referencing #935 which this should fix.

@sg-
Copy link
Contributor Author

sg- commented Apr 10, 2015

obj->nss = SPI_NSS_SOFT    // peripheral config for software based SS

which can later be used in spi_format to configure the IO as input or output

if (obj->nss != SPI_NSS_SOFT) {
    obj->nss = (slave) ? SPI_NSS_HARD_INPUT : SPI_NSS_HARD_OUTPUT;
}

https://github.com/sg-/mbed/blob/master/libraries/mbed/targets/hal/TARGET_STM/TARGET_STM32F0/spi_api.c#L162

@sg-
Copy link
Contributor Author

sg- commented Apr 10, 2015

Forgot to mention that nRF51822 spi_api.c is an exception at the moment. There is a lot of IO config that I didn't take the time to understand and refuse to implement with a variable as state flag.

@Sissors
Copy link
Contributor

Sissors commented Apr 11, 2015

Good to support this, however then you will also need to give the write options another argument: If it is the last or not, so if the CS pin should be raised afterwards or not. And I would say it might be best to add common code where the target specific code returns if CS is hardware or not, and if not that the common code switches to GPIO for CS.

Otherwise making a library is going to be a royal pain in the ass: Users need to supply the 4 pins, but how do you know if the CS pin is supposed to be a hardware pin?

@sg-
Copy link
Contributor Author

sg- commented Apr 11, 2015

I'm pretty certain in most peripherals that sport hardware CS/SS this is done automagically when the data register is written. I could point to datasheets where this IS the case but would rather be pointed to where this isn't the case.

That said, there may be a case where SS in master mode changes state (low/high) between writes due to calling overhead or lack of using a FIFO but if that's the case it'd be nice to add a multi-byte write member to the API.

Software CS/SS is still possible too

@Sissors
Copy link
Contributor

Sissors commented Apr 11, 2015

But the problem is that they do it automagically, depending on how you write the settings :). When HW SS is used it will always make the pin low, but it will make the pin high again afterwards, unless it is specifcally set not to do that. A quick check does not even show me an option for the LPC1768 not to make it high directly afterwards.

For the K64F however for example:
Continuous Peripheral Chip Select Enable
Selects a continuous selection format. The bit is used in SPI Master mode. The bit enables the selected
PCS signals to remain asserted between transfers.
0 Return PCSn signals to their inactive state between transfers.
1 Keep PCSn signals asserted between transfers.

You need to have some way of telling if it should keep the CS line asserted or not. And yeah software CS will still be possible, but how does a library now if the supplies CS pin can be used for hardware CS?

@sg-
Copy link
Contributor Author

sg- commented Apr 11, 2015

In case of 1768 looks like modes where CPHA = 1 the ssel is low during back to back transmissions. 18.5.2.3 (5) in user manual.

For other targets such as FSL as noted there maybe another few bits needed in the format methods.

From perspective of a library the library would determine this. In worst case an assert happens if a non ssel pin is passed and hw based cs is required. Alternatively it super libraries are written maybe creating an accessable pin_map function to determine compatibility??

I think lots of different ways to do this.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 13, 2015

We should create a new issue for this topic, where we can continue discussion.

This PR cleans the spi init and how the last parameter is used. @sg- what we can do with nrf51 ?

@sg-
Copy link
Contributor Author

sg- commented Apr 14, 2015

Need to spend more time and take a closer look at how the pins are used in the MUX and the peripheral in closer detail (it'll be a while)

@0xc0170
Copy link
Contributor

0xc0170 commented May 12, 2015

To be able to merge this

  • resolve SPI.cpp file conflict (2 lines only)
  • Silabs HAL spi modification is missing - in spi_init to see how master/slave changes enable pins function
  • Nordic HAL spi modifications missing.

Those 2 missing modifications looks similar, that enabling pins, which might require additional parameter to spi_init. Something we should look closer.

Edit: As spi hal init is currently used this change should not break any program, but we should resolve the above. We should also decide on how to use this hw cs, as we got now transfer method.
Plus some platforms might not even build, [Error] SPI.cpp@35: identifier "ssel" is undefined for DISCO_L053C8.

@0xc0170 0xc0170 merged commit c74b9d6 into ARMmbed:master May 27, 2015
@0xc0170
Copy link
Contributor

0xc0170 commented May 27, 2015

Rebased and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants