Hi Biju,
On Thu, 20 Feb 2025 at 10:45, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> There are a total of 96 AFL pages and each page has 16 entries with
> registers CFDGAFLIDr, CFDGAFLMr, CFDGAFLP0r, CFDGAFLP1r holding
> the rule entries (r = 0..15).
>
> Currently, RCANFD_GAFL* macros use a start variable to find AFL entries,
> which is incorrect as the testing on RZ/G3E shows ch1 and ch4
> gets a start value of 0 and the register contents are overwritten.
>
> Fix this issue by using rule_entry corresponding to the channel
> to find the page entries in the AFL list.
>
> Fixes: dd3bd23eb438 ("can: rcar_canfd: Add Renesas R-Car CAN FD driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
Thanks for your patch!
This finally fixes CAN2 and CAN3 on the White Hawk and White Hawk
Single development boards based on R-Car V4H with 8 CAN channels
(the transceivers for CAN4-7 are not mounted), so
Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
Unfortunately, it does not fix CAN2 and CAN3 on the Gray Hawk Single
development board, which is based on R-Car V4M with 4 CAN channels.
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -787,10 +787,11 @@ static void rcar_canfd_configure_controller(struct rcar_canfd_global *gpriv)
> }
>
> static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> - u32 ch)
> + u32 ch, u32 rule_entry)
> {
> u32 cfg;
> int offset, start, page, num_rules = RCANFD_CHANNEL_NUMRULES;
> + u32 rule_entry_index = rule_entry % 16;
> u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> if (ch == 0) {
The out-of-context code does:
start = 0; /* Channel 0 always starts from 0th rule */
} else {
/* Get number of Channel 0 rules and adjust */
cfg = rcar_canfd_read(gpriv->base, RCANFD_GAFLCFG(ch));
start = RCANFD_GAFLCFG_GETRNC(gpriv, 0, cfg);
}
After your changes below, "start" is set but never used.
Looking at the actual behavior of your patch, the same can be achieved
by updating start, by adding a single line here:
start += (ch & -2) * num_rules;
> @@ -802,7 +803,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> }
>
> /* Enable write access to entry */
> - page = RCANFD_GAFL_PAGENUM(start);
> + page = RCANFD_GAFL_PAGENUM(rule_entry);
> rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLECTR,
> (RCANFD_GAFLECTR_AFLPN(gpriv, page) |
> RCANFD_GAFLECTR_AFLDAE));
Out of context code:
/* Write number of rules for channel */
rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));
So the old code calculated the page number based on the pre-configured
number of rules in the RCANFD_GAFLCFGw register, not taking into
account ch > 2, and then reprogrammed the register with the new number
of rules... Hmm...
> @@ -818,13 +819,13 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> offset = RCANFD_C_GAFL_OFFSET;
>
> /* Accept all IDs */
> - rcar_canfd_write(gpriv->base, RCANFD_GAFLID(offset, start), 0);
> + rcar_canfd_write(gpriv->base, RCANFD_GAFLID(offset, rule_entry_index), 0);
> /* IDE or RTR is not considered for matching */
> - rcar_canfd_write(gpriv->base, RCANFD_GAFLM(offset, start), 0);
> + rcar_canfd_write(gpriv->base, RCANFD_GAFLM(offset, rule_entry_index), 0);
> /* Any data length accepted */
> - rcar_canfd_write(gpriv->base, RCANFD_GAFLP0(offset, start), 0);
> + rcar_canfd_write(gpriv->base, RCANFD_GAFLP0(offset, rule_entry_index), 0);
> /* Place the msg in corresponding Rx FIFO entry */
> - rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLP1(offset, start),
> + rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLP1(offset, rule_entry_index),
> RCANFD_GAFLP1_GAFLFDP(ridx));
>
> /* Disable write access to page */
> @@ -1851,6 +1852,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> unsigned long channels_mask = 0;
> int err, ch_irq, g_irq;
> int g_err_irq, g_recc_irq;
> + u32 rule_entry = 0;
> bool fdmode = true; /* CAN FD only mode - default */
> char name[9] = "channelX";
> int i;
> @@ -2023,7 +2025,8 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> rcar_canfd_configure_tx(gpriv, ch);
>
> /* Configure receive rules */
> - rcar_canfd_configure_afl_rules(gpriv, ch);
> + rcar_canfd_configure_afl_rules(gpriv, ch, rule_entry);
> + rule_entry += RCANFD_CHANNEL_NUMRULES;
> }
>
> /* Configure common interrupts */
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
