Date:   Fri, 7 Sep 2018 22:31:27 +0200
From:   Marek Vasut <marek.vasut@...il.com>
To:     Tudor Ambarus <tudor.ambarus@...rochip.com>,
        cyrille.pitchen@...rochip.com, dwmw2@...radead.org,
        computersforpeace@...il.com, boris.brezillon@...tlin.com,
        richard@....at
Cc:     linux-mtd@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        nicolas.ferre@...rochip.com, Cristian.Birsan@...rochip.com
Subject: Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform
 SFDP SPI NOR flash memories

On 09/07/2018 10:51 AM, Tudor Ambarus wrote:
> Thanks Marek,
> 
> On 09/03/2018 08:37 PM, Marek Vasut wrote:
>> On 08/27/2018 12:26 PM, Tudor Ambarus wrote:
>> [...]
>>
>>> +/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
>>> +static inline u64
>>> +spi_nor_div_by_erase_size(const struct spi_nor_erase_type *erase,
>>> +			  u64 dividend, u32 *remainder)
>>> +{
>>> +	*remainder = (u32)dividend & erase->size_mask;
>>
>> Is the cast really needed ? btw I think there might be a macro doing
>> just this, div_by_ or something in include/ .
> 
> The cast is not needed, the AND sets to zero all but the low-order 32bits of
> divided and then we have the implicit cast.
> 
> Are you referring to do_div()? I expect the bitwise operations to be faster.
> Bitwise operations are preferred in include/linux/mtd/mtd.h too:

I thought there was some macro to do this bitwise modulo operation
already , so you don't have to implement it here. I don't mean do_div, no.

> static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
> {
>         if (mtd->erasesize_shift)
>                 return sz >> mtd->erasesize_shift;
>         do_div(sz, mtd->erasesize);
>         return sz;
> }
> 
>>
>>> +	return dividend >> erase->size_shift;
>>> +}
>>> +
>>> +static const struct spi_nor_erase_type *
>>> +spi_nor_find_best_erase_type(const struct spi_nor_erase_map *map,
>>> +			     const struct spi_nor_erase_region *region,
>>> +			     u64 addr, u32 len)
>>> +{
>>> +	const struct spi_nor_erase_type *erase;
>>> +	u32 rem;
>>> +	int i;
>>> +	u8 erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> +	/*
>>> +	 * Erase types are ordered by size, with the biggest erase type at
>>> +	 * index 0.
>>> +	 */
>>> +	for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
>>> +		/* Does the erase region support the tested erase type? */
>>> +		if (!(erase_mask & BIT(i)))
>>> +			continue;
>>> +
>>> +		erase = &map->erase_type[i];
>>> +
>>> +		/* Don't erase more than what the user has asked for. */
>>> +		if (erase->size > len)
>>> +			continue;
>>> +
>>> +		/* Alignment is not mandatory for overlaid regions */
>>> +		if (region->offset & SNOR_OVERLAID_REGION)
>>> +			return erase;
>>> +
>>> +		spi_nor_div_by_erase_size(erase, addr, &rem);
>>> +		if (rem)
>>> +			continue;
>>> +		else
>>> +			return erase;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static struct spi_nor_erase_region *
>>> +spi_nor_region_next(struct spi_nor_erase_region *region)
>>> +{
>>> +	if (spi_nor_region_is_last(region))
>>> +		return NULL;
>>> +	return ++region;
>>
>> region++ ...
> 
> It's an array of regions, consecutive in address space, in which walking is done
> incrementally. If the received region is not the last, I want to return the next
> region, so ++region is correct.

return i++ is the same as return ++i;

>> [...]
>>
>>> +static int spi_nor_cmp_erase_type(const void *a, const void *b)
>>> +{
>>> +	const struct spi_nor_erase_type *erase1 = a;
>>> +	const struct spi_nor_erase_type *erase2 = b;
>>> +
>>> +	return erase1->size - erase2->size;
>>
>> What does this function do again ?
> 
> It's a compare function, I compare by size the map's Erase Types. I pass a
> pointer to this function in the sort() call. I sort in ascending order, by size,
> all the map's Erase Types when parsing bfpt. I'm doing the sort at init to speed
> up the finding of the best erase command at run-time.
> 
> A better name for this function is spi_nor_map_cmp_erase_type(), we compare the
> map's Erase Types by size.

More like a description would be most welcome, in the actual code. And I
really dislike how you fiddle around with void pointers, can't that be
fixed?

>>> +}
>>> +
>>> +static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>>> +{
>>> +	struct spi_nor_erase_region *region = map->regions;
>>> +	struct spi_nor_erase_type *erase_type = map->erase_type;
>>> +	int i;
>>> +	u8 region_erase_mask, ordered_erase_mask;
>>> +
>>> +	/*
>>> +	 * Sort each region's Erase Types in ascending order with the smallest
>>> +	 * Erase Type size starting at BIT(0).
>>> +	 */
>>> +	while (region) {
>>> +		region_erase_mask = region->offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> +		/*
>>> +		 * The region's erase mask indicates which erase types are
>>> +		 * supported from the erase types defined in the map.
>>> +		 */
>>> +		ordered_erase_mask = 0;
>>> +		for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>>> +			if (erase_type[i].size &&
>>> +			    region_erase_mask & BIT(erase_type[i].idx))
>>> +				ordered_erase_mask |= BIT(i);
>>> +
>>> +		/* Overwrite erase mask. */
>>> +		region->offset = (region->offset & ~SNOR_ERASE_TYPE_MASK) |
>>> +				 ordered_erase_mask;
>>> +
>>> +		region = spi_nor_region_next(region);
>>> +	}
>>> +}
>>> +
>>> +static inline void
>>
>> Drop the inline
> 
> Ok.
> 
>>
>>> +spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>>> +			       u8 erase_mask, u64 flash_size)
>>> +{
>>> +	map->uniform_region.offset = SNOR_ERASE_FLAGS_OFFSET(erase_mask, 1, 0,
>>> +							     0);
>>> +	map->uniform_region.size = flash_size;
>>> +	map->regions = &map->uniform_region;
>>> +	map->uniform_erase_type = erase_mask;
>>> +}
>>> +
>>
>> [...]
>>
>>> +#define spi_nor_region_is_last(region)  (region->offset & SNOR_LAST_REGION)
>>> +
>>> +static inline u64 spi_nor_region_end(const struct spi_nor_erase_region *region)
>>
>> Get rid of the inlines, really.
> 
> Agreed.
> 
>>
>>> +{
>>> +	return (region->offset & ~SNOR_ERASE_FLAGS_MASK) + region->size;
>>> +}
>>> +
>>> +static inline bool spi_nor_has_uniform_erase(const struct spi_nor *nor)
>>> +{
>>> +	return !!nor->erase_map.uniform_erase_type;
>>> +}
>>> +
>>>  static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>>  					  struct device_node *np)
>>>  {
>>>
>>
>> General question, what happens if the multi-block erase fails mid-way ,
>> is that handled or reported somehow to the user ?
> 
> I already implemented your suggestion. I build a list of erase commands to be
> executed once I validate that the erase can be performed.

You can send them, but what happens if it fails mid-way ? Is the user
somehow notified that part of his flash is empty and part is not ?

-- 
Best regards,
Marek Vasut

Powered by blists - more mailing lists