[RFC PATCH 0/4] can: bxcan: add support for ST bxCAN controller

[RFC PATCH 0/4] can: bxcan: add support for ST bxCAN controller

[Dario Binacchi]

The series adds support for the basic extended CAN controller (bxCAN)
found in many low- to middle-end STM32 SoCs.

The driver has been tested on the stm32f469i-discovery board with a
kernel version 5.19.0-rc2 in loopback + silent mode:

ip link set can0 type can bitrate 125000 loopback on listen-only on
ip link set up can0
candump can0 -L &
cansend can0 300#AC.AB.AD.AE.75.49.AD.D1

For uboot and kernel compilation, as well as for rootfs creation I used
buildroot:

make stm32f469_disco_sd_defconfig
make

but I had to patch can-utils and busybox as can-utils and iproute are
not compiled for MMU-less microcotrollers. In the case of can-utils,
replacing the calls to fork() with vfork(), I was able to compile the
package with working candump and cansend applications, while in the
case of iproute, I ran into more than one problem and finally I decided
to extend busybox's ip link command for CAN-type devices. I'm still
wondering if it was really necessary, but this way I was able to test
the driver.


Dario Binacchi (4):
  dt-bindings: net: can: add STM32 bxcan DT bindings
  ARM: dts: stm32: add CAN support on stm32f429
  ARM: dts: stm32: add pin map for CAN controller on stm32f4
  can: bxcan: add support for ST bxCAN controller

 .../devicetree/bindings/net/can/st,bxcan.yaml | 139 +++
 arch/arm/boot/dts/stm32f4-pinctrl.dtsi        |  32 +
 arch/arm/boot/dts/stm32f429.dtsi              |  30 +
 drivers/net/can/Kconfig                       |   1 +
 drivers/net/can/Makefile                      |   1 +
 drivers/net/can/bxcan/Kconfig                 |  34 +
 drivers/net/can/bxcan/Makefile                |   4 +
 drivers/net/can/bxcan/bxcan-core.c            | 201 ++++
 drivers/net/can/bxcan/bxcan-core.h            |  33 +
 drivers/net/can/bxcan/bxcan-drv.c             | 980 ++++++++++++++++++
 10 files changed, 1455 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml
 create mode 100644 drivers/net/can/bxcan/Kconfig
 create mode 100644 drivers/net/can/bxcan/Makefile
 create mode 100644 drivers/net/can/bxcan/bxcan-core.c
 create mode 100644 drivers/net/can/bxcan/bxcan-core.h
 create mode 100644 drivers/net/can/bxcan/bxcan-drv.c

-- 
2.32.0

[RFC PATCH 1/4] dt-bindings: net: can: add STM32 bxcan DT bindings

[Dario Binacchi]

Add documentation of device tree bindings for the STM32 basic extended
CAN (bxcan) controller.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 .../devicetree/bindings/net/can/st,bxcan.yaml | 139 ++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml

diff --git a/Documentation/devicetree/bindings/net/can/st,bxcan.yaml b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
new file mode 100644
index 000000000000..f4cfd26e4785
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/can/st,bxcan.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics bxCAN controller Device Tree Bindings
+
+description: STMicroelectronics BxCAN controller for CAN bus
+
+maintainers:
+  - Dario Binacchi <dario.binacchi@amarulasolutions.com>
+
+allOf:
+  - $ref: can-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - st,stm32-bxcan-core
+
+  reg:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  clocks:
+    description:
+      Input clock for registers access
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - resets
+  - clocks
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+patternProperties:
+  "^can@[0-9]+$":
+    type: object
+    description:
+      A CAN block node contains two subnodes, representing each one a CAN
+      instance available on the machine.
+
+    properties:
+      compatible:
+        enum:
+          - st,stm32-bxcan
+
+      master:
+        description:
+          Master and slave mode of the bxCAN peripheral is only relevant
+          if the chip has two CAN peripherals. In that case they share
+          some of the required logic, and that means you cannot use the
+          slave CAN without the master CAN.
+        type: boolean
+
+      reg:
+        description: |
+          Offset of CAN instance in CAN block. Valid values are:
+            - 0x0:   CAN1
+            - 0x400: CAN2
+        maxItems: 1
+
+      interrupts:
+        items:
+          - description: transmit interrupt
+          - description: FIFO 0 receive interrupt
+          - description: FIFO 1 receive interrupt
+          - description: status change error interrupt
+
+      interrupt-names:
+        items:
+          - const: tx
+          - const: rx0
+          - const: rx1
+          - const: sce
+
+      resets:
+        maxItems: 1
+
+      clocks:
+        description:
+          Input clock for registers access
+        maxItems: 1
+
+    additionalProperties: false
+
+    required:
+      - compatible
+      - reg
+      - interrupts
+      - resets
+
+examples:
+  - |
+    #include <dt-bindings/clock/stm32fx-clock.h>
+    #include <dt-bindings/mfd/stm32f4-rcc.h>
+
+    can: can@40006400 {
+        compatible = "st,stm32-bxcan-core";
+        reg = <0x40006400 0x800>;
+        resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
+        clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN1)>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        status = "disabled";
+
+        can1: can@0 {
+            compatible = "st,stm32-bxcan";
+            reg = <0x0>;
+            interrupts = <19>, <20>, <21>, <22>;
+            interrupt-names = "tx", "rx0", "rx1", "sce";
+            resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
+            master;
+            status = "disabled";
+        };
+
+        can2: can@400 {
+            compatible = "st,stm32-bxcan";
+            reg = <0x400>;
+            interrupts = <63>, <64>, <65>, <66>;
+            interrupt-names = "tx", "rx0", "rx1", "sce";
+            resets = <&rcc STM32F4_APB1_RESET(CAN2)>;
+            clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN2)>;
+            status = "disabled";
+        };
+    };
-- 
2.32.0

