diff mbox series

[net-next,v2,08/21] net: ravb: Move the IRQs get and request in the probe function

Message ID 20231214114600.2451162-9-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Commit Message

Claudiu Beznea Dec. 14, 2023, 11:45 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Move the IRQs get and request in the driver's probe function. As some IP
variants switches to reset operation mode as a result of setting module
standby through clock enable/disable APIs, to implement runtime PM the
resource parsing and requests are moved in the probe function and IP
settings are moved in the open functions. This is a preparatory change to
add runtime PM support for all IP variants.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 274 +++++++++++------------
 1 file changed, 132 insertions(+), 142 deletions(-)

Comments

Sergey Shtylyov Dec. 16, 2023, 3:53 p.m. UTC | #1 
On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Move the IRQs get and request in the driver's probe function. As some IP
> variants switches to reset operation mode as a result of setting module

   s/switches/switch/.
   Also, the manuals call this "operating mode", not to mix with one of
the modes -- "operation mode".

> standby through clock enable/disable APIs, to implement runtime PM the
> resource parsing and requests are moved in the probe function and IP

   Requesting.
   Could you explain in more detail why you need to do this?

> settings are moved in the open functions. This is a preparatory change to

   I don't see you moving anything into ravb_open() here...

> add runtime PM support for all IP variants.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 83691a0f0cc2..d7f6e8ea8e79 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);

   Ugh, I didn't realize we had the managed device API call in a function
called from ravb_open()... :-/

