| Message ID | 1314880043-22517-5-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 01 September 2011, Lee Jones wrote: > +static const char *db8500_get_soc_id(void) > +{ > + void __iomem *uid_base; > + char buf[1024]; > + ssize_t sz = 0; > + int i; > + > + uid_base = __io_address(U8500_BB_UID_BASE); > + for (i = 0; i < U8500_BB_UID_LENGTH; i++) { > + sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32))); > + } > + return kasprintf(GFP_KERNEL, "%s", buf); > +} You will get a warning from the stack checker here, about putting a 1024 byte string on the stack. Also, I still think it's bad to just access the U8500_BB_UID_BASE from a compile-time constant. Since this gets called from a function that knows the base address of the DB8500 register area, better pass the device in there so that you end up with something like static void __devinit db8500_read_soc_id(struct db8500_dev *dev) { u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET; snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x", readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4])); } The style you use here is preexisting in the db8500 code, but you should not keep adding more of that crap. All the code like #define db8500_add_i2c0(pdata) \ dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata) #define db8500_add_i2c1(pdata) \ dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata) should never really have been there. What you want to do for this is to call this from the code that initializes the db8500 controller, and pass the board specific pdata into the db8500 init function, along with the i2c client data. Arnd
On 02/09/11 15:22, Arnd Bergmann wrote: > On Thursday 01 September 2011, Lee Jones wrote: >> +static const char *db8500_get_soc_id(void) >> +{ >> + void __iomem *uid_base; >> + char buf[1024]; >> + ssize_t sz = 0; >> + int i; >> + >> + uid_base = __io_address(U8500_BB_UID_BASE); >> + for (i = 0; i < U8500_BB_UID_LENGTH; i++) { >> + sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32))); >> + } >> + return kasprintf(GFP_KERNEL, "%s", buf); >> +} > > You will get a warning from the stack checker here, about putting a 1024 byte string > on the stack. Also, I still think it's bad to just access the U8500_BB_UID_BASE > from a compile-time constant. Since this gets called from a function that knows > the base address of the DB8500 register area, better pass the device in there > so that you end up with something like > > static void __devinit db8500_read_soc_id(struct db8500_dev *dev) > { > u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET; > snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x", > readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4])); > } No problem. Where did you get dev->base from though? > The style you use here is preexisting in the db8500 code, but you should > not keep adding more of that crap. Not meaning to pass the buck at all, but this isn't my code. It's remnant from when I took this over (starting to wish I hadn't ;)) > All the code like > #define db8500_add_i2c0(pdata) \ > dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata) > #define db8500_add_i2c1(pdata) \ > dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata) > > should never really have been there. What you want to do for this is > to call this from the code that initializes the db8500 controller, and > pass the board specific pdata into the db8500 init function, along > with the i2c client data. Not my domain. Speak to Linus W. :)
On Friday 02 September 2011, Lee Jones wrote: > > static void __devinit db8500_read_soc_id(struct db8500_dev *dev) > > { > > u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET; > > snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x", > > readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4])); > > } > > No problem. > > Where did you get dev->base from though? In this example, db8500_dev would be derived from soc_device. and get initialized at the time you register it to the soc subsystem. You clearly have to pass in the base address at some point, so if there is no device tree, that init function should statically set it, but you would still avoid spreading the use of these constants all over the code. > > The style you use here is preexisting in the db8500 code, but you should > > not keep adding more of that crap. > > Not meaning to pass the buck at all, but this isn't my code. > > It's remnant from when I took this over (starting to wish I hadn't ;)) I feel your pain. Sorry that you got dragged in this so deeply when you only wanted a place to add a tiny bit of information. > > All the code like > > #define db8500_add_i2c0(pdata) \ > > dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata) > > #define db8500_add_i2c1(pdata) \ > > dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata) > > > > should never really have been there. What you want to do for this is > > to call this from the code that initializes the db8500 controller, and > > pass the board specific pdata into the db8500 init function, along > > with the i2c client data. > > Not my domain. Speak to Linus W. :) Yes, I understand. I was mostly using this as explanation about where the problem lies. I'm not expecting you to fix all of the problems of the platform, but in order to add new code the right way, we have to agree on how it should have been there to start with. Arnd
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c index 655639e..9824cd3 100644 --- a/arch/arm/mach-ux500/cpu-db8500.c +++ b/arch/arm/mach-ux500/cpu-db8500.c @@ -167,6 +167,20 @@ static void __init db8500_add_gpios(void) IRQ_DB8500_GPIO0, &pdata); } +static const char *db8500_get_soc_id(void) +{ + void __iomem *uid_base; + char buf[1024]; + ssize_t sz = 0; + int i; + + uid_base = __io_address(U8500_BB_UID_BASE); + for (i = 0; i < U8500_BB_UID_LENGTH; i++) { + sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32))); + } + return kasprintf(GFP_KERNEL, "%s", buf); +} + static int usb_db8500_rx_dma_cfg[] = { DB8500_DMA_DEV38_USB_OTG_IEP_1_9, DB8500_DMA_DEV37_USB_OTG_IEP_2_10, @@ -208,6 +222,8 @@ void __init u8500_init_devices(void) "cpufreq-u8500", -1, NULL, 0, NULL, 0); if (soc_dev) { + soc_dev->pfn_soc_id = db8500_get_soc_id; + for (i=0; i<ARRAY_SIZE(platform_devs); i++) platform_devs[i]->dev.parent = &soc_dev->dev; } diff --git a/arch/arm/mach-ux500/include/mach/db8500-regs.h b/arch/arm/mach-ux500/include/mach/db8500-regs.h index 0499971..f569d5c 100644 --- a/arch/arm/mach-ux500/include/mach/db8500-regs.h +++ b/arch/arm/mach-ux500/include/mach/db8500-regs.h @@ -166,4 +166,8 @@ #define U8500_MODEM_BASE 0xe000000 #define U8500_APE_BASE 0x6000000 +/* SoC identification number information */ +#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0) +#define U8500_BB_UID_LENGTH 5 + #endif
With this patch applied the Export SoC Information framework will be able to callback into u8500 Platform code in order to probe for the SoC's unique Identification (serial) number. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- arch/arm/mach-ux500/cpu-db8500.c | 16 ++++++++++++++++ arch/arm/mach-ux500/include/mach/db8500-regs.h | 4 ++++ 2 files changed, 20 insertions(+), 0 deletions(-)