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

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

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
  

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   |
  

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
  

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

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   |