Re: [PATCHv7 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar <sourav.poddar@ti.com>]

On Wednesday 31 July 2013 02:50 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Jul 31, 2013 at 02:40:51PM +0530, Sourav Poddar wrote:
>>>> +#define	QSPI_FRAME_MAX			0xfff
>>> Frame max is 4096, 0x1000, right ?
>> Yes,
>> this macro was used initially to fill the register bits, where 4095 =
>> 4096 words.
>> Will change it to now.
> you can make this something like:
>
> #define QSPI_FRAME(n)		(((n) - 1)&  0xfff)
>
Yes, but now its only used in a clamp function, where I should
provide the exact value 4096. Will use your previous suggestion.
#define    QSPI_FRAME_MAX            0x1000
>>>> +static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>>>> +{
>>>> +	const u8 *txbuf;
>>>> +	int wlen, count, ret;
>>>> +
>>>> +	count = t->len;
>>>> +	txbuf = t->tx_buf;
>>>> +	wlen = t->bits_per_word;
>>>> +
>>>> +	while (count--) {
>>> you're decrementing count by one, but in some cases you write 4 bytes or
>>> 2 bytes... This will blow up very soon. I can already see overflows
>>> happening...
>> we write 2 bytes and 4 bytes for 16 bits_per_word and 32 bits_per_word case.
>> count is t->len, which is the total number of words to transfer. This
> t->len is total number of bytes as you can see from the documentation in
> the header:
>
> * @len: size of rx and tx buffers (in bytes)
>
> As I said before, please read the documentation.
>
>> words can be of any length (1, 2 or 4) bytes. So, I think it should be
>> decremented by 1 only.
> this is wrong.
>
hmm..got the point.
I will pass the count address also to ti_qspi_read_data/write_data and make
use of the switch statement to decrement the count.
>>>> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (t->tx_buf) {
>>>> +		ret = qspi_write_msg(qspi, t);
>>>> +		if (ret) {
>>>> +			dev_dbg(qspi->dev, "Error while writing\n");
>>>> +			return -EINVAL;
>>> why do you change the return value from qspi_write_msg() ?
>>>
>> I  was not sure about this, I thought I had signals an ETIMEOUT during
>> timeout, So signal a invalid transfer here.
>> Do you suggest keeping ETIMEOUT here also?
> yeah, so we tell whoever called us that the transfer timed out. If you
> return -EINVAL you're telling your clients they gave you an invalid
> spi_transfer, which is not the case.
>
Ok.
>>>> +static int ti_qspi_start_transfer_one(struct spi_master *master,
>>>> +		struct spi_message *m)
>>>> +{
>>>> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
>>>> +	struct spi_device *spi = m->spi;
>>>> +	struct spi_transfer *t;
>>>> +	int status = 0, ret;
>>>> +	int frame_length;
>>>> +
>>>> +	/* setup device control reg */
>>>> +	qspi->dc = 0;
>>>> +
>>>> +	if (spi->mode&   SPI_CPHA)
>>>> +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
>>>> +	if (spi->mode&   SPI_CPOL)
>>>> +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
>>>> +	if (spi->mode&   SPI_CS_HIGH)
>>>> +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
>>>> +
>>>> +	frame_length = (m->frame_length<<   3) / spi->bits_per_word;
>>>> +
>>>> +	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
>>>> +
>>>> +	/* setup command reg */
>>>> +	qspi->cmd = 0;
>>>> +	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>>>> +	qspi->cmd |= QSPI_FLEN(frame_length);
>>>> +	qspi->cmd |= QSPI_WC_CMD_INT_EN;
>>>> +
>>>> +	ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>>>> +
>>>> +	list_for_each_entry(t,&m->transfers, transfer_list) {
>>> no locking around list traversal ?
>>>
>> hmm..can put a spin_lock around "qspi_transfer_msg" ?
> no dude, you need to protect the access to the list. So it needs to be
> around list_for_each_entry().
>
Ok.
>>>> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
>>>> +{
>>>> +	struct ti_qspi *qspi = dev_id;
>>>> +	u16 stat;
>>>> +
>>>> +	irqreturn_t ret = IRQ_HANDLED;
>>>> +
>>>> +	spin_lock(&qspi->lock);
>>>> +
>>>> +	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
>>>> +
>>>> +	if (!stat) {
>>>> +		dev_dbg(qspi->dev, "No IRQ triggered\n");
>>>> +		return IRQ_NONE;
>>> leaving lock held.
>>>
>> Will add a unlock before returning.
> there's a very nice C statement, goto, which you can use here.
>
Ok.
>>>> +static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id)
>>>> +{
>>>> +	struct ti_qspi *qspi = dev_id;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&qspi->lock, flags);
>>>> +
>>>> +	complete(&qspi->transfer_complete);
>>> you need to check if your word completion is actually set. Don't assume
>>> it's set because we might want to change the code later.
>>>
>> hmm..something like this.?
>>    if (ti_qspi_read(qspi, QSPI_SPI_STATUS_REG)&  WC)
>>          complete(&qspi->transfer_complete);
> I rather:
>
> stat = ti_qspi_read(qspi, QSPI_SPI_STATUS_REG);
>
> if (stat&  WC)
> 	complete()
>
> then, if we want to add frame interrupt handling later, we don't need to
> read status register again. In fact, to avoid reading the status
> register here, you could even cache the returned value you read in your
> hardirq handler inside your qspi struct.
>
Ok.