[RFC PATCH 0/5] Enable multiple MCAN on AM62x

[RFC PATCH 0/5] 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 a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.

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':
https://lore.kernel.org/lkml/775ec9ce-7668-429c-a977-6c8995968d6e@app.fastmail.com/T/

Judith Mendez (5):
  arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay
  arm64: defconfig: Enable MCAN driver
  dt-binding: can: m_can: Remove required interrupt attributes
  arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay
  can: m_can: Add hrtimer to generate software interrupt

 .../bindings/net/can/bosch,m_can.yaml         |  2 -
 arch/arm64/boot/dts/ti/Makefile               |  2 +
 .../boot/dts/ti/k3-am625-sk-mcan-main.dtso    | 35 +++++++++
 .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 75 +++++++++++++++++++
 arch/arm64/configs/defconfig                  |  2 +
 drivers/net/can/m_can/m_can.c                 | 24 +++++-
 drivers/net/can/m_can/m_can.h                 |  3 +
 drivers/net/can/m_can/m_can_platform.c        |  9 ++-
 8 files changed, 146 insertions(+), 6 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

-- 
2.17.1

[RFC PATCH 1/5] 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.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 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..72b68fd51121
--- /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 <dt-bindings/pinctrl/k3.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

[RFC PATCH 2/5] arm64: defconfig: Enable MCAN driver

[Judith Mendez]

Enable CAN_M_CAN and CAN_M_CAN_PLATFORM to be built
as modules by default for TI boards.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 7790ee42c68a..172a2523051f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -176,6 +176,8 @@ CONFIG_NET_ACT_GATE=m
 CONFIG_QRTR_SMD=m
 CONFIG_QRTR_TUN=m
 CONFIG_CAN=m
+CONFIG_CAN_M_CAN=m
+CONFIG_CAN_M_CAN_PLATFORM=m
 CONFIG_BT=m
 CONFIG_BT_HIDP=m
 # CONFIG_BT_LE is not set
-- 
2.17.1

[RFC PATCH 3/5] dt-binding: can: m_can: Remove required interrupt attributes

[Judith Mendez]

Remove required attributes for interrupt and interrupt names
since some MCANs may not have hardware interrupt routed to A53
Linux.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 2 --
 1 file changed, 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..43f1aa9addc0 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -122,8 +122,6 @@ required:
   - compatible
   - reg
   - reg-names
-  - interrupts
-  - interrupt-names
   - clocks
   - clock-names
   - bosch,mram-cfg
-- 
2.17.1

[RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Judith Mendez]

Enable two MCAN in MCU domain. AM62x does not have on-board CAN
transcievers, so instead of changing the DTB permanently, add
MCU MCAN nodes and transceiver nodes to a MCU MCAN overlay.

If there are no hardware interrupts rounted to the GIC interrupt
controller for MCAN IP, A53 Linux will not receive hardware
interrupts. If an hrtimer is used to generate software interrupts,
the two required interrupt attributes in the MCAN node do not have
to be included.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 arch/arm64/boot/dts/ti/Makefile               |  2 +-
 .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 75 +++++++++++++++++++
 2 files changed, 76 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-am625-sk-mcan-mcu.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
new file mode 100644
index 000000000000..777705aea546
--- /dev/null
+++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
@@ -0,0 +1,75 @@
+// 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 <dt-bindings/pinctrl/k3.h>
+#include <dt-bindings/soc/ti,sci_pm_domain.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 */
+		>;
+	};
+};
+
+&cbass_mcu {
+	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>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&mcu_mcan1_pins_default>;
+		phys = <&transceiver2>;
+		status = "okay";
+	};
+
+	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>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&mcu_mcan2_pins_default>;
+		phys = <&transceiver3>;
+		status = "okay";
+	};
+};
-- 
2.17.1

[RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Judith Mendez]

Add a hrtimer to MCAN struct. Each MCAN will have its own
hrtimer instantiated if there is no hardware interrupt found.

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>
---
 drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
 drivers/net/can/m_can/m_can.h          |  3 +++
 drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 8e83d6963d85..bb9d53f4d3cc 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -23,6 +23,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/hrtimer.h>
 
 #include "m_can.h"
 
@@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
 	if (!cdev->is_peripheral)
 		napi_disable(&cdev->napi);
 
+	if (dev->irq < 0) {
+		dev_info(cdev->dev, "Disabling the hrtimer\n");
+		hrtimer_cancel(&cdev->hrtimer);
+	}
+
 	m_can_stop(dev);
 	m_can_clk_stop(cdev);
 	free_irq(dev->irq, dev);
@@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
+{
+	irqreturn_t ret;
+	struct m_can_classdev *cdev =
+		container_of(timer, struct m_can_classdev, hrtimer);
+
+	ret = m_can_isr(0, cdev->net);
+
+	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));
+
+	return HRTIMER_RESTART;
+}
+
 static int m_can_open(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
 	}
 
 	if (err < 0) {
-		netdev_err(dev, "failed to request interrupt\n");
-		goto exit_irq_fail;
+		dev_info(cdev->dev, "Enabling the hrtimer\n");
+		cdev->hrtimer.function = &hrtimer_callback;
+		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
 	}
 
 	/* start the m_can controller */
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index a839dc71dc9b..ed046d77fdb9 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -28,6 +28,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/hrtimer.h>
 
 /* m_can lec values */
 enum m_can_lec_type {
@@ -93,6 +94,8 @@ struct m_can_classdev {
 	int is_peripheral;
 
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
+
+	struct hrtimer hrtimer;
 };
 
 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..53e1648e9dab 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -7,6 +7,7 @@
 
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
 
 #include "m_can.h"
 
@@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
 	irq = platform_get_irq_byname(pdev, "int0");
 	if (IS_ERR(addr) || irq < 0) {
-		ret = -EINVAL;
-		goto probe_fail;
+		if (irq == -EPROBE_DEFER) {
+			ret = -EPROBE_DEFER;
+			goto probe_fail;
+		}
+		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
+		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
 	}
 
 	/* message ram could be shared */
-- 
2.17.1

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Vignesh Raghavendra]

