Hi Vincent,
Thanks for the feedback.
> -----Original Message-----
> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> Sent: 28 March 2025 10:37
> Subject: Re: [PATCH v7 07/18] can: rcar_canfd: Add rnc_stride variable to struct rcar_canfd_hw_info
>
> 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.
Now the macro simplified as after introducing setrnc() [1]
#define RCANFD_GAFLCFG(w) (0x009c + (0x04 * (w)))
Where "w" is the index mentioned in the hardware manual.
>
> > /* 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));
By introducing setrnc(), this patch is no more needed as
rnc_stride is local variable inside strnc(). So I would like to drop this
patch in next version.
[1]
static void rcar_canfd_setrnc(struct rcar_canfd_global *gpriv, u32 ch,
int num_rules)
{
u32 shift, rnc, offset, w, rnc_stride;
rnc_stride = 32 / gpriv->info->rnc_field_width;
shift = 32 - ((ch % rnc_stride + 1) * gpriv->info->rnc_field_width);
rnc = (num_rules & (gpriv->info->num_supported_rules - 1)) << shift;
w = ch / rnc_stride;
offset = RCANFD_GAFLCFG(w);
rcar_canfd_set_bit(gpriv->base, offset, rnc);
}
Cheers,
Biju
