Topcliff PHUB: Generate PacketHub driver

Hi Arnd and Yong Y

We have updated phub patch about the following.
* Arnd's commnets
* Delete PCH_READ_REG/PCH_WRITE_REG
* Delete '_t' prefix
* Modify basic type
* Delete needless 'static' prefix
* Modify returned value
* Care returned value of get_user()
* Add .llseek line

* Yong Y's comments
* Applying to the latest checkpatch(2.6.35)
* Delete unused 'DEBUG' macro in Makefile
* Delete IEEE1588 lines
* Delete 'PCH_CAN_PCLK_50MHZ'

Thanks, Ohtake.

Kernel=2.6.33.1

Signed-off-by Masayuki Ohtake <[email protected]>

drivers/char/pch_phub/pch_phub.c new file +12 -0

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.

drivers/char/pch_phub/pch_phub.c new file +25 -0

+static void pch_phub_save_reg_conf(struct pci_dev *pdev)
+{
+	unsigned int i = 0;
+	void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf ENTRY\n");
+	/* to store contents of PHUB_ID register */
+	pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_PHUB_ID_REG);
+	/* to store contents of QUEUE_PRI_VAL register */
+	pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+	/* to store contents of RC_QUEUE_MAXSIZE register */
+	pch_phub_reg.rc_q_maxsize_reg =
+	    ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+	/* to store contents of BRI_QUEUE_MAXSIZE register */
+	pch_phub_reg.bri_q_maxsize_reg =
+	    ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+	/* to store contents of COMP_RESP_TIMEOUT register */
+	pch_phub_reg.comp_resp_timeout_reg =
+	    ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+	/* to store contents of BUS_SLAVE_CONTROL_REG register */
+	pch_phub_reg.bus_slave_control_reg =
+	    ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+	/* to store contents of DEADLOCK_AVOID_TYPE register */
+	pch_phub_reg.deadlock_avoid_type_reg =
+	    ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);

drivers/char/pch_phub/pch_phub.c new file +100 -0

+	pch_phub_reg.intpin_reg_wpermit_reg3 =
+	    ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+		"pch_phub_reg.phub_id_reg=%x, "
+		"pch_phub_reg.q_pri_val_reg=%x, "
+		"pch_phub_reg.rc_q_maxsize_reg=%x, "
+		"pch_phub_reg.bri_q_maxsize_reg=%x, "
+		"pch_phub_reg.comp_resp_timeout_reg=%x, "
+		"pch_phub_reg.bus_slave_control_reg=%x, "
+		"pch_phub_reg.deadlock_avoid_type_reg=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+		pch_phub_reg.phub_id_reg,
+		pch_phub_reg.q_pri_val_reg,
+		pch_phub_reg.rc_q_maxsize_reg,
+		pch_phub_reg.bri_q_maxsize_reg,
+		pch_phub_reg.comp_resp_timeout_reg,
+		pch_phub_reg.bus_slave_control_reg,
+		pch_phub_reg.deadlock_avoid_type_reg,
+		pch_phub_reg.intpin_reg_wpermit_reg0,
+		pch_phub_reg.intpin_reg_wpermit_reg1,
+		pch_phub_reg.intpin_reg_wpermit_reg2,
+		pch_phub_reg.intpin_reg_wpermit_reg3);
+	/* to store contents of INT_REDUCE_CONTROL registers */
+	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+		pch_phub_reg.int_reduce_control_reg[i] =
+		    ioread32(p +
+			PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+		dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+			"pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+			i, pch_phub_reg.int_reduce_control_reg[i]);
+	}
+	/* save clk cfg register */
+	pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
+
+	return;
+}
+
 
...
 
+static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
+{
+	unsigned int i;
+	void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+	dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+	/* to store contents of PHUB_ID register */
+	iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);
+	/* to store contents of QUEUE_PRI_VAL register */
+	iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+	/* to store contents of RC_QUEUE_MAXSIZE register */
+	iowrite32(pch_phub_reg.rc_q_maxsize_reg,
+					p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+	/* to store contents of BRI_QUEUE_MAXSIZE register */
+	iowrite32(pch_phub_reg.bri_q_maxsize_reg,
+					p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+	/* to store contents of COMP_RESP_TIMEOUT register */
+	iowrite32(pch_phub_reg.comp_resp_timeout_reg,
+					p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+	/* to store contents of BUS_SLAVE_CONTROL_REG register */
+	iowrite32(pch_phub_reg.bus_slave_control_reg,
+					p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+	/* to store contents of DEADLOCK_AVOID_TYPE register */
+	iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
+					p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+	/* to store contents of INTPIN_REG_WPERMIT register 0 */
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+	/* to store contents of INTPIN_REG_WPERMIT register 1 */
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+	/* to store contents of INTPIN_REG_WPERMIT register 2 */
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+	/* to store contents of INTPIN_REG_WPERMIT register 3 */
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+		"pch_phub_reg.phub_id_reg=%x, "
+		"pch_phub_reg.q_pri_val_reg=%x, "
+		"pch_phub_reg.rc_q_maxsize_reg=%x, "
+		"pch_phub_reg.bri_q_maxsize_reg=%x, "
+		"pch_phub_reg.comp_resp_timeout_reg=%x, "
+		"pch_phub_reg.bus_slave_control_reg=%x, "
+		"pch_phub_reg.deadlock_avoid_type_reg=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+		pch_phub_reg.phub_id_reg,
+		pch_phub_reg.q_pri_val_reg,
+		pch_phub_reg.rc_q_maxsize_reg,
+		pch_phub_reg.bri_q_maxsize_reg,
+		pch_phub_reg.comp_resp_timeout_reg,
+		pch_phub_reg.bus_slave_control_reg,
+		pch_phub_reg.deadlock_avoid_type_reg,
+		pch_phub_reg.intpin_reg_wpermit_reg0,
+		pch_phub_reg.intpin_reg_wpermit_reg1,
+		pch_phub_reg.intpin_reg_wpermit_reg2,
+		pch_phub_reg.intpin_reg_wpermit_reg3);

drivers/char/pch_phub/pch_phub.c new file +5 -0

+	iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
+
+	return;
+}
+

+ * @offset_address: Contains the Serial ROM address offset value
+ * @data: Contains the Serial ROM value
+ */

drivers/char/pch_phub/pch_phub.c new file +4 -0

+static int pch_phub_write_serial_rom(unsigned int offset_address,
+							 unsigned char data)
+{
+	int retval = 0;

drivers/char/pch_phub/pch_phub.c new file +44 -0

+	switch (offset_address % 4) {
+	case 0:
+		word_data &= 0xFFFFFF00;
+		iowrite32((word_data | (unsigned int)data),
+						mem_addr);
+		break;
+	case 1:
+		word_data &= 0xFFFF00FF;
+		iowrite32((word_data | ((unsigned int)data << 8)),
+						mem_addr);
+		break;
+	case 2:
+		word_data &= 0xFF00FFFF;
+		iowrite32((word_data | ((unsigned int)data << 16)),
+						mem_addr);
+		break;
+	case 3:
+		word_data &= 0x00FFFFFF;
+		iowrite32((word_data | ((unsigned int)data << 24)),
+						mem_addr);
+		break;
+	}
+	while (0x00 !=
+	       ioread8(pch_phub_reg.pch_phub_extrom_base_address +\
+							 PHUB_STATUS)) {
+		msleep(1);
+		if (PHUB_TIMEOUT == i) {
+			retval = -EPERM;
+			break;
+		}
+		i++;
+	}
+
+	iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
+			pch_phub_reg.pch_phub_extrom_base_address +\
+								 PHUB_CONTROL);
+
+	return retval;
+}
+
 
...
 
+static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size,
+								 loff_t *ppos)
+{
+	unsigned int rom_signature = 0;

drivers/char/pch_phub/pch_phub.c new file +14 -0

+	pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
+	pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
+	rom_signature |= (tmp & 0xff) << 8;
+	if (rom_signature == 0xAA55) {
+		pch_phub_read_serial_rom(0x82, &rom_length);
+		orom_size = rom_length * 512;
+		if (orom_size < pos)
+			return 0;
+
+		for (addr_offset = 0; addr_offset < size; addr_offset++) {
+			pch_phub_read_serial_rom(0x80 + addr_offset + pos,
+									 &data);
+			ret_value = copy_to_user(&buf[addr_offset], &data, 1);
+			if (ret_value)

drivers/char/pch_phub/pch_phub.c new file +12 -0

+	}
+	*ppos += addr_offset;
+	return addr_offset;
+}
+
+/** pch_phub_write - Implements the write functionality of the Packet Hub
+ *									 module.
+ *  @file: Contains the reference of the file structure
+ *  @buf: Usermode buffer pointer
+ *  @size: Usermode buffer size
+ *  @ppos: Contains the reference of the file structure
+ */

drivers/char/pch_phub/pch_phub.c new file +25 -0

+		if (PCH_PHUB_OROM_SIZE < pos + addr_offset) {
+			*ppos += addr_offset;
+			return addr_offset;
+		}
+
+	}
+
+	*ppos += addr_offset;
+	return addr_offset;
+}
+
+
+/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet
+ *								 Hub module.
+ *  @inode: Contains the reference of the inode structure
+ *  @file: Contains the reference of the file structure
+ *  @cmd: Contains the command value
+ *  @arg: Contains the command argument value
+ */
+
+
+static long pch_phub_ioctl(struct file *file, unsigned int cmd,
+							 unsigned long arg)
+{
+	int ret_value = 0;

drivers/char/pch_phub/pch_phub.c new file +4 -0

+		goto return_ioctrl;
+	}
+
+	switch (cmd) {

drivers/char/pch_phub/pch_phub.c new file +8 -0

+		for (i = 0; i < 6; i++)
+			pch_phub_write_gbe_mac_addr(i, mac_addr[i]);
+		break;
+
+	default:
+		ret_value = -EINVAL;
+		break;
+	}

drivers/char/pch_phub/pch_phub.c new file +7 -0

+static const struct file_operations pch_phub_fops = {
+	.owner = THIS_MODULE,
+	.read = pch_phub_read,
+	.write = pch_phub_write,
+	.unlocked_ioctl = pch_phub_ioctl,
+	.llseek = default_llseek
+};