[...]
> @@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>  	}
>  }
>  
> +static int ravb_get_irqs(struct ravb_private *priv)
> +{
> +	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;

   You don't seem to use these as the pointers. Could be bool instead?
But even that doesn't seem necessary..

> +	const struct ravb_hw_info *info = priv->info;
> +	struct platform_device *pdev = priv->pdev;
> +	struct net_device *ndev = priv->ndev;
> +	const char *irq_name, *emac_irq_name;
> +	int i, irq;
> +
> +	if (!info->multi_irqs) {
> +		irq = platform_get_irq(pdev, 0);
> +		if (irq < 0)
> +			return irq;
> +
> +		ndev->irq = irq;
> +		return 0;
> +	}
> +
> +	if (info->err_mgmt_irqs) {
> +		irq_name = "dia";
> +		emac_irq_name = "line3";
> +		err_a_irq_name = "err_a";
> +		mgmt_a_irq_name = "mgmt_a";
> +	} else {
> +		irq_name = "ch22";
> +		emac_irq_name = "ch24";
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, irq_name);
> +	if (irq < 0)
> +		return irq;
> +	ndev->irq = irq;
> +
> +	irq = platform_get_irq_byname(pdev, emac_irq_name);
> +	if (irq < 0)
> +		return irq;
> +	priv->emac_irq = irq;
> +
> +	if (err_a_irq_name) {

   Why not just ctest info->err_mgmt_irqs here, as it was before
this patch?

> +		irq = platform_get_irq_byname(pdev, "err_a");
> +		if (irq < 0)
> +			return irq;
> +		priv->erra_irq = irq;
> +	}
> +
> +	if (mgmt_a_irq_name) {
> +		irq = platform_get_irq_byname(pdev, "mgmt_a");
> +		if (irq < 0)
> +			return irq;
> +		priv->mgmta_irq = irq;
> +	}
> +
> +	for (i = 0; i < NUM_RX_QUEUE; i++) {
> +		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
> +		if (irq < 0)
> +			return irq;
> +		priv->rx_irqs[i] = irq;
> +	}
> +	for (i = 0; i < NUM_TX_QUEUE; i++) {
> +		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
> +		if (irq < 0)
> +			return irq;
> +		priv->tx_irqs[i] = irq;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ravb_request_irqs(struct ravb_private *priv)

   I'm not sure separating getting and requesting IRQs is a good idea.
As you're switching to using the managed device API anyway, you could
save on some IRQ-related fields in the *struct* ravb_private, I think...

[...]

MBR, Sergey
Claudiu Beznea Dec. 17, 2023, 11:56 a.m. UTC | #2 
On 16.12.2023 17:53, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Move the IRQs get and request in the driver's probe function. As some IP
>> variants switches to reset operation mode as a result of setting module
> 
>    s/switches/switch/.
>    Also, the manuals call this "operating mode", not to mix with one of
> the modes -- "operation mode".

ok

> 
>> standby through clock enable/disable APIs, to implement runtime PM the
>> resource parsing and requests are moved in the probe function and IP
> 
>    Requesting.
>    Could you explain in more detail why you need to do this?

Ok, I'll update it in the next version.

> 
>> settings are moved in the open functions. This is a preparatory change to
> 
>    I don't see you moving anything into ravb_open() here...

Indeed, this is the general explanation. I'll adapt it to explain it what
has been done in the commit (as it should have been).

> 
>> add runtime PM support for all IP variants.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 83691a0f0cc2..d7f6e8ea8e79 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1731,7 +1731,7 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> 
>    Ugh, I didn't realize we had the managed device API call in a function
> called from ravb_open()... :-/
> 
> [...]
>> @@ -2616,6 +2536,127 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
>>  	}
>>  }
>>  
>> +static int ravb_get_irqs(struct ravb_private *priv)
>> +{
>> +	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;
> 
>    You don't seem to use these as the pointers. Could be bool instead?
> But even that doesn't seem necessary..

Indeed, I've messed it a bit. I'll update it in the next version.

> 
>> +	const struct ravb_hw_info *info = priv->info;
>> +	struct platform_device *pdev = priv->pdev;
>> +	struct net_device *ndev = priv->ndev;
>> +	const char *irq_name, *emac_irq_name;
>> +	int i, irq;
>> +
>> +	if (!info->multi_irqs) {
>> +		irq = platform_get_irq(pdev, 0);
>> +		if (irq < 0)
>> +			return irq;
>> +
>> +		ndev->irq = irq;
>> +		return 0;
>> +	}
>> +
>> +	if (info->err_mgmt_irqs) {
>> +		irq_name = "dia";
>> +		emac_irq_name = "line3";
>> +		err_a_irq_name = "err_a";
>> +		mgmt_a_irq_name = "mgmt_a";
>> +	} else {
>> +		irq_name = "ch22";
>> +		emac_irq_name = "ch24";
>> +	}
>> +
>> +	irq = platform_get_irq_byname(pdev, irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	ndev->irq = irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, emac_irq_name);
>> +	if (irq < 0)
>> +		return irq;
>> +	priv->emac_irq = irq;
>> +
>> +	if (err_a_irq_name) {
> 
>    Why not just ctest info->err_mgmt_irqs here, as it was before
> this patch?

I can't tell ATM what I've wanted to achieve here. Indeed, just checking
info->err_mgmt_irqs should be better.

> 
>> +		irq = platform_get_irq_byname(pdev, "err_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->erra_irq = irq;
>> +	}
>> +
>> +	if (mgmt_a_irq_name) {
>> +		irq = platform_get_irq_byname(pdev, "mgmt_a");
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->mgmta_irq = irq;
>> +	}
>> +
>> +	for (i = 0; i < NUM_RX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->rx_irqs[i] = irq;
>> +	}
>> +	for (i = 0; i < NUM_TX_QUEUE; i++) {
>> +		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
>> +		if (irq < 0)
>> +			return irq;
>> +		priv->tx_irqs[i] = irq;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ravb_request_irqs(struct ravb_private *priv)
> 
>    I'm not sure separating getting and requesting IRQs is a good idea.
> As you're switching to using the managed device API anyway, you could
> save on some IRQ-related fields in the *struct* ravb_private, I think...

I'll have a look. By keeping them separated I tried to have the code doing
the similar things grouped together, tried to keep code similar to what was
previously and tried to avoid huge functions (having parse and request in a
single function will lead, AFAICT, at a function with more lines of code
(difficult to browse in my opinion)).

Thank you for your review,
Claudiu Beznea

> 
> [...]
> 
> MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 83691a0f0cc2..d7f6e8ea8e79 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1731,7 +1731,7 @@  static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
 	name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
 	if (!name)
 		return -ENOMEM;
-	error = request_irq(irq, handler, 0, name, ndev);
+	error = devm_request_irq(dev, irq, handler, 0, name, ndev);
 	if (error)
 		netdev_err(ndev, "cannot request IRQ %s\n", name);
 
@@ -1755,63 +1755,16 @@  static int ravb_open(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
-	struct platform_device *pdev = priv->pdev;
-	struct device *dev = &pdev->dev;
 	int error;
 
 	napi_enable(&priv->napi[RAVB_BE]);
 	if (info->nc_queues)
 		napi_enable(&priv->napi[RAVB_NC]);
 
-	if (!info->multi_irqs) {
-		error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
-				    ndev->name, ndev);
-		if (error) {
-			netdev_err(ndev, "cannot request IRQ\n");
-			goto out_napi_off;
-		}
-	} else {
-		error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
-				      dev, "ch22:multi");
-		if (error)
-			goto out_napi_off;
-		error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
-				      dev, "ch24:emac");
-		if (error)
-			goto out_free_irq;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch0:rx_be");
-		if (error)
-			goto out_free_irq_emac;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
-				      ndev, dev, "ch18:tx_be");
-		if (error)
-			goto out_free_irq_be_rx;
-		error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch1:rx_nc");
-		if (error)
-			goto out_free_irq_be_tx;
-		error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
-				      ndev, dev, "ch19:tx_nc");
-		if (error)
-			goto out_free_irq_nc_rx;
-
-		if (info->err_mgmt_irqs) {
-			error = ravb_hook_irq(priv->erra_irq, ravb_multi_interrupt,
-					      ndev, dev, "err_a");
-			if (error)
-				goto out_free_irq_nc_tx;
-			error = ravb_hook_irq(priv->mgmta_irq, ravb_multi_interrupt,
-					      ndev, dev, "mgmt_a");
-			if (error)
-				goto out_free_irq_erra;
-		}
-	}
-
 	/* Device init */
 	error = ravb_dmac_init(ndev);
 	if (error)
-		goto out_free_irq_mgmta;
+		goto out_napi_off;
 	ravb_emac_init(ndev);
 
 	/* Initialise PTP Clock driver */
@@ -1832,26 +1785,6 @@  static int ravb_open(struct net_device *ndev)
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
 	ravb_stop_dma(ndev);
-out_free_irq_mgmta:
-	if (!info->multi_irqs)
-		goto out_free_irq;
-	if (info->err_mgmt_irqs)
-		free_irq(priv->mgmta_irq, ndev);
-out_free_irq_erra:
-	if (info->err_mgmt_irqs)
-		free_irq(priv->erra_irq, ndev);
-out_free_irq_nc_tx:
-	free_irq(priv->tx_irqs[RAVB_NC], ndev);
-out_free_irq_nc_rx:
-	free_irq(priv->rx_irqs[RAVB_NC], ndev);
-out_free_irq_be_tx:
-	free_irq(priv->tx_irqs[RAVB_BE], ndev);
-out_free_irq_be_rx:
-	free_irq(priv->rx_irqs[RAVB_BE], ndev);
-out_free_irq_emac:
-	free_irq(priv->emac_irq, ndev);
-out_free_irq:
-	free_irq(ndev->irq, ndev);
 out_napi_off:
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
@@ -2177,19 +2110,6 @@  static int ravb_close(struct net_device *ndev)
 
 	cancel_work_sync(&priv->work);
 
-	if (info->multi_irqs) {
-		free_irq(priv->tx_irqs[RAVB_NC], ndev);
-		free_irq(priv->rx_irqs[RAVB_NC], ndev);
-		free_irq(priv->tx_irqs[RAVB_BE], ndev);
-		free_irq(priv->rx_irqs[RAVB_BE], ndev);
-		free_irq(priv->emac_irq, ndev);
-		if (info->err_mgmt_irqs) {
-			free_irq(priv->erra_irq, ndev);
-			free_irq(priv->mgmta_irq, ndev);
-		}
-	}
-	free_irq(ndev->irq, ndev);
-
 	if (info->nc_queues)
 		napi_disable(&priv->napi[RAVB_NC]);
 	napi_disable(&priv->napi[RAVB_BE]);
@@ -2616,6 +2536,127 @@  static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
 	}
 }
 
+static int ravb_get_irqs(struct ravb_private *priv)
+{
+	const char *err_a_irq_name = NULL, *mgmt_a_irq_name = NULL;
+	const struct ravb_hw_info *info = priv->info;
+	struct platform_device *pdev = priv->pdev;
+	struct net_device *ndev = priv->ndev;
+	const char *irq_name, *emac_irq_name;
+	int i, irq;
+
+	if (!info->multi_irqs) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+
+		ndev->irq = irq;
+		return 0;
+	}
+
+	if (info->err_mgmt_irqs) {
+		irq_name = "dia";
+		emac_irq_name = "line3";
+		err_a_irq_name = "err_a";
+		mgmt_a_irq_name = "mgmt_a";
+	} else {
+		irq_name = "ch22";
+		emac_irq_name = "ch24";
+	}
+
+	irq = platform_get_irq_byname(pdev, irq_name);
+	if (irq < 0)
+		return irq;
+	ndev->irq = irq;
+
+	irq = platform_get_irq_byname(pdev, emac_irq_name);
+	if (irq < 0)
+		return irq;
+	priv->emac_irq = irq;
+
+	if (err_a_irq_name) {
+		irq = platform_get_irq_byname(pdev, "err_a");
+		if (irq < 0)
+			return irq;
+		priv->erra_irq = irq;
+	}
+
+	if (mgmt_a_irq_name) {
+		irq = platform_get_irq_byname(pdev, "mgmt_a");
+		if (irq < 0)
+			return irq;
+		priv->mgmta_irq = irq;
+	}
+
+	for (i = 0; i < NUM_RX_QUEUE; i++) {
+		irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
+		if (irq < 0)
+			return irq;
+		priv->rx_irqs[i] = irq;
+	}
+	for (i = 0; i < NUM_TX_QUEUE; i++) {
+		irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
+		if (irq < 0)
+			return irq;
+		priv->tx_irqs[i] = irq;
+	}
+
+	return 0;
+}
+
+static int ravb_request_irqs(struct ravb_private *priv)
+{
+	const struct ravb_hw_info *info = priv->info;
+	struct net_device *ndev = priv->ndev;
+	struct device *dev = &priv->pdev->dev;
+	int error;
+
+	if (!info->multi_irqs) {
+		error = devm_request_irq(dev, ndev->irq, ravb_interrupt, IRQF_SHARED,
+					 ndev->name, ndev);
+		if (error)
+			netdev_err(ndev, "cannot request IRQ\n");
+
+		return error;
+	}
+
+	error = ravb_hook_irq(ndev->irq, ravb_multi_interrupt, ndev,
+			      dev, "ch22:multi");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->emac_irq, ravb_emac_interrupt, ndev,
+			      dev, "ch24:emac");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
+			      ndev, dev, "ch0:rx_be");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
+			      ndev, dev, "ch18:tx_be");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
+			      ndev, dev, "ch1:rx_nc");
+	if (error)
+		return error;
+	error = ravb_hook_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
+			      ndev, dev, "ch19:tx_nc");
+	if (error)
+		return error;
+
+	if (!info->err_mgmt_irqs)
+		return 0;
+
+	error = ravb_hook_irq(priv->erra_irq, ravb_multi_interrupt,
+			      ndev, dev, "err_a");
+	if (error)
+		return error;
+
+	return ravb_hook_irq(priv->mgmta_irq, ravb_multi_interrupt,
+			     ndev, dev, "mgmt_a");
+}
+
 static void ravb_set_delay_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -2635,9 +2676,8 @@  static int ravb_probe(struct platform_device *pdev)
 	struct reset_control *rstc;
 	struct ravb_private *priv;
 	struct net_device *ndev;
-	int error, irq, q;
 	struct resource *res;
-	int i;
+	int error, q;
 
 	if (!np) {
 		dev_err(&pdev->dev,
@@ -2664,20 +2704,6 @@  static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_free_netdev;
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "dia");
-		else
-			irq = platform_get_irq_byname(pdev, "ch22");
-	} else {
-		irq = platform_get_irq(pdev, 0);
-	}
-	if (irq < 0) {
-		error = irq;
-		goto out_reset_assert;
-	}
-	ndev->irq = irq;
-
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	priv = netdev_priv(ndev);
@@ -2692,6 +2718,14 @@  static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	error = ravb_get_irqs(priv);
+	if (error)
+		goto out_reset_assert;
+
+	error = ravb_request_irqs(priv);
+	if (error)
+		goto out_reset_assert;
+
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
 	if (IS_ERR(priv->refclk)) {
 		error = PTR_ERR(priv->refclk);
@@ -2725,50 +2759,6 @@  static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (info->multi_irqs) {
-		if (info->err_mgmt_irqs)
-			irq = platform_get_irq_byname(pdev, "line3");
-		else
-			irq = platform_get_irq_byname(pdev, "ch24");
-		if (irq < 0) {
-			error = irq;
-			goto out_rpm_put;
-		}
-		priv->emac_irq = irq;
-		for (i = 0; i < NUM_RX_QUEUE; i++) {
-			irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->rx_irqs[i] = irq;
-		}
-		for (i = 0; i < NUM_TX_QUEUE; i++) {
-			irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->tx_irqs[i] = irq;
-		}
-
-		if (info->err_mgmt_irqs) {
-			irq = platform_get_irq_byname(pdev, "err_a");
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->erra_irq = irq;
-
-			irq = platform_get_irq_byname(pdev, "mgmt_a");
-			if (irq < 0) {
-				error = irq;
-				goto out_rpm_put;
-			}
-			priv->mgmta_irq = irq;
-		}
-	}
-
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);