Skip to contentSkip to conversation

M_CAN Framework rework

RFC PATCH In review Dan Murphy 3 participants 20 messages Oct 10, 2018 – Jan 11, 2019

Subsystems (5)
lore
Share & tools
lore

Dan Murphy

author
#0 lore raw

cover M_CAN Framework rework

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
Signature · 1 line
2.19.0
author
#1 lore raw

RFC PATCH 1/3 can: m_can: Create m_can core to leverage common code

Create a common code base that can be leveraged by other devices that use the Bosch MCAN IP.

The common code manages the MCAN IP as well as registering the CAN device.

---
 drivers/net/can/m_can/Kconfig                 |   12 +
 drivers/net/can/m_can/Makefile                |    3 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 5 files changed, 360 insertions(+), 1917 deletions(-)
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
drivers/net/can/m_can/Kconfig +12 -0
index 04f20dd39007..b1a9358b7660 100644
@@ -1,5 +1,17 @@
 config CAN_M_CAN
+	tristate "Bosch M_CAN support"
+	---help---
+	  Say Y here if you want to support for Bosch M_CAN controller.
+
+config CAN_M_CAN_CORE
+	depends on CAN_M_CAN
+	tristate "Bosch M_CAN Core support"
+	---help---
+	  Say Y here if you want to support for Bosch M_CAN controller.
+
+config CAN_M_CAN_PLATFORM
 	depends on HAS_IOMEM
+	depends on CAN_M_CAN_CORE
 	tristate "Bosch M_CAN devices"
 	---help---
 	  Say Y here if you want to support for Bosch M_CAN controller.
drivers/net/can/m_can/Makefile +2 -1
index 8bbd7f24f5be..e013d6f4c941 100644
@@ -2,4 +2,5 @@
 #  Makefile for the Bosch M_CAN controller driver.
 #
 
-obj-$(CONFIG_CAN_M_CAN) += m_can.o
+obj-$(CONFIG_CAN_M_CAN_CORE) += m_can_core.o
+obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can.o
drivers/net/can/m_can/m_can.c +20 -1663

1,746 lines not on this page · raw message

drivers/net/can/m_can/m_can_core.c renamed from drivers/net/can/m_can/m_can.c +226 -253

865 lines not on this page · raw message

drivers/net/can/m_can/m_can_core.h new file +100 -0

100 lines not on this page · raw message

Signature · 1 line
2.19.0
#2 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

From 9966873dc261b2703e0545718222874ba1b88638 Mon Sep 17 00:00:00 2001
From: Dan Murphy <[email protected]>
Date: Wed, 10 Oct 2018 09:20:53 -0500
Subject: [PATCH] can: m_can: Create m_can core to leverage common code

Create a common code base that can be leveraged by other devices that use the Bosch MCAN IP.

The common code manages the MCAN IP as well as registering the CAN device.

---
 drivers/net/can/m_can/Kconfig          |  12 +
 drivers/net/can/m_can/Makefile         |   3 +-
 drivers/net/can/m_can/m_can.c          | 385 +++++++++++++++------------------
 drivers/net/can/m_can/m_can_core.h     | 100 +++++++++
 drivers/net/can/m_can/m_can_platform.c | 168 ++++++++++++++
 5 files changed, 461 insertions(+), 207 deletions(-)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/m_can_platform.c
drivers/net/can/m_can/Kconfig +12 -0
index 04f20dd..b1a9358 100644
@@ -1,5 +1,17 @@
 config CAN_M_CAN
+	tristate "Bosch M_CAN support"
+	---help---
+	  Say Y here if you want to support for Bosch M_CAN controller.
+
+config CAN_M_CAN_CORE
+	depends on CAN_M_CAN
+	tristate "Bosch M_CAN Core support"
+	---help---
+	  Say Y here if you want to support for Bosch M_CAN controller.
+
+config CAN_M_CAN_PLATFORM
 	depends on HAS_IOMEM
+	depends on CAN_M_CAN_CORE
 	tristate "Bosch M_CAN devices"
 	---help---
 	  Say Y here if you want to support for Bosch M_CAN controller.
drivers/net/can/m_can/Makefile +2 -1
index 8bbd7f2..e013d6f 100644
@@ -2,4 +2,5 @@
 #  Makefile for the Bosch M_CAN controller driver.
 #
 
-obj-$(CONFIG_CAN_M_CAN) += m_can.o
+obj-$(CONFIG_CAN_M_CAN_CORE) += m_can_core.o
+obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can.o

*** I haven't updated Kconfig and Makefile... it needs some adoptions anyway.

drivers/net/can/m_can/m_can.c +8 -46

72 lines not on this page · raw message

*** Why not just

       return priv->m_can_read(priv, priv->reg_offset, reg);

for both cases. Do we need "reg_offset" here or is it only needed in platform code?

+
+ return ret;
}

-static inline void m_can_write(const struct m_can_priv *priv, +static inline int m_can_write(const struct m_can_priv *priv,
enum m_can_reg reg, u32 val)
{
- writel(val, priv->base + reg);
+ int ret = 0;
+
+ if (priv->m_can_write)
+ ret = priv->m_can_write(priv, priv->reg_offset + reg, val);
+ else
+ writel(val, priv->base + reg);

*** Ditto.

+
+ return ret;
}

static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
u32 fgi, unsigned int offset)
{
- return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
- fgi * RXF0_ELEMENT_SIZE + offset);
+ u32 addr_offset = priv->mcfg[MRAM_RXF0].off + fgi * RXF0_ELEMENT_SIZE + offset;
+ u32 read_fifo_addr; /* need to take into account iomem cases */
+ u32 ret = 0;
+
+ if (priv->mram_start)
+ read_fifo_addr = priv->mram_start + addr_offset;
+ else
+ read_fifo_addr = priv->mram_base + addr_offset;

*** Why do we need two platform-specific variables here? Couldn't it be hidden in platform-specific code?

+ if (priv->m_can_fifo_read)
+ ret = priv->m_can_read(priv, read_fifo_addr);

*** Shouldn't that use "priv->m_can_fifo_read"? Looking to "tcan4x5x.c", the functions "m_can_read" and "m_can_fifo_read" are identical.

+ else
+ ret = readl(read_fifo_addr);
+
+ return ret;
}

static inline void m_can_fifo_write(const struct m_can_priv *priv,
u32 fpi, unsigned int offset, u32 val)
{
- writel(val, priv->mram_base + priv->mcfg[MRAM_TXB].off +
- fpi * TXB_ELEMENT_SIZE + offset);
+ u32 addr_offset = priv->mcfg[MRAM_TXB].off + fpi * TXB_ELEMENT_SIZE + offset;
+ u32 write_fifo_addr;
+ u32 ret;
+
+ if (priv->mram_start)
+ write_fifo_addr = priv->mram_start + addr_offset;
+ else
+ write_fifo_addr = priv->mram_base + addr_offset;
+
+ if (priv->m_can_write)
+ ret = priv->m_can_write(priv, write_fifo_addr, val);
+ else
+ writel(val, write_fifo_addr);
+
+ return ret;
}

static inline u32 m_can_txe_fifo_read(const struct m_can_priv *priv,
u32 fgi,
- u32 offset) {
- return readl(priv->mram_base + priv->mcfg[MRAM_TXE].off +
- fgi * TXE_ELEMENT_SIZE + offset);
+ u32 offset)
+{
+ u32 addr_offset = priv->mcfg[MRAM_TXE].off + fgi * TXE_ELEMENT_SIZE + offset;
+ u32 read_fifo_addr;
+ u32 ret = 0;
+
+printk("%s: Here\n", __func__);
+ if (priv->mram_start)
+ read_fifo_addr = priv->mram_start + addr_offset;
+ else
+ read_fifo_addr = priv->mram_base + addr_offset;
+
+ if (priv->m_can_fifo_read)
+ ret = priv->m_can_read(priv, read_fifo_addr);
+ else
+ ret = readl(read_fifo_addr);
+
+ return ret;
}

static inline bool m_can_tx_fifo_full(const struct m_can_priv *priv) {
+printk("%s: Here\n", __func__);
return !!(m_can_read(priv, M_CAN_TXFQS) & TXFQS_TFQF);
}

-static inline void m_can_config_endisable(const struct m_can_priv *priv,
- bool enable)
+void m_can_config_endisable(const struct m_can_priv *priv, bool enable)
{
u32 cccr = m_can_read(priv, M_CAN_CCCR);
u32 timeout = 10;
@@ -430,7 +442,7 @@ static inline void m_can_config_endisable(const struct m_can_priv *priv,

while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE)) != val) {
if (timeout == 0) {
- netdev_warn(priv->dev, "Failed to init module\n");
+ netdev_warn(priv->net, "Failed to init module\n");
return;
}
timeout--;
@@ -457,7 +469,7 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
struct sk_buff *skb;
u32 id, fgi, dlc;
int i;
-
+printk("%s: Here\n", __func__);
/* calculate the fifo get index for where to read data */
fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_SHIFT;
dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
@@ -512,7 +524,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
struct m_can_priv *priv = netdev_priv(dev);
u32 pkts = 0;
u32 rxfs;
-
+printk("%s: Here\n", __func__);
rxfs = m_can_read(priv, M_CAN_RXF0S);
if (!(rxfs & RXFS_FFL_MASK)) {
netdev_dbg(dev, "no messages in fifo0\n");
@@ -541,7 +553,7 @@ static int m_can_handle_lost_msg(struct net_device *dev)
struct net_device_stats *stats = &dev->stats;
struct sk_buff *skb;
struct can_frame *frame;
-
+printk("%s: Here\n", __func__);
netdev_err(dev, "msg lost in rxf0\n");

stats->rx_errors++;
@@ -566,7 +578,7 @@ static int m_can_handle_lec_err(struct net_device *dev,
struct net_device_stats *stats = &dev->stats;
struct can_frame *cf;
struct sk_buff *skb;
-
+printk("%s: Here\n", __func__);
priv->can.can_stats.bus_error++;
stats->rx_errors++;

@@ -633,9 +645,12 @@ static int m_can_clk_start(struct m_can_priv *priv)
{
int err;

- err = pm_runtime_get_sync(priv->device);
+ if (priv->pm_clock_support == 0)
+ return 0;
+
+ err = pm_runtime_get_sync(priv->dev);
if (err < 0) {
- pm_runtime_put_noidle(priv->device);
+ pm_runtime_put_noidle(priv->dev);
return err;
}

@@ -644,7 +659,8 @@ static int m_can_clk_start(struct m_can_priv *priv)

static void m_can_clk_stop(struct m_can_priv *priv) {
- pm_runtime_put_sync(priv->device);
+ if (priv->pm_clock_support)
+ pm_runtime_put_sync(priv->dev);
}

static int m_can_get_berr_counter(const struct net_device *dev,
@@ -1150,17 +1166,17 @@ static void m_can_chip_config(struct net_device *dev)

/* route all interrupts to INT0 */ m_can_write(priv, M_CAN_ILS, ILS_ALL_INT0);
-
+printk("%s: Bit timing\n", __func__);
/* set bittiming params */ m_can_set_bittiming(dev);
-
+printk("%s: out\n", __func__);
m_can_config_endisable(priv, false);
}

static void m_can_start(struct net_device *dev) {
struct m_can_priv *priv = netdev_priv(dev);
-
+printk("%s: Hear\n", __func__);
/* basic m_can configuration */ m_can_chip_config(dev);

@@ -1171,6 +1187,7 @@ static void m_can_start(struct net_device *dev)

static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
{
+printk("%s: Hear\n", __func__);
switch (mode) {
case CAN_MODE_START:
m_can_start(dev);
@@ -1188,20 +1205,17 @@ static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
* else it returns the release and step coded as:
* return value = 10 * <release> + 1 * <step>
*/
-static int m_can_check_core_release(void __iomem *m_can_base)
+static int m_can_check_core_release(struct m_can_priv *priv)
{
u32 crel_reg;
u8 rel;
u8 step;
int res;
- struct m_can_priv temp_priv = {
- .base = m_can_base
- };

/* Read Core Release Version and split into version number
* Example: Version 3.2.1 => rel = 3; step = 2; substep = 1; */
- crel_reg = m_can_read(&temp_priv, M_CAN_CREL);
+ crel_reg = m_can_read(priv, M_CAN_CREL);
rel = (u8)((crel_reg & CREL_REL_MASK) >> CREL_REL_SHIFT); step = (u8)((crel_reg & CREL_STEP_MASK) >> CREL_STEP_SHIFT);

@@ -1222,47 +1236,45 @@ static int m_can_check_core_release(void __iomem *m_can_base)
static bool m_can_niso_supported(const struct m_can_priv *priv)
{
u32 cccr_reg, cccr_poll;
- int niso_timeout;
+ int niso_timeout = 0;

m_can_config_endisable(priv, true); cccr_reg = m_can_read(priv, M_CAN_CCCR); cccr_reg |= CCCR_NISO; m_can_write(priv, M_CAN_CCCR, cccr_reg);
-
- niso_timeout = readl_poll_timeout((priv->base + M_CAN_CCCR), cccr_poll,
- (cccr_poll == cccr_reg), 0, 10);
+printk("%s: Fix readl poll timeout\n", __func__);
+/* niso_timeout = readl_poll_timeout((priv->base + M_CAN_CCCR), cccr_poll,
+ (cccr_poll == cccr_reg), 0, 10);*/

/* Clear NISO */
cccr_reg &= ~(CCCR_NISO);
m_can_write(priv, M_CAN_CCCR, cccr_reg);

m_can_config_endisable(priv, false);
-
+printk("%s: out\n", __func__);
/* return false if time out (-ETIMEDOUT), else return true */ return !niso_timeout;
}

-static int m_can_dev_setup(struct platform_device *pdev, struct net_device *dev,
- void __iomem *addr)
+static int m_can_dev_setup(struct net_device *dev)
{
struct m_can_priv *priv;
int m_can_version;

- m_can_version = m_can_check_core_release(addr);
+ priv = netdev_priv(dev);
+
+ m_can_version = m_can_check_core_release(priv);
/* return if unsupported version */
if (!m_can_version) {
- dev_err(&pdev->dev, "Unsupported version number: %2d",
+ dev_err(priv->dev, "Unsupported version number: %2d",
m_can_version);
return -EINVAL;
}

- priv = netdev_priv(dev);
netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);

/* Shared properties of all M_CAN versions */ priv->version = m_can_version;
- priv->dev = dev;
- priv->base = addr;
priv->can.do_set_mode = m_can_set_mode; priv->can.do_get_berr_counter = m_can_get_berr_counter;

@@ -1297,7 +1309,7 @@ static int m_can_dev_setup(struct platform_device *pdev, struct net_device *dev,
: 0);
break;
default:
- dev_err(&pdev->dev, "Unsupported version number: %2d",
+ dev_err(priv->dev, "Unsupported version number: %2d",
priv->version);
return -EINVAL;
}
@@ -1515,20 +1527,6 @@ static int register_m_can_dev(struct net_device *dev)
return register_candev(dev);
}

-static void m_can_init_ram(struct m_can_priv *priv)
-{
- int end, i, start;
-
- /* initialize the entire Message RAM in use to avoid possible
- * ECC/parity checksum errors when reading an uninitialized buffer
- */
- start = priv->mcfg[MRAM_SIDF].off;
- end = priv->mcfg[MRAM_TXB].off +
- priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
- for (i = start; i < end; i += 4)
- writel(0x0, priv->mram_base + i);
-}
-
static void m_can_of_parse_mram(struct m_can_priv *priv,
const u32 *mram_config_vals)
{
@@ -1556,7 +1554,7 @@ static void m_can_of_parse_mram(struct m_can_priv *priv,
priv->mcfg[MRAM_TXB].num = mram_config_vals[7] &
(TXBC_NDTB_MASK >> TXBC_NDTB_SHIFT);

- dev_dbg(priv->device,
+ dev_dbg(priv->dev,
"mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n", priv->mram_base,
priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num,
@@ -1566,63 +1564,57 @@ static void m_can_of_parse_mram(struct m_can_priv *priv,
priv->mcfg[MRAM_RXB].off, priv->mcfg[MRAM_RXB].num,
priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
-
- m_can_init_ram(priv);
}

-static int m_can_plat_probe(struct platform_device *pdev) +void m_can_init_ram(struct m_can_priv *priv)
{
- struct net_device *dev;
- struct m_can_priv *priv;
- struct resource *res;
- void __iomem *addr;
- void __iomem *mram_addr;
- struct clk *hclk, *cclk;
- int irq, ret;
- struct device_node *np;
- u32 mram_config_vals[MRAM_CFG_LEN];
- u32 tx_fifo_size;
-
- np = pdev->dev.of_node;
+ int end, i, start;

- hclk = devm_clk_get(&pdev->dev, "hclk");
- cclk = devm_clk_get(&pdev->dev, "cclk");
+ /* initialize the entire Message RAM in use to avoid possible
+ * ECC/parity checksum errors when reading an uninitialized buffer
+ */
+ start = priv->mcfg[MRAM_SIDF].off;
+ end = priv->mcfg[MRAM_TXB].off +
+ priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;

- if (IS_ERR(hclk) || IS_ERR(cclk)) {
- dev_err(&pdev->dev, "no clock found\n");
- ret = -ENODEV;
- goto failed_ret;
+ for (i = start; i < end; i += 4) {
+ if (priv->mram_start)
+ m_can_write(priv, priv->mram_start + i, 0x0);
+ else
+ writel(0x0, priv->mram_base + i);

*** See my comments above.

}
+}

- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
- addr = devm_ioremap_resource(&pdev->dev, res);
- irq = platform_get_irq_byname(pdev, "int0");
+int m_can_core_get_clocks(struct m_can_priv *priv) +{
+ int ret = 0;

- if (IS_ERR(addr) || irq < 0) {
- ret = -EINVAL;
- goto failed_ret;
- }
+ priv->hclk = devm_clk_get(priv->dev, "hclk");
+ priv->cclk = devm_clk_get(priv->dev, "cclk");

- /* message ram could be shared */
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
- if (!res) {
+ if (IS_ERR(priv->cclk)) {
+ dev_err(priv->dev, "no clock found\n");
ret = -ENODEV;
- goto failed_ret;
}

- mram_addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
- if (!mram_addr) {
- ret = -ENOMEM;
- goto failed_ret;
- }
+ return ret;
+}

- /* get message ram configuration */
- ret = of_property_read_u32_array(np, "bosch,mram-cfg",
- mram_config_vals,
- sizeof(mram_config_vals) / 4);
+struct m_can_priv *m_can_core_allocate_dev(struct device *dev) +{
+ struct m_can_priv *class_dev = NULL;
+ u32 mram_config_vals[MRAM_CFG_LEN];
+ struct net_device *net_dev;
+ u32 tx_fifo_size;
+ int ret;
+
+ ret = fwnode_property_read_u32_array(dev_fwnode(dev),

+					     "bosch,mram-cfg",
+					     mram_config_vals,
+					     sizeof(mram_config_vals) / 4);

if (ret) {
- dev_err(&pdev->dev, "Could not get Message RAM configuration.");
- goto failed_ret;
+ dev_err(dev, "Could not get Message RAM configuration.");
+ goto out;
}

/* Get TX FIFO size
@@ -1631,50 +1623,57 @@ static int m_can_plat_probe(struct platform_device *pdev)
tx_fifo_size = mram_config_vals[7];

/* allocate the m_can device */
- dev = alloc_candev(sizeof(*priv), tx_fifo_size);
- if (!dev) {
- ret = -ENOMEM;
- goto failed_ret;
+ net_dev = alloc_candev(sizeof(*class_dev), tx_fifo_size);
+ if (!net_dev) {
+ dev_err(dev, "Failed to allocate CAN device");
+ goto out;
}

- priv = netdev_priv(dev);
- dev->irq = irq;
- priv->device = &pdev->dev;
- priv->hclk = hclk;
- priv->cclk = cclk;
- priv->can.clock.freq = clk_get_rate(cclk);
- priv->mram_base = mram_addr;
-
- platform_set_drvdata(pdev, dev);
- SET_NETDEV_DEV(dev, &pdev->dev);
-
- /* Enable clocks. Necessary to read Core Release in order to determine
- * M_CAN version
- */
- pm_runtime_enable(&pdev->dev);
- ret = m_can_clk_start(priv);
- if (ret)
- goto pm_runtime_fail;
+ class_dev = netdev_priv(net_dev);
+ if (!class_dev) {
+ dev_err(dev, "Failed to init netdev private");
+ goto out;
+ }
+
+ class_dev->net = net_dev;
+ class_dev->dev = dev;
+ SET_NETDEV_DEV(net_dev, dev);
+
+ m_can_of_parse_mram(class_dev, mram_config_vals);
+out:
+ return class_dev;
+}
+
+int m_can_core_register(struct m_can_priv *priv)
+{
+ int ret;

- ret = m_can_dev_setup(pdev, dev, addr);
+ if (priv->pm_clock_support) {
+ pm_runtime_enable(priv->dev);
+ ret = m_can_clk_start(priv);
+ if (ret)
+ goto pm_runtime_fail;
+ }
+
+ ret = priv_setup(priv->net);
if (ret)
goto clk_disable;

- ret = register_m_can_dev(dev);
+ ret = register_priv(priv->net);
if (ret) {
- dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
- KBUILD_MODNAME, ret);
+ dev_err(priv->dev, "registering %s failed (err=%d)\n",
+ priv->net->name, ret);
goto clk_disable;
}

- m_can_of_parse_mram(priv, mram_config_vals);
+ devm_can_led_init(priv->net);

- devm_can_led_init(dev);
+ of_can_transceiver(priv->net);

- of_can_transceiver(dev);
+ dev_info(priv->dev, "%s device registered (irq=%d, version=%d)\n",
+ KBUILD_MODNAME, priv->net->irq, priv->version);

- dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
- KBUILD_MODNAME, dev->irq, priv->version);
+ m_can_set_bittiming(priv->net);

/* Probe finished
* Stop clocks. They will be reactivated once the M_CAN device is opened
@@ -1683,14 +1682,14 @@ static int m_can_plat_probe(struct platform_device *pdev)
m_can_clk_stop(priv);
pm_runtime_fail:
if (ret) {
- pm_runtime_disable(&pdev->dev);
- free_candev(dev);
+ pm_runtime_disable(priv->dev);
+ free_candev(priv->net);
}
-failed_ret:
+
return ret;
}

-static __maybe_unused int m_can_suspend(struct device *dev)
+int m_can_core_suspend(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
struct m_can_priv *priv = netdev_priv(ndev);
@@ -1709,7 +1708,7 @@ static __maybe_unused int m_can_suspend(struct device *dev)
return 0;
}

-static __maybe_unused int m_can_resume(struct device *dev)
+int m_can_core_resume(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
struct m_can_priv *priv = netdev_priv(ndev);
@@ -1747,8 +1746,6 @@ static int m_can_plat_remove(struct platform_device *pdev)

pm_runtime_disable(&pdev->dev);

- platform_set_drvdata(pdev, NULL);
-
free_candev(dev);

return 0;
@@ -1782,30 +1779,6 @@ static int __maybe_unused m_can_runtime_resume(struct device *dev)
return err;
}

-static const struct dev_pm_ops m_can_pmops = {
- SET_RUNTIME_PM_OPS(m_can_runtime_suspend,
- m_can_runtime_resume, NULL)
- SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
-};
-
-static const struct of_device_id m_can_of_table[] = {
- { .compatible = "bosch,m_can", .data = NULL },
- { /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, m_can_of_table);
-
-static struct platform_driver m_can_plat_driver = {
- .driver = {
- .name = KBUILD_MODNAME,
- .of_match_table = m_can_of_table,
- .pm = &m_can_pmops,
- },
- .probe = m_can_plat_probe,
- .remove = m_can_plat_remove,
-};
-
-module_platform_driver(m_can_plat_driver);
-
MODULE_AUTHOR("Dong Aisheng <[email protected]>"); MODULE_LICENSE("GPL v2");
MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");

*** I didn't review the rest of the patch for now.

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I was also thinking about optimized read/write functions handling more than 4 bytes of data, e.g. for the CAN payload data. That would speed-up SPI transfers, I think. But that could also be introduced later-on.

Wolfgang.

author
#3 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Wolfgang

Thanks for the review

Quoted 9 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

Quoted 8 lines

The file names are similar to what we have for the C_CAN driver.

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Quoted 5 lines

I was also thinking about optimized read/write functions handling more than 4 bytes of data, e.g. for the CAN payload data. That would speed-up SPI transfers, I think. But that could also be introduced later-on.

That would be the plan.

Quoted 6 lines

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
#4 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

Wolfgang.

author
#5 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

Sorry for the late reply I have been dealing with personal issues.

I guess it would only make sense if the IRQ was edge trigger and not level.

If the message is being processed and the interrupt is not cleared until later the device may produce another interrupt which may be missed by the processor for external devices. Or when coming out of suspend the edge may be missed.

Otherwise if it was a level only then they do not make sense to support both and having to be able to select between the two would be good.

Dan

Quoted 2 lines

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
author
#6 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

In reply to Wolfgang Grandegger

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

What are your thoughts?

Dan

Quoted 2 lines

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
#7 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

Quoted 1 line

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

Wolfgang.

#8 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Hi Dan, Wolfgang,

Quoted 77 lines · Wolfgang Grandegger

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 5 lines
Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

There are exactly 3 platforms in mainline that use the m_can driver. I can help Dan test it on a dra76x. I haven't had a chance to look at the changes in depth, but just testing for regressions on existing platforms shouldn't be too hard once we have it working on one.

Thanks,
Faiz

Quoted 14 lines
Quoted 2 lines

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

Wolfgang.

author
#9 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Faiz

Quoted 80 lines · Mohammad Faiz Abbas

Hi Dan, Wolfgang,

Quoted 77 lines · Wolfgang Grandegger

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

   mv m_can.c m_can_platform.c
   mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

   s/classdev/priv/
   variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet.  Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting.  Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context.  Doing this creates many deltas in the base code that may break iomapped devices.  I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi.  I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver.  A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

There are exactly 3 platforms in mainline that use the m_can driver. I can help Dan test it on a dra76x. I haven't had a chance to look at the changes in depth, but just testing for regressions on existing platforms shouldn't be too hard once we have it working on one.

Thanks Faiz. Once I have the TCAN fully working I will post the branch to my repo.

Dan

Quoted 17 lines

Thanks,
Faiz

Quoted 14 lines
Quoted 2 lines

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
author
#10 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

In reply to Wolfgang Grandegger

Wolfgang

Quoted 83 lines · Wolfgang Grandegger

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

Quoted 7 lines
Quoted 1 line

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

OK I will keep working on it. What you are describing is what I have done. I have abstracted the register reads and writes away and I am in the process of abstracting away the device specific initialization.

Quoted 3 lines

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

As stated above this is what I have. But the m_can driver was written for io-mapped that has no delays so we need to take into about peripheral wait time in IRQ and atomic context.

This is where the issues are stemming from mainly in the atomic context.

Dan

Quoted 3 lines

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
#11 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Hello Dan,

Am 10.01.19 um 13:53 schrieb Dan Murphy:

Quoted 94 lines

Wolfgang

Quoted 83 lines · Wolfgang Grandegger

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

Quoted 7 lines
Quoted 1 line

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

Quoted 3 lines

OK I will keep working on it. What you are describing is what I have done. I have abstracted the register reads and writes away and I am in the process of abstracting away the device specific initialization.

Would be nice if you could show your current implementation...

Quoted 8 lines
Quoted 3 lines

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

As stated above this is what I have. But the m_can driver was written for io-mapped that has no delays so we need to take into about peripheral wait time in IRQ and atomic context.

This is where the issues are stemming from mainly in the atomic context.

... to understand a bit better what you exactly mean. Or does the last patch you sent already highlight them.

Wolfgang.

author
#12 lore raw

Re: [RFC PATCH 1/3] can: m_can: Create m_can core to leverage common code

Wolfgang

Quoted 97 lines · Wolfgang Grandegger

Hello Dan,

Am 10.01.19 um 13:53 schrieb Dan Murphy:

Quoted 94 lines

Wolfgang

Quoted 90 lines · Wolfgang Grandegger

Hello Dan,

sorry for my late response on that topic...

Am 09.01.19 um 21:58 schrieb Dan Murphy:

Quoted 72 lines

Wolfgang

Quoted 48 lines · Wolfgang Grandegger

Hello Dan,

Am 31.10.2018 um 21:15 schrieb Dan Murphy:

Quoted 16 lines

Wolfgang

Thanks for the review

Quoted 11 lines · Wolfgang Grandegger

Hello Dan,

for the RFC, could you please just do the necessary changes to the existing code. We can discuss about better names, etc. later. For the review if the common code I quickly did:

mv m_can.c m_can_platform.c
mv m_can_core.c m_can.c

The file names are similar to what we have for the C_CAN driver.

Quoted 8 lines
Quoted 6 lines

s/classdev/priv/
variable name s/m_can_dev/priv/

Then your patch 1/3 looks as shown below. I'm going to comment on that one. The comments start with "***"....

So you would like me to align the names with the c_can driver?

That would be the obvious choice.

Quoted 19 lines

<snip>

Quoted 3 lines

*** I didn't review the rest of the patch for now.

snipped the code to reply to the comment.

Quoted 4 lines

Looking to the generic code, you didn't really change the way the driver is accessing the registers. Also the interrupt handling and rx polling is as it was before. Does that work properly using the SPI interface of the TCAN4x5x?

I don't want to change any of that yet. Maybe my cover letter was not clear or did not go through.

But the intention was just to break out the functionality to create a MCAN framework that can be used by devices that contain the Bosch MCAN core and provider their own protocal to access the registers in the device.

Quoted 9 lines
Quoted 5 lines

I don't want to do any functional changes at this time on the IP code itself until we have a framework. There should be no regression in the io mapped code.

I did comment on the interrupt handling and asked if a threaded work queue would affect CAN timing. For the original TCAN driver this was the way it was implemented.

Do threaded interrupts with RX polling make sense? I think we need a common interface allowing to select hard-irqs+napi or threaded-irqs.

I have been working on this code for about a month now and I am *not happy* with the amount of change that needs to be done to make the m_can a framework.

I can tx/rx frames from another CAN device to the TCAN part but I have not even touched the iomapped code.

The challenging part is that the m_can code that is currently available does not have to worry about atomic context because there is no peripheral waiting. Since the TCAN is a peripheral device we need to take into about the hard waits in IRQ context as well as the atomic context. Doing this creates many deltas in the base code that may break iomapped devices. I have had to add the thread_irqs and now I am in the midst of the issue you brought up with napi. I would have to schedule a queue for perp devices and leave the non-threaded iomapped irq.

Quoted 3 lines

At this point I think it may be wise to leave the m_can code alone as it is working and stable and just work on the TCAN driver as a standalone driver. A framework would be nice but I think it would destablize the m_can driver which is embedded in many SoC's and we cannot possibly test everyone of them.

Unfortunately, I do not have m_can hardware at hand.

Quoted 1 line

What are your thoughts?

What we need is a common set of functions doing tx, rx, error and state handling. This will requires substantial changes to the existing io-mapped m_can driver, of course. I still believe it's worth the effort, but I agree that it's difficult for you to re-write and test the existing m_can driver.

Quoted 6 lines
Quoted 3 lines

OK I will keep working on it. What you are describing is what I have done. I have abstracted the register reads and writes away and I am in the process of abstracting away the device specific initialization.

Would be nice if you could show your current implementation...

I did submit v2 with the current implementation but the code has changed a bit since then I clean up the code and post v3 today after I get the Rx working properly.

Quoted 11 lines
Quoted 8 lines
Quoted 3 lines

What about implementing such a set of common functions plus the SPI specific part for your TCAN device. What do you/others think?

As stated above this is what I have. But the m_can driver was written for io-mapped that has no delays so we need to take into about peripheral wait time in IRQ and atomic context.

This is where the issues are stemming from mainly in the atomic context.

... to understand a bit better what you exactly mean. Or does the last patch you sent already highlight them.

v3 will show the deltas

Dan

Quoted 3 lines

Wolfgang.

Signature · 2 lines
------------------
Dan Murphy
author
#13 lore raw

RFC PATCH 2/3 dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver

In reply to Dan Murphy

DT binding documentation for TI TCAN4x5x driver.

---
 .../devicetree/bindings/net/can/tcan4x5x.txt  | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
Documentation/devicetree/bindings/net/can/tcan4x5x.txt new file +34 -0
new file mode 100644
index 000000000000..c742d0f10861
@@ -0,0 +1,34 @@
+Texas Instruments TCAN4x5x CAN Controller
+================================================
+
+This file provides device node information for the TCAN4x5x interface contains.
+
+Required properties:
+	- compatible: "ti,tcan4x5x"
+	- reg: 0
+	- #address-cells : 1
+	- #size-cells : 0
+	- spi-max-frequency: Maximum frequency of the SPI bus the chip can
+			     operate at should be less than or equal to 18 MHz.
+	- data-ready-gpios: Interrupt GPIO for data and error reporting.
+	- wake-up-gpios: Wake up GPIO to wake up the TCAN device
+
+See Documentation/devicetree/bindings/net/can/m_can.txt for additional
+required property details.
+
+Optional properties:
+	- reset-gpios: Hardwired output GPIO. If not defined then software
+		       reset.
+
+Example:
+tcan4x5x: tcan4x5x@0 {
+		compatible = "ti,tcan4x5x";
+		reg = <0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		clocks = <&tclkin_ck>;
+		spi-max-frequency = <10000000>;
+		data-ready-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
+		wake-up-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
+		reset-gpios = <&gpio1 27 GPIO_ACTIVE_LOW>;
+	};
Signature · 1 line
2.19.0
author
#14 lore raw

RFC PATCH 3/3 can: tcan4x5x: Add tcan4x5x driver to the kernel

In reply to Dan Murphy

Add the TCAN4x5x SPI CAN driver. This device
uses the Bosch MCAN IP core along with a SPI
interface map. Leverage the MCAN common core
code to manage the MCAN IP.

This device has a special method to indicate a
write/read operation on the data payload.

---
 drivers/net/can/m_can/Kconfig    |   6 +
 drivers/net/can/m_can/Makefile   |   1 +
 drivers/net/can/m_can/tcan4x5x.c | 321 +++++++++++++++++++++++++++++++
 3 files changed, 328 insertions(+)
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
drivers/net/can/m_can/Kconfig +6 -0
index b1a9358b7660..943e10e15f17 100644
@@ -15,3 +15,9 @@ config CAN_M_CAN_PLATFORM
 	tristate "Bosch M_CAN devices"
 	---help---
 	  Say Y here if you want to support for Bosch M_CAN controller.
+
+config CAN_M_CAN_TCAN4X5X
+	depends on CAN_M_CAN_CORE
+	tristate "TCAN4X5X M_CAN device"
+	---help---
+	  Say Y here if you want to support for TI M_CAN controller.
drivers/net/can/m_can/Makefile +1 -0
index e013d6f4c941..6a9584908d10 100644
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_CAN_M_CAN_CORE) += m_can_core.o
 obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can.o
+obj-$(CONFIG_CAN_M_CAN_TCAN4X5X) += tcan4x5x.o
drivers/net/can/m_can/tcan4x5x.c new file +321 -0

321 lines not on this page · raw message

Signature · 1 line
2.19.0
author
#15 lore raw
In reply to Dan Murphy

Bump

Quoted 46 lines · Dan Murphy

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
Signature · 2 lines
------------------
Dan Murphy
#16 lore raw

Hi Dan,

Quoted 49 lines · Dan Murphy

Bump

Quoted 46 lines · Dan Murphy

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c

Patch 1/3 never arrived for me. Its not there on lkml either.
https://lkml.org/lkml/2018/10/10/611

Can you resend the complete series?

Thanks,
Faiz

author
#17 lore raw
Quoted 53 lines · Faiz Abbas

Hi Dan,

Quoted 49 lines · Dan Murphy

Bump

Quoted 46 lines · Dan Murphy

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
Quoted 3 lines

Patch 1/3 never arrived for me. Its not there on lkml either.
https://lkml.org/lkml/2018/10/10/611

LKML seems to have issue with 1/3. Here they were posted. Maybe the patch is to large but I received all 3 + the cover letter.

https://lore.kernel.org/patchwork/patch/998019/
https://lore.kernel.org/patchwork/patch/998017/
https://lore.kernel.org/patchwork/patch/998020/

Quoted 5 lines

Can you resend the complete series?

Thanks,
Faiz

Signature · 2 lines
------------------
Dan Murphy
#18 lore raw
In reply to Dan Murphy

Hallo Dan,

I will have a closer look later this week. Unfortunately you already renamed file names and structs making the patch almost unreadable. More soon..

Thanks for you patience,

Wolfgang.

Am 17.10.2018 um 22:21 schrieb Dan Murphy:

Quoted 51 lines

Bump

Quoted 46 lines · Dan Murphy

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
author
#19 lore raw

Wolfgang

Quoted 6 lines · Wolfgang Grandegger

Hallo Dan,

I will have a closer look later this week. Unfortunately you already renamed file names and structs making the patch almost unreadable. More soon..

I tried to format the patch to eliminate the massive change but format patch kept giving me the same result.

It is a massive change regardless not sure how else to make is smaller.

I guess this is why I opt'd to just create the dedicated TCAN driver.

Dan

Quoted 56 lines

Thanks for you patience,

Wolfgang.

Am 17.10.2018 um 22:21 schrieb Dan Murphy:

Quoted 51 lines

Bump

Quoted 46 lines · Dan Murphy

All

This patch series creates a m_can core framework that devices can register to. The m_can core manages the Bosch IP and CAN frames. Each device that is registered is responsible for managing device specific functions.

This rewrite was suggested in a device driver submission for the TCAN4x5x device
Reference upstream post:
https://lore.kernel.org/patchwork/patch/984163/

For instance the TCAN device is a SPI device that uses a specific data payload to determine writes and reads. In addition the device has a reset input as well as a wakeup pin. The register offset of the m_can registers differs and must be set by the device attached to the core.

The m_can core will use iomapped writes and reads as the default mechanism for writing and reading. The device driver can provide over rides for this.

This patch series is not complete as it does not handle the CAN interrupts nor can perform a CAN write. If this patch series is deemed acceptable I will finish debugging the driver and post a non RFC series.

Finally I did attempt to reduce the first patch with various git format patch directives but none seemed to reduce the patch.

Dan

Dan Murphy (3):
can: m_can: Create m_can core to leverage common code
dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
can: tcan4x5x: Add tcan4x5x driver to the kernel

 .../devicetree/bindings/net/can/tcan4x5x.txt  |   34 +
 drivers/net/can/m_can/Kconfig                 |   18 +
 drivers/net/can/m_can/Makefile                |    4 +-
 drivers/net/can/m_can/m_can.c                 | 1683 +----------------
 .../net/can/m_can/{m_can.c => m_can_core.c}   |  479 +++--
 drivers/net/can/m_can/m_can_core.h            |  100 +
 drivers/net/can/m_can/tcan4x5x.c              |  321 ++++
 7 files changed, 722 insertions(+), 1917 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
 copy drivers/net/can/m_can/{m_can.c => m_can_core.c} (83%)
 create mode 100644 drivers/net/can/m_can/m_can_core.h
 create mode 100644 drivers/net/can/m_can/tcan4x5x.c
Signature · 2 lines
------------------
Dan Murphy