[PATCH 0/4] can: bxcan: add support for single peripheral configuration

[PATCH 0/4] can: bxcan: add support for single peripheral configuration

[Dario Binacchi]

The series adds support for managing bxCAN controllers in single peripheral
configuration.
Unlike stm32f4 SOCs, where bxCAN controllers are only in dual peripheral
configuration, stm32f7 SOCs contain three CAN peripherals, CAN1 and CAN2
in dual peripheral configuration and CAN3 in single peripheral
configuration:
- Dual CAN peripheral configuration:
 * CAN1: Primary bxCAN for managing the communication between a secondary
   bxCAN and the 512-byte SRAM memory.
 * CAN2: Secondary bxCAN with no direct access to the SRAM memory.
   This means that the two bxCAN cells share the 512-byte SRAM memory and
   CAN2 can't be used without enabling CAN1.
- Single CAN peripheral configuration:
 * CAN3: Primary bxCAN with dedicated Memory Access Controller unit and
   512-byte SRAM memory.

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

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



Dario Binacchi (4):
  dt-bindings: mfd: stm32f7: add binding definition for CAN3
  ARM: dts: stm32: add CAN support on stm32f746
  ARM: dts: stm32: add pin map for CAN controller on stm32f7
  can: bxcan: add support for single peripheral configuration

 arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 82 ++++++++++++++++++++++++++
 arch/arm/boot/dts/stm32f746.dtsi       | 39 ++++++++++++
 drivers/net/can/bxcan.c                | 20 ++++++-
 include/dt-bindings/mfd/stm32f7-rcc.h  |  1 +
 4 files changed, 139 insertions(+), 3 deletions(-)

-- 
2.32.0

[PATCH 4/4] can: bxcan: add support for single peripheral configuration

[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 <dario.binacchi@amarulasolutions.com>

---

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

diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
index e26ccd41e3cb..9bcbbb85da6e 100644
--- a/drivers/net/can/bxcan.c
+++ b/drivers/net/can/bxcan.c
@@ -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)) {
-		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);
+		}
+	} 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");
-- 
2.32.0

Re: [PATCH 4/4] can: bxcan: add support for single peripheral configuration

[Marc Kleine-Budde]

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

On 23.04.2023 19:25:28, Dario Binacchi wrote:
> 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 <dario.binacchi@amarulasolutions.com>
> 
> ---
> 
>  drivers/net/can/bxcan.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
> index e26ccd41e3cb..9bcbbb85da6e 100644
> --- a/drivers/net/can/bxcan.c
> +++ b/drivers/net/can/bxcan.c
> @@ -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)) {
> -		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?

> +	} 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   |

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

Re: [PATCH 4/4] can: bxcan: add support for single peripheral configuration

[Dario Binacchi]

Hi Marc,

On Sun, Apr 23, 2023 at 9:16 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 23.04.2023 19:25:28, Dario Binacchi wrote:
> > 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 <dario.binacchi@amarulasolutions.com>
> >
> > ---
> >
> >  drivers/net/can/bxcan.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/can/bxcan.c b/drivers/net/can/bxcan.c
> > index e26ccd41e3cb..9bcbbb85da6e 100644
> > --- a/drivers/net/can/bxcan.c
> > +++ b/drivers/net/can/bxcan.c
> > @@ -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)) {
> > -             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?

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)?

Thanks and regards,

Dario

>
> > +     } 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   |



-- 

Dario Binacchi

Senior 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: [PATCH 4/4] can: bxcan: add support for single peripheral configuration

[Marc Kleine-Budde]

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

On 24.04.2023 08:56:03, Dario Binacchi wrote:
> > 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)?

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   |

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

Re: [PATCH 4/4] can: bxcan: add support for single peripheral configuration

[Dario Binacchi]

Hi Marc,

On Mon, Apr 24, 2023 at 12:06 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 24.04.2023 08:56:03, Dario Binacchi wrote:
> > > 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)?
>
> 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.

I did some tests following your suggestion. It is however necessary to
make some small changes to the driver.
I will send v2 as soon as possible.

Thanks and regards,
Dario

>
> 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   |



-- 

Dario Binacchi

Senior 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: [PATCH 4/4] can: bxcan: add support for single peripheral configuration

[Simon Horman]

On Sun, Apr 23, 2023 at 07:25:28PM +0200, Dario Binacchi wrote:
> Add support for bxCAN controller in single peripheral configuration:
> - primary bxCAN
> - dedicated Memory Access Controller unit
> - 512-byte SRAM memory
> - 14 fiter banks

nit: s/fiter/filter/ ?

Re: [PATCH 1/4] dt-bindings: mfd: stm32f7: add binding definition for CAN3

[Marc Kleine-Budde]

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

Hey Lee Jones,

On 24.04.2023 10:02:29, Lee Jones wrote:
> On Sun, 23 Apr 2023, Dario Binacchi wrote:
> 
> > Add binding definition for CAN3 peripheral.
> > 
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > ---
> > 
> >  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/20230427204540.3126234-1-dario.binacchi@amarulasolutions.com/)
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?

Sorry for the mess,
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: 484 bytes --]

Re: [PATCH 1/4] dt-bindings: mfd: stm32f7: add binding definition for CAN3

[Krzysztof Kozlowski]

On 17/05/2023 16:16, Marc Kleine-Budde wrote:
> Hey Lee Jones,
> 
> On 24.04.2023 10:02:29, Lee Jones wrote:
>> On Sun, 23 Apr 2023, Dario Binacchi wrote:
>>
>>> Add binding definition for CAN3 peripheral.
>>>
>>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>>> ---
>>>
>>>  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/20230427204540.3126234-1-dario.binacchi@amarulasolutions.com/)
> 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?

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.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: mfd: stm32f7: add binding definition for CAN3

[Marc Kleine-Budde]

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

On 17.05.2023 17:23:13, Krzysztof Kozlowski wrote:
> On 17/05/2023 16:16, Marc Kleine-Budde wrote:
> > Hey Lee Jones,
> > 
> > On 24.04.2023 10:02:29, Lee Jones wrote:
> >> On Sun, 23 Apr 2023, Dario Binacchi wrote:
> >>
> >>> Add binding definition for CAN3 peripheral.
> >>>
> >>> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >>> ---
> >>>
> >>>  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/20230427204540.3126234-1-dario.binacchi@amarulasolutions.com/)
> > 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?
> 
> 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.

Reverted: https://lore.kernel.org/20230517181950.1106697-1-mkl@pengutronix.de

Thanks,
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 --]