Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Ulrich Hecht <uli@fpond.eu>
Cc: Ulrich Hecht <uli+renesas@fpond.eu>,
	linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
	davem@davemloft.net, linux-can@vger.kernel.org,
	prabhakar.mahadev-lad.rj@bp.renesas.com,
	biju.das.jz@bp.renesas.com, wsa@kernel.org,
	yoshihiro.shimoda.uh@renesas.com, wg@grandegger.com,
	mkl@pengutronix.de, kuba@kernel.org, socketcan@hartkopp.net,
	geert@linux-m68k.org, kieran.bingham@ideasonboard.com
Subject: Re: [PATCH v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC
Date: Mon, 31 Jan 2022 19:46:59 +0900	[thread overview]
Message-ID: <CAMZ6RqLEO8JQV8f=xW1n9+UAgNMPp0z6W9vv6Kajh9Tvyu5vrA@mail.gmail.com> (raw)
In-Reply-To: <1747432551.1166887.1643621068539@webmail.strato.com>

On Mon. 31 Jan 2022 at 18:24, Ulrich Hecht <uli@fpond.eu> wrote:
>
> Thanks for your review.
>
> > On 01/31/2022 3:08 AM Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> > > @@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> > >
> > >         dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
> > >
> > > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > > +           gpriv->chip_id == RENESAS_R8A779A0) {
> > >                 rcar_canfd_write(priv->base,
> > >                                  RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
> > >                 rcar_canfd_write(priv->base,
> > >                                  RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
> > >
> > > -               if (can_is_canfd_skb(skb)) {
> > > +               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > > +                   can_is_canfd_skb(skb)) {
> >
> > Could you explain why this additional check is needed?
> > My understanding is that can_is_canfd_skb(skb) being true implies
> > that the CAN_CTRLMODE_FD flag is set.
>
> That might indeed be redundant.
>
> >
> > >                         /* CAN FD frame format */
> > >                         sts |= RCANFD_CFFDCSTS_CFFDF;
> > >                         if (cf->flags & CANFD_BRS)
> > > @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> > >  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
> > >  {
> > >         struct net_device_stats *stats = &priv->ndev->stats;
> > > +       struct rcar_canfd_global *gpriv = priv->gpriv;
> > >         struct canfd_frame *cf;
> > >         struct sk_buff *skb;
> > >         u32 sts = 0, id, dlc;
> > >         u32 ch = priv->channel;
> > >         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> > >
> > > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > > +           gpriv->chip_id == RENESAS_R8A779A0) {
> >
> > I guess that this is linked to the above comment. Does the
> > R8A779A0 chip support CAN-FD? If yes, why not simply use the
> > CAN_CTRLMODE_FD instead of adding this additional check?
>
> The non-V3U Gen3 CAN controllers have two different ways to be driven, depending on whether they are in classic or CAN-FD mode. The V3U controller is driven the CAN-FD way in both modes and thus needs to have this branch taken no matter what mode it is in.

Ack.
Makes sense. So actually, this isn’t related to the previous comments :)

In my previous message, I added two comments toward the macro. I
just want to double check that you have seen these because they
are missing from your reply.