RE: [EXTERNAL] Re: [PATCH v8 4/4] spi: cadence: Add MRVL overlay xfer operation support

[Witold Sadowski]

Hi

> 
> > +static int cdns_xspi_prepare_generic(int cs, const void *dout, int
> > +len, int glue, u32 *cmd_regs) {
> > +	u8 *data = (u8 *)dout;
> > +	int i;
> > +	int data_counter = 0;
> > +
> > +	memset(cmd_regs, 0x00, 6*4);
> 
> The magic numbers here aren't great...

That will be fixed.

> 
> > +static unsigned char reverse_bits(unsigned char num) {
> > +	unsigned int count = sizeof(num) * 8 - 1;
> > +	unsigned int reverse_num = num;
> > +
> > +	num >>= 1;
> > +	while (num) {
> > +		reverse_num <<= 1;
> > +		reverse_num |= num & 1;
> > +		num >>= 1;
> > +		count--;
> > +	}
> > +	reverse_num <<= count;
> > +	return reverse_num;
> > +}
> 
> I can't help but think there ought to be a helper for this though I can't
> think what it is off the top of my head.  If there isn't it probably makes
> sense to add this as one.
> 
> > +	/* Enable xfer state machine */
> > +	if (!cdns_xspi->xfer_in_progress) {
> > +		u32 xfer_control = readl(cdns_xspi->xferbase +
> > +MRVL_XFER_FUNC_CTRL);
> > +
> > +		cdns_xspi->current_xfer_qword = 0;
> > +		cdns_xspi->xfer_in_progress = true;
> > +		xfer_control |= (MRVL_XFER_RECEIVE_ENABLE |
> > +				 MRVL_XFER_CLK_CAPTURE_POL |
> > +				 MRVL_XFER_FUNC_START |
> > +				 MRVL_XFER_SOFT_RESET |
> > +				 FIELD_PREP(MRVL_XFER_CS_N_HOLD, (1 << cs)));
> > +		xfer_control &= ~(MRVL_XFER_FUNC_ENABLE |
> MRVL_XFER_CLK_DRIVE_POL);
> > +		writel(xfer_control, cdns_xspi->xferbase +
> MRVL_XFER_FUNC_CTRL);
> > +	}
> 
> Could this just be a prepare_transfer_hardware() and we could just use
> transfer_one()?

I have to run some experiments, but it should be possible.

> 
> > +	list_for_each_entry(t, &m->transfers, transfer_list) {
> > +		u8 *txd = (u8 *) t->tx_buf;
> > +		u8 *rxd = (u8 *) t->rx_buf;
> > +		u8 data[10];
> > +		u32 cmd_regs[6];
> > +
> > +		if (!txd)
> > +			txd = data;
> > +
> > +		cdns_xspi->in_buffer = txd + 1;
> > +		cdns_xspi->out_buffer = txd + 1;
> 
> Oh?

You are asking about that 1 byte offset? It is caused by way that
SDMA is handled in that specific case - all data except of first
byte is transferred via SDMA, the first byte is send in command, and
SDMA is not involved in that.

Regards
Witek