subject:"\[U\-Boot\] \[PATCH v3 3\/3\] spi\: mxs

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Akash Gajjar

Hello Marek,

On Fri, May 11, 2018 at 4:09 PM, Marek Vasut  wrote:

> On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> > Hi Marek,
> >
> > Thanks for the review comments.
> >
> > > -struct mxs_spi_slave {
> > > - struct spi_slaveslave;
> > > - uint32_tmax_khz;
> > > - uint32_tmode;
> > > - struct mxs_ssp_regs *regs;
> > > +struct mxs_spi_priv {
> > > + struct mxs_ssp_regs *regs;
> > > + u32 max_khz;
> > > + u32 mode;
> > > + u32 bus;
> > > + u32 cs;
> >
> > Type cleanup should be a separate patch
> >
> >
> > Okay, I will prepare seperate patch for type cleanup.
> >
> >
> > >  };
> > >   if (mxs_wait_mask_set(&ssp_regs->hw_ssp_ctrl0_reg,
> > >   SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > > - printf("MXS SPI: Timeout waiting for
> start\n");
> > > + debug("MXS SPI: Timeout waiting for
> start\n");
> >
> > printf , we don't want to hide errors
> >
> >
> > okay, will revert it back to printf.
> >
> >
> > >   return -ETIMEDOUT;
> > >   }
> >
> > > +
> > > +#ifndef __SPI_MXS_H
> > > +#define __SPI_MXS_H
> > > +
> > > +struct mxs_spi_platdata {
> > > + struct mxs_ssp_regs *regs;
> > > + u32 bus;
> > > + u32 max_hz;
> > > + u32 cs;
> >
> > Why is this header here at all ?
> >
> >
> > I didnt get this comment. do I need to place it somewhere else?
>
> See the beginning of this email, it seems the same structure exists twice.
>

My intention was to have two individual structure for private and platform
data.
But now I could use one structre and access its members using two structure
variables(one for private and one for platadata).

Is That looks okay?


>
> --
> Best regards,
> Marek Vasut
>


Thanks,

*Akash Gajjar*
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Marek Vasut

On 05/11/2018 01:09 PM, Akash Gajjar wrote:
> Hello Marek,
> 
> On Fri, May 11, 2018 at 4:09 PM, Marek Vasut  > wrote:
> 
> On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> > Hi Marek,
> >
> > Thanks for the review comments.
> >
> >     > -struct mxs_spi_slave {
> >     > -     struct spi_slave        slave;
> >     > -     uint32_t                max_khz;
> >     > -     uint32_t                mode;
> >     > -     struct mxs_ssp_regs     *regs;
> >     > +struct mxs_spi_priv {
> >     > +     struct mxs_ssp_regs *regs;
> >     > +     u32     max_khz;
> >     > +     u32     mode;
> >     > +     u32     bus;
> >     > +     u32     cs;
> >
> >     Type cleanup should be a separate patch
> >
> >  
> > Okay, I will prepare seperate patch for type cleanup.
> >
> >
> >     >  };
> >     >               if (mxs_wait_mask_set(&ssp_regs->hw_ssp_ctrl0_reg,
> >     >                       SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> >     > -                     printf("MXS SPI: Timeout waiting for
> start\n");
> >     > +                     debug("MXS SPI: Timeout waiting for
> start\n");
> >
> >     printf , we don't want to hide errors
> >
> >  
> > okay, will revert it back to printf.
> >
> >
> >     >                       return -ETIMEDOUT;
> >     >               }
> >
> >     > +
> >     > +#ifndef __SPI_MXS_H
> >     > +#define __SPI_MXS_H
> >     > +
> >     > +struct mxs_spi_platdata {
> >     > +     struct mxs_ssp_regs *regs;
> >     > +     u32 bus;
> >     > +     u32 max_hz;
> >     > +     u32 cs;
> >
> >     Why is this header here at all ?
> >
> >  
> > I didnt get this comment. do I need to place it somewhere else?
> 
> See the beginning of this email, it seems the same structure exists
> twice.
> 
> 
> My intention was to have two individual structure for private and
> platform data.
> But now I could use one structre and access its members using two
> structure variables(one for private and one for platadata).
> 
> Is That looks okay?

I do not quite understand what you mean, but there is a duplication of
information here. That's a problem and should be fixed.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Marek Vasut

On 05/11/2018 12:08 PM, Gajjar Akash wrote:
> Hi Marek,
> 
> Thanks for the review comments.
> 
> > -struct mxs_spi_slave {
> > -     struct spi_slave        slave;
> > -     uint32_t                max_khz;
> > -     uint32_t                mode;
> > -     struct mxs_ssp_regs     *regs;
> > +struct mxs_spi_priv {
> > +     struct mxs_ssp_regs *regs;
> > +     u32     max_khz;
> > +     u32     mode;
> > +     u32     bus;
> > +     u32     cs;
> 
> Type cleanup should be a separate patch
> 
>  
> Okay, I will prepare seperate patch for type cleanup.
> 
> 
> >  };
> >               if (mxs_wait_mask_set(&ssp_regs->hw_ssp_ctrl0_reg,
> >                       SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > -                     printf("MXS SPI: Timeout waiting for start\n");
> > +                     debug("MXS SPI: Timeout waiting for start\n");
> 
> printf , we don't want to hide errors
> 
>  
> okay, will revert it back to printf.
> 
> 
> >                       return -ETIMEDOUT;
> >               }
> 
> > +
> > +#ifndef __SPI_MXS_H
> > +#define __SPI_MXS_H
> > +
> > +struct mxs_spi_platdata {
> > +     struct mxs_ssp_regs *regs;
> > +     u32 bus;
> > +     u32 max_hz;
> > +     u32 cs;
> 
> Why is this header here at all ?
> 
>  
> I didnt get this comment. do I need to place it somewhere else?

See the beginning of this email, it seems the same structure exists twice.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-11 Thread Gajjar Akash

Hi Marek,

Thanks for the review comments.

> -struct mxs_spi_slave {
> > - struct spi_slaveslave;
> > - uint32_tmax_khz;
> > - uint32_tmode;
> > - struct mxs_ssp_regs *regs;
> > +struct mxs_spi_priv {
> > + struct mxs_ssp_regs *regs;
> > + u32 max_khz;
> > + u32 mode;
> > + u32 bus;
> > + u32 cs;
>
> Type cleanup should be a separate patch
>

Okay, I will prepare seperate patch for type cleanup.


> >  };
> >   if (mxs_wait_mask_set(&ssp_regs->hw_ssp_ctrl0_reg,
> >   SSP_CTRL0_RUN, MXS_SPI_MAX_TIMEOUT)) {
> > - printf("MXS SPI: Timeout waiting for start\n");
> > + debug("MXS SPI: Timeout waiting for start\n");
>
> printf , we don't want to hide errors
>

okay, will revert it back to printf.


> >   return -ETIMEDOUT;
> >   }

> +
> > +#ifndef __SPI_MXS_H
> > +#define __SPI_MXS_H
> > +
> > +struct mxs_spi_platdata {
> > + struct mxs_ssp_regs *regs;
> > + u32 bus;
> > + u32 max_hz;
> > + u32 cs;
>
> Why is this header here at all ?
>

I didnt get this comment. do I need to place it somewhere else?

> +};
> > +
> > +#endif /* __SPI_MXS_H */
> >
>
>
> --
> Best regards,
> Marek Vasut
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

Thanks,
Akash
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-10 Thread Akash Gajjar

This patch adds support for DM driver model to the mxs spi driver. Some TODOs
are left over for later. These would be enhancements to the original
functionality, and can come later. The legacy functionality is removed in
this version.

Signed-off-by: Akash Gajjar 
---
changes in v2:
 Register cs_info method 
 Remove unused function __mxs_spi_setup
 Merged __spi_xfer function to spi_xfer
 printf replaced by debug

changes in v3:
 changes made on top of previous patch is merged in v3
---
 drivers/spi/Kconfig|  12 +-
 drivers/spi/mxs_spi.c  | 258 +++--
 include/dm/platform_data/spi_mxs.h |  18 +++
 3 files changed, 183 insertions(+), 105 deletions(-)
 create mode 100644 include/dm/platform_data/spi_mxs.h

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ec92b84..5d3e152 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -106,6 +106,12 @@ config MVEBU_A3700_SPI
  used to access the SPI NOR flash on platforms embedding this
  Marvell IP core.
 
+config MXS_SPI
+   bool "MXS SPI Driver"
+   help
+ Enable the MXS SPI controller driver. This driver can be used
+ on the i.MX23 and i.MX28 SoCs.
+
 config PIC32_SPI
bool "Microchip PIC32 SPI driver"
depends on MACH_PIC32
@@ -299,12 +305,6 @@ config MXC_SPI
  Enable the MXC SPI controller driver. This driver can be used
  on various i.MX SoCs such as i.MX31/35/51/6/7.
 
-config MXS_SPI
-   bool "MXS SPI Driver"
-   help
- Enable the MXS SPI controller driver. This driver can be used
- on the i.MX23 and i.MX28 SoCs.
-
 config OMAP3_SPI
bool "McSPI driver for OMAP"
help
diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
index 790db78..3054438 100644
--- a/drivers/spi/mxs_spi.c
+++ b/drivers/spi/mxs_spi.c
@@ -1,6 +1,9 @@
 /*
  * Freescale i.MX28 SPI driver
  *
+ * Support for device model:
+ * Copyright (C) 2018 Akash Gajjar 
+ *
  * Copyright (C) 2011 Marek Vasut 
  * on behalf of DENX Software Engineering GmbH
  *
@@ -20,6 +23,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #defineMXS_SPI_MAX_TIMEOUT 100
 #defineMXS_SPI_PORT_OFFSET 0x2000
@@ -28,93 +33,14 @@
 
 #define MXSSSP_SMALL_TRANSFER  512
 
-struct mxs_spi_slave {
-   struct spi_slaveslave;
-   uint32_tmax_khz;
-   uint32_tmode;
-   struct mxs_ssp_regs *regs;
+struct mxs_spi_priv {
+   struct mxs_ssp_regs *regs;
+   u32 max_khz;
+   u32 mode;
+   u32 bus;
+   u32 cs;
 };
 
-static inline struct mxs_spi_slave *to_mxs_slave(struct spi_slave *slave)
-{
-   return container_of(slave, struct mxs_spi_slave, slave);
-}
-
-void spi_init(void)
-{
-}
-
-int spi_cs_is_valid(unsigned int bus, unsigned int cs)
-{
-   /* MXS SPI: 4 ports and 3 chip selects maximum */
-   if (!mxs_ssp_bus_id_valid(bus) || cs > 2)
-   return 0;
-   else
-   return 1;
-}
-
-struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
- unsigned int max_hz, unsigned int mode)
-{
-   struct mxs_spi_slave *mxs_slave;
-
-   if (!spi_cs_is_valid(bus, cs)) {
-   printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
-   return NULL;
-   }
-
-   mxs_slave = spi_alloc_slave(struct mxs_spi_slave, bus, cs);
-   if (!mxs_slave)
-   return NULL;
-
-   if (mxs_dma_init_channel(MXS_DMA_CHANNEL_AHB_APBH_SSP0 + bus))
-   goto err_init;
-
-   mxs_slave->max_khz = max_hz / 1000;
-   mxs_slave->mode = mode;
-   mxs_slave->regs = mxs_ssp_regs_by_bus(bus);
-
-   return &mxs_slave->slave;
-
-err_init:
-   free(mxs_slave);
-   return NULL;
-}
-
-void spi_free_slave(struct spi_slave *slave)
-{
-   struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
-   free(mxs_slave);
-}
-
-int spi_claim_bus(struct spi_slave *slave)
-{
-   struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
-   struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
-   uint32_t reg = 0;
-
-   mxs_reset_block(&ssp_regs->hw_ssp_ctrl0_reg);
-
-   writel((slave->cs << MXS_SSP_CHIPSELECT_SHIFT) |
-  SSP_CTRL0_BUS_WIDTH_ONE_BIT,
-  &ssp_regs->hw_ssp_ctrl0);
-
-   reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
-   reg |= (mxs_slave->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
-   reg |= (mxs_slave->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
-   writel(reg, &ssp_regs->hw_ssp_ctrl1);
-
-   writel(0, &ssp_regs->hw_ssp_cmd0);
-
-   mxs_set_ssp_busclock(slave->bus, mxs_slave->max_khz);
-
-   return 0;
-}
-
-void spi_release_bus(struct spi_slave *slave)
-{
-}
-
 static void mxs_spi_start_xfer(struct mxs_ssp_regs *ssp_regs)
 {
writel(SSP_CTRL0_LOCK_CS, &ssp_regs->hw_ssp_ctrl0_set);
@@ -127,10 +53,10

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

2018-05-10 Thread Marek Vasut

On 05/10/2018 04:17 PM, Akash Gajjar wrote:
> This patch adds support for DM driver model to the mxs spi driver. Some TODOs
> are left over for later. These would be enhancements to the original
> functionality, and can come later. The legacy functionality is removed in
> this version.
> 
> Signed-off-by: Akash Gajjar 
> ---
> changes in v2:
>  Register cs_info method 
>  Remove unused function __mxs_spi_setup
>  Merged __spi_xfer function to spi_xfer
>  printf replaced by debug
> 
> changes in v3:
>  changes made on top of previous patch is merged in v3
> ---
>  drivers/spi/Kconfig|  12 +-
>  drivers/spi/mxs_spi.c  | 258 
> +++--
>  include/dm/platform_data/spi_mxs.h |  18 +++
>  3 files changed, 183 insertions(+), 105 deletions(-)
>  create mode 100644 include/dm/platform_data/spi_mxs.h
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ec92b84..5d3e152 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -106,6 +106,12 @@ config MVEBU_A3700_SPI
> used to access the SPI NOR flash on platforms embedding this
> Marvell IP core.
>  
> +config MXS_SPI
> + bool "MXS SPI Driver"
> + help
> +   Enable the MXS SPI controller driver. This driver can be used
> +   on the i.MX23 and i.MX28 SoCs.
> +
>  config PIC32_SPI
>   bool "Microchip PIC32 SPI driver"
>   depends on MACH_PIC32
> @@ -299,12 +305,6 @@ config MXC_SPI
> Enable the MXC SPI controller driver. This driver can be used
> on various i.MX SoCs such as i.MX31/35/51/6/7.
>  
> -config MXS_SPI
> - bool "MXS SPI Driver"
> - help
> -   Enable the MXS SPI controller driver. This driver can be used
> -   on the i.MX23 and i.MX28 SoCs.
> -
>  config OMAP3_SPI
>   bool "McSPI driver for OMAP"
>   help
> diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
> index 790db78..3054438 100644
> --- a/drivers/spi/mxs_spi.c
> +++ b/drivers/spi/mxs_spi.c
> @@ -1,6 +1,9 @@
>  /*
>   * Freescale i.MX28 SPI driver
>   *
> + * Support for device model:
> + * Copyright (C) 2018 Akash Gajjar 
> + *
>   * Copyright (C) 2011 Marek Vasut 
>   * on behalf of DENX Software Engineering GmbH
>   *
> @@ -20,6 +23,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #define  MXS_SPI_MAX_TIMEOUT 100
>  #define  MXS_SPI_PORT_OFFSET 0x2000
> @@ -28,93 +33,14 @@
>  
>  #define MXSSSP_SMALL_TRANSFER512
>  
> -struct mxs_spi_slave {
> - struct spi_slaveslave;
> - uint32_tmax_khz;
> - uint32_tmode;
> - struct mxs_ssp_regs *regs;
> +struct mxs_spi_priv {
> + struct mxs_ssp_regs *regs;
> + u32 max_khz;
> + u32 mode;
> + u32 bus;
> + u32 cs;

Type cleanup should be a separate patch

>  };
>  
> -static inline struct mxs_spi_slave *to_mxs_slave(struct spi_slave *slave)
> -{
> - return container_of(slave, struct mxs_spi_slave, slave);
> -}
> -
> -void spi_init(void)
> -{
> -}
> -
> -int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> -{
> - /* MXS SPI: 4 ports and 3 chip selects maximum */
> - if (!mxs_ssp_bus_id_valid(bus) || cs > 2)
> - return 0;
> - else
> - return 1;
> -}
> -
> -struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> -   unsigned int max_hz, unsigned int mode)
> -{
> - struct mxs_spi_slave *mxs_slave;
> -
> - if (!spi_cs_is_valid(bus, cs)) {
> - printf("mxs_spi: invalid bus %d / chip select %d\n", bus, cs);
> - return NULL;
> - }
> -
> - mxs_slave = spi_alloc_slave(struct mxs_spi_slave, bus, cs);
> - if (!mxs_slave)
> - return NULL;
> -
> - if (mxs_dma_init_channel(MXS_DMA_CHANNEL_AHB_APBH_SSP0 + bus))
> - goto err_init;
> -
> - mxs_slave->max_khz = max_hz / 1000;
> - mxs_slave->mode = mode;
> - mxs_slave->regs = mxs_ssp_regs_by_bus(bus);
> -
> - return &mxs_slave->slave;
> -
> -err_init:
> - free(mxs_slave);
> - return NULL;
> -}
> -
> -void spi_free_slave(struct spi_slave *slave)
> -{
> - struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
> - free(mxs_slave);
> -}
> -
> -int spi_claim_bus(struct spi_slave *slave)
> -{
> - struct mxs_spi_slave *mxs_slave = to_mxs_slave(slave);
> - struct mxs_ssp_regs *ssp_regs = mxs_slave->regs;
> - uint32_t reg = 0;
> -
> - mxs_reset_block(&ssp_regs->hw_ssp_ctrl0_reg);
> -
> - writel((slave->cs << MXS_SSP_CHIPSELECT_SHIFT) |
> -SSP_CTRL0_BUS_WIDTH_ONE_BIT,
> -&ssp_regs->hw_ssp_ctrl0);
> -
> - reg = SSP_CTRL1_SSP_MODE_SPI | SSP_CTRL1_WORD_LENGTH_EIGHT_BITS;
> - reg |= (mxs_slave->mode & SPI_CPOL) ? SSP_CTRL1_POLARITY : 0;
> - reg |= (mxs_slave->mode & SPI_CPHA) ? SSP_CTRL1_PHASE : 0;
> - writel(reg, &ssp_regs->hw_ssp_ctrl1);
> -
> - writel(0, &ssp_regs->hw_ssp_cmd0);
>

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

[U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

Re: [U-Boot] [PATCH v3 3/3] spi: mxs_spi: DM conversion

6 matches

Site Navigation

Mail list logo

Footer information