Re: [RESEND PATCH v2 1/3] mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories

[Marek Vasut]

On 09/10/2018 12:18 PM, Tudor Ambarus wrote:
> Marek,

Hi,

> On 09/07/2018 11:31 PM, Marek Vasut wrote:
>> 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
> 
> I will describe all functions, together with arguments and return value.

Thanks !

>> really dislike how you fiddle around with void pointers, can't that be
>> fixed?
> 
> The void pointers are imposed by the sort() declaration, I'm not sure how we can
> avoid them. See include/linux/sort.h:
> void sort(void *base, size_t num, size_t size,
>           int (*cmp)(const void *, const void *),
>           void (*swap)(void *, void *, int));

Ah OK, thanks for clarifying.

>>>>> +
>>>>> +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 ?
> 
> The user will see just the error code, it is not notified which part of the
> flash is erased and which not, just like in the uniform erase case. I'm not sure
> what would be the advantage of reporting a partial erase. If the erase fails
> mid-way then it's not reliable, no?

Yes, that's right.

> Since your suggestion also applies for the uniform erase case, would you agree
> to let it apart and treat it in a different patch after the non-uniform erase
> gets approved?

That's fine, just document this behavior please.

-- 
Best regards,
Marek Vasut