* [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
@ 2014-06-24 15:54 ` Bhupesh Sharma
0 siblings, 0 replies; 16+ messages in thread
From: Bhupesh Sharma @ 2014-06-24 15:54 UTC (permalink / raw)
To: mkl, linux-can; +Cc: wg, netdev, Bhupesh Sharma
The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is
modelled in a big-endian fashion, i.e. the registers and the
message buffers are organized in a BE way.
More details about the LS1021A SoC can be seen here:
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A&nodeId=018rH325E4017B#
This patch ensures that the register read/write APIs are remodelled
to address such cases, while ensuring that existing platforms (where
the FlexCAN IP was modelled in LE way) do not break.
Tested on LS1021A-QDS board.
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
---
Rebased againt v3.16-rc1
drivers/net/can/flexcan.c | 213 +++++++++++++++++++++++++++------------------
1 file changed, 126 insertions(+), 87 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f425ec2..00c4740 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -198,6 +198,7 @@ struct flexcan_regs {
struct flexcan_devtype_data {
u32 features; /* hardware controller features */
+ bool module_is_big_endian; /* IP is big-endian */
};
struct flexcan_priv {
@@ -216,12 +217,30 @@ struct flexcan_priv {
struct regulator *reg_xceiver;
};
-static struct flexcan_devtype_data fsl_p1010_devtype_data = {
- .features = FLEXCAN_HAS_BROKEN_ERR_STATE,
+static struct flexcan_devtype_data fsl_imx25_devtype_data = {
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_imx28_devtype_data = {
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_imx53_devtype_data = {
+ .module_is_big_endian = false,
};
-static struct flexcan_devtype_data fsl_imx28_devtype_data;
+
static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
.features = FLEXCAN_HAS_V10_FEATURES,
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_ls1021a_devtype_data = {
+ .module_is_big_endian = true,
+};
+
+static struct flexcan_devtype_data fsl_p1010_devtype_data = {
+ .features = FLEXCAN_HAS_BROKEN_ERR_STATE,
+ .module_is_big_endian = false,
};
static const struct can_bittiming_const flexcan_bittiming_const = {
@@ -237,32 +256,36 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
};
/*
- * Abstract off the read/write for arm versus ppc. This
- * assumes that PPC uses big-endian registers and everything
- * else uses little-endian registers, independent of CPU
- * endianess.
+ * FlexCAN module is essentially modelled as a little-endian IP in most
+ * SoCs, i.e the registers as well as the message buffer areas are
+ * implemented in a little-endian fashion.
+ *
+ * However there are some SoCs (e.g. LS1021A) which implement the FlexCAN
+ * module in a big-endian fashion (i.e the registers as well as the
+ * message buffer areas are implemented in a big-endian way).
+ *
+ * In addition, the FlexCAN module can be found on SoCs having ARM or
+ * PPC cores. So, we need to abstract off the register read/write
+ * functions, ensuring that these cater to all the combinations of module
+ * endianess and underlying CPU endianess.
*/
-#if defined(CONFIG_PPC)
-static inline u32 flexcan_read(void __iomem *addr)
+static inline u32 flexcan_read(const struct flexcan_priv *priv,
+ 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);
+ if (priv->devtype_data->module_is_big_endian)
+ return ioread32be(addr);
+ else
+ return ioread32(addr);
}
-static inline void flexcan_write(u32 val, void __iomem *addr)
+static inline void flexcan_write(const struct flexcan_priv *priv,
+ u32 val, void __iomem *addr)
{
- writel(val, addr);
+ if (priv->devtype_data->module_is_big_endian)
+ iowrite32be(val, addr);
+ else
+ iowrite32(val, addr);
}
-#endif
static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
{
@@ -293,14 +316,15 @@ static int flexcan_chip_enable(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg &= ~FLEXCAN_MCR_MDIS;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_LPM_ACK))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_LPM_ACK)
return -ETIMEDOUT;
return 0;
@@ -312,14 +336,15 @@ static int flexcan_chip_disable(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_MDIS;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ while (timeout-- && !(flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_LPM_ACK))
usleep_range(10, 20);
- if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ if (!(flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_LPM_ACK))
return -ETIMEDOUT;
return 0;
@@ -331,14 +356,15 @@ static int flexcan_chip_freeze(struct flexcan_priv *priv)
unsigned int timeout = 1000 * 1000 * 10 / priv->can.bittiming.bitrate;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_HALT;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ while (timeout-- && !(flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_FRZ_ACK))
usleep_range(100, 200);
- if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ if (!(flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
return -ETIMEDOUT;
return 0;
@@ -350,14 +376,15 @@ static int flexcan_chip_unfreeze(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg &= ~FLEXCAN_MCR_HALT;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_FRZ_ACK))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
return -ETIMEDOUT;
return 0;
@@ -368,11 +395,12 @@ static int flexcan_chip_softreset(struct flexcan_priv *priv)
struct flexcan_regs __iomem *regs = priv->base;
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
- flexcan_write(FLEXCAN_MCR_SOFTRST, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST))
+ flexcan_write(priv, FLEXCAN_MCR_SOFTRST, ®s->mcr);
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_SOFTRST))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_SOFTRST)
return -ETIMEDOUT;
return 0;
@@ -383,7 +411,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 = flexcan_read(®s->ecr);
+ u32 reg = flexcan_read(priv, ®s->ecr);
bec->txerr = (reg >> 0) & 0xff;
bec->rxerr = (reg >> 8) & 0xff;
@@ -416,17 +444,19 @@ 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]);
- flexcan_write(data, ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
+ flexcan_write(priv, data,
+ ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
}
if (cf->can_dlc > 3) {
u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
- flexcan_write(data, ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
+ flexcan_write(priv, data,
+ ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
}
can_put_echo_skb(skb, dev, 0);
- flexcan_write(can_id, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
- flexcan_write(ctrl, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ flexcan_write(priv, can_id, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
+ flexcan_write(priv, ctrl, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
return NETDEV_TX_OK;
}
@@ -609,8 +639,8 @@ static void flexcan_read_fifo(const struct net_device *dev,
struct flexcan_mb __iomem *mb = ®s->cantxfg[0];
u32 reg_ctrl, reg_id;
- reg_ctrl = flexcan_read(&mb->can_ctrl);
- reg_id = flexcan_read(&mb->can_id);
+ reg_ctrl = flexcan_read(priv, &mb->can_ctrl);
+ reg_id = flexcan_read(priv, &mb->can_id);
if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
else
@@ -620,12 +650,14 @@ 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(flexcan_read(&mb->data[0]));
- *(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
+ *(__be32 *)(cf->data + 0) =
+ cpu_to_be32(flexcan_read(priv, &mb->data[0]));
+ *(__be32 *)(cf->data + 4) =
+ cpu_to_be32(flexcan_read(priv, &mb->data[1]));
/* mark as read */
- flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
- flexcan_read(®s->timer);
+ flexcan_write(priv, FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
+ flexcan_read(priv, ®s->timer);
}
static int flexcan_read_frame(struct net_device *dev)
@@ -663,17 +695,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 = flexcan_read(®s->esr) | priv->reg_esr;
+ reg_esr = flexcan_read(priv, ®s->esr) | priv->reg_esr;
/* handle state changes */
work_done += flexcan_poll_state(dev, reg_esr);
/* handle RX-FIFO */
- reg_iflag1 = flexcan_read(®s->iflag1);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
work_done < quota) {
work_done += flexcan_read_frame(dev);
- reg_iflag1 = flexcan_read(®s->iflag1);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
}
/* report bus errors */
@@ -683,8 +715,8 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
if (work_done < quota) {
napi_complete(napi);
/* enable IRQs */
- flexcan_write(FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default, ®s->ctrl);
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
+ flexcan_write(priv, priv->reg_ctrl_default, ®s->ctrl);
}
return work_done;
@@ -698,11 +730,11 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
struct flexcan_regs __iomem *regs = priv->base;
u32 reg_iflag1, reg_esr;
- reg_iflag1 = flexcan_read(®s->iflag1);
- reg_esr = flexcan_read(®s->esr);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
+ reg_esr = flexcan_read(priv, ®s->esr);
/* ACK all bus error and state change IRQ sources */
if (reg_esr & FLEXCAN_ESR_ALL_INT)
- flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
+ flexcan_write(priv, reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
/*
* schedule NAPI in case of:
@@ -718,16 +750,17 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
* save them for later use.
*/
priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
- flexcan_write(FLEXCAN_IFLAG_DEFAULT &
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT &
~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
- ®s->ctrl);
+ flexcan_write(priv, priv->reg_ctrl_default &
+ ~FLEXCAN_CTRL_ERR_ALL, ®s->ctrl);
napi_schedule(&priv->napi);
}
/* FIFO overflow */
if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
- flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, ®s->iflag1);
+ flexcan_write(priv, FLEXCAN_IFLAG_RX_FIFO_OVERFLOW,
+ ®s->iflag1);
dev->stats.rx_over_errors++;
dev->stats.rx_errors++;
}
@@ -737,7 +770,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
stats->tx_bytes += can_get_echo_skb(dev, 0);
stats->tx_packets++;
can_led_event(dev, CAN_LED_EVENT_TX);
- flexcan_write((1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
+ flexcan_write(priv, (1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
netif_wake_queue(dev);
}
@@ -751,7 +784,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
u32 reg;
- reg = flexcan_read(®s->ctrl);
+ reg = flexcan_read(priv, ®s->ctrl);
reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
FLEXCAN_CTRL_RJW(0x3) |
FLEXCAN_CTRL_PSEG1(0x7) |
@@ -775,11 +808,12 @@ static void flexcan_set_bittiming(struct net_device *dev)
reg |= FLEXCAN_CTRL_SMP;
netdev_info(dev, "writing ctrl=0x%08x\n", reg);
- flexcan_write(reg, ®s->ctrl);
+ flexcan_write(priv, reg, ®s->ctrl);
/* print chip status */
netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
- flexcan_read(®s->mcr), flexcan_read(®s->ctrl));
+ flexcan_read(priv, ®s->mcr),
+ flexcan_read(priv, ®s->ctrl));
}
/*
@@ -819,14 +853,14 @@ static int flexcan_chip_start(struct net_device *dev)
* disable local echo
*
*/
- reg_mcr = flexcan_read(®s->mcr);
+ reg_mcr = flexcan_read(priv, ®s->mcr);
reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
- flexcan_write(reg_mcr, ®s->mcr);
+ flexcan_write(priv, reg_mcr, ®s->mcr);
/*
* CTRL
@@ -840,7 +874,7 @@ static int flexcan_chip_start(struct net_device *dev)
* enable bus off interrupt
* (== FLEXCAN_CTRL_ERR_STATE)
*/
- reg_ctrl = flexcan_read(®s->ctrl);
+ reg_ctrl = flexcan_read(priv, ®s->ctrl);
reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
FLEXCAN_CTRL_ERR_STATE;
@@ -856,19 +890,19 @@ static int flexcan_chip_start(struct net_device *dev)
/* save for later use */
priv->reg_ctrl_default = reg_ctrl;
netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
- flexcan_write(reg_ctrl, ®s->ctrl);
+ flexcan_write(priv, reg_ctrl, ®s->ctrl);
/* Abort any pending TX, mark Mailbox as INACTIVE */
- flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
+ flexcan_write(priv, FLEXCAN_MB_CNT_CODE(0x4),
®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
/* acceptance mask/acceptance code (accept everything) */
- flexcan_write(0x0, ®s->rxgmask);
- flexcan_write(0x0, ®s->rx14mask);
- flexcan_write(0x0, ®s->rx15mask);
+ flexcan_write(priv, 0x0, ®s->rxgmask);
+ flexcan_write(priv, 0x0, ®s->rx14mask);
+ flexcan_write(priv, 0x0, ®s->rx15mask);
if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
- flexcan_write(0x0, ®s->rxfgmask);
+ flexcan_write(priv, 0x0, ®s->rxfgmask);
err = flexcan_transceiver_enable(priv);
if (err)
@@ -882,11 +916,12 @@ static int flexcan_chip_start(struct net_device *dev)
priv->can.state = CAN_STATE_ERROR_ACTIVE;
/* enable FIFO interrupts */
- flexcan_write(FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
/* print chip status */
netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
- flexcan_read(®s->mcr), flexcan_read(®s->ctrl));
+ flexcan_read(priv, ®s->mcr),
+ flexcan_read(priv, ®s->ctrl));
return 0;
@@ -913,8 +948,8 @@ static void flexcan_chip_stop(struct net_device *dev)
flexcan_chip_disable(priv);
/* Disable all interrupts */
- flexcan_write(0, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
+ flexcan_write(priv, 0, ®s->imask1);
+ flexcan_write(priv, priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
®s->ctrl);
flexcan_transceiver_disable(priv);
@@ -1032,26 +1067,26 @@ static int register_flexcandev(struct net_device *dev)
err = flexcan_chip_disable(priv);
if (err)
goto out_disable_per;
- reg = flexcan_read(®s->ctrl);
+ reg = flexcan_read(priv, ®s->ctrl);
reg |= FLEXCAN_CTRL_CLK_SRC;
- flexcan_write(reg, ®s->ctrl);
+ flexcan_write(priv, reg, ®s->ctrl);
err = flexcan_chip_enable(priv);
if (err)
goto out_chip_disable;
/* set freeze, halt and activate FIFO, restrict register access */
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->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 = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
if (!(reg & FLEXCAN_MCR_FEN)) {
netdev_err(dev, "Could not enable RX FIFO, unsupported core\n");
err = -ENODEV;
@@ -1078,7 +1113,11 @@ static void unregister_flexcandev(struct net_device *dev)
static const struct of_device_id flexcan_of_match[] = {
{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
+ { .compatible = "fsl,imx25-flexcan", .data = &fsl_imx25_devtype_data, },
{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
+ { .compatible = "fsl,imx53-flexcan", .data = &fsl_imx53_devtype_data, },
+ { .compatible = "fsl,ls1021a-flexcan",
+ .data = &fsl_ls1021a_devtype_data, },
{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
{ /* sentinel */ },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
@ 2014-06-24 15:54 ` Bhupesh Sharma
0 siblings, 0 replies; 16+ messages in thread
From: Bhupesh Sharma @ 2014-06-24 15:54 UTC (permalink / raw)
To: mkl, linux-can; +Cc: wg, netdev, Bhupesh Sharma
The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is
modelled in a big-endian fashion, i.e. the registers and the
message buffers are organized in a BE way.
More details about the LS1021A SoC can be seen here:
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A&nodeId=018rH325E4017B#
This patch ensures that the register read/write APIs are remodelled
to address such cases, while ensuring that existing platforms (where
the FlexCAN IP was modelled in LE way) do not break.
Tested on LS1021A-QDS board.
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
---
Rebased againt v3.16-rc1
drivers/net/can/flexcan.c | 213 +++++++++++++++++++++++++++------------------
1 file changed, 126 insertions(+), 87 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f425ec2..00c4740 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -198,6 +198,7 @@ struct flexcan_regs {
struct flexcan_devtype_data {
u32 features; /* hardware controller features */
+ bool module_is_big_endian; /* IP is big-endian */
};
struct flexcan_priv {
@@ -216,12 +217,30 @@ struct flexcan_priv {
struct regulator *reg_xceiver;
};
-static struct flexcan_devtype_data fsl_p1010_devtype_data = {
- .features = FLEXCAN_HAS_BROKEN_ERR_STATE,
+static struct flexcan_devtype_data fsl_imx25_devtype_data = {
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_imx28_devtype_data = {
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_imx53_devtype_data = {
+ .module_is_big_endian = false,
};
-static struct flexcan_devtype_data fsl_imx28_devtype_data;
+
static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
.features = FLEXCAN_HAS_V10_FEATURES,
+ .module_is_big_endian = false,
+};
+
+static struct flexcan_devtype_data fsl_ls1021a_devtype_data = {
+ .module_is_big_endian = true,
+};
+
+static struct flexcan_devtype_data fsl_p1010_devtype_data = {
+ .features = FLEXCAN_HAS_BROKEN_ERR_STATE,
+ .module_is_big_endian = false,
};
static const struct can_bittiming_const flexcan_bittiming_const = {
@@ -237,32 +256,36 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
};
/*
- * Abstract off the read/write for arm versus ppc. This
- * assumes that PPC uses big-endian registers and everything
- * else uses little-endian registers, independent of CPU
- * endianess.
+ * FlexCAN module is essentially modelled as a little-endian IP in most
+ * SoCs, i.e the registers as well as the message buffer areas are
+ * implemented in a little-endian fashion.
+ *
+ * However there are some SoCs (e.g. LS1021A) which implement the FlexCAN
+ * module in a big-endian fashion (i.e the registers as well as the
+ * message buffer areas are implemented in a big-endian way).
+ *
+ * In addition, the FlexCAN module can be found on SoCs having ARM or
+ * PPC cores. So, we need to abstract off the register read/write
+ * functions, ensuring that these cater to all the combinations of module
+ * endianess and underlying CPU endianess.
*/
-#if defined(CONFIG_PPC)
-static inline u32 flexcan_read(void __iomem *addr)
+static inline u32 flexcan_read(const struct flexcan_priv *priv,
+ 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);
+ if (priv->devtype_data->module_is_big_endian)
+ return ioread32be(addr);
+ else
+ return ioread32(addr);
}
-static inline void flexcan_write(u32 val, void __iomem *addr)
+static inline void flexcan_write(const struct flexcan_priv *priv,
+ u32 val, void __iomem *addr)
{
- writel(val, addr);
+ if (priv->devtype_data->module_is_big_endian)
+ iowrite32be(val, addr);
+ else
+ iowrite32(val, addr);
}
-#endif
static inline int flexcan_transceiver_enable(const struct flexcan_priv *priv)
{
@@ -293,14 +316,15 @@ static int flexcan_chip_enable(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg &= ~FLEXCAN_MCR_MDIS;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_LPM_ACK))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_LPM_ACK)
return -ETIMEDOUT;
return 0;
@@ -312,14 +336,15 @@ static int flexcan_chip_disable(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_MDIS;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ while (timeout-- && !(flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_LPM_ACK))
usleep_range(10, 20);
- if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
+ if (!(flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_LPM_ACK))
return -ETIMEDOUT;
return 0;
@@ -331,14 +356,15 @@ static int flexcan_chip_freeze(struct flexcan_priv *priv)
unsigned int timeout = 1000 * 1000 * 10 / priv->can.bittiming.bitrate;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_HALT;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ while (timeout-- && !(flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_FRZ_ACK))
usleep_range(100, 200);
- if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ if (!(flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
return -ETIMEDOUT;
return 0;
@@ -350,14 +376,15 @@ static int flexcan_chip_unfreeze(struct flexcan_priv *priv)
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
u32 reg;
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg &= ~FLEXCAN_MCR_HALT;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_FRZ_ACK))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
return -ETIMEDOUT;
return 0;
@@ -368,11 +395,12 @@ static int flexcan_chip_softreset(struct flexcan_priv *priv)
struct flexcan_regs __iomem *regs = priv->base;
unsigned int timeout = FLEXCAN_TIMEOUT_US / 10;
- flexcan_write(FLEXCAN_MCR_SOFTRST, ®s->mcr);
- while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST))
+ flexcan_write(priv, FLEXCAN_MCR_SOFTRST, ®s->mcr);
+ while (timeout-- && (flexcan_read(priv, ®s->mcr) &
+ FLEXCAN_MCR_SOFTRST))
usleep_range(10, 20);
- if (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST)
+ if (flexcan_read(priv, ®s->mcr) & FLEXCAN_MCR_SOFTRST)
return -ETIMEDOUT;
return 0;
@@ -383,7 +411,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 = flexcan_read(®s->ecr);
+ u32 reg = flexcan_read(priv, ®s->ecr);
bec->txerr = (reg >> 0) & 0xff;
bec->rxerr = (reg >> 8) & 0xff;
@@ -416,17 +444,19 @@ 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]);
- flexcan_write(data, ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
+ flexcan_write(priv, data,
+ ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
}
if (cf->can_dlc > 3) {
u32 data = be32_to_cpup((__be32 *)&cf->data[4]);
- flexcan_write(data, ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
+ flexcan_write(priv, data,
+ ®s->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
}
can_put_echo_skb(skb, dev, 0);
- flexcan_write(can_id, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
- flexcan_write(ctrl, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ flexcan_write(priv, can_id, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
+ flexcan_write(priv, ctrl, ®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
return NETDEV_TX_OK;
}
@@ -609,8 +639,8 @@ static void flexcan_read_fifo(const struct net_device *dev,
struct flexcan_mb __iomem *mb = ®s->cantxfg[0];
u32 reg_ctrl, reg_id;
- reg_ctrl = flexcan_read(&mb->can_ctrl);
- reg_id = flexcan_read(&mb->can_id);
+ reg_ctrl = flexcan_read(priv, &mb->can_ctrl);
+ reg_id = flexcan_read(priv, &mb->can_id);
if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
else
@@ -620,12 +650,14 @@ 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(flexcan_read(&mb->data[0]));
- *(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
+ *(__be32 *)(cf->data + 0) =
+ cpu_to_be32(flexcan_read(priv, &mb->data[0]));
+ *(__be32 *)(cf->data + 4) =
+ cpu_to_be32(flexcan_read(priv, &mb->data[1]));
/* mark as read */
- flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
- flexcan_read(®s->timer);
+ flexcan_write(priv, FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->iflag1);
+ flexcan_read(priv, ®s->timer);
}
static int flexcan_read_frame(struct net_device *dev)
@@ -663,17 +695,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 = flexcan_read(®s->esr) | priv->reg_esr;
+ reg_esr = flexcan_read(priv, ®s->esr) | priv->reg_esr;
/* handle state changes */
work_done += flexcan_poll_state(dev, reg_esr);
/* handle RX-FIFO */
- reg_iflag1 = flexcan_read(®s->iflag1);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
work_done < quota) {
work_done += flexcan_read_frame(dev);
- reg_iflag1 = flexcan_read(®s->iflag1);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
}
/* report bus errors */
@@ -683,8 +715,8 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
if (work_done < quota) {
napi_complete(napi);
/* enable IRQs */
- flexcan_write(FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default, ®s->ctrl);
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
+ flexcan_write(priv, priv->reg_ctrl_default, ®s->ctrl);
}
return work_done;
@@ -698,11 +730,11 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
struct flexcan_regs __iomem *regs = priv->base;
u32 reg_iflag1, reg_esr;
- reg_iflag1 = flexcan_read(®s->iflag1);
- reg_esr = flexcan_read(®s->esr);
+ reg_iflag1 = flexcan_read(priv, ®s->iflag1);
+ reg_esr = flexcan_read(priv, ®s->esr);
/* ACK all bus error and state change IRQ sources */
if (reg_esr & FLEXCAN_ESR_ALL_INT)
- flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
+ flexcan_write(priv, reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
/*
* schedule NAPI in case of:
@@ -718,16 +750,17 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
* save them for later use.
*/
priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
- flexcan_write(FLEXCAN_IFLAG_DEFAULT &
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT &
~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
- ®s->ctrl);
+ flexcan_write(priv, priv->reg_ctrl_default &
+ ~FLEXCAN_CTRL_ERR_ALL, ®s->ctrl);
napi_schedule(&priv->napi);
}
/* FIFO overflow */
if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
- flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, ®s->iflag1);
+ flexcan_write(priv, FLEXCAN_IFLAG_RX_FIFO_OVERFLOW,
+ ®s->iflag1);
dev->stats.rx_over_errors++;
dev->stats.rx_errors++;
}
@@ -737,7 +770,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
stats->tx_bytes += can_get_echo_skb(dev, 0);
stats->tx_packets++;
can_led_event(dev, CAN_LED_EVENT_TX);
- flexcan_write((1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
+ flexcan_write(priv, (1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
netif_wake_queue(dev);
}
@@ -751,7 +784,7 @@ static void flexcan_set_bittiming(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
u32 reg;
- reg = flexcan_read(®s->ctrl);
+ reg = flexcan_read(priv, ®s->ctrl);
reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
FLEXCAN_CTRL_RJW(0x3) |
FLEXCAN_CTRL_PSEG1(0x7) |
@@ -775,11 +808,12 @@ static void flexcan_set_bittiming(struct net_device *dev)
reg |= FLEXCAN_CTRL_SMP;
netdev_info(dev, "writing ctrl=0x%08x\n", reg);
- flexcan_write(reg, ®s->ctrl);
+ flexcan_write(priv, reg, ®s->ctrl);
/* print chip status */
netdev_dbg(dev, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
- flexcan_read(®s->mcr), flexcan_read(®s->ctrl));
+ flexcan_read(priv, ®s->mcr),
+ flexcan_read(priv, ®s->ctrl));
}
/*
@@ -819,14 +853,14 @@ static int flexcan_chip_start(struct net_device *dev)
* disable local echo
*
*/
- reg_mcr = flexcan_read(®s->mcr);
+ reg_mcr = flexcan_read(priv, ®s->mcr);
reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
- flexcan_write(reg_mcr, ®s->mcr);
+ flexcan_write(priv, reg_mcr, ®s->mcr);
/*
* CTRL
@@ -840,7 +874,7 @@ static int flexcan_chip_start(struct net_device *dev)
* enable bus off interrupt
* (== FLEXCAN_CTRL_ERR_STATE)
*/
- reg_ctrl = flexcan_read(®s->ctrl);
+ reg_ctrl = flexcan_read(priv, ®s->ctrl);
reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
FLEXCAN_CTRL_ERR_STATE;
@@ -856,19 +890,19 @@ static int flexcan_chip_start(struct net_device *dev)
/* save for later use */
priv->reg_ctrl_default = reg_ctrl;
netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
- flexcan_write(reg_ctrl, ®s->ctrl);
+ flexcan_write(priv, reg_ctrl, ®s->ctrl);
/* Abort any pending TX, mark Mailbox as INACTIVE */
- flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
+ flexcan_write(priv, FLEXCAN_MB_CNT_CODE(0x4),
®s->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
/* acceptance mask/acceptance code (accept everything) */
- flexcan_write(0x0, ®s->rxgmask);
- flexcan_write(0x0, ®s->rx14mask);
- flexcan_write(0x0, ®s->rx15mask);
+ flexcan_write(priv, 0x0, ®s->rxgmask);
+ flexcan_write(priv, 0x0, ®s->rx14mask);
+ flexcan_write(priv, 0x0, ®s->rx15mask);
if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
- flexcan_write(0x0, ®s->rxfgmask);
+ flexcan_write(priv, 0x0, ®s->rxfgmask);
err = flexcan_transceiver_enable(priv);
if (err)
@@ -882,11 +916,12 @@ static int flexcan_chip_start(struct net_device *dev)
priv->can.state = CAN_STATE_ERROR_ACTIVE;
/* enable FIFO interrupts */
- flexcan_write(FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
+ flexcan_write(priv, FLEXCAN_IFLAG_DEFAULT, ®s->imask1);
/* print chip status */
netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
- flexcan_read(®s->mcr), flexcan_read(®s->ctrl));
+ flexcan_read(priv, ®s->mcr),
+ flexcan_read(priv, ®s->ctrl));
return 0;
@@ -913,8 +948,8 @@ static void flexcan_chip_stop(struct net_device *dev)
flexcan_chip_disable(priv);
/* Disable all interrupts */
- flexcan_write(0, ®s->imask1);
- flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
+ flexcan_write(priv, 0, ®s->imask1);
+ flexcan_write(priv, priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
®s->ctrl);
flexcan_transceiver_disable(priv);
@@ -1032,26 +1067,26 @@ static int register_flexcandev(struct net_device *dev)
err = flexcan_chip_disable(priv);
if (err)
goto out_disable_per;
- reg = flexcan_read(®s->ctrl);
+ reg = flexcan_read(priv, ®s->ctrl);
reg |= FLEXCAN_CTRL_CLK_SRC;
- flexcan_write(reg, ®s->ctrl);
+ flexcan_write(priv, reg, ®s->ctrl);
err = flexcan_chip_enable(priv);
if (err)
goto out_chip_disable;
/* set freeze, halt and activate FIFO, restrict register access */
- reg = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
- flexcan_write(reg, ®s->mcr);
+ flexcan_write(priv, reg, ®s->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 = flexcan_read(®s->mcr);
+ reg = flexcan_read(priv, ®s->mcr);
if (!(reg & FLEXCAN_MCR_FEN)) {
netdev_err(dev, "Could not enable RX FIFO, unsupported core\n");
err = -ENODEV;
@@ -1078,7 +1113,11 @@ static void unregister_flexcandev(struct net_device *dev)
static const struct of_device_id flexcan_of_match[] = {
{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
+ { .compatible = "fsl,imx25-flexcan", .data = &fsl_imx25_devtype_data, },
{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
+ { .compatible = "fsl,imx53-flexcan", .data = &fsl_imx53_devtype_data, },
+ { .compatible = "fsl,ls1021a-flexcan",
+ .data = &fsl_ls1021a_devtype_data, },
{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
{ /* sentinel */ },
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-24 15:54 ` Bhupesh Sharma
(?)
@ 2014-06-25 8:27 ` Marc Kleine-Budde
2014-06-25 9:41 ` bhupesh.sharma
-1 siblings, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-25 8:27 UTC (permalink / raw)
To: Bhupesh Sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]
On 06/24/2014 05:54 PM, Bhupesh Sharma wrote:
> The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is
> modelled in a big-endian fashion, i.e. the registers and the
> message buffers are organized in a BE way.
Do you have any idea, why fsl did this? The messed up the network
controller on the mx28, too. :/
> More details about the LS1021A SoC can be seen here:
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A&nodeId=018rH325E4017B#
Is there any "real" documentation for this SoC available?
> This patch ensures that the register read/write APIs are remodelled
> to address such cases, while ensuring that existing platforms (where
> the FlexCAN IP was modelled in LE way) do not break.
I'm not sure if it's better to handle this via the DT attributes
big-endian, little-endian, no attribute would mean native endianess for
backwards compatibility.
With this solution, you're breaking all ARM non DT boards, as the struct
platform_device_id still uses fsl_p1010_devtype_data. You're breaking DT
based mx35, as struct of_device_id has no entry for mx35.
With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs any
more, please change the device trees in the kernel.
Please update the "FLEXCAN hardware feature flags" table in the driver
and check if any of the mentioned quirks are needed for the LS1021A.
See comment inline.....
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
> ---
> Rebased againt v3.16-rc1
>
> drivers/net/can/flexcan.c | 213 +++++++++++++++++++++++++++------------------
> 1 file changed, 126 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..00c4740 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
[...]
> static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -237,32 +256,36 @@ static const struct can_bittiming_const flexcan_bittiming_const = {
> };
>
> /*
> - * Abstract off the read/write for arm versus ppc. This
> - * assumes that PPC uses big-endian registers and everything
> - * else uses little-endian registers, independent of CPU
> - * endianess.
> + * FlexCAN module is essentially modelled as a little-endian IP in most
> + * SoCs, i.e the registers as well as the message buffer areas are
> + * implemented in a little-endian fashion.
> + *
> + * However there are some SoCs (e.g. LS1021A) which implement the FlexCAN
> + * module in a big-endian fashion (i.e the registers as well as the
> + * message buffer areas are implemented in a big-endian way).
> + *
> + * In addition, the FlexCAN module can be found on SoCs having ARM or
> + * PPC cores. So, we need to abstract off the register read/write
> + * functions, ensuring that these cater to all the combinations of module
> + * endianess and underlying CPU endianess.
> */
> -#if defined(CONFIG_PPC)
> -static inline u32 flexcan_read(void __iomem *addr)
> +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> + 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);
> + if (priv->devtype_data->module_is_big_endian)
> + return ioread32be(addr);
> + else
> + return ioread32(addr);
> }
>
> -static inline void flexcan_write(u32 val, void __iomem *addr)
> +static inline void flexcan_write(const struct flexcan_priv *priv,
> + u32 val, void __iomem *addr)
> {
> - writel(val, addr);
> + if (priv->devtype_data->module_is_big_endian)
Please move the devtype_data into the struct flexcan_priv, so that you
don't need a double pointer dereference in the hot path.
> + iowrite32be(val, addr);
> + else
> + iowrite32(val, addr);
> }
> -#endif
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-24 15:54 ` Bhupesh Sharma
(?)
(?)
@ 2014-06-25 8:58 ` David Laight
2014-06-25 9:55 ` bhupesh.sharma
-1 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2014-06-25 8:58 UTC (permalink / raw)
To: 'Bhupesh Sharma', mkl, linux-can; +Cc: wg, netdev
From: Bhupesh Sharma
> The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is
> modelled in a big-endian fashion, i.e. the registers and the
> message buffers are organized in a BE way.
>
> More details about the LS1021A SoC can be seen here:
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A&nodeId=018rH325E4017B#
>
> This patch ensures that the register read/write APIs are remodelled
> to address such cases, while ensuring that existing platforms (where
> the FlexCAN IP was modelled in LE way) do not break.
...
Munged to the new code....
> +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> + void __iomem *addr)
> {
> + if (priv->devtype_data->module_is_big_endian)
> + return ioread32be(addr);
> + else
> + return ioread32(addr);
> }
>
> +static inline void flexcan_write(const struct flexcan_priv *priv,
> + u32 val, void __iomem *addr)
> {
> + if (priv->devtype_data->module_is_big_endian)
> + iowrite32be(val, addr);
> + else
> + iowrite32(val, addr);
> }
Hmmm....
If performance ever matters that is horrid.
Probably to the point where making the functions 'inline' is just code bloat.
David
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 8:27 ` Marc Kleine-Budde
@ 2014-06-25 9:41 ` bhupesh.sharma
2014-06-25 10:26 ` Marc Kleine-Budde
2014-06-25 10:29 ` David Laight
0 siblings, 2 replies; 16+ messages in thread
From: bhupesh.sharma @ 2014-06-25 9:41 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: wg, netdev
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, June 25, 2014 1:58 PM
> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
>
> On 06/24/2014 05:54 PM, Bhupesh Sharma wrote:
> > The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is modelled
> > in a big-endian fashion, i.e. the registers and the message buffers
> > are organized in a BE way.
>
> Do you have any idea, why fsl did this? The messed up the network
> controller on the mx28, too. :/
Not really. I guess s/w drivers are meant to hide h/w obscurities :)
>
> > More details about the LS1021A SoC can be seen here:
> > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A
> > &nodeId=018rH325E4017B#
>
> Is there any "real" documentation for this SoC available?
At the moment only the product brief is available for public distribution,
which I have pointed-to in the URL above.
>
> > This patch ensures that the register read/write APIs are remodelled to
> > address such cases, while ensuring that existing platforms (where the
> > FlexCAN IP was modelled in LE way) do not break.
>
> I'm not sure if it's better to handle this via the DT attributes big-
> endian, little-endian, no attribute would mean native endianess for
> backwards compatibility.
My 1st approach path was do it via DT itself, but that would mean
changing existing DTS/DTSI for SoCs which use FlexCAN, unless we
say no endianess attribute means that the module is still LE, and thus
effectively add 'big-endian' only a node to the LS1021A FlexCAN DT node.
> With this solution, you're breaking all ARM non DT boards, as the struct
> platform_device_id still uses fsl_p1010_devtype_data. You're breaking DT
> based mx35, as struct of_device_id has no entry for mx35.
>
> With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs any
> more, please change the device trees in the kernel.
>
> Please update the "FLEXCAN hardware feature flags" table in the driver
> and check if any of the mentioned quirks are needed for the LS1021A.
I have confirmed the same. No quirks are required for LS1021A.
BTW, can't we have the quirks field in the DT node itself?
>
> See comment inline.....
>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
> > ---
> > Rebased againt v3.16-rc1
> >
> > drivers/net/can/flexcan.c | 213
> > +++++++++++++++++++++++++++------------------
> > 1 file changed, 126 insertions(+), 87 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index f425ec2..00c4740 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
>
> [...]
>
> > static const struct can_bittiming_const flexcan_bittiming_const = {
> > @@ -237,32 +256,36 @@ static const struct can_bittiming_const
> > flexcan_bittiming_const = { };
> >
> > /*
> > - * Abstract off the read/write for arm versus ppc. This
> > - * assumes that PPC uses big-endian registers and everything
> > - * else uses little-endian registers, independent of CPU
> > - * endianess.
> > + * FlexCAN module is essentially modelled as a little-endian IP in
> > + most
> > + * SoCs, i.e the registers as well as the message buffer areas are
> > + * implemented in a little-endian fashion.
> > + *
> > + * However there are some SoCs (e.g. LS1021A) which implement the
> > + FlexCAN
> > + * module in a big-endian fashion (i.e the registers as well as the
> > + * message buffer areas are implemented in a big-endian way).
> > + *
> > + * In addition, the FlexCAN module can be found on SoCs having ARM or
> > + * PPC cores. So, we need to abstract off the register read/write
> > + * functions, ensuring that these cater to all the combinations of
> > + module
> > + * endianess and underlying CPU endianess.
> > */
> > -#if defined(CONFIG_PPC)
> > -static inline u32 flexcan_read(void __iomem *addr)
> > +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> > + 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);
> > + if (priv->devtype_data->module_is_big_endian)
> > + return ioread32be(addr);
> > + else
> > + return ioread32(addr);
> > }
> >
> > -static inline void flexcan_write(u32 val, void __iomem *addr)
> > +static inline void flexcan_write(const struct flexcan_priv *priv,
> > + u32 val, void __iomem *addr)
> > {
> > - writel(val, addr);
> > + if (priv->devtype_data->module_is_big_endian)
>
> Please move the devtype_data into the struct flexcan_priv, so that you
> don't need a double pointer dereference in the hot path.
Ok. Or should I create two functions for read and write - one does it in LE way and the other
in BE way and parse the DT to understand which endianness the module supports.
>
> > + iowrite32be(val, addr);
> > + else
> > + iowrite32(val, addr);
> > }
> > -#endif
>
Please share your views.
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 8:58 ` David Laight
@ 2014-06-25 9:55 ` bhupesh.sharma
0 siblings, 0 replies; 16+ messages in thread
From: bhupesh.sharma @ 2014-06-25 9:55 UTC (permalink / raw)
To: David Laight, mkl, linux-can; +Cc: wg, netdev
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Wednesday, June 25, 2014 2:28 PM
> To: Sharma Bhupesh-B45370; mkl@pengutronix.de; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org
> Subject: RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
>
> From: Bhupesh Sharma
> > The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is modelled
> > in a big-endian fashion, i.e. the registers and the message buffers
> > are organized in a BE way.
> >
> > More details about the LS1021A SoC can be seen here:
> > http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=LS1021A
> > &nodeId=018rH325E4017B#
> >
> > This patch ensures that the register read/write APIs are remodelled to
> > address such cases, while ensuring that existing platforms (where the
> > FlexCAN IP was modelled in LE way) do not break.
> ...
> Munged to the new code....
> > +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> > + void __iomem *addr)
> > {
> > + if (priv->devtype_data->module_is_big_endian)
> > + return ioread32be(addr);
> > + else
> > + return ioread32(addr);
> > }
> >
> > +static inline void flexcan_write(const struct flexcan_priv *priv,
> > + u32 val, void __iomem *addr)
> > {
> > + if (priv->devtype_data->module_is_big_endian)
> > + iowrite32be(val, addr);
> > + else
> > + iowrite32(val, addr);
> > }
>
> Hmmm....
> If performance ever matters that is horrid.
> Probably to the point where making the functions 'inline' is just code
> bloat.
>
Got the point. See my reply already sent to Marc's review comments.
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 9:41 ` bhupesh.sharma
@ 2014-06-25 10:26 ` Marc Kleine-Budde
2014-06-25 11:01 ` bhupesh.sharma
2014-06-25 10:29 ` David Laight
1 sibling, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-25 10:26 UTC (permalink / raw)
To: bhupesh.sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 4728 bytes --]
On 06/25/2014 11:41 AM, bhupesh.sharma@freescale.com wrote:
>>> This patch ensures that the register read/write APIs are remodelled to
>>> address such cases, while ensuring that existing platforms (where the
>>> FlexCAN IP was modelled in LE way) do not break.
>>
>> I'm not sure if it's better to handle this via the DT attributes big-
>> endian, little-endian, no attribute would mean native endianess for
>> backwards compatibility.
>
> My 1st approach path was do it via DT itself, but that would mean
> changing existing DTS/DTSI for SoCs which use FlexCAN, unless we
> say no endianess attribute means that the module is still LE, and thus
> effectively add 'big-endian' only a node to the LS1021A FlexCAN DT node.
If no attributes means native endianess the dts will stay compatible.
>> With this solution, you're breaking all ARM non DT boards, as the struct
>> platform_device_id still uses fsl_p1010_devtype_data. You're breaking DT
>> based mx35, as struct of_device_id has no entry for mx35.
>>
>> With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs any
>> more, please change the device trees in the kernel.
>>
>> Please update the "FLEXCAN hardware feature flags" table in the driver
>> and check if any of the mentioned quirks are needed for the LS1021A.
>
> I have confirmed the same. No quirks are required for LS1021A.
> BTW, can't we have the quirks field in the DT node itself?
I don't know, probably for historic reasons, feel free to post a patch.
>> See comment inline.....
>>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
>>> ---
>>> Rebased againt v3.16-rc1
>>>
>>> drivers/net/can/flexcan.c | 213
>>> +++++++++++++++++++++++++++------------------
>>> 1 file changed, 126 insertions(+), 87 deletions(-)
>>>
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index f425ec2..00c4740 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>
>> [...]
>>
>>> static const struct can_bittiming_const flexcan_bittiming_const = {
>>> @@ -237,32 +256,36 @@ static const struct can_bittiming_const
>>> flexcan_bittiming_const = { };
>>>
>>> /*
>>> - * Abstract off the read/write for arm versus ppc. This
>>> - * assumes that PPC uses big-endian registers and everything
>>> - * else uses little-endian registers, independent of CPU
>>> - * endianess.
>>> + * FlexCAN module is essentially modelled as a little-endian IP in
>>> + most
>>> + * SoCs, i.e the registers as well as the message buffer areas are
>>> + * implemented in a little-endian fashion.
>>> + *
>>> + * However there are some SoCs (e.g. LS1021A) which implement the
>>> + FlexCAN
>>> + * module in a big-endian fashion (i.e the registers as well as the
>>> + * message buffer areas are implemented in a big-endian way).
>>> + *
>>> + * In addition, the FlexCAN module can be found on SoCs having ARM or
>>> + * PPC cores. So, we need to abstract off the register read/write
>>> + * functions, ensuring that these cater to all the combinations of
>>> + module
>>> + * endianess and underlying CPU endianess.
>>> */
>>> -#if defined(CONFIG_PPC)
>>> -static inline u32 flexcan_read(void __iomem *addr)
>>> +static inline u32 flexcan_read(const struct flexcan_priv *priv,
>>> + 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);
>>> + if (priv->devtype_data->module_is_big_endian)
>>> + return ioread32be(addr);
>>> + else
>>> + return ioread32(addr);
>>> }
>>>
>>> -static inline void flexcan_write(u32 val, void __iomem *addr)
>>> +static inline void flexcan_write(const struct flexcan_priv *priv,
>>> + u32 val, void __iomem *addr)
>>> {
>>> - writel(val, addr);
>>> + if (priv->devtype_data->module_is_big_endian)
>>
>> Please move the devtype_data into the struct flexcan_priv, so that you
>> don't need a double pointer dereference in the hot path.
>
> Ok. Or should I create two functions for read and write - one does it in LE way and the other
> in BE way and parse the DT to understand which endianness the module supports.
What about function pointers in the priv? So that flexcan_read() becomes
priv->read().
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 9:41 ` bhupesh.sharma
2014-06-25 10:26 ` Marc Kleine-Budde
@ 2014-06-25 10:29 ` David Laight
2014-06-25 10:34 ` Marc Kleine-Budde
1 sibling, 1 reply; 16+ messages in thread
From: David Laight @ 2014-06-25 10:29 UTC (permalink / raw)
To: 'bhupesh.sharma@freescale.com', Marc Kleine-Budde, linux-can
Cc: wg, netdev
From: bhupesh.sharma@freescale.com
> > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> > On 06/24/2014 05:54 PM, Bhupesh Sharma wrote:
> > > The FlexCAN IP on certain SoCs like (Freescale's LS1021A) is modelled
> > > in a big-endian fashion, i.e. the registers and the message buffers
> > > are organized in a BE way.
> >
> > Do you have any idea, why fsl did this? The messed up the network
> > controller on the mx28, too. :/
>
> Not really. I guess s/w drivers are meant to hide h/w obscurities :)
I sometimes think they just like making life hard for software engineers.
It isn't as though 'gate count' is likely to be critical these days.
...
> Ok. Or should I create two functions for read and write - one does it in LE way and the other
> in BE way and parse the DT to understand which endianness the module supports.
An indirect call is likely to be slower than a conditional.
The conditional inside a non-inlined function is likely to get
predicted correctly on any code paths that matter.
Unfortunately using a real function increases register pressure.
Maybe a compile-time option for BE, LE or both.
So a 'generic' kernel can work, but a SoC specific one will be faster.
Then have the driver load/attach/init fail if it is the wrong endianness.
David
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 10:29 ` David Laight
@ 2014-06-25 10:34 ` Marc Kleine-Budde
0 siblings, 0 replies; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-25 10:34 UTC (permalink / raw)
To: David Laight, 'bhupesh.sharma@freescale.com', linux-can
Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]
On 06/25/2014 12:29 PM, David Laight wrote:
>> Ok. Or should I create two functions for read and write - one does it in LE way and the other
>> in BE way and parse the DT to understand which endianness the module supports.
>
> An indirect call is likely to be slower than a conditional.
> The conditional inside a non-inlined function is likely to get
> predicted correctly on any code paths that matter.
> Unfortunately using a real function increases register pressure.
>
> Maybe a compile-time option for BE, LE or both.
> So a 'generic' kernel can work, but a SoC specific one will be faster.
> Then have the driver load/attach/init fail if it is the wrong endianness.
Don't overengineer, CAN is max 1 MiB/s.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 10:26 ` Marc Kleine-Budde
@ 2014-06-25 11:01 ` bhupesh.sharma
2014-06-25 11:07 ` Marc Kleine-Budde
0 siblings, 1 reply; 16+ messages in thread
From: bhupesh.sharma @ 2014-06-25 11:01 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: wg, netdev
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, June 25, 2014 3:57 PM
> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
>
> On 06/25/2014 11:41 AM, bhupesh.sharma@freescale.com wrote:
> >>> This patch ensures that the register read/write APIs are remodelled
> >>> to address such cases, while ensuring that existing platforms (where
> >>> the FlexCAN IP was modelled in LE way) do not break.
> >>
> >> I'm not sure if it's better to handle this via the DT attributes big-
> >> endian, little-endian, no attribute would mean native endianess for
> >> backwards compatibility.
> >
> > My 1st approach path was do it via DT itself, but that would mean
> > changing existing DTS/DTSI for SoCs which use FlexCAN, unless we say
> > no endianess attribute means that the module is still LE, and thus
> > effectively add 'big-endian' only a node to the LS1021A FlexCAN DT
> node.
>
> If no attributes means native endianess the dts will stay compatible.
>
> >> With this solution, you're breaking all ARM non DT boards, as the
> >> struct platform_device_id still uses fsl_p1010_devtype_data. You're
> >> breaking DT based mx35, as struct of_device_id has no entry for mx35.
> >>
> >> With this patch fsl,p1010-flexcan isn't compatible with the imx/mxs
> >> any more, please change the device trees in the kernel.
> >>
> >> Please update the "FLEXCAN hardware feature flags" table in the
> >> driver and check if any of the mentioned quirks are needed for the
> LS1021A.
> >
> > I have confirmed the same. No quirks are required for LS1021A.
> > BTW, can't we have the quirks field in the DT node itself?
>
> I don't know, probably for historic reasons, feel free to post a patch.
Ok.
>
> >> See comment inline.....
> >>
> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@freescale.com>
> >>> ---
> >>> Rebased againt v3.16-rc1
> >>>
> >>> drivers/net/can/flexcan.c | 213
> >>> +++++++++++++++++++++++++++------------------
> >>> 1 file changed, 126 insertions(+), 87 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >>> index f425ec2..00c4740 100644
> >>> --- a/drivers/net/can/flexcan.c
> >>> +++ b/drivers/net/can/flexcan.c
> >>
> >> [...]
> >>
> >>> static const struct can_bittiming_const flexcan_bittiming_const = {
> >>> @@ -237,32 +256,36 @@ static const struct can_bittiming_const
> >>> flexcan_bittiming_const = { };
> >>>
> >>> /*
> >>> - * Abstract off the read/write for arm versus ppc. This
> >>> - * assumes that PPC uses big-endian registers and everything
> >>> - * else uses little-endian registers, independent of CPU
> >>> - * endianess.
> >>> + * FlexCAN module is essentially modelled as a little-endian IP in
> >>> + most
> >>> + * SoCs, i.e the registers as well as the message buffer areas are
> >>> + * implemented in a little-endian fashion.
> >>> + *
> >>> + * However there are some SoCs (e.g. LS1021A) which implement the
> >>> + FlexCAN
> >>> + * module in a big-endian fashion (i.e the registers as well as the
> >>> + * message buffer areas are implemented in a big-endian way).
> >>> + *
> >>> + * In addition, the FlexCAN module can be found on SoCs having ARM
> >>> + or
> >>> + * PPC cores. So, we need to abstract off the register read/write
> >>> + * functions, ensuring that these cater to all the combinations of
> >>> + module
> >>> + * endianess and underlying CPU endianess.
> >>> */
> >>> -#if defined(CONFIG_PPC)
> >>> -static inline u32 flexcan_read(void __iomem *addr)
> >>> +static inline u32 flexcan_read(const struct flexcan_priv *priv,
> >>> + 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);
> >>> + if (priv->devtype_data->module_is_big_endian)
> >>> + return ioread32be(addr);
> >>> + else
> >>> + return ioread32(addr);
> >>> }
> >>>
> >>> -static inline void flexcan_write(u32 val, void __iomem *addr)
> >>> +static inline void flexcan_write(const struct flexcan_priv *priv,
> >>> + u32 val, void __iomem *addr)
> >>> {
> >>> - writel(val, addr);
> >>> + if (priv->devtype_data->module_is_big_endian)
> >>
> >> Please move the devtype_data into the struct flexcan_priv, so that
> >> you don't need a double pointer dereference in the hot path.
> >
> > Ok. Or should I create two functions for read and write - one does it
> > in LE way and the other in BE way and parse the DT to understand which
> endianness the module supports.
>
> What about function pointers in the priv? So that flexcan_read() becomes
> priv->read().
>
That's what I propose (similar to what I did for C_CAN driver for 16-bit and 32-bit
reg interfaces using platform data):
bool module_is_be = false;
module_is_be = get-endianess-from-DT-node;
priv->read() = module_is_be ? flexcan_read_be : flexcan_read_le;
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 11:01 ` bhupesh.sharma
@ 2014-06-25 11:07 ` Marc Kleine-Budde
2014-06-25 14:16 ` bhupesh.sharma
0 siblings, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-25 11:07 UTC (permalink / raw)
To: bhupesh.sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 844 bytes --]
On 06/25/2014 01:01 PM, bhupesh.sharma@freescale.com wrote:
>> What about function pointers in the priv? So that flexcan_read() becomes
>> priv->read().
> That's what I propose (similar to what I did for C_CAN driver for 16-bit and 32-bit
> reg interfaces using platform data):
Yes...but...
> bool module_is_be = false;
...module_is_be is not that simple, on PPC true must be the default to
be compatible with existing dts.
> module_is_be = get-endianess-from-DT-node;
>
> priv->read() = module_is_be ? flexcan_read_be : flexcan_read_le;
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 11:07 ` Marc Kleine-Budde
@ 2014-06-25 14:16 ` bhupesh.sharma
2014-06-25 19:00 ` Marc Kleine-Budde
0 siblings, 1 reply; 16+ messages in thread
From: bhupesh.sharma @ 2014-06-25 14:16 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: wg, netdev
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, June 25, 2014 4:38 PM
> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
>
> On 06/25/2014 01:01 PM, bhupesh.sharma@freescale.com wrote:
> >> What about function pointers in the priv? So that flexcan_read()
> >> becomes
> >> priv->read().
>
> > That's what I propose (similar to what I did for C_CAN driver for
> > 16-bit and 32-bit reg interfaces using platform data):
>
> Yes...but...
>
> > bool module_is_be = false;
>
> ...module_is_be is not that simple, on PPC true must be the default to be
> compatible with existing dts.
How does this look (a bit messed up, so I am open for comments):
#if defined(CONFIG_ARM)
static inline u32 flexcan_read_ip_is_be(void __iomem *addr)
{
return ioread32be(addr);
}
static inline u32 flexcan_read_ip_is_le(void __iomem *addr)
{
return ioread32(addr);
}
#endif
#if defined(CONFIG_PPC)
static inline u32 flexcan_read_ip_is_be(void __iomem *addr)
{
return ioread32(addr);
}
static inline u32 flexcan_read_ip_is_le(void __iomem *addr)
{
return ioread32be(addr);
}
#endif
[.. snip]
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 14:16 ` bhupesh.sharma
@ 2014-06-25 19:00 ` Marc Kleine-Budde
2014-06-26 9:28 ` Marc Kleine-Budde
0 siblings, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-25 19:00 UTC (permalink / raw)
To: bhupesh.sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]
On 06/25/2014 04:16 PM, bhupesh.sharma@freescale.com wrote:
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Wednesday, June 25, 2014 4:38 PM
>> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
>> Cc: wg@grandegger.com; netdev@vger.kernel.org
>> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
>> for BE instances
>>
>> On 06/25/2014 01:01 PM, bhupesh.sharma@freescale.com wrote:
>>>> What about function pointers in the priv? So that flexcan_read()
>>>> becomes
>>>> priv->read().
>>
>>> That's what I propose (similar to what I did for C_CAN driver for
>>> 16-bit and 32-bit reg interfaces using platform data):
>>
>> Yes...but...
>>
>>> bool module_is_be = false;
>>
>> ...module_is_be is not that simple, on PPC true must be the default to be
>> compatible with existing dts.
>
> How does this look (a bit messed up, so I am open for comments):
What about:
static u32 flexcan_read_le(void __iomem *addr)
{
return ioread32(addr);
}
static u32 flexcan_read_be(void __iomem *addr)
{
return ioread32be(addr);
}
flexcan_probe ()
{
bool core_is_little = true;
/* or #ifdef __BIG_ENDIAN ... #endif instead of IS_ENABLED() */
if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ||
of_property_read_bool(dev->dev.of_node, "big-endian"))
core_is_little = false;
if (core_is_little)
priv->read = flexcan_read_le;
else
priv->read = flexcan_read_be;
}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-25 19:00 ` Marc Kleine-Budde
@ 2014-06-26 9:28 ` Marc Kleine-Budde
2014-06-26 9:30 ` bhupesh.sharma
0 siblings, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-26 9:28 UTC (permalink / raw)
To: bhupesh.sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]
On 06/25/2014 09:00 PM, Marc Kleine-Budde wrote:
>> How does this look (a bit messed up, so I am open for comments):
>
> What about:
>
> static u32 flexcan_read_le(void __iomem *addr)
> {
> return ioread32(addr);
> }
>
> static u32 flexcan_read_be(void __iomem *addr)
> {
> return ioread32be(addr);
> }
>
> flexcan_probe ()
> {
> bool core_is_little = true;
>
> /* or #ifdef __BIG_ENDIAN ... #endif instead of IS_ENABLED() */
> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ||
> of_property_read_bool(dev->dev.of_node, "big-endian"))
> core_is_little = false;
if (of_property_read_bool(dev->dev.of_node, "little-endian"))
core_is_little = true;
> if (core_is_little)
> priv->read = flexcan_read_le;
> else
> priv->read = flexcan_read_be;
>
> }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-26 9:28 ` Marc Kleine-Budde
@ 2014-06-26 9:30 ` bhupesh.sharma
2014-06-26 9:35 ` Marc Kleine-Budde
0 siblings, 1 reply; 16+ messages in thread
From: bhupesh.sharma @ 2014-06-26 9:30 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: wg, netdev
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Thursday, June 26, 2014 2:59 PM
> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
> for BE instances
>
> On 06/25/2014 09:00 PM, Marc Kleine-Budde wrote:
> >> How does this look (a bit messed up, so I am open for comments):
> >
> > What about:
> >
> > static u32 flexcan_read_le(void __iomem *addr) {
> > return ioread32(addr);
> > }
> >
> > static u32 flexcan_read_be(void __iomem *addr) {
> > return ioread32be(addr);
> > }
> >
> > flexcan_probe ()
> > {
> > bool core_is_little = true;
> >
> > /* or #ifdef __BIG_ENDIAN ... #endif instead of IS_ENABLED() */
> > if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ||
> > of_property_read_bool(dev->dev.of_node, "big-endian"))
> > core_is_little = false;
>
> if (of_property_read_bool(dev->dev.of_node, "little-endian"))
> core_is_little = true;
Seems better :)
Let me try to spin a patch and test it at my end.
>
> > if (core_is_little)
> > priv->read = flexcan_read_le;
> > else
> > priv->read = flexcan_read_be;
> >
> > }
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances
2014-06-26 9:30 ` bhupesh.sharma
@ 2014-06-26 9:35 ` Marc Kleine-Budde
0 siblings, 0 replies; 16+ messages in thread
From: Marc Kleine-Budde @ 2014-06-26 9:35 UTC (permalink / raw)
To: bhupesh.sharma, linux-can; +Cc: wg, netdev
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
On 06/26/2014 11:30 AM, bhupesh.sharma@freescale.com wrote:
>
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Thursday, June 26, 2014 2:59 PM
>> To: Sharma Bhupesh-B45370; linux-can@vger.kernel.org
>> Cc: wg@grandegger.com; netdev@vger.kernel.org
>> Subject: Re: [PATCH] net: can: Remodel FlexCAN register read/write APIs
>> for BE instances
>>
>> On 06/25/2014 09:00 PM, Marc Kleine-Budde wrote:
>>>> How does this look (a bit messed up, so I am open for comments):
>>>
>>> What about:
>>>
>>> static u32 flexcan_read_le(void __iomem *addr) {
>>> return ioread32(addr);
>>> }
>>>
>>> static u32 flexcan_read_be(void __iomem *addr) {
>>> return ioread32be(addr);
>>> }
>>>
>>> flexcan_probe ()
>>> {
>>> bool core_is_little = true;
>>>
>>> /* or #ifdef __BIG_ENDIAN ... #endif instead of IS_ENABLED() */
>>> if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ||
>>> of_property_read_bool(dev->dev.of_node, "big-endian"))
>>> core_is_little = false;
>>
>> if (of_property_read_bool(dev->dev.of_node, "little-endian"))
>> core_is_little = true;
>
> Seems better :)
>
> Let me try to spin a patch and test it at my end.
Totally untested this is :)
BTW: Please make an update to the DT bindings as a separate patch. This
patch should be the first one. Once the dust is settled, add the device
tree discussion mailing list on CC.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-06-26 9:35 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-24 15:54 [PATCH] net: can: Remodel FlexCAN register read/write APIs for BE instances Bhupesh Sharma
2014-06-24 15:54 ` Bhupesh Sharma
2014-06-25 8:27 ` Marc Kleine-Budde
2014-06-25 9:41 ` bhupesh.sharma
2014-06-25 10:26 ` Marc Kleine-Budde
2014-06-25 11:01 ` bhupesh.sharma
2014-06-25 11:07 ` Marc Kleine-Budde
2014-06-25 14:16 ` bhupesh.sharma
2014-06-25 19:00 ` Marc Kleine-Budde
2014-06-26 9:28 ` Marc Kleine-Budde
2014-06-26 9:30 ` bhupesh.sharma
2014-06-26 9:35 ` Marc Kleine-Budde
2014-06-25 10:29 ` David Laight
2014-06-25 10:34 ` Marc Kleine-Budde
2014-06-25 8:58 ` David Laight
2014-06-25 9:55 ` bhupesh.sharma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.