Re: [U-Boot] [EXT] Re: [PATCH v2 2/5] dm: spi: Convert Freescale ESPI driver to driver model

24 Jun 2019

...
-----Original Message-----
From: Simon Glass sjg@chromium.org
Sent: 2019年6月23日 3:10
To: Chuanhua Han chuanhua.han@nxp.com
Cc: Jagan Teki jagan@openedev.com; Wolfgang Denk wd@denx.de;
Shengzhou Liu shengzhou.liu@nxp.com; Ruchika Gupta
ruchika.gupta@nxp.com; U-Boot Mailing List u-boot@lists.denx.de;
Prabhakar Kushwaha prabhakar.kushwaha@nxp.com; Jiafei Pan
jiafei.pan@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; Bin Meng
bmeng.cn@gmail.com; Jagdish Gediya jagdish.gediya@nxp.com
Subject: [EXT] Re: [PATCH v2 2/5] dm: spi: Convert Freescale ESPI driver to
driver model
Caution: EXT Email
Hi,
On Fri, 24 May 2019 at 05:39, Chuanhua Han chuanhua.han@nxp.com
wrote:
...
Modify the Freescale ESPI driver to support the driver model.
Also resolved the following problems:
===================== WARNING ====================== This board
does
...
not use CONFIG_DM_SPI. Please update the board before v2019.04 for no
dm conversion and v2019.07 for partially dm converted drivers.
Failure to update can lead to driver/board removal See
doc/driver-model/MIGRATION.txt for more info.
====================================================
===================== WARNING ====================== This board
does
...
not use CONFIG_DM_SPI_FLASH. Please update the board to use
CONFIG_SPI_FLASH before the v2019.07 release.
Failure to update by the deadline may result in board removal.
See doc/driver-model/MIGRATION.txt for more info.
====================================================
Signed-off-by: Chuanhua Han chuanhua.han@nxp.com
Changes in v2:
        - The fsl_espi driver support both OF_CONTROL and PLATDATA
drivers/spi/fsl_espi.c | 454
+++++++++++++++++++++++++++++------------
 1 file changed, 320 insertions(+), 134 deletions(-)

diff --git a/drivers/spi/fsl_espi.c b/drivers/spi/fsl_espi.c index
7444ae1a06..a2f4027fca 100644
--- a/drivers/spi/fsl_espi.c
+++ b/drivers/spi/fsl_espi.c
@@ -4,17 +4,27 @@


Copyright 2010-2011 Freescale Semiconductor, Inc.
Author: Mingkai Hu (Mingkai.hu@freescale.com)



   Chuanhua Han (chuanhua.han@nxp.com)



*/

#include <common.h>



#include <malloc.h>
 #include <spi.h>
 #include <asm/immap_85xx.h>
Please sort the includes - this one should go at the end
The header file is also in this position before I modify this file. Is this necessary?
...
...
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>



+struct fsl_espi_platdata {

  uint flags;


  uint speed_hz;


  uint num_chipselect;


  fdt_addr_t regs_addr;



+};
-struct fsl_spi_slave {

  struct spi_slave slave;



+struct fsl_espi_priv {
        ccsr_espi_t     *espi;

  u32 speed_hz;
  unsigned int    div16;
  unsigned int    pm;
  int             tx_timeout;



@@ -25,9 +35,18 @@ struct fsl_spi_slave {
        unsigned int    max_transfer_length;
 };
+struct fsl_spi_slave {

  struct spi_slave slave;


  struct fsl_espi_priv priv;



+};



#define to_fsl_spi_slave(s) container_of(s, struct fsl_spi_slave,
slave)
+#define to_fsl_spi_priv(p) container_of(p, struct fsl_spi_slave,
+priv)
We shouldn't really need this. Can we just pass the containing stuct instead?
This has been removed in the new version of the patch.
...
...
#define US_PER_SECOND          1000000UL
+/* default SCK frequency, unit: HZ */
+#define FSL_ESPI_DEFAULT_SCK_FREQ   10000000



#define ESPI_MAX_CS_NUM                4
 #define ESPI_FIFO_WIDTH_BIT    32
@@ -62,121 +81,46 @@ struct fsl_spi_slave {
#define ESPI_MAX_DATA_TRANSFER_LEN 0xFFF0
-struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,

          unsigned int max_hz, unsigned int mode)



-{

  struct fsl_spi_slave *fsl;


  sys_info_t sysinfo;


  unsigned long spibrg = 0;


  unsigned long spi_freq = 0;


  unsigned char pm = 0;



  if (!spi_cs_is_valid(bus, cs))


          return NULL;



  fsl = spi_alloc_slave(struct fsl_spi_slave, bus, cs);


  if (!fsl)


          return NULL;



  fsl->espi = (void *)(CONFIG_SYS_MPC85xx_ESPI_ADDR);


  fsl->mode = mode;


  fsl->max_transfer_length = ESPI_MAX_DATA_TRANSFER_LEN;



  /* Set eSPI BRG clock source */


  get_sys_info(&sysinfo);


  spibrg = sysinfo.freq_systembus / 2;


  fsl->div16 = 0;


  if ((spibrg / max_hz) > 32) {


          fsl->div16 = ESPI_CSMODE_DIV16;


          pm = spibrg / (max_hz * 16 * 2);


          if (pm > 16) {


                  pm = 16;


                  debug("Requested speed is too low: %d Hz, %ld



Hz "
...

                          "is used.\n", max_hz, spibrg / (32 * 16));


          }


  } else


          pm = spibrg / (max_hz * 2);


  if (pm)


          pm--;


  fsl->pm = pm;



  if (fsl->div16)


          spi_freq = spibrg / ((pm + 1) * 2 * 16);


  else


          spi_freq = spibrg / ((pm + 1) * 2);



  /* set tx_timeout to 10 times of one espi FIFO entry go out */


  fsl->tx_timeout = DIV_ROUND_UP((US_PER_SECOND *



ESPI_FIFO_WIDTH_BIT
...

                          * 10), spi_freq);



  return &fsl->slave;



-}



-void spi_free_slave(struct spi_slave *slave)
+#ifndef CONFIG_DM_SPI
It is more common to put #idef CONFIG_DM_SPI first with the old code last.
I have checked many files using #ifndef CONFIG_DM_SPI to separate DM code and non-dm code.
...
...
+void spi_cs_activate(struct spi_slave *slave)
 {
        struct fsl_spi_slave *fsl = to_fsl_spi_slave(slave);

  free(fsl);



-}

  ccsr_espi_t *espi = fsl->priv.espi;


  unsigned int com = 0;


  size_t data_len = fsl->priv.data_len;




-int spi_claim_bus(struct spi_slave *slave)

  com &= ~(ESPI_COM_CS(0x3) | ESPI_COM_TRANLEN(0xFFFF));


  com |= ESPI_COM_CS(slave->cs);


  com |= ESPI_COM_TRANLEN(data_len - 1);


  out_be32(&espi->com, com);



+}
+#else
+void fsl_spi_cs_activate(struct fsl_espi_priv *priv,  uint cs)
 {

  struct fsl_spi_slave *fsl = to_fsl_spi_slave(slave);


  ccsr_espi_t *espi = fsl->espi;


  unsigned char pm = fsl->pm;


  unsigned int cs = slave->cs;


  unsigned int mode =  fsl->mode;


  unsigned int div16 = fsl->div16;


  int i;



  debug("%s: bus:%i cs:%i\n", __func__, slave->bus, cs);



  /* Enable eSPI interface */


  out_be32(&espi->mode, ESPI_MODE_RXTHR(3)


                  | ESPI_MODE_TXTHR(4) | ESPI_MODE_EN);



  out_be32(&espi->event, 0xffffffff); /* Clear all eSPI events */


  out_be32(&espi->mask, 0x00000000); /* Mask  all eSPI interrupts



*/
...


  /* Init CS mode interface */


  for (i = 0; i < ESPI_MAX_CS_NUM; i++)


          out_be32(&espi->csmode[i], ESPI_CSMODE_INIT_VAL);



  out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs]) &


          ~(ESPI_CSMODE_PM(0xF) | ESPI_CSMODE_DIV16


          | ESPI_CSMODE_CI_INACTIVEHIGH |



ESPI_CSMODE_CP_BEGIN_EDGCLK
...

          | ESPI_CSMODE_REV_MSB_FIRST |



ESPI_CSMODE_LEN(0xF)));
...


  /* Set eSPI BRG clock source */


  out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])


          | ESPI_CSMODE_PM(pm) | div16);



  /* Set eSPI mode */


  if (mode & SPI_CPHA)


          out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])


                  | ESPI_CSMODE_CP_BEGIN_EDGCLK);


  if (mode & SPI_CPOL)


          out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])


                  | ESPI_CSMODE_CI_INACTIVEHIGH);



  /* Character bit order: msb first */


  out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])


          | ESPI_CSMODE_REV_MSB_FIRST);



  /* Character length in bits, between 0x3~0xf, i.e. 4bits~16bits */


  out_be32(&espi->csmode[cs], in_be32(&espi->csmode[cs])


          | ESPI_CSMODE_LEN(7));




  ccsr_espi_t *espi = priv->espi;


  unsigned int com = 0;


  size_t data_len = priv->data_len;





  return 0;




  com &= ~(ESPI_COM_CS(0x3) | ESPI_COM_TRANLEN(0xFFFF));


  com |= ESPI_COM_CS(cs);


  com |= ESPI_COM_TRANLEN(data_len - 1);


  out_be32(&espi->com, com);



}
+#endif
-void spi_release_bus(struct spi_slave *slave)
+void spi_cs_deactivate(struct spi_slave *slave)
Should this be static?
This function may be called by the code of other files, so this function should not need to use static to limit it.
...
Regards,
Simon

    