Re: [PATCH] can: flexcan: add TX support for variable payload size

[Marc Kleine-Budde]

On 12/12/18 7:46 AM, Joakim Zhang wrote:
> Now the FlexCAN driver always use last mailbox for TX, it will work well
> when MB payload size is 8/16 bytes.
> TX mailbox would change to 13 when MB payload size is 64 bytes to
> support CANFD. So we may need to set iflag register to add support for
> variable payload size.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zh...@nxp.com>
> ---
>  drivers/net/can/flexcan.c | 42 +++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 0f36eafe3ac1..13fd085fcf84 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -141,7 +141,9 @@
>  #define FLEXCAN_TX_MB_RESERVED_OFF_FIFO              8
>  #define FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP 0
>  #define FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST    
> (FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP + 1)
> -#define FLEXCAN_IFLAG_MB(x)          BIT((x) & 0x1f)
> +#define FLEXCAN_IFLAG1_MB_NUM                32
> +#define FLEXCAN_IFLAG1_MB(x)         BIT(x)
> +#define FLEXCAN_IFLAG2_MB(x)         BIT((x) & 0x1f)
>  #define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW       BIT(7)
>  #define FLEXCAN_IFLAG_RX_FIFO_WARN   BIT(6)
>  #define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE      BIT(5)
> @@ -822,9 +824,15 @@ static inline u64 flexcan_read_reg_iflag_rx(struct 
> flexcan_priv *priv)
>       struct flexcan_regs __iomem *regs = priv->regs;
>       u32 iflag1, iflag2;
>  
> -     iflag2 = priv->read(&regs->iflag2) & priv->reg_imask2_default &
> -             ~FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> -     iflag1 = priv->read(&regs->iflag1) & priv->reg_imask1_default;
> +     if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> +             iflag2 = priv->read(&regs->iflag2) & priv->reg_imask2_default &
> +                     ~FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> +             iflag1 = priv->read(&regs->iflag1) & priv->reg_imask1_default;
> +     } else {
> +             iflag2 = priv->read(&regs->iflag2) & priv->reg_imask2_default;
> +             iflag1 = priv->read(&regs->iflag1) & priv->reg_imask1_default &
> +                     ~FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> +     }

I just noticed, that FLEXCAN_IFLAGx_MB(priv->tx_mb_idx) should already
be part of the corresponding imaskx_default. See flexcan_open(). So we
can remove it completely here, right?

>  
>       return (u64)iflag2 << 32 | iflag1;
>  }
> @@ -836,7 +844,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>       struct flexcan_priv *priv = netdev_priv(dev);
>       struct flexcan_regs __iomem *regs = priv->regs;
>       irqreturn_t handled = IRQ_NONE;
> -     u32 reg_iflag2, reg_esr;
> +     u32 reg_tx_iflag, tx_iflag_idx, reg_esr;

"tx_iflag_idx" is not an index (going from 0...63) but a bit-mask.

> +     void __iomem *reg_iflag;

"reg_iflag" is not a register but a pointer to a register, better rename
it. There is a "u64 reg_iflag" in the same function already, but in a
different scope. Why not make it a u32 instead of a void?

>       enum can_state last_state = priv->can.state;
>  
>       /* reception interrupt */
> @@ -870,10 +879,18 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>               }
>       }
>  
> -     reg_iflag2 = priv->read(&regs->iflag2);
> +     if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> +             reg_tx_iflag = priv->read(&regs->iflag2);
> +             tx_iflag_idx = FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> +             reg_iflag = &regs->iflag2;
> +     } else {
> +             reg_tx_iflag = priv->read(&regs->iflag1);
> +             tx_iflag_idx = FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> +             reg_iflag = &regs->iflag1;
> +     }
>  
>       /* transmission complete interrupt */
> -     if (reg_iflag2 & FLEXCAN_IFLAG_MB(priv->tx_mb_idx)) {
> +     if (reg_tx_iflag & tx_iflag_idx) {
>               u32 reg_ctrl = priv->read(&priv->tx_mb->can_ctrl);
>  
>               handled = IRQ_HANDLED;
> @@ -885,7 +902,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>               /* after sending a RTR frame MB is in RX mode */
>               priv->write(FLEXCAN_MB_CODE_TX_INACTIVE,
>                           &priv->tx_mb->can_ctrl);
> -             priv->write(FLEXCAN_IFLAG_MB(priv->tx_mb_idx), &regs->iflag2);
> +             priv->write(tx_iflag_idx, reg_iflag);
>               netif_wake_queue(dev);
>       }
>  
> @@ -1244,8 +1261,13 @@ static int flexcan_open(struct net_device *dev)
>       priv->tx_mb_idx = priv->mb_count - 1;
>       priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx);
>  
> -     priv->reg_imask1_default = 0;
> -     priv->reg_imask2_default = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> +     if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> +             priv->reg_imask1_default = 0;
> +             priv->reg_imask2_default = FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> +     } else {
> +             priv->reg_imask1_default = FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> +             priv->reg_imask2_default = 0;
> +     }
>  
>       priv->offload.mailbox_read = flexcan_mailbox_read;
>  
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

signature.asc
Description: OpenPGP digital signature