drivers/char/pch_phub/pch_phub.c new file +11 -0

+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_dbg(&pdev->dev,
+				"\npch_phub_probe : pci_enable_device FAILED");
+		goto err_probe;
+	}
+	dev_dbg(&pdev->dev, "pch_phub_probe : "
+			"pci_enable_device returns %d\n", ret);
+
+	ret = pci_request_regions(pdev, DRIVER_NAME);
+	if (ret) {

drivers/char/pch_phub/pch_phub.c new file +9 -0

+		ret = -ENOMEM;
+		goto err_probe;
+	}
+	dev_dbg(&pdev->dev, "pch_phub_probe : "
+		"pci_map_rom SUCCESS and value in "
+		"pch_phub_extrom_base_address variable is 0x%08x\n",
+		(unsigned int)pch_phub_reg.pch_phub_extrom_base_address);
+
+	if (pch_phub_major_no) {

drivers/char/pch_phub/pch_phub.c new file +6 -0

+			pci_release_regions(pdev);
+			pci_disable_device(pdev);
+			goto err_probe;
+		}
+		dev_dbg(&pdev->dev, "pch_phub_probe : "
+				"register_chrdev_region returns %d\n", ret);

drivers/char/pch_phub/pch_phub.c new file +115 -0

+			pci_release_regions(pdev);
+			pci_disable_device(pdev);
+			goto err_probe;
+		}
+		dev_dbg(&pdev->dev, "pch_phub_probe : "
+			"alloc_chrdev_region returns %d\n", ret);
+	}
+
+	cdev_init(&pch_phub_dev, &pch_phub_fops);
+	dev_dbg(&pdev->dev,
+			"pch_phub_probe :  cdev_init invoked successfully\n");
+
+	pch_phub_dev.owner = THIS_MODULE;
+	pch_phub_dev.ops = &pch_phub_fops;
+
+	ret = cdev_add(&pch_phub_dev, pch_phub_dev_no, PCH_MINOR_NOS);
+	if (ret) {
+		dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add FAILED");
+		unregister_chrdev_region(pch_phub_dev_no, PCH_MINOR_NOS);
+		pci_unmap_rom(pdev,
+			(void *)pch_phub_reg.pch_phub_extrom_base_address);
+		pci_iounmap(pdev, (void *)pch_phub_reg.pch_phub_base_address);
+		pci_release_regions(pdev);
+		pci_disable_device(pdev);
+		goto err_probe;
+	}
+	dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add returns %d\n", ret);
+
+	/*set the clock config reg if CAN clock is 50Mhz */
+	dev_dbg(&pdev->dev, "pch_phub_probe : invoking "
+		"pch_phub_read_modify_write_reg "
+		"to set CLKCFG reg for CAN clk 50Mhz\n");
+	pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
+					  CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
+
+	/* set the prefech value */
+	iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
+	/* set the interrupt delay value */
+	iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
+	return 0;
+
+err_probe:
+	dev_dbg(&pdev->dev, "pch_phub_probe returns %d\n", ret);
+	return ret;
+}
+
 
...
 
+#ifdef CONFIG_PM
+
+/** pch_phub_suspend - Implements the suspend functionality of the module.
+ *  @pdev: Contains the reference of the pci_dev structure
+ *  @state: Contains the reference of the pm_message_t structure
+ */
+static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	int ret;
+
+	pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
+
+	pch_phub_save_reg_conf(pdev);
+	dev_dbg(&pdev->dev, "pch_phub_suspend - "
+		"pch_phub_save_reg_conf Invoked successfully\n");
+
+	ret = pci_save_state(pdev);
+	if (ret) {
+		dev_dbg(&pdev->dev,
+			" pch_phub_suspend -pci_save_state returns-%d\n", ret);
+		return ret;
+	}
+	dev_dbg(&pdev->dev,
+			"pch_phub_suspend - pci_save_state returns %d\n", ret);
+	pci_enable_wake(pdev, PCI_D3hot, 0);
+	dev_dbg(&pdev->dev, "pch_phub_suspend - "
+			"pci_enable_wake Invoked successfully\n");
+
+	pci_disable_device(pdev);
+	dev_dbg(&pdev->dev, "pch_phub_suspend - "
+			"pci_disable_device Invoked successfully\n");
+
+	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	dev_dbg(&pdev->dev, "pch_phub_suspend - "
+			"pci_set_power_state Invoked successfully   "
+			"return = %d\n", 0);
+
+	return 0;
+}
+
+/** pch_phub_resume - Implements the resume functionality of the module.
+ *  @pdev: Contains the reference of the pci_dev structure
+ */
+static int pch_phub_resume(struct pci_dev *pdev)
+{
+
+	int ret;
+
+	pci_set_power_state(pdev, PCI_D0);
+	dev_dbg(&pdev->dev, "pch_phub_resume - "
+		"pci_set_power_state Invoked successfully\n");
+
+	pci_restore_state(pdev);
+	dev_dbg(&pdev->dev, "pch_phub_resume - "
+		"pci_restore_state Invoked successfully\n");
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_dbg(&pdev->dev,
+				"pch_phub_resume-pci_enable_device failed ");
+		return ret;
+	}
+
+	dev_dbg(&pdev->dev, "pch_phub_resume - "
+			"pci_enable_device returns -%d\n", ret);
+
+	pci_enable_wake(pdev, PCI_D3hot, 0);
+	dev_dbg(&pdev->dev, "pch_phub_resume - "
+			"pci_enable_wake Invoked successfully\n");

drivers/char/pch_phub/pch_phub.c new file +8 -0

+#endif /* CONFIG_PM */
+
+static struct pci_device_id pch_phub_pcidev_id[] = {
+
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
+	{0,}
+};
+

drivers/char/pch_phub/pch_phub.h new file +17 -0

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+/*
+ * History:
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.

drivers/char/pch_phub/pch_phub.h new file +19 -0

+
+/* Registers address offset */
+#define PCH_PHUB_PHUB_ID_REG			0x0000
+#define PCH_PHUB_QUEUE_PRI_VAL_REG		0x0004
+#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG		0x0008
+#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG		0x000C
+#define PCH_PHUB_COMP_RESP_TIMEOUT_REG		0x0010
+#define PCH_PHUB_BUS_SLAVE_CONTROL_REG		0x0014
+#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG	0x0018
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0	0x0020
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1	0x0024
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2	0x0028
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3	0x002C
+#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE	0x0040
+#define CLKCFG_REG_OFFSET			0x500
+
+#define PCH_PHUB_OROM_SIZE 15360
+
+#endif

Quoted 18 lines · On Tue, 22 Jun 2010 19:33:34 +0900 Masayuki Ohtak <[email protected]>…

Hi Arnd and Yong Y

We have updated phub patch about the following.
* Arnd's commnets
* Delete PCH_READ_REG/PCH_WRITE_REG
* Delete '_t' prefix
* Modify basic type
* Delete needless 'static' prefix
* Modify returned value
* Care returned value of get_user()
* Add .llseek line

* Yong Y's comments
* Applying to the latest checkpatch(2.6.35)
* Delete unused 'DEBUG' macro in Makefile
* Delete IEEE1588 lines
* Delete 'PCH_CAN_PCLK_50MHZ'

Quoted 4 lines

Thanks, Ohtake.

Kernel=2.6.33.1

Signed-off-by Masayuki Ohtake <[email protected]>

Please prepare a proper changelog for the patch - one which is suitabe for inclusion in the kernel's permanent git record. Include that changelog with each version of the patch, perhaps after alterations.

Please describe the module parameters in that changelog. Please describe the major/minor number handling within the changelog - permitting the major number to be specified on the modprobe command line is unusual and should be fully described and justified, please.

Please ensure that the changelog tells us what the driver actually does! I don't even know what a "PCH packet hub" _is_ :(

drivers/char/pch_phub/pch_phub.c line 6

Quoted 12 lines

...

drivers/char/pch_phub/pch_phub.c new file +6 -0

@@ -0,0 +1,937 @@
+/*!
+ * @file pch_phub.c
+ * @brief Provides all the implementation of the interfaces pertaining to
+ * the Packet Hub module.
+ * @version 1.0.0.0
+ * @section

This appears to use a markup format whcih the kernel doesn't implement.

Quoted 12 lines

drivers/char/pch_phub/pch_phub.c new file +12 -0

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.

drivers/char/pch_phub/pch_phub.c

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +7 -0

+ */
 
...
 
+/** pch_phub_read_modify_write_reg - Implements the functionality of reading,
+ * modifying and writing register.
+ *  @reg_addr_offset:  Contains the register offset address value
+ *  @data:             Contains the writing value
+ *  @mask:             Contains the mask value
+ */

kerneldoc comments are usually formatted as

/**
* foo() - do something
* @arg1 ...

I don't know whether the layout whcih this driver uses will be properly handled by the kerneldoc tools, but I'd suggest that it all be converted to the more usual style for consistency.

drivers/char/pch_phub/pch_phub.c line 142

Quoted 12 lines

drivers/char/pch_phub/pch_phub.c new file +12 -0

+static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
+        unsigned int data, unsigned int mask)
+{
+ void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +\
+ reg_addr_offset;
+ iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
+ return;
+}
+
+
+/** pch_phub_save_reg_conf - saves register configuration
+ */

The driver has quite a lot of comments which use the kerneldoc toke "/**" but which don't really look like they wre intended to be kerneldoc comments. So I'd suggest converting these to plain old comments:

/*
* pch_phub_save_reg_conf - saves register configuration */

or

/* pch_phub_save_reg_conf - saves register configuration */

or finish off the kerneldoc work by documenting the arguments (and the return values, please).

Quoted 25 lines

drivers/char/pch_phub/pch_phub.c new file +25 -0

