lkml.org 
[lkml]   [2019]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] can: flexcan: add TX support for variable payload size
Date

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年2月27日 22:03
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] can: flexcan: add TX support for variable payload size
>
> 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.zhang@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?

Hi Marc,

flexcan_read_reg_iflag_rx() is the function to confirm the irq which RX mailbox generated, if we remove it completely here, how can we exclude that it is not a TX mailbox irq?

> >
> > 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?

Of course, we can make it a u32.

Best Regards,
Joakim Zhang

> > 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 |

\
 
 \ /
  Last update: 2019-02-28 04:44    [W:0.122 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site