Netdev List
 help / color / mirror / Atom feed
* [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 16:27 UTC (permalink / raw)
  To: Robin Holt, Kumar Gala, Wolfgang Grandegger, U Bhaskar-B22300,
	Scott Wood
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	PPC list
In-Reply-To: <1312993670-23999-1-git-send-email-holt-sJ/iWh9BUns@public.gmane.org>

This patch cleans up the documentation of the device-tree binding for
the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
properties are not needed as the frequency of the source clock is
fixed, there is not external divider beyond what the driver already
works with, and the clock source can not be selected.

Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
Acked-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
To: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
To: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
---
 .../devicetree/bindings/net/can/fsl-flexcan.txt    |   70 ++++----------------
 arch/powerpc/boot/dts/p1010rdb.dts                 |   10 +--
 arch/powerpc/boot/dts/p1010si.dtsi                 |   10 +--
 3 files changed, 19 insertions(+), 71 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
index 1a729f0..869f4ca 100644
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
@@ -1,61 +1,17 @@
-CAN Device Tree Bindings
-------------------------
-2011 Freescale Semiconductor, Inc.
+Flexcan CAN contoller on Freescale's ARM and PowerPC processors
 
-fsl,flexcan-v1.0 nodes
------------------------
-In addition to the required compatible-, reg- and interrupt-properties, you can
-also specify which clock source shall be used for the controller.
+Required properties:
 
-CPI Clock- Can Protocol Interface Clock
-	This CLK_SRC bit of CTRL(control register) selects the clock source to
-	the CAN Protocol Interface(CPI) to be either the peripheral clock
-	(driven by the PLL) or the crystal oscillator clock. The selected clock
-	is the one fed to the prescaler to generate the Serial Clock (Sclock).
-	The PRESDIV field of CTRL(control register) controls a prescaler that
-	generates the Serial Clock (Sclock), whose period defines the
-	time quantum used to compose the CAN waveform.
+- compatible : Should be "fsl,flexcan" and optionally
+               "fsl,flexcan-<processor>"
+- reg : Offset and length of the register set for this device
+- interrupts : Interrupt tuple for this device
 
-Can Engine Clock Source
-	There are two sources for CAN clock
-	- Platform Clock  It represents the bus clock
-	- Oscillator Clock
+Example:
 
-	Peripheral Clock (PLL)
-	--------------
-		     |
-		    ---------		      -------------
-		    |       |CPI Clock	      | Prescaler |       Sclock
-		    |       |---------------->| (1.. 256) |------------>
-		    ---------		      -------------
-                     |  |
-	--------------  ---------------------CLK_SRC
-	Oscillator Clock
-
-- fsl,flexcan-clock-source : CAN Engine Clock Source.This property selects
-			     the peripheral clock. PLL clock is fed to the
-			     prescaler to generate the Serial Clock (Sclock).
-			     Valid values are "oscillator" and "platform"
-			     "oscillator": CAN engine clock source is oscillator clock.
-			     "platform" The CAN engine clock source is the bus clock
-		             (platform clock).
-
-- fsl,flexcan-clock-divider : for the reference and system clock, an additional
-			      clock divider can be specified.
-- clock-frequency: frequency required to calculate the bitrate for FlexCAN.
-
-Note:
-	- v1.0 of flexcan-v1.0 represent the IP block version for P1010 SOC.
-	- P1010 does not have oscillator as the Clock Source.So the default
-	  Clock Source is platform clock.
-Examples:
-
-	can0@1c000 {
-		compatible = "fsl,flexcan-v1.0";
-		reg = <0x1c000 0x1000>;
-		interrupts = <48 0x2>;
-		interrupt-parent = <&mpic>;
-		fsl,flexcan-clock-source = "platform";
-		fsl,flexcan-clock-divider = <2>;
-		clock-frequency = <fixed by u-boot>;
-	};
+  can@1c000 {
+          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
+          reg = <0x1c000 0x1000>;
+          interrupts = <48 0x2>;
+          interrupt-parent = <&mpic>;
+  };
diff --git a/arch/powerpc/boot/dts/p1010rdb.dts b/arch/powerpc/boot/dts/p1010rdb.dts
index 6b33b73..d6c669c 100644
--- a/arch/powerpc/boot/dts/p1010rdb.dts
+++ b/arch/powerpc/boot/dts/p1010rdb.dts
@@ -23,6 +23,8 @@
 		ethernet2 = &enet2;
 		pci0 = &pci0;
 		pci1 = &pci1;
+		can0 = &can0;
+		can1 = &can1;
 	};
 
 	memory {
@@ -169,14 +171,6 @@
 			};
 		};
 
-		can0@1c000 {
-			fsl,flexcan-clock-source = "platform";
-		};
-
-		can1@1d000 {
-			fsl,flexcan-clock-source = "platform";
-		};
-
 		usb@22000 {
 			phy_type = "utmi";
 		};
diff --git a/arch/powerpc/boot/dts/p1010si.dtsi b/arch/powerpc/boot/dts/p1010si.dtsi
index 7f51104..f00076b 100644
--- a/arch/powerpc/boot/dts/p1010si.dtsi
+++ b/arch/powerpc/boot/dts/p1010si.dtsi
@@ -140,20 +140,18 @@
 			interrupt-parent = <&mpic>;
 		};
 