+static void pch_phub_save_reg_conf(struct pci_dev *pdev)
+{
+ unsigned int i = 0;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+ dev_dbg(&pdev->dev, "pch_phub_save_reg_conf ENTRY\n");
+ /* to store contents of PHUB_ID register */
+ pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_PHUB_ID_REG);
+ /* to store contents of QUEUE_PRI_VAL register */
+ pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+ /* to store contents of RC_QUEUE_MAXSIZE register */
+ pch_phub_reg.rc_q_maxsize_reg =
+     ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+ /* to store contents of BRI_QUEUE_MAXSIZE register */
+ pch_phub_reg.bri_q_maxsize_reg =
+     ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+ /* to store contents of COMP_RESP_TIMEOUT register */
+ pch_phub_reg.comp_resp_timeout_reg =
+     ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+ /* to store contents of BUS_SLAVE_CONTROL_REG register */
+ pch_phub_reg.bus_slave_control_reg =
+     ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+ /* to store contents of DEADLOCK_AVOID_TYPE register */
+ pch_phub_reg.deadlock_avoid_type_reg =
+     ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);

drivers/char/pch_phub/pch_phub.c line 177

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ /* to store contents of INTPIN_REG_WPERMIT register 0 */
+ pch_phub_reg.intpin_reg_wpermit_reg0 =
+     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+ /* to store contents of INTPIN_REG_WPERMIT register 1 */
+ pch_phub_reg.intpin_reg_wpermit_reg1 =
+     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+ /* to store contents of INTPIN_REG_WPERMIT register 2 */
+ pch_phub_reg.intpin_reg_wpermit_reg2 =
+     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+ /* to store contents of INTPIN_REG_WPERMIT register 3 */

s/to //g

Quoted 103 lines

+ pch_phub_reg.intpin_reg_wpermit_reg3 =

drivers/char/pch_phub/pch_phub.c new file +99 -0

+     ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+ dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+ "pch_phub_reg.phub_id_reg=%x, "
+ "pch_phub_reg.q_pri_val_reg=%x, "
+ "pch_phub_reg.rc_q_maxsize_reg=%x, "
+ "pch_phub_reg.bri_q_maxsize_reg=%x, "
+ "pch_phub_reg.comp_resp_timeout_reg=%x, "
+ "pch_phub_reg.bus_slave_control_reg=%x, "
+ "pch_phub_reg.deadlock_avoid_type_reg=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+ pch_phub_reg.phub_id_reg,
+ pch_phub_reg.q_pri_val_reg,
+ pch_phub_reg.rc_q_maxsize_reg,
+ pch_phub_reg.bri_q_maxsize_reg,
+ pch_phub_reg.comp_resp_timeout_reg,
+ pch_phub_reg.bus_slave_control_reg,
+ pch_phub_reg.deadlock_avoid_type_reg,
+ pch_phub_reg.intpin_reg_wpermit_reg0,
+ pch_phub_reg.intpin_reg_wpermit_reg1,
+ pch_phub_reg.intpin_reg_wpermit_reg2,
+ pch_phub_reg.intpin_reg_wpermit_reg3);
+ /* to store contents of INT_REDUCE_CONTROL registers */
+ for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+ pch_phub_reg.int_reduce_control_reg[i] =
+     ioread32(p +
+ PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+ dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+ "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+ i, pch_phub_reg.int_reduce_control_reg[i]);
+ }
+ /* save clk cfg register */
+ pch_phub_reg.clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
+
+ return;
+}
+
 
...
 
+static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
+{
+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+ dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+ /* to store contents of PHUB_ID register */
+ iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);
+ /* to store contents of QUEUE_PRI_VAL register */
+ iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+ /* to store contents of RC_QUEUE_MAXSIZE register */
+ iowrite32(pch_phub_reg.rc_q_maxsize_reg,
+ p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+ /* to store contents of BRI_QUEUE_MAXSIZE register */
+ iowrite32(pch_phub_reg.bri_q_maxsize_reg,
+ p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+ /* to store contents of COMP_RESP_TIMEOUT register */
+ iowrite32(pch_phub_reg.comp_resp_timeout_reg,
+ p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+ /* to store contents of BUS_SLAVE_CONTROL_REG register */
+ iowrite32(pch_phub_reg.bus_slave_control_reg,
+ p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+ /* to store contents of DEADLOCK_AVOID_TYPE register */
+ iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
+ p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+ /* to store contents of INTPIN_REG_WPERMIT register 0 */
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+ /* to store contents of INTPIN_REG_WPERMIT register 1 */
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+ /* to store contents of INTPIN_REG_WPERMIT register 2 */
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+ /* to store contents of INTPIN_REG_WPERMIT register 3 */
+ iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
+ p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+ dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+ "pch_phub_reg.phub_id_reg=%x, "
+ "pch_phub_reg.q_pri_val_reg=%x, "
+ "pch_phub_reg.rc_q_maxsize_reg=%x, "
+ "pch_phub_reg.bri_q_maxsize_reg=%x, "
+ "pch_phub_reg.comp_resp_timeout_reg=%x, "
+ "pch_phub_reg.bus_slave_control_reg=%x, "
+ "pch_phub_reg.deadlock_avoid_type_reg=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+ "pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+ pch_phub_reg.phub_id_reg,
+ pch_phub_reg.q_pri_val_reg,
+ pch_phub_reg.rc_q_maxsize_reg,
+ pch_phub_reg.bri_q_maxsize_reg,
+ pch_phub_reg.comp_resp_timeout_reg,
+ pch_phub_reg.bus_slave_control_reg,
+ pch_phub_reg.deadlock_avoid_type_reg,
+ pch_phub_reg.intpin_reg_wpermit_reg0,
+ pch_phub_reg.intpin_reg_wpermit_reg1,
+ pch_phub_reg.intpin_reg_wpermit_reg2,
+ pch_phub_reg.intpin_reg_wpermit_reg3);

drivers/char/pch_phub/pch_phub.c line 290

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ /* to store contents of INT_REDUCE_CONTROL register */
+ for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+ iowrite32(pch_phub_reg.int_reduce_control_reg[i],
+ p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+ dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+ "pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+ i, pch_phub_reg.int_reduce_control_reg[i]);
+ }
+
+ /*restore the clock config reg */

One space after the /*, please.

Quoted 5 lines

+ iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);

drivers/char/pch_phub/pch_phub.c new file +4 -0

+
+ return;
+}
+

drivers/char/pch_phub/pch_phub.c line 305

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+/** pch_phub_read_serial_rom - Implements the functionality of reading Serial
+ * ROM.
+ *  @offset_address: Contains the Serial ROM address offset value
+ *  @data: Contains the Serial ROM value
+ */
+static int pch_phub_read_serial_rom(unsigned int offset_address,
+ unsigned char *data)
+{
+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
+ offset_address;

Unneeded \. There are several instances of this in the driver.

drivers/char/pch_phub/pch_phub.c line 313

Quoted 7 lines

+ *data = ioread8(mem_addr);

drivers/char/pch_phub/pch_phub.c new file +6 -0

+
+ return 0;
+}
+
+/** pch_phub_write_serial_rom - Implements the functionality of writing Serial
+ * ROM.

Strange layout. I don't know what this will look like after kerneldoc processing - it might make a mess. Conventional layout is

/**
* pch_phub_write_serial_rom - Implements the functionality of writing Serial ROM.

or

/**
* pch_phub_write_serial_rom - Implements the functionality of writing Serial
* ROM.

(there are several instances of this).

Quoted 7 lines

+ * @offset_address: Contains the Serial ROM address offset value
+ * @data: Contains the Serial ROM value
+ */

drivers/char/pch_phub/pch_phub.c new file +4 -0

+static int pch_phub_write_serial_rom(unsigned int offset_address,
+ unsigned char data)
+{
+ int retval = 0;

drivers/char/pch_phub/pch_phub.c line 330

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
+ (offset_address & 0xFFFFFFFC);
+ int i = 0;
+ unsigned int word_data = 0;
+
+ iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
+ pch_phub_reg.pch_phub_extrom_base_address +\
+ PHUB_CONTROL);
+
+ word_data = ioread32(mem_addr);

The driver tends to do

int foo = 0;

...

foo = ...

in quite a lot of places. The initialisation to zero is harmless - the compiler will take it out again. But it's unusual and jsut adds noise.

The compiler will warn about use of uninitialised variables anyway. In fact this unnecessary initalisation will suppress compiler warnings which might have revealed real bugs. I'd suggest that they all be removed (there are quite a lot in this driver).

Quoted 47 lines

+ switch (offset_address % 4) {
+ case 0:
+ word_data &= 0xFFFFFF00;
+ iowrite32((word_data | (unsigned int)data),
+ mem_addr);
+ break;
+ case 1:
+ word_data &= 0xFFFF00FF;
+ iowrite32((word_data | ((unsigned int)data << 8)),
+ mem_addr);
+ break;
+ case 2:
+ word_data &= 0xFF00FFFF;
+ iowrite32((word_data | ((unsigned int)data << 16)),
+ mem_addr);
+ break;
+ case 3:
+ word_data &= 0x00FFFFFF;
+ iowrite32((word_data | ((unsigned int)data << 24)),
+ mem_addr);
+ break;
+ }
+ while (0x00 !=

drivers/char/pch_phub/pch_phub.c new file +21 -0

+        ioread8(pch_phub_reg.pch_phub_extrom_base_address +\
+ PHUB_STATUS)) {
+ msleep(1);
+ if (PHUB_TIMEOUT == i) {
+ retval = -EPERM;
+ break;
+ }
+ i++;
+ }
+
+ iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
+ pch_phub_reg.pch_phub_extrom_base_address +\
+ PHUB_CONTROL);
+
+ return retval;
+}
+
 
...
 
+static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size,
+ loff_t *ppos)
+{
+ unsigned int rom_signature = 0;

drivers/char/pch_phub/pch_phub.c

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ unsigned char rom_length;
+ int ret_value;
+ unsigned int tmp;
+ unsigned char data;
+ unsigned int addr_offset = 0;
+ unsigned int orom_size;
+ loff_t pos = *ppos;
+
+ if (pos < 0)
+ return -EINVAL;

I think vfs_read() -> rw_verify_area() already did that, so indovidual ->read() implementations don't need to check for negative offset.

Quoted 1 line

+ /*Get Rom signature*/

Spaces after /* and before */, please.

Quoted 14 lines

+ pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
+ pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);
+ rom_signature |= (tmp & 0xff) << 8;
+ if (rom_signature == 0xAA55) {
+ pch_phub_read_serial_rom(0x82, &rom_length);
+ orom_size = rom_length * 512;
+ if (orom_size < pos)
+ return 0;

drivers/char/pch_phub/pch_phub.c new file +6 -0

+
+ for (addr_offset = 0; addr_offset < size; addr_offset++) {
+ pch_phub_read_serial_rom(0x80 + addr_offset + pos,
+ &data);
+ ret_value = copy_to_user(&buf[addr_offset], &data, 1);
+ if (ret_value)

drivers/char/pch_phub/pch_phub.c line 525

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ return -EFAULT;
+
+ if (orom_size < pos + addr_offset) {
+ *ppos += addr_offset;
+ return addr_offset;
+ }
+
+ }
+ } else {
+ return -ENOEXEC;

ENOEXEC means "your executable file doesn't have suitable contents". If this error gets propagated to userspace the poor user will be wondering who corrupted his ELF files and would be surprised to find out that the error in fact came from his packethub driver.

So please use a more appropriate errno here.

Quoted 12 lines

+ }
+ *ppos += addr_offset;
+ return addr_offset;

