| Message ID | 20221025155657.1426948-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | R-Car CANFD fixes | expand |
On 25.10.2022 16:56:56, Biju Das wrote: > RZ/G2L has separate channel specific IRQs for transmit and error > interrupt. But the IRQ handler, process the code for both channels > even if there is no interrupt occurred on one of the channels. > > This patch fixes the issue by passing channel specific context > parameter instead of global one for irq register and on irq handler, > it just handles the channel which is triggered the interrupt. Please clean up signatures of the IRQ handlers you touch, it's a little mess. Change: | rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32 ch) to: | rcar_canfd_handle_channel_tx(struct rcar_canfd_channel *priv) Same for: | static void rcar_canfd_handle_channel_err(struct rcar_canfd_global *gpriv, u32 ch) In a separate patch, please clean up these, too: | static void rcar_canfd_handle_global_err(struct rcar_canfd_global *gpriv, u32 ch) | static void rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u32 ch) | static void rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch) Why are 2 of the above functions called "global" as they work on a specific channel? That can be streamlined, too. Marc
Hi Marc, Thanks for the review. > Subject: Re: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ > handling for RZ/G2L > > On 25.10.2022 16:56:56, Biju Das wrote: > > RZ/G2L has separate channel specific IRQs for transmit and error > > interrupt. But the IRQ handler, process the code for both channels > > even if there is no interrupt occurred on one of the channels. > > > > This patch fixes the issue by passing channel specific context > > parameter instead of global one for irq register and on irq handler, > > it just handles the channel which is triggered the interrupt. > > Please clean up signatures of the IRQ handlers you touch, it's a > little mess. Change: > > | rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32 > ch) > > to: > > | rcar_canfd_handle_channel_tx(struct rcar_canfd_channel *priv) > > Same for: > > | static void rcar_canfd_handle_channel_err(struct rcar_canfd_global > | *gpriv, u32 ch) > OK. > > > In a separate patch, please clean up these, too: > > | static void rcar_canfd_handle_global_err(struct rcar_canfd_global > | *gpriv, u32 ch) static void rcar_canfd_handle_global_receive(struct > | rcar_canfd_global *gpriv, u32 ch) static void > | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch) > > Why are 2 of the above functions called "global" as they work on a > specific channel? That can be streamlined, too. > The function name is as per the hardware manual, Interrupt sources are classified into global and channel interrupts. • Global interrupts (2 sources): — Receive FIFO interrupt — Global error interrupt • Channel interrupts (3 sources/channel): Maybe we could change "rcar_canfd_handle_global_receive"->"rcar_canfd_handle_channel_receive", as from driver point It is not global anymore?? Please let me know. Cheers, Biju
On 26.10.2022 09:34:41, Biju Das wrote: > > In a separate patch, please clean up these, too: > > > > | static void rcar_canfd_handle_global_err(struct rcar_canfd_global > > | *gpriv, u32 ch) static void rcar_canfd_handle_global_receive(struct > > | rcar_canfd_global *gpriv, u32 ch) static void > > | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch) > > > > Why are 2 of the above functions called "global" as they work on a > > specific channel? That can be streamlined, too. > > > > The function name is as per the hardware manual, Interrupt sources are > classified into global and channel interrupts. > > • Global interrupts (2 sources): > — Receive FIFO interrupt > — Global error interrupt > • Channel interrupts (3 sources/channel): I see. Keep the functions as is. > Maybe we could change > "rcar_canfd_handle_global_receive"->"rcar_canfd_handle_channel_receive", > as from driver point It is not global anymore?? Please let me know. Never mind - the gpriv and channel numbers are needed sometimes even in the functions working on a single channel. Never mind. I'll take patches 1 and 2 as they are. regards, Marc
Hi Marc, Thanks for the feedback. > Subject: Re: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ > handling for RZ/G2L > > On 26.10.2022 09:34:41, Biju Das wrote: > > > In a separate patch, please clean up these, too: > > > > > > | static void rcar_canfd_handle_global_err(struct > rcar_canfd_global > > > | *gpriv, u32 ch) static void > > > | rcar_canfd_handle_global_receive(struct > > > | rcar_canfd_global *gpriv, u32 ch) static void > > > | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 > ch) > > > > > > Why are 2 of the above functions called "global" as they work on a > > > specific channel? That can be streamlined, too. > > > > > > > The function name is as per the hardware manual, Interrupt sources > are > > classified into global and channel interrupts. > > > > • Global interrupts (2 sources): > > — Receive FIFO interrupt > > — Global error interrupt > > • Channel interrupts (3 sources/channel): > > I see. Keep the functions as is. > > > Maybe we could change > > "rcar_canfd_handle_global_receive"- > >"rcar_canfd_handle_channel_receive > > ", as from driver point It is not global anymore?? Please let me > know. > > Never mind - the gpriv and channel numbers are needed sometimes even > in the functions working on a single channel. Never mind. I'll take > patches > 1 and 2 as they are. OK, Thanks. Cheers, Biju
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index ea828c1bd3a1..198da643ee6d 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -1246,11 +1246,9 @@ static void rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32 ch static irqreturn_t rcar_canfd_channel_tx_interrupt(int irq, void *dev_id) { - struct rcar_canfd_global *gpriv = dev_id; - u32 ch; + struct rcar_canfd_channel *priv = dev_id; - for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) - rcar_canfd_handle_channel_tx(gpriv, ch); + rcar_canfd_handle_channel_tx(priv->gpriv, priv->channel); return IRQ_HANDLED; } @@ -1278,11 +1276,9 @@ static void rcar_canfd_handle_channel_err(struct rcar_canfd_global *gpriv, u32 c static irqreturn_t rcar_canfd_channel_err_interrupt(int irq, void *dev_id) { - struct rcar_canfd_global *gpriv = dev_id; - u32 ch; + struct rcar_canfd_channel *priv = dev_id; - for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) - rcar_canfd_handle_channel_err(gpriv, ch); + rcar_canfd_handle_channel_err(priv->gpriv, priv->channel); return IRQ_HANDLED; } @@ -1723,6 +1719,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, priv->ndev = ndev; priv->base = gpriv->base; priv->channel = ch; + priv->gpriv = gpriv; priv->can.clock.freq = fcan_freq; dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq); @@ -1751,7 +1748,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, } err = devm_request_irq(&pdev->dev, err_irq, rcar_canfd_channel_err_interrupt, 0, - irq_name, gpriv); + irq_name, priv); if (err) { dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n", err_irq, err); @@ -1765,7 +1762,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, } err = devm_request_irq(&pdev->dev, tx_irq, rcar_canfd_channel_tx_interrupt, 0, - irq_name, gpriv); + irq_name, priv); if (err) { dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n", tx_irq, err); @@ -1791,7 +1788,6 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, priv->can.do_set_mode = rcar_canfd_do_set_mode; priv->can.do_get_berr_counter = rcar_canfd_get_berr_counter; - priv->gpriv = gpriv; SET_NETDEV_DEV(ndev, &pdev->dev); netif_napi_add_weight(ndev, &priv->napi, rcar_canfd_rx_poll,
RZ/G2L has separate channel specific IRQs for transmit and error interrupt. But the IRQ handler, process the code for both channels even if there is no interrupt occurred on one of the channels. This patch fixes the issue by passing channel specific context parameter instead of global one for irq register and on irq handler, it just handles the channel which is triggered the interrupt. Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family") Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * No change --- drivers/net/can/rcar/rcar_canfd.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)