Re: [PATCH 2/2] Add support for > 2GiB MTD devices

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

Hi Bruce,

On Tue, 2008-08-19 at 14:27 -0700, Bruce Leonard wrote:
> +/*
> + * Inline function for determining the size of the MTD device, independant
> + * of old or new way of doing things.
> + *
> + */
> +static inline u_int64_t device_size(struct mtd_info *a)
> +{
> +	return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks * a->erasesize;
> +}

I do not think it is a good idea to do multiplication every time we need
MTD device size. It is unnecessarily large overhead in terms of speed
and code size.

Did you consider a possibility of just making mtd->size 64 bit?
Or using eraseblock:offset pairs instead of absolute address?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Bruce Leonard]

On Aug 26, 2008, at 10:40 PM, Artem Bityutskiy wrote:

> Hi Bruce,
>
> On Tue, 2008-08-19 at 14:27 -0700, Bruce Leonard wrote:
>> +/*
>> + * Inline function for determining the size of the MTD device,  
>> independant
>> + * of old or new way of doing things.
>> + *
>> + */
>> +static inline u_int64_t device_size(struct mtd_info *a)
>> +{
>> +	return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks * a- 
>> >erasesize;
>> +}
>
> I do not think it is a good idea to do multiplication every time we  
> need
> MTD device size. It is unnecessarily large overhead in terms of speed
> and code size.
>
> Did you consider a possibility of just making mtd->size 64 bit?

I did consider making size 64-bit, but it seemed less intrusive to go  
the direction I did.  I wanted to change as little code as possible  
but at the same time make it obvious there was a fundamental change.   
There's also a desire to move more in the direction of a BIO-like  
aspect to the MTD layer and some of the suggestions I got early made  
it seem that this would make that future move easier.

>
> Or using eraseblock:offset pairs instead of absolute address?

I didn't really see how I could convey the idea of size using  
eraseblock:offset.

Bruce

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Bruce_Leonard]

Artem,

> > +/*
> > + * Inline function for determining the size of the MTD device, 
independant
> > + * of old or new way of doing things.
> > + *
> > + */
> > +static inline u_int64_t device_size(struct mtd_info *a)
> > +{
> > +   return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks * 
a->erasesize;
> > +}
> 
> I do not think it is a good idea to do multiplication every time we need
> MTD device size. It is unnecessarily large overhead in terms of speed
> and code size.
> 

I'm still reluctant to change size to a 64-bit value.  There's a vague 
recolection of early conversations on the list that there would be little 
acceptance for that.  And that probably has to do with the ongoing 
conversation about ABI changes.  What I could do to eliminate the 
multiplication is introduce the same concept that the NAND layer uses, 
shift values.  After all, erasesize should always be a power of 2, making 
that a power of 2 multiplication which can be done via shifts.  By 
changing erasesize to erasesize_shift, I'd get something like this:

return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks << 
a->erasesize_shift

How would that suit you?

Bruce

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Jamie Lokier]

Bruce_Leonard@selinc.com wrote:
> I'm still reluctant to change size to a 64-bit value.  There's a vague 
> recolection of early conversations on the list that there would be little 
> acceptance for that.  And that probably has to do with the ongoing 
> conversation about ABI changes.  What I could do to eliminate the 
> multiplication is introduce the same concept that the NAND layer uses, 
> shift values.  After all, erasesize should always be a power of 2, making 
> that a power of 2 multiplication which can be done via shifts.  By 
> changing erasesize to erasesize_shift, I'd get something like this:
> 
> return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks << 
> a->erasesize_shift
> 
> How would that suit you?

Are you sure it's always going to be a power of 2?

What if someone targets a board with 3 chips wired to shared address
and parallel data buses?

Or if someone makes a weird chip?  Or if you can format it in
different ways according to desired ECC level (like you can with CDs)?

If there's ongoing conversation about ABI changes, it sounds like it
would be good for this change to be done together that, instead of
doing this change then changing the ABI _again_ shortly after and
having to support 3 different ABIs in tools instead of 2.

-- Jamie

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Carl-Daniel Hailfinger]

On 27.08.2008 20:51, Jamie Lokier wrote:
> Bruce_Leonard@selinc.com wrote:
>   
>> I'm still reluctant to change size to a 64-bit value.  There's a vague 
>> recolection of early conversations on the list that there would be little 
>> acceptance for that.  And that probably has to do with the ongoing 
>> conversation about ABI changes.  What I could do to eliminate the 
>> multiplication is introduce the same concept that the NAND layer uses, 
>> shift values.  After all, erasesize should always be a power of 2, making 
>> that a power of 2 multiplication which can be done via shifts.  By 
>> changing erasesize to erasesize_shift, I'd get something like this:
>>
>> return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks << 
>> a->erasesize_shift
>>
>> How would that suit you?
>>     
>
> Are you sure it's always going to be a power of 2?
>
> What if someone targets a board with 3 chips wired to shared address
> and parallel data buses?
>
> Or if someone makes a weird chip?  Or if you can format it in
> different ways according to desired ECC level (like you can with CDs)?
>   