[RFC PATCH 2/4] ARM: dts: stm32: add CAN support on stm32f429

[Dario Binacchi]

Add support for bxcan (Basic eXtended CAN controller) to STM32F429. The
chip contains two CAN peripherals, CAN1 the master and CAN2 the slave,
that share some of the required logic like clock and filters. This means
that the slave CAN can't be used without the master CAN.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 arch/arm/boot/dts/stm32f429.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index c31ceb821231..28db387ad5e8 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -362,6 +362,36 @@ i2c3: i2c@40005c00 {
 			status = "disabled";
 		};
 
+		can: can@40006400 {
+			compatible = "st,stm32-bxcan-core";
+			reg = <0x40006400 0x800>;
+			resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
+			clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN1)>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			can1: can@0 {
+				compatible = "st,stm32-bxcan";
+				reg = <0x0>;
+				interrupts = <19>, <20>, <21>, <22>;
+				interrupt-names = "tx", "rx0", "rx1", "sce";
+				resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
+				master;
+				status = "disabled";
+			};
+
+			can2: can@400 {
+				compatible = "st,stm32-bxcan";
+				reg = <0x400>;
+				interrupts = <63>, <64>, <65>, <66>;
+				interrupt-names = "tx", "rx0", "rx1", "sce";
+				resets = <&rcc STM32F4_APB1_RESET(CAN2)>;
+				clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN2)>;
+				status = "disabled";
+			};
+		};
+
 		dac: dac@40007400 {
 			compatible = "st,stm32f4-dac-core";
 			reg = <0x40007400 0x400>;
-- 
2.32.0

[RFC PATCH 3/4] ARM: dts: stm32: add pin map for CAN controller on stm32f4

[Dario Binacchi]

Add pin configurations for using CAN controller on stm32f469-disco
board. They are located on the Arduino compatible connector CN5 (CAN1)
and on the extension connector CN12 (CAN2).

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 arch/arm/boot/dts/stm32f4-pinctrl.dtsi | 32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f4-pinctrl.dtsi b/arch/arm/boot/dts/stm32f4-pinctrl.dtsi
index 500bcc302d42..a8709363898a 100644
--- a/arch/arm/boot/dts/stm32f4-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32f4-pinctrl.dtsi
@@ -448,6 +448,38 @@ pins2 {
 					slew-rate = <2>;
 				};
 			};
+
+			can1_pins_a: can1-0 {
+				pins1 {
+					pinmux = <STM32_PINMUX('B', 9, AF9)>; /* CAN1_TX */
+				};
+				pins2 {
+					pinmux = <STM32_PINMUX('B', 8, AF9)>; /* CAN1_RX */
+					bias-pull-up;
+				};
+			};
+
+			can2_pins_a: can2-0 {
+				pins1 {
+					pinmux = <STM32_PINMUX('B', 13, AF9)>; /* CAN2_TX */
+				};
+				pins2 {
+					pinmux = <STM32_PINMUX('B', 5, AF9)>; /* CAN2_RX */
+					bias-pull-up;
+				};
+			};
+
+			can2_pins_b: can2-1 {
+				pins1 {
+					pinmux = <STM32_PINMUX('B', 13, AF9)>; /* CAN2_TX */
+
+				};
+				pins2 {
+					pinmux = <STM32_PINMUX('B', 12, AF9)>; /* CAN2_RX */
+					bias-pull-up;
+				};
+			};
+
 		};
 	};
 };
-- 
2.32.0

Re: [RFC PATCH 1/4] dt-bindings: net: can: add STM32 bxcan DT bindings

[Krzysztof Kozlowski]

On 17/08/2022 17:35, Dario Binacchi wrote:
> Add documentation of device tree bindings for the STM32 basic extended
> CAN (bxcan) controller.
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

You do not need two SoBs. Keep only one, matching the From field.

> ---
> 
>  .../devicetree/bindings/net/can/st,bxcan.yaml | 139 ++++++++++++++++++
>  1 file changed, 139 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/can/st,bxcan.yaml b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> new file mode 100644
> index 000000000000..f4cfd26e4785
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml

File name like compatible, so st,stm32-bxcan-core.yaml (or some other
name, see comment later)

> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/st,bxcan.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics bxCAN controller Device Tree Bindings

s/Device Tree Bindings//

> +
> +description: STMicroelectronics BxCAN controller for CAN bus
> +
> +maintainers:
> +  - Dario Binacchi <dario.binacchi@amarulasolutions.com>
> +
> +allOf:
> +  - $ref: can-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,stm32-bxcan-core

compatibles are supposed to be specific. If this is some type of
micro-SoC, then it should have its name/number. If it is dedicated
device, is the final name bxcan core? Google says  the first is true, so
you miss specific device part.
	
> +
> +  reg:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      Input clock for registers access
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - resets
> +  - clocks
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +additionalProperties: false
> +
> +patternProperties:

This goes after "properties: in top level (before "required").

> +  "^can@[0-9]+$":
> +    type: object
> +    description:
> +      A CAN block node contains two subnodes, representing each one a CAN
> +      instance available on the machine.
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - st,stm32-bxcan

Why exactly do you need compatible for the child? Is it an entierly
separate device?

Comments about specific part are applied here as well.

> +
> +      master:

Is this a standard property? I don't see it anywhere else. Non-standard
properties require vendor prefix.

> +        description:
> +          Master and slave mode of the bxCAN peripheral is only relevant
> +          if the chip has two CAN peripherals. In that case they share
> +          some of the required logic, and that means you cannot use the
> +          slave CAN without the master CAN.
> +        type: boolean
> +
> +      reg:
> +        description: |
> +          Offset of CAN instance in CAN block. Valid values are:
> +            - 0x0:   CAN1
> +            - 0x400: CAN2
> +        maxItems: 1
> +
> +      interrupts:
> +        items:
> +          - description: transmit interrupt
> +          - description: FIFO 0 receive interrupt
> +          - description: FIFO 1 receive interrupt
> +          - description: status change error interrupt
> +
> +      interrupt-names:
> +        items:
> +          - const: tx
> +          - const: rx0
> +          - const: rx1
> +          - const: sce
> +
> +      resets:
> +        maxItems: 1
> +
> +      clocks:
> +        description:
> +          Input clock for registers access
> +        maxItems: 1
> +
> +    additionalProperties: false
> +
> +    required:
> +      - compatible
> +      - reg
> +      - interrupts
> +      - resets
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/stm32fx-clock.h>
> +    #include <dt-bindings/mfd/stm32f4-rcc.h>
> +
> +    can: can@40006400 {
> +        compatible = "st,stm32-bxcan-core";
> +        reg = <0x40006400 0x800>;
> +        resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
> +        clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN1)>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "disabled";

No status in examples.

> +
> +        can1: can@0 {
> +            compatible = "st,stm32-bxcan";
> +            reg = <0x0>;
> +            interrupts = <19>, <20>, <21>, <22>;
> +            interrupt-names = "tx", "rx0", "rx1", "sce";
> +            resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
> +            master;
> +            status = "disabled";

No status in examples.


> +        };
> +
> +        can2: can@400 {
> +            compatible = "st,stm32-bxcan";
> +            reg = <0x400>;
> +            interrupts = <63>, <64>, <65>, <66>;
> +            interrupt-names = "tx", "rx0", "rx1", "sce";
> +            resets = <&rcc STM32F4_APB1_RESET(CAN2)>;
> +            clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN2)>;
> +            status = "disabled";