Hi Judith,

On 14/04/23 04:00, Judith Mendez wrote:
> Judith Mendez (5):
>   arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay
>   arm64: defconfig: Enable MCAN driver
>   dt-binding: can: m_can: Remove required interrupt attributes
>   arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay
>   can: m_can: Add hrtimer to generate software interrupt

This is fine for RFC, but next time, please split DT and defconfig
changes (1/5,2/5, and 4/5) to separate series as they have to go via
arm64 tree.

-- 
Regards
Vignesh

Re: [RFC PATCH 3/5] dt-binding: can: m_can: Remove required interrupt attributes

[Krzysztof Kozlowski]

On 14/04/2023 00:30, Judith Mendez wrote:
> Remove required attributes for interrupt and interrupt names
> since some MCANs may not have hardware interrupt routed to A53

Like which? Can you give specific model names?

> Linux.
> 

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

It's dt-bindings:


> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 2 --
>  1 file changed, 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..43f1aa9addc0 100644
> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> @@ -122,8 +122,6 @@ required:
>    - compatible
>    - reg
>    - reg-names
> -  - interrupts
> -  - interrupt-names
>    - clocks
>    - clock-names
>    - bosch,mram-cfg

Best regards,
Krzysztof

Re: [RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Krzysztof Kozlowski]

On 14/04/2023 00:30, Judith Mendez wrote:
> Enable two MCAN in MCU domain. AM62x does not have on-board CAN
> transcievers, so instead of changing the DTB permanently, add
> MCU MCAN nodes and transceiver nodes to a MCU MCAN overlay.
> 
> If there are no hardware interrupts rounted to the GIC interrupt
> controller for MCAN IP, A53 Linux will not receive hardware
> interrupts. If an hrtimer is used to generate software interrupts,
> the two required interrupt attributes in the MCAN node do not have
> to be included.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  arch/arm64/boot/dts/ti/Makefile               |  2 +-
>  .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 75 +++++++++++++++++++
>  2 files changed, 76 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-am625-sk-mcan-mcu.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
> new file mode 100644
> index 000000000000..777705aea546
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
> @@ -0,0 +1,75 @@
> +// 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 <dt-bindings/pinctrl/k3.h>
> +#include <dt-bindings/soc/ti,sci_pm_domain.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 */
> +		>;
> +	};
> +};
> +
> +&cbass_mcu {
> +	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>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&mcu_mcan1_pins_default>;
> +		phys = <&transceiver2>;
> +		status = "okay";

okay is by default. Why do you need it?



Best regards,
Krzysztof

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Marc Kleine-Budde]

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

On 13.04.2023 17:30: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.

Is this a general hardware limitation, that effects all MCU domain
peripherals? Is there a mailbox mechanism between the MCU and the MAIN
domain, would it be possible to pass the IRQ with a small firmware on
the MCU? Anyways, that's future optimization.

> This solution instantiates a hrtimer with 1 ms polling interval
> for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.

Some Linux input drivers have the property poll-interval, would it make
sense to ass this here too?

> 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':
> https://lore.kernel.org/lkml/775ec9ce-7668-429c-a977-6c8995968d6e@app.fastmail.com/T/

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: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 13.04.2023 17:30:51, Judith Mendez wrote:
> Add a hrtimer to MCAN struct. Each MCAN will have its own
> hrtimer instantiated if there is no hardware interrupt found.
> 
> The hrtimer will generate a software interrupt every 1 ms. In

Are you sure about the 1ms?

> 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>
> ---
>  drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>  drivers/net/can/m_can/m_can.h          |  3 +++
>  drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 8e83d6963d85..bb9d53f4d3cc 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -23,6 +23,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/hrtimer.h>
>  
>  #include "m_can.h"
>  
> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>  	if (!cdev->is_peripheral)
>  		napi_disable(&cdev->napi);
>  
> +	if (dev->irq < 0) {
> +		dev_info(cdev->dev, "Disabling the hrtimer\n");

Make it a dev_dbg() or remove completely.

> +		hrtimer_cancel(&cdev->hrtimer);
> +	}
> +
>  	m_can_stop(dev);
>  	m_can_clk_stop(cdev);
>  	free_irq(dev->irq, dev);
> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	irqreturn_t ret;

never read value?

> +	struct m_can_classdev *cdev =
> +		container_of(timer, struct m_can_classdev, hrtimer);
> +
> +	ret = m_can_isr(0, cdev->net);
> +
> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));

There's ms_to_ktime()....and the "5" doesn't match your patch
description.

> +
> +	return HRTIMER_RESTART;
> +}
> +
>  static int m_can_open(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>  	}
>  
>  	if (err < 0) {
> -		netdev_err(dev, "failed to request interrupt\n");
> -		goto exit_irq_fail;
> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
> +		cdev->hrtimer.function = &hrtimer_callback;
> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);

IMHO it makes no sense to request an IRQ if the device doesn't have one,
and then try to fix up things in the error path. What about this?

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
                 err = request_threaded_irq(dev->irq, NULL, m_can_isr,
                                            IRQF_ONESHOT,
                                            dev->name, dev);
-        } else {
+        } else if (dev->irq) {
                 err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
                                   dev);
+        } else {
+                // polling
         }
 
         if (err < 0) {

>  	}
>  
>  	/* start the m_can controller */
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index a839dc71dc9b..ed046d77fdb9 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/hrtimer.h>
>  
>  /* m_can lec values */
>  enum m_can_lec_type {
> @@ -93,6 +94,8 @@ struct m_can_classdev {
>  	int is_peripheral;
>  
>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
> +
> +	struct hrtimer hrtimer;
>  };
>  
>  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..53e1648e9dab 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
>  
>  #include "m_can.h"
>  
> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>  	irq = platform_get_irq_byname(pdev, "int0");
>  	if (IS_ERR(addr) || irq < 0) {

What about the IS_ERR(addr) case?

> -		ret = -EINVAL;
> -		goto probe_fail;
> +		if (irq == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto probe_fail;
> +		}
> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);

I don't like it when polling is unconditionally set up in case of an irq
error. I'm not sure if we need an explicit device tree property....

>  	}
>  
>  	/* message ram could be shared */
> -- 
> 2.17.1
> 
>

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: [RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Nishanth Menon]

On 10:01-20230414, Krzysztof Kozlowski wrote:
> On 14/04/2023 00:30, Judith Mendez wrote:
> > Enable two MCAN in MCU domain. AM62x does not have on-board CAN
> > transcievers, so instead of changing the DTB permanently, add
> > MCU MCAN nodes and transceiver nodes to a MCU MCAN overlay.
> > 
> > If there are no hardware interrupts rounted to the GIC interrupt
> > controller for MCAN IP, A53 Linux will not receive hardware
> > interrupts. If an hrtimer is used to generate software interrupts,
> > the two required interrupt attributes in the MCAN node do not have
> > to be included.
> > 
> > Signed-off-by: Judith Mendez <jm@ti.com>
> > ---
> >  arch/arm64/boot/dts/ti/Makefile               |  2 +-
> >  .../boot/dts/ti/k3-am625-sk-mcan-mcu.dtso     | 75 +++++++++++++++++++
> >  2 files changed, 76 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-am625-sk-mcan-mcu.dtso b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
> > new file mode 100644
> > index 000000000000..777705aea546
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-am625-sk-mcan-mcu.dtso
> > @@ -0,0 +1,75 @@
> > +// 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 <dt-bindings/pinctrl/k3.h>
> > +#include <dt-bindings/soc/ti,sci_pm_domain.h>

NAK.

> > +
> > +
> > +&{/} {
> > +	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 */
> > +		>;
> > +	};
> > +};
> > +
> > +&cbass_mcu {
> > +	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>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&mcu_mcan1_pins_default>;
> > +		phys = <&transceiver2>;
> > +		status = "okay";
> 
> okay is by default. Why do you need it?

mcan is not functional without pinmux, so it has been disabled by
default in SoC. this overlay is supposed to enable it. But this is done
entirely wrongly.


The mcu_mcan1 should first be added to the SoC.dtsi as disabled, then
set to okay with pinctrl and  transciever in the overlay.

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

Re: [RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Krzysztof Kozlowski]

On 14/04/2023 20:29, Nishanth Menon wrote:
>>> +
>>> +&cbass_mcu {
>>> +	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>;
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&mcu_mcan1_pins_default>;
>>> +		phys = <&transceiver2>;
>>> +		status = "okay";
>>
>> okay is by default. Why do you need it?
> 
> mcan is not functional without pinmux, so it has been disabled by
> default in SoC. this overlay is supposed to enable it. But this is done
> entirely wrongly.

Ah, so this is override of existing node? Why not overriding by
label/phandle?

Best regards,
Krzysztof

Re: [RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Nishanth Menon]

On 22:44-20230414, Krzysztof Kozlowski wrote:
> On 14/04/2023 20:29, Nishanth Menon wrote:
> >>> +
> >>> +&cbass_mcu {
> >>> +	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>;
> >>> +		pinctrl-names = "default";
> >>> +		pinctrl-0 = <&mcu_mcan1_pins_default>;
> >>> +		phys = <&transceiver2>;
> >>> +		status = "okay";
> >>
> >> okay is by default. Why do you need it?
> > 
> > mcan is not functional without pinmux, so it has been disabled by
> > default in SoC. this overlay is supposed to enable it. But this is done
> > entirely wrongly.
> 
> Ah, so this is override of existing node? Why not overriding by
> label/phandle?

Yep, that is how it should be done (as every other node is done for
mcan):
a) SoC.dtsi -> introduce mcu_mcan1, disabled since no transciever or
pinmux, set status = "disabled";
b) overlay -> use the label and provide the missing properties, set
status = "okay";

The series definitely needs a respin.

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

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Oliver Hartkopp]

On 4/14/23 20:20, Marc Kleine-Budde wrote:
> On 13.04.2023 17:30:51, Judith Mendez wrote:
>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
> 
> Are you sure about the 1ms?

The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC 
= 0 and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => 
~50 usecs

So it should be something about

     50 usecs * (FIFO queue len - 2)

if there is some FIFO involved, right?

Best regards,
Oliver

>> 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>
>> ---
>>   drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>>   drivers/net/can/m_can/m_can.h          |  3 +++
>>   drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>>   3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index 8e83d6963d85..bb9d53f4d3cc 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>>   	if (!cdev->is_peripheral)
>>   		napi_disable(&cdev->napi);
>>   
>> +	if (dev->irq < 0) {
>> +		dev_info(cdev->dev, "Disabling the hrtimer\n");
> 
> Make it a dev_dbg() or remove completely.
> 
>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>   	m_can_stop(dev);
>>   	m_can_clk_stop(cdev);
>>   	free_irq(dev->irq, dev);
>> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>   	return NETDEV_TX_OK;
>>   }
>>   
>> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	irqreturn_t ret;
> 
> never read value?
> 
>> +	struct m_can_classdev *cdev =
>> +		container_of(timer, struct m_can_classdev, hrtimer);
>> +
>> +	ret = m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));
> 
> There's ms_to_ktime()....and the "5" doesn't match your patch
> description.
> 
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>   static int m_can_open(struct net_device *dev)
>>   {
>>   	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>>   	}
>>   
>>   	if (err < 0) {
>> -		netdev_err(dev, "failed to request interrupt\n");
>> -		goto exit_irq_fail;
>> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
> 
> IMHO it makes no sense to request an IRQ if the device doesn't have one,
> and then try to fix up things in the error path. What about this?
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
>                   err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>                                              IRQF_ONESHOT,
>                                              dev->name, dev);
> -        } else {
> +        } else if (dev->irq) {
>                   err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>                                     dev);
> +        } else {
> +                // polling
>           }
>   
>           if (err < 0) {
> 
>>   	}
>>   
>>   	/* start the m_can controller */
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..ed046d77fdb9 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -28,6 +28,7 @@
>>   #include <linux/pm_runtime.h>
>>   #include <linux/slab.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/hrtimer.h>
>>   
>>   /* m_can lec values */
>>   enum m_can_lec_type {
>> @@ -93,6 +94,8 @@ struct m_can_classdev {
>>   	int is_peripheral;
>>   
>>   	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>>   };
>>   
>>   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..53e1648e9dab 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>   	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>>   	irq = platform_get_irq_byname(pdev, "int0");
>>   	if (IS_ERR(addr) || irq < 0) {
> 
> What about the IS_ERR(addr) case?
> 
>> -		ret = -EINVAL;
>> -		goto probe_fail;
>> +		if (irq == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto probe_fail;
>> +		}
>> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
>> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> 
> I don't like it when polling is unconditionally set up in case of an irq
> error. I'm not sure if we need an explicit device tree property....
> 
>>   	}
>>   
>>   	/* message ram could be shared */
>> -- 
>> 2.17.1
>>
>>
> 
> Marc
> 

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 16.04.2023 14:33:11, Oliver Hartkopp wrote:
> 
> 
> On 4/14/23 20:20, Marc Kleine-Budde wrote:
> > On 13.04.2023 17:30:51, Judith Mendez wrote:
> > > Add a hrtimer to MCAN struct. Each MCAN will have its own
> > > hrtimer instantiated if there is no hardware interrupt found.
> > > 
> > > The hrtimer will generate a software interrupt every 1 ms. In
> > 
> > Are you sure about the 1ms?

I had the 5ms that are actually used in the code in mind. But this is a
good calculation.

> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> usecs
> 
> So it should be something about
> 
>     50 usecs * (FIFO queue len - 2)

Where does the "2" come from?

> if there is some FIFO involved, right?

Yes, the mcan core has a FIFO. In the current driver the FIFO
configuration is done via device tree and fixed after that. And I don't
know the size of the available RAM in the mcan IP core on that TI SoC.

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: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Oliver Hartkopp]

On 16.04.23 17:35, Marc Kleine-Budde wrote:
> On 16.04.2023 14:33:11, Oliver Hartkopp wrote:
>>
>>
>> On 4/14/23 20:20, Marc Kleine-Budde wrote:
>>> On 13.04.2023 17:30:51, Judith Mendez wrote:
>>>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>>>> hrtimer instantiated if there is no hardware interrupt found.
>>>>
>>>> The hrtimer will generate a software interrupt every 1 ms. In
>>>
>>> Are you sure about the 1ms?
> 
> I had the 5ms that are actually used in the code in mind. But this is a
> good calculation.

@Judith: Can you acknowledge the value calculation?

>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>> usecs
>>
>> So it should be something about
>>
>>      50 usecs * (FIFO queue len - 2)
> 
> Where does the "2" come from?

I thought about handling the FIFO earlier than it gets completely "full".

The fetching routine would need some time too and the hrtimer could also 
jitter to some extend.

>> if there is some FIFO involved, right?
> 
> Yes, the mcan core has a FIFO. In the current driver the FIFO
> configuration is done via device tree and fixed after that. And I don't
> know the size of the available RAM in the mcan IP core on that TI SoC.
> 
> Marc
> 

Best regards,
Oliver

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
> > I had the 5ms that are actually used in the code in mind. But this is a
> > good calculation.
> 
> @Judith: Can you acknowledge the value calculation?
> 
> > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > usecs
> > > 
> > > So it should be something about
> > > 
> > >      50 usecs * (FIFO queue len - 2)
> > 
> > Where does the "2" come from?
> 
> I thought about handling the FIFO earlier than it gets completely "full".
> 
> The fetching routine would need some time too and the hrtimer could also
> jitter to some extend.

I was assuming something like this.

I would argue that the polling time should be:

    50 µs * FIFO length - IRQ overhead.

The max IRQ overhead depends on your SoC and kernel configuration.

regards,
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: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Oliver Hartkopp]

On 17.04.23 09:26, Marc Kleine-Budde wrote:
> On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
>>> I had the 5ms that are actually used in the code in mind. But this is a
>>> good calculation.
>>
>> @Judith: Can you acknowledge the value calculation?
>>
>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>> usecs
>>>>
>>>> So it should be something about
>>>>
>>>>       50 usecs * (FIFO queue len - 2)
>>>
>>> Where does the "2" come from?
>>
>> I thought about handling the FIFO earlier than it gets completely "full".
>>
>> The fetching routine would need some time too and the hrtimer could also
>> jitter to some extend.
> 
> I was assuming something like this.
> 
> I would argue that the polling time should be:
> 
>      50 µs * FIFO length - IRQ overhead.
> 
> The max IRQ overhead depends on your SoC and kernel configuration.

I just tried an educated guess to prevent the FIFO to be filled up 
completely. How can you estimate the "IRQ overhead"? And how do you 
catch the CAN frames that are received while the IRQ is handled?

Best regards,
Oliver

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 17.04.2023 19:34:03, Oliver Hartkopp wrote:
> On 17.04.23 09:26, Marc Kleine-Budde wrote:
> > On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
> > > > I had the 5ms that are actually used in the code in mind. But this is a
> > > > good calculation.
> > > 
> > > @Judith: Can you acknowledge the value calculation?
> > > 
> > > > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > > > usecs
> > > > > 
> > > > > So it should be something about
> > > > > 
> > > > >       50 usecs * (FIFO queue len - 2)
> > > > 
> > > > Where does the "2" come from?
> > > 
> > > I thought about handling the FIFO earlier than it gets completely "full".
> > > 
> > > The fetching routine would need some time too and the hrtimer could also
> > > jitter to some extend.
> > 
> > I was assuming something like this.
> > 
> > I would argue that the polling time should be:
> > 
> >      50 µs * FIFO length - IRQ overhead.
> > 
> > The max IRQ overhead depends on your SoC and kernel configuration.
> 
> I just tried an educated guess to prevent the FIFO to be filled up
> completely. How can you estimate the "IRQ overhead"? And how do you catch
> the CAN frames that are received while the IRQ is handled?

We're talking about polling, better call it "overhead" or "latency from
timer expiration until FIFO has at least one frame room". This value
depends on your system.

It depends on many, many factors, SoC, Kernel configuration (preempt RT,
powersaving, frequency scaling, system load. In your example it's 100
µs. I wanted to say there's an overhead (or latency) and we need enough
space in the FIFO, to cover it.

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: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Mendez, Judith]

Hello Marc,

On 4/14/2023 12:49 PM, Marc Kleine-Budde wrote:
> On 13.04.2023 17:30: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.
> 
> Is this a general hardware limitation, that effects all MCU domain
> peripherals? Is there a mailbox mechanism between the MCU and the MAIN
> domain, would it be possible to pass the IRQ with a small firmware on
> the MCU? Anyways, that's future optimization.
> 

This is a hardware limitation that affects AM62x SoC and has been 
carried over to at least 1 other SoC. Using the MCU is an idea that we 
have juggled around for a while, we will definitely keep it in mind for 
future optimization. Thanks for your feedback.

>> This solution instantiates a hrtimer with 1 ms polling interval
>> for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.
> 
> Some Linux input drivers have the property poll-interval, would it make
> sense to ass this here too?
> 
>> 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':
>> https://lore.kernel.org/lkml/775ec9ce-7668-429c-a977-6c8995968d6e@app.fastmail.com/T/
> 
> Marc
> 

regards,
Judith

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Mendez, Judith]

Hello Marc,

On 4/17/2023 2:26 PM, Marc Kleine-Budde wrote:
> On 17.04.2023 19:34:03, Oliver Hartkopp wrote:
>> On 17.04.23 09:26, Marc Kleine-Budde wrote:
>>> On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
>>>>> I had the 5ms that are actually used in the code in mind. But this is a
>>>>> good calculation.
>>>>
>>>> @Judith: Can you acknowledge the value calculation?
>>>>
>>>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>>>> usecs
>>>>>>
>>>>>> So it should be something about
>>>>>>
>>>>>>        50 usecs * (FIFO queue len - 2)
>>>>>
>>>>> Where does the "2" come from?
>>>>
>>>> I thought about handling the FIFO earlier than it gets completely "full".
>>>>
>>>> The fetching routine would need some time too and the hrtimer could also
>>>> jitter to some extend.
>>>
>>> I was assuming something like this.
>>>
>>> I would argue that the polling time should be:
>>>
>>>       50 µs * FIFO length - IRQ overhead.
>>>
>>> The max IRQ overhead depends on your SoC and kernel configuration.
>>
>> I just tried an educated guess to prevent the FIFO to be filled up
>> completely. How can you estimate the "IRQ overhead"? And how do you catch
>> the CAN frames that are received while the IRQ is handled?
> 
> We're talking about polling, better call it "overhead" or "latency from
> timer expiration until FIFO has at least one frame room". This value
> depends on your system.
> 
> It depends on many, many factors, SoC, Kernel configuration (preempt RT,
> powersaving, frequency scaling, system load. In your example it's 100
> µs. I wanted to say there's an overhead (or latency) and we need enough
> space in the FIFO, to cover it.
> 

I am not sure how to estimate IRQ overhead, but FIFO length should be 64
elements.

50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.

Running a few benchmarks showed that using 0.5 ms timer polling interval 
starts to take a toll on CPU load, that is why I chose 1 ms polling 
interval.

regards,
Judith

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Marc Kleine-Budde]

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

On 18.04.2023 11:15:35, Mendez, Judith wrote:
> Hello Marc,
> 
> On 4/14/2023 12:49 PM, Marc Kleine-Budde wrote:
> > On 13.04.2023 17:30: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.
> > 
> > Is this a general hardware limitation, that effects all MCU domain
> > peripherals? Is there a mailbox mechanism between the MCU and the MAIN
> > domain, would it be possible to pass the IRQ with a small firmware on
> > the MCU? Anyways, that's future optimization.
> 
> This is a hardware limitation that affects AM62x SoC and has been carried
> over to at least 1 other SoC. Using the MCU is an idea that we have juggled
> around for a while, we will definitely keep it in mind for future
> optimization. Thanks for your feedback.

Once you have a proper IRQ de-multiplexer, you can integrate it into the
system with a DT change only. No need for changes in the m_can driver.

> > > This solution instantiates a hrtimer with 1 ms polling interval
> > > for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.

Have you seen my suggestion of the poll-interval?

Some Linux input drivers have the property poll-interval, would it make
sense to ass this here too?

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: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Marc Kleine-Budde]

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

On 18.04.2023 15:59:57, Mendez, Judith wrote:
> > > > > > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > > > > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > > > > > usecs
> > > > > > > 
> > > > > > > So it should be something about
> > > > > > > 
> > > > > > >        50 usecs * (FIFO queue len - 2)
> > > > > > 
> > > > > > Where does the "2" come from?
> > > > > 
> > > > > I thought about handling the FIFO earlier than it gets completely "full".
> > > > > 
> > > > > The fetching routine would need some time too and the hrtimer could also
> > > > > jitter to some extend.
> > > > 
> > > > I was assuming something like this.
> > > > 
> > > > I would argue that the polling time should be:
> > > > 
> > > >       50 µs * FIFO length - IRQ overhead.
> > > > 
> > > > The max IRQ overhead depends on your SoC and kernel configuration.
> > > 
> > > I just tried an educated guess to prevent the FIFO to be filled up
> > > completely. How can you estimate the "IRQ overhead"? And how do you catch
> > > the CAN frames that are received while the IRQ is handled?
> > 
> > We're talking about polling, better call it "overhead" or "latency from
> > timer expiration until FIFO has at least one frame room". This value
> > depends on your system.
> > 
> > It depends on many, many factors, SoC, Kernel configuration (preempt RT,
> > powersaving, frequency scaling, system load. In your example it's 100
> > µs. I wanted to say there's an overhead (or latency) and we need enough
> > space in the FIFO, to cover it.
> > 
> 
> I am not sure how to estimate IRQ overhead, but FIFO length should be 64
> elements.

Ok

> 50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.

Sounds good.

> Running a few benchmarks showed that using 0.5 ms timer polling interval
> starts to take a toll on CPU load, that is why I chose 1 ms polling
> interval.

However in the code you use 5 ms.

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: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Mendez, Judith]

Hello Marc,

On 4/19/2023 1:13 AM, Marc Kleine-Budde wrote:
> On 18.04.2023 15:59:57, Mendez, Judith wrote:
>>>>>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>>>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>>>>>> usecs
>>>>>>>>
>>>>>>>> So it should be something about
>>>>>>>>
>>>>>>>>         50 usecs * (FIFO queue len - 2)
>>>>>>>
>>>>>>> Where does the "2" come from?
>>>>>>
>>>>>> I thought about handling the FIFO earlier than it gets completely "full".
>>>>>>
>>>>>> The fetching routine would need some time too and the hrtimer could also
>>>>>> jitter to some extend.
>>>>>
>>>>> I was assuming something like this.
>>>>>
>>>>> I would argue that the polling time should be:
>>>>>
>>>>>        50 µs * FIFO length - IRQ overhead.
>>>>>
>>>>> The max IRQ overhead depends on your SoC and kernel configuration.
>>>>
>>>> I just tried an educated guess to prevent the FIFO to be filled up
>>>> completely. How can you estimate the "IRQ overhead"? And how do you catch
>>>> the CAN frames that are received while the IRQ is handled?
>>>
>>> We're talking about polling, better call it "overhead" or "latency from
>>> timer expiration until FIFO has at least one frame room". This value
>>> depends on your system.
>>>
>>> It depends on many, many factors, SoC, Kernel configuration (preempt RT,
>>> powersaving, frequency scaling, system load. In your example it's 100
>>> µs. I wanted to say there's an overhead (or latency) and we need enough
>>> space in the FIFO, to cover it.
>>>
>>
>> I am not sure how to estimate IRQ overhead, but FIFO length should be 64
>> elements.
> 
> Ok
> 
>> 50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.
> 
> Sounds good.
> 
>> Running a few benchmarks showed that using 0.5 ms timer polling interval
>> starts to take a toll on CPU load, that is why I chose 1 ms polling
>> interval.
> 
> However in the code you use 5 ms.

Yes, it was a mistake, will send out a respin with the correct value, 
thanks.

regards,
Judith

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Mendez, Judith]

Hello Vignesh,

On 4/14/2023 1:12 AM, Vignesh Raghavendra wrote:
> Hi Judith,
> 
> On 14/04/23 04:00, Judith Mendez wrote:
>> Judith Mendez (5):
>>    arm64: dts: ti: Add AM62x MCAN MAIN domain transceiver overlay
>>    arm64: defconfig: Enable MCAN driver
>>    dt-binding: can: m_can: Remove required interrupt attributes
>>    arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay
>>    can: m_can: Add hrtimer to generate software interrupt
> 
> This is fine for RFC, but next time, please split DT and defconfig
> changes (1/5,2/5, and 4/5) to separate series as they have to go via
> arm64 tree.

Thanks, will do in the next respin.

Re: [RFC PATCH 4/5] arm64: dts: ti: Enable multiple MCAN for AM62x in MCU MCAN overlay

[Mendez, Judith]

Hello, all

On 4/14/2023 5:11 PM, Nishanth Menon wrote:
> On 22:44-20230414, Krzysztof Kozlowski wrote:
>> On 14/04/2023 20:29, Nishanth Menon wrote:
>>>>> +
>>>>> +&cbass_mcu {
>>>>> +	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>;
>>>>> +		pinctrl-names = "default";
>>>>> +		pinctrl-0 = <&mcu_mcan1_pins_default>;
>>>>> +		phys = <&transceiver2>;
>>>>> +		status = "okay";
>>>>
>>>> okay is by default. Why do you need it?
>>>
>>> mcan is not functional without pinmux, so it has been disabled by
>>> default in SoC. this overlay is supposed to enable it. But this is done
>>> entirely wrongly.
>>
>> Ah, so this is override of existing node? Why not overriding by
>> label/phandle?
> 
> Yep, that is how it should be done (as every other node is done for
> mcan):
> a) SoC.dtsi -> introduce mcu_mcan1, disabled since no transciever or
> pinmux, set status = "disabled";
> b) overlay -> use the label and provide the missing properties, set
> status = "okay";
> 
> The series definitely needs a respin.
> 

Thanks for your feedback, I will definitely fix and send out a v2 with 
this update.

Thanks,
Judith

Re: [RFC PATCH 5/5] can: m_can: Add hrtimer to generate software interrupt

[Mendez, Judith]

Hello Marc,

On 4/14/2023 1:20 PM, Marc Kleine-Budde wrote:
> On 13.04.2023 17:30:51, Judith Mendez wrote:
>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
> 
> Are you sure about the 1ms?
> 
>> 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>
>> ---
>>   drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>>   drivers/net/can/m_can/m_can.h          |  3 +++
>>   drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>>   3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index 8e83d6963d85..bb9d53f4d3cc 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>>   	if (!cdev->is_peripheral)
>>   		napi_disable(&cdev->napi);
>>   
>> +	if (dev->irq < 0) {
>> +		dev_info(cdev->dev, "Disabling the hrtimer\n");
> 
> Make it a dev_dbg() or remove completely.
> 

Will do, thanks.

>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>   	m_can_stop(dev);
>>   	m_can_clk_stop(cdev);
>>   	free_irq(dev->irq, dev);
>> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>   	return NETDEV_TX_OK;
>>   }
>>   
>> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	irqreturn_t ret;
> 
> never read value?
> 

I have removed ret completely now.

>> +	struct m_can_classdev *cdev =
>> +		container_of(timer, struct m_can_classdev, hrtimer);
>> +
>> +	ret = m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));
> 
> There's ms_to_ktime()....and the "5" doesn't match your patch
> description.
> 
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>   static int m_can_open(struct net_device *dev)
>>   {
>>   	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>>   	}
>>   
>>   	if (err < 0) {
>> -		netdev_err(dev, "failed to request interrupt\n");
>> -		goto exit_irq_fail;
>> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
> 
> IMHO it makes no sense to request an IRQ if the device doesn't have one,
> and then try to fix up things in the error path. What about this?
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
>                   err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>                                              IRQF_ONESHOT,
>                                              dev->name, dev);
> -        } else {
> +        } else if (dev->irq) {
>                   err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>                                     dev);
> +        } else {
> +                // polling
>           }
>   
>           if (err < 0) {
> 
>>   	}

Thanks for the recommendation, I will include in v2.

>>   
>>   	/* start the m_can controller */
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..ed046d77fdb9 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -28,6 +28,7 @@
>>   #include <linux/pm_runtime.h>
>>   #include <linux/slab.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/hrtimer.h>
>>   
>>   /* m_can lec values */
>>   enum m_can_lec_type {
>> @@ -93,6 +94,8 @@ struct m_can_classdev {
>>   	int is_peripheral;
>>   
>>   	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>>   };
>>   
>>   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..53e1648e9dab 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>   	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>>   	irq = platform_get_irq_byname(pdev, "int0");
>>   	if (IS_ERR(addr) || irq < 0) {
> 
> What about the IS_ERR(addr) case?
> 

Added, thanks.

>> -		ret = -EINVAL;
>> -		goto probe_fail;
>> +		if (irq == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto probe_fail;
>> +		}
>> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
>> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> 
> I don't like it when polling is unconditionally set up in case of an irq
> error. I'm not sure if we need an explicit device tree property....

This is an interesting thought, I would definitely like to look more 
into this. Though I am thinking it would be part of future optimization 
patch. Thanks so much for your recommendation.

regards,
Judith

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Mendez, Judith]

Hello Marc,

On 4/19/2023 1:10 AM, Marc Kleine-Budde wrote:
> On 18.04.2023 11:15:35, Mendez, Judith wrote:
>> Hello Marc,
>>
>> On 4/14/2023 12:49 PM, Marc Kleine-Budde wrote:
>>> On 13.04.2023 17:30: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.
>>>
>>> Is this a general hardware limitation, that effects all MCU domain
>>> peripherals? Is there a mailbox mechanism between the MCU and the MAIN
>>> domain, would it be possible to pass the IRQ with a small firmware on
>>> the MCU? Anyways, that's future optimization.
>>
>> This is a hardware limitation that affects AM62x SoC and has been carried
>> over to at least 1 other SoC. Using the MCU is an idea that we have juggled
>> around for a while, we will definitely keep it in mind for future
>> optimization. Thanks for your feedback.
> 
> Once you have a proper IRQ de-multiplexer, you can integrate it into the
> system with a DT change only. No need for changes in the m_can driver.
> 

Is this a recommendation for the current patch?

The reason I am asking is because adding firmware for the M4 to forward
a mailbox with the IRQ to the A53 sounds like a good idea and we have 
been juggling the idea, but it is not an ideal solution if customers are
using the M4 for other purposes like safety.

>>>> This solution instantiates a hrtimer with 1 ms polling interval
>>>> for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.
> 
> Have you seen my suggestion of the poll-interval?
> 
> Some Linux input drivers have the property poll-interval, would it make
> sense to ass this here too?

Looking at some examples, I do think we could implement this 
poll-interval attribute, then read in the driver and initialize the 
hrtimer based on this. I like the idea to submit as a future 
optimization patch, thanks!

regards,
Judith

Re: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Marc Kleine-Budde]

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