IIRC I saw a datasheet for such a chip (selectable erasesize with
non-power-of-2 default) some weeks ago and it had entered production a
few months ago. The erasesize was alwas a multiple of 16, though. Sorry
for not remembering more details.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Trent Piepho]

On Wed, 27 Aug 2008, Carl-Daniel Hailfinger wrote:
> On 27.08.2008 20:51, Jamie Lokier wrote:
>> Bruce_Leonard@selinc.com wrote:
>>
>>> I'm still reluctant to change size to a 64-bit value.  There's a vague
>>> recolection of early conversations on the list that there would be little
>>> acceptance for that.  And that probably has to do with the ongoing
>>> conversation about ABI changes.  What I could do to eliminate the
>>> multiplication is introduce the same concept that the NAND layer uses,
>>> shift values.  After all, erasesize should always be a power of 2, making
>>> that a power of 2 multiplication which can be done via shifts.  By
>>> changing erasesize to erasesize_shift, I'd get something like this:
>>>
>>> return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks <<
>>> a->erasesize_shift
>>>
>>> How would that suit you?
>>>
>>
>> Are you sure it's always going to be a power of 2?
>>
>> What if someone targets a board with 3 chips wired to shared address
>> and parallel data buses?
>>
>> Or if someone makes a weird chip?  Or if you can format it in
>> different ways according to desired ECC level (like you can with CDs)?
>>
>
> IIRC I saw a datasheet for such a chip (selectable erasesize with
> non-power-of-2 default) some weeks ago and it had entered production a
> few months ago. The erasesize was alwas a multiple of 16, though. Sorry
> for not remembering more details.

It seems like the device size is always going to have some zeros in the least
significant bits.  Maybe a 32-bit size plus a shift is enough?  That could be
a lot more efficient than a 64-bit size and avoid penalizing most users who
don't need >32-bit size chips.

Re: [PATCH 2/2] Add support for > 2GiB MTD devices

[Bruce_Leonard]

linux-mtd-bounces@lists.infradead.org wrote on 08/27/2008 02:52:24 PM:

> On 27.08.2008 20:51, Jamie Lokier wrote:
> > Bruce_Leonard@selinc.com wrote:
> > 
> >> I'm still reluctant to change size to a 64-bit value.  There's a 
vague 
> >> recolection of early conversations on the list that there would be 
little 
> >> acceptance for that.  And that probably has to do with the ongoing 
> >> conversation about ABI changes.  What I could do to eliminate the 
> >> multiplication is introduce the same concept that the NAND layer 
uses, 
> >> shift values.  After all, erasesize should always be a power of 2, 
making 
> >> that a power of 2 multiplication which can be done via shifts.  By 
> >> changing erasesize to erasesize_shift, I'd get something like this:
> >>
> >> return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks << 
> >> a->erasesize_shift
> >>
> >> How would that suit you?
> >> 
> >
> > Are you sure it's always going to be a power of 2?
> >
> > What if someone targets a board with 3 chips wired to shared address
> > and parallel data buses?
> >
> > Or if someone makes a weird chip?  Or if you can format it in
> > different ways according to desired ECC level (like you can with CDs)?
> > 
> 
> IIRC I saw a datasheet for such a chip (selectable erasesize with
> non-power-of-2 default) some weeks ago and it had entered production a
> few months ago. The erasesize was alwas a multiple of 16, though. Sorry
> for not remembering more details.
> 
> Regards,
> Carl-Daniel
> 

Well in that case I don't see that there's much option other than a 
multiplication.  If there were an odd number of eraseblocks (i.e., 3 
chips) you could still do the shifting operations.  But if someone has 
come up with a flash part that has a non-power of two sector/erasesize, 
then there's no way to do it by shift.  I supose I could just change 
erasesize to size64, make it a 64-bit type and do this:

return a->num_eraseblocks == 0 ? a->size : a->size64

Doesn't seem quite as elegant, but it is simpler.  What ever I do, I can't 
change the meaning or type of size.  That's an kernel <=> userspace ABI 
change, and we know what happens when I try to do that ;).

Bruce

------------------------------------------------

This e-mail may contain SEL confidential information.  The opinions
expressed are not necessarily those of SEL.  Any unauthorized disclosure,
distribution or other use is prohibited.  If you received this e-mail in
error, please notify the sender, permanently delete it, and destroy any
printout.

Thank you.

------------------------------------------------