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.
> > @@ -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).
>
> > + 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));
> > }
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
