Alexander Shishkin had written, on 11/02/2009 07:12 AM, the following:
2009/10/21 Nishanth Menon <nm@xxxxxx>:
Paul Walmsley had written, on 10/20/2009 06:14 PM, the following:
Hi Vikram, Nishanth, Richard,
a few comments on this:
On Tue, 20 Oct 2009, Vikram Pandita wrote:
Add bits for future expansion of omap_chip_id type field.
This is needed to accomodate 3630ES1 chip id which is bit10
...
diff --git a/arch/arm/plat-omap/include/plat/cpu.h
b/arch/arm/plat-omap/include/plat/cpu.h
index 7cb0556..922bf1c 100644
--- a/arch/arm/plat-omap/include/plat/cpu.h
+++ b/arch/arm/plat-omap/include/plat/cpu.h
@@ -45,7 +45,7 @@ int omap_type(void);
struct omap_chip_id {
u8 oc;
- u8 type;
+ u32 type;
};
Just wanted to understand the motivation for using the u32.
Earlier in the life of these patches, two comments were mentioned: the
desire to 'futureproof' and the desire to reserve space for other
34xx-family parts.
Regarding 'futureproofing:' that's part of the reason that a separate
struct was defined for this: to prevent code that uses it from depending on
the size of the type. (Originally it was a typedef, but Linus hates
typedefs...) So it shouldn't matter how big or small the type is here, as
long as it can handle all of the bits allocated for it.
Also mentioned was the idea of reserving space for other 34xx-family
chips. I'd suggest simply renumbering the bits when and if those versions
appear. Code that uses the omap_chip_id system should always use the macros
(e.g. CHIP_IS_OMAP3430) and not encode separate bit shift values, so
renumbering should be completely safe and transparent for core code. Module
code shouldn't be using the omap_chip code, it's for core usage only.
So, since only one bit is being added, why not continue the use of the u8?
Then when the next bits need to be added, the type can be expanded at that
point, and the bits renumbered if necessary. This should be a completely
transparent operation for code that uses it. Vikram's original patch:
http://patchwork.kernel.org/patch/54847/
should be fine.
Assumptions:
a) omap_chip_id is supposedly constant for all devices within the same
family. 3630, 3430 rev x will belong to the same family.
If my understanding of the matter is correct, that's only possible if
you can foretell the total number of upcoming 34xx revisions worth
mentioning in the code. Also, can you please elaborate on why is it
supposed to be constant?
The assumption I made was that omap_chip_id structure was to store info
regarding the chip and used by the pm framework. The term constant,
which I used, is probably wrong in this context, esp considering that we
cannot foretell all the upcoming revisions of the 34xx family.
Issues with the strategy of restricting to the current 8 bits:
a) Why extrabits now:
we have 8 bits now and we would have used all 8 bits with 3630 with the
mentioned patch. What do we do with the next revision of 3430? Do we want to
increase the size once it comes along? OR Do we want to do it right now? Why
wait till we get additional silicons to go figure how to add those bits as
Richard pointed out, when there could be one more in the pipeline?
But this code will have to be revisited for each additional silicon
revision anyway, right? Why reserve now?
Agreed, that is one of the possible approaches we could take (and seems
to be the common consensus), we can review the structure at a later
point of time.
http://patchwork.kernel.org/patch/54847/ seems to be the right direction
for now at least.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
