[PATCH 1/5] can: flexcan: add platform data for ColdFire

[PATCH 1/5] can: flexcan: add platform data for ColdFire

[Angelo Dureghello]

Add platform data object for ColdFire architecture.

Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
---
 include/linux/platform_data/flexcan-mcf.h | 27 +++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 include/linux/platform_data/flexcan-mcf.h

diff --git a/include/linux/platform_data/flexcan-mcf.h b/include/linux/platform_data/flexcan-mcf.h
new file mode 100644
index 000000000000..71fe1a9df084
--- /dev/null
+++ b/include/linux/platform_data/flexcan-mcf.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Flexcan options for ColdFire family
+ *
+ * Copyright (C) 2021  Angelo Dureghello <angelo@kernel-space.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _PLAT_FLEXCAN_MCF_H
+#define _PLAT_FLEXCAN_MCF_H
+
+struct mcf_flexcan_platform_data {
+	int stop_mode;
+	int clk_src;
+	int clock_frequency;
+	bool big_endian;
+};
+
+#endif /* _PLAT_FLEXCAN_MCF_H */
-- 
2.31.1

[PATCH 2/5] m68k: stmark2: update board setup

[Angelo Dureghello]

Add configuration for flexcan pads.

Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
---
 arch/m68k/coldfire/stmark2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/coldfire/stmark2.c b/arch/m68k/coldfire/stmark2.c
index 8b5af9c83244..036a6ae5f599 100644
--- a/arch/m68k/coldfire/stmark2.c
+++ b/arch/m68k/coldfire/stmark2.c
@@ -111,7 +111,9 @@ static int __init init_stmark2(void)
 	__raw_writeb(0x00, MCFGPIO_PAR_BE);
 	__raw_writeb(0x00, MCFGPIO_PAR_FBCTL);
 	__raw_writeb(0x00, MCFGPIO_PAR_CS);
-	__raw_writeb(0x00, MCFGPIO_PAR_CANI2C);
+
+	/* CAN pads */
+	__raw_writeb(0x50, MCFGPIO_PAR_CANI2C);
 
 	platform_add_devices(stmark2_devices, ARRAY_SIZE(stmark2_devices));
 
@@ -121,4 +123,4 @@ static int __init init_stmark2(void)
 	return 0;
 }
 
-late_initcall(init_stmark2);
+device_initcall(init_stmark2);
-- 
2.31.1

[PATCH 3/5] m68k: m5441x: add flexcan support

[Angelo Dureghello]

Add flexcan support.

Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
---
 arch/m68k/coldfire/device.c       | 31 +++++++++++++++++++++++++++++++
 arch/m68k/coldfire/m5441x.c       |  8 ++++----
 arch/m68k/include/asm/m5441xsim.h | 19 +++++++++++++++++++
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/arch/m68k/coldfire/device.c b/arch/m68k/coldfire/device.c
index 59f7dfe50a4d..b3263283bf71 100644
--- a/arch/m68k/coldfire/device.c
+++ b/arch/m68k/coldfire/device.c
@@ -23,6 +23,7 @@
 #include <linux/platform_data/edma.h>
 #include <linux/platform_data/dma-mcf-edma.h>
 #include <linux/platform_data/mmc-esdhc-mcf.h>
+#include <linux/platform_data/flexcan-mcf.h>
 
 /*
  *	All current ColdFire parts contain from 2, 3, 4 or 10 UARTS.
@@ -581,6 +582,33 @@ static struct platform_device mcf_esdhc = {
 };
 #endif /* MCFSDHC_BASE */
 
+#if IS_ENABLED(CONFIG_CAN)
+static struct mcf_flexcan_platform_data mcf_flexcan_info = {
+	.clk_src = 1,
+	.clock_frequency = 120000000,
+};
+
+static struct resource mcf_flexcan0_resource[] = {
+	{
+		.start = MCFFLEXCAN_BASE0,
+		.end = MCFFLEXCAN_BASE0 + MCFFLEXCAN_SIZE,
+		.flags = IORESOURCE_MEM,
+	}, {
+		.start = MCF_IRQ_IFL0,
+		.end = MCF_IRQ_ERR0,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device mcf_flexcan0 = {
+	.name = "flexcan",
+	.id = 0,
+	.num_resources = ARRAY_SIZE(mcf_flexcan0_resource),
+	.resource = mcf_flexcan0_resource,
+	.dev.platform_data = &mcf_flexcan_info,
+};
+#endif /* IS_ENABLED(CONFIG_CAN) */
+
 static struct platform_device *mcf_devices[] __initdata = {
 	&mcf_uart,
 #if IS_ENABLED(CONFIG_FEC)
@@ -616,6 +644,9 @@ static struct platform_device *mcf_devices[] __initdata = {
 #ifdef MCFSDHC_BASE
 	&mcf_esdhc,
 #endif
+#if IS_ENABLED(CONFIG_CAN)
+	&mcf_flexcan0,
+#endif
 };
 
 /*
diff --git a/arch/m68k/coldfire/m5441x.c b/arch/m68k/coldfire/m5441x.c
index 1e5259a652d1..18b152edb69c 100644
--- a/arch/m68k/coldfire/m5441x.c
+++ b/arch/m68k/coldfire/m5441x.c
@@ -18,8 +18,8 @@
 #include <asm/mcfclk.h>
 
 DEFINE_CLK(0, "flexbus", 2, MCF_CLK);
-DEFINE_CLK(0, "mcfcan.0", 8, MCF_CLK);
-DEFINE_CLK(0, "mcfcan.1", 9, MCF_CLK);
+DEFINE_CLK(0, "flexcan.0", 8, MCF_CLK);
+DEFINE_CLK(0, "flexcan.1", 9, MCF_CLK);
 DEFINE_CLK(0, "imx1-i2c.1", 14, MCF_CLK);
 DEFINE_CLK(0, "mcfdspi.1", 15, MCF_CLK);
 DEFINE_CLK(0, "edma", 17, MCF_CLK);
@@ -146,6 +146,8 @@ struct clk *mcf_clks[] = {
 
 static struct clk * const enable_clks[] __initconst = {
 	/* make sure these clocks are enabled */
+	&__clk_0_8, /* flexcan.0 */
+	&__clk_0_9, /* flexcan.1 */
 	&__clk_0_15, /* dspi.1 */
 	&__clk_0_17, /* eDMA */
 	&__clk_0_18, /* intc0 */
@@ -166,8 +168,6 @@ static struct clk * const enable_clks[] __initconst = {
 	&__clk_1_37, /* gpio */
 };
 static struct clk * const disable_clks[] __initconst = {
-	&__clk_0_8, /* can.0 */
-	&__clk_0_9, /* can.1 */
 	&__clk_0_14, /* i2c.1 */
 	&__clk_0_22, /* i2c.0 */
 	&__clk_0_23, /* dspi.0 */
diff --git a/arch/m68k/include/asm/m5441xsim.h b/arch/m68k/include/asm/m5441xsim.h
index e091e36d3464..f48cf63bd782 100644
--- a/arch/m68k/include/asm/m5441xsim.h
+++ b/arch/m68k/include/asm/m5441xsim.h
@@ -73,6 +73,12 @@
 #define MCFINT0_FECENTC1	55
 
 /* on interrupt controller 1 */
+#define MCFINT1_FLEXCAN0_IFL	0
+#define MCFINT1_FLEXCAN0_BOFF	1
+#define MCFINT1_FLEXCAN0_ERR	3
+#define MCFINT1_FLEXCAN1_IFL	4
+#define MCFINT1_FLEXCAN1_BOFF	5
+#define MCFINT1_FLEXCAN1_ERR	7
 #define MCFINT1_UART4		48
 #define MCFINT1_UART5		49
 #define MCFINT1_UART6		50
@@ -314,4 +320,17 @@
 #define MCF_IRQ_SDHC		(MCFINT2_VECBASE + MCFINT2_SDHC)
 #define MCFSDHC_CLK		(MCFSDHC_BASE + 0x2c)
 
+/*
+ * Flexcan module
+ */
+#define MCFFLEXCAN_BASE0	0xfc020000
+#define MCFFLEXCAN_BASE1	0xfc024000
+#define MCFFLEXCAN_SIZE		0x4000
+#define MCF_IRQ_IFL0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_IFL)
+#define MCF_IRQ_BOFF0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_BOFF)
+#define MCF_IRQ_ERR0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_ERR)
+#define MCF_IRQ_IFL1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_IFL)
+#define MCF_IRQ_BOFF1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_BOFF)
+#define MCF_IRQ_ERR1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_ERR)
+
 #endif /* m5441xsim_h */
-- 
2.31.1

[PATCH 4/5] can: flexcan: enable Kconfig for ColdFire

[Angelo Dureghello]

Enable flexcan KConfig for ColdFire m5441x cpu's.

Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
---
 drivers/net/can/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e355d3974977..7a1be9dad52d 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -97,7 +97,7 @@ config CAN_AT91
 
 config CAN_FLEXCAN
 	tristate "Support for Freescale FLEXCAN based chips"
-	depends on OF && HAS_IOMEM
+	depends on (OF || M5441x) && HAS_IOMEM
 	help
 	  Say Y here if you want to support for Freescale FlexCAN.
 
-- 
2.31.1

[PATCH 5/5] can: flexcan: add mcf5441x support

[Angelo Dureghello]

Add flexcan support for NXP ColdFire mcf5441x family.

This flexcan module is quite similar to imx6 flexcan module, but
with some exceptions:

- 3 separate interrupt sources, MB, BOFF and ERR,
- implements 16 mb only,
- m68k architecture is not supporting devicetrees, so a
  platform data check/case has been added,
- ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
  module.

Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
---
 drivers/net/can/flexcan.c | 80 +++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 57f3635ad8d7..af95273327cd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -28,6 +28,7 @@
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/flexcan-mcf.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -205,6 +206,10 @@
 
 #define FLEXCAN_TIMEOUT_US		(250)
 
+#define FLEXCAN_OFFS_INT_BOFF		1
+#define FLEXCAN_OFFS_INT_ERR		3
+#define FLEXCAN_MB_CNT_MCF		16
+
 /* FLEXCAN hardware feature flags
  *
  * Below is some version info we got:
@@ -246,6 +251,8 @@
 #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
 /* Setup stop mode with SCU firmware to support wakeup */
 #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
+/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts  */
+#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
 
 /* Structure of the message buffer */
 struct flexcan_mb {
@@ -371,6 +378,12 @@ struct flexcan_priv {
 	void (*write)(u32 val, void __iomem *addr);
 };
 
+static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
+	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
+		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
+		FLEXCAN_QUIRK_SETUP_MCF,
+};
+
 static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
 		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
@@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct flexcan_priv *priv)
 {
 	int err;
 
-	err = clk_prepare_enable(priv->clk_ipg);
-	if (err)
-		return err;
+	if (priv->clk_ipg) {
+		err = clk_prepare_enable(priv->clk_ipg);
+		if (err)
+			return err;
+	}
 
-	err = clk_prepare_enable(priv->clk_per);
-	if (err)
-		clk_disable_unprepare(priv->clk_ipg);
+	if (priv->clk_per) {
+		err = clk_prepare_enable(priv->clk_per);
+		if (err)
+			clk_disable_unprepare(priv->clk_ipg);
+	}
 
 	return err;
 }
@@ -1401,8 +1418,13 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
 		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
 	else
 		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
-	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
-			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
+
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
+		priv->mb_count = FLEXCAN_MB_CNT_MCF;
+	} else {
+		priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
+				 (sizeof(priv->regs->mb[1]) / priv->mb_size);
+	}
 
 	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
 		priv->tx_mb_reserved =
@@ -1774,6 +1796,18 @@ static int flexcan_open(struct net_device *dev)
 	if (err)
 		goto out_can_rx_offload_disable;
 
+	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
+		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_BOFF,
+				  flexcan_irq, IRQF_SHARED, dev->name, dev);
+		if (err)
+			goto out_can_rx_offload_disable;
+
+		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_ERR,
+				  flexcan_irq, IRQF_SHARED, dev->name, dev);
+		if (err)
+			goto out_can_rx_offload_disable;
+	}
+
 	flexcan_chip_interrupts_enable(dev);
 
 	can_led_event(dev, CAN_LED_EVENT_OPEN);
@@ -2047,7 +2081,9 @@ static int flexcan_probe(struct platform_device *pdev)
 	struct regulator *reg_xceiver;
 	struct clk *clk_ipg = NULL, *clk_per = NULL;
 	struct flexcan_regs __iomem *regs;
+	struct mcf_flexcan_platform_data *pdata;
 	int err, irq;
+	bool big_endian_module;
 	u8 clk_src = 1;
 	u32 clock_freq = 0;
 
@@ -2059,11 +2095,17 @@ static int flexcan_probe(struct platform_device *pdev)
 	else if (IS_ERR(reg_xceiver))
 		return PTR_ERR(reg_xceiver);
 
-	if (pdev->dev.of_node) {
-		of_property_read_u32(pdev->dev.of_node,
-				     "clock-frequency", &clock_freq);
-		of_property_read_u8(pdev->dev.of_node,
-				    "fsl,clk-source", &clk_src);
+	pdata = dev_get_platdata(&pdev->dev);
+	if (pdata) {
+		clock_freq = pdata->clock_frequency;
+		clk_src = pdata->clk_src;
+	} else {
+		if (pdev->dev.of_node) {
+			of_property_read_u32(pdev->dev.of_node,
+					     "clock-frequency", &clock_freq);
+			of_property_read_u8(pdev->dev.of_node,
+					    "fsl,clk-source", &clk_src);
+		}
 	}
 
 	if (!clock_freq) {
@@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device *pdev)
 
 	devtype_data = of_device_get_match_data(&pdev->dev);
 
+	if (pdata && !devtype_data) {
+		devtype_data =
+			(struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
+	}
+
 	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
 	    !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
 		dev_err(&pdev->dev, "CAN-FD mode doesn't work with FIFO mode!\n");
@@ -2110,8 +2157,11 @@ static int flexcan_probe(struct platform_device *pdev)
 
 	priv = netdev_priv(dev);
 
-	if (of_property_read_bool(pdev->dev.of_node, "big-endian") ||
-	    devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN) {
+	big_endian_module = pdata ? pdata->big_endian :
+		of_property_read_bool(pdev->dev.of_node, "big-endian");
+
+	if (big_endian_module ||
+		(devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN)) {
 		priv->read = flexcan_read_be;
 		priv->write = flexcan_write_be;
 	} else {
-- 
2.31.1

RE: [PATCH 1/5] can: flexcan: add platform data for ColdFire

[Joakim Zhang]

> -----Original Message-----
> From: Angelo Dureghello <angelo@kernel-space.org>
> Sent: 2021年6月9日 4:46
> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
> Angelo Dureghello <angelo@kernel-space.org>
> Subject: [PATCH 1/5] can: flexcan: add platform data for ColdFire
> 
> Add platform data object for ColdFire architecture.
> 
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> ---
>  include/linux/platform_data/flexcan-mcf.h | 27 +++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 include/linux/platform_data/flexcan-mcf.h
> 
> diff --git a/include/linux/platform_data/flexcan-mcf.h
> b/include/linux/platform_data/flexcan-mcf.h
> new file mode 100644
> index 000000000000..71fe1a9df084
> --- /dev/null
> +++ b/include/linux/platform_data/flexcan-mcf.h

Should locate CAN header file here? include/linux/can/platform/

Best Regards,
Joakim Zhang
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Flexcan options for ColdFire family
> + *
> + * Copyright (C) 2021  Angelo Dureghello <angelo@kernel-space.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _PLAT_FLEXCAN_MCF_H
> +#define _PLAT_FLEXCAN_MCF_H
> +
> +struct mcf_flexcan_platform_data {
> +	int stop_mode;
> +	int clk_src;
> +	int clock_frequency;
> +	bool big_endian;
> +};
> +
> +#endif /* _PLAT_FLEXCAN_MCF_H */
> --
> 2.31.1

RE: [PATCH 5/5] can: flexcan: add mcf5441x support

[Joakim Zhang]

> -----Original Message-----
> From: Angelo Dureghello <angelo@kernel-space.org>
> Sent: 2021年6月9日 4:46
> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
> Angelo Dureghello <angelo@kernel-space.org>
> Subject: [PATCH 5/5] can: flexcan: add mcf5441x support
> 
> Add flexcan support for NXP ColdFire mcf5441x family.
> 
> This flexcan module is quite similar to imx6 flexcan module, but with some
> exceptions:
> 
> - 3 separate interrupt sources, MB, BOFF and ERR,
> - implements 16 mb only,
> - m68k architecture is not supporting devicetrees, so a
>   platform data check/case has been added,
> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>   module.
> 
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> ---
>  drivers/net/can/flexcan.c | 80 +++++++++++++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
> 57f3635ad8d7..af95273327cd 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_device.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> +#include <linux/platform_data/flexcan-mcf.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> @@ -205,6 +206,10 @@
> 
>  #define FLEXCAN_TIMEOUT_US		(250)
> 
> +#define FLEXCAN_OFFS_INT_BOFF		1
> +#define FLEXCAN_OFFS_INT_ERR		3
> +#define FLEXCAN_MB_CNT_MCF		16

Could you rename the macro, like FLEXCAN_MCF5411X_, then user know this only for mcf5411x? And move this into header file?
Why not put this also into mcf_flexcan_platform_data? The interrupt offset and the numbers of mailboxes would be various from different SoCs.

>  /* FLEXCAN hardware feature flags
>   *
>   * Below is some version info we got:
> @@ -246,6 +251,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>  /* Setup stop mode with SCU firmware to support wakeup */  #define
> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
> +/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts  */
> +#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
> 
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -371,6 +378,12 @@ struct flexcan_priv {
>  	void (*write)(u32 val, void __iomem *addr);  };
> 
> +static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
> +	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> +		FLEXCAN_QUIRK_SETUP_MCF,
> +};
> +
>  static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>  		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct
> flexcan_priv *priv)  {
>  	int err;
> 
> -	err = clk_prepare_enable(priv->clk_ipg);
> -	if (err)
> -		return err;
> +	if (priv->clk_ipg) {
> +		err = clk_prepare_enable(priv->clk_ipg);
> +		if (err)
> +			return err;
> +	}
> 
> -	err = clk_prepare_enable(priv->clk_per);
> -	if (err)
> -		clk_disable_unprepare(priv->clk_ipg);
> +	if (priv->clk_per) {
> +		err = clk_prepare_enable(priv->clk_per);
> +		if (err)
> +			clk_disable_unprepare(priv->clk_ipg);
> +	}

No need do this check, it will be handled in clk_prepare_enable() / clk_disable_unprepare(). So this change is unnecessary.

>  	return err;
>  }
> @@ -1401,8 +1418,13 @@ static int flexcan_rx_offload_setup(struct
> net_device *dev)
>  		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
>  	else
>  		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
> -	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
> -			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
> +
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
> +		priv->mb_count = FLEXCAN_MB_CNT_MCF;
> +	} else {
> +		priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
> +				 (sizeof(priv->regs->mb[1]) / priv->mb_size);
> +	}
> 
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
>  		priv->tx_mb_reserved =
> @@ -1774,6 +1796,18 @@ static int flexcan_open(struct net_device *dev)
>  	if (err)
>  		goto out_can_rx_offload_disable;
> 
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_BOFF,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_can_rx_offload_disable;
> +
> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_ERR,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_can_rx_offload_disable;
> +	}
> +
>  	flexcan_chip_interrupts_enable(dev);
> 
>  	can_led_event(dev, CAN_LED_EVENT_OPEN); @@ -2047,7 +2081,9 @@
> static int flexcan_probe(struct platform_device *pdev)
>  	struct regulator *reg_xceiver;
>  	struct clk *clk_ipg = NULL, *clk_per = NULL;
>  	struct flexcan_regs __iomem *regs;
> +	struct mcf_flexcan_platform_data *pdata;
>  	int err, irq;
> +	bool big_endian_module;
>  	u8 clk_src = 1;
>  	u32 clock_freq = 0;
> 
> @@ -2059,11 +2095,17 @@ static int flexcan_probe(struct platform_device
> *pdev)
>  	else if (IS_ERR(reg_xceiver))
>  		return PTR_ERR(reg_xceiver);
> 
> -	if (pdev->dev.of_node) {
> -		of_property_read_u32(pdev->dev.of_node,
> -				     "clock-frequency", &clock_freq);
> -		of_property_read_u8(pdev->dev.of_node,
> -				    "fsl,clk-source", &clk_src);
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (pdata) {
> +		clock_freq = pdata->clock_frequency;
> +		clk_src = pdata->clk_src;
> +	} else {
> +		if (pdev->dev.of_node) {
> +			of_property_read_u32(pdev->dev.of_node,
> +					     "clock-frequency", &clock_freq);
> +			of_property_read_u8(pdev->dev.of_node,
> +					    "fsl,clk-source", &clk_src);
> +		}
>  	}
> 
>  	if (!clock_freq) {
> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device
> *pdev)
> 
>  	devtype_data = of_device_get_match_data(&pdev->dev);
> 
> +	if (pdata && !devtype_data) {
> +		devtype_data =
> +			(struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
> +	}
> +
>  	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>  	    !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
>  		dev_err(&pdev->dev, "CAN-FD mode doesn't work with FIFO
> mode!\n"); @@ -2110,8 +2157,11 @@ static int flexcan_probe(struct
> platform_device *pdev)
> 
>  	priv = netdev_priv(dev);
> 
> -	if (of_property_read_bool(pdev->dev.of_node, "big-endian") ||
> -	    devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN) {
> +	big_endian_module = pdata ? pdata->big_endian :
> +		of_property_read_bool(pdev->dev.of_node, "big-endian");
> +
> +	if (big_endian_module ||
> +		(devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN)) {
>  		priv->read = flexcan_read_be;
>  		priv->write = flexcan_write_be;
>  	} else {
> --

Best Regards,
Joakim Zhang
> 2.31.1

Re: [PATCH 1/5] can: flexcan: add platform data for ColdFire

[Angelo Dureghello]

Hi Joakim,

On 09/06/21 4:05 AM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Angelo Dureghello <angelo@kernel-space.org>
>> Sent: 2021年6月9日 4:46
>> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
>> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
>> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
>> Angelo Dureghello <angelo@kernel-space.org>
>> Subject: [PATCH 1/5] can: flexcan: add platform data for ColdFire
>>
>> Add platform data object for ColdFire architecture.
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>> ---
>>   include/linux/platform_data/flexcan-mcf.h | 27 +++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>   create mode 100644 include/linux/platform_data/flexcan-mcf.h
>>
>> diff --git a/include/linux/platform_data/flexcan-mcf.h
>> b/include/linux/platform_data/flexcan-mcf.h
>> new file mode 100644
>> index 000000000000..71fe1a9df084
>> --- /dev/null
>> +++ b/include/linux/platform_data/flexcan-mcf.h
> 
> Should locate CAN header file here? include/linux/can/platform/
> 

oh, missed that place, looks like that location is more correct,
sure, can move the file there.

> Best Regards,
> Joakim Zhang
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Flexcan options for ColdFire family
>> + *
>> + * Copyright (C) 2021  Angelo Dureghello <angelo@kernel-space.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _PLAT_FLEXCAN_MCF_H
>> +#define _PLAT_FLEXCAN_MCF_H
>> +
>> +struct mcf_flexcan_platform_data {
>> +	int stop_mode;
>> +	int clk_src;
>> +	int clock_frequency;
>> +	bool big_endian;
>> +};
>> +
>> +#endif /* _PLAT_FLEXCAN_MCF_H */
>> --
>> 2.31.1
> 

Regards
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Geert Uytterhoeven]

Hi Joakim, Angelo,

On Wed, Jun 9, 2021 at 4:05 AM Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> > From: Angelo Dureghello <angelo@kernel-space.org>
> > Sent: 2021年6月9日 4:46
> > To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
> > Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
> > linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
> > Angelo Dureghello <angelo@kernel-space.org>
> > Subject: [PATCH 5/5] can: flexcan: add mcf5441x support
> >
> > Add flexcan support for NXP ColdFire mcf5441x family.
> >
> > This flexcan module is quite similar to imx6 flexcan module, but with some
> > exceptions:
> >
> > - 3 separate interrupt sources, MB, BOFF and ERR,
> > - implements 16 mb only,
> > - m68k architecture is not supporting devicetrees, so a
> >   platform data check/case has been added,
> > - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
> >   module.
> >
> > Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>

> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c

> > @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct
> > flexcan_priv *priv)  {
> >       int err;
> >
> > -     err = clk_prepare_enable(priv->clk_ipg);
> > -     if (err)
> > -             return err;
> > +     if (priv->clk_ipg) {
> > +             err = clk_prepare_enable(priv->clk_ipg);
> > +             if (err)
> > +                     return err;
> > +     }
> >
> > -     err = clk_prepare_enable(priv->clk_per);
> > -     if (err)
> > -             clk_disable_unprepare(priv->clk_ipg);
> > +     if (priv->clk_per) {
> > +             err = clk_prepare_enable(priv->clk_per);
> > +             if (err)
> > +                     clk_disable_unprepare(priv->clk_ipg);
> > +     }
>
> No need do this check, it will be handled in clk_prepare_enable() / clk_disable_unprepare(). So this change is unnecessary.

Except that the non-CCF implementation of clk_enable() in
arch/m68k/coldfire/clk.c still returns -EINVAL instead of NULL.
Any plans to move to CCF? Or at least fix legacy clk_enable().

> > @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device
> > *pdev)
> >
> >       devtype_data = of_device_get_match_data(&pdev->dev);
> >
> > +     if (pdata && !devtype_data) {
> > +             devtype_data =
> > +                     (struct flexcan_devtype_data *)&fsl_mcf_devtype_data;

Cast not needed?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] can: flexcan: add platform data for ColdFire

[Geert Uytterhoeven]

Hi Angelo,

On Tue, Jun 8, 2021 at 10:46 PM Angelo Dureghello
<angelo@kernel-space.org> wrote:
> Add platform data object for ColdFire architecture.
>
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>

Thanks for your patch!

> --- /dev/null
> +++ b/include/linux/platform_data/flexcan-mcf.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Flexcan options for ColdFire family
> + *
> + * Copyright (C) 2021  Angelo Dureghello <angelo@kernel-space.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _PLAT_FLEXCAN_MCF_H
> +#define _PLAT_FLEXCAN_MCF_H
> +
> +struct mcf_flexcan_platform_data {
> +       int stop_mode;

Unused?

> +       int clk_src;
> +       int clock_frequency;

Might be worthwhile to match the types used in the driver
(i.e. u8 and u32).

> +       bool big_endian;
> +};
> +
> +#endif /* _PLAT_FLEXCAN_MCF_H */

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Angelo Dureghello]

Hi Joakim, Geert,

thanks for the feedbacks,

On 09/06/21 4:05 AM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Angelo Dureghello <angelo@kernel-space.org>
>> Sent: 2021年6月9日 4:46
>> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
>> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
>> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
>> Angelo Dureghello <angelo@kernel-space.org>
>> Subject: [PATCH 5/5] can: flexcan: add mcf5441x support
>>
>> Add flexcan support for NXP ColdFire mcf5441x family.
>>
>> This flexcan module is quite similar to imx6 flexcan module, but with some
>> exceptions:
>>
>> - 3 separate interrupt sources, MB, BOFF and ERR,
>> - implements 16 mb only,
>> - m68k architecture is not supporting devicetrees, so a
>>    platform data check/case has been added,
>> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>>    module.
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>> ---
>>   drivers/net/can/flexcan.c | 80 +++++++++++++++++++++++++++++++--------
>>   1 file changed, 65 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index
>> 57f3635ad8d7..af95273327cd 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -28,6 +28,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/platform_data/flexcan-mcf.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>>   #include <linux/regulator/consumer.h>
>> @@ -205,6 +206,10 @@
>>
>>   #define FLEXCAN_TIMEOUT_US		(250)
>>
>> +#define FLEXCAN_OFFS_INT_BOFF		1
>> +#define FLEXCAN_OFFS_INT_ERR		3
>> +#define FLEXCAN_MB_CNT_MCF		16
> 
> Could you rename the macro, like FLEXCAN_MCF5411X_, then user know this only for mcf5411x? And move this into header file?
> Why not put this also into mcf_flexcan_platform_data? The interrupt offset and the numbers of mailboxes would be various from different SoCs.

ok for me, i can name them as

FLEXCAN_MCF5411X_OFFS_INT_BOF
FLEXCAN_MCF5411X_OFFS_INT_ERR
FLEXCAN_MCF5411X_MB_CNT_MCF

and will move them in platform data header


> 
>>   /* FLEXCAN hardware feature flags
>>    *
>>    * Below is some version info we got:
>> @@ -246,6 +251,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>>   /* Setup stop mode with SCU firmware to support wakeup */  #define
>> FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
>> +/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts  */
>> +#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
>>
>>   /* Structure of the message buffer */
>>   struct flexcan_mb {
>> @@ -371,6 +378,12 @@ struct flexcan_priv {
>>   	void (*write)(u32 val, void __iomem *addr);  };
>>
>> +static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
>> +	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> +		FLEXCAN_QUIRK_SETUP_MCF,
>> +};
>> +
>>   static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>>   		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct
>> flexcan_priv *priv)  {
>>   	int err;
>>
>> -	err = clk_prepare_enable(priv->clk_ipg);
>> -	if (err)
>> -		return err;
>> +	if (priv->clk_ipg) {
>> +		err = clk_prepare_enable(priv->clk_ipg);
>> +		if (err)
>> +			return err;
>> +	}
>>
>> -	err = clk_prepare_enable(priv->clk_per);
>> -	if (err)
>> -		clk_disable_unprepare(priv->clk_ipg);
>> +	if (priv->clk_per) {
>> +		err = clk_prepare_enable(priv->clk_per);
>> +		if (err)
>> +			clk_disable_unprepare(priv->clk_ipg);
>> +	}
> 
> No need do this check, it will be handled in clk_prepare_enable() / clk_disable_unprepare(). So this change is unnecessary.
> 

as Geert pointed out, right now without this protection
(that shouldn't anyway harm), probe fails:

[    1.680000] flexcan: probe of flexcan.0 failed with error -22


>>   	return err;
>>   }
>> @@ -1401,8 +1418,13 @@ static int flexcan_rx_offload_setup(struct
>> net_device *dev)
>>   		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
>>   	else
>>   		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
>> -	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> -			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
>> +
>> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> +		priv->mb_count = FLEXCAN_MB_CNT_MCF;
>> +	} else {
>> +		priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> +				 (sizeof(priv->regs->mb[1]) / priv->mb_size);
>> +	}
>>
>>   	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
>>   		priv->tx_mb_reserved =
>> @@ -1774,6 +1796,18 @@ static int flexcan_open(struct net_device *dev)
>>   	if (err)
>>   		goto out_can_rx_offload_disable;
>>
>> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_BOFF,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_can_rx_offload_disable;
>> +
>> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_ERR,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_can_rx_offload_disable;
>> +	}
>> +
>>   	flexcan_chip_interrupts_enable(dev);
>>
>>   	can_led_event(dev, CAN_LED_EVENT_OPEN); @@ -2047,7 +2081,9 @@
>> static int flexcan_probe(struct platform_device *pdev)
>>   	struct regulator *reg_xceiver;
>>   	struct clk *clk_ipg = NULL, *clk_per = NULL;
>>   	struct flexcan_regs __iomem *regs;
>> +	struct mcf_flexcan_platform_data *pdata;
>>   	int err, irq;
>> +	bool big_endian_module;
>>   	u8 clk_src = 1;
>>   	u32 clock_freq = 0;
>>
>> @@ -2059,11 +2095,17 @@ static int flexcan_probe(struct platform_device
>> *pdev)
>>   	else if (IS_ERR(reg_xceiver))
>>   		return PTR_ERR(reg_xceiver);
>>
>> -	if (pdev->dev.of_node) {
>> -		of_property_read_u32(pdev->dev.of_node,
>> -				     "clock-frequency", &clock_freq);
>> -		of_property_read_u8(pdev->dev.of_node,
>> -				    "fsl,clk-source", &clk_src);
>> +	pdata = dev_get_platdata(&pdev->dev);
>> +	if (pdata) {
>> +		clock_freq = pdata->clock_frequency;
>> +		clk_src = pdata->clk_src;
>> +	} else {
>> +		if (pdev->dev.of_node) {
>> +			of_property_read_u32(pdev->dev.of_node,
>> +					     "clock-frequency", &clock_freq);
>> +			of_property_read_u8(pdev->dev.of_node,
>> +					    "fsl,clk-source", &clk_src);
>> +		}
>>   	}
>>
>>   	if (!clock_freq) {
>> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device
>> *pdev)
>>
>>   	devtype_data = of_device_get_match_data(&pdev->dev);
>>
>> +	if (pdata && !devtype_data) {
>> +		devtype_data =
>> +			(struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
>> +	}
>> +
>>   	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>>   	    !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
>>   		dev_err(&pdev->dev, "CAN-FD mode doesn't work with FIFO
>> mode!\n"); @@ -2110,8 +2157,11 @@ static int flexcan_probe(struct
>> platform_device *pdev)
>>
>>   	priv = netdev_priv(dev);
>>
>> -	if (of_property_read_bool(pdev->dev.of_node, "big-endian") ||
>> -	    devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN) {
>> +	big_endian_module = pdata ? pdata->big_endian :
>> +		of_property_read_bool(pdev->dev.of_node, "big-endian");
>> +
>> +	if (big_endian_module ||
>> +		(devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN)) {
>>   		priv->read = flexcan_read_be;
>>   		priv->write = flexcan_write_be;
>>   	} else {
>> --
> 
> Best Regards,
> Joakim Zhang
>> 2.31.1
> 

Best Regards,
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Angelo Dureghello]

Hi Geert,

On 09/06/21 10:12 AM, Geert Uytterhoeven wrote:
> Hi Joakim, Angelo,
> 
> On Wed, Jun 9, 2021 at 4:05 AM Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
>>> From: Angelo Dureghello <angelo@kernel-space.org>
>>> Sent: 2021年6月9日 4:46
>>> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
>>> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
>>> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
>>> Angelo Dureghello <angelo@kernel-space.org>
>>> Subject: [PATCH 5/5] can: flexcan: add mcf5441x support
>>>
>>> Add flexcan support for NXP ColdFire mcf5441x family.
>>>
>>> This flexcan module is quite similar to imx6 flexcan module, but with some
>>> exceptions:
>>>
>>> - 3 separate interrupt sources, MB, BOFF and ERR,
>>> - implements 16 mb only,
>>> - m68k architecture is not supporting devicetrees, so a
>>>    platform data check/case has been added,
>>> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>>>    module.
>>>
>>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> 
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
> 
>>> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct
>>> flexcan_priv *priv)  {
>>>        int err;
>>>
>>> -     err = clk_prepare_enable(priv->clk_ipg);
>>> -     if (err)
>>> -             return err;
>>> +     if (priv->clk_ipg) {
>>> +             err = clk_prepare_enable(priv->clk_ipg);
>>> +             if (err)
>>> +                     return err;
>>> +     }
>>>
>>> -     err = clk_prepare_enable(priv->clk_per);
>>> -     if (err)
>>> -             clk_disable_unprepare(priv->clk_ipg);
>>> +     if (priv->clk_per) {
>>> +             err = clk_prepare_enable(priv->clk_per);
>>> +             if (err)
>>> +                     clk_disable_unprepare(priv->clk_ipg);
>>> +     }
>>
>> No need do this check, it will be handled in clk_prepare_enable() / clk_disable_unprepare(). So this change is unnecessary.
> 
> Except that the non-CCF implementation of clk_enable() in
> arch/m68k/coldfire/clk.c still returns -EINVAL instead of NULL.
> Any plans to move to CCF? Or at least fix legacy clk_enable().
> 
>>> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device
>>> *pdev)
>>>
>>>        devtype_data = of_device_get_match_data(&pdev->dev);
>>>
>>> +     if (pdata && !devtype_data) {
>>> +             devtype_data =
>>> +                     (struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
> 
> Cast not needed?
> 
Thanks, fixed for v2.

> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Regards,
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 7412 bytes --]

On 08.06.2021 22:45:42, Angelo Dureghello wrote:
> Add flexcan support for NXP ColdFire mcf5441x family.
> 
> This flexcan module is quite similar to imx6 flexcan module, but
> with some exceptions:
> 
> - 3 separate interrupt sources, MB, BOFF and ERR,

Please don't hard code the 2 extra interrupts as offsets, but create
proper IRQ resources.

> - implements 16 mb only,
> - m68k architecture is not supporting devicetrees, so a
>   platform data check/case has been added,

You might want to revert:

| 2c0ac9208135 can: flexcan: convert the driver to DT-only

and use it as a starting point.

> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>   module.
> 
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> ---
>  drivers/net/can/flexcan.c | 80 +++++++++++++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 57f3635ad8d7..af95273327cd 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -28,6 +28,7 @@
>  #include <linux/of_device.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> +#include <linux/platform_data/flexcan-mcf.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> @@ -205,6 +206,10 @@
>  
>  #define FLEXCAN_TIMEOUT_US		(250)
>  
> +#define FLEXCAN_OFFS_INT_BOFF		1
> +#define FLEXCAN_OFFS_INT_ERR		3
> +#define FLEXCAN_MB_CNT_MCF		16
> +
>  /* FLEXCAN hardware feature flags
>   *
>   * Below is some version info we got:
> @@ -246,6 +251,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>  /* Setup stop mode with SCU firmware to support wakeup */
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
> +/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts  */
> +#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
>  
>  /* Structure of the message buffer */
>  struct flexcan_mb {
> @@ -371,6 +378,12 @@ struct flexcan_priv {
>  	void (*write)(u32 val, void __iomem *addr);
>  };
>  
> +static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
> +	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> +		FLEXCAN_QUIRK_SETUP_MCF,
> +};
> +
>  static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>  		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct flexcan_priv *priv)
>  {
>  	int err;
>  
> -	err = clk_prepare_enable(priv->clk_ipg);
> -	if (err)
> -		return err;
> +	if (priv->clk_ipg) {
> +		err = clk_prepare_enable(priv->clk_ipg);
> +		if (err)
> +			return err;
> +	}
>  
> -	err = clk_prepare_enable(priv->clk_per);
> -	if (err)
> -		clk_disable_unprepare(priv->clk_ipg);
> +	if (priv->clk_per) {
> +		err = clk_prepare_enable(priv->clk_per);
> +		if (err)
> +			clk_disable_unprepare(priv->clk_ipg);
> +	}
>  
>  	return err;
>  }
> @@ -1401,8 +1418,13 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
>  		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
>  	else
>  		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
> -	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
> -			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
> +
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
> +		priv->mb_count = FLEXCAN_MB_CNT_MCF;
> +	} else {
> +		priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
> +				 (sizeof(priv->regs->mb[1]) / priv->mb_size);
> +	}

nitpick: no { } needed

>  
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
>  		priv->tx_mb_reserved =
> @@ -1774,6 +1796,18 @@ static int flexcan_open(struct net_device *dev)
>  	if (err)
>  		goto out_can_rx_offload_disable;
>  
> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_BOFF,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_can_rx_offload_disable;
> +
> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_ERR,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_can_rx_offload_disable;

I'm missing the free_irq() in error handling and the flexcan_close()
function. Please test your driver if it work still after several
modprobe rmmod cycles.

> +	}
> +
>  	flexcan_chip_interrupts_enable(dev);
>  
>  	can_led_event(dev, CAN_LED_EVENT_OPEN);
> @@ -2047,7 +2081,9 @@ static int flexcan_probe(struct platform_device *pdev)
>  	struct regulator *reg_xceiver;
>  	struct clk *clk_ipg = NULL, *clk_per = NULL;
>  	struct flexcan_regs __iomem *regs;
> +	struct mcf_flexcan_platform_data *pdata;
>  	int err, irq;
> +	bool big_endian_module;
>  	u8 clk_src = 1;
>  	u32 clock_freq = 0;
>  
> @@ -2059,11 +2095,17 @@ static int flexcan_probe(struct platform_device *pdev)
>  	else if (IS_ERR(reg_xceiver))
>  		return PTR_ERR(reg_xceiver);
>  
> -	if (pdev->dev.of_node) {
> -		of_property_read_u32(pdev->dev.of_node,
> -				     "clock-frequency", &clock_freq);
> -		of_property_read_u8(pdev->dev.of_node,
> -				    "fsl,clk-source", &clk_src);
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (pdata) {
> +		clock_freq = pdata->clock_frequency;
> +		clk_src = pdata->clk_src;
> +	} else {
> +		if (pdev->dev.of_node) {

else if

> +			of_property_read_u32(pdev->dev.of_node,
> +					     "clock-frequency", &clock_freq);
> +			of_property_read_u8(pdev->dev.of_node,
> +					    "fsl,clk-source", &clk_src);
> +		}
>  	}
>  
>  	if (!clock_freq) {
> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device *pdev)
>  
>  	devtype_data = of_device_get_match_data(&pdev->dev);
>  
> +	if (pdata && !devtype_data) {
> +		devtype_data =
> +			(struct flexcan_devtype_data *)&fsl_mcf_devtype_data;

That looks broken, see 2c0ac9208135 how to do this properly. Please add a
name "flexcan-mcf" to the id_table and use fsl_mcf_devtype_data as
driver_data.

> +	}
> +
>  	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>  	    !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
>  		dev_err(&pdev->dev, "CAN-FD mode doesn't work with FIFO mode!\n");
> @@ -2110,8 +2157,11 @@ static int flexcan_probe(struct platform_device *pdev)
>  
>  	priv = netdev_priv(dev);
>  
> -	if (of_property_read_bool(pdev->dev.of_node, "big-endian") ||
> -	    devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN) {
> +	big_endian_module = pdata ? pdata->big_endian :
> +		of_property_read_bool(pdev->dev.of_node, "big-endian");

Please add the FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN to you
fsl_mcf_devtype_data if needed. There's no need to add a 3rd way of
configuring this.

> +
> +	if (big_endian_module ||
> +		(devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN)) {
>  		priv->read = flexcan_read_be;
>  		priv->write = flexcan_write_be;
>  	} else {
> -- 
> 2.31.1
> 
>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] can: flexcan: add platform data for ColdFire

[Angelo Dureghello]

Hi Geert,

On 09/06/21 10:14 AM, Geert Uytterhoeven wrote:
> Hi Angelo,
> 
> On Tue, Jun 8, 2021 at 10:46 PM Angelo Dureghello
> <angelo@kernel-space.org> wrote:
>> Add platform data object for ColdFire architecture.
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/include/linux/platform_data/flexcan-mcf.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Flexcan options for ColdFire family
>> + *
>> + * Copyright (C) 2021  Angelo Dureghello <angelo@kernel-space.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _PLAT_FLEXCAN_MCF_H
>> +#define _PLAT_FLEXCAN_MCF_H
>> +
>> +struct mcf_flexcan_platform_data {
>> +       int stop_mode;
> 
> Unused?

yes, unused for now, removed

> 
>> +       int clk_src;
>> +       int clock_frequency;
> 
> Might be worthwhile to match the types used in the driver
> (i.e. u8 and u32).
> 
done, thanks

>> +       bool big_endian;
>> +};
>> +
>> +#endif /* _PLAT_FLEXCAN_MCF_H */
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Regards,
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Angelo Dureghello]

Hi Mark,

thanks for the feedbacks,

On 09/06/21 10:56 AM, Marc Kleine-Budde wrote:
> On 08.06.2021 22:45:42, Angelo Dureghello wrote:
>> Add flexcan support for NXP ColdFire mcf5441x family.
>>
>> This flexcan module is quite similar to imx6 flexcan module, but
>> with some exceptions:
>>
>> - 3 separate interrupt sources, MB, BOFF and ERR,
> 
> Please don't hard code the 2 extra interrupts as offsets, but create
> proper IRQ resources.
> 

ok

>> - implements 16 mb only,
>> - m68k architecture is not supporting devicetrees, so a
>>    platform data check/case has been added,
> 
> You might want to revert:
> 
> | 2c0ac9208135 can: flexcan: convert the driver to DT-only
> 
> and use it as a starting point.
> 

ok

>> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>>    module.
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>> ---
>>   drivers/net/can/flexcan.c | 80 +++++++++++++++++++++++++++++++--------
>>   1 file changed, 65 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 57f3635ad8d7..af95273327cd 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -28,6 +28,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/platform_data/flexcan-mcf.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>>   #include <linux/regulator/consumer.h>
>> @@ -205,6 +206,10 @@
>>   
>>   #define FLEXCAN_TIMEOUT_US		(250)
>>   
>> +#define FLEXCAN_OFFS_INT_BOFF		1
>> +#define FLEXCAN_OFFS_INT_ERR		3
>> +#define FLEXCAN_MB_CNT_MCF		16
>> +
>>   /* FLEXCAN hardware feature flags
>>    *
>>    * Below is some version info we got:
>> @@ -246,6 +251,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_ECC BIT(10)
>>   /* Setup stop mode with SCU firmware to support wakeup */
>>   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCFW BIT(11)
>> +/* Setup for flexcan module as in mcf, 16 mb, 3 separate interrupts  */
>> +#define FLEXCAN_QUIRK_SETUP_MCF BIT(12)
>>   
>>   /* Structure of the message buffer */
>>   struct flexcan_mb {
>> @@ -371,6 +378,12 @@ struct flexcan_priv {
>>   	void (*write)(u32 val, void __iomem *addr);
>>   };
>>   
>> +static const struct flexcan_devtype_data fsl_mcf_devtype_data = {
>> +	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>> +		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> +		FLEXCAN_QUIRK_SETUP_MCF,
>> +};
>> +
>>   static const struct flexcan_devtype_data fsl_p1010_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_BROKEN_WERR_STATE |
>>   		FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct flexcan_priv *priv)
>>   {
>>   	int err;
>>   
>> -	err = clk_prepare_enable(priv->clk_ipg);
>> -	if (err)
>> -		return err;
>> +	if (priv->clk_ipg) {
>> +		err = clk_prepare_enable(priv->clk_ipg);
>> +		if (err)
>> +			return err;
>> +	}
>>   
>> -	err = clk_prepare_enable(priv->clk_per);
>> -	if (err)
>> -		clk_disable_unprepare(priv->clk_ipg);
>> +	if (priv->clk_per) {
>> +		err = clk_prepare_enable(priv->clk_per);
>> +		if (err)
>> +			clk_disable_unprepare(priv->clk_ipg);
>> +	}
>>   
>>   	return err;
>>   }
>> @@ -1401,8 +1418,13 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
>>   		priv->mb_size = sizeof(struct flexcan_mb) + CANFD_MAX_DLEN;
>>   	else
>>   		priv->mb_size = sizeof(struct flexcan_mb) + CAN_MAX_DLEN;
>> -	priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> -			 (sizeof(priv->regs->mb[1]) / priv->mb_size);
>> +
>> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> +		priv->mb_count = FLEXCAN_MB_CNT_MCF;
>> +	} else {
>> +		priv->mb_count = (sizeof(priv->regs->mb[0]) / priv->mb_size) +
>> +				 (sizeof(priv->regs->mb[1]) / priv->mb_size);
>> +	}
> 
> nitpick: no { } needed
> 
ok

>>   
>>   	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)
>>   		priv->tx_mb_reserved =
>> @@ -1774,6 +1796,18 @@ static int flexcan_open(struct net_device *dev)
>>   	if (err)
>>   		goto out_can_rx_offload_disable;
>>   
>> +	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_SETUP_MCF) {
>> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_BOFF,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_can_rx_offload_disable;
>> +
>> +		err = request_irq(dev->irq + FLEXCAN_OFFS_INT_ERR,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_can_rx_offload_disable;
> 
> I'm missing the free_irq() in error handling and the flexcan_close()
> function. Please test your driver if it work still after several
> modprobe rmmod cycles.
> 

forgot, will fix

>> +	}
>> +
>>   	flexcan_chip_interrupts_enable(dev);
>>   
>>   	can_led_event(dev, CAN_LED_EVENT_OPEN);
>> @@ -2047,7 +2081,9 @@ static int flexcan_probe(struct platform_device *pdev)
>>   	struct regulator *reg_xceiver;
>>   	struct clk *clk_ipg = NULL, *clk_per = NULL;
>>   	struct flexcan_regs __iomem *regs;
>> +	struct mcf_flexcan_platform_data *pdata;
>>   	int err, irq;
>> +	bool big_endian_module;
>>   	u8 clk_src = 1;
>>   	u32 clock_freq = 0;
>>   
>> @@ -2059,11 +2095,17 @@ static int flexcan_probe(struct platform_device *pdev)
>>   	else if (IS_ERR(reg_xceiver))
>>   		return PTR_ERR(reg_xceiver);
>>   
>> -	if (pdev->dev.of_node) {
>> -		of_property_read_u32(pdev->dev.of_node,
>> -				     "clock-frequency", &clock_freq);
>> -		of_property_read_u8(pdev->dev.of_node,
>> -				    "fsl,clk-source", &clk_src);
>> +	pdata = dev_get_platdata(&pdev->dev);
>> +	if (pdata) {
>> +		clock_freq = pdata->clock_frequency;
>> +		clk_src = pdata->clk_src;
>> +	} else {
>> +		if (pdev->dev.of_node) {
> 
> else if
> 

ok

>> +			of_property_read_u32(pdev->dev.of_node,
>> +					     "clock-frequency", &clock_freq);
>> +			of_property_read_u8(pdev->dev.of_node,
>> +					    "fsl,clk-source", &clk_src);
>> +		}
>>   	}
>>   
>>   	if (!clock_freq) {
>> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device *pdev)
>>   
>>   	devtype_data = of_device_get_match_data(&pdev->dev);
>>   
>> +	if (pdata && !devtype_data) {
>> +		devtype_data =
>> +			(struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
> 
> That looks broken, see 2c0ac9208135 how to do this properly. Please add a
> name "flexcan-mcf" to the id_table and use fsl_mcf_devtype_data as
> driver_data.

ok

> 
>> +	}
>> +
>>   	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>>   	    !(devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP)) {
>>   		dev_err(&pdev->dev, "CAN-FD mode doesn't work with FIFO mode!\n");
>> @@ -2110,8 +2157,11 @@ static int flexcan_probe(struct platform_device *pdev)
>>   
>>   	priv = netdev_priv(dev);
>>   
>> -	if (of_property_read_bool(pdev->dev.of_node, "big-endian") ||
>> -	    devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN) {
>> +	big_endian_module = pdata ? pdata->big_endian :
>> +		of_property_read_bool(pdev->dev.of_node, "big-endian");
> 
> Please add the FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN to you
> fsl_mcf_devtype_data if needed. There's no need to add a 3rd way of
> configuring this.

i think that quirk is for big endian flexcan modules.
In my case, cpu is big endian but flexcan registers are
little endian, thgis is why i am not using that quirk.

> 
>> +
>> +	if (big_endian_module ||
>> +		(devtype_data->quirks & FLEXCAN_QUIRK_DEFAULT_BIG_ENDIAN)) {
>>   		priv->read = flexcan_read_be;
>>   		priv->write = flexcan_write_be;
>>   	} else {
>> -- 
>> 2.31.1
>>
>>
> 
> Marc
> 

will fix all to a v2

Thanks !
Regards,
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 5/5] can: flexcan: add mcf5441x support

[Greg Ungerer]

Hi Geert,

On 9/6/21 6:12 pm, Geert Uytterhoeven wrote:
> Hi Joakim, Angelo,
> 
> On Wed, Jun 9, 2021 at 4:05 AM Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
>>> From: Angelo Dureghello <angelo@kernel-space.org>
>>> Sent: 2021年6月9日 4:46
>>> To: gerg@linux-m68k.org; wg@grandegger.com; mkl@pengutronix.de
>>> Cc: geert@linux-m68k.org; linux-m68k@vger.kernel.org;
>>> linux-can@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>;
>>> Angelo Dureghello <angelo@kernel-space.org>
>>> Subject: [PATCH 5/5] can: flexcan: add mcf5441x support
>>>
>>> Add flexcan support for NXP ColdFire mcf5441x family.
>>>
>>> This flexcan module is quite similar to imx6 flexcan module, but with some
>>> exceptions:
>>>
>>> - 3 separate interrupt sources, MB, BOFF and ERR,
>>> - implements 16 mb only,
>>> - m68k architecture is not supporting devicetrees, so a
>>>    platform data check/case has been added,
>>> - ColdFire is m68k, so big-endian cpu, with a little-endian flexcan
>>>    module.
>>>
>>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> 
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
> 
>>> @@ -637,13 +650,17 @@ static int flexcan_clks_enable(const struct
>>> flexcan_priv *priv)  {
>>>        int err;
>>>
>>> -     err = clk_prepare_enable(priv->clk_ipg);
>>> -     if (err)
>>> -             return err;
>>> +     if (priv->clk_ipg) {
>>> +             err = clk_prepare_enable(priv->clk_ipg);
>>> +             if (err)
>>> +                     return err;
>>> +     }
>>>
>>> -     err = clk_prepare_enable(priv->clk_per);
>>> -     if (err)
>>> -             clk_disable_unprepare(priv->clk_ipg);
>>> +     if (priv->clk_per) {
>>> +             err = clk_prepare_enable(priv->clk_per);
>>> +             if (err)
>>> +                     clk_disable_unprepare(priv->clk_ipg);
>>> +     }
>>
>> No need do this check, it will be handled in clk_prepare_enable() / clk_disable_unprepare(). So this change is unnecessary.
> 
> Except that the non-CCF implementation of clk_enable() in
> arch/m68k/coldfire/clk.c still returns -EINVAL instead of NULL.
> Any plans to move to CCF? Or at least fix legacy clk_enable().

That was a recent change, see commit c1fb1bf64bb6 ("m68k: let clk_enable() return
immediately if clk is NULL").

It could just as easily just return on that NULL check.

Regards
Greg



>>> @@ -2091,6 +2133,11 @@ static int flexcan_probe(struct platform_device
>>> *pdev)
>>>
>>>        devtype_data = of_device_get_match_data(&pdev->dev);
>>>
>>> +     if (pdata && !devtype_data) {
>>> +             devtype_data =
>>> +                     (struct flexcan_devtype_data *)&fsl_mcf_devtype_data;
> 
> Cast not needed?
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Re: [PATCH 3/5] m68k: m5441x: add flexcan support

[Greg Ungerer]

Hi Angelo,

On 9/6/21 6:45 am, Angelo Dureghello wrote:
> Add flexcan support.
> 
> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
> ---
>   arch/m68k/coldfire/device.c       | 31 +++++++++++++++++++++++++++++++
>   arch/m68k/coldfire/m5441x.c       |  8 ++++----
>   arch/m68k/include/asm/m5441xsim.h | 19 +++++++++++++++++++
>   3 files changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/m68k/coldfire/device.c b/arch/m68k/coldfire/device.c
> index 59f7dfe50a4d..b3263283bf71 100644
> --- a/arch/m68k/coldfire/device.c
> +++ b/arch/m68k/coldfire/device.c
> @@ -23,6 +23,7 @@
>   #include <linux/platform_data/edma.h>
>   #include <linux/platform_data/dma-mcf-edma.h>
>   #include <linux/platform_data/mmc-esdhc-mcf.h>
> +#include <linux/platform_data/flexcan-mcf.h>
>   
>   /*
>    *	All current ColdFire parts contain from 2, 3, 4 or 10 UARTS.
> @@ -581,6 +582,33 @@ static struct platform_device mcf_esdhc = {
>   };
>   #endif /* MCFSDHC_BASE */
>   
> +#if IS_ENABLED(CONFIG_CAN)
> +static struct mcf_flexcan_platform_data mcf_flexcan_info = {
> +	.clk_src = 1,
> +	.clock_frequency = 120000000,
> +};
> +
> +static struct resource mcf_flexcan0_resource[] = {
> +	{
> +		.start = MCFFLEXCAN_BASE0,
> +		.end = MCFFLEXCAN_BASE0 + MCFFLEXCAN_SIZE,
> +		.flags = IORESOURCE_MEM,
> +	}, {
> +		.start = MCF_IRQ_IFL0,
> +		.end = MCF_IRQ_ERR0,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device mcf_flexcan0 = {
> +	.name = "flexcan",
> +	.id = 0,
> +	.num_resources = ARRAY_SIZE(mcf_flexcan0_resource),
> +	.resource = mcf_flexcan0_resource,
> +	.dev.platform_data = &mcf_flexcan_info,
> +};
> +#endif /* IS_ENABLED(CONFIG_CAN) */
> +
>   static struct platform_device *mcf_devices[] __initdata = {
>   	&mcf_uart,
>   #if IS_ENABLED(CONFIG_FEC)
> @@ -616,6 +644,9 @@ static struct platform_device *mcf_devices[] __initdata = {
>   #ifdef MCFSDHC_BASE
>   	&mcf_esdhc,
>   #endif
> +#if IS_ENABLED(CONFIG_CAN)
> +	&mcf_flexcan0,
> +#endif
>   };
>   
>   /*
> diff --git a/arch/m68k/coldfire/m5441x.c b/arch/m68k/coldfire/m5441x.c
> index 1e5259a652d1..18b152edb69c 100644
> --- a/arch/m68k/coldfire/m5441x.c
> +++ b/arch/m68k/coldfire/m5441x.c
> @@ -18,8 +18,8 @@
>   #include <asm/mcfclk.h>
>   
>   DEFINE_CLK(0, "flexbus", 2, MCF_CLK);
> -DEFINE_CLK(0, "mcfcan.0", 8, MCF_CLK);
> -DEFINE_CLK(0, "mcfcan.1", 9, MCF_CLK);
> +DEFINE_CLK(0, "flexcan.0", 8, MCF_CLK);
> +DEFINE_CLK(0, "flexcan.1", 9, MCF_CLK);
>   DEFINE_CLK(0, "imx1-i2c.1", 14, MCF_CLK);

Just a heads up, but this will likely conflict with Arnd's clock changes, see:

     https://lkml.org/lkml/2021/6/8/774

Regards
Greg


>   DEFINE_CLK(0, "mcfdspi.1", 15, MCF_CLK);
>   DEFINE_CLK(0, "edma", 17, MCF_CLK);
> @@ -146,6 +146,8 @@ struct clk *mcf_clks[] = {
>   
>   static struct clk * const enable_clks[] __initconst = {
>   	/* make sure these clocks are enabled */
> +	&__clk_0_8, /* flexcan.0 */
> +	&__clk_0_9, /* flexcan.1 */
>   	&__clk_0_15, /* dspi.1 */
>   	&__clk_0_17, /* eDMA */
>   	&__clk_0_18, /* intc0 */
> @@ -166,8 +168,6 @@ static struct clk * const enable_clks[] __initconst = {
>   	&__clk_1_37, /* gpio */
>   };
>   static struct clk * const disable_clks[] __initconst = {
> -	&__clk_0_8, /* can.0 */
> -	&__clk_0_9, /* can.1 */
>   	&__clk_0_14, /* i2c.1 */
>   	&__clk_0_22, /* i2c.0 */
>   	&__clk_0_23, /* dspi.0 */
> diff --git a/arch/m68k/include/asm/m5441xsim.h b/arch/m68k/include/asm/m5441xsim.h
> index e091e36d3464..f48cf63bd782 100644
> --- a/arch/m68k/include/asm/m5441xsim.h
> +++ b/arch/m68k/include/asm/m5441xsim.h
> @@ -73,6 +73,12 @@
>   #define MCFINT0_FECENTC1	55
>   
>   /* on interrupt controller 1 */
> +#define MCFINT1_FLEXCAN0_IFL	0
> +#define MCFINT1_FLEXCAN0_BOFF	1
> +#define MCFINT1_FLEXCAN0_ERR	3
> +#define MCFINT1_FLEXCAN1_IFL	4
> +#define MCFINT1_FLEXCAN1_BOFF	5
> +#define MCFINT1_FLEXCAN1_ERR	7
>   #define MCFINT1_UART4		48
>   #define MCFINT1_UART5		49
>   #define MCFINT1_UART6		50
> @@ -314,4 +320,17 @@
>   #define MCF_IRQ_SDHC		(MCFINT2_VECBASE + MCFINT2_SDHC)
>   #define MCFSDHC_CLK		(MCFSDHC_BASE + 0x2c)
>   
> +/*
> + * Flexcan module
> + */
> +#define MCFFLEXCAN_BASE0	0xfc020000
> +#define MCFFLEXCAN_BASE1	0xfc024000
> +#define MCFFLEXCAN_SIZE		0x4000
> +#define MCF_IRQ_IFL0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_IFL)
> +#define MCF_IRQ_BOFF0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_BOFF)
> +#define MCF_IRQ_ERR0		(MCFINT1_VECBASE + MCFINT1_FLEXCAN0_ERR)
> +#define MCF_IRQ_IFL1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_IFL)
> +#define MCF_IRQ_BOFF1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_BOFF)
> +#define MCF_IRQ_ERR1		(MCFINT1_VECBASE + MCFINT1_FLEXCAN1_ERR)
> +
>   #endif /* m5441xsim_h */
> 

Re: [PATCH 3/5] m68k: m5441x: add flexcan support

[Angelo Dureghello]

Hi Greg,

On 09/06/21 3:24 PM, Greg Ungerer wrote:
> Hi Angelo,
> 
> On 9/6/21 6:45 am, Angelo Dureghello wrote:
>> Add flexcan support.
>>
>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>> ---
>>   arch/m68k/coldfire/device.c       | 31 +++++++++++++++++++++++++++++++
>>   arch/m68k/coldfire/m5441x.c       |  8 ++++----
>>   arch/m68k/include/asm/m5441xsim.h | 19 +++++++++++++++++++
>>   3 files changed, 54 insertions(+), 4 deletions(-)
>>

>> diff --git a/arch/m68k/coldfire/m5441x.c b/arch/m68k/coldfire/m5441x.c
>> index 1e5259a652d1..18b152edb69c 100644
>> --- a/arch/m68k/coldfire/m5441x.c
>> +++ b/arch/m68k/coldfire/m5441x.c
>> @@ -18,8 +18,8 @@
>>   #include <asm/mcfclk.h>
>>   DEFINE_CLK(0, "flexbus", 2, MCF_CLK);
>> -DEFINE_CLK(0, "mcfcan.0", 8, MCF_CLK);
>> -DEFINE_CLK(0, "mcfcan.1", 9, MCF_CLK);
>> +DEFINE_CLK(0, "flexcan.0", 8, MCF_CLK);
>> +DEFINE_CLK(0, "flexcan.1", 9, MCF_CLK);
>>   DEFINE_CLK(0, "imx1-i2c.1", 14, MCF_CLK);
> 
> Just a heads up, but this will likely conflict with Arnd's clock 
> changes, see:
> 
>      https://lkml.org/lkml/2021/6/8/774
> 

is this a clock naming issue ? The "mcfcan" is actually
not referenced in any driver so naming it "flexcan"
shouldn't be an issue.

Or i should get and enable "mcfcan" clock from the driver ?

> Regards
> Greg
> 

Thanks,
regards,
-- 
Angelo Dureghello
+++ kernelspace +++
+E: angelo AT kernel-space.org
+W: www.kernel-space.org

Re: [PATCH 3/5] m68k: m5441x: add flexcan support

[Greg Ungerer]

Hi Angelo,

On 10/6/21 5:59 pm, Angelo Dureghello wrote:
> Hi Greg,
> 
> On 09/06/21 3:24 PM, Greg Ungerer wrote:
>> Hi Angelo,
>>
>> On 9/6/21 6:45 am, Angelo Dureghello wrote:
>>> Add flexcan support.
>>>
>>> Signed-off-by: Angelo Dureghello <angelo@kernel-space.org>
>>> ---
>>>   arch/m68k/coldfire/device.c       | 31 +++++++++++++++++++++++++++++++
>>>   arch/m68k/coldfire/m5441x.c       |  8 ++++----
>>>   arch/m68k/include/asm/m5441xsim.h | 19 +++++++++++++++++++
>>>   3 files changed, 54 insertions(+), 4 deletions(-)
>>>
> 
>>> diff --git a/arch/m68k/coldfire/m5441x.c b/arch/m68k/coldfire/m5441x.c
>>> index 1e5259a652d1..18b152edb69c 100644
>>> --- a/arch/m68k/coldfire/m5441x.c
>>> +++ b/arch/m68k/coldfire/m5441x.c
>>> @@ -18,8 +18,8 @@
>>>   #include <asm/mcfclk.h>
>>>   DEFINE_CLK(0, "flexbus", 2, MCF_CLK);
>>> -DEFINE_CLK(0, "mcfcan.0", 8, MCF_CLK);
>>> -DEFINE_CLK(0, "mcfcan.1", 9, MCF_CLK);
>>> +DEFINE_CLK(0, "flexcan.0", 8, MCF_CLK);
>>> +DEFINE_CLK(0, "flexcan.1", 9, MCF_CLK);
>>>   DEFINE_CLK(0, "imx1-i2c.1", 14, MCF_CLK);
>>
>> Just a heads up, but this will likely conflict with Arnd's clock changes, see:
>>
>>      https://lkml.org/lkml/2021/6/8/774
>>
> 
> is this a clock naming issue ? The "mcfcan" is actually
> not referenced in any driver so naming it "flexcan"
> shouldn't be an issue.

Yeah, no problem with that. The "mcfcan" name was really just a
place holder here. Not used previously.

My comment is more that your patches and Anrd's are touching
some of the same things. Depending on whose is applied first one
or the other will need to be updated to reflect the others.
No big deal, and easy to do in this case.

Regards
Greg



> Or i should get and enable "mcfcan" clock from the driver ?
> 
>> Regards
>> Greg
>>
> 
> Thanks,
> regards,