On 1/5/24 11:23 AM, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The runtime PM implementation will disable clocks at the end of
> ravb_probe(). As some IP variants switch to reset mode as a result of
> setting module standby through clock disable APIs, to implement runtime PM
> the resource parsing and requesting are moved in the probe function and IP
> settings are moved in the open function. This is done because at the end of
> the probe some IP variants will switch anyway to reset mode and the
> registers content is lost. Also keeping only register specific operations
> in the ravb_open()/ravb_close() functions will make them faster.
>
> Commit moves IRQ requests to ravb_probe() to have all the IRQs ready when
> the interface is open. As now IRQs gets and requests are in a single place
> there is no need to keep intermediary data (like ravb_rx_irqs[] and
> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>
> This is a preparatory change to add runtime PM support for all IP variants.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..e3506888cca6 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1089,10 +1089,6 @@ struct ravb_private {
> int msg_enable;
> int speed;
> int emac_irq;
> - int erra_irq;
> - int mgmta_irq;
> - int rx_irqs[NUM_RX_QUEUE];
> - int tx_irqs[NUM_TX_QUEUE];
Good! :-)
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4673cc2faec0..ac6488ffa29a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1727,85 +1717,21 @@ static const struct ethtool_ops ravb_ethtool_ops = {
> .set_wol = ravb_set_wol,
> };
>
> -static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
> - struct net_device *ndev, struct device *dev,
> - const char *ch)
> -{
> - char *name;
> - int error;
> -
> - name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
Ugh! Should've fixed this outrage... :-/
[...]
> @@ -2616,6 +2509,90 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
> }
> }
>
> +static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
> + const char *ch, int *irq, irq_handler_t handler)
> +{
> + struct platform_device *pdev = priv->pdev;
> + struct net_device *ndev = priv->ndev;
> + struct device *dev = &pdev->dev;
> + const char *dev_name;
> + unsigned long flags;
> + int error;
> +
> + if (irq_name) {
> + dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> + if (!dev_name)
> + return -ENOMEM;
> +
> + *irq = platform_get_irq_byname(pdev, irq_name);
> + flags = 0;
> + } else {
> + dev_name = ndev->name;
> + *irq = platform_get_irq(pdev, 0);
> + flags = IRQF_SHARED;
> + }
> + if (*irq < 0)
> + return *irq;
> +
> + error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
> + if (error)
> + netdev_err(ndev, "cannot request IRQ %s\n", irq_name);
What will be printed when irq_name is NULL? Shouldn't this be dev_name
instead?
> +
> + return error;
> +}
> +
> +static int ravb_setup_irqs(struct ravb_private *priv)
> +{
> + const struct ravb_hw_info *info = priv->info;
> + struct net_device *ndev = priv->ndev;
> + const char *irq_name, *emac_irq_name;
> + int error, irq;
> +
> + if (!info->multi_irqs)
> + return ravb_setup_irq(priv, NULL, NULL, &ndev->irq, ravb_interrupt);
> +
> + if (info->err_mgmt_irqs) {
> + irq_name = "dia";
> + emac_irq_name = "line3";
> + } else {
> + irq_name = "ch22";
> + emac_irq_name = "ch24";
> + }
> +
> + error = ravb_setup_irq(priv, irq_name, "ch22:multi", &ndev->irq, ravb_multi_interrupt);
> + if (error)
> + return error;
> +
> + error = ravb_setup_irq(priv, emac_irq_name, "ch24:emac", &priv->emac_irq,
> + ravb_emac_interrupt);
> + if (error)
> + return error;
> +
> + if (info->err_mgmt_irqs) {
> + error = ravb_setup_irq(priv, "err_a", "err_a", &irq, ravb_multi_interrupt);
Hm, why pass 2 identical names?
> + if (error)
> + return error;
> +
> + error = ravb_setup_irq(priv, "mgmt_a", "mgmt_a", &irq, ravb_multi_interrupt);
Here as well?
> + if (error)
> + return error;
> + }
> +
> + error = ravb_setup_irq(priv, "ch0", "ch0:rx_be", &irq, ravb_be_interrupt);
Hm, won't this result in "ch0:ch0:rx_be" as IRQ name?
> + if (error)
> + return error;
> +
> + error = ravb_setup_irq(priv, "ch1", "ch1:rx_nc", &irq, ravb_nc_interrupt);
Same question...
> + if (error)
> + return error;
> +
> + error = ravb_setup_irq(priv, "ch18", "ch18:tx_be", &irq, ravb_be_interrupt);
And here as well...
> + if (error)
> + return error;
> +
> + return ravb_setup_irq(priv, "ch19", "ch19:tx_nc", &irq, ravb_nc_interrupt);
Here too...
[...]
MBR, Sergey