drivers/char/pch_phub/pch_phub.c new file +9 -0

+}
+
+/** pch_phub_write - Implements the write functionality of the Packet Hub
+ * module.
+ *  @file: Contains the reference of the file structure
+ *  @buf: Usermode buffer pointer
+ *  @size: Usermode buffer size
+ *  @ppos: Contains the reference of the file structure
+ */

drivers/char/pch_phub/pch_phub.c line 547

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+static ssize_t pch_phub_write(struct file *file, const char __user *buf,
+ size_t size, loff_t *ppos)
+{
+ unsigned int data;
+ int ret_value;
+ unsigned int addr_offset = 0;
+ loff_t pos = *ppos;
+
+ if (pos < 0)
+ return -EINVAL;

vfs_write() already did that.

Quoted 9 lines

+ for (addr_offset = 0; addr_offset < size; addr_offset++) {
+ ret_value = get_user(data, &buf[addr_offset]);
+ if (ret_value)
+ return -EFAULT;
+
+ ret_value = pch_phub_write_serial_rom(0x80 + addr_offset + pos,
+ data);
+ if (ret_value)
+ return -EIO;

This overwrites the pch_phub_write_serial_rom() return value. it's generally better to propagate error return values instead of replacing them with different ones.

otoh pch_phub_write_serial_rom() can return -EPERM. "Operation not permitted. An attempt was made to perform an operation limited to processes with appropriate privileges or to the owner of a file or other resource.". That doesn't sound like an appropriate errno for a driver to use?

Quoted 25 lines

+ if (PCH_PHUB_OROM_SIZE < pos + addr_offset) {
+ *ppos += addr_offset;
+ return addr_offset;
+ }

drivers/char/pch_phub/pch_phub.c new file +21 -0

+
+ }
+
+ *ppos += addr_offset;
+ return addr_offset;
+}
+
+
+/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet
+ * Hub module.
+ *  @inode: Contains the reference of the inode structure
+ *  @file: Contains the reference of the file structure
+ *  @cmd: Contains the command value
+ *  @arg: Contains the command argument value
+ */
+
+
+static long pch_phub_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ int ret_value = 0;

drivers/char/pch_phub/pch_phub.c line 593

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ __u8 mac_addr[6];
+ int ret;
+ unsigned int i;
+ void __user *varg = (void __user *)arg;
+
+ ret = mutex_lock_interruptible(&pch_phub_ioctl_mutex);
+ if (ret) {
+ ret_value = -ERESTARTSYS;
+ goto return_nomutex;
+ }

A plain old `return -ERESTARTSYS' is OK here.

Quoted 2 lines

+ if (pch_phub_reg.pch_phub_suspended == true) {
+ ret_value = -EPERM;

EPERM?

Quoted 4 lines

+ goto return_ioctrl;
+ }

drivers/char/pch_phub/pch_phub.c new file +2 -0

+
+ switch (cmd) {

drivers/char/pch_phub/pch_phub.c line 610

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ case IOCTL_PHUB_READ_MAC_ADDR:
+ for (i = 0; i < 6; i++)
+ pch_phub_read_gbe_mac_addr(i, &mac_addr[i]);
+
+ ret_value = copy_to_user(varg,
+ mac_addr, sizeof(mac_addr));
+ if (ret_value) {
+ ret_value = -EFAULT;
+ goto return_ioctrl;
+ }

The goto is unneeded.

drivers/char/pch_phub/pch_phub.c line 619

Quoted 9 lines

+ break;

drivers/char/pch_phub/pch_phub.c new file +8 -0

+
+ case IOCTL_PHUB_WRITE_MAC_ADDR:
+ ret_value = copy_from_user(mac_addr, varg, sizeof(mac_addr));
+
+ if (ret_value) {
+ ret_value = -EFAULT;
+ goto return_ioctrl;
+ }

Could do a `break'.

Quoted 8 lines

+ for (i = 0; i < 6; i++)
+ pch_phub_write_gbe_mac_addr(i, mac_addr[i]);
+ break;

drivers/char/pch_phub/pch_phub.c new file +5 -0

+
+ default:
+ ret_value = -EINVAL;
+ break;
+ }

drivers/char/pch_phub/pch_phub.c line 637

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+return_ioctrl:
+ mutex_unlock(&pch_phub_ioctl_mutex);
+return_nomutex:
+ return ret_value;
+}
+
+
+/**
+ * file_operations structure initialization
+ */

That's not a kerneldoc comment, but it has the /** kerneldoc token.

Quoted 7 lines

drivers/char/pch_phub/pch_phub.c new file +7 -0

+static const struct file_operations pch_phub_fops = {
+ .owner = THIS_MODULE,
+ .read = pch_phub_read,
+ .write = pch_phub_write,
+ .unlocked_ioctl = pch_phub_ioctl,
+ .llseek = default_llseek
+};

drivers/char/pch_phub/pch_phub.c line 654

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+
+
+/** pch_phub_probe - Implements the probe functionality of the module.
+ *  @pdev: Contains the reference of the pci_dev structure
+ *  @id: Contains the reference of the pci_device_id structure
+ */
+static int __devinit pch_phub_probe(struct pci_dev *pdev,
+        const struct pci_device_id *id)
+{
+ char *DRIVER_NAME = "pch_phub";

hm, why was that upper case?

Maybe use MODULE_NAME instead? Or

static const char driver_name[] = "pch_phub";

Quoted 5 lines

+ int ret;
+ unsigned int rom_size;
+
+ pch_phub_major_no = (pch_phub_major_no < 0 || pch_phub_major_no > 254) ?
+ 0 : pch_phub_major_no;

This looks odd - should be explained in code comments and/or changelog.

Quoted 11 lines

+ ret = pci_enable_device(pdev);
+ if (ret) {
+ dev_dbg(&pdev->dev,
+ "\npch_phub_probe : pci_enable_device FAILED");
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "pci_enable_device returns %d\n", ret);

drivers/char/pch_phub/pch_phub.c new file +3 -0

+
+ ret = pci_request_regions(pdev, DRIVER_NAME);
+ if (ret) {

drivers/char/pch_phub/pch_phub.c line 681

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ dev_dbg(&pdev->dev,
+ "pch_phub_probe : pci_request_regions FAILED");
+ pci_disable_device(pdev);
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "pci_request_regions returns %d\n", ret);
+
+ pch_phub_reg.pch_phub_base_address = \
+ (void __iomem *)pci_iomap(pdev, 1, 0);

Unneeded and undesirable typeast.

drivers/char/pch_phub/pch_phub.c line 695

Quoted 13 lines

+ if (pch_phub_reg.pch_phub_base_address == 0) {
+ dev_dbg(&pdev->dev, "pch_phub_probe : pci_iomap FAILED");
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+ ret = -ENOMEM;
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : pci_iomap SUCCESS and value "
+ "in pch_phub_base_address variable is 0x%08x\n",
+ (unsigned int)pch_phub_reg.pch_phub_base_address);

drivers/char/pch_phub/pch_phub.c new file +3 -0

+
+ pch_phub_reg.pch_phub_extrom_base_address =
+     (void __iomem *)pci_map_rom(pdev, &rom_size);

Ditto

Quoted 5 lines

+ if (pch_phub_reg.pch_phub_extrom_base_address == 0) {
+ dev_dbg(&pdev->dev, "pch_phub_probe : pci_map_rom FAILED");
+ pci_iounmap(pdev, (void *)pch_phub_reg.pch_phub_base_address);
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);

This is getting painful. I'd suggest removal of the duplicated code via the standard `goto out_iounmap' unwinding approach.

Quoted 9 lines

+ ret = -ENOMEM;
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "pci_map_rom SUCCESS and value in "
+ "pch_phub_extrom_base_address variable is 0x%08x\n",
+ (unsigned int)pch_phub_reg.pch_phub_extrom_base_address);

drivers/char/pch_phub/pch_phub.c new file +2 -0

+
+ if (pch_phub_major_no) {

drivers/char/pch_phub/pch_phub.c line 719

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ pch_phub_dev_no = MKDEV(pch_phub_major_no, 0);
+ ret = register_chrdev_region(pch_phub_dev_no,
+    PCH_MINOR_NOS, DRIVER_NAME);
+ if (ret) {
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "register_chrdev_region FAILED");
+ pci_unmap_rom(pdev,
+ (void *)pch_phub_reg.pch_phub_extrom_base_address);
+ pci_iounmap(pdev,
+ (void *)pch_phub_reg.pch_phub_base_address);

More unneeded typecasts.

Quoted 8 lines

+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "register_chrdev_region returns %d\n", ret);
+ } else {
+ ret = alloc_chrdev_region(&pch_phub_dev_no, 0,

Quoted 8 lines

+ PCH_MINOR_NOS, DRIVER_NAME);
+ if (ret) {
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "alloc_chrdev_region FAILED");
+ pci_unmap_rom(pdev,
+ (void *)pch_phub_reg.pch_phub_extrom_base_address);
+ pci_iounmap(pdev,
+ (void *)pch_phub_reg.pch_phub_base_address);

more..

Quoted 118 lines

+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "alloc_chrdev_region returns %d\n", ret);
+ }

drivers/char/pch_phub/pch_phub.c new file +108 -0

+
+ cdev_init(&pch_phub_dev, &pch_phub_fops);
+ dev_dbg(&pdev->dev,
+ "pch_phub_probe :  cdev_init invoked successfully\n");
+
+ pch_phub_dev.owner = THIS_MODULE;
+ pch_phub_dev.ops = &pch_phub_fops;
+
+ ret = cdev_add(&pch_phub_dev, pch_phub_dev_no, PCH_MINOR_NOS);
+ if (ret) {
+ dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add FAILED");
+ unregister_chrdev_region(pch_phub_dev_no, PCH_MINOR_NOS);
+ pci_unmap_rom(pdev,
+ (void *)pch_phub_reg.pch_phub_extrom_base_address);
+ pci_iounmap(pdev, (void *)pch_phub_reg.pch_phub_base_address);
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
+ goto err_probe;
+ }
+ dev_dbg(&pdev->dev, "pch_phub_probe :  cdev_add returns %d\n", ret);
+
+ /*set the clock config reg if CAN clock is 50Mhz */
+ dev_dbg(&pdev->dev, "pch_phub_probe : invoking "
+ "pch_phub_read_modify_write_reg "
+ "to set CLKCFG reg for CAN clk 50Mhz\n");
+ pch_phub_read_modify_write_reg((unsigned int)CLKCFG_REG_OFFSET,
+   CLKCFG_CAN_50MHZ, CLKCFG_CANCLK_MASK);
+
+ /* set the prefech value */
+ iowrite32(0x000ffffa, pch_phub_reg.pch_phub_base_address + 0x14);
+ /* set the interrupt delay value */
+ iowrite32(0x25, pch_phub_reg.pch_phub_base_address + 0x44);
+ return 0;
+
+err_probe:
+ dev_dbg(&pdev->dev, "pch_phub_probe returns %d\n", ret);
+ return ret;
+}
+
 
...
 
+#ifdef CONFIG_PM
+
+/** pch_phub_suspend - Implements the suspend functionality of the module.
+ *  @pdev: Contains the reference of the pci_dev structure
+ *  @state: Contains the reference of the pm_message_t structure
+ */
+static int pch_phub_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+ int ret;
+
+ pch_phub_reg.pch_phub_suspended = true;/* For blocking further IOCTLs */
+
+ pch_phub_save_reg_conf(pdev);
+ dev_dbg(&pdev->dev, "pch_phub_suspend - "
+ "pch_phub_save_reg_conf Invoked successfully\n");
+
+ ret = pci_save_state(pdev);
+ if (ret) {
+ dev_dbg(&pdev->dev,
+ " pch_phub_suspend -pci_save_state returns-%d\n", ret);
+ return ret;
+ }
+ dev_dbg(&pdev->dev,
+ "pch_phub_suspend - pci_save_state returns %d\n", ret);
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ dev_dbg(&pdev->dev, "pch_phub_suspend - "
+ "pci_enable_wake Invoked successfully\n");
+
+ pci_disable_device(pdev);
+ dev_dbg(&pdev->dev, "pch_phub_suspend - "
+ "pci_disable_device Invoked successfully\n");
+
+ pci_set_power_state(pdev, pci_choose_state(pdev, state));
+ dev_dbg(&pdev->dev, "pch_phub_suspend - "
+ "pci_set_power_state Invoked successfully   "
+ "return = %d\n", 0);
+
+ return 0;
+}
+
+/** pch_phub_resume - Implements the resume functionality of the module.
+ *  @pdev: Contains the reference of the pci_dev structure
+ */
+static int pch_phub_resume(struct pci_dev *pdev)
+{
+
+ int ret;
+
+ pci_set_power_state(pdev, PCI_D0);
+ dev_dbg(&pdev->dev, "pch_phub_resume - "
+ "pci_set_power_state Invoked successfully\n");
+
+ pci_restore_state(pdev);
+ dev_dbg(&pdev->dev, "pch_phub_resume - "
+ "pci_restore_state Invoked successfully\n");
+
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ dev_dbg(&pdev->dev,
+ "pch_phub_resume-pci_enable_device failed ");
+ return ret;
+ }
+
+ dev_dbg(&pdev->dev, "pch_phub_resume - "
+ "pci_enable_device returns -%d\n", ret);
+
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ dev_dbg(&pdev->dev, "pch_phub_resume - "
+ "pci_enable_wake Invoked successfully\n");

drivers/char/pch_phub/pch_phub.c line 891

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+
+ pch_phub_restore_reg_conf(pdev);
+ dev_dbg(&pdev->dev, "pch_phub_resume - "
+ "pch_phub_restore_reg_conf Invoked successfully\n");
+
+ pch_phub_reg.pch_phub_suspended = false;
+
+ dev_dbg(&pdev->dev, "pch_phub_resume  returns- %d\n", 0);
+ return 0;
+}

Here you can put

#else
#define pch_phub_suspend NULL
#define pch_phub_resume NULL

Quoted 8 lines

drivers/char/pch_phub/pch_phub.c new file +8 -0

+#endif /* CONFIG_PM */
+
+static struct pci_device_id pch_phub_pcidev_id[] = {
+
+ {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH1_PHUB)},
+ {0,}
+};
+

