On 26/03/2025 at 21:19, Biju Das wrote:
> The shift values in RCANFD_GAFLCFG_SETRNC are dictated by the field width:
> - R-Car Gen4 packs 2 values in a 32-bit word, using a field width
> of 16 bits,
> - R-Car Gen3 packs up to 4 values in a 32-bit word, using a field
> width of 8 bits.
>
> Add rnc_field_width variable to struct rcar_canfd_hw_info to handle this
> difference and simplify the shift value in RCANFD_GAFLCFG_SETRNC macro by
> using a formula (32 - (n % rnc_stride + 1) * rnc_field_width).
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v6->7:
> * Collected tag.
> v5->6:
> * Replaced RCANFD_RNC_PER_REG macro with rnc_stride variable.
> * Updated commit description
> * Dropped the Rb tag.
> v5:
> * New patch.
> ---
> drivers/net/can/rcar/rcar_canfd.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 0001c8043c25..62cde1efa0c0 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -89,7 +89,7 @@
> /* RSCFDnCFDGAFLCFG0 / RSCFDnGAFLCFG0 */
> #define RCANFD_GAFLCFG_SETRNC(gpriv, n, x) \
> (((x) & ((gpriv)->info->num_supported_rules - 1)) << \
> - (reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8)))
> + (32 - (((n) % (gpriv)->info->rnc_stride + 1) * (gpriv)->info->rnc_field_width)))
I can not follow how this is the same. Let's take the gen4 as an
example. Before:
(reg_gen4(gpriv, 16, 24) - ((n) & 1) * reg_gen4(gpriv, 16, 8))) =
16 - ((n & 1) * 16)
So, I have:
n shift value
---------------------------------
0 16 - ((0 & 1) * 16) = 0
1 16 - ((1 & 1) * 16) = 16
2 16 - ((2 & 1) * 16) = 0
3 16 - ((3 & 1) * 16) = 16
4 16 - ((4 & 1) * 16) = 0
After:
(32 - ((n % rnc_stride + 1)) * rnc_field_width) =
32 - ((n % (2 + 1)) * 16) =
32 - ((n % 3) * 16)
n shift value
---------------------------------
0 32 - ((0 % 3) * 16) = 32
1 32 - ((1 % 3) * 16) = 16
2 32 - ((2 % 3) * 16) = 0
3 32 - ((3 % 3) * 16) = 32
4 32 - ((4 % 3) * 16) = 16
Is there something wrong in my calculation? What am I missing?
More generally, it is really hard to review and understand what this
macro does.
Can add one more patch:
can: rcar_canfd: turn RCANFD_GAFLCFG_SETRNC into a function
and then apply your change?
I do not see the reason why this needs to be a macro. If you make this a
function, at least, it will be easier to follow what is going on and the
compiler optimizer will inline it anyway so you should not get any penalty.
> /* RSCFDnCFDGAFLECTR / RSCFDnGAFLECTR */
> #define RCANFD_GAFLECTR_AFLDAE BIT(8)
> @@ -506,6 +506,7 @@ struct rcar_canfd_global;
> struct rcar_canfd_hw_info {
> u16 num_supported_rules;
> u8 rnc_stride;
> + u8 rnc_field_width;
Following my previous comment on patch #7, if you replace
rcar_canfd_hw_info->rnc_stride by sizeof_rnc, then you can get the width
with:
rcar_canfd_hw_info->sizeof_rnc * BITS_PER_BYTE
> u8 max_channels;
> u8 postdiv;
> /* hardware features */
> @@ -584,6 +585,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,
> + .rnc_field_width = 8,
> .max_channels = 2,
> .postdiv = 2,
> .shared_global_irqs = 1,
> @@ -592,6 +594,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,
> + .rnc_field_width = 16,
> .max_channels = 8,
> .postdiv = 2,
> .shared_global_irqs = 1,
> @@ -600,6 +603,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,
> + .rnc_field_width = 8,
> .max_channels = 2,
> .postdiv = 1,
> .multi_channel_irqs = 1,
Yours sincerely,
Vincent Mailhol
