Topcliff PHUB: Generate PacketHub driver

PATCH In review Masayuki Ohtake 4 participants 8 messages Jun 4, 2010 – Jun 9, 2010

lore

Share & tools

lore

Masayuki Ohtake

author

2010-06-04 10:43 UTC #0 lore raw

PATCH 1/1 Topcliff PHUB: Generate PacketHub driver

From: Masayuki Ohtake <[email protected]>

This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1

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

---
 linux-2.6.33.1/drivers/char/Kconfig             |   20 +
 linux-2.6.33.1/drivers/char/Makefile            |    4 +
 linux-2.6.33.1/drivers/char/pch_phub/Makefile   |   11 +
 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c | 1500 +++++++++++++++++++++++
 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h |  182 +++
 5 files changed, 1717 insertions(+), 0 deletions(-)
 create mode 100644 linux-2.6.33.1/drivers/char/pch_phub/Makefile
 create mode 100644 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c
 create mode 100644 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h

linux-2.6.33.1/drivers/char/Kconfig +20 -0

index e023682..16a8334 100644
@@ -4,6 +4,26 @@
 
 menu "Character devices"
 
+config PCH_IEEE1588
+	tristate "PCH IEEE1588"
+	depends on PCI
+	help
+	  If you say yes to this option, support will be included for the
+	  PCH IEEE1588 Host controller.
+
+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.
+
+config PCH_CAN_PCLK_50MHZ
+	bool "CAN PCLK 50MHz"
+	depends on PCH_PHUB
+        help
+          If you say yes to this option, clock is set to 50MHz.(For CAN control)
+
 config VT
 	bool "Virtual terminal" if EMBEDDED
 	depends on !S390

linux-2.6.33.1/drivers/char/Makefile +4 -0

index f957edf..dc092d0 100644
@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH)		+= ps3flash.o
 obj-$(CONFIG_JS_RTC)		+= js-rtc.o
 js-rtc-y = rtc.o
 
+obj-$(CONFIG_PCH_IEEE1588)		+= pch_ieee1588/
+
+obj-$(CONFIG_PCH_PHUB)	+= pch_phub/
+
 # Files generated that shall be removed upon make clean
 clean-files := consolemap_deftbl.c defkeymap.c

linux-2.6.33.1/drivers/char/pch_phub/Makefile new file +11 -0

new file mode 100644
index 0000000..bfe2a2b
@@ -0,0 +1,11 @@
+ifeq ($(CONFIG_PHUB_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
+
+obj-$(CONFIG_PCH_PHUB) += pch_phub_drv.o
+#to set CAN clock to 50Mhz
+ifdef CONFIG_PCH_CAN_PCLK_50MHZ
+EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ
+endif
+
+pch_phub_drv-objs := pch_phub.o

linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c new file +1500 -0

1,500 lines not on this page · raw message

linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h new file +182 -0

182 lines not on this page · raw message

-- 1.6.2.2

Alan Cox

2010-06-04 11:52 UTC #1 lore raw

Quoted 5 lines

+/*!
+ * @ingroup PHUB_PCILayerAPI
+ * @fn static void __exit pch_phub_pci_exit(void)
+ * @brief Provides the functionality of exiting the module
+ * */

Can we please do something about all this stuff first so the code is easier to review by those not familiar with the kernel format.

I sent a bit of code out last time that did most of the work

Alan

Masayuki Ohtake

author

2010-06-07 07:54 UTC #2 lore raw

Hi Alan

Thank you for your comment.
We are now modifying.
We will resubmit as soon as we can possible.

Thanks.
----- Original Message -----
From: "Alan Cox" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Friday, June 04, 2010 9:00 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

Quoted 13 lines

Quoted 5 lines
+/*!
+ * @ingroup PHUB_PCILayerAPI
+ * @fn static void __exit pch_phub_pci_exit(void)
+ * @brief Provides the functionality of exiting the module
+ * */
Can we please do something about all this stuff first so the code is easier to review by those not familiar with the kernel format.
I sent a bit of code out last time that did most of the work
Alan

Arnd Bergmann

2010-06-07 13:37 UTC #3 lore raw

In reply to Masayuki Ohtake

Quoted 6 lines · Masayuki Ohtake

From: Masayuki Ohtake <[email protected]>
This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1
Signed-off-by Masayuki Ohtake <[email protected]>

Most of the comments from my first review still apply to this version, please have another look at what I wrote
back in april, see http://article.gmane.org/gmane.linux.network/158879

Note also that patch submissions should be against the latest development kernel, in this case 2.6.35-rc2, not against stable kernel releases like 2.6.33.1.

Arnd

Masayuki Ohtake

author

2010-06-08 00:19 UTC #4 lore raw

Hi Arnd,

Thank you for your comments.
We will integrate your comments to our PHUB as soon as possible.

Ohtake..

----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Monday, June 07, 2010 10:37 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

Quoted 18 lines · Masayuki Ohtake

Quoted 6 lines
From: Masayuki Ohtake <[email protected]>
This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1
Signed-off-by Masayuki Ohtake <[email protected]>
Most of the comments from my first review still apply to this version, please have another look at what I wrote
back in april, see http://article.gmane.org/gmane.linux.network/158879
Note also that patch submissions should be against the latest development kernel, in this case 2.6.35-rc2, not against stable kernel releases like 2.6.33.1.
Arnd

Masayuki Ohtake

author

2010-06-08 08:48 UTC #5 lore raw

In reply to Arnd Bergmann

Hi Arnd

We are studying your indication.

Quoted 6 lines

My feeling is that this ioctl interface is too low-level in general. You only export access to specific registers, not to functionality exposed by them. The best kernel interfaces are defined in a way that is independent of the underlying hardware and convert generic commands into device specific commands.

I have a question.
We don't know 'generic commands' concretely. Let me know 'generic commands' in detail.

Thanks. Ohtake.

Quoted 18 lines · Masayuki Ohtake

Quoted 6 lines
From: Masayuki Ohtake <[email protected]>
This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1
Signed-off-by Masayuki Ohtake <[email protected]>
Most of the comments from my first review still apply to this version, please have another look at what I wrote
back in april, see http://article.gmane.org/gmane.linux.network/158879
Note also that patch submissions should be against the latest development kernel, in this case 2.6.35-rc2, not against stable kernel releases like 2.6.33.1.
Arnd

Arnd Bergmann

2010-06-08 09:29 UTC #6 lore raw

Quoted 11 lines · Masayuki Ohtake

We are studying your indication.
Quoted 6 lines
My feeling is that this ioctl interface is too low-level in general. You only export access to specific registers, not to functionality exposed by them. The best kernel interfaces are defined in a way that is independent of the underlying hardware and convert generic commands into device specific commands.
I have a question.
We don't know 'generic commands' concretely.
Let me know 'generic commands' in detail.

I have not seen the application using the driver, but a better abstraction IMHO would be to take an abstraction you have in your application and move it into the kernel.

I can be more specific if you tell me where to find the source of the application. Generally speaking, you'd transform a function like

/* the function that knows how to do 'this' */ int phub_do_this(int phub_fd, unsigned long arg)

{
	struct pch_phub_req req = {
		.addr_offset = SOME_ADDR_OFF,
	};
	ioctl(fd, IOCTL_PHUB_READ_REG, &req);

	if (req.data & SOME_BITS)
		return ERROR;

	req.addr_offset = ANOTHER_ADDR_OFF;
	req.data = arg | REALLY_DO_IT_BITMASK;

	ioctl(fd, IOCTL_PHUB_WRITE_REG, &req);

	return 0;
}

into another function that does the same thing but without knowing anything about the registers:

/* the same function on the abstract interface */ int phub_do_this_new(int phub_fd, unsigned long arg)

{
	return ioctl(phub_fd, IOCTL_PHUB_DO_THIS, &arg);
}

Arnd

Wang, Qi

2010-06-09 00:14 UTC #7 lore raw

In reply to Masayuki Ohtake

Ohtake-san,

Let me explain what Arnd means.

Writing the all the registers with ioctl is discouraged. For this case, what you need is writing/updating or reading some registers, not all the registers of Topcliff. It's better that you implement as the following example.

For example, you want to update MAC register with 'aaaa'.
It's not a better way to pass the register of mac address and aaaa via the ioctl routine. It's better only pass the aaaa with the 'update mac address command' (update aaaa to mac register). Actually it's discouraged to pass the address information with ioctl routine, because it'll bring the security issue even if you check the address-range.

It's better to pass the command and relative argument to implement something, not 'the address', 'read/write register command' and 'the value to read/write'.

Any question, please let me know.

Best Regards,
Qi.

-----Original Message-----
From: Masayuki Ohtake [mailto:[email protected]]
Sent: Tuesday, June 08, 2010 4:49 PM
To: Arnd Bergmann
Cc: LKML; Khor, Andrew Chih Howe; Clark, Joel; Wang, Qi; Wang, Yong Y
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

Hi Arnd

We are studying your indication.

Quoted 6 lines

My feeling is that this ioctl interface is too low-level in general. You only export access to specific registers, not to functionality exposed by them. The best kernel interfaces are defined in a way that is independent of the underlying hardware and convert generic commands into device specific commands.

I have a question.
We don't know 'generic commands' concretely.
Let me know 'generic commands' in detail.

Thanks. Ohtake.

Quoted 18 lines · Masayuki Ohtake

Quoted 6 lines
From: Masayuki Ohtake <[email protected]>
This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1
Signed-off-by Masayuki Ohtake <[email protected]>
Most of the comments from my first review still apply to this version, please have another look at what I wrote
back in april, see http://article.gmane.org/gmane.linux.network/158879
Note also that patch submissions should be against the latest development kernel, in this case 2.6.35-rc2, not against stable kernel releases like 2.6.33.1.
Arnd