On Sat, Aug 27, 2022 at 01:03:22AM +0300, Serge Semin wrote:
> On Tue, Aug 02, 2022 at 06:57:47PM +0100, Sudip Mukherjee wrote:
> > The SPI mode needs to be mentioned in CTRLR0[23:22] register. Define a
> > configuration variable to keep the mode based on the buswidth, which will
> > then be used to update CR0. If the transfer is using dual/quad/octal
> > mode then mark enhanced_spi as true.
> >
> > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@xxxxxxxxxx>
> > ---
> > drivers/spi/spi-dw-core.c | 29 +++++++++++++++++++++++++++++
> > drivers/spi/spi-dw.h | 7 +++++++
> > 2 files changed, 36 insertions(+)
> >
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index 77529e359b6d..8c84a2e991b5 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -333,6 +333,14 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> > /* CTRLR0[11:10] Transfer Mode */
> > cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_TMOD_MASK, cfg->tmode);
> >
>
> > + if (dws->caps & DW_SPI_CAP_EXT_SPI) {
>
> Drop this conditional statement. Just always set the spi_frf field.
>
>
> > + if (cfg->spi_frf)
>
> Drop this conditional statement. Just always set the spi_frf. It shall
> be zero if the Enhanced SPI modes are unsupported.
>
> > + cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,
>
> Since the SPI_FRF field has different offset in the DW APB and DW AHB
> SSI controllers, you'll need to initialize the mode based on the
> IP-core ID using the dw_spi_ip_is() macro (see the as it's done for
> tmode).
>
> > + cfg->spi_frf);
> > + else
>
> > + cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;
>
> This is redundant. The initial value never has the SPI_FRF field set
> (see the dw_spi_setup() and dw_spi_prepare_cr0() methods for details).
>
> > + }
> > +
> > dw_writel(dws, DW_SPI_CTRLR0, cr0);
> >
> > if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> > @@ -679,10 +687,31 @@ static void dw_spi_stop_mem_op(struct dw_spi *dws, struct spi_device *spi)
> > static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> > {
> > struct dw_spi *dws = spi_controller_get_devdata(mem->spi->controller);
> > + bool enhanced_spi = false;
> > struct dw_spi_cfg cfg;
> > unsigned long flags;
> > int ret;
> >
>
> > + if (dws->caps & DW_SPI_CAP_EXT_SPI) {
> > + switch (op->data.buswidth) {
> > + case 2:
> > + cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI;
> > + enhanced_spi = true;
> > + break;
> > + case 4:
> > + cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI;
> > + enhanced_spi = true;
> > + break;
> > + case 8:
> > + cfg.spi_frf = DW_SSI_CTRLR0_SPI_FRF_OCT_SPI;
> > + enhanced_spi = true;
> > + break;
> > + default:
> > + cfg.spi_frf = 0;
> > + break;
> > + }
> > + }
> > +
>
> Please create a separate dw_spi_exec_enh_mem_op() method for the
> Enhanced SPI communications and parse the data.buswidth field there.
> Also it would be better to define a new macro
> DW_SSI_CTRLR0_SPI_FRF_STD_SPI and use it instead of the explicit zero
> SPI_FRF value.
>
> > /*
> > * Collect the outbound data into a single buffer to speed the
> > * transmission up at least on the initial stage.
> > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > index 71d18e9291a3..b8cc20e0deaa 100644
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -96,6 +96,12 @@
> > #define DW_HSSI_CTRLR0_SRL BIT(13)
> > #define DW_HSSI_CTRLR0_MST BIT(31)
> >
>
> > +/* Bit fields in CTRLR0 for enhanced SPI */
>
> No need in the comment. The macros name is descriptive enough.
>
> > +#define DW_HSSI_CTRLR0_SPI_FRF_MASK GENMASK(23, 22)
>
> Please also add the next new macros
> #define DW_PSSI_CTRLR0_SPI_FRF_MASK GENMASK(22, 21)
> #define DW_SSI_CTRLR0_SPI_FRF_STD_SPI 0x0
>
> and group them together with the rest of the APB SSI CTRL0 macros.
>
> > +#define DW_SSI_CTRLR0_SPI_FRF_DUAL_SPI 0x1
> > +#define DW_SSI_CTRLR0_SPI_FRF_QUAD_SPI 0x2
> > +#define DW_SSI_CTRLR0_SPI_FRF_OCT_SPI 0x3
One more thing s/DW_SSI/DW_SPI . All the common macros are defined
with the DW_SPI prefix.
-Sergey
>
> Please move these to be defined together with the APB SSI
> CTRL0.SPI_FRF-related macros.
>
> -Sergey
>
> > +
> > /* Bit fields in CTRLR1 */
> > #define DW_SPI_NDF_MASK GENMASK(15, 0)
> >
> > @@ -136,6 +142,7 @@ struct dw_spi_cfg {
> > u8 dfs;
> > u32 ndf;
> > u32 freq;
> > + u8 spi_frf;
> > };
> >
> > struct dw_spi;
> > --
> > 2.30.2
> >