-		can0@1c000 {
-			compatible = "fsl,flexcan-v1.0";
+		can0: can@1c000 {
+			compatible = "fsl,p1010-flexcan", "fsl,flexcan";
 			reg = <0x1c000 0x1000>;
 			interrupts = <48 0x2>;
 			interrupt-parent = <&mpic>;
-			fsl,flexcan-clock-divider = <2>;
 		};
 
-		can1@1d000 {
-			compatible = "fsl,flexcan-v1.0";
+		can1: can@1d000 {
+			compatible = "fsl,p1010-flexcan", "fsl,flexcan";
 			reg = <0x1d000 0x1000>;
 			interrupts = <61 0x2>;
 			interrupt-parent = <&mpic>;
-			fsl,flexcan-clock-divider = <2>;
 		};
 
 		L2: l2-cache-controller@20000 {
-- 
1.7.2.1

^ permalink raw reply related

* [PATCH v11 2/5] flexcan: Abstract off read/write for big/little endian.
From: Robin Holt @ 2011-08-10 16:27 UTC (permalink / raw)
  To: Robin Holt, Kumar Gala, Wolfgang Grandegger, U Bhaskar-B22300
  Cc: Robin Holt, Marc Kleine-Budde, Wolfgang Grandegger,
	U Bhaskar-B22300, socketcan-core, netdev, PPC list
In-Reply-To: <1312993670-23999-1-git-send-email-holt@sgi.com>

Make flexcan driver handle register reads in the appropriate endianess.
This was a basic search and replace and then define some inlines.

Signed-off-by: Robin Holt <holt@sgi.com>
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Cc: U Bhaskar-B22300 <B22300@freescale.com>
Cc: socketcan-core@lists.berlios.de
Cc: netdev@vger.kernel.org
Cc: PPC list <linuxppc-dev@lists.ozlabs.org>
---
 drivers/net/can/flexcan.c |  140 ++++++++++++++++++++++++++------------------
 1 files changed, 83 insertions(+), 57 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 586b2cd..68cbe52 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -190,6 +190,31 @@ static struct can_bittiming_const flexcan_bittiming_const = {
 };
 
 /*
+ * Abstract off the read/write for arm versus ppc.
+ */
+#if defined(__BIG_ENDIAN)
+static inline u32 flexcan_read(void __iomem *addr)
+{
+	return in_be32(addr);
+}
+
+static inline void flexcan_write(u32 val, void __iomem *addr)
+{
+	out_be32(addr, val);
+}
+#else
+static inline u32 flexcan_read(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static inline void flexcan_write(u32 val, void __iomem *addr)
+{
+	writel(val, addr);
+}
+#endif
+
+/*
  * Swtich transceiver on or off
  */
 static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
@@ -210,9 +235,9 @@ static inline void flexcan_chip_enable(struct flexcan_priv *priv)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg &= ~FLEXCAN_MCR_MDIS;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	udelay(10);
 }
@@ -222,9 +247,9 @@ static inline void flexcan_chip_disable(struct flexcan_priv *priv)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 }
 
 static int flexcan_get_berr_counter(const struct net_device *dev,
@@ -232,7 +257,7 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg = readl(&regs->ecr);
+	u32 reg = flexcan_read(&regs->ecr);
 
 	bec->txerr = (reg >> 0) & 0xff;
 	bec->rxerr = (reg >> 8) & 0xff;
@@ -266,15 +291,15 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (cf->can_dlc > 0) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[0]);
-		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
+		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
 	}
 	if (cf->can_dlc > 3) {
 		u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
-		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
+		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
 	}
 
-	writel(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
-	writel(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
+	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
 	kfree_skb(skb);
 
@@ -462,8 +487,8 @@ static void flexcan_read_fifo(const struct net_device *dev,
 	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
 	u32 reg_ctrl, reg_id;
 
-	reg_ctrl = readl(&mb->can_ctrl);
-	reg_id = readl(&mb->can_id);
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	reg_id = flexcan_read(&mb->can_id);
 	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
 		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -473,12 +498,12 @@ static void flexcan_read_fifo(const struct net_device *dev,
 		cf->can_id |= CAN_RTR_FLAG;
 	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
 
-	*(__be32 *)(cf->data + 0) = cpu_to_be32(readl(&mb->data[0]));
-	*(__be32 *)(cf->data + 4) = cpu_to_be32(readl(&mb->data[1]));
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(flexcan_read(&mb->data[0]));
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
 
 	/* mark as read */
-	writel(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
-	readl(&regs->timer);
+	flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
+	flexcan_read(&regs->timer);
 }
 
 static int flexcan_read_frame(struct net_device *dev)
@@ -514,17 +539,17 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	 * The error bits are cleared on read,
 	 * use saved value from irq handler.
 	 */
-	reg_esr = readl(&regs->esr) | priv->reg_esr;
+	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
 
 	/* handle state changes */
 	work_done += flexcan_poll_state(dev, reg_esr);
 
 	/* handle RX-FIFO */
-	reg_iflag1 = readl(&regs->iflag1);
+	reg_iflag1 = flexcan_read(&regs->iflag1);
 	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
 	       work_done < quota) {
 		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = readl(&regs->iflag1);
+		reg_iflag1 = flexcan_read(&regs->iflag1);
 	}
 
 	/* report bus errors */
@@ -534,8 +559,8 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	if (work_done < quota) {
 		napi_complete(napi);
 		/* enable IRQs */
-		writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		writel(priv->reg_ctrl_default, &regs->ctrl);
+		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
 	}
 
 	return work_done;
@@ -549,9 +574,9 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg_iflag1, reg_esr;
 
-	reg_iflag1 = readl(&regs->iflag1);
-	reg_esr = readl(&regs->esr);
-	writel(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
+	reg_iflag1 = flexcan_read(&regs->iflag1);
+	reg_esr = flexcan_read(&regs->esr);
+	flexcan_write(FLEXCAN_ESR_ERR_INT, &regs->esr);	/* ACK err IRQ */
 
 	/*
 	 * schedule NAPI in case of:
@@ -567,16 +592,16 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		 * save them for later use.
 		 */
 		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE,
-		       &regs->imask1);
-		writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
+		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
+			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
+		flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
 		       &regs->ctrl);
 		napi_schedule(&priv->napi);
 	}
 
 	/* FIFO overflow */
 	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
-		writel(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
+		flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
 		dev->stats.rx_over_errors++;
 		dev->stats.rx_errors++;
 	}
@@ -585,7 +610,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		/* tx_bytes is incremented in flexcan_start_xmit */
 		stats->tx_packets++;
-		writel((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
+		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
 		netif_wake_queue(dev);
 	}
 
@@ -599,7 +624,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg;
 
-	reg = readl(&regs->ctrl);
+	reg = flexcan_read(&regs->ctrl);
 	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
 		 FLEXCAN_CTRL_RJW(0x3) |
 		 FLEXCAN_CTRL_PSEG1(0x7) |
@@ -623,11 +648,11 @@ static void flexcan_set_bittiming(struct net_device *dev)
 		reg |= FLEXCAN_CTRL_SMP;
 
 	dev_info(dev->dev.parent, "writing ctrl=0x%08x\n", reg);
-	writel(reg, &regs->ctrl);
+	flexcan_write(reg, &regs->ctrl);
 
 	/* print chip status */
 	dev_dbg(dev->dev.parent, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
-		readl(&regs->mcr), readl(&regs->ctrl));
+		flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
 }
 
 /*
@@ -648,10 +673,10 @@ static int flexcan_chip_start(struct net_device *dev)
 	flexcan_chip_enable(priv);
 
 	/* soft reset */
-	writel(FLEXCAN_MCR_SOFTRST, &regs->mcr);
+	flexcan_write(FLEXCAN_MCR_SOFTRST, &regs->mcr);
 	udelay(10);
 
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	if (reg_mcr & FLEXCAN_MCR_SOFTRST) {
 		dev_err(dev->dev.parent,
 			"Failed to softreset can module (mcr=0x%08x)\n",
@@ -673,12 +698,12 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * choose format C
 	 *
 	 */
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
 		FLEXCAN_MCR_IDAM_C;
 	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
-	writel(reg_mcr, &regs->mcr);
+	flexcan_write(reg_mcr, &regs->mcr);
 
 	/*
 	 * CTRL
@@ -696,7 +721,7 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * (FLEXCAN_CTRL_ERR_MSK), too. Otherwise we don't get any
 	 * warning or bus passive interrupts.
 	 */
-	reg_ctrl = readl(&regs->ctrl);
+	reg_ctrl = flexcan_read(&regs->ctrl);
 	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
 	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
 		FLEXCAN_CTRL_ERR_STATE | FLEXCAN_CTRL_ERR_MSK;
@@ -704,38 +729,39 @@ static int flexcan_chip_start(struct net_device *dev)
 	/* save for later use */
 	priv->reg_ctrl_default = reg_ctrl;
 	dev_dbg(dev->dev.parent, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
-	writel(reg_ctrl, &regs->ctrl);
+	flexcan_write(reg_ctrl, &regs->ctrl);
 
 	for (i = 0; i < ARRAY_SIZE(regs->cantxfg); i++) {
-		writel(0, &regs->cantxfg[i].can_ctrl);
-		writel(0, &regs->cantxfg[i].can_id);
-		writel(0, &regs->cantxfg[i].data[0]);
-		writel(0, &regs->cantxfg[i].data[1]);
+		flexcan_write(0, &regs->cantxfg[i].can_ctrl);
+		flexcan_write(0, &regs->cantxfg[i].can_id);
+		flexcan_write(0, &regs->cantxfg[i].data[0]);
+		flexcan_write(0, &regs->cantxfg[i].data[1]);
 
 		/* put MB into rx queue */
-		writel(FLEXCAN_MB_CNT_CODE(0x4), &regs->cantxfg[i].can_ctrl);
+		flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
+			&regs->cantxfg[i].can_ctrl);
 	}
 
 	/* acceptance mask/acceptance code (accept everything) */
-	writel(0x0, &regs->rxgmask);
-	writel(0x0, &regs->rx14mask);
-	writel(0x0, &regs->rx15mask);
+	flexcan_write(0x0, &regs->rxgmask);
+	flexcan_write(0x0, &regs->rx14mask);
+	flexcan_write(0x0, &regs->rx15mask);
 
 	flexcan_transceiver_switch(priv, 1);
 
 	/* synchronize with the can bus */
-	reg_mcr = readl(&regs->mcr);
+	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr &= ~FLEXCAN_MCR_HALT;
-	writel(reg_mcr, &regs->mcr);
+	flexcan_write(reg_mcr, &regs->mcr);
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* enable FIFO interrupts */
-	writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+	flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
 
 	/* print chip status */
 	dev_dbg(dev->dev.parent, "%s: reading mcr=0x%08x ctrl=0x%08x\n",
-		__func__, readl(&regs->mcr), readl(&regs->ctrl));
+		__func__, flexcan_read(&regs->mcr), flexcan_read(&regs->ctrl));
 
 	return 0;
 
@@ -757,12 +783,12 @@ static void flexcan_chip_stop(struct net_device *dev)
 	u32 reg;
 
 	/* Disable all interrupts */
-	writel(0, &regs->imask1);
+	flexcan_write(0, &regs->imask1);
 
 	/* Disable + halt module */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	flexcan_transceiver_switch(priv, 0);
 	priv->can.state = CAN_STATE_STOPPED;
@@ -854,24 +880,24 @@ static int __devinit register_flexcandev(struct net_device *dev)
 
 	/* select "bus clock", chip must be disabled */
 	flexcan_chip_disable(priv);
-	reg = readl(&regs->ctrl);
+	reg = flexcan_read(&regs->ctrl);
 	reg |= FLEXCAN_CTRL_CLK_SRC;
-	writel(reg, &regs->ctrl);
+	flexcan_write(reg, &regs->ctrl);
 
 	flexcan_chip_enable(priv);
 
 	/* set freeze, halt and activate FIFO, restrict register access */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
-	writel(reg, &regs->mcr);
+	flexcan_write(reg, &regs->mcr);
 
 	/*
 	 * Currently we only support newer versions of this core
 	 * featuring a RX FIFO. Older cores found on some Coldfire
 	 * derivates are not yet supported.
 	 */
-	reg = readl(&regs->mcr);
+	reg = flexcan_read(&regs->mcr);
 	if (!(reg & FLEXCAN_MCR_FEN)) {
 		dev_err(dev->dev.parent,
 			"Could not enable RX FIFO, unsupported core\n");
-- 
1.7.2.1


^ permalink raw reply related

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Kumar Gala @ 2011-08-10 16:53 UTC (permalink / raw)
  To: Robin Holt
  Cc: Wood Scott-B07421, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	U Bhaskar-B22300,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
	Marc Kleine-Budde, PPC list, Wolfgang Grandegger
In-Reply-To: <20110810160054.GT4926-sJ/iWh9BUns@public.gmane.org>


On Aug 10, 2011, at 11:00 AM, Robin Holt wrote:

> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
>> 
>> 
>>> -----Original Message-----
>>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
>>> Sent: Wednesday, August 10, 2011 7:46 PM
>>> To: Wolfgang Grandegger
>>> Cc: Robin Holt; Marc Kleine-Budde; U Bhaskar-B22300; Wood Scott-B07421;
>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Kumar Gala; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; PPC
>>> list
>>> Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree
>>> binding.
>>> 
>>> On Wed, Aug 10, 2011 at 03:47:43PM +0200, Wolfgang Grandegger wrote:
>>>> Hi Robin,
>>>> 
>>>> On 08/10/2011 05:06 AM, Robin Holt wrote:
>>>>> In working with the socketcan developers, we have come to the
>>>>> conclusion the Documentation...fsl-flexcan.txt device tree
>>>>> documentation needs to be cleaned up.  The driver does not depend
>>>>> upon any properties other
>>>> 
>>>> Your first sentence could be misleading. Please just describe what the
>>>> patch does and why, something like:
>>>> 
>>>> "This patch cleans up the documentation of the device-tree binding for
>>>> the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
>>>> properties are not needed as the frequency of the source clock is
>>>> fixed..." and so on.
>>> 
>>> I borrowed heavily from your message. ;)
>>> 
>>>>> than the required properties so we are removing the file.
>>>>> Additionally, the p1010*dts* files are not following the standard
>>>>> for node naming in that they have a trailing -v1.0.
>>>> 
>>>>> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
>>>>> To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
>>>>> To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
>>>>> To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
>>>>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
>>>>> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
>>>>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>>>> ---
>>>>> .../devicetree/bindings/net/can/fsl-flexcan.txt    |   61 ----------
>>> ----------
>>>>> arch/powerpc/boot/dts/p1010rdb.dts                 |    8 ---
>>>>> arch/powerpc/boot/dts/p1010si.dtsi                 |    8 +-
>>>>> 3 files changed, 4 insertions(+), 73 deletions(-)  delete mode
>>>>> 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>> 
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>> deleted file mode 100644
>>>>> index 1a729f0..0000000
>>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>> +++ /dev/null
>>>>> @@ -1,61 +0,0 @@
>>>>> -CAN Device Tree Bindings
>>>>> -------------------------
>>>>> -2011 Freescale Semiconductor, Inc.
>>>>> -
>>>>> -fsl,flexcan-v1.0 nodes
>>>>> ------------------------
>>>>> -In addition to the required compatible-, reg- and
>>>>> interrupt-properties, you can -also specify which clock source shall
>>> be used for the controller.
>>>>> -
>>>>> -CPI Clock- Can Protocol Interface Clock
>>>>> -	This CLK_SRC bit of CTRL(control register) selects the clock source
>>> to
>>>>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
>>>>> -	(driven by the PLL) or the crystal oscillator clock. The selected
>>> clock
>>>>> -	is the one fed to the prescaler to generate the Serial Clock
>>> (Sclock).
>>>>> -	The PRESDIV field of CTRL(control register) controls a prescaler
>>> that
>>>>> -	generates the Serial Clock (Sclock), whose period defines the
>>>>> -	time quantum used to compose the CAN waveform.
>>>>> -
>>>>> -Can Engine Clock Source
>>>>> -	There are two sources for CAN clock
>>>>> -	- Platform Clock  It represents the bus clock
>>>>> -	- Oscillator Clock
>>>>> -
>>>>> -	Peripheral Clock (PLL)
>>>>> -	--------------
>>>>> -		     |
>>>>> -		    ---------		      -------------
>>>>> -		    |       |CPI Clock	      | Prescaler |       Sclock
>>>>> -		    |       |---------------->| (1.. 256) |------------>
>>>>> -		    ---------		      -------------
>>>>> -                     |  |
>>>>> -	--------------  ---------------------CLK_SRC
>>>>> -	Oscillator Clock
>>>>> -
>>>>> -- fsl,flexcan-clock-source : CAN Engine Clock Source.This property
>>> selects
>>>>> -			     the peripheral clock. PLL clock is fed to the
>>>>> -			     prescaler to generate the Serial Clock (Sclock).
>>>>> -			     Valid values are "oscillator" and "platform"
>>>>> -			     "oscillator": CAN engine clock source is
>>> oscillator clock.
>>>>> -			     "platform" The CAN engine clock source is the bus
>>> clock
>>>>> -		             (platform clock).
>>>>> -
>>>>> -- fsl,flexcan-clock-divider : for the reference and system clock, an
>>> additional
>>>>> -			      clock divider can be specified.
>>>>> -- clock-frequency: frequency required to calculate the bitrate for
>>> FlexCAN.
>>>>> -
>>>>> -Note:
>>>>> -	- v1.0 of flexcan-v1.0 represent the IP block version for P1010
>>> SOC.
>>>>> -	- P1010 does not have oscillator as the Clock Source.So the default
>>>>> -	  Clock Source is platform clock.
>>>>> -Examples:
>>>>> -
>>>>> -	can0@1c000 {
>>>>> -		compatible = "fsl,flexcan-v1.0";
>>>>> -		reg = <0x1c000 0x1000>;
>>>>> -		interrupts = <48 0x2>;
>>>>> -		interrupt-parent = <&mpic>;
>>>>> -		fsl,flexcan-clock-source = "platform";
>>>>> -		fsl,flexcan-clock-divqider = <2>;
>>>>> -		clock-frequency = <fixed by u-boot>;
>>>>> -	};
>>>> 
>>>> Do we really want to drop the documentation for that binding. I think
>>>> something like the following text would be still useful:
>>>> 
>>>> ------------------------
>>>> Flexcan CAN contoller on Freescale's ARM and PowerPC processors
>>>> 
>>>> Required properties:
>>>> 
>>>> - compatible : Should be "fsl,flexcan" and optionally
>>>>               "fsl,flexcan-<processor>"
>>>> - reg : Offset and length of the register set for this device
>>>> - interrupts : Interrupt tuple for this device
>>>> 
>>>> Example:
>>>> 
>>>>  can@1c000 {
>>>>          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
>>>>          reg = <0x1c000 0x1000>;
>>>>          interrupts = <48 0x2>;
>>>>          interrupt-parent = <&mpic>;
>>>>  };
>>>> -------------------------
>>> 
>>> Done, except the
>>>>          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
>>> 
>>> line is
>>> 	compatible = "fsl,flexcan", "fsl,flexcan-p1010";
>>> 
>>>> 
>>>> What do you think?
>>>> 
>>>>> diff --git a/arch/powerpc/boot/dts/p1010rdb.dts
>>>>> b/arch/powerpc/boot/dts/p1010rdb.dts
>>>>> index 6b33b73..d6a0bb2 100644
>>>>> --- a/arch/powerpc/boot/dts/p1010rdb.dts
>>>>> +++ b/arch/powerpc/boot/dts/p1010rdb.dts
>>>>> @@ -169,14 +169,6 @@
>>>>> 			};
>>>>> 		};
>>>>> 
>>>>> -		can0@1c000 {
>>>>> -			fsl,flexcan-clock-source = "platform";
>>>>> -		};
>>>>> -
>>>>> -		can1@1d000 {
>>>>> -			fsl,flexcan-clock-source = "platform";
>>>>> -		};
>>>>> -
>>>>> 		usb@22000 {
>>>>> 			phy_type = "utmi";
>>>>> 		};
>>>>> diff --git a/arch/powerpc/boot/dts/p1010si.dtsi
>>>>> b/arch/powerpc/boot/dts/p1010si.dtsi
>>>>> index 7f51104..20c396d 100644
>>>>> --- a/arch/powerpc/boot/dts/p1010si.dtsi
>>>>> +++ b/arch/powerpc/boot/dts/p1010si.dtsi
>>>>> @@ -141,19 +141,19 @@
>>>>> 		};
>>>>> 
>>>>> 		can0@1c000 {
>>>>> -			compatible = "fsl,flexcan-v1.0";
>>>>> +			compatible = "fsl,p1010-flexcan",
>>>>> +					"fsl,flexcan";
>>>> 
>>>> Does fit on one line.
>>>> 
>>>>> 			reg = <0x1c000 0x1000>;
>>>>> 			interrupts = <48 0x2>;
>>>>> 			interrupt-parent = <&mpic>;
>>>>> -			fsl,flexcan-clock-divider = <2>;
>>>>> 		};
>>>>> 
>>>>> 		can1@1d000 {
>>>>> -			compatible = "fsl,flexcan-v1.0";
>>>>> +			compatible = "fsl,p1010-flexcan",
>>>>> +					"fsl,flexcan";
>>>> 
>>>> Ditto
>>>> 
>>>>> 			reg = <0x1d000 0x1000>;
>>>>> 			interrupts = <61 0x2>;
>>>>> 			interrupt-parent = <&mpic>;
>>>>> -			fsl,flexcan-clock-divider = <2>;
>>>>> 		};
>>>>> 
>>>>> 		L2: l2-cache-controller@20000 {
>>>> 
>>>> Please also correct the node names (not using the number suffix).
>>> 
>>> So the node names should be
>>> 		can@1c000 {
>>> 		can@1d000 {
>>> correct?
>>> 
>> [Bhaskar] As there are two CAN controllers on P1010,So won't it be better
>> 	    to distinguish it by can0 and can1 instead by simple "can" ?
> 
> It looks like the way to do that is to assign a label to those devices
> and then associate the label with an alias.  I have no idea how that
> works under the hood, but it is the way other files are set up.  Take a
> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
> interfaces.
> 
> Grant or Wolfgang, is that the right way to handle the concern about
> names or does it have no practical effect with the Linux kernel?

It has not effect.  The label is just if you need to reference it via some other means.

- k

^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Scott Wood @ 2011-08-10 16:56 UTC (permalink / raw)
  To: Robin Holt
  Cc: Kumar Gala, Wolfgang Grandegger, U Bhaskar-B22300, Grant Likely,
	Marc Kleine-Budde, socketcan-core, netdev, PPC list,
	devicetree-discuss
In-Reply-To: <1312993670-23999-6-git-send-email-holt@sgi.com>

On 08/10/2011 11:27 AM, Robin Holt wrote:
> -CPI Clock- Can Protocol Interface Clock
> -	This CLK_SRC bit of CTRL(control register) selects the clock source to
> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
> -	(driven by the PLL) or the crystal oscillator clock. The selected clock
> -	is the one fed to the prescaler to generate the Serial Clock (Sclock).
> -	The PRESDIV field of CTRL(control register) controls a prescaler that
> -	generates the Serial Clock (Sclock), whose period defines the
> -	time quantum used to compose the CAN waveform.
> +- compatible : Should be "fsl,flexcan" and optionally
> +               "fsl,flexcan-<processor>"

fsl,<processor>-flexcan, and it should not be optional, and should come
before "fsl,flexcan".

Also may want to list fsl,p1010-rdb as a "canonical compatible" for
anything which is backwards compatible with p1010's implementation.

-Scott


^ permalink raw reply

* Re: [PATCH v11 4/5] powerpc: Add flexcan device support for p1010rdb.
From: Kumar Gala @ 2011-08-10 17:01 UTC (permalink / raw)
  To: Robin Holt
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	PPC list, Wolfgang Grandegger
In-Reply-To: <1312993670-23999-5-git-send-email-holt-sJ/iWh9BUns@public.gmane.org>


On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:

> I added a simple clock source for the p1010rdb so the flexcan driver
> could determine a clock frequency.  The p1010 flexcan device only has
> an oscillator of system bus frequency divided by 2.
> 
> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
> Acked-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
> Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
> Cc: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> ---
> arch/powerpc/platforms/85xx/Kconfig    |    2 +
> arch/powerpc/platforms/85xx/Makefile   |    2 +
> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
> 4 files changed, 64 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/clock.c

I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.

This either seems to global or I'm missing something.

I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.

- k

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 17:16 UTC (permalink / raw)
  To: Kumar Gala
  Cc: Wood Scott-B07421, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	U Bhaskar-B22300,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org, PPC list
In-Reply-To: <63306A24-3FBF-4DAA-A0B9-6005F56BB76F-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Wed, Aug 10, 2011 at 11:53:15AM -0500, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 11:00 AM, Robin Holt wrote:
> 
> > On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
> >> 
> >> 
> >>> -----Original Message-----
> >>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
> >>> Sent: Wednesday, August 10, 2011 7:46 PM
> >>> To: Wolfgang Grandegger
> >>> Cc: Robin Holt; Marc Kleine-Budde; U Bhaskar-B22300; Wood Scott-B07421;
> >>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Kumar Gala; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; PPC
> >>> list
> >>> Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree
> >>> binding.
> >>> 
> >>> On Wed, Aug 10, 2011 at 03:47:43PM +0200, Wolfgang Grandegger wrote:
> >>>> Hi Robin,
> >>>> 
> >>>> On 08/10/2011 05:06 AM, Robin Holt wrote:
> >>>>> In working with the socketcan developers, we have come to the
> >>>>> conclusion the Documentation...fsl-flexcan.txt device tree
> >>>>> documentation needs to be cleaned up.  The driver does not depend
> >>>>> upon any properties other
> >>>> 
> >>>> Your first sentence could be misleading. Please just describe what the
> >>>> patch does and why, something like:
> >>>> 
> >>>> "This patch cleans up the documentation of the device-tree binding for
> >>>> the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
> >>>> properties are not needed as the frequency of the source clock is
> >>>> fixed..." and so on.
> >>> 
> >>> I borrowed heavily from your message. ;)
> >>> 
> >>>>> than the required properties so we are removing the file.
> >>>>> Additionally, the p1010*dts* files are not following the standard
> >>>>> for node naming in that they have a trailing -v1.0.
> >>>> 
> >>>>> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
> >>>>> To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
> >>>>> To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
> >>>>> To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
> >>>>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> >>>>> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
> >>>>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> >>>>> ---
> >>>>> .../devicetree/bindings/net/can/fsl-flexcan.txt    |   61 ----------
> >>> ----------
> >>>>> arch/powerpc/boot/dts/p1010rdb.dts                 |    8 ---
> >>>>> arch/powerpc/boot/dts/p1010si.dtsi                 |    8 +-
> >>>>> 3 files changed, 4 insertions(+), 73 deletions(-)  delete mode
> >>>>> 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>> 
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>> deleted file mode 100644
> >>>>> index 1a729f0..0000000
> >>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>> +++ /dev/null
> >>>>> @@ -1,61 +0,0 @@
> >>>>> -CAN Device Tree Bindings
> >>>>> -------------------------
> >>>>> -2011 Freescale Semiconductor, Inc.
> >>>>> -
> >>>>> -fsl,flexcan-v1.0 nodes
> >>>>> ------------------------
> >>>>> -In addition to the required compatible-, reg- and
> >>>>> interrupt-properties, you can -also specify which clock source shall
> >>> be used for the controller.
> >>>>> -
> >>>>> -CPI Clock- Can Protocol Interface Clock
> >>>>> -	This CLK_SRC bit of CTRL(control register) selects the clock source
> >>> to
> >>>>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
> >>>>> -	(driven by the PLL) or the crystal oscillator clock. The selected
> >>> clock
> >>>>> -	is the one fed to the prescaler to generate the Serial Clock
> >>> (Sclock).
> >>>>> -	The PRESDIV field of CTRL(control register) controls a prescaler
> >>> that
> >>>>> -	generates the Serial Clock (Sclock), whose period defines the
> >>>>> -	time quantum used to compose the CAN waveform.
> >>>>> -
> >>>>> -Can Engine Clock Source
> >>>>> -	There are two sources for CAN clock
> >>>>> -	- Platform Clock  It represents the bus clock
> >>>>> -	- Oscillator Clock
> >>>>> -
> >>>>> -	Peripheral Clock (PLL)
> >>>>> -	--------------
> >>>>> -		     |
> >>>>> -		    ---------		      -------------
> >>>>> -		    |       |CPI Clock	      | Prescaler |       Sclock
> >>>>> -		    |       |---------------->| (1.. 256) |------------>
> >>>>> -		    ---------		      -------------
> >>>>> -                     |  |
> >>>>> -	--------------  ---------------------CLK_SRC
> >>>>> -	Oscillator Clock
> >>>>> -
> >>>>> -- fsl,flexcan-clock-source : CAN Engine Clock Source.This property
> >>> selects
> >>>>> -			     the peripheral clock. PLL clock is fed to the
> >>>>> -			     prescaler to generate the Serial Clock (Sclock).
> >>>>> -			     Valid values are "oscillator" and "platform"
> >>>>> -			     "oscillator": CAN engine clock source is
> >>> oscillator clock.
> >>>>> -			     "platform" The CAN engine clock source is the bus
> >>> clock
> >>>>> -		             (platform clock).
> >>>>> -
> >>>>> -- fsl,flexcan-clock-divider : for the reference and system clock, an
> >>> additional
> >>>>> -			      clock divider can be specified.
> >>>>> -- clock-frequency: frequency required to calculate the bitrate for
> >>> FlexCAN.
> >>>>> -
> >>>>> -Note:
> >>>>> -	- v1.0 of flexcan-v1.0 represent the IP block version for P1010
> >>> SOC.
> >>>>> -	- P1010 does not have oscillator as the Clock Source.So the default
> >>>>> -	  Clock Source is platform clock.
> >>>>> -Examples:
> >>>>> -
> >>>>> -	can0@1c000 {
> >>>>> -		compatible = "fsl,flexcan-v1.0";
> >>>>> -		reg = <0x1c000 0x1000>;
> >>>>> -		interrupts = <48 0x2>;
> >>>>> -		interrupt-parent = <&mpic>;
> >>>>> -		fsl,flexcan-clock-source = "platform";
> >>>>> -		fsl,flexcan-clock-divqider = <2>;
> >>>>> -		clock-frequency = <fixed by u-boot>;
> >>>>> -	};
> >>>> 
> >>>> Do we really want to drop the documentation for that binding. I think
> >>>> something like the following text would be still useful:
> >>>> 
> >>>> ------------------------
> >>>> Flexcan CAN contoller on Freescale's ARM and PowerPC processors
> >>>> 
> >>>> Required properties:
> >>>> 
> >>>> - compatible : Should be "fsl,flexcan" and optionally
> >>>>               "fsl,flexcan-<processor>"
> >>>> - reg : Offset and length of the register set for this device
> >>>> - interrupts : Interrupt tuple for this device
> >>>> 
> >>>> Example:
> >>>> 
> >>>>  can@1c000 {
> >>>>          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
> >>>>          reg = <0x1c000 0x1000>;
> >>>>          interrupts = <48 0x2>;
> >>>>          interrupt-parent = <&mpic>;
> >>>>  };
> >>>> -------------------------
> >>> 
> >>> Done, except the
> >>>>          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
> >>> 
> >>> line is
> >>> 	compatible = "fsl,flexcan", "fsl,flexcan-p1010";
> >>> 
> >>>> 
> >>>> What do you think?
> >>>> 
> >>>>> diff --git a/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>> b/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>> index 6b33b73..d6a0bb2 100644
> >>>>> --- a/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>> +++ b/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>> @@ -169,14 +169,6 @@
> >>>>> 			};
> >>>>> 		};
> >>>>> 
> >>>>> -		can0@1c000 {
> >>>>> -			fsl,flexcan-clock-source = "platform";
> >>>>> -		};
> >>>>> -
> >>>>> -		can1@1d000 {
> >>>>> -			fsl,flexcan-clock-source = "platform";
> >>>>> -		};
> >>>>> -
> >>>>> 		usb@22000 {
> >>>>> 			phy_type = "utmi";
> >>>>> 		};
> >>>>> diff --git a/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>> b/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>> index 7f51104..20c396d 100644
> >>>>> --- a/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>> +++ b/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>> @@ -141,19 +141,19 @@
> >>>>> 		};
> >>>>> 
> >>>>> 		can0@1c000 {
> >>>>> -			compatible = "fsl,flexcan-v1.0";
> >>>>> +			compatible = "fsl,p1010-flexcan",
> >>>>> +					"fsl,flexcan";
> >>>> 
> >>>> Does fit on one line.
> >>>> 
> >>>>> 			reg = <0x1c000 0x1000>;
> >>>>> 			interrupts = <48 0x2>;
> >>>>> 			interrupt-parent = <&mpic>;
> >>>>> -			fsl,flexcan-clock-divider = <2>;
> >>>>> 		};
> >>>>> 
> >>>>> 		can1@1d000 {
> >>>>> -			compatible = "fsl,flexcan-v1.0";
> >>>>> +			compatible = "fsl,p1010-flexcan",
> >>>>> +					"fsl,flexcan";
> >>>> 
> >>>> Ditto
> >>>> 
> >>>>> 			reg = <0x1d000 0x1000>;
> >>>>> 			interrupts = <61 0x2>;
> >>>>> 			interrupt-parent = <&mpic>;
> >>>>> -			fsl,flexcan-clock-divider = <2>;
> >>>>> 		};
> >>>>> 
> >>>>> 		L2: l2-cache-controller@20000 {
> >>>> 
> >>>> Please also correct the node names (not using the number suffix).
> >>> 
> >>> So the node names should be
> >>> 		can@1c000 {
> >>> 		can@1d000 {
> >>> correct?
> >>> 
> >> [Bhaskar] As there are two CAN controllers on P1010,So won't it be better
> >> 	    to distinguish it by can0 and can1 instead by simple "can" ?
> > 
> > It looks like the way to do that is to assign a label to those devices
> > and then associate the label with an alias.  I have no idea how that
> > works under the hood, but it is the way other files are set up.  Take a
> > look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
> > interfaces.
> > 
> > Grant or Wolfgang, is that the right way to handle the concern about
> > names or does it have no practical effect with the Linux kernel?
> 
> It has not effect.  The label is just if you need to reference it via some other means.

Does the alias have an effect?

Robin

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Kumar Gala @ 2011-08-10 17:17 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
	Wood Scott-B07421, U Bhaskar-B22300, PPC list,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20110810171600.GV4926-sJ/iWh9BUns@public.gmane.org>


On Aug 10, 2011, at 12:16 PM, Robin Holt wrote:

> On Wed, Aug 10, 2011 at 11:53:15AM -0500, Kumar Gala wrote:
>> 
>> On Aug 10, 2011, at 11:00 AM, Robin Holt wrote:
>> 
>>> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
>>>>> Sent: Wednesday, August 10, 2011 7:46 PM
>>>>> To: Wolfgang Grandegger
>>>>> Cc: Robin Holt; Marc Kleine-Budde; U Bhaskar-B22300; Wood Scott-B07421;
>>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Kumar Gala; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; PPC
>>>>> list
>>>>> Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree
>>>>> binding.
>>>>> 
>>>>> On Wed, Aug 10, 2011 at 03:47:43PM +0200, Wolfgang Grandegger wrote:
>>>>>> Hi Robin,
>>>>>> 
>>>>>> On 08/10/2011 05:06 AM, Robin Holt wrote:
>>>>>>> In working with the socketcan developers, we have come to the
>>>>>>> conclusion the Documentation...fsl-flexcan.txt device tree
>>>>>>> documentation needs to be cleaned up.  The driver does not depend
>>>>>>> upon any properties other
>>>>>> 
>>>>>> Your first sentence could be misleading. Please just describe what the
>>>>>> patch does and why, something like:
>>>>>> 
>>>>>> "This patch cleans up the documentation of the device-tree binding for
>>>>>> the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
>>>>>> properties are not needed as the frequency of the source clock is
>>>>>> fixed..." and so on.
>>>>> 
>>>>> I borrowed heavily from your message. ;)
>>>>> 
>>>>>>> than the required properties so we are removing the file.
>>>>>>> Additionally, the p1010*dts* files are not following the standard
>>>>>>> for node naming in that they have a trailing -v1.0.
>>>>>> 
>>>>>>> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
>>>>>>> To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
>>>>>>> To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
>>>>>>> To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
>>>>>>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
>>>>>>> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
>>>>>>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/net/can/fsl-flexcan.txt    |   61 ----------
>>>>> ----------
>>>>>>> arch/powerpc/boot/dts/p1010rdb.dts                 |    8 ---
>>>>>>> arch/powerpc/boot/dts/p1010si.dtsi                 |    8 +-
>>>>>>> 3 files changed, 4 insertions(+), 73 deletions(-)  delete mode
>>>>>>> 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>> 
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>> deleted file mode 100644
>>>>>>> index 1a729f0..0000000
>>>>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
>>>>>>> +++ /dev/null
>>>>>>> @@ -1,61 +0,0 @@
>>>>>>> -CAN Device Tree Bindings
>>>>>>> -------------------------
>>>>>>> -2011 Freescale Semiconductor, Inc.
>>>>>>> -
>>>>>>> -fsl,flexcan-v1.0 nodes
>>>>>>> ------------------------
>>>>>>> -In addition to the required compatible-, reg- and
>>>>>>> interrupt-properties, you can -also specify which clock source shall
>>>>> be used for the controller.
>>>>>>> -
>>>>>>> -CPI Clock- Can Protocol Interface Clock
>>>>>>> -	This CLK_SRC bit of CTRL(control register) selects the clock source
>>>>> to
>>>>>>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
>>>>>>> -	(driven by the PLL) or the crystal oscillator clock. The selected
>>>>> clock
>>>>>>> -	is the one fed to the prescaler to generate the Serial Clock
>>>>> (Sclock).
>>>>>>> -	The PRESDIV field of CTRL(control register) controls a prescaler
>>>>> that
>>>>>>> -	generates the Serial Clock (Sclock), whose period defines the
>>>>>>> -	time quantum used to compose the CAN waveform.
>>>>>>> -
>>>>>>> -Can Engine Clock Source
>>>>>>> -	There are two sources for CAN clock
>>>>>>> -	- Platform Clock  It represents the bus clock
>>>>>>> -	- Oscillator Clock
>>>>>>> -
>>>>>>> -	Peripheral Clock (PLL)
>>>>>>> -	--------------
>>>>>>> -		     |
>>>>>>> -		    ---------		      -------------
>>>>>>> -		    |       |CPI Clock	      | Prescaler |       Sclock
>>>>>>> -		    |       |---------------->| (1.. 256) |------------>
>>>>>>> -		    ---------		      -------------
>>>>>>> -                     |  |
>>>>>>> -	--------------  ---------------------CLK_SRC
>>>>>>> -	Oscillator Clock
>>>>>>> -
>>>>>>> -- fsl,flexcan-clock-source : CAN Engine Clock Source.This property
>>>>> selects
>>>>>>> -			     the peripheral clock. PLL clock is fed to the
>>>>>>> -			     prescaler to generate the Serial Clock (Sclock).
>>>>>>> -			     Valid values are "oscillator" and "platform"
>>>>>>> -			     "oscillator": CAN engine clock source is
>>>>> oscillator clock.
>>>>>>> -			     "platform" The CAN engine clock source is the bus
>>>>> clock
>>>>>>> -		             (platform clock).
>>>>>>> -
>>>>>>> -- fsl,flexcan-clock-divider : for the reference and system clock, an
>>>>> additional
>>>>>>> -			      clock divider can be specified.
>>>>>>> -- clock-frequency: frequency required to calculate the bitrate for
>>>>> FlexCAN.
>>>>>>> -
>>>>>>> -Note:
>>>>>>> -	- v1.0 of flexcan-v1.0 represent the IP block version for P1010
>>>>> SOC.
>>>>>>> -	- P1010 does not have oscillator as the Clock Source.So the default
>>>>>>> -	  Clock Source is platform clock.
>>>>>>> -Examples:
>>>>>>> -
>>>>>>> -	can0@1c000 {
>>>>>>> -		compatible = "fsl,flexcan-v1.0";
>>>>>>> -		reg = <0x1c000 0x1000>;
>>>>>>> -		interrupts = <48 0x2>;
>>>>>>> -		interrupt-parent = <&mpic>;
>>>>>>> -		fsl,flexcan-clock-source = "platform";
>>>>>>> -		fsl,flexcan-clock-divqider = <2>;
>>>>>>> -		clock-frequency = <fixed by u-boot>;
>>>>>>> -	};
>>>>>> 
>>>>>> Do we really want to drop the documentation for that binding. I think
>>>>>> something like the following text would be still useful:
>>>>>> 
>>>>>> ------------------------
>>>>>> Flexcan CAN contoller on Freescale's ARM and PowerPC processors
>>>>>> 
>>>>>> Required properties:
>>>>>> 
>>>>>> - compatible : Should be "fsl,flexcan" and optionally
>>>>>>              "fsl,flexcan-<processor>"
>>>>>> - reg : Offset and length of the register set for this device
>>>>>> - interrupts : Interrupt tuple for this device
>>>>>> 
>>>>>> Example:
>>>>>> 
>>>>>> can@1c000 {
>>>>>>         compatible = "fsl,p1010-flexcan", "fsl,flexcan";
>>>>>>         reg = <0x1c000 0x1000>;
>>>>>>         interrupts = <48 0x2>;
>>>>>>         interrupt-parent = <&mpic>;
>>>>>> };
>>>>>> -------------------------
>>>>> 
>>>>> Done, except the
>>>>>>         compatible = "fsl,p1010-flexcan", "fsl,flexcan";
>>>>> 
>>>>> line is
>>>>> 	compatible = "fsl,flexcan", "fsl,flexcan-p1010";
>>>>> 
>>>>>> 
>>>>>> What do you think?
>>>>>> 
>>>>>>> diff --git a/arch/powerpc/boot/dts/p1010rdb.dts
>>>>>>> b/arch/powerpc/boot/dts/p1010rdb.dts
>>>>>>> index 6b33b73..d6a0bb2 100644
>>>>>>> --- a/arch/powerpc/boot/dts/p1010rdb.dts
>>>>>>> +++ b/arch/powerpc/boot/dts/p1010rdb.dts
>>>>>>> @@ -169,14 +169,6 @@
>>>>>>> 			};
>>>>>>> 		};
>>>>>>> 
>>>>>>> -		can0@1c000 {
>>>>>>> -			fsl,flexcan-clock-source = "platform";
>>>>>>> -		};
>>>>>>> -
>>>>>>> -		can1@1d000 {
>>>>>>> -			fsl,flexcan-clock-source = "platform";
>>>>>>> -		};
>>>>>>> -
>>>>>>> 		usb@22000 {
>>>>>>> 			phy_type = "utmi";
>>>>>>> 		};
>>>>>>> diff --git a/arch/powerpc/boot/dts/p1010si.dtsi
>>>>>>> b/arch/powerpc/boot/dts/p1010si.dtsi
>>>>>>> index 7f51104..20c396d 100644
>>>>>>> --- a/arch/powerpc/boot/dts/p1010si.dtsi
>>>>>>> +++ b/arch/powerpc/boot/dts/p1010si.dtsi
>>>>>>> @@ -141,19 +141,19 @@
>>>>>>> 		};
>>>>>>> 
>>>>>>> 		can0@1c000 {
>>>>>>> -			compatible = "fsl,flexcan-v1.0";
>>>>>>> +			compatible = "fsl,p1010-flexcan",
>>>>>>> +					"fsl,flexcan";
>>>>>> 
>>>>>> Does fit on one line.
>>>>>> 
>>>>>>> 			reg = <0x1c000 0x1000>;
>>>>>>> 			interrupts = <48 0x2>;
>>>>>>> 			interrupt-parent = <&mpic>;
>>>>>>> -			fsl,flexcan-clock-divider = <2>;
>>>>>>> 		};
>>>>>>> 
>>>>>>> 		can1@1d000 {
>>>>>>> -			compatible = "fsl,flexcan-v1.0";
>>>>>>> +			compatible = "fsl,p1010-flexcan",
>>>>>>> +					"fsl,flexcan";
>>>>>> 
>>>>>> Ditto
>>>>>> 
>>>>>>> 			reg = <0x1d000 0x1000>;
>>>>>>> 			interrupts = <61 0x2>;
>>>>>>> 			interrupt-parent = <&mpic>;
>>>>>>> -			fsl,flexcan-clock-divider = <2>;
>>>>>>> 		};
>>>>>>> 
>>>>>>> 		L2: l2-cache-controller@20000 {
>>>>>> 
>>>>>> Please also correct the node names (not using the number suffix).
>>>>> 
>>>>> So the node names should be
>>>>> 		can@1c000 {
>>>>> 		can@1d000 {
>>>>> correct?
>>>>> 
>>>> [Bhaskar] As there are two CAN controllers on P1010,So won't it be better
>>>> 	    to distinguish it by can0 and can1 instead by simple "can" ?
>>> 
>>> It looks like the way to do that is to assign a label to those devices
>>> and then associate the label with an alias.  I have no idea how that
>>> works under the hood, but it is the way other files are set up.  Take a
>>> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
>>> interfaces.
>>> 
>>> Grant or Wolfgang, is that the right way to handle the concern about
>>> names or does it have no practical effect with the Linux kernel?
>> 
>> It has not effect.  The label is just if you need to reference it via some other means.
> 
> Does the alias have an effect?

nope

- k

^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 17:19 UTC (permalink / raw)
  To: Scott Wood
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, U Bhaskar-B22300,
	Kumar Gala, Grant Likely, Marc Kleine-Budde, PPC list,
	Wolfgang Grandegger
In-Reply-To: <4E42B83C.2040705-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

On Wed, Aug 10, 2011 at 11:56:28AM -0500, Scott Wood wrote:
> On 08/10/2011 11:27 AM, Robin Holt wrote:
> > -CPI Clock- Can Protocol Interface Clock
> > -	This CLK_SRC bit of CTRL(control register) selects the clock source to
> > -	the CAN Protocol Interface(CPI) to be either the peripheral clock
> > -	(driven by the PLL) or the crystal oscillator clock. The selected clock
> > -	is the one fed to the prescaler to generate the Serial Clock (Sclock).
> > -	The PRESDIV field of CTRL(control register) controls a prescaler that
> > -	generates the Serial Clock (Sclock), whose period defines the
> > -	time quantum used to compose the CAN waveform.
> > +- compatible : Should be "fsl,flexcan" and optionally
> > +               "fsl,flexcan-<processor>"
> 
> fsl,<processor>-flexcan, and it should not be optional, and should come
> before "fsl,flexcan".
> 
> Also may want to list fsl,p1010-rdb as a "canonical compatible" for
> anything which is backwards compatible with p1010's implementation.

How do I specify 'canonical compatible'?  What would be the use of it
in that implementation?

Robin

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 17:20 UTC (permalink / raw)
  To: Kumar Gala
  Cc: Wood Scott-B07421, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	U Bhaskar-B22300,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org, PPC list
In-Reply-To: <8C9817C6-735D-4B7C-A03B-5661C04C725A-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Wed, Aug 10, 2011 at 12:17:45PM -0500, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 12:16 PM, Robin Holt wrote:
> 
> > On Wed, Aug 10, 2011 at 11:53:15AM -0500, Kumar Gala wrote:
> >> 
> >> On Aug 10, 2011, at 11:00 AM, Robin Holt wrote:
> >> 
> >>> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
> >>>> 
> >>>> 
> >>>>> -----Original Message-----
> >>>>> From: Robin Holt [mailto:holt-sJ/iWh9BUns@public.gmane.org]
> >>>>> Sent: Wednesday, August 10, 2011 7:46 PM
> >>>>> To: Wolfgang Grandegger
> >>>>> Cc: Robin Holt; Marc Kleine-Budde; U Bhaskar-B22300; Wood Scott-B07421;
> >>>>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Kumar Gala; socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; PPC
> >>>>> list
> >>>>> Subject: Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree
> >>>>> binding.
> >>>>> 
> >>>>> On Wed, Aug 10, 2011 at 03:47:43PM +0200, Wolfgang Grandegger wrote:
> >>>>>> Hi Robin,
> >>>>>> 
> >>>>>> On 08/10/2011 05:06 AM, Robin Holt wrote:
> >>>>>>> In working with the socketcan developers, we have come to the
> >>>>>>> conclusion the Documentation...fsl-flexcan.txt device tree
> >>>>>>> documentation needs to be cleaned up.  The driver does not depend
> >>>>>>> upon any properties other
> >>>>>> 
> >>>>>> Your first sentence could be misleading. Please just describe what the
> >>>>>> patch does and why, something like:
> >>>>>> 
> >>>>>> "This patch cleans up the documentation of the device-tree binding for
> >>>>>> the Flexcan devices on Freescale's PowerPC and ARM cores. Extra
> >>>>>> properties are not needed as the frequency of the source clock is
> >>>>>> fixed..." and so on.
> >>>>> 
> >>>>> I borrowed heavily from your message. ;)
> >>>>> 
> >>>>>>> than the required properties so we are removing the file.
> >>>>>>> Additionally, the p1010*dts* files are not following the standard
> >>>>>>> for node naming in that they have a trailing -v1.0.
> >>>>>> 
> >>>>>>> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
> >>>>>>> To: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
> >>>>>>> To: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
> >>>>>>> To: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>> To: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>>>>>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
> >>>>>>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> >>>>>>> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
> >>>>>>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> >>>>>>> ---
> >>>>>>> .../devicetree/bindings/net/can/fsl-flexcan.txt    |   61 ----------
> >>>>> ----------
> >>>>>>> arch/powerpc/boot/dts/p1010rdb.dts                 |    8 ---
> >>>>>>> arch/powerpc/boot/dts/p1010si.dtsi                 |    8 +-
> >>>>>>> 3 files changed, 4 insertions(+), 73 deletions(-)  delete mode
> >>>>>>> 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>> 
> >>>>>>> diff --git
> >>>>>>> a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>> deleted file mode 100644
> >>>>>>> index 1a729f0..0000000
> >>>>>>> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> >>>>>>> +++ /dev/null
> >>>>>>> @@ -1,61 +0,0 @@
> >>>>>>> -CAN Device Tree Bindings
> >>>>>>> -------------------------
> >>>>>>> -2011 Freescale Semiconductor, Inc.
> >>>>>>> -
> >>>>>>> -fsl,flexcan-v1.0 nodes
> >>>>>>> ------------------------
> >>>>>>> -In addition to the required compatible-, reg- and
> >>>>>>> interrupt-properties, you can -also specify which clock source shall
> >>>>> be used for the controller.
> >>>>>>> -
> >>>>>>> -CPI Clock- Can Protocol Interface Clock
> >>>>>>> -	This CLK_SRC bit of CTRL(control register) selects the clock source
> >>>>> to
> >>>>>>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
> >>>>>>> -	(driven by the PLL) or the crystal oscillator clock. The selected
> >>>>> clock
> >>>>>>> -	is the one fed to the prescaler to generate the Serial Clock
> >>>>> (Sclock).
> >>>>>>> -	The PRESDIV field of CTRL(control register) controls a prescaler
> >>>>> that
> >>>>>>> -	generates the Serial Clock (Sclock), whose period defines the
> >>>>>>> -	time quantum used to compose the CAN waveform.
> >>>>>>> -
> >>>>>>> -Can Engine Clock Source
> >>>>>>> -	There are two sources for CAN clock
> >>>>>>> -	- Platform Clock  It represents the bus clock
> >>>>>>> -	- Oscillator Clock
> >>>>>>> -
> >>>>>>> -	Peripheral Clock (PLL)
> >>>>>>> -	--------------
> >>>>>>> -		     |
> >>>>>>> -		    ---------		      -------------
> >>>>>>> -		    |       |CPI Clock	      | Prescaler |       Sclock
> >>>>>>> -		    |       |---------------->| (1.. 256) |------------>
> >>>>>>> -		    ---------		      -------------
> >>>>>>> -                     |  |
> >>>>>>> -	--------------  ---------------------CLK_SRC
> >>>>>>> -	Oscillator Clock
> >>>>>>> -
> >>>>>>> -- fsl,flexcan-clock-source : CAN Engine Clock Source.This property
> >>>>> selects
> >>>>>>> -			     the peripheral clock. PLL clock is fed to the
> >>>>>>> -			     prescaler to generate the Serial Clock (Sclock).
> >>>>>>> -			     Valid values are "oscillator" and "platform"
> >>>>>>> -			     "oscillator": CAN engine clock source is
> >>>>> oscillator clock.
> >>>>>>> -			     "platform" The CAN engine clock source is the bus
> >>>>> clock
> >>>>>>> -		             (platform clock).
> >>>>>>> -
> >>>>>>> -- fsl,flexcan-clock-divider : for the reference and system clock, an
> >>>>> additional
> >>>>>>> -			      clock divider can be specified.
> >>>>>>> -- clock-frequency: frequency required to calculate the bitrate for
> >>>>> FlexCAN.
> >>>>>>> -
> >>>>>>> -Note:
> >>>>>>> -	- v1.0 of flexcan-v1.0 represent the IP block version for P1010
> >>>>> SOC.
> >>>>>>> -	- P1010 does not have oscillator as the Clock Source.So the default
> >>>>>>> -	  Clock Source is platform clock.
> >>>>>>> -Examples:
> >>>>>>> -
> >>>>>>> -	can0@1c000 {
> >>>>>>> -		compatible = "fsl,flexcan-v1.0";
> >>>>>>> -		reg = <0x1c000 0x1000>;
> >>>>>>> -		interrupts = <48 0x2>;
> >>>>>>> -		interrupt-parent = <&mpic>;
> >>>>>>> -		fsl,flexcan-clock-source = "platform";
> >>>>>>> -		fsl,flexcan-clock-divqider = <2>;
> >>>>>>> -		clock-frequency = <fixed by u-boot>;
> >>>>>>> -	};
> >>>>>> 
> >>>>>> Do we really want to drop the documentation for that binding. I think
> >>>>>> something like the following text would be still useful:
> >>>>>> 
> >>>>>> ------------------------
> >>>>>> Flexcan CAN contoller on Freescale's ARM and PowerPC processors
> >>>>>> 
> >>>>>> Required properties:
> >>>>>> 
> >>>>>> - compatible : Should be "fsl,flexcan" and optionally
> >>>>>>              "fsl,flexcan-<processor>"
> >>>>>> - reg : Offset and length of the register set for this device
> >>>>>> - interrupts : Interrupt tuple for this device
> >>>>>> 
> >>>>>> Example:
> >>>>>> 
> >>>>>> can@1c000 {
> >>>>>>         compatible = "fsl,p1010-flexcan", "fsl,flexcan";
> >>>>>>         reg = <0x1c000 0x1000>;
> >>>>>>         interrupts = <48 0x2>;
> >>>>>>         interrupt-parent = <&mpic>;
> >>>>>> };
> >>>>>> -------------------------
> >>>>> 
> >>>>> Done, except the
> >>>>>>         compatible = "fsl,p1010-flexcan", "fsl,flexcan";
> >>>>> 
> >>>>> line is
> >>>>> 	compatible = "fsl,flexcan", "fsl,flexcan-p1010";
> >>>>> 
> >>>>>> 
> >>>>>> What do you think?
> >>>>>> 
> >>>>>>> diff --git a/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>>>> b/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>>>> index 6b33b73..d6a0bb2 100644
> >>>>>>> --- a/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>>>> +++ b/arch/powerpc/boot/dts/p1010rdb.dts
> >>>>>>> @@ -169,14 +169,6 @@
> >>>>>>> 			};
> >>>>>>> 		};
> >>>>>>> 
> >>>>>>> -		can0@1c000 {
> >>>>>>> -			fsl,flexcan-clock-source = "platform";
> >>>>>>> -		};
> >>>>>>> -
> >>>>>>> -		can1@1d000 {
> >>>>>>> -			fsl,flexcan-clock-source = "platform";
> >>>>>>> -		};
> >>>>>>> -
> >>>>>>> 		usb@22000 {
> >>>>>>> 			phy_type = "utmi";
> >>>>>>> 		};
> >>>>>>> diff --git a/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>>>> b/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>>>> index 7f51104..20c396d 100644
> >>>>>>> --- a/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>>>> +++ b/arch/powerpc/boot/dts/p1010si.dtsi
> >>>>>>> @@ -141,19 +141,19 @@
> >>>>>>> 		};
> >>>>>>> 
> >>>>>>> 		can0@1c000 {
> >>>>>>> -			compatible = "fsl,flexcan-v1.0";
> >>>>>>> +			compatible = "fsl,p1010-flexcan",
> >>>>>>> +					"fsl,flexcan";
> >>>>>> 
> >>>>>> Does fit on one line.
> >>>>>> 
> >>>>>>> 			reg = <0x1c000 0x1000>;
> >>>>>>> 			interrupts = <48 0x2>;
> >>>>>>> 			interrupt-parent = <&mpic>;
> >>>>>>> -			fsl,flexcan-clock-divider = <2>;
> >>>>>>> 		};
> >>>>>>> 
> >>>>>>> 		can1@1d000 {
> >>>>>>> -			compatible = "fsl,flexcan-v1.0";
> >>>>>>> +			compatible = "fsl,p1010-flexcan",
> >>>>>>> +					"fsl,flexcan";
> >>>>>> 
> >>>>>> Ditto
> >>>>>> 
> >>>>>>> 			reg = <0x1d000 0x1000>;
> >>>>>>> 			interrupts = <61 0x2>;
> >>>>>>> 			interrupt-parent = <&mpic>;
> >>>>>>> -			fsl,flexcan-clock-divider = <2>;
> >>>>>>> 		};
> >>>>>>> 
> >>>>>>> 		L2: l2-cache-controller@20000 {
> >>>>>> 
> >>>>>> Please also correct the node names (not using the number suffix).
> >>>>> 
> >>>>> So the node names should be
> >>>>> 		can@1c000 {
> >>>>> 		can@1d000 {
> >>>>> correct?
> >>>>> 
> >>>> [Bhaskar] As there are two CAN controllers on P1010,So won't it be better
> >>>> 	    to distinguish it by can0 and can1 instead by simple "can" ?
> >>> 
> >>> It looks like the way to do that is to assign a label to those devices
> >>> and then associate the label with an alias.  I have no idea how that
> >>> works under the hood, but it is the way other files are set up.  Take a
> >>> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
> >>> interfaces.
> >>> 
> >>> Grant or Wolfgang, is that the right way to handle the concern about
> >>> names or does it have no practical effect with the Linux kernel?
> >> 
> >> It has not effect.  The label is just if you need to reference it via some other means.
> > 
> > Does the alias have an effect?
> 
> nope

Then how does the device number get associated with a particular device
and how is user-space ensured a consistent namespace?

Robin

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Kumar Gala @ 2011-08-10 17:26 UTC (permalink / raw)
  To: Robin Holt
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
	Wood Scott-B07421, U Bhaskar-B22300, PPC list,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20110810172040.GX4926-sJ/iWh9BUns@public.gmane.org>

>>>>>>> So the node names should be
>>>>>>> 		can@1c000 {
>>>>>>> 		can@1d000 {
>>>>>>> correct?
>>>>>>> 
>>>>>> [Bhaskar] As there are two CAN controllers on P1010,So won't it be better
>>>>>> 	    to distinguish it by can0 and can1 instead by simple "can" ?
>>>>> 
>>>>> It looks like the way to do that is to assign a label to those devices
>>>>> and then associate the label with an alias.  I have no idea how that
>>>>> works under the hood, but it is the way other files are set up.  Take a
>>>>> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
>>>>> interfaces.
>>>>> 
>>>>> Grant or Wolfgang, is that the right way to handle the concern about
>>>>> names or does it have no practical effect with the Linux kernel?
>>>> 
>>>> It has not effect.  The label is just if you need to reference it via some other means.
>>> 
>>> Does the alias have an effect?
>> 
>> nope
> 
> Then how does the device number get associated with a particular device

What do you mean by device number?

> and how is user-space ensured a consistent namespace?

that is left to udev rules.

- k

^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Scott Wood @ 2011-08-10 17:36 UTC (permalink / raw)
  To: Robin Holt
  Cc: Kumar Gala, Wolfgang Grandegger, U Bhaskar-B22300, Grant Likely,
	Marc Kleine-Budde, socketcan-core, netdev, PPC list,
	devicetree-discuss
In-Reply-To: <20110810171933.GW4926@sgi.com>

On 08/10/2011 12:19 PM, Robin Holt wrote:
> On Wed, Aug 10, 2011 at 11:56:28AM -0500, Scott Wood wrote:
>> On 08/10/2011 11:27 AM, Robin Holt wrote:
>>> -CPI Clock- Can Protocol Interface Clock
>>> -	This CLK_SRC bit of CTRL(control register) selects the clock source to
>>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
>>> -	(driven by the PLL) or the crystal oscillator clock. The selected clock
>>> -	is the one fed to the prescaler to generate the Serial Clock (Sclock).
>>> -	The PRESDIV field of CTRL(control register) controls a prescaler that
>>> -	generates the Serial Clock (Sclock), whose period defines the
>>> -	time quantum used to compose the CAN waveform.
>>> +- compatible : Should be "fsl,flexcan" and optionally
>>> +               "fsl,flexcan-<processor>"
>>
>> fsl,<processor>-flexcan, and it should not be optional, and should come
>> before "fsl,flexcan".
>>
>> Also may want to list fsl,p1010-rdb as a "canonical compatible" for
>> anything which is backwards compatible with p1010's implementation.
> 
> How do I specify 'canonical compatible'?

Something like:

  compatible: Should be "fsl,<processor>-flexcan" and "fsl,flexcan".

  An implementation should also claim any of the following compatibles
  that it is fully backwards compatible with:

  - fsl,p1010-rdb

> What would be the use of it in that implementation?

It limits the number of compatibles a driver has to care about, so you
don't need a huge ID table just to be able to figure out whether this is
a p1010-style flexcan or ARM-style.

-Scott


^ permalink raw reply

* Re: Use of 802.3ad bonding for increasing link throughput
From: Jay Vosburgh @ 2011-08-10 17:46 UTC (permalink / raw)
  To: Tom Brown; +Cc: netdev
In-Reply-To: <4E427499.8060108@cyconix.com>

Tom Brown <sa212+glibc@cyconix.com> wrote:

>[couldn't thread with '802.3ad bonding brain damaged', as I've just signed
>up]
>
>So, under what circumstances would a user actually use 802.3ad mode to
>"increase" link throughput, rather than just for redundancy? Are there any
>circumstances in which a single file, for example, could be transferred at
>multiple-NIC speed? 

	Network load balancing, by and large, increases throughput in
aggregate, not for individual connections.

[...] The 3 hashing options are:
>
>- layer 2: presumably this always puts traffic on the same NIC, even in a
>LAG with multiple NICs? Should layer 2 ever be used?

	Perhaps the network is such that the destinations are not
bonded, and can't handle more than 1 interface's worth of throughput.
Having the "server" end bonded still permits the clients deal with a
single IP address, handle failures of devices on the server, etc.

>- layer2+3: can't be used for a single file, since it still hashes to the
>same NIC, and can't be used for load-balancing, since different IP
>endpoints go unintelligently to different NICs
>
>- layer3+4: seems to have exactly the same issue as layer2+3, as well as
>being non-compliant
>
>I guess my problem is in understanding whether the 802.3/802.1AX spec has
>any use at all beyond redundancy. Given the requirement to maintain frame
>order at the distributor, I can't immediately see how having a bonded
>group of, say, 3 NICs is any better than having 3 separate NICs. Have I
>missed something obvious?

	Others have answered this part already (that it permits larger
aggregate throughput to/from the host, but not single-stream throughput
greater than one interface's worth).  This is by design, to prevent out
of order delivery of packets.

	An aggregate of N devices can be better than 3 individual
devices in that it will gracefully handle failure of one of the devices
in the aggregate, and permits sharing of the bandwidth in aggregate
without the peers having to be hard-coded to specific destinations.

>And, having said that, the redundancy features seem limited. For hot
>standby, when the main link fails, you have to wait for both ends to
>timeout, and re-negotiate via LACP, and hopefully pick up the same
>lower-priority NIC, and then rely on a higher layer to request
>retransmission of the missing frame. Do any of you have any experience of
>using 802.1AX for anything useful and non-trivial?

	In the linux implementation, as soon as the link goes down, that
port is removed from the aggregator and a new aggregator is selected
(which may be the same aggregator, depending on the option and
configuration).  Language in 802.1AX section 5.3.13 permits us to
immediately remove a failed port from an aggregator without waiting for
LACP to time out.

>So, to get multiple-NIC speed, are we stuck with balance-rr? But
>presumably this only works if the other end of the link is also running
>the bonding driver?

	Striping a single connection across multiple network interfaces
is very difficult to do without causing packets to be delivered out of
order.

	Now, that said, if you want to have one TCP connection utilize
more than one interface's worth of throughput, then yes, balance-rr is
the only mode that may do that.  The other end doesn't have to run
bonding, but it must have sufficient aggregate bandwidth to accomodate
the aggregate rate (e.g., N slower devices feeding into one faster
device).

	Running balance-rr itself can be tricky to configure.  An
unmanaged switch may not handle multiple ports with the same MAC address
very well (e.g., sending everything to one port, or sending everything
to all the ports).  A managed switch must have the relevant ports
configured for etherchannel ("static link aggregation" in some
documentation), and the switch may balance the traffic when it leaves
the switch using its transmit algorithm.  I'm not aware of any switches
that have a round-robin balance policy, so the switch may end up hashing
your traffic anyway (which will probably drop some of your packets,
because you're feeding them in faster than the switch can send them out
after they're hashed to one switch port).

	It's possible to play games on managed switches and, e.g., put
each pair of ports (one at each end) into a separate VLAN, but schemes
like that will fail badly if a link goes down somewhere.

	If each member of the bond goes through a different unmanaged
and not interconnected switch, that may avoid those issues (and this was
a common configuration back in the 10 Mb/sec days; it's described in
bonding.txt in more detail).  That configuration still has issues if a
link fails.  Connecting systems directly, back-to-back, should also
avoid those issues.

	Lastly, balance-rr will deliver traffic out of order.  Even the
best case, N slow links feeding one faster link, delivers some small
percentage out of order (in the low single digits).

	On linux, the tcp_reordering sysctl value can be raised to
compensate, but it will still result in increased packet overhead, and
is not likely to be very efficient, and doesn't help with anything
that's not TCP/IP.  I have not tested balance-rr in a few years now, but
my recollection is that, as a best case, throughput of one TCP
connection could reach about 1.5x with 2 slaves, or about 2.5x with 4
slaves (where the multipliers are in units of "bandwidth of one slave").

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

^ permalink raw reply

* Re: [PATCH v11 4/5] powerpc: Add flexcan device support for p1010rdb.
From: Wolfgang Grandegger @ 2011-08-10 18:16 UTC (permalink / raw)
  To: Kumar Gala
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, U Bhaskar-B22300,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Marc Kleine-Budde,
	PPC list
In-Reply-To: <8E5FA886-038D-4DF4-8A54-DD60188A21A2-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On 08/10/2011 07:01 PM, Kumar Gala wrote:
> 
> On Aug 10, 2011, at 11:27 AM, Robin Holt wrote:
> 
>> I added a simple clock source for the p1010rdb so the flexcan driver
>> could determine a clock frequency.  The p1010 flexcan device only has
>> an oscillator of system bus frequency divided by 2.
>>
>> Signed-off-by: Robin Holt <holt-sJ/iWh9BUns@public.gmane.org>
>> Acked-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
>> Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>,
>> Cc: U Bhaskar-B22300 <B22300-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
>> Cc: PPC list <linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>
>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> ---
>> arch/powerpc/platforms/85xx/Kconfig    |    2 +
>> arch/powerpc/platforms/85xx/Makefile   |    2 +
>> arch/powerpc/platforms/85xx/clock.c    |   52 ++++++++++++++++++++++++++++++++
>> arch/powerpc/platforms/85xx/p1010rdb.c |    8 +++++
>> 4 files changed, 64 insertions(+), 0 deletions(-)
>> create mode 100644 arch/powerpc/platforms/85xx/clock.c
> 
> I dont understand how mpc85xx_clk_functions() ends up being associated with the frequency the flexcan is running at.

The function mpc85xx_clk_get_rate() returns "fsl_get_sys_freq() / 2" for
Flexcan devices.

> This either seems to global or I'm missing something.

This patch extends the existing Flexcan platform driver for ARM for the
PowerPC using the device tree. Due to the nice integration of the device
tree (of-platform) into the platform driver and devices, the difference
are quite small (see patches 1..3). Apart from the endianess issue, only
the clock needs to be handled in a common way. As ARM already uses the
clk interface, we found it straight-forward to implement it for the
P1010, or more general for the 85xx, as well, instead of using an
additional helper function.

> I still think the clk / freq info should be in the device tree and handled in the driver and NOT arch/powerpc platform code.

If I understand you correctly, you want the boot-loader to provide the
relevant information by fixing up the device tree, which then can be
handled arch-independently by the driver, right?

Wolfgang.

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Wolfgang Grandegger @ 2011-08-10 18:23 UTC (permalink / raw)
  To: Robin Holt
  Cc: Wood Scott-B07421, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	U Bhaskar-B22300, Kumar Gala,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
	Marc Kleine-Budde, PPC list
In-Reply-To: <20110810160054.GT4926-sJ/iWh9BUns@public.gmane.org>

On 08/10/2011 06:00 PM, Robin Holt wrote:
> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
...
> It looks like the way to do that is to assign a label to those devices
> and then associate the label with an alias.  I have no idea how that
> works under the hood, but it is the way other files are set up.  Take a
> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
> interfaces.

With a label you mean "label:" at the beginning of a node. Such labels
are translated by the device tree compiler in node handles, which can be
referenced within nodes by using <&label>, e.g.:

UIC0: interrupt-controller0 {
	...
};
UIC1: interrupt-controller1 {
	...
	interrupt-parent = <&UIC0>;
	...
};

It has nothing to do with the name of the node.

Wolfgang.

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Scott Wood @ 2011-08-10 18:27 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: Robin Holt, U Bhaskar-B22300, Wood Scott-B07421,
	netdev@vger.kernel.org, Kumar Gala,
	socketcan-core@lists.berlios.de, Marc Kleine-Budde, PPC list
In-Reply-To: <4E42CCB0.8090803@grandegger.com>

On 08/10/2011 01:23 PM, Wolfgang Grandegger wrote:
> On 08/10/2011 06:00 PM, Robin Holt wrote:
>> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
> ...
>> It looks like the way to do that is to assign a label to those devices
>> and then associate the label with an alias.  I have no idea how that
>> works under the hood, but it is the way other files are set up.  Take a
>> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
>> interfaces.
> 
> With a label you mean "label:" at the beginning of a node. Such labels
> are translated by the device tree compiler in node handles, which can be
> referenced within nodes by using <&label>, e.g.:
> 
> UIC0: interrupt-controller0 {
> 	...
> };
> UIC1: interrupt-controller1 {
> 	...
> 	interrupt-parent = <&UIC0>;
> 	...
> };
> 
> It has nothing to do with the name of the node.

"...and then associate the label with an alias."

The alias can then be used if you want "can0" versus "can1".

Appending numbers to the node name is typically only done when there's
no unit address, and a need to disambiguate.

-Scott


^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 18:30 UTC (permalink / raw)
  To: Scott Wood
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, U Bhaskar-B22300,
	Kumar Gala, Grant Likely, Marc Kleine-Budde, PPC list,
	Wolfgang Grandegger
In-Reply-To: <4E42C196.7030708-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

On Wed, Aug 10, 2011 at 12:36:22PM -0500, Scott Wood wrote:
> On 08/10/2011 12:19 PM, Robin Holt wrote:
> > On Wed, Aug 10, 2011 at 11:56:28AM -0500, Scott Wood wrote:
> >> On 08/10/2011 11:27 AM, Robin Holt wrote:
> >>> -CPI Clock- Can Protocol Interface Clock
> >>> -	This CLK_SRC bit of CTRL(control register) selects the clock source to
> >>> -	the CAN Protocol Interface(CPI) to be either the peripheral clock
> >>> -	(driven by the PLL) or the crystal oscillator clock. The selected clock
> >>> -	is the one fed to the prescaler to generate the Serial Clock (Sclock).
> >>> -	The PRESDIV field of CTRL(control register) controls a prescaler that
> >>> -	generates the Serial Clock (Sclock), whose period defines the
> >>> -	time quantum used to compose the CAN waveform.
> >>> +- compatible : Should be "fsl,flexcan" and optionally
> >>> +               "fsl,flexcan-<processor>"
> >>
> >> fsl,<processor>-flexcan, and it should not be optional, and should come
> >> before "fsl,flexcan".
> >>
> >> Also may want to list fsl,p1010-rdb as a "canonical compatible" for
> >> anything which is backwards compatible with p1010's implementation.
> > 
> > How do I specify 'canonical compatible'?
> 
> Something like:
> 
>   compatible: Should be "fsl,<processor>-flexcan" and "fsl,flexcan".
> 
>   An implementation should also claim any of the following compatibles
>   that it is fully backwards compatible with:
> 
>   - fsl,p1010-rdb

I am so confused.  fsl,p1010-flexcan refers, in my mind at least, to
a particular chiplet on the p1010 freescale processor.  fsl,p1010-rdb
would mean nothing to me as that is a p1010 processor with two flexcan
chiplets wired to a pair of DB-9 jacks.  For the driver, what additional
information is being conveyed?

Let's cut to the chase.  Here is what I have after incorporating your
earlier comment about the compatible line.  Please mark this up to
exactly what you are asking for.

Thanks,
Robin
------------------------------------------------------------------------
Flexcan CAN contoller on Freescale's ARM and PowerPC processors

Required properties:

- compatible : Should be "fsl,<processor>-flexcan" and "fsl,flexcan"
- reg : Offset and length of the register set for this device
- interrupts : Interrupt tuple for this device

Example:

  can@1c000 {
          compatible = "fsl,p1010-flexcan", "fsl,flexcan";
          reg = <0x1c000 0x1000>;
          interrupts = <48 0x2>;
          interrupt-parent = <&mpic>;
  };

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 18:35 UTC (permalink / raw)
  To: Scott Wood
  Cc: Wood Scott-B07421, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	U Bhaskar-B22300, Kumar Gala,
	socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org,
	Marc Kleine-Budde, PPC list, Wolfgang Grandegger
In-Reply-To: <4E42CDA8.5050902-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

On Wed, Aug 10, 2011 at 01:27:52PM -0500, Scott Wood wrote:
> On 08/10/2011 01:23 PM, Wolfgang Grandegger wrote:
> > On 08/10/2011 06:00 PM, Robin Holt wrote:
> >> On Wed, Aug 10, 2011 at 02:36:20PM +0000, U Bhaskar-B22300 wrote:
> > ...
> >> It looks like the way to do that is to assign a label to those devices
> >> and then associate the label with an alias.  I have no idea how that
> >> works under the hood, but it is the way other files are set up.  Take a
> >> look at arch/powerpc/boot/dts/bamboo.dts for how they define the serial
> >> interfaces.
> > 
> > With a label you mean "label:" at the beginning of a node. Such labels
> > are translated by the device tree compiler in node handles, which can be
> > referenced within nodes by using <&label>, e.g.:
> > 
> > UIC0: interrupt-controller0 {
> > 	...
> > };
> > UIC1: interrupt-controller1 {
> > 	...
> > 	interrupt-parent = <&UIC0>;
> > 	...
> > };
> > 
> > It has nothing to do with the name of the node.
> 
> "...and then associate the label with an alias."
> 
> The alias can then be used if you want "can0" versus "can1".

Does the alias get used by either the kernel or something else or is it
just extra detail with no purpose?

Robin

^ permalink raw reply

* Re: [PATCH v10 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
From: Scott Wood @ 2011-08-10 18:39 UTC (permalink / raw)
  To: Robin Holt
  Cc: Wolfgang Grandegger, U Bhaskar-B22300, Wood Scott-B07421,
	netdev@vger.kernel.org, Kumar Gala,
	socketcan-core@lists.berlios.de, Marc Kleine-Budde, PPC list
In-Reply-To: <20110810183526.GZ4926@sgi.com>

On 08/10/2011 01:35 PM, Robin Holt wrote:
> On Wed, Aug 10, 2011 at 01:27:52PM -0500, Scott Wood wrote:
>> "...and then associate the label with an alias."
>>
>> The alias can then be used if you want "can0" versus "can1".
> 
> Does the alias get used by either the kernel or something else or is it
> just extra detail with no purpose?

It could be used by udev to produce a symlink.  Currently, the major
non-human consumer of the aliases is U-Boot.

-Scott


^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Scott Wood @ 2011-08-10 18:40 UTC (permalink / raw)
  To: Robin Holt
  Cc: Kumar Gala, Wolfgang Grandegger, U Bhaskar-B22300, Grant Likely,
	Marc Kleine-Budde, socketcan-core, netdev, PPC list,
	devicetree-discuss
In-Reply-To: <20110810183016.GY4926@sgi.com>

On 08/10/2011 01:30 PM, Robin Holt wrote:
> On Wed, Aug 10, 2011 at 12:36:22PM -0500, Scott Wood wrote:
>> On 08/10/2011 12:19 PM, Robin Holt wrote:
>>> On Wed, Aug 10, 2011 at 11:56:28AM -0500, Scott Wood wrote:
>>>> Also may want to list fsl,p1010-rdb as a "canonical compatible" for
>>>> anything which is backwards compatible with p1010's implementation.
>>>
>>> How do I specify 'canonical compatible'?
>>
>> Something like:
>>
>>   compatible: Should be "fsl,<processor>-flexcan" and "fsl,flexcan".
>>
>>   An implementation should also claim any of the following compatibles
>>   that it is fully backwards compatible with:
>>
>>   - fsl,p1010-rdb

Gah, I don't know how "rdb" replaced "flexcan" in the above.  Sorry for
any confusion.

> I am so confused.  fsl,p1010-flexcan refers, in my mind at least, to
> a particular chiplet on the p1010 freescale processor. 

It refers to a particular version of the flexcan logic, for which the
hardware doc people weren't kind enough to give us a public version number.

It has been common and recommended practice in such cases, when there
are multiple chips containing the same device, to pick a canonical chip
(such as the first one to have the device or to be supported) and have
others claim compatibility with it.

> fsl,p1010-rdb
> would mean nothing to me as that is a p1010 processor with two flexcan
> chiplets wired to a pair of DB-9 jacks.  For the driver, what additional
> information is being conveyed?

The programming model of the flexcan chiplet.

> Let's cut to the chase.  Here is what I have after incorporating your
> earlier comment about the compatible line.  Please mark this up to
> exactly what you are asking for.
> 
> Thanks,
> Robin
> ------------------------------------------------------------------------
> Flexcan CAN contoller on Freescale's ARM and PowerPC processors
> 
> Required properties:
> 
> - compatible : Should be "fsl,<processor>-flexcan" and "fsl,flexcan"

   An implementation should also claim any of the following compatibles
   that it is fully backwards compatible with:

   - fsl,p1010-flexcan

> - reg : Offset and length of the register set for this device
> - interrupts : Interrupt tuple for this device
> 
> Example:
> 
>   can@1c000 {
>           compatible = "fsl,p1010-flexcan", "fsl,flexcan";
>           reg = <0x1c000 0x1000>;
>           interrupts = <48 0x2>;
>           interrupt-parent = <&mpic>;
>   };
> 

-Scott


^ permalink raw reply

* Re: [PATCH v11 5/5] powerpc: Fix up fsl-flexcan device tree binding.
From: Robin Holt @ 2011-08-10 18:45 UTC (permalink / raw)
  To: Scott Wood
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, U Bhaskar-B22300,
	Kumar Gala, Grant Likely, Marc Kleine-Budde, PPC list,
	Wolfgang Grandegger
In-Reply-To: <4E42D09E.4080405-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

On Wed, Aug 10, 2011 at 01:40:30PM -0500, Scott Wood wrote:
> On 08/10/2011 01:30 PM, Robin Holt wrote:
> > On Wed, Aug 10, 2011 at 12:36:22PM -0500, Scott Wood wrote:
> >> On 08/10/2011 12:19 PM, Robin Holt wrote:
> >>> On Wed, Aug 10, 2011 at 11:56:28AM -0500, Scott Wood wrote:
> >>>> Also may want to list fsl,p1010-rdb as a "canonical compatible" for
> >>>> anything which is backwards compatible with p1010's implementation.
> >>>
> >>> How do I specify 'canonical compatible'?
> >>
> >> Something like:
> >>
> >>   compatible: Should be "fsl,<processor>-flexcan" and "fsl,flexcan".
> >>
> >>   An implementation should also claim any of the following compatibles
> >>   that it is fully backwards compatible with:
> >>
> >>   - fsl,p1010-rdb
> 
> Gah, I don't know how "rdb" replaced "flexcan" in the above.  Sorry for
> any confusion.
> 
> > I am so confused.  fsl,p1010-flexcan refers, in my mind at least, to
> > a particular chiplet on the p1010 freescale processor. 
> 
> It refers to a particular version of the flexcan logic, for which the
> hardware doc people weren't kind enough to give us a public version number.
> 
> It has been common and recommended practice in such cases, when there
> are multiple chips containing the same device, to pick a canonical chip
> (such as the first one to have the device or to be supported) and have
> others claim compatibility with it.
> 
> > fsl,p1010-rdb
> > would mean nothing to me as that is a p1010 processor with two flexcan
> > chiplets wired to a pair of DB-9 jacks.  For the driver, what additional
> > information is being conveyed?
> 
> The programming model of the flexcan chiplet.
> 
> > Let's cut to the chase.  Here is what I have after incorporating your
> > earlier comment about the compatible line.  Please mark this up to
> > exactly what you are asking for.
> > 
> > Thanks,
> > Robin
> > ------------------------------------------------------------------------
> > Flexcan CAN contoller on Freescale's ARM and PowerPC processors
> > 
> > Required properties:
> > 
> > - compatible : Should be "fsl,<processor>-flexcan" and "fsl,flexcan"
> 
>    An implementation should also claim any of the following compatibles
>    that it is fully backwards compatible with:
> 
>    - fsl,p1010-flexcan

Ah, there is my confusion.  I did not realize you were saying the
entire preceeding 4 lines should be included.  I thought you were
making a comment which I did not understand.

Thank you for your patience with my ignorance,
Robin

^ permalink raw reply

* [PATCH] tcp: initialize variable ecn_ok in syncookies path
From: Mike Waychison @ 2011-08-10 18:51 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy <kabe
  Cc: netdev, linux-kernel, Mike Waychison

Using a gcc 4.4.3, warnings are emitted for a possibly uninitialized use
of ecn_ok.

This can happen if cookie_check_timestamp() returns due to not having
seen a timestamp.  Defaulting to ecn off seems like a reasonable thing to do in this case, so initialized ecn_ok to false.

Signed-off-by: Mike Waychison <mikew@google.com>
---
 net/ipv4/syncookies.c |    2 +-
 net/ipv6/syncookies.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 92bb943..3bc5c8f 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -276,7 +276,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	int mss;
 	struct rtable *rt;
 	__u8 rcv_wscale;
-	bool ecn_ok;
+	bool ecn_ok = false;
 
 	if (!sysctl_tcp_syncookies || !th->ack || th->rst)
 		goto out;
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 89d5bf8..ac83896 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -165,7 +165,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	int mss;
 	struct dst_entry *dst;
 	__u8 rcv_wscale;
-	bool ecn_ok;
+	bool ecn_ok = false;
 
 	if (!sysctl_tcp_syncookies || !th->ack || th->rst)
 		goto out;
-- 
1.7.3.1

^ permalink raw reply related

* Re: Intel 82599 ixgbe driver performance
From: Martin Josefsson @ 2011-08-10 19:18 UTC (permalink / raw)
  To: J.Hwan Kim; +Cc: netdev
In-Reply-To: <4E4222F6.7050304@gmail.com>

On Wed, Aug 10, 2011 at 8:19 AM, J.Hwan Kim <frog1120@gmail.com> wrote:

> I'm testing our network card which includes intel 82599 based on ixgbe driver.
> I wonder what is the Rx performance of i82599 without network stack only with 64Byte frames.
> Our driver reads the packet directly from DMA packet buffer and push to the application
> without passing through linux kernel stack.
> It seems that the intel 82599 cannot push 64B frames to DMA area in 10G.
> Is it right?
>
> If it is the case, what is the bottleneck of 82599?

My experience with 82599 is that it can RX 13.4 Mpps using 64byte
frames and a single port.
When using both ports the rate drops to 10.7 Mpps per port.

Note that my experience does not involve the ixgbe driver, these
numbers were obtained using a custom driver and OS, but should give
some indication of what the hardware is capable of.

(If you want to see something "fun", try 65byte packets with 82599 and
the Intel X58 IOH (or 5500/5520 server versions) :)

--
/Martin

^ permalink raw reply

* Re: Intel 82599 ixgbe driver performance
From: Rick Jones @ 2011-08-10 20:58 UTC (permalink / raw)
  To: J.Hwan Kim; +Cc: netdev, rizzo
In-Reply-To: <4E4222F6.7050304@gmail.com>

On 08/09/2011 11:19 PM, J.Hwan Kim wrote:
> Hi, everyone
>
> I'm testing our network card which includes intel 82599 based on
> ixgbe driver. I wonder what is the Rx performance of i82599 without
> network stack only with 64Byte frames. Our driver reads the packet
> directly from DMA packet buffer and push to the application without
> passing through linux kernel stack. It seems that the intel 82599
> cannot push 64B frames to DMA area in 10G. Is it right?

Does your driver perform a copy of that 64B frame to user space?

Is this a single-threaded test running?

What does an lat_mem_rd -t (-t for random stride) test from lmbench give 
for your system's memory latency?  (Perhaps using numactl to ensure 
local, or remote memory access, as you desire)

At line rate for minimum sized frames over 10 GbE, you have a frame 
arriving every 60-odd nanoseconds. At that speed, you cannot take even 
one cache miss per frame (*) in a single-threaded path and still achieve 
line-rate PPS.

As it happens, there was a presentation at HP Labs recently, given by 
Luigi Rizzo on his netmap work.  The slides can be found at
http://info.iet.unipi.it/~luigi/netmap/talk-hp.html .  As it happens, 
Luigi presented some performance figures using an Intel 82599.

happy benchmarking,

rick jones
(*) much of my time has been spent in a world where a cache miss is 
three digits worth of nanoseconds (to the left of the decimal), 
sometimes high two digits.

^ permalink raw reply

* Move interface across network namespaces
From: Renato Westphal @ 2011-08-11  0:31 UTC (permalink / raw)
  To: netdev, Eric W. Biederman

Hello,

I have two questions regarding the process of moving a network
interface across different network namespaces:

* When I move an interface, all the virtual interfaces attached to it
are deleted. Is there any reason for such odd behavior? I would like
to move some network interfaces and keep the attached vlans untouched.

* The target network namespace sends a RTM_NEWLINK netlink message
when an interface is moved to it. In the other hand, the source
network namespace doesn't sends a RTM_DELLINK message when an
interface is moved from it. This is very annoying because user space
applications (such as zebra) can't detect some interface moving
operations and then get into an inconsistent state. Anyone knows if
there's a workaround for this?

Any help would be appreciated.

Best Regards,
Renato.

-- 
Renato Westphal

^ permalink raw reply

* [net-next 01/10] drivers/net/ethernet: Add ethernet dir and config option
From: Jeff Kirsher @ 2011-08-11  3:27 UTC (permalink / raw)
  To: davem; +Cc: Jeff Kirsher, netdev, gospo, sassmann
In-Reply-To: <1313033278-7337-1-git-send-email-jeffrey.t.kirsher@intel.com>

This is the initial patch to organize the drivers/net directory
structure and networking device driver config options.  This patch
does the following:
  - add drivers/net/ethernet/Kconfig
  - integrate the new files into the existing config

Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/Kconfig           |    2 ++
 drivers/net/Makefile          |    2 +-
 drivers/net/ethernet/Kconfig  |   14 ++++++++++++++
 drivers/net/ethernet/Makefile |    3 +++
 4 files changed, 20 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/ethernet/Kconfig
 create mode 100644 drivers/net/ethernet/Makefile

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 8d0314d..5b95796 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -193,6 +193,8 @@ source "drivers/net/phy/Kconfig"
 #	Ethernet
 #
 
+source "drivers/net/ethernet/Kconfig"
+
 menuconfig NET_ETHERNET
 	bool "Ethernet (10 or 100Mbit)"
 	depends on !UML
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index e1eca2a..670b514 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -1,5 +1,5 @@
 #
-# Makefile for the Linux network (ethercard) device drivers.
+# Makefile for the Linux network device drivers.
 #
 
 obj-$(CONFIG_MII) += mii.o
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
new file mode 100644
index 0000000..d59e4f2
--- /dev/null
+++ b/drivers/net/ethernet/Kconfig
@@ -0,0 +1,14 @@
+#
+# Ethernet LAN device configuration
+#
+
+menuconfig ETHERNET
+	bool "Ethernet driver support"
+	depends on NET
+	default y
+	---help---
+	  This section contains all the Ethernet device drivers.
+
+if ETHERNET
+
+endif # ETHERNET
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
new file mode 100644
index 0000000..0d21dda
--- /dev/null
+++ b/drivers/net/ethernet/Makefile
@@ -0,0 +1,3 @@
+#
+# Makefile for the Linux network Ethernet device drivers.
+#
-- 
1.7.6


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox