[PATCH v4 0/4] Enable multiple MCAN on AM62x

[PATCH v4 0/4] Enable multiple MCAN on AM62x

[Judith Mendez]

On AM62x there is one MCAN in MAIN domain and two in MCU domain.
The MCANs in MCU domain were not enabled since there is no
hardware interrupt routed to A53 GIC interrupt controller.
Therefore A53 Linux cannot be interrupted by MCU MCANs.

This solution instantiates a hrtimer with 1 ms polling interval
for MCAN device when there is no hardware interrupt and there is
poll-interval property in DTB MCAN node. The hrtimer generates a
recurring software interrupt which allows to call the isr. The isr
will check if there is pending transaction by reading a register
and proceed normally if there is.

On AM62x, this series enables two MCU MCAN which will use the hrtimer
implementation. MCANs with hardware interrupt routed to A53 Linux
will continue to use the hardware interrupt as expected.

Timer polling method was tested on both classic CAN and CAN-FD
at 125 KBPS, 250 KBPS, 1 MBPS and 2.5 MBPS with 4 MBPS bitrate
switching.

Letency and CPU load benchmarks were tested on 3x MCAN on AM62x.
1 MBPS timer polling interval is the better timer polling interval
since it has comparable latency to hardware interrupt with the worse
case being 1ms + CAN frame propagation time and CPU load is not
substantial. Latency can be improved further with less than 1 ms
polling intervals, howerver it is at the cost of CPU usage since CPU
load increases at 0.5 ms.

Note that in terms of power, enabling MCU MCANs with timer-polling
implementation might have negative impact since we will have to wake
up every 1 ms whether there are CAN packets pending in the RX FIFO or
not. This might prevent the CPU from entering into deeper idle states
for extended periods of time.

This patch series depends on 'Enable CAN PHY transceiver driver':
Link: https://lore.kernel.org/lkml/775ec9ce-7668-429c-a977-6c8995968d6e@app.fastmail.com/T/

v2:
Link: https://lore.kernel.org/linux-can/20230424195402.516-1-jm@ti.com/T/#t

V1:
Link: https://lore.kernel.org/linux-can/19d8ae7f-7b74-a869-a818-93b74d106709@ti.com/T/#t

RFC:
Link: https://lore.kernel.org/linux-can/52a37e51-4143-9017-42ee-8d17c67028e3@ti.com/T/#t

Changes since v3:
- Wrong patch sent, resend correct patch series

Changes since v2:
- Change binding patch first
- Update binding poll-interval description
- Add oneOf to select either interrupts/interrupt-names or poll-interval
- Sort list of includes
- Create a define for 1 ms polling interval
- Change plarform_get_irq to optional to not print error msg
- Fix indentations, lengths of code lines, and added other style changes

Changes since v1:
- Add poll-interval property to bindings and MCAN DTB node
- Add functionality to check for 'poll-interval' property in MCAN node 
- Bindings: add an example using poll-interval
- Add 'polling' flag in driver to check if device is using polling method
- Check for both timer polling and hardware interrupt case, default to
hardware interrupt method
- Change ns_to_ktime() to ms_to_ktime()

Judith Mendez (4):
  dt-bindings: net: can: Add poll-interval for MCAN
  can: m_can: Add hrtimer to generate software interrupt
  arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay
  arm64: dts: ti: Enable MCU MCANs for AM62x

 .../bindings/net/can/bosch,m_can.yaml         | 36 +++++++++++-
 arch/arm64/boot/dts/ti/Makefile               |  2 +
 arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi       | 24 ++++++++
 .../boot/dts/ti/k3-am625-sk-mcan-main.dtso    | 35 ++++++++++++
 .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 57 +++++++++++++++++++
 drivers/net/can/m_can/m_can.c                 | 29 +++++++++-
 drivers/net/can/m_can/m_can.h                 |  4 ++
 drivers/net/can/m_can/m_can_platform.c        | 32 ++++++++++-
 8 files changed, 212 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso
 create mode 100644 arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso


base-commit: 92e815cf07ed24ee1c51b122f24ffcf2964b4b13
-- 
2.17.1

[PATCH v4 1/4] dt-bindings: net: can: Add poll-interval for MCAN

[Judith Mendez]

On AM62x SoC, MCANs on MCU domain do not have hardware interrupt
routed to A53 Linux, instead they will use software interrupt by
hrtimer. To enable timer method, interrupts should be optional so
remove interrupts property from required section and introduce
poll-interval property.

Signed-off-by: Judith Mendez <jm@ti.com>
---
Changelog:
v3:
 1. Move binding patch to first in series
 2. Update description for poll-interval
 3. Add oneOf to specify using interrupts/interrupt-names or poll-interval
 4. Fix example property: add comment below 'example'

v2:
  1. Add poll-interval property to enable timer polling method
  2. Add example using poll-interval property
  
 .../bindings/net/can/bosch,m_can.yaml         | 36 +++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
index 67879aab623b..c024ee49962c 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -14,6 +14,13 @@ maintainers:
 allOf:
   - $ref: can-controller.yaml#
 
+oneOf:
+  - required:
+      - interrupts
+      - interrupt-names
+  - required:
+      - poll-interval
+
 properties:
   compatible:
     const: bosch,m_can
@@ -40,6 +47,14 @@ properties:
       - const: int1
     minItems: 1
 
+  poll-interval:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Enable hrtimer polling method for an M_CAN device.
+      If this property is defined in MCAN node, it tells the driver to
+      enable polling method for an MCAN device. If for an MCAN device,
+      hardware interrupt is found and hrtimer polling method is enabled,
+      the driver will use hardware interrupt method.
+
   clocks:
     items:
       - description: peripheral clock
@@ -122,8 +137,6 @@ required:
   - compatible
   - reg
   - reg-names
-  - interrupts
-  - interrupt-names
   - clocks
   - clock-names
   - bosch,mram-cfg
@@ -132,6 +145,7 @@ additionalProperties: false
 
 examples:
   - |
+    // Example with interrupts
     #include <dt-bindings/clock/imx6sx-clock.h>
     can@20e8000 {
       compatible = "bosch,m_can";
@@ -149,4 +163,22 @@ examples:
       };
     };
 
+  - |
+    // Example with timer polling
+    #include <dt-bindings/clock/imx6sx-clock.h>
+    can@20e8000 {
+      compatible = "bosch,m_can";
+      reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
+      reg-names = "m_can", "message_ram";
+      poll-interval;
+      clocks = <&clks IMX6SX_CLK_CANFD>,
+               <&clks IMX6SX_CLK_CANFD>;
+      clock-names = "hclk", "cclk";
+      bosch,mram-cfg = <0x0 0 0 32 0 0 0 1>;
+
+      can-transceiver {
+        max-bitrate = <5000000>;
+      };
+    };
+
 ...
-- 
2.17.1

[PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Judith Mendez]

Add an hrtimer to MCAN class device. Each MCAN will have its own
hrtimer instantiated if there is no hardware interrupt found and
poll-interval property is defined in device tree M_CAN node.

The hrtimer will generate a software interrupt every 1 ms. In
hrtimer callback, we check if there is a transaction pending by
reading a register, then process by calling the isr if there is.

Signed-off-by: Judith Mendez <jm@ti.com>
---
Changelog:
v1:
 1. Sort list of includes
 2. Create a define for HR_TIMER_POLL_INTERVAL
 3. Fix indentations and style issues/warnings
 4. Change polling variable to type bool
 5. Change platform_get_irq to optional so not to print error msg
 6. Move error check for addr directly after assignment
 7. Print appropriate error msg with dev_err_probe insead of dev_dbg

v2:
 1. Add poll-interval to MCAN class device to check if poll-interval propery is
    present in MCAN node, this enables timer polling method
 2. Add 'polling' flag to MCAN class device to check if a device is using timer
    polling method
 3. Check if both timer polling and hardware interrupt are enabled for a MCAN
    device, default to hardware interrupt mode if both are enabled
 4. Change ms_to_ktime() to ns_to_ktime()
 5. Remove newlines, tabs, and restructure if/else section
 
 drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
 drivers/net/can/m_can/m_can.h          |  4 ++++
 drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a5003435802b..e1ac0c1d85a3 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -11,6 +11,7 @@
 #include <linux/bitfield.h>
 #include <linux/can/dev.h>
 #include <linux/ethtool.h>
+#include <linux/hrtimer.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -308,6 +309,9 @@ enum m_can_reg {
 #define TX_EVENT_MM_MASK	GENMASK(31, 24)
 #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
 
+/* Hrtimer polling interval */
+#define HRTIMER_POLL_INTERVAL		1
+
 /* The ID and DLC registers are adjacent in M_CAN FIFO memory,
  * and we can save a (potentially slow) bus round trip by combining
  * reads and writes to them.
@@ -1587,6 +1591,11 @@ static int m_can_close(struct net_device *dev)
 	if (!cdev->is_peripheral)
 		napi_disable(&cdev->napi);
 
+	if (cdev->polling) {
+		dev_dbg(cdev->dev, "Disabling the hrtimer\n");
+		hrtimer_cancel(&cdev->hrtimer);
+	}
+
 	m_can_stop(dev);
 	m_can_clk_stop(cdev);
 	free_irq(dev->irq, dev);
@@ -1793,6 +1802,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
+{
+	struct m_can_classdev *cdev = container_of(timer, struct
+						   m_can_classdev, hrtimer);
+
+	m_can_isr(0, cdev->net);
+
+	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL));
+
+	return HRTIMER_RESTART;
+}
+
 static int m_can_open(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -1827,13 +1848,17 @@ static int m_can_open(struct net_device *dev)
 		}
 
 		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
-
 		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
 					   IRQF_ONESHOT,
 					   dev->name, dev);
-	} else {
+	} else if (!cdev->polling) {
 		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
 				  dev);
+	} else {
+		dev_dbg(cdev->dev, "Start hrtimer\n");
+		cdev->hrtimer.function = &hrtimer_callback;
+		hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL),
+			      HRTIMER_MODE_REL_PINNED);
 	}
 
 	if (err < 0) {
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index a839dc71dc9b..e9db5cce4e68 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/freezer.h>
+#include <linux/hrtimer.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -93,6 +94,9 @@ struct m_can_classdev {
 	int is_peripheral;
 
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
+
+	struct hrtimer hrtimer;
+	bool polling;
 };
 
 struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 9c1dcf838006..0fcb436298f8 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -5,6 +5,7 @@
 //
 // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
 
+#include <linux/hrtimer.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 
@@ -96,12 +97,37 @@ static int m_can_plat_probe(struct platform_device *pdev)
 		goto probe_fail;
 
 	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
-	irq = platform_get_irq_byname(pdev, "int0");
-	if (IS_ERR(addr) || irq < 0) {
-		ret = -EINVAL;
+	if (IS_ERR(addr)) {
+		ret = PTR_ERR(addr);
 		goto probe_fail;
 	}
 
+	irq = platform_get_irq_byname_optional(pdev, "int0");
+	if (irq == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto probe_fail;
+	}
+
+	if (device_property_present(mcan_class->dev, "poll-interval"))
+		mcan_class->polling = 1;
+
+	if (!mcan_class->polling && irq < 0) {
+		ret = -ENXIO;
+		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
+		goto probe_fail;
+	}
+
+	if (mcan_class->polling) {
+		if (irq > 0) {
+			mcan_class->polling = 0;
+			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
+		} else {
+			dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
+			hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC,
+				     HRTIMER_MODE_REL_PINNED);
+		}
+	}
+
 	/* message ram could be shared */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
 	if (!res) {
-- 
2.17.1

[PATCH v4 3/4] DO_NOT_MERGE arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay

[Judith Mendez]

Add an overlay for main domain MCAN on AM62x SK. The AM62x
SK board does not have on-board CAN transceiver so instead
of changing the DTB permanently, add an overlay to enable
MAIN domain MCAN and support for 1 CAN transceiver.

This DT overlay can be used with the following EVM:
Link: https://www.ti.com/tool/TCAN1042DEVM

Signed-off-by: Judith Mendez <jm@ti.com>
---
Changelog:
v3:
 1. Add link for specific board
 
 arch/arm64/boot/dts/ti/Makefile               |  2 ++
 .../boot/dts/ti/k3-am625-sk-mcan-main.dtso    | 35 +++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso

diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
index c83c9d772b81..abe15e76b614 100644
--- a/arch/arm64/boot/dts/ti/Makefile
+++ b/arch/arm64/boot/dts/ti/Makefile
@@ -9,8 +9,10 @@
 # alphabetically.
 
 # Boards with AM62x SoC
+k3-am625-sk-mcan-dtbs := k3-am625-sk.dtb k3-am625-sk-mcan-main.dtbo
 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
 dtb-$(CONFIG_ARCH_K3) += k3-am625-sk.dtb
+dtb-$(CONFIG_ARCH_K3) += k3-am625-sk-mcan.dtb
 dtb-$(CONFIG_ARCH_K3) += k3-am62-lp-sk.dtb
 
 # Boards with AM62Ax SoC
diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso
new file mode 100644
index 000000000000..0a7b2f394f87
--- /dev/null
+++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * DT overlay for MCAN transceiver in main domain on AM625 SK
+ *
+ * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include "k3-pinctrl.h"
+
+&{/} {
+	transceiver1: can-phy0 {
+		compatible = "ti,tcan1042";
+		#phy-cells = <0>;
+		max-bitrate = <5000000>;
+	};
+};
+
+&main_pmx0 {
+	main_mcan0_pins_default: main-mcan0-pins-default {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x1dc, PIN_INPUT, 0) /* (E15) MCAN0_RX */
+			AM62X_IOPAD(0x1d8, PIN_OUTPUT, 0) /* (C15) MCAN0_TX */
+		>;
+	};
+};
+
+&main_mcan0 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&main_mcan0_pins_default>;
+	phys = <&transceiver1>;
+};
-- 
2.17.1

[PATCH v4 4/4] DO_NOT_MERGE arm64: dts: ti: Enable MCU MCANs for AM62x

[Judith Mendez]

On AM62x there are no hardware interrupts routed to A53 GIC
interrupt controller for MCU MCAN IPs, so MCU MCANs were not
added to the MCU dtsi. In this patch series an hrtimer is introduced
to MCAN driver to generate software interrupts. Now add MCU MCAN
nodes to the MCU dtsi but disable the MCAN devices by default.

AM62x does not carry on-board CAN transceivers, so instead of
changing DTB permanently use an overlay to enable MCU MCANs and to
add CAN transceiver nodes.

If there is no hardware interrupt and timer method is used, remove
interrupt properties and add poll-interval to enable the hrtimer
per MCAN node.

This DT overlay can be used with the following EVM:
Link: https://www.ti.com/tool/TCAN1042DEVM

Signed-off-by: Judith Mendez <jm@ti.com>
---
Changelog:
v3:
 1. Add link for specific board
 
 arch/arm64/boot/dts/ti/Makefile               |  2 +-
 arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi       | 24 ++++++++
 .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 57 +++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso

diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
index abe15e76b614..c76be3888e4d 100644
--- a/arch/arm64/boot/dts/ti/Makefile
+++ b/arch/arm64/boot/dts/ti/Makefile
@@ -9,7 +9,7 @@
 # alphabetically.
 
 # Boards with AM62x SoC
-k3-am625-sk-mcan-dtbs := k3-am625-sk.dtb k3-am625-sk-mcan-main.dtbo
+k3-am625-sk-mcan-dtbs := k3-am625-sk.dtb k3-am625-sk-mcan-main.dtbo k3-am625-sk-mcan-mcu.dtbo
 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
 dtb-$(CONFIG_ARCH_K3) += k3-am625-sk.dtb
 dtb-$(CONFIG_ARCH_K3) += k3-am625-sk-mcan.dtb
diff --git a/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi b/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
index 076601a41e84..20462f457643 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-mcu.dtsi
@@ -141,4 +141,28 @@
 		/* Tightly coupled to M4F */
 		status = "reserved";
 	};
+
+	mcu_mcan1: can@4e00000 {
+		compatible = "bosch,m_can";
+		reg = <0x00 0x4e00000 0x00 0x8000>,
+			  <0x00 0x4e08000 0x00 0x200>;
+		reg-names = "message_ram", "m_can";
+		power-domains = <&k3_pds 188 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 188 6>, <&k3_clks 188 1>;
+		clock-names = "hclk", "cclk";
+		bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+		status = "disabled";
+	};
+
+	mcu_mcan2: can@4e10000 {
+		compatible = "bosch,m_can";
+		reg = <0x00 0x4e10000 0x00 0x8000>,
+			  <0x00 0x4e18000 0x00 0x200>;
+		reg-names = "message_ram", "m_can";
+		power-domains = <&k3_pds 189 TI_SCI_PD_EXCLUSIVE>;
+		clocks = <&k3_clks 189 6>, <&k3_clks 189 1>;
+		clock-names = "hclk", "cclk";
+		bosch,mram-cfg = <0x0 128 64 64 64 64 32 32>;
+		status = "disabled";
+	};
 };
diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
new file mode 100644
index 000000000000..5145b3de4f9b
--- /dev/null
+++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * DT overlay for MCAN in MCU domain on AM625 SK
+ *
+ * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include "k3-pinctrl.h"
+
+&{/} {
+	transceiver2: can-phy1 {
+		compatible = "ti,tcan1042";
+		#phy-cells = <0>;
+		max-bitrate = <5000000>;
+	};
+
+	transceiver3: can-phy2 {
+		compatible = "ti,tcan1042";
+		#phy-cells = <0>;
+		max-bitrate = <5000000>;
+	};
+};
+
+&mcu_pmx0 {
+	mcu_mcan1_pins_default: mcu-mcan1-pins-default {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x038, PIN_INPUT, 0) /* (B3) MCU_MCAN0_RX */
+			AM62X_IOPAD(0x034, PIN_OUTPUT, 0) /* (D6) MCU_MCAN0_TX */
+		>;
+	};
+
+	mcu_mcan2_pins_default: mcu-mcan2-pins-default {
+		pinctrl-single,pins = <
+			AM62X_IOPAD(0x040, PIN_INPUT, 0) /* (D4) MCU_MCAN1_RX */
+			AM62X_IOPAD(0x03C, PIN_OUTPUT, 0) /* (E5) MCU_MCAN1_TX */
+		>;
+	};
+};
+
+&mcu_mcan1 {
+	poll-interval;
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcu_mcan1_pins_default>;
+	phys = <&transceiver2>;
+	status = "okay";
+};
+
+&mcu_mcan2 {
+	poll-interval;
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcu_mcan2_pins_default>;
+	phys = <&transceiver3>;
+	status = "okay";
+};
-- 
2.17.1

Re: [PATCH v4 3/4] DO_NOT_MERGE arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay

[Nishanth Menon]

On 17:46-20230501, Judith Mendez wrote:
> Add an overlay for main domain MCAN on AM62x SK. The AM62x
> SK board does not have on-board CAN transceiver so instead
> of changing the DTB permanently, add an overlay to enable
> MAIN domain MCAN and support for 1 CAN transceiver.
> 
> This DT overlay can be used with the following EVM:
> Link: https://www.ti.com/tool/TCAN1042DEVM
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
> Changelog:
> v3:
>  1. Add link for specific board
>  
>  arch/arm64/boot/dts/ti/Makefile               |  2 ++
>  .../boot/dts/ti/k3-am625-sk-mcan-main.dtso    | 35 +++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso
> 
> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
> index c83c9d772b81..abe15e76b614 100644
> --- a/arch/arm64/boot/dts/ti/Makefile
> +++ b/arch/arm64/boot/dts/ti/Makefile
> @@ -9,8 +9,10 @@
>  # alphabetically.
>  
>  # Boards with AM62x SoC
> +k3-am625-sk-mcan-dtbs := k3-am625-sk.dtb k3-am625-sk-mcan-main.dtbo

NAK.
https://lore.kernel.org/all/4e406c96-3f47-1695-324f-a9e45be8c142@ti.com/

Same reasons - we don't want specific instance based overlays please -
that would'nt make sense - maxbitrate will depend on transceiver so it
has nothing to do with mcan-main or mcan-mcu and has everything to do
with the board that is plugged in.

>  dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am625-sk.dtb
> +dtb-$(CONFIG_ARCH_K3) += k3-am625-sk-mcan.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am62-lp-sk.dtb
>  
>  # Boards with AM62Ax SoC
> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso
> new file mode 100644
> index 000000000000..0a7b2f394f87
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-main.dtso

If you are going down this road: am625-sk-tcan10242d.dsto (enable main
and mcu?) or something reasonable. Though looking at the pins, I fail
to see how this physically plugs into AM625-SK (I am hoping the answer
isn't breadboard or jumper wires..).



> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * DT overlay for MCAN transceiver in main domain on AM625 SK
> + *
> + * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include "k3-pinctrl.h"
> +
> +&{/} {
> +	transceiver1: can-phy0 {
> +		compatible = "ti,tcan1042";
> +		#phy-cells = <0>;
> +		max-bitrate = <5000000>;
> +	};
> +};
> +
> +&main_pmx0 {
> +	main_mcan0_pins_default: main-mcan0-pins-default {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x1dc, PIN_INPUT, 0) /* (E15) MCAN0_RX */
> +			AM62X_IOPAD(0x1d8, PIN_OUTPUT, 0) /* (C15) MCAN0_TX */
> +		>;
> +	};
> +};
> +
> +&main_mcan0 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&main_mcan0_pins_default>;
> +	phys = <&transceiver1>;
> +};
> -- 
> 2.17.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 01.05.2023 17:46:22, Judith Mendez wrote:
> Add an hrtimer to MCAN class device. Each MCAN will have its own
> hrtimer instantiated if there is no hardware interrupt found and
> poll-interval property is defined in device tree M_CAN node.
> 
> The hrtimer will generate a software interrupt every 1 ms. In
> hrtimer callback, we check if there is a transaction pending by
> reading a register, then process by calling the isr if there is.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>

I think this patch is as good as it gets, given the HW and SW
limitations of the coprocessor.

Some minor nitpicks inline. No need to resend from my point of view,
I'll fixup while applying the patch.

Marc

> ---
> Changelog:
> v1:
>  1. Sort list of includes
>  2. Create a define for HR_TIMER_POLL_INTERVAL
>  3. Fix indentations and style issues/warnings
>  4. Change polling variable to type bool
>  5. Change platform_get_irq to optional so not to print error msg
>  6. Move error check for addr directly after assignment
>  7. Print appropriate error msg with dev_err_probe insead of dev_dbg
> 
> v2:
>  1. Add poll-interval to MCAN class device to check if poll-interval propery is
>     present in MCAN node, this enables timer polling method
>  2. Add 'polling' flag to MCAN class device to check if a device is using timer
>     polling method
>  3. Check if both timer polling and hardware interrupt are enabled for a MCAN
>     device, default to hardware interrupt mode if both are enabled
>  4. Change ms_to_ktime() to ns_to_ktime()
>  5. Remove newlines, tabs, and restructure if/else section
>  
>  drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
>  drivers/net/can/m_can/m_can.h          |  4 ++++
>  drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..e1ac0c1d85a3 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -11,6 +11,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/can/dev.h>
>  #include <linux/ethtool.h>
> +#include <linux/hrtimer.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> @@ -308,6 +309,9 @@ enum m_can_reg {
>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>  
> +/* Hrtimer polling interval */
> +#define HRTIMER_POLL_INTERVAL		1
> +
>  /* The ID and DLC registers are adjacent in M_CAN FIFO memory,
>   * and we can save a (potentially slow) bus round trip by combining
>   * reads and writes to them.
> @@ -1587,6 +1591,11 @@ static int m_can_close(struct net_device *dev)
>  	if (!cdev->is_peripheral)
>  		napi_disable(&cdev->napi);
>  
> +	if (cdev->polling) {
> +		dev_dbg(cdev->dev, "Disabling the hrtimer\n");
> +		hrtimer_cancel(&cdev->hrtimer);
> +	}
> +
>  	m_can_stop(dev);
>  	m_can_clk_stop(cdev);
>  	free_irq(dev->irq, dev);
> @@ -1793,6 +1802,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	struct m_can_classdev *cdev = container_of(timer, struct
> +						   m_can_classdev, hrtimer);
> +
> +	m_can_isr(0, cdev->net);
> +
> +	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL));
> +
> +	return HRTIMER_RESTART;
> +}
> +
>  static int m_can_open(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -1827,13 +1848,17 @@ static int m_can_open(struct net_device *dev)
>  		}
>  
>  		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
> -

unrelated change

>  		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>  					   IRQF_ONESHOT,
>  					   dev->name, dev);
> -	} else {
> +	} else if (!cdev->polling) {
>  		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>  				  dev);
> +	} else {
> +		dev_dbg(cdev->dev, "Start hrtimer\n");
> +		cdev->hrtimer.function = &hrtimer_callback;
> +		hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL),
> +			      HRTIMER_MODE_REL_PINNED);
>  	}
>  
>  	if (err < 0) {
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index a839dc71dc9b..e9db5cce4e68 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/freezer.h>
> +#include <linux/hrtimer.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> @@ -93,6 +94,9 @@ struct m_can_classdev {
>  	int is_peripheral;
>  
>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
> +
> +	struct hrtimer hrtimer;
> +	bool polling;
>  };
>  
>  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index 9c1dcf838006..0fcb436298f8 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -5,6 +5,7 @@
>  //
>  // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
>  
> +#include <linux/hrtimer.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  
> @@ -96,12 +97,37 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  		goto probe_fail;
>  
>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
> -	irq = platform_get_irq_byname(pdev, "int0");
> -	if (IS_ERR(addr) || irq < 0) {
> -		ret = -EINVAL;
> +	if (IS_ERR(addr)) {
> +		ret = PTR_ERR(addr);
>  		goto probe_fail;
>  	}
>  
> +	irq = platform_get_irq_byname_optional(pdev, "int0");
> +	if (irq == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto probe_fail;
> +	}
> +
> +	if (device_property_present(mcan_class->dev, "poll-interval"))
> +		mcan_class->polling = 1;

true

> +
> +	if (!mcan_class->polling && irq < 0) {
> +		ret = -ENXIO;
> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
> +		goto probe_fail;
> +	}
> +
> +	if (mcan_class->polling) {
> +		if (irq > 0) {
> +			mcan_class->polling = 0;

false

> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");

"...using hardware IRQ"

Use dev_info(), as there is something not 100% correct with the DT.

> +		} else {
> +			dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
> +			hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC,
> +				     HRTIMER_MODE_REL_PINNED);
> +		}
> +	}
> +
>  	/* message ram could be shared */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
>  	if (!res) {
> -- 
> 2.17.1
> 
> 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v4 0/4] Enable multiple MCAN on AM62x

[Krzysztof Kozlowski]

On 02/05/2023 00:46, Judith Mendez wrote:
> On AM62x there is one MCAN in MAIN domain and two in MCU domain.
> The MCANs in MCU domain were not enabled since there is no
> hardware interrupt routed to A53 GIC interrupt controller.
> Therefore A53 Linux cannot be interrupted by MCU MCANs.
> 
> This solution instantiates a hrtimer with 1 ms polling interval
> for MCAN device when there is no hardware interrupt and there is
> poll-interval property in DTB MCAN node. The hrtimer generates a
> recurring software interrupt which allows to call the isr. The isr
> will check if there is pending transaction by reading a register
> and proceed normally if there is.
> 
> On AM62x, this series enables two MCU MCAN which will use the hrtimer
> implementation. MCANs with hardware interrupt routed to A53 Linux
> will continue to use the hardware interrupt as expected.
> 
> Timer polling method was tested on both classic CAN and CAN-FD
> at 125 KBPS, 250 KBPS, 1 MBPS and 2.5 MBPS with 4 MBPS bitrate
> switching.
> 
> Letency and CPU load benchmarks were tested on 3x MCAN on AM62x.
> 1 MBPS timer polling interval is the better timer polling interval
> since it has comparable latency to hardware interrupt with the worse
> case being 1ms + CAN frame propagation time and CPU load is not
> substantial. Latency can be improved further with less than 1 ms
> polling intervals, howerver it is at the cost of CPU usage since CPU
> load increases at 0.5 ms.
> 
> Note that in terms of power, enabling MCU MCANs with timer-polling
> implementation might have negative impact since we will have to wake
> up every 1 ms whether there are CAN packets pending in the RX FIFO or
> not. This might prevent the CPU from entering into deeper idle states
> for extended periods of time.
> 
> This patch series depends on 'Enable CAN PHY transceiver driver':
> Link: https://lore.kernel.org/lkml/775ec9ce-7668-429c-a977-6c8995968d6e@app.fastmail.com/T/
> 
> v2:
> Link: https://lore.kernel.org/linux-can/20230424195402.516-1-jm@ti.com/T/#t
> 
> V1:
> Link: https://lore.kernel.org/linux-can/19d8ae7f-7b74-a869-a818-93b74d106709@ti.com/T/#t
> 
> RFC:
> Link: https://lore.kernel.org/linux-can/52a37e51-4143-9017-42ee-8d17c67028e3@ti.com/T/#t
> 
> Changes since v3:
> - Wrong patch sent, resend correct patch series

Sending patchsets every 10 minutes does not help us...

Best regards,
Krzysztof

Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Judith Mendez]

Hello Marc

On 5/2/23 01:37, Marc Kleine-Budde wrote:
> On 01.05.2023 17:46:22, Judith Mendez wrote:
>> Add an hrtimer to MCAN class device. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found and
>> poll-interval property is defined in device tree M_CAN node.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
>> hrtimer callback, we check if there is a transaction pending by
>> reading a register, then process by calling the isr if there is.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
> 
> I think this patch is as good as it gets, given the HW and SW
> limitations of the coprocessor.
> 
> Some minor nitpicks inline. No need to resend from my point of view,
> I'll fixup while applying the patch.

Thanks Marc, really appreciate your feedback and attention.

Same to everyone who helped make these patches better. (:

regards,
Judith

Re: [PATCH v4 1/4] dt-bindings: net: can: Add poll-interval for MCAN

[Rob Herring]

On Mon, May 01, 2023 at 05:46:21PM -0500, Judith Mendez wrote:
> On AM62x SoC, MCANs on MCU domain do not have hardware interrupt
> routed to A53 Linux, instead they will use software interrupt by
> hrtimer. To enable timer method, interrupts should be optional so
> remove interrupts property from required section and introduce
> poll-interval property.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
> Changelog:
> v3:
>  1. Move binding patch to first in series
>  2. Update description for poll-interval
>  3. Add oneOf to specify using interrupts/interrupt-names or poll-interval
>  4. Fix example property: add comment below 'example'
> 
> v2:
>   1. Add poll-interval property to enable timer polling method
>   2. Add example using poll-interval property
>   
>  .../bindings/net/can/bosch,m_can.yaml         | 36 +++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> index 67879aab623b..c024ee49962c 100644
> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> @@ -14,6 +14,13 @@ maintainers:
>  allOf:
>    - $ref: can-controller.yaml#
>  
> +oneOf:
> +  - required:
> +      - interrupts
> +      - interrupt-names
> +  - required:
> +      - poll-interval

Move this next to 'required'.

> +
>  properties:
>    compatible:
>      const: bosch,m_can
> @@ -40,6 +47,14 @@ properties:
>        - const: int1
>      minItems: 1
>  
> +  poll-interval:
> +    $ref: /schemas/types.yaml#/definitions/flag

This is a common property already defined as a uint32. You shouldn't 
define a new type.

A flag doesn't even make sense. If that's all you need, then just enable 
polling if no interrupt is present.

> +    description: Enable hrtimer polling method for an M_CAN device.
> +      If this property is defined in MCAN node, it tells the driver to
> +      enable polling method for an MCAN device. If for an MCAN device,
> +      hardware interrupt is found and hrtimer polling method is enabled,

What's hrtimer? (Don't put Linuxisms in bindings)

> +      the driver will use hardware interrupt method.
> +
>    clocks:
>      items:
>        - description: peripheral clock
> @@ -122,8 +137,6 @@ required:
>    - compatible
>    - reg
>    - reg-names
> -  - interrupts
> -  - interrupt-names
>    - clocks
>    - clock-names
>    - bosch,mram-cfg
> @@ -132,6 +145,7 @@ additionalProperties: false
>  
>  examples:
>    - |
> +    // Example with interrupts
>      #include <dt-bindings/clock/imx6sx-clock.h>
>      can@20e8000 {
>        compatible = "bosch,m_can";
> @@ -149,4 +163,22 @@ examples:
>        };
>      };
>  
> +  - |
> +    // Example with timer polling
> +    #include <dt-bindings/clock/imx6sx-clock.h>
> +    can@20e8000 {
> +      compatible = "bosch,m_can";
> +      reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> +      reg-names = "m_can", "message_ram";
> +      poll-interval;
> +      clocks = <&clks IMX6SX_CLK_CANFD>,
> +               <&clks IMX6SX_CLK_CANFD>;
> +      clock-names = "hclk", "cclk";
> +      bosch,mram-cfg = <0x0 0 0 32 0 0 0 1>;
> +
> +      can-transceiver {
> +        max-bitrate = <5000000>;
> +      };
> +    };
> +
>  ...
> -- 
> 2.17.1
> 

Re: [PATCH v4 1/4] dt-bindings: net: can: Add poll-interval for MCAN

[Marc Kleine-Budde]

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

On 05.05.2023 16:29:48, Rob Herring wrote:
> On Mon, May 01, 2023 at 05:46:21PM -0500, Judith Mendez wrote:
> > On AM62x SoC, MCANs on MCU domain do not have hardware interrupt
> > routed to A53 Linux, instead they will use software interrupt by
> > hrtimer. To enable timer method, interrupts should be optional so
> > remove interrupts property from required section and introduce
> > poll-interval property.
> > 
> > Signed-off-by: Judith Mendez <jm@ti.com>
> > ---
> > Changelog:
> > v3:
> >  1. Move binding patch to first in series
> >  2. Update description for poll-interval
> >  3. Add oneOf to specify using interrupts/interrupt-names or poll-interval
> >  4. Fix example property: add comment below 'example'
> > 
> > v2:
> >   1. Add poll-interval property to enable timer polling method
> >   2. Add example using poll-interval property
> >   
> >  .../bindings/net/can/bosch,m_can.yaml         | 36 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > index 67879aab623b..c024ee49962c 100644
> > --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> > @@ -14,6 +14,13 @@ maintainers:
> >  allOf:
> >    - $ref: can-controller.yaml#
> >  
> > +oneOf:
> > +  - required:
> > +      - interrupts
> > +      - interrupt-names
> > +  - required:
> > +      - poll-interval
> 
> Move this next to 'required'.
> 
> > +
> >  properties:
> >    compatible:
> >      const: bosch,m_can
> > @@ -40,6 +47,14 @@ properties:
> >        - const: int1
> >      minItems: 1
> >  
> > +  poll-interval:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> 
> This is a common property already defined as a uint32. You shouldn't 
> define a new type.
> 
> A flag doesn't even make sense. If that's all you need, then just enable 
> polling if no interrupt is present.

Ok, then it's implicit. No IRQs -> polling.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v4 1/4] dt-bindings: net: can: Add poll-interval for MCAN

[Judith Mendez]

Hello all,

On 5/9/23 07:27, Marc Kleine-Budde wrote:
> On 05.05.2023 16:29:48, Rob Herring wrote:
>> On Mon, May 01, 2023 at 05:46:21PM -0500, Judith Mendez wrote:
>>> On AM62x SoC, MCANs on MCU domain do not have hardware interrupt
>>> routed to A53 Linux, instead they will use software interrupt by
>>> hrtimer. To enable timer method, interrupts should be optional so
>>> remove interrupts property from required section and introduce
>>> poll-interval property.
>>>
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>> Changelog:
>>> v3:
>>>  1. Move binding patch to first in series
>>>  2. Update description for poll-interval
>>>  3. Add oneOf to specify using interrupts/interrupt-names or poll-interval
>>>  4. Fix example property: add comment below 'example'
>>>
>>> v2:
>>>   1. Add poll-interval property to enable timer polling method
>>>   2. Add example using poll-interval property
>>>   
>>>  .../bindings/net/can/bosch,m_can.yaml         | 36 +++++++++++++++++--
>>>  1 file changed, 34 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>>> index 67879aab623b..c024ee49962c 100644
>>> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>>> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>>> @@ -14,6 +14,13 @@ maintainers:
>>>  allOf:
>>>    - $ref: can-controller.yaml#
>>>  
>>> +oneOf:
>>> +  - required:
>>> +      - interrupts
>>> +      - interrupt-names
>>> +  - required:
>>> +      - poll-interval
>>
>> Move this next to 'required'.
>>
>>> +
>>>  properties:
>>>    compatible:
>>>      const: bosch,m_can
>>> @@ -40,6 +47,14 @@ properties:
>>>        - const: int1
>>>      minItems: 1
>>>  
>>> +  poll-interval:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>
>> This is a common property already defined as a uint32. You shouldn't 
>> define a new type.
>>
>> A flag doesn't even make sense. If that's all you need, then just enable 
>> polling if no interrupt is present.
> 
> Ok, then it's implicit. No IRQs -> polling.

Ok, will send out a v5 with these updates. Thanks.

regards,
Judith

Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Judith Mendez]

Hello Marc,

On 5/2/23 01:37, Marc Kleine-Budde wrote:
> On 01.05.2023 17:46:22, Judith Mendez wrote:
>> Add an hrtimer to MCAN class device. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found and
>> poll-interval property is defined in device tree M_CAN node.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
>> hrtimer callback, we check if there is a transaction pending by
>> reading a register, then process by calling the isr if there is.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
> 
> I think this patch is as good as it gets, given the HW and SW
> limitations of the coprocessor.
> 
> Some minor nitpicks inline. No need to resend from my point of view,
> I'll fixup while applying the patch.
> 
> Marc
> 
>> ---
>> Changelog:
>> v1:
>>  1. Sort list of includes
>>  2. Create a define for HR_TIMER_POLL_INTERVAL
>>  3. Fix indentations and style issues/warnings
>>  4. Change polling variable to type bool
>>  5. Change platform_get_irq to optional so not to print error msg
>>  6. Move error check for addr directly after assignment
>>  7. Print appropriate error msg with dev_err_probe insead of dev_dbg
>>
>> v2:
>>  1. Add poll-interval to MCAN class device to check if poll-interval propery is
>>     present in MCAN node, this enables timer polling method
>>  2. Add 'polling' flag to MCAN class device to check if a device is using timer
>>     polling method
>>  3. Check if both timer polling and hardware interrupt are enabled for a MCAN
>>     device, default to hardware interrupt mode if both are enabled
>>  4. Change ms_to_ktime() to ns_to_ktime()
>>  5. Remove newlines, tabs, and restructure if/else section
>>  
>>  drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
>>  drivers/net/can/m_can/m_can.h          |  4 ++++
>>  drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
>>  3 files changed, 60 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index a5003435802b..e1ac0c1d85a3 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/can/dev.h>
>>  #include <linux/ethtool.h>
>> +#include <linux/hrtimer.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/iopoll.h>
>> @@ -308,6 +309,9 @@ enum m_can_reg {
>>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>>  
>> +/* Hrtimer polling interval */
>> +#define HRTIMER_POLL_INTERVAL		1
>> +
>>  /* The ID and DLC registers are adjacent in M_CAN FIFO memory,
>>   * and we can save a (potentially slow) bus round trip by combining
>>   * reads and writes to them.
>> @@ -1587,6 +1591,11 @@ static int m_can_close(struct net_device *dev)
>>  	if (!cdev->is_peripheral)
>>  		napi_disable(&cdev->napi);
>>  
>> +	if (cdev->polling) {
>> +		dev_dbg(cdev->dev, "Disabling the hrtimer\n");
>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>  	m_can_stop(dev);
>>  	m_can_clk_stop(cdev);
>>  	free_irq(dev->irq, dev);
>> @@ -1793,6 +1802,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>  	return NETDEV_TX_OK;
>>  }
>>  
>> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	struct m_can_classdev *cdev = container_of(timer, struct
>> +						   m_can_classdev, hrtimer);
>> +
>> +	m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL));
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>  static int m_can_open(struct net_device *dev)
>>  {
>>  	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1827,13 +1848,17 @@ static int m_can_open(struct net_device *dev)
>>  		}
>>  
>>  		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
>> -
> 
> unrelated change
> 
>>  		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>>  					   IRQF_ONESHOT,
>>  					   dev->name, dev);
>> -	} else {
>> +	} else if (!cdev->polling) {
>>  		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>>  				  dev);
>> +	} else {
>> +		dev_dbg(cdev->dev, "Start hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL),
>> +			      HRTIMER_MODE_REL_PINNED);
>>  	}
>>  
>>  	if (err < 0) {
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..e9db5cce4e68 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/device.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/freezer.h>
>> +#include <linux/hrtimer.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/iopoll.h>
>> @@ -93,6 +94,9 @@ struct m_can_classdev {
>>  	int is_peripheral;
>>  
>>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>> +	bool polling;
>>  };
>>  
>>  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
>> index 9c1dcf838006..0fcb436298f8 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -5,6 +5,7 @@
>>  //
>>  // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
>>  
>> +#include <linux/hrtimer.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>>  
>> @@ -96,12 +97,37 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  		goto probe_fail;
>>  
>>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>> -	irq = platform_get_irq_byname(pdev, "int0");
>> -	if (IS_ERR(addr) || irq < 0) {
>> -		ret = -EINVAL;
>> +	if (IS_ERR(addr)) {
>> +		ret = PTR_ERR(addr);
>>  		goto probe_fail;
>>  	}
>>  
>> +	irq = platform_get_irq_byname_optional(pdev, "int0");
>> +	if (irq == -EPROBE_DEFER) {
>> +		ret = -EPROBE_DEFER;
>> +		goto probe_fail;
>> +	}
>> +
>> +	if (device_property_present(mcan_class->dev, "poll-interval"))
>> +		mcan_class->polling = 1;
> 
> true
> 
>> +
>> +	if (!mcan_class->polling && irq < 0) {
>> +		ret = -ENXIO;
>> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
>> +		goto probe_fail;
>> +	}
>> +
>> +	if (mcan_class->polling) {
>> +		if (irq > 0) {
>> +			mcan_class->polling = 0;
> 
> false
> 
>> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
> 
> "...using hardware IRQ"
> 
> Use dev_info(), as there is something not 100% correct with the DT.

Is it dev_info or dev_dbg? I used to have dev_info since it was nice to see when polling was
enabled. Also, I had seen this print and the next as informative prints, hence the dev_info().
However, I was told in this review process to change to dev_dbg. Which is correct?

>> +		} else {
>> +			dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
>> +			hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC,
>> +				     HRTIMER_MODE_REL_PINNED);
>> +		}
>> +	}
>> +
>>  	/* message ram could be shared */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
>>  	if (!res) {
>> -- 
>> 2.17.1
>>
>>
> 

regards,
Judith

Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 09.05.2023 17:18:06, Judith Mendez wrote:
[...]
> >> +	if (!mcan_class->polling && irq < 0) {
> >> +		ret = -ENXIO;
> >> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
> >> +		goto probe_fail;
> >> +	}
> >> +
> >> +	if (mcan_class->polling) {
> >> +		if (irq > 0) {
> >> +			mcan_class->polling = 0;
> > 
> > false
> > 
> >> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
> > 
> > "...using hardware IRQ"
> > 
> > Use dev_info(), as there is something not 100% correct with the DT.
> 
> Is it dev_info or dev_dbg?

dev_info() - But without an explicit "poll-interval' in the DT, this
code path doesn't exist anymore.

> I used to have dev_info since it was nice to see when polling was
> enabled.

Re-read your code, this is not about enabling polling. This message
handles the case where an IRQ was given _and_ "poll-interval" was
specified. So there is something not 100% correct with the DT (IRQ _and_
polling), but this is obsolete now.

> Also, I had seen this print and the next as informative prints, hence the dev_info().

We don't print messages when IRQs are enabled, so enabling polling
should be a dev_dbg(), too.

> However, I was told in this review process to change to dev_dbg. Which is correct?

Driver works correct -> dev_dbg()
Something is strange -> dev_info()

Hope that helps,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

[Judith Mendez]

Hello Marc,

On 5/10/23 02:21, Marc Kleine-Budde wrote:
> On 09.05.2023 17:18:06, Judith Mendez wrote:
> [...]
>>>> +	if (!mcan_class->polling && irq < 0) {
>>>> +		ret = -ENXIO;
>>>> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
>>>> +		goto probe_fail;
>>>> +	}
>>>> +
>>>> +	if (mcan_class->polling) {
>>>> +		if (irq > 0) {
>>>> +			mcan_class->polling = 0;
>>>
>>> false
>>>
>>>> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
>>>
>>> "...using hardware IRQ"
>>>
>>> Use dev_info(), as there is something not 100% correct with the DT.
>>
>> Is it dev_info or dev_dbg?
> 
> dev_info() - But without an explicit "poll-interval' in the DT, this
> code path doesn't exist anymore.
> 
>> I used to have dev_info since it was nice to see when polling was
>> enabled.
> 
> Re-read your code, this is not about enabling polling. This message
> handles the case where an IRQ was given _and_ "poll-interval" was
> specified. So there is something not 100% correct with the DT (IRQ _and_
> polling), but this is obsolete now.

Sorry, I meant it was nice to see when polling was enabled or not. In

this case, if polling was enabled but hardware IRQ exists, it is good

information to see this print. But I understand now how this is a case

where 'something is strange'.

>> Also, I had seen this print and the next as informative prints, hence the dev_info().
> 
> We don't print messages when IRQs are enabled, so enabling polling
> should be a dev_dbg(), too.
> 
>> However, I was told in this review process to change to dev_dbg. Which is correct?
> 
> Driver works correct -> dev_dbg()
> Something is strange -> dev_info()

Got it, this is very helpful. Thanks.



regards,

Judith