Skip to content

Support enhanced SPI operation in spi_dw driver #77492

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Support enhanced SPI operation in spi_dw driver #77492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0969d8
drivers: spi: dw: support rx sample delay
myguitar Aug 23, 2024
1a86f37
drivers: spi: dw: fix dfs offset in HSSI controller
myguitar Aug 23, 2024
7370465
drivers: spi: dw: fix txftlr in HSSI controller
myguitar Aug 23, 2024
45ebdf1
drivers: spi: dw: read ssi component version
myguitar Aug 23, 2024
a0bd53d
drivers: spi: support enhanced SPI operations
myguitar Aug 23, 2024
7ad1b6f
drivers: spi: dw: support enhanced SPI operation
myguitar Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 66 additions & 1 deletion drivers/spi/spi_dw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ LOG_MODULE_REGISTER(spi_dw);
#include <zephyr/drivers/pinctrl.h>
#endif

#define DW_HSSI_VER_102A (0x3130322a)

static inline bool spi_dw_is_slave(struct spi_dw_data *spi)
{
return (IS_ENABLED(CONFIG_SPI_SLAVE) &&
Expand Down Expand Up @@ -213,6 +215,7 @@ static int spi_dw_configure(const struct device *dev,

if ((config->operation & SPI_TRANSFER_LSB) ||
(IS_ENABLED(CONFIG_SPI_EXTENDED_MODES) &&
!IS_ENABLED(CONFIG_SPI_DW_HSSI) &&
(config->operation & (SPI_LINES_DUAL |
SPI_LINES_QUAD | SPI_LINES_OCTAL)))) {
LOG_ERR("Unsupported configuration");
Expand All @@ -226,7 +229,7 @@ static int spi_dw_configure(const struct device *dev,
}

/* Word size */
if (info->max_xfer_size == 32) {
if (!IS_ENABLED(CONFIG_SPI_DW_HSSI) && (info->max_xfer_size == 32)) {
ctrlr0 |= DW_SPI_CTRLR0_DFS_32(SPI_WORD_SIZE_GET(config->operation));
} else {
ctrlr0 |= DW_SPI_CTRLR0_DFS_16(SPI_WORD_SIZE_GET(config->operation));
Expand All @@ -248,6 +251,23 @@ static int spi_dw_configure(const struct device *dev,
ctrlr0 |= DW_SPI_CTRLR0_SRL;
}

#if defined(CONFIG_SPI_DW_HSSI) && defined(CONFIG_SPI_EXTENDED_MODES)
if (spi->version >= DW_HSSI_VER_102A) {
/* SPI frame format for Tx/Rx data */
switch (SPI_LINES_GET(config->operation)) {
case SPI_LINES_DUAL:
ctrlr0 |= DW_SPI_CTRLR0_SPI_DUAL;
break;
case SPI_LINES_QUAD:
ctrlr0 |= DW_SPI_CTRLR0_SPI_QUAD;
break;
case SPI_LINES_OCTAL:
ctrlr0 |= DW_SPI_CTRLR0_SPI_OCTAL;
break;
}
}
#endif

/* Installing the configuration */
write_ctrlr0(dev, ctrlr0);

Expand Down Expand Up @@ -323,6 +343,18 @@ static void spi_dw_update_txftlr(const struct device *dev,
} else if (spi->ctx.tx_len < dw_spi_txftlr_dflt) {
reg_data = spi->ctx.tx_len - 1;
}
} else {
#if defined(CONFIG_SPI_DW_HSSI) && defined(CONFIG_SPI_EXTENDED_MODES)
/*
* TXFTLR field in the TXFTLR register is valid only for
* Controller mode operation
*/
if (!spi->ctx.tx_len) {
reg_data = 0U;
} else if (spi->ctx.tx_len < dw_spi_txftlr_dflt) {
reg_data = (spi->ctx.tx_len - 1) << DW_SPI_TXFTLR_TXFTLR_SHIFT;
}
#endif
}

LOG_DBG("TxFTLR: %u", reg_data);
Expand Down Expand Up @@ -398,6 +430,23 @@ static int transceive(const struct device *dev,

write_ctrlr0(dev, reg_data);

#if defined(CONFIG_SPI_DW_HSSI) && defined(CONFIG_SPI_EXTENDED_MODES)
if (spi->version >= DW_HSSI_VER_102A) {
/* Enhanced SPI operation */
reg_data = read_spi_ctrlr0(dev);
reg_data &= ~DW_SPI_ESPI_CTRLR0_TRANS_TYPE_MASK;
reg_data |= FIELD_PREP(DW_SPI_ESPI_CTRLR0_TRANS_TYPE_MASK,
SPI_TRANS_TYPE_FIELD_GET(config->operation));
reg_data &= ~DW_SPI_ESPI_CTRLR0_ADDR_L_MASK;
reg_data |= FIELD_PREP(DW_SPI_ESPI_CTRLR0_ADDR_L_MASK,
SPI_ADDR_L_FIELD_GET(config->operation));
reg_data &= ~DW_SPI_ESPI_CTRLR0_INST_L_MASK;
reg_data |= FIELD_PREP(DW_SPI_ESPI_CTRLR0_INST_L_MASK,
SPI_INST_L_FIELD_GET(config->operation));
write_spi_ctrlr0(dev, reg_data);
}
#endif

/* Set buffers info */
spi_context_buffers_setup(&spi->ctx, tx_bufs, rx_bufs, spi->dfs);

Expand All @@ -423,6 +472,15 @@ static int transceive(const struct device *dev,
/* Rx Threshold */
write_rxftlr(dev, reg_data);

/* Rx sample delay */
if (info->rx_sample_delay) {
reg_data = read_rx_sample_dly(dev);
reg_data &= ~DW_SPI_RX_SAMPLE_DELAY_MASK;
reg_data |= FIELD_PREP(DW_SPI_RX_SAMPLE_DELAY_MASK,
info->rx_sample_delay);
write_rx_sample_dly(dev, reg_data);
}

/* Enable interrupts */
reg_data = !rx_bufs ?
DW_SPI_IMR_UNMASK & DW_SPI_IMR_MASK_RX :
Expand Down Expand Up @@ -554,6 +612,12 @@ int spi_dw_init(const struct device *dev)
write_imr(dev, DW_SPI_IMR_MASK);
clear_bit_ssienr(dev);

/* SSI component version */
spi->version = read_ssi_comp_version(dev);
LOG_DBG("Version: %c.%c%c%c", (spi->version >> 24) & 0xff,
(spi->version >> 16) & 0xff, (spi->version >> 8) & 0xff,
spi->version & 0xff);

LOG_DBG("Designware SPI driver initialized on device: %p", dev);

err = spi_context_cs_configure_all(&spi->ctx);
Expand Down Expand Up @@ -647,6 +711,7 @@ COND_CODE_1(IS_EQ(DT_NUM_IRQS(DT_DRV_INST(inst)), 1), \
.serial_target = DT_INST_PROP(inst, serial_target), \
.fifo_depth = DT_INST_PROP(inst, fifo_depth), \
.max_xfer_size = DT_INST_PROP(inst, max_xfer_size), \
.rx_sample_delay = DT_INST_PROP(inst, rx_sample_delay), \
IF_ENABLED(CONFIG_PINCTRL, (.pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(inst),)) \
COND_CODE_1(DT_INST_PROP(inst, aux_reg), \
(.read_func = aux_reg_read, \
Expand Down
22 changes: 22 additions & 0 deletions drivers/spi/spi_dw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ struct spi_dw_config {
bool serial_target;
uint8_t fifo_depth;
uint8_t max_xfer_size;
uint8_t rx_sample_delay;
#ifdef CONFIG_PINCTRL
const struct pinctrl_dev_config *pcfg;
#endif
Expand All @@ -48,6 +49,7 @@ struct spi_dw_config {
struct spi_dw_data {
DEVICE_MMIO_RAM;
struct spi_context ctx;
uint32_t version; /* ssi comp version */
uint8_t dfs; /* dfs in bytes: 1,2 or 4 */
uint8_t fifo_diff; /* cannot be bigger than FIFO depth */
};
Copy link
Contributor

@tbursztyka tbursztyka Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it above dfs for better struct alignement

Expand Down Expand Up @@ -194,6 +196,23 @@ static int reg_test_bit(uint8_t bit, mm_reg_t addr, uint32_t off)
#define DW_SPI_CTRLR0_SRL_BIT (13)
#endif

#if defined(CONFIG_SPI_DW_HSSI) && defined(CONFIG_SPI_EXTENDED_MODES)
/* TXFTLR setting. Only valid for Controller operation mode. */
#define DW_SPI_TXFTLR_TXFTLR_SHIFT (16)

/* SPI Frame Format */
#define DW_SPI_CTRLR0_SPI_FRF_SHIFT (22)
#define DW_SPI_CTRLR0_SPI_STANDARD (0 << DW_SPI_CTRLR0_SPI_FRF_SHIFT)
#define DW_SPI_CTRLR0_SPI_DUAL (1 << DW_SPI_CTRLR0_SPI_FRF_SHIFT)
#define DW_SPI_CTRLR0_SPI_QUAD (2 << DW_SPI_CTRLR0_SPI_FRF_SHIFT)
#define DW_SPI_CTRLR0_SPI_OCTAL (3 << DW_SPI_CTRLR0_SPI_FRF_SHIFT)

/* SPI_CTRLR0 settings */
#define DW_SPI_ESPI_CTRLR0_INST_L_MASK GENMASK(9, 8)
#define DW_SPI_ESPI_CTRLR0_ADDR_L_MASK GENMASK(5, 2)
#define DW_SPI_ESPI_CTRLR0_TRANS_TYPE_MASK GENMASK(1, 0)
#endif

#define DW_SPI_CTRLR0_SCPH BIT(DW_SPI_CTRLR0_SCPH_BIT)
#define DW_SPI_CTRLR0_SCPOL BIT(DW_SPI_CTRLR0_SCPOL_BIT)
#define DW_SPI_CTRLR0_SRL BIT(DW_SPI_CTRLR0_SRL_BIT)
Expand Down Expand Up @@ -273,6 +292,9 @@ static int reg_test_bit(uint8_t bit, mm_reg_t addr, uint32_t off)
DW_SPI_IMR_RXOIM | \
DW_SPI_IMR_RXFIM))

/* RX Sample Delay */
#define DW_SPI_RX_SAMPLE_DELAY_MASK GENMASK(7, 0)

/*
* Including the right register definition file
* SoC SPECIFIC!
Expand Down
5 changes: 5 additions & 0 deletions drivers/spi/spi_dw_regs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ extern "C" {
#define DW_SPI_REG_SSI_COMP_VERSION (0x5c)
#define DW_SPI_REG_DR (0x60)
#define DW_SPI_REG_RX_SAMPLE_DLY (0xf0)
#define DW_SPI_REG_SPI_CTRLR0 (0xf4)

/* Register helpers */
DEFINE_MM_REG_WRITE(ctrlr0, DW_SPI_REG_CTRLR0, 32)
Expand All @@ -51,6 +52,10 @@ DEFINE_MM_REG_READ(txftlr, DW_SPI_REG_TXFTLR, 32)
DEFINE_MM_REG_WRITE(dr, DW_SPI_REG_DR, 32)
DEFINE_MM_REG_READ(dr, DW_SPI_REG_DR, 32)
DEFINE_MM_REG_READ(ssi_comp_version, DW_SPI_REG_SSI_COMP_VERSION, 32)
DEFINE_MM_REG_WRITE(rx_sample_dly, DW_SPI_REG_RX_SAMPLE_DLY, 32)
DEFINE_MM_REG_READ(rx_sample_dly, DW_SPI_REG_RX_SAMPLE_DLY, 32)
DEFINE_MM_REG_WRITE(spi_ctrlr0, DW_SPI_REG_SPI_CTRLR0, 32)
DEFINE_MM_REG_READ(spi_ctrlr0, DW_SPI_REG_SPI_CTRLR0, 32)

#ifdef CONFIG_SPI_DW_ACCESS_WORD_ONLY
DEFINE_MM_REG_WRITE(ctrlr1, DW_SPI_REG_CTRLR1, 32)
Expand Down
9 changes: 9 additions & 0 deletions dts/bindings/spi/snps,designware-spi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,12 @@ properties:
enum:
- 16
- 32

rx-sample-delay:
type: int
default: 0
description: |
Receive Data(rxd) Sample Delay. This is used to delay
the sample of the rxd input port. Each value represents
a single ssi_clk delay on the sample of rxd. Default is
0 and delay ranges from 0-255.
109 changes: 108 additions & 1 deletion include/zephyr/drivers/spi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,110 @@ extern "C" {
#define SPI_LINES_OCTAL (3U << 16) /**< Octal lines */

#define SPI_LINES_MASK (0x3U << 16) /**< Mask for MISO lines in spi_operation_t */
#define SPI_LINES_GET(_line_) \
((_line_) & SPI_LINES_MASK)

/** @} */

/**
Copy link
Contributor

@tbursztyka tbursztyka Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not going to get in. First, it's not well thought through: when proposing a change in generic API, it should be possible to apply such change to a maximum of devices. I can see this has been crafted too much around DW's specs.

If you are willing to support dual/quad/octal modes on dw, SPI API is unfortunately not going to be the place. I tried it before ( see #39991 as a preliminary proposal) and it never came to an agreement on extending SPI API to support these modes.

Instead, you'll have to write a totally new driver for MSPI API.

Copy link
Contributor Author

@myguitar myguitar Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing valuable feedback. I also had the same concern on the DW specific operation configuration and wanted to get some feedback about it first. I am also considering to have a priv_operation in spi_config for the device specific operation rather than creating a new driver since most of DW code is reusable. Do it make sense?

Copy link
Contributor

@tbursztyka tbursztyka Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not the right thing to do because it would open up driver specific features up to the user directly, then user code would become:

  • not generic
  • thus not portable to another SPI controller.

Copy link
Contributor Author

@myguitar myguitar Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tbursztyka It's not generic only for device specific features. We could move it to common later once other devices need the same features which is very common steps and somewhat expected. It would be simple to create a new driver, but we may have other challenges like code reusability or backward compatibility. There are always pros and cons, but I still think reusing the existing spi_dw driver would be acceptable than creating a new one. What about using an another CONFIG to extend features in include/zephyr/drivers/spi.h with an additional field like espi_operation?

Copy link
Contributor

@tbursztyka tbursztyka Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be no alternative such as mspi, I would push for an update of the SPI API like #39991 etc... But we are way over this. I am sorry your PR came too late (I was alone at the time trying to push for an update, which did not get any traction, because only flash memories were the actual use case: ppl were making qspi drivers directly for flash, which made sense at the time).

* @name SPI Instruction and Address transfer format
* @{
*
* Some controllers support the enhanced SPI operations divided into multi
* phases - Instruction/Address/Data.
* The Instruction/Address transfer format will be either in Standard SPI mode
* or the SPI mode configured in CTRLR0 frame format.
*
* TT0: Instruction/Address in Standard SPI mode
* TT1: Instruction in Standard SPI mode, Address in the mode specified by
* CTRLR0 frame format
* TT2: Instruction/Address in the mode specified by CTRLR0 frame format field
* TT3: Dual Octal mode. Instruction/Address in octal mode, data is transferred
* on 16 data lines
*
* Supported only if @kconfig{CONFIG_SPI_EXTENDED_MODES} is enabled
*/

#define SPI_TRANS_TYPE_SHIFT (18)
#define SPI_TRANS_TYPE_MASK (0x3U << SPI_TRANS_TYPE_SHIFT)
#define SPI_TRANS_TYPE_TT0 (0U << SPI_TRANS_TYPE_SHIFT)
#define SPI_TRANS_TYPE_TT1 (1U << SPI_TRANS_TYPE_SHIFT)
#define SPI_TRANS_TYPE_TT2 (2U << SPI_TRANS_TYPE_SHIFT)
#define SPI_TRANS_TYPE_TT3 (3U << SPI_TRANS_TYPE_SHIFT)
#define SPI_TRANS_TYPE_GET(_type_) \
((_type_) & SPI_TRANS_TYPE_MASK)
#define SPI_TRANS_TYPE_FIELD_GET(_type_) \
(SPI_TRANS_TYPE_GET(_type_) >> SPI_TRANS_TYPE_SHIFT)

/** @} */

/**
* @name SPI address length to be transmitted
* @{
*
* Some controllers support the enhanced SPI operations divided into multi
* phases - Instruction/Address/Data. The address transfer begins only after
* this much bits are programmed into the FIFO.
*
* L0: No Address
* L4: 4 bit Address length
* L8: 8 bit Address length
* Ln: n bit Address length respectively
*
* Supported only if @kconfig{CONFIG_SPI_EXTENDED_MODES} is enabled
*/

#define SPI_ADDR_L_SHIFT (20)
#define SPI_ADDR_L_MASK (0xFU << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L0 (0U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L4 (1U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L8 (2U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L12 (3U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L16 (4U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L20 (5U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L24 (6U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L28 (7U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L32 (8U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L36 (9U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L40 (10U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L44 (11U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L48 (12U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L52 (13U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L56 (14U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L60 (15U << SPI_ADDR_L_SHIFT)
#define SPI_ADDR_L_GET(_length_) \
((_length_) & SPI_ADDR_L_MASK)
#define SPI_ADDR_L_FIELD_GET(_length_) \
(SPI_ADDR_L_GET(_length_) >> SPI_ADDR_L_SHIFT)

/** @} */

/**
* @name SPI Instruction and Address transfer format.
* @{
*
* Some controllers support the enhanced SPI operations divided into multi
* phases - Instruction/Address/Data. This specifies the instruction length
* in bits.
*
* L0: No Instruction
* L4: 4 bit Instruction length
* L8: 8 bit Instruction length
* L16: 16 bit Instruction length
*
* Supported only if @kconfig{CONFIG_SPI_EXTENDED_MODES} is enabled
*/

#define SPI_INST_L_SHIFT (24)
#define SPI_INST_L_MASK (0x3U << SPI_INST_L_SHIFT)
#define SPI_INST_L0 (0U << SPI_INST_L_SHIFT)
#define SPI_INST_L4 (1U << SPI_INST_L_SHIFT)
#define SPI_INST_L8 (2U << SPI_INST_L_SHIFT)
#define SPI_INST_L12 (3U << SPI_INST_L_SHIFT)
#define SPI_INST_L_GET(_length_) \
((_length_) & SPI_INST_L_MASK)
#define SPI_INST_L_FIELD_GET(_length_) \
(SPI_INST_L_GET(_length_) >> SPI_INST_L_SHIFT)

/** @} */

Expand Down Expand Up @@ -319,7 +423,10 @@ struct spi_config {
* If @kconfig{CONFIG_SPI_EXTENDED_MODES} is enabled:
*
* - 16..17: MISO lines (Single/Dual/Quad/Octal).
* - 18..31: Reserved for future use.
* - 18..19: Transfer Type (Address and instruction transfer format).
* - 20..23: Length of Address to be transmitted.
* - 24..25: Enhanced SPI mode instruction length in bits
* - 26..31: Reserved for future use.
*/
spi_operation_t operation;
/** @brief Slave number from 0 to host controller slave limit. */
Expand Down