Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller

[Marek Vasut]

On 07/31/2017 10:54 PM, Alexandru Gagniuc wrote:
> Hi Marek,
> 
> Me again!
> 
> On 07/29/2017 02:34 AM, Marek Vasut wrote:
>> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf,
>>> size_t len)
>>> +{
>>> +    uint32_t data;
>>
>> Is this stuff below something like ioread32_rep() ?
>>
> [snip]
>>> +    aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>> +    while (len >= 4) {
>>> +        memcpy(&data, buf, sizeof(data));
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>
>> iowrite32_rep ?
>>
>>> +        buf += 4;
>>> +        len -= 4;
>>> +    }
> 
> I looked at using io(read|write)32_rep in these two places, and I've run
> into some issues.
> 
> First, I'm seeing unaligned MMIO accesses, which are not supported on
> ARC. Note that 'buf' has an alignment of 1, while the register requires
> an alignment of 4. The memcpy() in-between takes care of that, which was
> the original intent.
> 
> Other than that, we still need to break off the tail because we need to
> update ASPI_REG_BYTE_COUNT before writing/reading any more data from the
> FIFO. We have to keep track of the remainder, so we're not really saving
> any SLOC.
> 
> I'd like to keep the original version as I find it to be much more
> symmetrical and readable.

Look at the other drivers, AFAIR the io*_rep() issue was solved over and
over again, maybe you can factor that code out.

> Thanks,
> Alex
> 
>>> +
>>> +    if (len) {
>>> +        aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>>> +        memcpy(&data, buf, len);
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>> +    }
>>> +}
> 
> 
> Exhibit A: aspi_seed_fifo with writesl()
> 
> static void aspi_seed_fifo(struct anarion_qspi *spi,
>                const uint8_t *buf, size_t len)
> {
>     uint32_t data;
>     void __iomem *data_reg = (void *)(spi->regbase + ASPI_REG_DATA1);
> 
>     aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>     writesl(data_reg, buf, len / 4);
>     buf += len & ~0x03;
>     len &= 0x03;
> 
>     if (len) {
>         aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>         memcpy(&data, buf, len);
>         aspi_write_reg(spi, ASPI_REG_DATA1, data);
>     }
> }


-- 
Best regards,
Marek Vasut