| Message ID | 1408634706-5762-3-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Aug 21, 2014 at 06:25:06PM +0300, Grygorii Strashko wrote: > +- ti,davinci-spi-wdelay : delay between transmissions. I don't understand why this is here - there is already standard support in the SPI core for client drivers specifying inter-transfer delays. If there is a need to provide platform configuration for this in addition to that then it should also be a standard property, there is nothing device specific about these. > +- ti,davinci-spi-odd-parity : odd partity enabled > + OR > + ti,davinci-spi-even-parity : even parity enabled What do these mean? > +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) The bindings should be independent of the kernel, the values need to be included here (and the defines moved to include/dt-bindings so they can be used when writing DTs). > +- ti,davinci-spi-disable-timer: disable CS timer (SPIFMTn) > +- ti,davinci-spi-c2t-delay: c2t delay > +- ti,davinci-spi-t2c-delay: t2c delay > +- ti,davinci-spi-t2e-delay: t2e delay > +- ti,davinci-spi-c2e-delay: c2e delay What are all these timers/delays - at least some reference to the datasheet please?
Hi Mark, On 08/21/2014 09:20 PM, Mark Brown wrote: > On Thu, Aug 21, 2014 at 06:25:06PM +0300, Grygorii Strashko wrote: > >> +- ti,davinci-spi-wdelay : delay between transmissions. > > I don't understand why this is here - there is already standard support > in the SPI core for client drivers specifying inter-transfer delays. If > there is a need to provide platform configuration for this in addition > to that then it should also be a standard property, there is nothing > device specific about these. Sorry, may be I've missed smth, because I was not able to find such common property in SPI bindings document and code. Could you point me on, pls? In our case it's hardware delay applied between transmissions: "Delay in between transmissions. Idle time that will be applied at the end of the current transmission if the bit WDEL is set in the current buffer. The delay to be applied is equal to: WDELAY × PSPI module clock + 2 × PSPI module clock PSPI module clock -> Period of SPI module clock" > >> +- ti,davinci-spi-odd-parity : odd partity enabled >> + OR >> + ti,davinci-spi-even-parity : even parity enabled > > What do these mean? Supported by OMAP-L138/da830: " SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. 0 An even parity flag is added at the end of the transmit data stream. 1 An odd parity flag is added at the end of the transmit data stream. SPIFMTn[22].PARENA - Parity enable. 0 - No parity generation/ verification is performed. 1 - A parity is transmitted at the end of each transmit data stream. At the end of a transfer the parity generator compares the received parity bit with the locally calculated parity flag. If the parity bits do not match the PARERR flag is set in the corresponding control field. The parity type (even or odd) can be selected via the PARPOL bit. " > >> +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) > > The bindings should be independent of the kernel, the values need to be > included here (and the defines moved to include/dt-bindings so they can > be used when writing DTs). Allowed values here are: #define SPI_IO_TYPE_INTR 0 #define SPI_IO_TYPE_POLL 1 #define SPI_IO_TYPE_DMA 2 I'll update. > >> +- ti,davinci-spi-disable-timer: disable CS timer (SPIFMTn) >> +- ti,davinci-spi-c2t-delay: c2t delay >> +- ti,davinci-spi-t2c-delay: t2c delay >> +- ti,davinci-spi-t2e-delay: t2e delay >> +- ti,davinci-spi-c2e-delay: c2e delay > > What are all these timers/delays - at least some reference to the > datasheet please? > Sorry, I'll update bindings with links on datasheet. Keystone 2: http://www.ti.com/lit/ug/sprugp2a/sprugp2a.pdf Davinci: dm644x - http://www.ti.com/lit/ug/sprue32a/sprue32a.pdf OMAP-L138/da830 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf Regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 22, 2014 at 04:33:09PM +0300, Grygorii Strashko wrote: > On 08/21/2014 09:20 PM, Mark Brown wrote: > > On Thu, Aug 21, 2014 at 06:25:06PM +0300, Grygorii Strashko wrote: > >> +- ti,davinci-spi-wdelay : delay between transmissions. > > I don't understand why this is here - there is already standard support > > in the SPI core for client drivers specifying inter-transfer delays. If > > there is a need to provide platform configuration for this in addition > > to that then it should also be a standard property, there is nothing > > device specific about these. > Sorry, may be I've missed smth, because I was not able to find such common > property in SPI bindings document and code. Could you point me on, pls? It's not a property, it's what the delay_usecs in the transfer does. There is no obvious reason to apply something like this unconditionally on every transfer in DT - either it's needed only at particular points in the transfer for device reasons (in which case the device driver needs to know about it) or we need some sort of association with what the device is doing since the way the client driver splits up transfers is entirely up to it. > >> +- ti,davinci-spi-odd-parity : odd partity enabled > >> + OR > >> + ti,davinci-spi-even-parity : even parity enabled > > What do these mean? > Supported by OMAP-L138/da830: > " > SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. > 0 An even parity flag is added at the end of the transmit data stream. > 1 An odd parity flag is added at the end of the transmit data stream. Why would these be specified in DT and not with runtime flags enabled by the device? It looks like they affect the data stream generated by the controller so the client device needs to know about them; I'd expect that it's device driver would be controlling when they are enabled if the controller supports them. > >> +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) > > The bindings should be independent of the kernel, the values need to be > > included here (and the defines moved to include/dt-bindings so they can > > be used when writing DTs). > Allowed values here are: > #define SPI_IO_TYPE_INTR 0 > #define SPI_IO_TYPE_POLL 1 > #define SPI_IO_TYPE_DMA 2 > I'll update. Now I see these it's not at all clear why this is configurable at all - I would expect the controller driver to automatically select the most appropriate scheme for the transfers it's doing in order to obtain the best performance rather than having a global switch for this. For almost all devices the best results will be obtained by doing a mix of these modes. Typically for each level of transfer there will be some minimal size below which it doesn't make sense to use that control type, for example it's common to only DMA if there's more than a FIFO worth of data to transfer.
Hi Mark, I'm very sorry for delayed replay. On 08/22/2014 06:06 PM, Mark Brown wrote: > On Fri, Aug 22, 2014 at 04:33:09PM +0300, Grygorii Strashko wrote: >> On 08/21/2014 09:20 PM, Mark Brown wrote: >>> On Thu, Aug 21, 2014 at 06:25:06PM +0300, Grygorii Strashko wrote: > >>>> +- ti,davinci-spi-wdelay : delay between transmissions. > >>> I don't understand why this is here - there is already standard support >>> in the SPI core for client drivers specifying inter-transfer delays. If >>> there is a need to provide platform configuration for this in addition >>> to that then it should also be a standard property, there is nothing >>> device specific about these. > >> Sorry, may be I've missed smth, because I was not able to find such common >> property in SPI bindings document and code. Could you point me on, pls? > > It's not a property, it's what the delay_usecs in the transfer does. > There is no obvious reason to apply something like this unconditionally > on every transfer in DT - either it's needed only at particular points > in the transfer for device reasons (in which case the device driver > needs to know about it) or we need some sort of association with what > the device is doing since the way the client driver splits up transfers > is entirely up to it. I think we have some misunderstanding here :( 1) All new properties a optional and should be specified for SPI Slave devices 2) Seems we are talking using different terms: - you referring to the term "transfers" - sequence of packets. Each packet is one transfer (array of words). - while these new properties affect on "transmissions" - sequence of words. Each word is one transmission. Below is timing diagram which shows, in general, how these new parameters affect on words transmission over Keystone/Davinci SPI bus: +-+ +-+ +-+ +-+ +-+ +-+ +-+ +-+ SPI_CLK | | | | | | | | | | | | | | | | +----------+ +-+ +-+ +-+ +-+ +---------------------------+ +-+ +-+ +- SPI_SOMI/SIMO+-----------------+ +----------- +----------+ word1 +---------------------------+word2 +-----------------+ +----------- WDELAY <---------> + + SPI_CS | | +----+ +-----------+ | | | +-----+-----------------+--------+ +-----+------------ | | | | | | + + + | + + <---> <------> <---> C2TDELAY T2CDELAY C2TDELAY Where: WDELAY - Delay in between transmissions C2TDELAY - Chip-select-active-to-transmit-start-delay T2CDELAY - Transmit-end-to-chip-select-inactive-delay Also, below is additional information about properties which are used in 5-pin mode (SPI_READY) to improve error detection [OMAP-L138/da830 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf]: >- ti,davinci-spi-t2e-delay: t2e delay Transmit-data-finished-to-SPIx_ENA-pin-inactive-time-out. T2EDELAY is used in master mode only. It defines a time-out value as a multiple of SPI clock before the SPIx_ENA signal has to become inactive and after the CS becomes inactive. The SPI clock depends on which data format is selected. If the slave device is missing one or more clock edges, it is becoming desynchronized. Although the master has finished the data transfer the slave is still waiting for the missed clock pulses and the SPIx_ENA signal is not disabled. The T2EDELAY defines a time-out value that triggers the DESYNC flag, if the SPIx_ENA signal is not deactivated in time. The DESYNC flag is set to indicate that the slave device did not deassert its SPIx_ENA pin in time to acknowledge that it has received all the bits of the sent character. The DESYNC flag is also set if the SPI detects a deassertion of the SPIx_ENA pin even before the end of the transmission. >- ti,davinci-spi-c2e-delay: c2e delay Chip-select-active-to-SPIx_ENA-signal-active-time-out. C2EDELAY is used only in master mode and it applies only if the addressed slave generates an SPIx_ENA signal as a hardware handshake response. C2EDELAY defines the maximum time between the SPI activates the chip select signal and the addressed slave has to respond by activating the SPIx_ENA signal. > >>>> +- ti,davinci-spi-odd-parity : odd partity enabled >>>> + OR >>>> + ti,davinci-spi-even-parity : even parity enabled > >>> What do these mean? > >> Supported by OMAP-L138/da830: >> " >> SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. >> 0 An even parity flag is added at the end of the transmit data stream. >> 1 An odd parity flag is added at the end of the transmit data stream. > > Why would these be specified in DT and not with runtime flags enabled by > the device? It looks like they affect the data stream generated by the > controller so the client device needs to know about them; I'd expect > that it's device driver would be controlling when they are enabled if > the controller supports them. > Could you clarify, pls - Do you mean that struct spi_device.mode and common SPI DT bindings should be extended to support this? >>>> +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) > >>> The bindings should be independent of the kernel, the values need to be >>> included here (and the defines moved to include/dt-bindings so they can >>> be used when writing DTs). > >> Allowed values here are: >> #define SPI_IO_TYPE_INTR 0 >> #define SPI_IO_TYPE_POLL 1 >> #define SPI_IO_TYPE_DMA 2 > >> I'll update. > > Now I see these it's not at all clear why this is configurable at all - > I would expect the controller driver to automatically select the most > appropriate scheme for the transfers it's doing in order to obtain the > best performance rather than having a global switch for this. For > almost all devices the best results will be obtained by doing a mix of > these modes. > > Typically for each level of transfer there will be some minimal size > below which it doesn't make sense to use that control type, for example > it's common to only DMA if there's more than a FIFO worth of data to > transfer. > Ok. It can be dropped from DT Best regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 05, 2014 at 05:21:56PM +0300, Grygorii Strashko wrote: > I think we have some misunderstanding here :( > 1) All new properties a optional and should be specified for SPI Slave devices > 2) Seems we are talking using different terms: > - you referring to the term "transfers" - sequence of packets. > Each packet is one transfer (array of words). > - while these new properties affect on "transmissions" - sequence of words. > Each word is one transmission. That's *very* unusual terminology which doesn't match my expectations at all. Please describe words as words, that'll be much more obvious. > Also, below is additional information about properties which > are used in 5-pin mode (SPI_READY) to improve error detection > [OMAP-L138/da830 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf]: This is a *whole* other thing, please split these out and work on this separately. The client device is going to need to be doing the same thing here so implementing this as a local option in the controller driver isn't the best way forwards. > >> SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. > >> 0 An even parity flag is added at the end of the transmit data stream. > >> 1 An odd parity flag is added at the end of the transmit data stream. > > Why would these be specified in DT and not with runtime flags enabled by > > the device? It looks like they affect the data stream generated by the > > controller so the client device needs to know about them; I'd expect > > that it's device driver would be controlling when they are enabled if > > the controller supports them. > Could you clarify, pls - Do you mean that struct spi_device.mode and > common SPI DT bindings should be extended to support this? Yes, if they aren't something that's purely internal to the device they need to be generic so that both devices can be configured appropriately.
Hi Mark, On 09/06/2014 05:31 PM, Mark Brown wrote: > On Fri, Sep 05, 2014 at 05:21:56PM +0300, Grygorii Strashko wrote: > >> I think we have some misunderstanding here :( >> 1) All new properties a optional and should be specified for SPI Slave devices >> 2) Seems we are talking using different terms: >> - you referring to the term "transfers" - sequence of packets. >> Each packet is one transfer (array of words). >> - while these new properties affect on "transmissions" - sequence of words. >> Each word is one transmission. > > That's *very* unusual terminology which doesn't match my expectations at > all. Please describe words as words, that'll be much more obvious. These terms are used in DM/TRM :( I'll split this patch and first introduce WDELAY, C2TDELAY, T2CDELAY (with updated documentation). The only question is - Should they be somehow common or specific for spi-davinci? > >> Also, below is additional information about properties which >> are used in 5-pin mode (SPI_READY) to improve error detection >> [OMAP-L138/da830 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf]: > > This is a *whole* other thing, please split these out and work on this > separately. The client device is going to need to be doing the same > thing here so implementing this as a local option in the controller > driver isn't the best way forwards. > >>>> SPIFMTn[23].PARPOL - Parity polarity: even or odd. PARPOL can be modified in privilege mode only. >>>> 0 An even parity flag is added at the end of the transmit data stream. >>>> 1 An odd parity flag is added at the end of the transmit data stream. > >>> Why would these be specified in DT and not with runtime flags enabled by >>> the device? It looks like they affect the data stream generated by the >>> controller so the client device needs to know about them; I'd expect >>> that it's device driver would be controlling when they are enabled if >>> the controller supports them. > >> Could you clarify, pls - Do you mean that struct spi_device.mode and >> common SPI DT bindings should be extended to support this? > > Yes, if they aren't something that's purely internal to the device they > need to be generic so that both devices can be configured appropriately. > Regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 08, 2014 at 04:30:57PM +0300, Grygorii Strashko wrote: > I'll split this patch and first introduce WDELAY, C2TDELAY, T2CDELAY > (with updated documentation). > The only question is - Should they be somehow common or specific for spi-davinci? If they don't have any impact on the device at the remote end of the link then making them device specific is fine.
diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt index f80887b..bfcd42d 100644 --- a/Documentation/devicetree/bindings/spi/spi-davinci.txt +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt @@ -24,6 +24,22 @@ Optional: cs-gpios = <0>, <0>, <0>, <&gpio1 30 0>, <&gpio1 31 0>; where first three are internal CS and last two are GPIO CS. +Optional properties for slave devices: +SPI slave nodes can contain the following properties. +Not all SPI Peripherals from Texas Instruments support this. +Please check SPI peripheral documentation for a device before using these. + +- ti,davinci-spi-wdelay : delay between transmissions. +- ti,davinci-spi-odd-parity : odd partity enabled + OR + ti,davinci-spi-even-parity : even parity enabled +- ti,davinci-spi-io-type: io type (check platform_data/spi-davinci.h) +- ti,davinci-spi-disable-timer: disable CS timer (SPIFMTn) +- ti,davinci-spi-c2t-delay: c2t delay +- ti,davinci-spi-t2c-delay: t2c delay +- ti,davinci-spi-t2e-delay: t2e delay +- ti,davinci-spi-c2e-delay: c2e delay + Example of a NOR flash slave device (n25q032) connected to DaVinci SPI controller device over the SPI bus. @@ -43,6 +59,8 @@ spi0:spi@20BF0000 { compatible = "st,m25p32"; spi-max-frequency = <25000000>; reg = <0>; + ti,davinci-spi-c2t-delay = <8>; + ti,davinci-spi-t2c-delay = <8>; partition@0 { label = "u-boot-spl"; diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c index 48f1d26..0e276ad 100644 --- a/drivers/spi/spi-davinci.c +++ b/drivers/spi/spi-davinci.c @@ -65,6 +65,7 @@ /* SPIDAT1 (upper 16 bit defines) */ #define SPIDAT1_CSHOLD_MASK BIT(12) +#define SPIDAT1_WDEL BIT(10) /* SPIGCR1 */ #define SPIGCR1_CLKMOD_MASK BIT(1) @@ -209,6 +210,7 @@ static void davinci_spi_chipselect(struct spi_device *spi, int value) { struct davinci_spi *dspi; struct davinci_spi_platform_data *pdata; + struct davinci_spi_config *spicfg = spi->controller_data; u8 chip_sel = spi->chip_select; u16 spidat1 = CS_DEFAULT; bool gpio_chipsel = false; @@ -223,6 +225,10 @@ static void davinci_spi_chipselect(struct spi_device *spi, int value) gpio = spi->cs_gpio; } + /* program delay transfers if tx_delay is non zero */ + if (spicfg->wdelay) + spidat1 |= SPIDAT1_WDEL; + /* * Board specific chip select logic decides the polarity and cs * line for the controller @@ -237,9 +243,9 @@ static void davinci_spi_chipselect(struct spi_device *spi, int value) spidat1 |= SPIDAT1_CSHOLD_MASK; spidat1 &= ~(0x1 << chip_sel); } - - iowrite16(spidat1, dspi->base + SPIDAT1 + 2); } + + iowrite16(spidat1, dspi->base + SPIDAT1 + 2); } /** @@ -285,7 +291,7 @@ static int davinci_spi_setup_transfer(struct spi_device *spi, int prescale; dspi = spi_master_get_devdata(spi->master); - spicfg = (struct davinci_spi_config *)spi->controller_data; + spicfg = spi->controller_data; if (!spicfg) spicfg = &davinci_spi_default_cfg; @@ -333,6 +339,14 @@ static int davinci_spi_setup_transfer(struct spi_device *spi, spifmt |= SPIFMT_PHASE_MASK; /* + * Assume wdelay is used only on SPI peripherals that has this field + * in SPIFMTn register and when it's configured from board file or DT. + */ + if (spicfg->wdelay) + spifmt |= ((spicfg->wdelay << SPIFMT_WDELAY_SHIFT) + & SPIFMT_WDELAY_MASK); + + /* * Version 1 hardware supports two basic SPI modes: * - Standard SPI mode uses 4 pins, with chipselect * - 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS) @@ -349,9 +363,6 @@ static int davinci_spi_setup_transfer(struct spi_device *spi, u32 delay = 0; - spifmt |= ((spicfg->wdelay << SPIFMT_WDELAY_SHIFT) - & SPIFMT_WDELAY_MASK); - if (spicfg->odd_parity) spifmt |= SPIFMT_ODD_PARITY_MASK; @@ -383,6 +394,55 @@ static int davinci_spi_setup_transfer(struct spi_device *spi, return 0; } +static int davinci_spi_of_setup(struct spi_device *spi) +{ + struct davinci_spi_config *spicfg = spi->controller_data; + struct device_node *np = spi->dev.of_node; + u32 prop; + int len; + + if (spicfg == NULL && np) { + spicfg = kzalloc(sizeof(*spicfg), GFP_KERNEL); + if (!spicfg) + return -ENOMEM; + *spicfg = davinci_spi_default_cfg; + /* override with dt configured values */ + if (!of_property_read_u32(np, + "ti,davinci-spi-wdelay", &prop)) + spicfg->wdelay = (u8)prop; + if (of_find_property(np, + "ti,davinci-spi-odd-parity", &len)) { + spicfg->parity_enable = 1; + spicfg->odd_parity = 1; + } else if (of_find_property(np, "ti,davinci-spi-even-parity", + &len)) { + spicfg->parity_enable = 1; + spicfg->odd_parity = 0; + } + if (!of_property_read_u32(np, "ti,davinci-spi-io-type", + &prop)) + spicfg->io_type = (u8)prop; + if (of_find_property(np, + "ti,davinci-spi-disable-timer", &len)) + spicfg->timer_disable = 1; + if (!of_property_read_u32(np, + "ti,davinci-spi-c2t-delay", &prop)) + spicfg->c2tdelay = (u8)prop; + if (!of_property_read_u32(np, + "ti,davinci-spi-t2c-delay", &prop)) + spicfg->t2cdelay = (u8)prop; + if (!of_property_read_u32(np, + "ti,davinci-spi-t2e-delay", &prop)) + spicfg->t2edelay = (u8)prop; + if (!of_property_read_u32(np, + "ti,davinci-spi-c2e-delay", &prop)) + spicfg->c2edelay = (u8)prop; + spi->controller_data = spicfg; + } + + return 0; +} + /** * davinci_spi_setup - This functions will set default transfer method * @spi: spi device on which data transfer to be done @@ -436,11 +496,17 @@ static int davinci_spi_setup(struct spi_device *spi) else clear_io_bits(dspi->base + SPIGCR1, SPIGCR1_LOOPBACK_MASK); - return retval; + return davinci_spi_of_setup(spi); } static void davinci_spi_cleanup(struct spi_device *spi) { + struct davinci_spi_config *spicfg = spi->controller_data; + + spi->controller_data = NULL; + if (spi->dev.of_node) + kfree(spicfg); + if (spi->cs_gpio >= 0) gpio_free(spi->cs_gpio); }