Date:	Wed, 21 Apr 2010 13:16:15 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Alan Cox <alan@...ux.intel.com>
Cc:	mingo@...e.hu, linux-kernel@...r.kernel.org,
	Sreedhara DS <sreedhara.ds@...el.com>
Subject: Re: [PATCH] IPC driver for Intel Mobile Internet Device (MID)
 platforms

On Fri, 09 Apr 2010 11:29:23 +0100
Alan Cox <alan@...ux.intel.com> 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/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 <sreedhara.ds@...el.com>
> 
> 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_.

>
> ...
> 
> +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).

>
> ...
> 
> +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.

> +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)
> +
> +/*
> + * 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?

>
> ...
> 
> +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 */
> +		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.

> +	}
> +	return (status >> 1) & 1;
> +}

This function has seven-odd callsites and is waaaaaaaay to fat and slow
to be inlined.

> +/* 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.

> +	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*/
> +		}
> +	}
> +
> +	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)

> +		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[],

>
> ...
> 
> +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?

> +		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);
> +
>
> ...
> 
> +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.

> +		*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;
> +}
>
> ...
> 
> +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.

> +	mutex_lock(&ipclock);
> +	fw_update_base = ioremap_nocache(IPC_FW_LOAD_ADDR, (128*1024));
> +	if (fw_update_base == NULL) {
> +		mutex_unlock(&ipclock);
> +		return -ENOMEM;
> +	}
> +	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.

> +	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.

> +	/* 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?

> +	ipc_command(IPC_CMD_FW_UPDATE_GO);
> +
> +	/*Driver stalls until scu_flag is set */

Odd comment layout.

> +	while (mailbox->scu_flag != 1) {
> +		rmb();
> +		mdelay(1);
> +	}
> +
> +	/* 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?

> +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();
> +		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?

> +		rmb();
> +		mdelay(1);
> +	}
> +
> +	/* check for 'BADN' */
> +	if (mailbox->status == IPC_FW_UPDATE_BADN)
> +		goto update_end;
> +
> +	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?

> +		return 0;
> +	return -1;
> +}
> +EXPORT_SYMBOL(intel_scu_ipc_fw_update);
>
> ...
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists