Re: [PATCH 1/2] spi: Add MXIC controller driver

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

Hi Mason,

On Fri, 3 Aug 2018 15:06:34 +0800
masonccyang@xxxxxxxxxxx wrote:

> Hi Trent,
> 
> 
> Trent Piepho <tpiepho@xxxxxxxxxx> 已在 2018/08/03 上午 03:27:45 上寫入: 
> 
> > Trent Piepho <tpiepho@xxxxxxxxxx> 
> > 2018/08/03 上午 03:28
> > 
> > To
> > 
> > "linux-spi@xxxxxxxxxxxxxxx" <linux-spi@xxxxxxxxxxxxxxx>, 
> > "masonccyang@xxxxxxxxxxx" <masonccyang@xxxxxxxxxxx>, 
> > "broonie@xxxxxxxxxx" <broonie@xxxxxxxxxx>, 
> > 
> > cc
> > 
> > "boris.brezillon@xxxxxxxxxxx" <boris.brezillon@xxxxxxxxxxx>, 
> > "zhengxunli@xxxxxxxxxxx" <zhengxunli@xxxxxxxxxxx>, 
> > "juliensu@xxxxxxxxxxx" <juliensu@xxxxxxxxxxx>
> > 
> > Subject
> > 
> > Re: [PATCH 1/2] spi: Add MXIC controller driver
> > 
> > On Thu, 2018-08-02 at 10:07 +0800, masonccyang@xxxxxxxxxxx wrote:  
> > > +
> > > +#define GPIO         0xc4
> > > +#define GPIO_PT(x)      BIT(3 + ((x) * 16))
> > > +#define GPIO_RESET(x)      BIT(2 + ((x) * 16))
> > > +#define GPIO_HOLDB(x)      BIT(1 + ((x) * 16))
> > > +#define GPIO_WPB(x)      BIT((x) * 16)  
> > 
> > Is this one of the device's registers?  Doesn't seem to be used.  In
> > fact, most of the registers that were defined here are not used.  The
> > above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
> > GPIO_RESET(1) is bit 18?
> > There are just two gpios?  Or maybe, since
> > each GPIO has four bits associated with it, that should be x*4 instead
> > of x*16?
> >   
> 
> Actually, our flash host controller supports serial and parallel flash, 
> including NOR and NAND. Therefore, we defined all of device's registers 
> here.
> Up to two flash devices can be controlled by host controller.
> 
> The detail description of GPIO as follow:
> 
> 

You forgot the description :-).

> 
> 
> > > +
> > > +#define HC_VER         0xd0
> > > +
> > > +#define HW_TEST(x)      (0xe0 + ((x) * 4))
> > > +
> > > +struct mxic_spi {
> > > +   struct clk *ps_clk;
> > > +   struct clk *send_clk;
> > > +   struct clk *send_dly_clk;
> > > +   void __iomem *regs;
> > > +   struct {
> > > +      void __iomem *map;
> > > +      dma_addr_t dma;
> > > +   } linear;  
> > 
> > Nothing here uses linear.  
> 
> Our host controller operates in I/O mode, Linear read mode and DMA-slave 
> mode.
> Linear read mode is going to be patched once direct mapping mode is ready 
> in spi-mem layer.

That's true, but those fields are not used yet, so better introduce
them when we actually need them (i.e. when adding support for direct
mapping).

> 
> >   
> > > 
> > > +
> > > +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void   
> *txbuf,
> > > +               void *rxbuf, unsigned int len)
> > > +{
> > > +   unsigned int pos = 0;
> > > +
> > > +   while (pos < len) {
> > > +      unsigned int nbytes = len - pos;
> > > +      u32 data = 0xffffffff;
> > > +      u32 sts;
> > > +      int ret;
> > > +
> > > +      if (nbytes > 4)
> > > +         nbytes = 4;
> > > +
> > > +      if (txbuf)
> > > +         memcpy(&data, txbuf + pos, nbytes);
> > > +
> > > +      ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> > > +                sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> > > +      if (ret)
> > > +         return ret;
> > > +
> > > +      writel(data, mxic->regs + TXD(nbytes % 4));  
> > 
> > This looks strange.  To send one byte of data, you write a 32-bit word
> > to the register at 0x18.  To send two bytes, a 32-bit word is written
> > to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
> > there are four TX data registers, each 32 bits wide, and which register
> > is written to controls how many bytes are sent?
> >   
> 
> yes, this is our host controller design.
> 
> 
> > > 
> > > +
> > > +int mxic_spi_setup(struct spi_device *spi)
> > > +{
> > > +   struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> > > +   unsigned long freq = spi->max_speed_hz;
> > > +   int ret;
> > > +
> > > +   ret = clk_set_rate(mxic->send_clk, freq);
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   ret = clk_set_rate(mxic->send_dly_clk, freq);
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   mxic_spi_set_input_delay_dqs(mxic, 0xf);
> > > +   ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 /   
> freq));
> > 
> > Can also be written as freq*9/25000000.
> >   
> > > +   if (ret)
> > > +      return ret;
> > > +
> > > +   clk_prepare_enable(mxic->send_clk);
> > > +   clk_prepare_enable(mxic->send_dly_clk);
> > > +
> > > +   return 0;  
> > 
> > Won't changing the clock rate in spi_setup break if there are two
> > slaves which use different rates?
> >   
> > >   
> 
> SCLK is connected to two flash devices in our design.

Well, this is board dependent, and we should support all cases even if
your specific board design use the same setup for both CS lines.
Trent's point is that we should change the frequency (if needed) when
doing a transfer not in ->spi_setup().

Regards,

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html