On 19.04.2023 15:40:24, Mendez, Judith wrote:
> Hello Marc,
> 
> On 4/19/2023 1:10 AM, Marc Kleine-Budde wrote:
> > On 18.04.2023 11:15:35, Mendez, Judith wrote:
> > > Hello Marc,
> > > 
> > > On 4/14/2023 12:49 PM, Marc Kleine-Budde wrote:
> > > > On 13.04.2023 17:30: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.
> > > > 
> > > > Is this a general hardware limitation, that effects all MCU domain
> > > > peripherals? Is there a mailbox mechanism between the MCU and the MAIN
> > > > domain, would it be possible to pass the IRQ with a small firmware on
> > > > the MCU? Anyways, that's future optimization.
> > > 
> > > This is a hardware limitation that affects AM62x SoC and has been carried
> > > over to at least 1 other SoC. Using the MCU is an idea that we have juggled
> > > around for a while, we will definitely keep it in mind for future
> > > optimization. Thanks for your feedback.
> > 
> > Once you have a proper IRQ de-multiplexer, you can integrate it into the
> > system with a DT change only. No need for changes in the m_can driver.
> > 
> 
> Is this a recommendation for the current patch?

It is a recommendation on how to get around the hardware limitation,
instead of falling back to polling.

> The reason I am asking is because adding firmware for the M4 to forward
> a mailbox with the IRQ to the A53 sounds like a good idea and we have been
> juggling the idea, but it is not an ideal solution if customers are
> using the M4 for other purposes like safety.

Of course, the feasibility of this approach depends on your system
design.

> > > > > This solution instantiates a hrtimer with 1 ms polling interval
> > > > > for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.
> > 
> > Have you seen my suggestion of the poll-interval?
> > 
> > Some Linux input drivers have the property poll-interval, would it make
> > sense to ass this here too?
> 
> Looking at some examples, I do think we could implement this poll-interval
> attribute, then read in the driver and initialize the hrtimer based on this.
> I like the idea to submit as a future optimization patch, thanks!

I would like to have the DT bindings in place, as handling legacy DT
without poll interval adds unnecessary complexity.

regards,
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: [RFC PATCH 0/5] Enable multiple MCAN on AM62x

[Mendez, Judith]

Hello Marc,

On 4/20/2023 4:36 AM, Marc Kleine-Budde wrote:
> On 19.04.2023 15:40:24, Mendez, Judith wrote:
>> Hello Marc,
>>
>> On 4/19/2023 1:10 AM, Marc Kleine-Budde wrote:
>>> On 18.04.2023 11:15:35, Mendez, Judith wrote:
>>>> Hello Marc,
>>>>
>>>> On 4/14/2023 12:49 PM, Marc Kleine-Budde wrote:
>>>>> On 13.04.2023 17:30: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.
>>>>>
>>>>> Is this a general hardware limitation, that effects all MCU domain
>>>>> peripherals? Is there a mailbox mechanism between the MCU and the MAIN
>>>>> domain, would it be possible to pass the IRQ with a small firmware on
>>>>> the MCU? Anyways, that's future optimization.
>>>>
>>>> This is a hardware limitation that affects AM62x SoC and has been carried
>>>> over to at least 1 other SoC. Using the MCU is an idea that we have juggled
>>>> around for a while, we will definitely keep it in mind for future
>>>> optimization. Thanks for your feedback.
>>>
>>> Once you have a proper IRQ de-multiplexer, you can integrate it into the
>>> system with a DT change only. No need for changes in the m_can driver.
>>>
>>
>> Is this a recommendation for the current patch?
> 
> It is a recommendation on how to get around the hardware limitation,
> instead of falling back to polling.
> 
>> The reason I am asking is because adding firmware for the M4 to forward
>> a mailbox with the IRQ to the A53 sounds like a good idea and we have been
>> juggling the idea, but it is not an ideal solution if customers are
>> using the M4 for other purposes like safety.
> 
> Of course, the feasibility of this approach depends on your system
> design.

I understand your concern. Like mentioned, using the M4 approach may not 
be the best solution since some customers use the M4 for various reasons 
that could provide problems for this design.

I think the best way to go would be to enable polling + your suggestion 
of using poll-interval in device tree. If poll-interval is specified, 
then we can enable polling mode for MCAN.

>>>>>> This solution instantiates a hrtimer with 1 ms polling interval
>>>>>> for a MCAN when there is no hardware interrupt. This 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 and lower polling periods than 1ms.
>>>
>>> Have you seen my suggestion of the poll-interval?
>>>
>>> Some Linux input drivers have the property poll-interval, would it make
>>> sense to ass this here too?
>>
>> Looking at some examples, I do think we could implement this poll-interval
>> attribute, then read in the driver and initialize the hrtimer based on this.
>> I like the idea to submit as a future optimization patch, thanks!
> 
> I would like to have the DT bindings in place, as handling legacy DT
> without poll interval adds unnecessary complexity.

Understood, thanks so much for your feedback.

regards,
Judith