On 02/06/2025 at 23:08, Geert Uytterhoeven wrote:
> Hi Vincent,
>
> On Mon, 2 Jun 2025 at 15:41, Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> wrote:
>> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
>>> The Renesas CAN-FD hardware block supports configuring Transceiver Delay
>>> Compensation, and reading back the Transceiver Delay Compensation
>>> Result, which is needed to support high transfer rates like 8 Mbps.
>>> The Secondary Sample Point is either the measured delay plus the
>>> configured offset, or just the configured offset.
>>>
>>> Fix the existing RCANFD_FDCFG_TDCO() macro for the intended use case
>>> (writing instead of reading the field). Add register definition bits
>>> for the Channel n CAN-FD Status Register.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
>>> --- a/drivers/net/can/rcar/rcar_canfd.c
>>> +++ b/drivers/net/can/rcar/rcar_canfd.c
>>> @@ -191,9 +191,19 @@
>>> /* RSCFDnCFDCmFDCFG */
>>> #define RCANFD_GEN4_FDCFG_CLOE BIT(30)
>>> #define RCANFD_GEN4_FDCFG_FDOE BIT(28)
>>> +#define RCANFD_FDCFG_TDCO GENMASK(23, 16)
>>> #define RCANFD_FDCFG_TDCE BIT(9)
>>> #define RCANFD_FDCFG_TDCOC BIT(8)
>>> -#define RCANFD_FDCFG_TDCO(x) (((x) & 0x7f) >> 16)
>>> +
>>> +/* RSCFDnCFDCmFDSTS */
>>> +#define RCANFD_FDSTS_SOC GENMASK(31, 24)
>>> +#define RCANFD_FDSTS_EOC GENMASK(23, 16)
>>> +#define RCANFD_GEN4_FDSTS_PNSTS GENMASK(13, 12)
>>> +#define RCANFD_FDSTS_SOCO BIT(9)
>>> +#define RCANFD_FDSTS_EOCO BIT(8)
>>> +#define RCANFD_FDSTS_TDCR(gpriv, x) ((x) & ((gpriv)->info->tdc_const->tdcv_max - 1))
>>> +#define RCANFD_FDSTS_TDCVF(gpriv) \
>>> + ((gpriv)->info->tdc_const->tdcv_max > 128 ? BIT(15) : BIT(7))
>>
>> See my previous comment: no more function like macro please.
>
> OK, "int rcar_canfd_get_fdsts_tdcr(gpriv, sts)".
>
> RCANFD_FDSTS_TDCVF() is unused, so I'll drop it.
>
>>> @@ -634,6 +645,25 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
>>> .brp_inc = 1,
>>> };
>>>
>>> +/* CAN FD Transmission Delay Compensation constants */
>>> +static const struct can_tdc_const rcar_canfd_gen3_tdc_const = {
>>> + .tdcv_min = 1,
>>
>> Interesting. This is the first time I see a driver with the tdcv_min and the
>> tdco_min different than 0. At one point in time, I wanted those to be implicit
>> values. Guess it was finally a good idea to include those minimums to the framework.
>
> Really? As most other timing values need subtracting 1 when programming
> the hardware, I would expect it to be rather common.
Do a
git grep "tdc[ov]_min ="
and see by yourself :)
I also expected that this might occur one day, so glad I anticipated.
>>> @@ -1477,6 +1514,22 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>>> rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
>>> netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>>> brp, sjw, tseg1, tseg2);
>>> +
>>> + /* Transceiver Delay Compensation */
>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
>>> + /* TDC enabled, measured + offset */
>>> + tdcmode = RCANFD_FDCFG_TDCE;
>>> + tdco = tdc->tdco - 1;
>>> + netdev_dbg(ndev, "tdc: auto %u\n", tdco);
>>
>> Same as previously. Are those debugs really useful? You can get the value
>> through the netlink interface (OK, you still have to do the minus one by hand,
>> but don't tell me that's the reason).
>
> No, I just mimicked the existing debug prints, which we already agreed
> upon to remove.
>
>>> + } else if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
>>> + /* TDC enabled, offset only */
>>> + tdcmode = RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
>>> + tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;
>>
>> That's an edge case I did not think of and that is thus not handled by the
>> framework. This min() is a bit hacky, but I do not see a better workaround.
>> Also, I guess that this edge case will rarely occur.
>
> can_calc_tdco() also does a silent min(..., tdc_const->tdco_max).
Good catch!
>>> + netdev_dbg(ndev, "tdc: manual %u\n", tdco);
>>> + }
>>> +
>>> + rcar_canfd_update_bit(gpriv->base, RCANFD_F_CFDCFG(gpriv, ch), mask,
>>> + tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
>>> }
Yours sincerely,
Vincent Mailhol