drivers/char/pch_phub/pch_phub.c line 909

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+
+static struct pci_driver pch_phub_driver = {
+ .name = "pch_phub",
+ .id_table = pch_phub_pcidev_id,
+ .probe = pch_phub_probe,
+ .remove = __devexit_p(pch_phub_remove),
+#ifdef CONFIG_PM
+ .suspend = pch_phub_suspend,
+ .resume = pch_phub_resume
+#endif

and then remove this ifdef.

drivers/char/pch_phub/pch_phub.h line 7

Quoted 13 lines

...

drivers/char/pch_phub/pch_phub.h new file +7 -0

@@ -0,0 +1,58 @@
+#ifndef __PCH_PHUB_H__
+#define __PCH_PHUB_H__
+/*!
+ * @file pch_phub.h
+ * @brief Provides all the interfaces pertaining to the Packet Hub module.
+ * @version 1.0.0.0
+ * @section

?

Quoted 17 lines

drivers/char/pch_phub/pch_phub.h new file +17 -0

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+/*
+ * History:
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.

drivers/char/pch_phub/pch_phub.h line 34

Quoted 10 lines

drivers/char/pch_phub/pch_phub.h new file +10 -0

+ *
+ * created:
+ * OKI SEMICONDUCTOR 04/14/2010
+ * modified:
+ *
+ */
+
+#define PHUB_IOCTL_MAGIC (0xf7)
+
+/*Outlines the read mac address function signature. */

Space after /*, please (check the whole patch)

drivers/char/pch_phub/pch_phub.h line 38

Quoted 4 lines

drivers/char/pch_phub/pch_phub.h new file +4 -0

+#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 6, __u8[6]))
+
+/*brief Outlines the write mac address function signature. */
+#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u8[6]))

"brief"?

I didn't notice any documentation for these ioctls anywhere?

Quoted 19 lines

drivers/char/pch_phub/pch_phub.h new file +19 -0

+
+/* Registers address offset */
+#define PCH_PHUB_PHUB_ID_REG 0x0000
+#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004
+#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008
+#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C
+#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010
+#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014
+#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028
+#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C
+#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040
+#define CLKCFG_REG_OFFSET 0x500
+
+#define PCH_PHUB_OROM_SIZE 15360
+
+#endif

Hi, Arnd and Yong Y

I have released Phub patch file.
Please confirm below.

Signed-off-by Masayuki Ohtake <[email protected]>

Quoted 4 lines · Masayuki Ohtak

Hi, Arnd and Yong Y

I have released Phub patch file.
Please confirm below.

You have addressed all of my comments, the driver looks ready for inclusion from my perspective.

Quoted 1 line

Signed-off-by Masayuki Ohtake <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

+/*!
+ * @file pch_phub.c
+ * @brief Provides all the implementation of the interfaces pertaining to
+ * the Packet Hub module.

+/*
+ * History:
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * created:
+ * OKI SEMICONDUCTOR 04/14/2010
+ * modified:

+/* includes */

+/** pch_phub_restore_reg_conf - restore register configuration
+ */

+/** pch_phub_read_serial_rom - Implements the functionality of reading Serial

+ *  									 ROM.
+ *  @offset_address: Contains the Serial ROM address offset value
+ *  @data: Contains the Serial ROM value

+ */

drivers/char/pch_phub/pch_phub.c new file +11 -0

+static int pch_phub_write_serial_rom(unsigned int offset_address,
+							 unsigned char data)
+{
+	int retval = 0;
+	void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
+						 (offset_address & 0xFFFFFFFC);
+	int i = 0;
+	unsigned int word_data = 0;
+
+	iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
+			pch_phub_reg.pch_phub_extrom_base_address +\

drivers/char/pch_phub/pch_phub.c new file +8 -0

+static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size,
+								 loff_t *ppos)
+{
+	unsigned int rom_signature = 0;
+	unsigned char rom_length;
+	int ret_value;
+	unsigned int tmp;
+	unsigned char data;

drivers/char/Kconfig line 13

Quoted 7 lines

drivers/char/Kconfig +7 -0

+config PCH_PHUB
+ tristate "PCH PHUB"
+ depends on PCI
+ help
+   If you say yes to this option, support will be included for the
+   PCH Packet Hub Host controller.
+   This driver is for PCH Packet hub driver for Topcliff.

"Topcliff" is probably some kind of internal codename; please use a name

that will be visible to customers of this product. References to what kind of device (IEEE standards it implements, what systems it might be present on, etc) are also appropriate.

Quoted 2 lines

+ This driver is integrated as built-in.

This sentence doesn't parse to me. What are you trying to say?

drivers/char/Kconfig line 16

Quoted 2 lines

drivers/char/Kconfig +2 -0

+   This driver can access GbE MAC address.
+   This driver can access HW register.

I think you mean something more like "This driver allows access to the GbE MAC address and HW registers of the PCH Packet Hub."

If this is a driver for an Ethernet MAC, what is it doing in drivers/char/ ?

Quoted 2 lines

+ You can also be integrated as module.

Please look at any other tristate and copy the standard wording.

Quoted 5 lines

+/*!
+ * @file pch_phub.c
+ * @brief Provides all the implementation of the interfaces pertaining to
+ * the Packet Hub module.

We don't normally merge comment markup languages other than kernel-doc. Please read Documentation/kernel-doc-nano-HOWTO.txt and follow it. (Or, provide a pointer to documentation and tools for this mysterious markup language.)

Quoted 8 lines

+/*
+ * History:
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * created:
+ * OKI SEMICONDUCTOR 04/14/2010
+ * modified:

No changelogs in comments, that's what git history is for.

Quoted 2 lines

+/* includes */

Useless comment.

Quoted 4 lines

+/*--------------------------------------------
+ macros
+--------------------------------------------*/

More useless comments.

drivers/char/pch_phub/pch_phub.c line 54

Quoted 5 lines

drivers/char/pch_phub/pch_phub.c new file +4 -0

+/* Status Register offset */
+#define PHUB_STATUS 0x00
+/* Control Register offset */
+#define PHUB_CONTROL 0x04

I would format this as:

#define PHUB_STATUS     0x00    /* Status Register offset */
#define PHUB_CONTROL    0x04    /* Control Register offset */

but it's up to you.

Quoted 3 lines

+struct pch_phub_reg {
+ unsigned int phub_id_reg;

Since these are used to hold HW-defined data, you should use fixed-size types such as u32. Also, you should consider whether they should be marked little endian / big endian.

drivers/char/pch_phub/pch_phub.c line 120

Quoted 5 lines

drivers/char/pch_phub/pch_phub.c new file +4 -0

+/* ToDo: major number allocation via module parameter */
+static dev_t pch_phub_dev_no;
+static int pch_phub_major_no;
+static struct cdev pch_phub_dev;

If this is to remain a chardev, use register_chrdev(). You shouldn't need a module parameter to configure your device numbers.

Quoted 3 lines

+ void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +\
+ reg_addr_offset;

That \ is superfluous. There are several of these in this file.

The indentation on the second line is too large, and the fact that "a = b + c" spills onto a second line is a clue that your struct names are too long.

Quoted 3 lines

+ iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
+ return;

The return is unneeded if this remains a void function. (many more occurrences.)

Quoted 3 lines

+/** pch_phub_restore_reg_conf - restore register configuration
+ */

Don't use /** unless you're using kernel-doc.

Quoted 7 lines

+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address; +
+ dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+ /* to store contents of PHUB_ID register */
+ iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);

Don't include comments that just duplicate the code. Also, rename your constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something.

Quoted 6 lines

+/** pch_phub_read_serial_rom - Implements the functionality of reading Serial

+ *  ROM.
+ *  @offset_address: Contains the Serial ROM address offset value
+ *  @data: Contains the Serial ROM value

+ */

This comment is almost correctly formatted, but there are extra words and some whitespace problems, and it doesn't document what actually happens.

+/**
+ * pch_phub_read_serial_rom - read PHUB Serial ROM.

+ *  @offset_address:   serial ROM offset to read.
+ *  @data:             pointer at which to store value that was read.

+ */

is more correct.

Similar problems in several function comments below.

Quoted 2 lines

+static int pch_phub_read_serial_rom(unsigned int offset_address,
+ unsigned char *data)

The second line is indented too far. We use 8-space tabstops, so the "u" of unsigned is all the way over under the "t" of offset_address. It should either be two tabstops indented, or lined up with the previous argument. (This applies to several functions below too.)

It should be "u8 *data".

drivers/char/pch_phub/pch_phub.c line 309

Quoted 7 lines

+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
+ offset_address;

drivers/char/pch_phub/pch_phub.c new file +4 -0

+
+ *data = ioread8(mem_addr);
+
+ return 0;

If this can't fail, why not have it just return the value rather than forcing the caller to provide a pointer? (If you want to be futureproof and support errorhandling in the future, that's OK.)

Quoted 12 lines

drivers/char/pch_phub/pch_phub.c new file +11 -0

+static int pch_phub_write_serial_rom(unsigned int offset_address,
+ unsigned char data)
+{
+ int retval = 0;
+ void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +\
+ (offset_address & 0xFFFFFFFC);
+ int i = 0;
+ unsigned int word_data = 0;
+
+ iowrite32(PCH_PHUB_ROM_WRITE_ENABLE,
+ pch_phub_reg.pch_phub_extrom_base_address +\

drivers/char/pch_phub/pch_phub.c line 337

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ PHUB_CONTROL);
+
+ word_data = ioread32(mem_addr);
+
+ switch (offset_address % 4) {
+ case 0:
+ word_data &= 0xFFFFFF00;
+ iowrite32((word_data | (unsigned int)data),
+ mem_addr);
+ break;

This function is much larger than need be. Remove the 0xFFFFFFFC magic number -- something like

#define PCH_WORD_ADDR_MASK (~((1 << 4) - 1))

and implement the byte masking as something like

    pos = (offset_address % 4) * 8;
    mask = ~(0xFF << pos);
    iowrite32((word_data & mask) | (u32)data << pos, mem_addr);

(You can keep the switch if there's a significant performance benefit to doing so, but please provide measurements.)

This is a read-modify-write cycle -- where is the locking if two users try to update the serial ROM simultaneously? Any required locking should be documented in the kernel-doc comment.

Quoted 10 lines

+ while (0x00 !=
+ ioread8(pch_phub_reg.pch_phub_extrom_base_address +\
+ PHUB_STATUS)) {
+ msleep(1);
+ if (PHUB_TIMEOUT == i) {
+ retval = -EPERM;
+ break;
+ }
+ i++;

Rewrite this as a for loop (and remove the initialization from the declaration of i at the top of this function). I am not sure if msleep() is safe here -- what context will this be called from?

Quoted 4 lines

drivers/char/pch_phub/pch_phub.c new file +3 -0

+/** pch_phub_read_serial_rom_val - Implements the functionality of reading
+ *  Serial ROM value.
+ *  @offset_address: Contains the Serial ROM address offset value

drivers/char/pch_phub/pch_phub.c line 384

Quoted 10 lines

drivers/char/pch_phub/pch_phub.c new file +10 -0

+ *  @data: Contains the Serial ROM value
+ */
+static int pch_phub_read_serial_rom_val(unsigned int offset_address,
+ unsigned char *data)
+{
+ int retval = 0;
+ unsigned int mem_addr;
+
+ mem_addr = (offset_address / 4 * 8) + 3 -
+ (offset_address % 4) + PCH_PHUB_ROM_START_ADDR;

Is that formula correct? It is very suprising.

If it is correct, the formula is bizarre enough to warrant a long comment explaining the theory of operation and/or pointing to hardware docs.

Quoted 17 lines

drivers/char/pch_phub/pch_phub.c new file +16 -0

+static ssize_t pch_phub_read(struct file *file, char __user *buf, size_t size,
+ loff_t *ppos)
+{
+ unsigned int rom_signature = 0;
+ unsigned char rom_length;
+ int ret_value;
+ unsigned int tmp;
+ unsigned char data;
+ unsigned int addr_offset = 0;
+ unsigned int orom_size;
+ loff_t pos = *ppos;
+
+ if (pos < 0)
+ return -EINVAL;
+
+ /*Get Rom signature*/

Quoted 2 lines

+ pch_phub_read_serial_rom(0x80, (unsigned char *)&rom_signature);
+ pch_phub_read_serial_rom(0x81, (unsigned char *)&tmp);

I seriously doubt that your device is special enough to warrant a custom /dev node with proprietary semantics. If this is just part of an Ethernet driver, please implement it in drivers/net/; if this is a generic PROM accessor, there must be some semi-standardized EPROM access interface but I don't know what it is offhand.

-andy

Quoted 7 lines

Quoted 6 lines

+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address; +
+ dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+ /* to store contents of PHUB_ID register */
+ iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);

Quoted 2 lines

Don't include comments that just duplicate the code. Also, rename your constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something.

Sorry, I can't understand your intention.
Please give us more information.

Quoted 5 lines

I seriously doubt that your device is special enough to warrant a custom /dev node with proprietary semantics. If this is just part of an Ethernet driver, please implement it in drivers/net/; if this is a generic PROM accessor, there must be some semi-standardized EPROM access interface but I don't know what it is offhand.

Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. Packet hub is not generic driver but special device.

1. When writing comments, do not write comments that duplicate the code. Instead of writing

2. your register names are very long. Since the #define names are private to this driver, there's no need to make them extremely descriptive. Instead of naming your registers PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like PHUB_ID_REG or PCH_ID_REG. This will make your source code much more readable by reducing linewrapping.

It sounds like PHUB is a system-level device which provides access to a SROM which contains GbE configuration data. If that is correct, then I have two comments:

1. There are many other systems with similar configurations -- MIPS SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to name a few. None of them expose the SROM as a custom /dev node AFAIK. Is there a shared infrastructure that you can implement?

2. How does your GbE driver get the MAC address from the SPROM? If there is an in-kernel user of the PHUB interface, it might be much easier to understand the design.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

+ unsigned int i;
+ void __iomem *p = pch_phub_reg.pch_phub_base_address; +
+ dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+ /* to store contents of PHUB_ID register */
+ iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);

Quoted 5 lines

Quoted 2 lines

Don't include comments that just duplicate the code. Also, rename your constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something.

Sorry, I can't understand your intention.
Please give us more information.

My mistake, I merged two comments into one paragraph, let me clarify.

1. When writing comments, do not write comments that duplicate the code. Instead of writing
/* store PHUB_ID */
iowrite32(..._PHUB_ID_REG);
/* store PHUB_FOO */
iowrite32(..._PHUB_FOO_REG);
you should delete the line-by-line comments and just write iowrite32(..._PHUB_ID_REG);
iowrite32(..._PHUB_FOO_REG);

2. your register names are very long. Since the #define names are private to this driver, there's no need to make them extremely descriptive. Instead of naming your registers PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like PHUB_ID_REG or PCH_ID_REG. This will make your source code much more readable by reducing linewrapping.

Quoted 8 lines

Quoted 5 lines

I seriously doubt that your device is special enough to warrant a custom /dev node with proprietary semantics. If this is just part of an Ethernet driver, please implement it in drivers/net/; if this is a generic PROM accessor, there must be some semi-standardized EPROM access interface but I don't know what it is offhand.

Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver. Packet hub is not generic driver but special device.

It sounds like PHUB is a system-level device which provides access to a SROM which contains GbE configuration data. If that is correct, then I have two comments:

1. There are many other systems with similar configurations -- MIPS SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to name a few. None of them expose the SROM as a custom /dev node AFAIK. Is there a shared infrastructure that you can implement?

2. How does your GbE driver get the MAC address from the SPROM? If there is an in-kernel user of the PHUB interface, it might be much easier to understand the design.

Thanks for responding to my review so quickly!
-andy

Hi Andy and Andrew

I have modified for your comments.
Please confirm below.

---

Packet hub driver of Topcliff PCH

Topcliff PCH is the platform controller hub that is going to be used in Intel’s upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data.
These data are in SROM which is connected to PCIE bus.
Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM.

Signed-off-by Masayuki Ohtake <[email protected]>

Acked-by Arnd <Arnd’s email address>

drivers/char/Kconfig +0 -0

index e023682..1851e97 100644

It's listed in the MAINTAINERS file. Please use that.

Maybe lose some acromyns? Overall, this help text should be about *what* PCH_PHUB is, not *how* it accomplishes its work.
If you want to document *how* it works, put that into the source code or Documentation/ somewhere.

Quoted 22 lines

Hi Andy and Andrew

I have modified for your comments.
Please confirm below.

---

Packet hub driver of Topcliff PCH

Topcliff PCH is the platform controller hub that is going to be used in Intel$B!G(Bs upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data.
These data are in SROM which is connected to PCIE bus.
Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM.

Quoted 2 lines

Signed-off-by Masayuki Ohtake <[email protected]>

Acked-by Arnd <Arnd$B!G(Bs email address>

It's listed in the MAINTAINERS file. Please use that.

Quoted 6 lines

drivers/char/Kconfig +0 -0

index e023682..1851e97 100644

drivers/char/Kconfig line 11

Quoted 8 lines

drivers/char/Kconfig +5 -0

@@ -4,6 +4,16 @@
 
 menu "Character devices"
 
+config PCH_PHUB
+ tristate "PCH PHUB"
+ depends on PCI
+ help
+   Topcliff is a IOH for x86 embedded processor. This IOH is quite

is an IOH for x86 embedded processors.

drivers/char/Kconfig line 12

Quoted 1 line

drivers/char/Kconfig +1 -0

+   different with the traditional IOH. Topcliff is a kind of ARM-based

from

drivers/char/Kconfig line 14

Quoted 2 lines

drivers/char/Kconfig +2 -0

+   processor and connected with PCIe bus (from x86 processor).
+   PHUB work as a gateway transform the PCIe transaction into the AMBA

works as a gateway to transform

drivers/char/Kconfig line 15

Quoted 1 line

drivers/char/Kconfig +1 -0

+   transaction, and vise verse, and have several transform windows also.

vice versa, and has several

Maybe lose some acromyns? Overall, this help text should be about *what* PCH_PHUB is, not *how* it accomplishes its work.
If you want to document *how* it works, put that into the source code or Documentation/ somewhere.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Hi Andy and Randy

I have modified for your comments.
Please confirm below.

drivers/char/pch_phub/pch_phub.c new file +2 -0

+static long pch_phub_ioctl(struct file *file, unsigned int cmd,
+			   unsigned long arg)

I still have concerns about the userland interface design. It still seems to me that the MAC interface should be used by something in drivers/net and there's no reason to expose the SROM in /dev unless it contains more than just the MAC address.

A few more style issues I saw on a quick scan through -- this was not a comprehensive review:

Quoted 4 lines

Hi Andy and Randy

I have modified for your comments.
Please confirm below.

Your style is looking better, and the additional documentation is much appreciated.

I still have concerns about the userland interface design. It still seems to me that the MAC interface should be used by something in drivers/net and there's no reason to expose the SROM in /dev unless it contains more than just the MAC address.

A few more style issues I saw on a quick scan through -- this was not a comprehensive review:

Quoted 2 lines

drivers/char/pch_phub/pch_phub.c new file +2 -0

+static long pch_phub_ioctl(struct file *file, unsigned int cmd,
+    unsigned long arg)

[snip]

Quoted 2 lines

+ ret_value = copy_to_user(varg,
+ mac_addr, sizeof(mac_addr));

Reformat this to fit on one line:
+ ret_value = copy_to_user(varg, mac_addr, sizeof(mac_addr));

[snip]

Quoted 4 lines

+ case IOCTL_PHUB_WRITE_MAC_ADDR:
+ ret_value = copy_from_user(mac_addr, varg, sizeof(mac_addr)); +
+ if (ret_value)

Here we need to break:

+ {

Quoted 1 line

+ ret_value = -EFAULT;

(quoted diff) +2 -0

+                       break;
+               }

because if copy_from_user failed ...

Quoted 2 lines

+ for (i = 0; i < 6; i++)
+ pch_phub_write_gbe_mac_addr(i, mac_addr[i]);

... we would pass garbage to pch_phub_write_gbe_mac_addr.

[snip]

Quoted 2 lines

+ dev_dbg(&pdev->dev,
+ "\npch_phub_probe : pci_enable_device FAILED");

Prefix \n is not correct. This should be

+ dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device FAILED");

In general dev_dbg format strings should fit on one 80-char line. If your format strings are longer than that, it's a clue you're doing something wrong. For example:

Quoted 2 lines

+ dev_dbg(&pdev->dev, "pch_phub_probe : "
+ "pci_enable_device returns %d\n", ret);

+ dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device returns %d\n",
+ ret);

Quoted 4 lines

+ ret = pci_request_regions(pdev, MODULE_NAME);
+ if (ret) {
+ dev_dbg(&pdev->dev,
+ "pch_phub_probe : pci_request_regions FAILED");

If you have a dev_dbg, please print ret. It may give an important clue.

[snip a bunch more I don't have time to review right now]

Thanks,
-andy

Packet hub driver of Topcliff PCH

Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is a special converter device in Topcliff PCH that translate AMBA transactions to PCI Express transactions and vice versa. Thus packet hub helps present all IO peripherals in Topcliff PCH as PCIE devices to IA system. Topcliff PCH have MAC address and Option ROM data.
These data are in SROM which is connected to PCIE bus.
Packet hub driver of Topcliff PCH can access MAC address and Option ROM data in SROM.

Signed-off-by Masayuki Ohtake <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

---
 drivers/char/Kconfig             |    9 +
 drivers/char/Makefile            |    2 +
 drivers/char/pch_phub/Makefile   |    3 +
 drivers/char/pch_phub/pch_phub.c |  811 ++++++++++++++++++++++++++++++++++++++
 drivers/char/pch_phub/pch_phub.h |   48 +++
 5 files changed, 873 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/pch_phub/Makefile
 create mode 100755 drivers/char/pch_phub/pch_phub.c
 create mode 100755 drivers/char/pch_phub/pch_phub.h

drivers/char/Kconfig +0 -0

index e023682..9ef3c85 100644

drivers/char/pch_phub/pch_phub.c new file +156 -0

+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/string.h>
+#include <linux/pci.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include "pch_phub.h"
+
+#define PHUB_STATUS 0x00		/* Status Register offset */
+#define PHUB_CONTROL 0x04		/* Control Register offset */
+#define PHUB_TIMEOUT 0x05		/* Time out value for Status Register */
+#define PCH_PHUB_ROM_WRITE_ENABLE 0x01	/* Enabling for writing ROM */
+#define PCH_PHUB_ROM_WRITE_DISABLE 0x00	/* Disabling for writing ROM */
+#define PCH_PHUB_ROM_START_ADDR 0x14	/* ROM data area start address offset */
+
+/* MAX number of INT_REDUCE_CONTROL registers */
+#define MAX_NUM_INT_REDUCE_CONTROL_REG 128
+#define PCI_DEVICE_ID_PCH1_PHUB 0x8801
+#define PCH_MINOR_NOS 1
+#define CLKCFG_CAN_50MHZ 0x12000000
+#define CLKCFG_CANCLK_MASK 0xFF000000
+#define MODULE_NAME "pch_phub"
+
+/**
+ * struct pch_phub_reg - PHUB register structure
+ * @phub_id_reg:			PHUB_ID register val
+ * @q_pri_val_reg:			QUEUE_PRI_VAL register val
+ * @rc_q_maxsize_reg:			RC_QUEUE_MAXSIZE register val
+ * @bri_q_maxsize_reg:			BRI_QUEUE_MAXSIZE register val
+ * @comp_resp_timeout_reg:		COMP_RESP_TIMEOUT register val
+ * @bus_slave_control_reg:		BUS_SLAVE_CONTROL_REG register val
+ * @deadlock_avoid_type_reg:		DEADLOCK_AVOID_TYPE register val
+ * @intpin_reg_wpermit_reg0:		INTPIN_REG_WPERMIT register 0 val
+ * @intpin_reg_wpermit_reg1:		INTPIN_REG_WPERMIT register 1 val
+ * @intpin_reg_wpermit_reg2:		INTPIN_REG_WPERMIT register 2 val
+ * @intpin_reg_wpermit_reg3:		INTPIN_REG_WPERMIT register 3 val
+ * @int_reduce_control_reg:		INT_REDUCE_CONTROL registers val
+ * @clkcfg_reg:				CLK CFG register val
+ * @pch_phub_base_address:		Register base address
+ * @pch_phub_extrom_base_address:	external rom base address
+ * @pch_phub_suspended:			PHUB status val
+ */
+struct pch_phub_reg {
+	u32 phub_id_reg;
+	u32 q_pri_val_reg;
+	u32 rc_q_maxsize_reg;
+	u32 bri_q_maxsize_reg;
+	u32 comp_resp_timeout_reg;
+	u32 bus_slave_control_reg;
+	u32 deadlock_avoid_type_reg;
+	u32 intpin_reg_wpermit_reg0;
+	u32 intpin_reg_wpermit_reg1;
+	u32 intpin_reg_wpermit_reg2;
+	u32 intpin_reg_wpermit_reg3;
+	u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
+	u32 clkcfg_reg;
+	void __iomem *pch_phub_base_address;
+	void __iomem *pch_phub_extrom_base_address;
+	int pch_phub_suspended;
+} pch_phub_reg;
+
+/* SROM SPEC for MAC address assignment offset */
+static const int pch_phub_mac_offset[6] = {0x3, 0x2, 0x1, 0x0, 0xb, 0xa};
+
+static DEFINE_MUTEX(pch_phub_ioctl_mutex);
+static DEFINE_MUTEX(pch_phub_read_mutex);
+static DEFINE_MUTEX(pch_phub_write_mutex);
+static dev_t pch_phub_dev_no;
+static struct cdev pch_phub_dev;
+
+/**
+ * pch_phub_read_modify_write_reg() - Reading modifying and writing register
+ * @reg_addr_offset:	Register offset address value.
+ * @data:		Writing value.
+ * @mask:		Mask value.
+ */
+static void pch_phub_read_modify_write_reg(unsigned int reg_addr_offset,
+					   unsigned int data, unsigned int mask)
+{
+	void __iomem *reg_addr = pch_phub_reg.pch_phub_base_address +
+					 reg_addr_offset;
+	iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
+}
+
+
+/* pch_phub_save_reg_conf - saves register configuration */
+static void pch_phub_save_reg_conf(struct pci_dev *pdev)
+{
+	unsigned int i;
+	void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf ENTRY\n");
+	pch_phub_reg.phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
+	pch_phub_reg.q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+	pch_phub_reg.rc_q_maxsize_reg =
+	    ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+	pch_phub_reg.bri_q_maxsize_reg =
+	    ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+	pch_phub_reg.comp_resp_timeout_reg =
+	    ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+	pch_phub_reg.bus_slave_control_reg =
+	    ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+	pch_phub_reg.deadlock_avoid_type_reg =
+	    ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+	pch_phub_reg.intpin_reg_wpermit_reg0 =
+	    ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+	pch_phub_reg.intpin_reg_wpermit_reg1 =
+	    ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+	pch_phub_reg.intpin_reg_wpermit_reg2 =
+	    ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+	pch_phub_reg.intpin_reg_wpermit_reg3 =
+	    ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+		"pch_phub_reg.phub_id_reg=%x, "
+		"pch_phub_reg.q_pri_val_reg=%x, "
+		"pch_phub_reg.rc_q_maxsize_reg=%x, "
+		"pch_phub_reg.bri_q_maxsize_reg=%x, "
+		"pch_phub_reg.comp_resp_timeout_reg=%x, "
+		"pch_phub_reg.bus_slave_control_reg=%x, "
+		"pch_phub_reg.deadlock_avoid_type_reg=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+		pch_phub_reg.phub_id_reg,
+		pch_phub_reg.q_pri_val_reg,
+		pch_phub_reg.rc_q_maxsize_reg,
+		pch_phub_reg.bri_q_maxsize_reg,
+		pch_phub_reg.comp_resp_timeout_reg,
+		pch_phub_reg.bus_slave_control_reg,
+		pch_phub_reg.deadlock_avoid_type_reg,
+		pch_phub_reg.intpin_reg_wpermit_reg0,
+		pch_phub_reg.intpin_reg_wpermit_reg1,
+		pch_phub_reg.intpin_reg_wpermit_reg2,

drivers/char/pch_phub/pch_phub.c new file +92 -0

+}
+
+/* pch_phub_restore_reg_conf - restore register configuration */
+static void pch_phub_restore_reg_conf(struct pci_dev *pdev)
+{
+	unsigned int i;
+	void __iomem *p = pch_phub_reg.pch_phub_base_address;
+
+	dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
+	iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_ID_REG);
+	iowrite32(pch_phub_reg.q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
+	iowrite32(pch_phub_reg.rc_q_maxsize_reg,
+					p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
+	iowrite32(pch_phub_reg.bri_q_maxsize_reg,
+					p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
+	iowrite32(pch_phub_reg.comp_resp_timeout_reg,
+					p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
+	iowrite32(pch_phub_reg.bus_slave_control_reg,
+					p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
+	iowrite32(pch_phub_reg.deadlock_avoid_type_reg,
+					p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg0,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg1,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg2,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
+	iowrite32(pch_phub_reg.intpin_reg_wpermit_reg3,
+					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
+	dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+		"pch_phub_reg.phub_id_reg=%x, "
+		"pch_phub_reg.q_pri_val_reg=%x, "
+		"pch_phub_reg.rc_q_maxsize_reg=%x, "
+		"pch_phub_reg.bri_q_maxsize_reg=%x, "
+		"pch_phub_reg.comp_resp_timeout_reg=%x, "
+		"pch_phub_reg.bus_slave_control_reg=%x, "
+		"pch_phub_reg.deadlock_avoid_type_reg=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg0=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg1=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg2=%x, "
+		"pch_phub_reg.intpin_reg_wpermit_reg3=%x\n",
+		pch_phub_reg.phub_id_reg,
+		pch_phub_reg.q_pri_val_reg,
+		pch_phub_reg.rc_q_maxsize_reg,
+		pch_phub_reg.bri_q_maxsize_reg,
+		pch_phub_reg.comp_resp_timeout_reg,
+		pch_phub_reg.bus_slave_control_reg,
+		pch_phub_reg.deadlock_avoid_type_reg,
+		pch_phub_reg.intpin_reg_wpermit_reg0,
+		pch_phub_reg.intpin_reg_wpermit_reg1,
+		pch_phub_reg.intpin_reg_wpermit_reg2,
+		pch_phub_reg.intpin_reg_wpermit_reg3);
+	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
+		iowrite32(pch_phub_reg.int_reduce_control_reg[i],
+			p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
+		dev_dbg(&pdev->dev, "pch_phub_save_reg_conf : "
+			"pch_phub_reg.int_reduce_control_reg[%d]=%x\n",
+			i, pch_phub_reg.int_reduce_control_reg[i]);
+	}
+
+	iowrite32(pch_phub_reg.clkcfg_reg, p + CLKCFG_REG_OFFSET);
+}
+
+/**
+ * pch_phub_read_serial_rom() - Reading Serial ROM
+ * @offset_address:	Serial ROM offset address to read.
+ * @data:		Read buffer for specified Serial ROM value.
+ */
+static void pch_phub_read_serial_rom(unsigned int offset_address, u8 *data)
+{
+	void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
+					 offset_address;
+
+	*data = ioread8(mem_addr);
+}
+
+/**
+ * pch_phub_write_serial_rom() - Writing Serial ROM
+ * @offset_address:	Serial ROM offset address.
+ * @data:		Serial ROM value to write.
+ */
+static int pch_phub_write_serial_rom(unsigned int offset_address, u8 data)
+{
+	void __iomem *mem_addr = pch_phub_reg.pch_phub_extrom_base_address +
+					(offset_address & PCH_WORD_ADDR_MASK);
+	int i;
+	unsigned int word_data;
+	unsigned int pos;
+	unsigned int mask;
+	pos = (offset_address % 4) * 8;
+	mask = ~(0xFF << pos);
+

drivers/char/pch_phub/pch_phub.c new file +17 -0

+		msleep(1);
+		if (PHUB_TIMEOUT == i)
+			return -EPERM;
+		i++;
+	}
+
+	iowrite32(PCH_PHUB_ROM_WRITE_DISABLE,
+			pch_phub_reg.pch_phub_extrom_base_address +
+			PHUB_CONTROL);
+
+	return 0;
+}
+
+/**
+ * pch_phub_read_serial_rom_val() - Read Serial ROM value
+ * @offset_address:	Serial ROM address offset value.
+ * @data:		Serial ROM value to read.

drivers/char/pch_phub/pch_phub.c new file +60 -0

+}
+
+
+/**
+ * pch_phub_write_serial_rom_val() - writing Serial ROM value
+ * @offset_address:	Serial ROM address offset value.
+ * @data:		Serial ROM value.
+ */
+static int pch_phub_write_serial_rom_val(unsigned int offset_address, u8 data)
+{
+	int retval;
+	unsigned int mem_addr;
+
+	mem_addr = PCH_PHUB_ROM_START_ADDR +
+			pch_phub_mac_offset[offset_address];
+
+	retval = pch_phub_write_serial_rom(mem_addr, data);
+
+	return retval;
+}
+
+/* pch_phub_gbe_serial_rom_conf - makes Serial ROM header format configuration
+ * for Gigabit Ethernet MAC address
+ */
+static int pch_phub_gbe_serial_rom_conf(void)
+{
+	int retval;
+
+	retval = pch_phub_write_serial_rom(0x0b, 0xbc);
+	retval |= pch_phub_write_serial_rom(0x0a, 0x10);
+	retval |= pch_phub_write_serial_rom(0x09, 0x01);
+	retval |= pch_phub_write_serial_rom(0x08, 0x02);
+
+	retval |= pch_phub_write_serial_rom(0x0f, 0x00);
+	retval |= pch_phub_write_serial_rom(0x0e, 0x00);
+	retval |= pch_phub_write_serial_rom(0x0d, 0x00);
+	retval |= pch_phub_write_serial_rom(0x0c, 0x80);
+
+	retval |= pch_phub_write_serial_rom(0x13, 0xbc);
+	retval |= pch_phub_write_serial_rom(0x12, 0x10);
+	retval |= pch_phub_write_serial_rom(0x11, 0x01);
+	retval |= pch_phub_write_serial_rom(0x10, 0x18);
+
+	retval |= pch_phub_write_serial_rom(0x1b, 0xbc);
+	retval |= pch_phub_write_serial_rom(0x1a, 0x10);
+	retval |= pch_phub_write_serial_rom(0x19, 0x01);
+	retval |= pch_phub_write_serial_rom(0x18, 0x19);
+
+	retval |= pch_phub_write_serial_rom(0x23, 0xbc);
+	retval |= pch_phub_write_serial_rom(0x22, 0x10);
+	retval |= pch_phub_write_serial_rom(0x21, 0x01);
+	retval |= pch_phub_write_serial_rom(0x20, 0x3a);
+
+	retval |= pch_phub_write_serial_rom(0x27, 0x01);
+	retval |= pch_phub_write_serial_rom(0x26, 0x00);
+	retval |= pch_phub_write_serial_rom(0x25, 0x00);
+	retval |= pch_phub_write_serial_rom(0x24, 0x00);
+
+	return retval;
+}