Am 2020-02-19 11:50, schrieb Jungseung Lee:
Hi, Tudor and all
2020-02-10 (Mon), 11:26 +0000, Tudor.Ambarus@xxxxxxxxxxxxx:
On Monday, February 10, 2020 12:29:34 PM EET Michael Walle wrote:
> > It's bytes. Take a look at W25Q128JV. The sector size for this
> > flash is
> > 64KByte. The flash has 256 sectors. For this specific case:
> > bp_slots_available = 6;
> > bp_slots_needed = 8;
> >
> > The if condition is true, so
> > bp_slot_count = 6;
> > bp_min_slot_size = 64k << (8 - 6); //256k
>
> But nor->info->n_sectors is not 64k, its 256. Do you mean
> sector_size
> (like in
> my example below? Then we are on the same page
Indeed, there is a typo in the pseudo code; I'm happy that the
example is
correct at least. I meant sector_size, not sectors. Now we should
exercise the
logic to all the known (corner) cases. Maybe Jungseung will tell us
if he
spots a flaw in the overall logic.
I didn't find any flaw in this logic. But IMHO for the pseudo code.
bp_slots_available = (bp_mask >> shift) + 1 - 2;
bp_slots_needed = ilog2(nor->info->n_sectors);
if (bp_slots_needed > bp_slots_available) {
bp_slot_count = bp_slots_available;
bp_min_slot_size = nor->info->sector_size <<
(bp_slots_needed - bp_slots_available);
} else {
bp_slot_count = bp_slots_needed;
bp_min_slot_size = mtd->size >> bp_slot_count;
}
Probably we can use directly nor->info->sector_size for bp_min_slot_
size.
sector_size x n_sectors = mtd->size
mtd->size / n_sectors = mtd->size >>
ilog2(n_sectors) = sector_size
bp_slot_count is equal to log2(n_sectors) now so if we can trust the
value, we can also trust sector_size.
After change it,
if (bp_slots_needed > bp_slots_available)
bp_min_slot_size = nor->info->sector_size <<
(bp_slots_needed - bp_slots_available);
else
bp_min_slot_size = nor->info->sector_size
Yes, thats what I pointed out in my previous mail, too. So I guess we
agree on that.
This is a comment from my previous mail.
> The exact fact is that locks operate in two different ways
> according to flash model.
>
> (1) the smallest protected portion is fixed.
> for BP0-2 : 1/64
> for BP0-1 : 1/4
> (2) the smallest protected portion is inversely propotional with
> number of sectors.
Again. please don't consider the ratio of the protected area to the
flash size.
This is only the result of applying the sector size and IMHO really bad
for
understanding. Use the number of protected sectors. Thus your (2) is
actually
always one sector (as you've already pointed out above).
(1) - if the slot is insufficient.
(2) - if the slot is sufficient.
From the fact, that could be rewritten like below. I think it's more
intuitive one.
if (bp_slots_needed > bp_slots_available) // (1)
bp_min_slot_size = mtd->size >> bp_slots_available;
Given the reasoning above, I'd prefer having the number of sectors and
thus
the sector_size. Eg. keep the following
bp_min_slot_size = nor->info->sector_size <<
(bp_slots_needed - bp_slots_available);
else // (2)
bp_min_slot_size = nor->info->sector_size;
We could also find a few flashes that does not following the overall
logic. For example, "en25qh256" and "en25qh16" which was manufactured
by EON. They are always following way (2) no matter what the number of
slot is. It seems that it could be handled like below with custom hook
later.
if (bp_slots_needed < bp_slots_available || force)
For the en25qh16 the "bp_slots_needed < bp_slots_available" is already
true, isn't it?
But good catch for the en25qh256. IMHO the rework of the BP bits should
already
add a flag (together with a reference to this flash) so this information
is not lost. what about sth like MIN_LOCK_SIZE_IS_ONE_SECTOR.
-michael
bp_min_slot_size = nor->info->sector_size;
else
bp_min_slot_size = mtd->size >> bp_slots_available;
Cheers,
ta
Thanks,
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
