Re: [PATCH 2/6] can: rcar_canfd: Add support for RZ/G2L family - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Rob Herring <robh+dt@kernel.org>,
	Wolfgang Grandegger <wg@grandegger.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	linux-can@vger.kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	Prabhakar <prabhakar.csengg@gmail.com>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH 2/6] can: rcar_canfd: Add support for RZ/G2L family
Date: Fri, 16 Jul 2021 12:10:01 +0200	[thread overview]
Message-ID: <20210716101001.m5sgit3l354mljai@pengutronix.de> (raw)
In-Reply-To: <20210715182123.23372-3-prabhakar.mahadev-lad.rj@bp.renesas.com>

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

On 15.07.2021 19:21:19, Lad Prabhakar wrote:
> CANFD block on RZ/G2L SoC is almost identical to one found on
> R-Car Gen3 SoC's.
> 
> On RZ/G2L SoC interrupt sources for each channel are split into
> different sources, irq handlers for the same are added.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for the patch! Some nitpicks inline, Geert already commented to
use the same IRQ handler for all interrutps.

> ---
>  drivers/net/can/rcar/rcar_canfd.c | 275 ++++++++++++++++++++++++++----
>  1 file changed, 244 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 311e6ca3bdc4..5dfbc5fa2d81 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -37,9 +37,13 @@
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/iopoll.h>
> +#include <linux/reset.h>
>  
>  #define RCANFD_DRV_NAME			"rcar_canfd"
>  
> +#define RENESAS_RCAR_GEN3	0
> +#define RENESAS_RZG2L		1
> +

Please make this an enum.

>  /* Global register bits */
>  
>  /* RSCFDnCFDGRMCFG */
> @@ -513,6 +517,9 @@ struct rcar_canfd_global {
>  	enum rcar_canfd_fcanclk fcan;	/* CANFD or Ext clock */
>  	unsigned long channels_mask;	/* Enabled channels mask */
>  	bool fdmode;			/* CAN FD or Classical CAN only mode */
> +	struct reset_control *rstc1;     /* Pointer to reset source1 */
> +	struct reset_control *rstc2;     /* Pointer to reset source2 */
> +	unsigned int chip_id;

enum here, too

>  };
>  
>  /* CAN FD mode nominal rate constants */
> @@ -1070,6 +1077,56 @@ static void rcar_canfd_tx_done(struct net_device *ndev)
>  	can_led_event(ndev, CAN_LED_EVENT_TX);
>  }
>  
[...]

> @@ -1635,8 +1784,11 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	struct rcar_canfd_global *gpriv;
>  	struct device_node *of_child;
>  	unsigned long channels_mask = 0;
> -	int err, ch_irq, g_irq;
> +	int err, ch_irq, g_irq, g_rx_irq;
>  	bool fdmode = true;			/* CAN FD only mode - default */
> +	unsigned int chip_id;
> +
> +	chip_id = (uintptr_t)of_device_get_match_data(&pdev->dev);

The cast looks wrong.

>  
>  	if (of_property_read_bool(pdev->dev.of_node, "renesas,no-can-fd"))
>  		fdmode = false;			/* Classical CAN only mode */
> @@ -1649,27 +1801,56 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	if (of_child && of_device_is_available(of_child))
>  		channels_mask |= BIT(1);	/* Channel 1 */
>  
> -	ch_irq = platform_get_irq(pdev, 0);
> -	if (ch_irq < 0) {
> -		err = ch_irq;
> -		goto fail_dev;
> -	}
> +	if (chip_id == RENESAS_RCAR_GEN3) {
> +		ch_irq = platform_get_irq(pdev, 0);
> +		if (ch_irq < 0)
> +			return ch_irq;
>  
> -	g_irq = platform_get_irq(pdev, 1);
> -	if (g_irq < 0) {
> -		err = g_irq;
> -		goto fail_dev;
> +		g_irq = platform_get_irq(pdev, 1);
> +		if (g_irq < 0)
> +			return g_irq;
> +	} else {
> +		g_irq = platform_get_irq(pdev, 0);
> +		if (g_irq < 0)
> +			return g_irq;
> +
> +		g_rx_irq = platform_get_irq(pdev, 1);
> +		if (g_rx_irq < 0)
> +			return g_rx_irq;
>  	}
>  
>  	/* Global controller context */
>  	gpriv = devm_kzalloc(&pdev->dev, sizeof(*gpriv), GFP_KERNEL);
> -	if (!gpriv) {
> -		err = -ENOMEM;
> -		goto fail_dev;
> -	}
> +	if (!gpriv)
> +		return -ENOMEM;
> +
>  	gpriv->pdev = pdev;
>  	gpriv->channels_mask = channels_mask;
>  	gpriv->fdmode = fdmode;
> +	gpriv->chip_id = chip_id;
> +
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		gpriv->rstc1 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0);
> +		if (IS_ERR(gpriv->rstc1)) {
> +			dev_err(&pdev->dev, "failed to get reset index 0\n");
> +			return PTR_ERR(gpriv->rstc1);
> +		}
> +
> +		err = reset_control_reset(gpriv->rstc1);
> +		if (err)
> +			return err;
> +
> +		gpriv->rstc2 = devm_reset_control_get_exclusive_by_index(&pdev->dev, 1);
> +		if (IS_ERR(gpriv->rstc2)) {
> +			dev_err(&pdev->dev, "failed to get reset index 1\n");
> +			return PTR_ERR(gpriv->rstc2);
> +		}
> +		err = reset_control_reset(gpriv->rstc2);
> +		if (err) {
> +			reset_control_assert(gpriv->rstc1);
> +			return err;
> +		}
> +	}
>  
>  	/* Peripheral clock */
>  	gpriv->clkp = devm_clk_get(&pdev->dev, "fck");
> @@ -1699,7 +1880,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	}
>  	fcan_freq = clk_get_rate(gpriv->can_clk);
>  
> -	if (gpriv->fcan == RCANFD_CANFDCLK)
> +	if (gpriv->fcan == RCANFD_CANFDCLK && gpriv->chip_id == RENESAS_RCAR_GEN3)
>  		/* CANFD clock is further divided by (1/2) within the IP */
>  		fcan_freq /= 2;
>  
> @@ -1711,21 +1892,43 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	gpriv->base = addr;
>  
>  	/* Request IRQ that's common for both channels */
> -	err = devm_request_irq(&pdev->dev, ch_irq,
> -			       rcar_canfd_channel_interrupt, 0,
> -			       "canfd.chn", gpriv);
> -	if (err) {
> -		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> -			ch_irq, err);
> -		goto fail_dev;
> -	}
> -	err = devm_request_irq(&pdev->dev, g_irq,
> -			       rcar_canfd_global_interrupt, 0,
> -			       "canfd.gbl", gpriv);
> -	if (err) {
> -		dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> -			g_irq, err);
> -		goto fail_dev;
> +	if (gpriv->chip_id == RENESAS_RCAR_GEN3) {
> +		err = devm_request_irq(&pdev->dev, ch_irq,
> +				       rcar_canfd_channel_interrupt, 0,
> +				       "canfd.chn", gpriv);
> +		if (err) {
> +			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +				ch_irq, err);
> +			goto fail_dev;
> +		}
> +
> +		err = devm_request_irq(&pdev->dev, g_irq,
> +				       rcar_canfd_global_interrupt, 0,
> +				       "canfd.gbl", gpriv);
> +		if (err) {
> +			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +				g_irq, err);
> +			goto fail_dev;
> +		}
> +	} else {
> +		err = devm_request_irq(&pdev->dev, g_rx_irq,
> +				       rcar_canfd_global_recieve_fifo_interrupt, 0,
> +				       "canfd.gblrx", gpriv);
> +
> +		if (err) {
> +			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +				g_rx_irq, err);
> +			goto fail_dev;
> +		}
> +
> +		err = devm_request_irq(&pdev->dev, g_irq,
> +				       rcar_canfd_global_err_interrupt, 0,
> +				       "canfd.gblerr", gpriv);
> +		if (err) {
> +			dev_err(&pdev->dev, "devm_request_irq(%d) failed, error %d\n",
> +				g_irq, err);
> +			goto fail_dev;
> +		}
>  	}
>  
>  	/* Enable peripheral clock for register access */
> @@ -1791,6 +1994,10 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  fail_clk:
>  	clk_disable_unprepare(gpriv->clkp);
>  fail_dev:
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		reset_control_assert(gpriv->rstc1);
> +		reset_control_assert(gpriv->rstc2);

reset_control_assert() can handle NULL pointers

> +	}
>  	return err;
>  }
>  
> @@ -1810,6 +2017,11 @@ static int rcar_canfd_remove(struct platform_device *pdev)
>  	/* Enter global sleep mode */
>  	rcar_canfd_set_bit(gpriv->base, RCANFD_GCTR, RCANFD_GCTR_GSLPR);
>  	clk_disable_unprepare(gpriv->clkp);
> +	if (gpriv->chip_id == RENESAS_RZG2L) {
> +		reset_control_assert(gpriv->rstc1);
> +		reset_control_assert(gpriv->rstc2);
> +	}

same here

> +
>  	return 0;
>  }
>  
> @@ -1827,7 +2039,8 @@ static SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
>  			 rcar_canfd_resume);
>  
>  static const struct of_device_id rcar_canfd_of_table[] = {
> -	{ .compatible = "renesas,rcar-gen3-canfd" },
> +	{ .compatible = "renesas,rcar-gen3-canfd", .data = (void *)RENESAS_RCAR_GEN3 },
> +	{ .compatible = "renesas,rzg2l-canfd", .data = (void *)RENESAS_RZG2L },
>  	{ }
>  };

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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