* Re: [PATCH 2/2] Add support for > 2GiB MTD devices [not found] <48AB3AAF.3010504@earthlink.net> @ 2008-08-27 5:40 ` Artem Bityutskiy 2008-08-27 7:15 ` Bruce Leonard 2008-08-27 18:18 ` Bruce_Leonard 0 siblings, 2 replies; 7+ messages in thread From: Artem Bityutskiy @ 2008-08-27 5:40 UTC (permalink / raw) To: Bruce Leonard; +Cc: linux-mtd, linux-kernel 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 (Битюцкий Артём) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 5:40 ` [PATCH 2/2] Add support for > 2GiB MTD devices Artem Bityutskiy @ 2008-08-27 7:15 ` Bruce Leonard 2008-08-27 18:18 ` Bruce_Leonard 1 sibling, 0 replies; 7+ messages in thread From: Bruce Leonard @ 2008-08-27 7:15 UTC (permalink / raw) To: dedekind; +Cc: linux-mtd, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 5:40 ` [PATCH 2/2] Add support for > 2GiB MTD devices Artem Bityutskiy 2008-08-27 7:15 ` Bruce Leonard @ 2008-08-27 18:18 ` Bruce_Leonard 2008-08-27 18:51 ` Jamie Lokier 1 sibling, 1 reply; 7+ messages in thread From: Bruce_Leonard @ 2008-08-27 18:18 UTC (permalink / raw) To: dedekind; +Cc: Bruce Leonard, linux-kernel, linux-mtd, linux-mtd-bounces 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 18:18 ` Bruce_Leonard @ 2008-08-27 18:51 ` Jamie Lokier 2008-08-27 21:52 ` Carl-Daniel Hailfinger 0 siblings, 1 reply; 7+ messages in thread From: Jamie Lokier @ 2008-08-27 18:51 UTC (permalink / raw) To: Bruce_Leonard Cc: dedekind, linux-mtd-bounces, linux-mtd, linux-kernel, Bruce Leonard 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 18:51 ` Jamie Lokier @ 2008-08-27 21:52 ` Carl-Daniel Hailfinger 2008-08-27 22:32 ` Trent Piepho 2008-08-28 17:48 ` Bruce_Leonard 0 siblings, 2 replies; 7+ messages in thread From: Carl-Daniel Hailfinger @ 2008-08-27 21:52 UTC (permalink / raw) To: Jamie Lokier Cc: Bruce_Leonard, linux-mtd-bounces, linux-mtd, linux-kernel, Bruce Leonard 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 21:52 ` Carl-Daniel Hailfinger @ 2008-08-27 22:32 ` Trent Piepho 2008-08-28 17:48 ` Bruce_Leonard 1 sibling, 0 replies; 7+ messages in thread From: Trent Piepho @ 2008-08-27 22:32 UTC (permalink / raw) To: Carl-Daniel Hailfinger Cc: Jamie Lokier, MTD mailing list, linux-kernel, Bruce_Leonard, Bruce Leonard 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Add support for > 2GiB MTD devices 2008-08-27 21:52 ` Carl-Daniel Hailfinger 2008-08-27 22:32 ` Trent Piepho @ 2008-08-28 17:48 ` Bruce_Leonard 1 sibling, 0 replies; 7+ messages in thread From: Bruce_Leonard @ 2008-08-28 17:48 UTC (permalink / raw) To: Carl-Daniel Hailfinger Cc: Bruce Leonard, Jamie Lokier, linux-kernel, linux-mtd, linux-mtd-bounces 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. ------------------------------------------------ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-08-28 18:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <48AB3AAF.3010504@earthlink.net>
2008-08-27 5:40 ` [PATCH 2/2] Add support for > 2GiB MTD devices Artem Bityutskiy
2008-08-27 7:15 ` Bruce Leonard
2008-08-27 18:18 ` Bruce_Leonard
2008-08-27 18:51 ` Jamie Lokier
2008-08-27 21:52 ` Carl-Daniel Hailfinger
2008-08-27 22:32 ` Trent Piepho
2008-08-28 17:48 ` Bruce_Leonard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox