can: bxcan: add support for single peripheral configuration

Add binding definition for CAN3 peripheral.

Signed-off-by Dario Binacchi <[email protected]>

---

 include/dt-bindings/mfd/stm32f7-rcc.h | 1 +
 1 file changed, 1 insertion(+)

Quoted 7 lines · Dario Binacchi

Add binding definition for CAN3 peripheral.

Signed-off-by Dario Binacchi <[email protected]>

---

 include/dt-bindings/mfd/stm32f7-rcc.h | 1 +
 1 file changed, 1 insertion(+)

Applied, thanks

Hey Lee Jones,

Quoted 11 lines · Lee Jones

Quoted 7 lines · Dario Binacchi

Add binding definition for CAN3 peripheral.

Signed-off-by Dario Binacchi <[email protected]>

---

 include/dt-bindings/mfd/stm32f7-rcc.h | 1 +
 1 file changed, 1 insertion(+)

Applied, thanks

I upstreamed the v2 of this series
(https://lore.kernel.org/all/[email protected]/)
that doesn't contain this change to net/main without noticing that the DT changes in that series depend on it.

This broke the DT compilation of the stm32f746.dtsi in the net/main tree. I don't see the stm32f7-rcc.h changes in linus/master so I'm afraid this will break mainline too :/

What are the possible solutions? I see:
1) revert the stm32f746.dtsi changes via net/main
2) upstream the stm32f7-rcc.h changes via net/main, too
3) upstream the stm32f7-rcc.h changes via you tree, so that it hits mainline in the v6.4 release cycle.

I'm in favor of solution number 1. Thoughts?

Quoted 30 lines · Marc Kleine-Budde

Hey Lee Jones,

Quoted 11 lines · Lee Jones

Quoted 7 lines · Dario Binacchi

Add binding definition for CAN3 peripheral.

Signed-off-by Dario Binacchi <[email protected]>

---

 include/dt-bindings/mfd/stm32f7-rcc.h | 1 +
 1 file changed, 1 insertion(+)

Applied, thanks

I upstreamed the v2 of this series
(https://lore.kernel.org/all/[email protected]/)
that doesn't contain this change to net/main without noticing that the DT changes in that series depend on it.

This broke the DT compilation of the stm32f746.dtsi in the net/main tree. I don't see the stm32f7-rcc.h changes in linus/master so I'm afraid this will break mainline too :/

What are the possible solutions? I see:
1) revert the stm32f746.dtsi changes via net/main
2) upstream the stm32f7-rcc.h changes via net/main, too
3) upstream the stm32f7-rcc.h changes via you tree, so that it hits mainline in the v6.4 release cycle.

Quoted 1 line

I'm in favor of solution number 1. Thoughts?

DTS should never go with driver changes or with driver trees, not only because it hides ABI breaks but also for above reasons. The best if you just drop or revert DTS commits, so they can go via platform maintainer.

Add support for bxCAN controller in single peripheral configuration:
- primary bxCAN
- dedicated Memory Access Controller unit
- 512-byte SRAM memory
- 14 fiter banks

Signed-off-by Dario Binacchi <[email protected]>

---

 drivers/net/can/bxcan.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/net/can/bxcan.c +7 -0

index e26ccd41e3cb..9bcbbb85da6e 100644
@@ -155,6 +155,7 @@ struct bxcan_regs {
 	u32 reserved0[88];		/* 0x20 */
 	struct bxcan_mb tx_mb[BXCAN_TX_MB_NUM];	/* 0x180 - tx mailbox */
 	struct bxcan_mb rx_mb[BXCAN_RX_MB_NUM];	/* 0x1b0 - rx mailbox */
+	u32 reserved1[12];		/* 0x1d0 */
 };
 
 struct bxcan_priv {

@@ -922,6 +923,12 @@ static int bxcan_get_berr_counter(const struct net_device *ndev,
 	return 0;
 }
 
+static const struct regmap_config bxcan_gcan_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
 static int bxcan_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;

@@ -942,11 +949,18 @@ static int bxcan_probe(struct platform_device *pdev)
 
 	gcan = syscon_regmap_lookup_by_phandle(np, "st,gcan");
 	if (IS_ERR(gcan)) {

Quoted 42 lines · Dario Binacchi

Add support for bxCAN controller in single peripheral configuration:
- primary bxCAN
- dedicated Memory Access Controller unit
- 512-byte SRAM memory
- 14 fiter banks

Signed-off-by Dario Binacchi <[email protected]>

---

 drivers/net/can/bxcan.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

drivers/net/can/bxcan.c +7 -0

index e26ccd41e3cb..9bcbbb85da6e 100644
@@ -155,6 +155,7 @@ struct bxcan_regs {
      u32 reserved0[88];              /* 0x20 */
      struct bxcan_mb tx_mb[BXCAN_TX_MB_NUM]; /* 0x180 - tx mailbox */
      struct bxcan_mb rx_mb[BXCAN_RX_MB_NUM]; /* 0x1b0 - rx mailbox */
+     u32 reserved1[12];              /* 0x1d0 */
 };
 
 struct bxcan_priv {

@@ -922,6 +923,12 @@ static int bxcan_get_berr_counter(const struct net_device *ndev,
      return 0;
 }
 
+static const struct regmap_config bxcan_gcan_regmap_config = {
+     .reg_bits = 32,
+     .val_bits = 32,
+     .reg_stride = 4,
+};
+
 static int bxcan_probe(struct platform_device *pdev)
 {
      struct device_node *np = pdev->dev.of_node;

@@ -942,11 +949,18 @@ static int bxcan_probe(struct platform_device *pdev)
 
      gcan = syscon_regmap_lookup_by_phandle(np, "st,gcan");
      if (IS_ERR(gcan)) {

drivers/net/can/bxcan.c static int bxcan_probe(struct platform_device *pdev) · line 959

Quoted 10 lines

drivers/net/can/bxcan.c +8 -2

-             dev_err(dev, "failed to get shared memory base address\n");
-             return PTR_ERR(gcan);
+             primary = true;
+             gcan = devm_regmap_init_mmio(dev,
+                                          regs + sizeof(struct bxcan_regs),
+                                          &bxcan_gcan_regmap_config);
+             if (IS_ERR(gcan)) {
+                     dev_err(dev, "failed to get filter base address\n");
+                     return PTR_ERR(gcan);
+             }

This probably works. Can we do better, i.e. without this additional code?

If you add a syscon node for the single instance CAN, too, you don't need a code change here, right?

drivers/net/can/bxcan.c static int bxcan_probe(struct platform_device *pdev) · line 966

Quoted 9 lines

drivers/net/can/bxcan.c +2 -1

+     } else {
+             primary = of_property_read_bool(np, "st,can-primary");
      }
 
-     primary = of_property_read_bool(np, "st,can-primary");
      clk = devm_clk_get(dev, NULL);
      if (IS_ERR(clk)) {
              dev_err(dev, "failed to get clock\n");

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 |

Quoted 4 lines

This probably works. Can we do better, i.e. without this additional code?

If you add a syscon node for the single instance CAN, too, you don't need a code change here, right?

I think so.

I have only one doubt about it. This implementation allows, implicitly, to distinguish if the peripheral is in single configuration (without handle to the gcan node) or in double configuration (with handle to the gcan node). For example, in single configuration the peripheral has 14 filter banks, while in double configuration there are 26 shared banks. Without code changes, this kind of information is lost. Is it better then, for future developments, to add a new boolean property to the can node of the dts (e.g. single-conf)?

Quoted 7 lines · Dario Binacchi

Quoted 4 lines

This probably works. Can we do better, i.e. without this additional code?

If you add a syscon node for the single instance CAN, too, you don't need a code change here, right?

I think so.

Quoted 9 lines

I have only one doubt about it. This implementation allows, implicitly, to distinguish if the peripheral is in single configuration (without handle to the gcan node) or in double configuration (with handle to the gcan node). For example, in single configuration the peripheral has 14 filter banks, while in double configuration there are 26 shared banks. Without code changes, this kind of information is lost. Is it better then, for future developments, to add a new boolean property to the can node of the dts (e.g. single-conf)?

The DT ist not yet mainline, so we can still change it. Another option is to have "st,can-primary" and "st,can-secondary" for the shared peripherals and nothing for the single instance.

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 |

Add support for bxCAN controller in single peripheral configuration:
- primary bxCAN
- dedicated Memory Access Controller unit
- 512-byte SRAM memory
- 14 fiter banks