Message-ID: <20251121-ancient-flamingo-of-cubism-cda563-mkl@pengutronix.de>
Date: Fri, 21 Nov 2025 11:52:20 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Arun Muthusamy <arun.muthusamy@...sler.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	mailhol@...nel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-can@...r.kernel.org, Daniel Hellstrom <daniel@...sler.com>
Subject: Re: [PATCH 05/10] can: grcan: add FD capability detection and
 nominal bit-timing

On 18.11.2025 10:21:10, Arun Muthusamy wrote:
> From: Daniel Hellstrom <daniel@...sler.com>
>
> Add capability for the driver to detect CAN FD support
> and adjust accordingly. Introduce structures and functions
> for setting nominal bit-timing for standard CAN and CAN FD.
> The `grcan_hwcap` structure defines hardware capabilities like
> CAN FD support and baud-rate options. Additionally, improved
> device tree compatibility by updating the `of_device_id` table
> for better matching of GRCAN and GRCANFD devices. Also update
> Kconfig to mention GRCANFD support.
>
> Signed-off-by: Arun Muthusamy <arun.muthusamy@...sler.com>
> Signed-off-by: Daniel Hellstrom <daniel@...sler.com>

Your S-o-b must come last.

> ---
>  drivers/net/can/Kconfig |   6 +-
>  drivers/net/can/grcan.c | 189 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 176 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index d43d56694667..816b6e71a990 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -133,10 +133,12 @@ config CAN_FLEXCAN
>  	  Say Y here if you want to support for Freescale FlexCAN.
>
>  config CAN_GRCAN
> -	tristate "Aeroflex Gaisler GRCAN and GRHCAN CAN devices"
> +	tristate "Aeroflex Gaisler GRCAN, GRCANFD and GRHCAN CAN devices"
>  	depends on OF && HAS_DMA && HAS_IOMEM
>  	help
> -	  Say Y here if you want to use Aeroflex Gaisler GRCAN or GRHCAN.
> +	  Say Y here if you want to use Aeroflex Gaisler GRCAN or GRCANFD
> +	  or GRHCAN.
> +
>  	  Note that the driver supports little endian, even though little
>  	  endian syntheses of the cores would need some modifications on
>  	  the hardware level to work.
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 538a9b4f82ab..b9b0dd7d53f6 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -33,6 +33,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
> @@ -50,7 +51,11 @@ struct grcan_registers {
>  	u32 __reserved1[GRCAN_RESERVE_SIZE(0x08, 0x18)];
>  	u32 smask;	/* 0x18 - CanMASK */
>  	u32 scode;	/* 0x1c - CanCODE */
> -	u32 __reserved2[GRCAN_RESERVE_SIZE(0x1c, 0x100)];
> +	u32 __reserved2[GRCAN_RESERVE_SIZE(0x1c, 0x40)];
> +	u32 nbtr;	/* 0x40 */
> +	u32 fdbtr;	/* 0x44 */
> +	u32 tdelay;	/* 0x48 */
> +	u32 __reserved2_[GRCAN_RESERVE_SIZE(0x48, 0x100)];
>  	u32 pimsr;	/* 0x100 */
>  	u32 pimr;	/* 0x104 */
>  	u32 pisr;	/* 0x108 */
> @@ -212,6 +217,67 @@ struct grcan_registers {
>  #error "Invalid default buffer size"
>  #endif
>
> +#define GRCANFD_NOMCONF_SJW_MIN	1
> +#define GRCANFD_NOMCONF_SJW_MAX	16
> +#define GRCANFD_NOMCONF_PS1_MIN	2
> +#define GRCANFD_NOMCONF_PS1_MAX	63
> +#define GRCANFD_NOMCONF_PS2_MIN	2
> +#define GRCANFD_NOMCONF_PS2_MAX	16
> +#define GRCANFD_NOMCONF_SCALER_MIN	0
> +#define GRCANFD_NOMCONF_SCALER_MAX	255
> +#define GRCANFD_NOMCONF_SCALER_INC	1

Please directly add the values to the struct can_bittiming_const.

> +
> +#define GRCANFD_NBTR_SCALER 0x00ff0000
> +#define GRCANFD_NBTR_PS1 0x0000fc00
> +#define GRCANFD_NBTR_PS2 0x000003e0
> +#define GRCANFD_NBTR_SJW 0x0000001f

Please use GEN_MASK() for these

> +#define GRCANFD_NBTR_TIMING						\
> +	(GRCANFD_NBTR_SCALER | GRCANFD_NBTR_PS1 | GRCANFD_NBTR_PS2 |	\
> +	 GRCANFD_NBTR_SJW)
> +
> +#define GRCANFD_NBTR_SCALER_BIT 16
> +#define GRCANFD_NBTR_PS1_BIT 10
> +#define GRCANFD_NBTR_PS2_BIT 5
> +#define GRCANFD_NBTR_SJW_BIT 0

These can be removed, if you use FIELD_PREP()...see below

Same comments apply to the FD defines...

> +
> +#define GRCANFD_FDCONF_SJW_MIN	1
> +#define GRCANFD_FDCONF_SJW_MAX	8
> +#define GRCANFD_FDCONF_PS1_MIN	1
> +#define GRCANFD_FDCONF_PS1_MAX	15
> +#define GRCANFD_FDCONF_PS2_MIN	2
> +#define GRCANFD_FDCONF_PS2_MAX	8
> +#define GRCANFD_FDCONF_SCALER_MIN	0
> +#define GRCANFD_FDCONF_SCALER_MAX	255
> +#define GRCANFD_FDCONF_SCALER_INC	1
> +
> +#define GRCANFD_FDBTR_SCALER 0x00ff0000
> +#define GRCANFD_FDBTR_PS1 0x00003c00
> +#define GRCANFD_FDBTR_PS2 0x000001e0
> +#define GRCANFD_FDBTR_SJW 0x0000000f
> +#define GRCANFD_FDBTR_TIMING						\
> +	(GRCANFD_FDBTR_SCALER | GRCANFD_FDBTR_PS1 | GRCANFD_FDBTR_PS2 |	\
> +	 GRCANFD_FDBTR_SJW)
> +
> +#define GRCANFD_FDBTR_SCALER_BIT 16
> +#define GRCANFD_FDBTR_PS1_BIT 10
> +#define GRCANFD_FDBTR_PS2_BIT 5
> +#define GRCANFD_FDBTR_SJW_BIT 0
> +
> +/* Hardware capabilities */
> +struct grcan_hwcap {
> +	/* CAN-FD capable, indicates GRCANFD IP.
> +	 * The GRCANFD has different baud-rate registers and extended DMA
> +	 * format to also describe FD-frames.
> +	 */
> +	bool fd;
> +	bool txbug_possible;

Please move the bools the end of the struct for better packing.

> +	const struct can_bittiming_const *bt_const;
> +	int (*set_bittiming)(struct net_device *dev);
> +};
> +
> +static const struct grcan_hwcap grcan_hwcap;
> +static const struct of_device_id grcan_match[];

can you avoid forward declaration by re-arranging you code?

> +
>  struct grcan_dma_buffer {
>  	size_t size;
>  	void *buf;
> @@ -304,6 +370,8 @@ struct grcan_priv {
>  	 */
>  	bool resetting;
>  	bool closing;
> +
> +	const struct grcan_hwcap *hwcap;

Same here, move the pointer in front of the bools. You can check the
struct packing with "pahole -a /path/to/driver.o"

>  };
>
>  /* Wait time for a short wait for ongoing to clear */
> @@ -402,6 +470,19 @@ static const struct can_bittiming_const grcan_bittiming_const = {
>  	.brp_inc	= GRCAN_CONF_SCALER_INC,
>  };
>
> +/* GRCANFD nominal boundaries for baud-rate parameters */
> +static const struct can_bittiming_const grcanfd_bittiming_const = {
> +	.name		= DRV_NAME,
> +	.tseg1_min	= GRCANFD_NOMCONF_PS1_MIN,
> +	.tseg1_max	= GRCANFD_NOMCONF_PS1_MAX,
> +	.tseg2_min	= GRCANFD_NOMCONF_PS2_MIN,
> +	.tseg2_max	= GRCANFD_NOMCONF_PS2_MAX,
> +	.sjw_max	= GRCANFD_NOMCONF_SJW_MAX,
> +	.brp_min	= GRCANFD_NOMCONF_SCALER_MIN + 1,
> +	.brp_max	= GRCANFD_NOMCONF_SCALER_MAX + 1,
> +	.brp_inc	= GRCANFD_NOMCONF_SCALER_INC,
> +};
> +
>  static int grcan_set_bittiming(struct net_device *dev)
>  {
>  	struct grcan_priv *priv = netdev_priv(dev);
> @@ -439,12 +520,55 @@ static int grcan_set_bittiming(struct net_device *dev)
>  	timing |= (ps1 << GRCAN_CONF_PS1_BIT) & GRCAN_CONF_PS1;
>  	timing |= (ps2 << GRCAN_CONF_PS2_BIT) & GRCAN_CONF_PS2;
>  	timing |= (scaler << GRCAN_CONF_SCALER_BIT) & GRCAN_CONF_SCALER;
> -	netdev_info(dev, "setting timing=0x%x\n", timing);
> +	netdev_dbg(dev, "setting timing=0x%x\n", timing);

nice!

>  	grcan_write_bits(&regs->conf, timing, GRCAN_CONF_TIMING);
>
>  	return 0;
>  }
>
> +static int grcanfd_set_bittiming(struct net_device *dev)
> +{
> +	struct grcan_priv *priv = netdev_priv(dev);
> +	struct grcan_registers __iomem *regs = priv->regs;
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	const char *msg;
> +	u32 timing = 0;
> +	int sjw, ps1, ps2, scaler;
> +
> +	/* Should never happen - function will not be called when
> +	 * device is up
> +	 */
> +	if (grcan_read_bits(&regs->ctrl, GRCAN_CTRL_ENABLE))
> +		return -EBUSY;

The framework will take care of this

> +
> +	sjw = bt->sjw;
> +	ps1 = (bt->prop_seg + bt->phase_seg1); /* tseg1 */

nitpick:
I think the comment can be removed, it's obvious.

> +	ps2 = bt->phase_seg2;
> +	scaler = (bt->brp - 1);

please remove the ( )

> +	netdev_dbg(dev, "Request for SJW=%d, PS1=%d, PS2=%d, SCALER=%d",
> +		   sjw, ps1, ps2, scaler);

I think the debug can be removed

> +	if (sjw > min(ps1, ps2)) {
> +		msg = "SJW <= MIN(PS1,PS2) must hold: PS1=%d, PS2=%d, SJW=%d\n";
> +		netdev_err(dev, msg, ps1, ps2, sjw);
> +		return -EINVAL;
> +	}
> +	if (ps2 < sjw) {
> +		netdev_err(dev, "PS2 >= SJW must hold: PS2=%d, SJW=%d\n",
> +			   ps2, sjw);
> +		return -EINVAL;
> +	}

The framework already does these checks for you:

https://elixir.bootlin.com/linux/v6.17.8/source/drivers/net/can/dev/bittiming.c#L18

> +
> +	timing |= (sjw << GRCANFD_NBTR_SJW_BIT) & GRCANFD_NBTR_SJW;
> +	timing |= (ps1 << GRCANFD_NBTR_PS1_BIT) & GRCANFD_NBTR_PS1;
> +	timing |= (ps2 << GRCANFD_NBTR_PS2_BIT) & GRCANFD_NBTR_PS2;
> +	timing |= (scaler << GRCANFD_NBTR_SCALER_BIT) &
> +		  GRCANFD_NBTR_SCALER;

Please use FIELD_PREP() instead of doing the shifting yourself.

> +	netdev_info(dev, "setting timing=0x%x\n", timing);
> +	grcan_write_bits(&regs->nbtr, timing, GRCANFD_NBTR_TIMING);
> +
> +	return 0;
> +}
> +
>  static int grcan_get_berr_counter(const struct net_device *dev,
>  				  struct can_berr_counter *bec)
>  {
> @@ -1569,7 +1693,8 @@ static const struct ethtool_ops grcan_ethtool_ops = {
>
>  static int grcan_setup_netdev(struct platform_device *ofdev,
>  			      void __iomem *base,
> -			      int irq, u32 ambafreq, bool txbug)
> +			      int irq, u32 ambafreq, bool txbug,
> +			      const struct grcan_hwcap *hwcap)
>  {
>  	struct net_device *dev;
>  	struct grcan_priv *priv;
> @@ -1592,14 +1717,15 @@ static int grcan_setup_netdev(struct platform_device *ofdev,
>  	priv->dev = dev;
>  	priv->ofdev_dev = &ofdev->dev;
>  	priv->regs = base;
> -	priv->can.bittiming_const = &grcan_bittiming_const;
> -	priv->can.do_set_bittiming = grcan_set_bittiming;
> +	priv->can.bittiming_const = hwcap->bt_const;
> +	priv->can.do_set_bittiming = hwcap->set_bittiming;
>  	priv->can.do_set_mode = grcan_set_mode;
>  	priv->can.do_get_berr_counter = grcan_get_berr_counter;
>  	priv->can.clock.freq = ambafreq;
>  	priv->can.ctrlmode_supported =
>  		CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_ONE_SHOT;
>  	priv->need_txbug_workaround = txbug;
> +	priv->hwcap = hwcap;
>
>  	/* Discover if triple sampling is supported by hardware */
>  	regs = priv->regs;
> @@ -1644,22 +1770,32 @@ static int grcan_probe(struct platform_device *ofdev)
>  {
>  	struct device_node *np = ofdev->dev.of_node;
>  	struct device_node *sysid_parent;
> +	const struct of_device_id *of_id;
> +	const struct grcan_hwcap *hwcap = &grcan_hwcap;
                                          ^^^^^^^^^^^^^

Why do you initialize this variable?

The networking subsystem thinks that the variables should be placed in
reverse-christmas-tree style. Here it should be possible to arrange it
like this:

>  {
> +	const struct grcan_hwcap *hwcap = &grcan_hwcap;
>  	struct device_node *np = ofdev->dev.of_node;
>  	struct device_node *sysid_parent;
> +	const struct of_device_id *of_id;


>  	struct clk *clk;
>  	u32 sysid, ambafreq;
>  	int irq, err;
>  	void __iomem *base;
>  	bool txbug = true;
>
> +	of_id = of_match_device(grcan_match, &ofdev->dev);
> +	if (of_id && of_id->data)
> +		hwcap = (struct grcan_hwcap *)of_id->data;
> +

please use device_get_match_data(&ofdev->dev);

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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists