Subject: Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI
 controller
To: Marek Vasut <marek.vasut@gmail.com>,
        linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20170728220707.13960-1-alex.g@adaptrum.com>
 <20170728220707.13960-5-alex.g@adaptrum.com>
 <135fdf95-1029-2d34-2802-1283a73588e5@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Boris Brezillon <boris.brezillon@free-electrons.com>,
        Richard Weinberger <richard@nod.at>,
        Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
        Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
        linux-mtd@lists.infradead.org, devicetree@vger.kernel.org
From: Alexandru Gagniuc <alex.g@adaptrum.com>
Message-ID: <63953f85-795e-20a2-04aa-4d486db608cc@adaptrum.com>
Date: Mon, 31 Jul 2017 13:54:58 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <135fdf95-1029-2d34-2802-1283a73588e5@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jul 2017 20:55:03.2534 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR04MB0840
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1863
Lines: 71

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.

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);
	}
}