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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

Hi Trent,

On Thu, 2 Aug 2018 19:27:45 +0000
Trent Piepho <tpiepho@xxxxxxxxxx> wrote:

> 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?

If by device you mean SPI controller, then yes it is.

> 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?

It is correct. We have 2 sets of 4 pins (one set of pin per CS line),
so x is 0 or 1.

> There are just two gpios?

Nope 2 sets of GPIO and each set contains 4 GPIOs.

> Or maybe, since
> each GPIO has four bits associated with it, that should be x*4 instead
> of x*16?

Nope, the current GPIO_XXX defs are correct.

> 
> > +
> > +#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.

Yep, we'll drop it.

> 
> > 
> > +
> > +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?

As pointed by Mason, it's not a mistake, the controller really work
like that.

> 
> > 
> > +
> > +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.

I'd like to keep 360 here. The phase is in degree, hence the 360. The
compiler should be smart enough to optimize that at compilation
time ;-).

> 
> > +	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?

Correct. We'll calculate the expected phase and clk rate, keep an
internal cache in the spi controller struct and change it if needed
when a transfer is done or a spi_mem_op is executed.

Thanks for your review.

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