IPC driver for Intel Mobile Internet Device (MID) platforms
PATCH 1/1 IPC driver for Intel Mobile Internet Device (MID) platforms
It would be nice to get this into the tree in some form as a pile of the driver stuff pending from Intel depends upon it. It's currently slotted into arch/x86 as the IPC interface is very much part of the hardware, so don't be fooled by its apparent PCI interface.
--
From: Sreedhara DS <[email protected]>
The IPC is used to bridge the communications between kernel and SCU on some embedded Intel x86 platforms.
(Some API tweaking Alan Cox)
---
arch/x86/Kconfig | 8 arch/x86/include/asm/intel_scu_ipc.h | 93 ++++ arch/x86/kernel/Makefile | 1 arch/x86/kernel/intel_scu_ipc.c | 882 ++++++++++++++++++++++++++++++++++ 4 files changed, 984 insertions(+), 0 deletions(-) create mode 100644 arch/x86/include/asm/intel_scu_ipc.h create mode 100644 arch/x86/kernel/intel_scu_ipc.c
arch/x86/Kconfig +8 -0
@@ -410,6 +410,14 @@ config X86_MRST nor standard legacy replacement devices/features. e.g. Moorestown does not contain i8259, i8254, HPET, legacy BIOS, most of the io ports. +config INTEL_SCU_IPC + bool "Intel SCU IPC Support" + depends on X86_MRST + default y + ---help--- + IPC is used to bridge the communications between kernel and SCU on + some embedded Intel x86 platforms. + config X86_RDC321X bool "RDC R-321x SoC" depends on X86_32
arch/x86/include/asm/intel_scu_ipc.h new file +93 -0
93 lines not on this page · raw message
arch/x86/kernel/Makefile +1 -0
@@ -107,6 +107,7 @@ scx200-y += scx200_32.o obj-$(CONFIG_OLPC) += olpc.o obj-$(CONFIG_X86_MRST) += mrst.o +obj-$(CONFIG_INTEL_SCU_IPC) += intel_scu_ipc.o microcode-y := microcode_core.o microcode-$(CONFIG_MICROCODE_INTEL) += microcode_intel.o
arch/x86/kernel/intel_scu_ipc.c new file +882 -0
882 lines not on this page · raw message
Quoted 5 lines · On Fri, 09 Apr 2010 11:29:23 +0100 Alan Cox <[email protected]> wrote:
It would be nice to get this into the tree in some form as a pile of the driver stuff pending from Intel depends upon it. It's currently slotted into
arch/x86as the IPC interface is very much part of the hardware, so don't be fooled by its apparent PCI interface.
Quoted 8 lines
--
From: Sreedhara DS <[email protected]>
The IPC is used to bridge the communications between kernel and SCU on some embedded Intel x86 platforms.
(Some API tweaking Alan Cox)
It's be nice to have some words or a link describing what an IPC actually _is_.
arch/x86/include/asm/intel_scu_ipc.h line 28
Quoted 11 lines
...
arch/x86/include/asm/intel_scu_ipc.h new file +8
-0+struct battery_property { +u32 capacity;/* Charger capacity */ +u8 crnt;/* Quick charge current value*/ +u8 volt;/* Fine adjustment of constant charge voltage */ +u8 prot;/* CHRGPROT register value */ +u8 prot2;/* CHRGPROT1 register value */ +u8 timer;/* Charging timer */ +} __attribute__((packed));
__packed, please. (I've requested that this be added to checkpatch, but Mr Checkpatch is asleep).
arch/x86/kernel/intel_scu_ipc.c line 80
Quoted 11 lines
...
arch/x86/kernel/intel_scu_ipc.c new file +8
-0+struct intel_scu_ipc_dev { +struct pci_dev *pdev; +void __iomem *ipc_base; +void __iomem *i2c_base; +void __iomem *pci_base; +}; + +static struct intel_scu_ipc_dev ipcdev;/* Only one for now */
Could do
static struct intel_scu_ipc_dev {
...
} ipcdev;if so inclined.
Quoted 39 lines
arch/x86/kernel/intel_scu_ipc.c new file +38
-0+static int platform =1 ; +module_param (platform, int,0 ); +MODULE_PARM_DESC (platform,"1 for moorestown platform" ); + +/* +* Command Register (Write Only): +* A write to this register results in an interrupt to the SCU core processor +* Format: +* |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)| +*/ +#define IPC_COMMAND_REG ipcdev.ipc_base +/* +* Status Register (Read Only): +* Driver will read this register to get the ready/busy status of the IPC +* block and error status of the IPC command that was just processed by SCU +* Format: +* |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)| +*/ +#define IPC_STATUS_REG (ipcdev.ipc_base + 0x04) + +/* +* IPC Source Pointer (Write Only): +* Use content as pointer for read location +*/ +#define IPC_SPTR_REG (ipcdev.ipc_base + 0x08) + +/* +* IPC destination Pointer (Write Only): +* Use content as pointer for destination write +*/ +#define IPC_DPTR_REG (ipcdev.ipc_base + 0x0C) + +/* +* IPC Write Buffer (Write Only): +* 16-byte buffer for sending data associated with IPC command to +* SCU. Size of the data is specified in the IPC_COMMAND_REG register +*/ +#define IPC_WRITE_BUFFER (ipcdev.ipc_base + 0x80)
arch/x86/kernel/intel_scu_ipc.c line 130
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+ +/* +* IPC Read Buffer (Read Only): +* 16 byte buffer for receiving data from SCU, if IPC command +* processing results in response data +*/ +#define IPC_READ_BUFFER (ipcdev.ipc_base + 0x90) + +#define IPC_I2C_CNTRL_ADDR ipcdev.i2c_base +#define I2C_DATA_ADDR (ipcdev.i2c_base + 0x04)
Do we actually need these aliases? Why not open-code `ipcdev.ipc_base', etc in the very few places where these macros are used?
Quoted 6 lines
...
arch/x86/kernel/intel_scu_ipc.c new file +3
-0+static inline int busy_loop (void )/* Wait till scu status is busy */ +{ +u32 status =0 ;
arch/x86/kernel/intel_scu_ipc.c line 176
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+u32 loop_count =0 ; + + status =__raw_readl (IPC_STATUS_REG ); +while (status &1 ) { +udelay (1 );/* scu processing time is in few u secods */ + status =__raw_readl (IPC_STATUS_REG ); + loop_count++; +/* break if scu doesn't reset busy bit after huge retry */ +if (loop_count >100000 ) +return -ETIMEDOUT ;
I'd suggest adding a printk if this failure happens. Otherwise the results will be pretty mysterious.
arch/x86/kernel/intel_scu_ipc.c line 179
Quoted 3 lines
arch/x86/kernel/intel_scu_ipc.c new file +3
-0+ } +return (status >>1 ) &1 ; +}
This function has seven-odd callsites and is waaaaaaaay to fat and slow to be inlined.
arch/x86/kernel/intel_scu_ipc.c line 187
Quoted 7 lines
arch/x86/kernel/intel_scu_ipc.c new file +7
-0+/* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */ +static int pwr_reg_rdwr (u16 *addr,u8 *data,u32 count,u32 op,u32 id) +{ +int nc; +u32 offset =0 ; +u32 err =0 ; +u8 cbuf[IPC_WWBUF_SIZE ] = {'\0' };
Actually, `= { }' will suffice.
Quoted 35 lines
arch/x86/kernel/intel_scu_ipc.c new file +35
-0+u32 *wbuf = (u32 *)&cbuf; + +mutex_lock (&ipclock); +if (ipcdev.pdev ==NULL ) { +mutex_unlock (&ipclock); +return -ENODEV ; + } + +if (platform ==1 ) { +/* Entry is 4 bytes for read/write, 5 bytes for read modify */ +for (nc =0 ; nc < count; nc++) { + cbuf[offset] = addr[nc]; + cbuf[offset +1 ] = addr[nc] >>8 ; +if (id !=IPC_CMD_PCNTRL_R ) + cbuf[offset +2 ] = data[nc]; +if (id ==IPC_CMD_PCNTRL_M ) { + cbuf[offset +3 ] = data[nc +1 ]; + offset +=1 ; + } + offset +=3 ; + } +for (nc =0 , offset =0 ; nc < count; nc++, offset +=4 ) +ipc_write (wbuf[nc], offset);/* Write wbuff */ + + }else { +for (nc =0 , offset =0 ; nc < count; nc++, offset +=2 ) +ipc_write (addr[nc], offset);/* Write addresses */ +if (id !=IPC_CMD_PCNTRL_R ) { +for (nc =0 ; nc < count; nc++, offset++) +ipc_write (data[nc], offset);/* Write data */ +if (id ==IPC_CMD_PCNTRL_M ) +ipc_write (data[nc +1 ], offset);/* Mask value*/ + } + } +
arch/x86/kernel/intel_scu_ipc.c line 232
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+if (id !=IPC_CMD_PCNTRL_M ) +ipc_command ((count*3 ) <<16 | id <<12 |0 <<8 | op); +else +ipc_command ((count*4 ) <<16 | id <<12 |0 <<8 | op); + + err =busy_loop (); + +if (id ==IPC_CMD_PCNTRL_R ) {/* Read rbuf */ +/* Workaround: values are read as 0 without memcpy_fromio */ +memcpy_fromio (cbuf,IPC_READ_BUFFER ,16 );
Should we still be doing this if busy_loop() failed?
(Lots of dittoes on this question)
arch/x86/kernel/intel_scu_ipc.c line 243
Quoted 11 lines
arch/x86/kernel/intel_scu_ipc.c new file +11
-0+if (platform ==1 ) { +for (nc =0 , offset =2 ; nc < count; nc++, offset +=3 ) + data[nc] =ipc_readb (offset); + }else { +for (nc =0 ; nc < count; nc++) + data[nc] =ipc_readb (nc); + } + } +mutex_unlock (&ipclock); +return err; +}
I wonder if this function would look better if cbuf had type u16[],
arch/x86/kernel/intel_scu_ipc.c line 384
Quoted 9 lines
...
arch/x86/kernel/intel_scu_ipc.c new file +6
-0+int intel_scu_ipc_register_read (u32 addr,u32 *value) +{ +u32 err =0 ; + +mutex_lock (&ipclock); +if (ipcdev.pdev ==NULL ) {
This check happens a lot. Can it really happen?
Quoted 14 lines
arch/x86/kernel/intel_scu_ipc.c new file +12
-0+mutex_unlock (&ipclock); +return -ENODEV ; + } +ipc_write_sptr (addr); +ipc_command (4 <<16 |IPC_CMD_INDIRECT_RD ); + err =busy_loop (); + *value =ipc_readl (0 ); +mutex_unlock (&ipclock); +return err; +} +EXPORT_SYMBOL (intel_scu_ipc_register_read); +
arch/x86/kernel/intel_scu_ipc.c line 609
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +9
-0+int intel_scu_ipc_i2c_cntrl (u32 addr,u32 *data) +{ +u32 cmd =0 ; + +mutex_lock (&ipclock); + cmd = (addr >>24 ) &0xFF ; +if (cmd ==IPC_I2C_READ ) { +writel (addr,IPC_I2C_CNTRL_ADDR ); +mdelay (1 );/*Write Not getting updated without delay*/
Odd commenting layout.
Quoted 15 lines
arch/x86/kernel/intel_scu_ipc.c new file +15
-0+ *data =readl (I2C_DATA_ADDR ); + }else if (cmd ==IPC_I2C_WRITE ) { +writel (addr,I2C_DATA_ADDR ); +mdelay (1 ); +writel (addr,IPC_I2C_CNTRL_ADDR ); + }else { +dev_err (&ipcdev.pdev->dev, +"intel_scu_ipc: I2C INVALID_CMD = 0x%x\n" , cmd); + +mutex_unlock (&ipclock); +return -1 ; + } +mutex_unlock (&ipclock); +return 0 ; +}
arch/x86/kernel/intel_scu_ipc.c line 662
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +7
-0+int intel_scu_ipc_fw_update (u8 *buffer,u32 length) +{ +void __iomem *fw_update_base; +void __iomem *mailbox_base; +int retry_cnt =0 ; + +struct fw_update_mailbox *mailbox =NULL ;
Nuke random newline.
Quoted 5 lines
arch/x86/kernel/intel_scu_ipc.c new file +5
-0+mutex_lock (&ipclock); + fw_update_base =ioremap_nocache (IPC_FW_LOAD_ADDR , (128 *1024 )); +if (fw_update_base ==NULL ) { +mutex_unlock (&ipclock); +return -ENOMEM ;
arch/x86/kernel/intel_scu_ipc.c line 678
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+ } + mailbox_base =ioremap_nocache (IPC_FW_UPDATE_MBOX_ADDR , +sizeof (struct fw_update_mailbox )); +if (mailbox_base ==NULL ) { +iounmap (fw_update_base); +mutex_unlock (&ipclock); +return -ENOMEM ; + } + + mailbox = (struct fw_update_mailbox *)mailbox_base;
I think mailbox_base could/should have had type `struct fw_update_mailbox __iomem *'. ioremap_nocache() should handle that cleanly, and this cast goes away.
Otherwise, this cast is missing an __iomem.
And shouldn't `mailbox' have a __iomem too? It's all a bit confuddled.
arch/x86/kernel/intel_scu_ipc.c line 685
Quoted 6 lines
arch/x86/kernel/intel_scu_ipc.c new file +6
-0+ipc_command (IPC_CMD_FW_UPDATE_READY ); + +/* Intitialize mailbox */ + mailbox->status =0 ; + mailbox->scu_flag =0 ; + mailbox->driver_flag =0 ;
So this is driectly writing into iomem.
arch/x86/kernel/intel_scu_ipc.c line 694
Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c new file +8
-0+/* Driver copies the 2KB MIP header to SRAM at 0xFFFC0000*/ +memcpy_toio ((u8 *)(fw_update_base), buffer,0x800 ); + +/* Driver sends "FW Update" IPC command (CMD_ID 0xFE; MSG_ID 0x02). +* Upon receiving this command, SCU will write the 2K MIP header +* from 0xFFFC0000 into NAND. +* SCU will write a status code into the Mailbox, and then set scu_flag. +*/
Do we need to do something here to ensure that all the above writes have landed?
arch/x86/kernel/intel_scu_ipc.c line 698
Quoted 3 lines
arch/x86/kernel/intel_scu_ipc.c new file +3
-0+ipc_command (IPC_CMD_FW_UPDATE_GO ); + +/*Driver stalls until scu_flag is set */
Odd comment layout.
Quoted 4 lines
arch/x86/kernel/intel_scu_ipc.c new file +4
-0+while (mailbox->scu_flag !=1 ) { +rmb (); +mdelay (1 ); + }
arch/x86/kernel/intel_scu_ipc.c line 712
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+ +/* Driver checks Mailbox status. +* If the status is 'BADN', then abort (bad NAND). +* If the status is 'IPC_FW_TXLOW', then continue. +*/ +while (mailbox->status !=IPC_FW_TXLOW ) { +rmb (); +mdelay (10 ); + } +mdelay (10 );
Would be nice to explain the mysterious mdelay()s to the reader. What's it for? Why 10?
Quoted 20 lines
arch/x86/kernel/intel_scu_ipc.c new file +20
-0+update_retry: +if (retry_cnt >5 ) + goto update_end; + +if (mailbox->status !=IPC_FW_TXLOW ) + goto update_end; + buffer = buffer+0x800 ; +memcpy_toio ((u8 *)(fw_update_base), buffer,0x20000 ); + mailbox->driver_flag =0x1 ; +while (mailbox->scu_flag ==1 ) { +rmb (); +mdelay (1 ); + } + +/* check for 'BADN' */ +if (mailbox->status ==IPC_FW_UPDATE_BADN ) + goto update_end; + +while (mailbox->status !=IPC_FW_TXHIGH ) { +rmb ();
arch/x86/kernel/intel_scu_ipc.c line 743
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+mdelay (10 ); + } +mdelay (10 ); + +if (mailbox->status !=IPC_FW_TXHIGH ) + goto update_end; + buffer = buffer+0x20000 ; +memcpy_toio ((u8 *)(fw_update_base), buffer,0x20000 ); + mailbox->driver_flag =0 ; +while (mailbox->scu_flag ==0 ) {
Is there anything to prevent the compiler (or hardware?) from caching ->scu_flag from the previous read?
Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c new file +8
-0+rmb (); +mdelay (1 ); + } + +/* check for 'BADN' */ +if (mailbox->status ==IPC_FW_UPDATE_BADN ) + goto update_end; +
arch/x86/kernel/intel_scu_ipc.c line 761
Quoted 10 lines
arch/x86/kernel/intel_scu_ipc.c new file +10
-0+if (mailbox->status ==IPC_FW_TXLOW ) { + ++retry_cnt; + goto update_retry; + } + +update_end: +iounmap (fw_update_base); +iounmap (mailbox_base); +mutex_unlock (&ipclock); +if (mailbox->status ==IPC_FW_UPDATE_SUCCESS )
Confused. mailbox' equals mailbox_base', and `mailbox_base' just got iounmapped. Shouldn't this oops?
Quoted 7 lines
arch/x86/kernel/intel_scu_ipc.c new file +4
-0+return 0 ; +return -1 ; +} +EXPORT_SYMBOL (intel_scu_ipc_fw_update);
Quoted 2 lines
It's be nice to have some words or a link describing what an IPC actually _is_.
"Inter-process" ("or" I guess) "communication"
I'll go through the other stuff. The driver is a few versions on from the one you reviewed but quite a few of those problems are still present.
I'll then post up a new version to the platform driver list.
Alan
Quoted 2 lines
__packed, please. (I've requested that this be added to checkpatch, but Mr Checkpatch is asleep).
Done
Quoted 8 lines
Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c new file +8
-0+static inline int busy_loop (void )/* Wait till scu status is busy */ +{ +u32 status =0 ; +u32 loop_count =0 ; + + status =__raw_readl (IPC_STATUS_REG ); +while (status &1 ) { +udelay (1 );/* scu processing time is in few u secods */
Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c line 176
Quoted 5 lines
arch/x86/kernel/intel_scu_ipc.c new file +5
-0+ status =__raw_readl (IPC_STATUS_REG ); + loop_count++; +/* break if scu doesn't reset busy bit after huge retry */ +if (loop_count >100000 ) +return -ETIMEDOUT ;This function has seven-odd callsites and is waaaaaaaay to fat and slow to be inlined.
Looking at the asm I'm not convinced.
Quoted 7 lines
arch/x86/kernel/intel_scu_ipc.c line 232
Quoted 3 lines
arch/x86/kernel/intel_scu_ipc.c new file +3
-0+if (id ==IPC_CMD_PCNTRL_R ) {/* Read rbuf */ +/* Workaround: values are read as 0 without memcpy_fromio */ +memcpy_fromio (cbuf,IPC_READ_BUFFER ,16 );Should we still be doing this if
busy_loop()failed?(Lots of dittoes on this question)
Does no harm
Quoted 3 lines
Quoted 1 line
+}
I wonder if this function would look better if cbuf had type u16[],
No - we do byte aligned access to it. and cbuf[offset + 1.5] is sadly not supported by C (although I suspect you can make it work in C++ ;))
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ mutex_lock(&ipclock); + if (ipcdev.pdev == NULL) {This check happens a lot. Can it really happen?
Yes.
Quoted 3 lines
I think
mailbox_basecould/should have had type `structfw_update_mailbox__iomem*'.ioremap_nocache()should handle that cleanly, and this cast goes away.
Definitely - and the rest them cleans up by magic
Alan
Quoted 17 lines · On Thu, 22 Apr 2010 14:16:11 +0100 Alan Cox <[email protected]> wrote:
Quoted 8 lines
Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c new file +8
-0+static inline int busy_loop (void )/* Wait till scu status is busy */ +{ +u32 status =0 ; +u32 loop_count =0 ; + + status =__raw_readl (IPC_STATUS_REG ); +while (status &1 ) { +udelay (1 );/* scu processing time is in few u secods */ Quoted 8 lines
arch/x86/kernel/intel_scu_ipc.c line 176
Quoted 5 lines
arch/x86/kernel/intel_scu_ipc.c new file +5
-0+ status =__raw_readl (IPC_STATUS_REG ); + loop_count++; +/* break if scu doesn't reset busy bit after huge retry */ +if (loop_count >100000 ) +return -ETIMEDOUT ;This function has seven-odd callsites and is waaaaaaaay to fat and slow to be inlined.
Quoted 1 line
Looking at the asm I'm not convinced.
I tried two version of gcc and both of them just refuse to inline the function anyway.
If the function is changed to __always_inline, gcc will then inline it.
text data bss dec hex filename
inline 6182 960 1680 8822 2276 arch/x86/kernel/intel_scu_ipc.o __always_inline 6582 928 1760 9270 2436 arch/x86/kernel/intel_scu_ipc.o
I agree with gcc ;)