On 26/03/2025 at 21:19, Biju Das wrote:
> R-Car Gen4 packs 2 RNC values in a 32-bit word, whereas R-Car Gen3 packs
> up to 4 values in a 32-bit word. Handle this difference by adding
> rnc_stride variable to struct rcar_canfd_hw_info and update the macro
> RCANFD_GAFLCFG.
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v6->v7:
> * Collected tag.
> v5->v6:
> * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
> * Updated commit description
> * Dropped Rb tag.
> v5:
> * New patch.
> ---
> drivers/net/can/rcar/rcar_canfd.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 32d700962d69..0001c8043c25 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -291,7 +291,7 @@
> /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
> #define RCANFD_GAFLECTR (0x0098)
> /* RSCFDnCFDGAFLCFG / RSCFDnGAFLCFG */
> -#define RCANFD_GAFLCFG(ch) (0x009c + (0x04 * ((ch) / 2)))
> +#define RCANFD_GAFLCFG(gpriv, ch) (0x009c + (0x04 * ((ch) / (gpriv)->info->rnc_stride)))
I find it rather hard to follow the logic here. Your are multiplying by
four and then dividing again to get the good results. Here, I guess that
0x04 represents sizeof(u32), but this needs some thinking to figure that
out.
Wouldn't it be more intuitive to instead store the size in bytes of the
RNC value?
#define RCANFD_GAFLCFG(gpriv, ch) \
(0x009c + (gpriv)->info->sizeof_rnc * (ch))
This way, no more 0x04 magic number and it is easier to process a
multiplication than a division in your head when reading the code.
> /* RSCFDnCFDRMNB / RSCFDnRMNB */
> #define RCANFD_RMNB (0x00a4)
> /* RSCFDnCFDRMND / RSCFDnRMND */
> @@ -505,6 +505,7 @@ struct rcar_canfd_global;
>
> struct rcar_canfd_hw_info {
> u16 num_supported_rules;
> + u8 rnc_stride;
> u8 max_channels;
> u8 postdiv;
> /* hardware features */
> @@ -582,6 +583,7 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
>
> static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
> .num_supported_rules = 256,
> + .rnc_stride = 4,
> .max_channels = 2,
> .postdiv = 2,
> .shared_global_irqs = 1,
> @@ -589,6 +591,7 @@ static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>
> static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
> .num_supported_rules = 512,
> + .rnc_stride = 2,
> .max_channels = 8,
> .postdiv = 2,
> .shared_global_irqs = 1,
> @@ -596,6 +599,7 @@ static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>
> static const struct rcar_canfd_hw_info rzg2l_hw_info = {
> .num_supported_rules = 256,
> + .rnc_stride = 4,
> .max_channels = 2,
> .postdiv = 1,
> .multi_channel_irqs = 1,
> @@ -797,7 +801,7 @@ static void rcar_canfd_configure_afl_rules(struct rcar_canfd_global *gpriv,
> RCANFD_GAFLECTR_AFLDAE));
>
> /* Write number of rules for channel */
> - rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(ch),
> + rcar_canfd_set_bit(gpriv->base, RCANFD_GAFLCFG(gpriv, ch),
> RCANFD_GAFLCFG_SETRNC(gpriv, ch, num_rules));
> if (is_gen4(gpriv))
> offset = RCANFD_GEN4_GAFL_OFFSET;
Yours sincerely,
Vincent Mailhol
