Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Marek Vasut <marek.vasut@gmail.com>]

On 12/10/2016 06:34 PM, Cédric Le Goater wrote:
> Hello, 

Hi!

> On 12/10/2016 05:01 AM, Marek Vasut wrote:
>> On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
>>
>> [...]
>>
>>
>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>>> +				    size_t len)
>>> +{
>>> +	if (IS_ALIGNED((u32)src, sizeof(u32)) &&
>>> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
>>> +	    IS_ALIGNED(len, sizeof(u32))) {
>>
>> Did you try compiling this on any 64bit system ?
> 
> I cross compile the kernel on a 64bit host and then upload on the
> target. What kind of problem are you forseeing ? 

Something about the pointer being clipped to 4 bytes, I'd rather use
uintptr_t cast instead of u32 . I don't think it matters for this check,
but just to be extra precise ...

>>
>>> +		while (len > 3) {
>>> +			*(u32 *)buf = readl(src);


[...]

>>> +/*
>>> + * Segment Address Registers. Start and end addresses are encoded
>>> + * using 8MB units
>>> + */
>>> +#define SEGMENT_ADDR_REG0		0x30
>>> +#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
>>
>> is that ((r) & 0xff0000) << 7 ?
>>
>>> +#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
>>
>> ((r) & 0xff000000) >> 1 ?
> 
> yes. 
> 
> I rather keep the initial macros though, which I found easier to 
> understand.
> 
> The Segment Register uses a 8MB unit to encode the start address
> and the end address of the mapping window of a flash SPI slave :
> 
>         | byte 1 | byte 2 | byte 3 | byte 4 |
>         +--------+--------+--------+--------+
>         |  end   |  start |   0    |   0    |

Then the above should be in the comment , it's a good explanation :)

Thanks!

-- 
Best regards,
Marek Vasut