No status in examples.

> +        };
> +    };


Best regards,
Krzysztof

Re: [RFC PATCH 3/4] ARM: dts: stm32: add pin map for CAN controller on stm32f4

[Krzysztof Kozlowski]

On 17/08/2022 17:35, Dario Binacchi wrote:
> Add pin configurations for using CAN controller on stm32f469-disco
> board. They are located on the Arduino compatible connector CN5 (CAN1)
> and on the extension connector CN12 (CAN2).
> 


> +
> +			can2_pins_b: can2-1 {
> +				pins1 {
> +					pinmux = <STM32_PINMUX('B', 13, AF9)>; /* CAN2_TX */
> +
> +				};
> +				pins2 {
> +					pinmux = <STM32_PINMUX('B', 12, AF9)>; /* CAN2_RX */
> +					bias-pull-up;
> +				};
> +			};
> +

No need for blank line.



Best regards,
Krzysztof

Re: [RFC PATCH 1/4] dt-bindings: net: can: add STM32 bxcan DT bindings

[Dario Binacchi]

Hi Krzysztof,

On Thu, Aug 18, 2022 at 10:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/08/2022 17:35, Dario Binacchi wrote:
> > Add documentation of device tree bindings for the STM32 basic extended
> > CAN (bxcan) controller.
> >
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> You do not need two SoBs. Keep only one, matching the From field.

I started implementing this driver in my spare time, so my intention
was to keep track of it.

>
> > ---
> >
> >  .../devicetree/bindings/net/can/st,bxcan.yaml | 139 ++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/st,bxcan.yaml b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> > new file mode 100644
> > index 000000000000..f4cfd26e4785
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
>
> File name like compatible, so st,stm32-bxcan-core.yaml (or some other
> name, see comment later)

>
> > @@ -0,0 +1,139 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/can/st,bxcan.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: STMicroelectronics bxCAN controller Device Tree Bindings
>
> s/Device Tree Bindings//

>
> > +
> > +description: STMicroelectronics BxCAN controller for CAN bus
> > +
> > +maintainers:
> > +  - Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > +
> > +allOf:
> > +  - $ref: can-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - st,stm32-bxcan-core
>
> compatibles are supposed to be specific. If this is some type of
> micro-SoC, then it should have its name/number. If it is dedicated
> device, is the final name bxcan core? Google says  the first is true, so
> you miss specific device part.

I don't know if I understand correctly, I hope the change in version 2
is what you requested.

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description:
> > +      Input clock for registers access
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - resets
> > +  - clocks
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> > +additionalProperties: false
> > +
> > +patternProperties:
>
> This goes after "properties: in top level (before "required").
>
> > +  "^can@[0-9]+$":
> > +    type: object
> > +    description:
> > +      A CAN block node contains two subnodes, representing each one a CAN
> > +      instance available on the machine.
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - st,stm32-bxcan
>
> Why exactly do you need compatible for the child? Is it an entierly
> separate device?

I took inspiration from other drivers for ST microcontroller
peripherals (e. g. drivers/iio/adc/stm32-adc-core.c,
drivers/iio/adc/stm32-adc.c) where
some resources are shared between the peripheral instances. In the
case of CAN, master (CAN1) and slave (CAN2) share the registers for
configuring the filters and the clock.
In the core module you can find the functions about the shared
resources, while the childrens implement the driver.

>
> Comments about specific part are applied here as well.
>
> > +
> > +      master:
>
> Is this a standard property?

no

> I don't see it anywhere else. Non-standard
> properties require vendor prefix.

ok, you'll find it in V2.

Thanks and regards,
Dario

>
> > +        description:
> > +          Master and slave mode of the bxCAN peripheral is only relevant
> > +          if the chip has two CAN peripherals. In that case they share
> > +          some of the required logic, and that means you cannot use the
> > +          slave CAN without the master CAN.
> > +        type: boolean
> > +
> > +      reg:
> > +        description: |
> > +          Offset of CAN instance in CAN block. Valid values are:
> > +            - 0x0:   CAN1
> > +            - 0x400: CAN2
> > +        maxItems: 1
> > +
> > +      interrupts:
> > +        items:
> > +          - description: transmit interrupt
> > +          - description: FIFO 0 receive interrupt
> > +          - description: FIFO 1 receive interrupt
> > +          - description: status change error interrupt
> > +
> > +      interrupt-names:
> > +        items:
> > +          - const: tx
> > +          - const: rx0
> > +          - const: rx1
> > +          - const: sce
> > +
> > +      resets:
> > +        maxItems: 1
> > +
> > +      clocks:
> > +        description:
> > +          Input clock for registers access
> > +        maxItems: 1
> > +
> > +    additionalProperties: false
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - interrupts
> > +      - resets
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/stm32fx-clock.h>
> > +    #include <dt-bindings/mfd/stm32f4-rcc.h>
> > +
> > +    can: can@40006400 {
> > +        compatible = "st,stm32-bxcan-core";
> > +        reg = <0x40006400 0x800>;
> > +        resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
> > +        clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN1)>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        status = "disabled";
>
> No status in examples.
>
> > +
> > +        can1: can@0 {
> > +            compatible = "st,stm32-bxcan";
> > +            reg = <0x0>;
> > +            interrupts = <19>, <20>, <21>, <22>;
> > +            interrupt-names = "tx", "rx0", "rx1", "sce";
> > +            resets = <&rcc STM32F4_APB1_RESET(CAN1)>;
> > +            master;
> > +            status = "disabled";
>
> No status in examples.
>
>
> > +        };
> > +
> > +        can2: can@400 {
> > +            compatible = "st,stm32-bxcan";
> > +            reg = <0x400>;
> > +            interrupts = <63>, <64>, <65>, <66>;
> > +            interrupt-names = "tx", "rx0", "rx1", "sce";
> > +            resets = <&rcc STM32F4_APB1_RESET(CAN2)>;
> > +            clocks = <&rcc 0 STM32F4_APB1_CLOCK(CAN2)>;
> > +            status = "disabled";
>
> No status in examples.
>
> > +        };
> > +    };
>
>
> Best regards,
> Krzysztof



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [RFC PATCH 1/4] dt-bindings: net: can: add STM32 bxcan DT bindings

[Krzysztof Kozlowski]

On 20/08/2022 11:08, Dario Binacchi wrote:
> Hi Krzysztof,
> 
> On Thu, Aug 18, 2022 at 10:22 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/08/2022 17:35, Dario Binacchi wrote:
>>> Add documentation of device tree bindings for the STM32 basic extended
>>> CAN (bxcan) controller.
>>>
>>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>
>> You do not need two SoBs. Keep only one, matching the From field.
> 
> I started implementing this driver in my spare time, so my intention
> was to keep track of it.

SoB is not related to copyrights. Keep personal copyrights (with/next to
work ones), but SoB is coming from a person and that's only one. Choose
one "person".

> 
>>
>>> ---
>>>
>>>  .../devicetree/bindings/net/can/st,bxcan.yaml | 139 ++++++++++++++++++
>>>  1 file changed, 139 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/st,bxcan.yaml b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
>>> new file mode 100644
>>> index 000000000000..f4cfd26e4785
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
>>
>> File name like compatible, so st,stm32-bxcan-core.yaml (or some other
>> name, see comment later)
> 
>>
>>> @@ -0,0 +1,139 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/can/st,bxcan.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: STMicroelectronics bxCAN controller Device Tree Bindings
>>
>> s/Device Tree Bindings//
> 
>>
>>> +
>>> +description: STMicroelectronics BxCAN controller for CAN bus
>>> +
>>> +maintainers:
>>> +  - Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>> +
>>> +allOf:
>>> +  - $ref: can-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - st,stm32-bxcan-core
>>
>> compatibles are supposed to be specific. If this is some type of
>> micro-SoC, then it should have its name/number. If it is dedicated
>> device, is the final name bxcan core? Google says  the first is true, so
>> you miss specific device part.
> 
> I don't know if I understand correctly, I hope the change in version 2
> is what you requested.

What is the name of the SoC, where this is in?

> 
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    description:
>>> +      Input clock for registers access
>>> +    maxItems: 1
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 0
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - resets
>>> +  - clocks
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +
>>> +additionalProperties: false
>>> +
>>> +patternProperties:
>>
>> This goes after "properties: in top level (before "required").
>>
>>> +  "^can@[0-9]+$":
>>> +    type: object
>>> +    description:
>>> +      A CAN block node contains two subnodes, representing each one a CAN
>>> +      instance available on the machine.
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - st,stm32-bxcan
>>
>> Why exactly do you need compatible for the child? Is it an entierly
>> separate device?
> 
> I took inspiration from other drivers for ST microcontroller
> peripherals (e. g. drivers/iio/adc/stm32-adc-core.c,
> drivers/iio/adc/stm32-adc.c) where
> some resources are shared between the peripheral instances. In the
> case of CAN, master (CAN1) and slave (CAN2) share the registers for
> configuring the filters and the clock.
> In the core module you can find the functions about the shared
> resources, while the childrens implement the driver.

In both cases you refer to the driver, but we talk here about bindings
which are rather not related. So I repeat the question - is the child
entirely separate device which can be used in other devices?


Best regards,
Krzysztof

Re: [RFC PATCH 1/4] dt-bindings: net: can: add STM32 bxcan DT bindings

[Dario Binacchi]

Hi Krzysztof,

On Mon, Aug 22, 2022 at 7:39 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 20/08/2022 11:08, Dario Binacchi wrote:
> > Hi Krzysztof,
> >
> > On Thu, Aug 18, 2022 at 10:22 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 17/08/2022 17:35, Dario Binacchi wrote:
> >>> Add documentation of device tree bindings for the STM32 basic extended
> >>> CAN (bxcan) controller.
> >>>
> >>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>
> >> You do not need two SoBs. Keep only one, matching the From field.
> >
> > I started implementing this driver in my spare time, so my intention
> > was to keep track of it.
>
> SoB is not related to copyrights. Keep personal copyrights (with/next to
> work ones), but SoB is coming from a person and that's only one. Choose
> one "person".

Ok, I got it.

>
> >
> >>
> >>> ---
> >>>
> >>>  .../devicetree/bindings/net/can/st,bxcan.yaml | 139 ++++++++++++++++++
> >>>  1 file changed, 139 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/can/st,bxcan.yaml b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> >>> new file mode 100644
> >>> index 000000000000..f4cfd26e4785
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/can/st,bxcan.yaml
> >>
> >> File name like compatible, so st,stm32-bxcan-core.yaml (or some other
> >> name, see comment later)
> >
> >>
> >>> @@ -0,0 +1,139 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/net/can/st,bxcan.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: STMicroelectronics bxCAN controller Device Tree Bindings
> >>
> >> s/Device Tree Bindings//
> >
> >>
> >>> +
> >>> +description: STMicroelectronics BxCAN controller for CAN bus
> >>> +
> >>> +maintainers:
> >>> +  - Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: can-controller.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - st,stm32-bxcan-core
> >>
> >> compatibles are supposed to be specific. If this is some type of
> >> micro-SoC, then it should have its name/number. If it is dedicated
> >> device, is the final name bxcan core? Google says  the first is true, so
> >> you miss specific device part.
> >
> > I don't know if I understand correctly, I hope the change in version 2
> > is what you requested.
>
> What is the name of the SoC, where this is in?

STM32F4

>
> >
> >>
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  resets:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    description:
> >>> +      Input clock for registers access
> >>> +    maxItems: 1
> >>> +
> >>> +  '#address-cells':
> >>> +    const: 1
> >>> +
> >>> +  '#size-cells':
> >>> +    const: 0
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - resets
> >>> +  - clocks
> >>> +  - '#address-cells'
> >>> +  - '#size-cells'
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +patternProperties:
> >>
> >> This goes after "properties: in top level (before "required").
> >>
> >>> +  "^can@[0-9]+$":
> >>> +    type: object
> >>> +    description:
> >>> +      A CAN block node contains two subnodes, representing each one a CAN
> >>> +      instance available on the machine.
> >>> +
> >>> +    properties:
> >>> +      compatible:
> >>> +        enum:
> >>> +          - st,stm32-bxcan
> >>
> >> Why exactly do you need compatible for the child? Is it an entierly
> >> separate device?
> >
> > I took inspiration from other drivers for ST microcontroller
> > peripherals (e. g. drivers/iio/adc/stm32-adc-core.c,
> > drivers/iio/adc/stm32-adc.c) where
> > some resources are shared between the peripheral instances. In the
> > case of CAN, master (CAN1) and slave (CAN2) share the registers for
> > configuring the filters and the clock.
> > In the core module you can find the functions about the shared
> > resources, while the childrens implement the driver.
>
> In both cases you refer to the driver, but we talk here about bindings
> which are rather not related. So I repeat the question - is the child
> entirely separate device which can be used in other devices?

IMHO, I think so.

Thanks and regards,
Dario
>
>
> Best regards,
> Krzysztof



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com