Thanks for your review.
> On 01/31/2022 3:08 AM Vincent MAILHOL <mailhol.vincent@xxxxxxxxxx> 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.
CU
Uli
