All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] can: flexcan: add TX support for variable payload size
@ 2018-12-12  6:46 Joakim Zhang
  2019-01-17  6:26 ` Joakim Zhang
  2019-02-27 14:03 ` Marc Kleine-Budde
  0 siblings, 2 replies; 5+ messages in thread
From: Joakim Zhang @ 2018-12-12  6:46 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: wg, netdev, linux-kernel, dl-linux-imx, Joakim Zhang

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);
+	}
 
 	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;
+	void __iomem *reg_iflag;
 	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;
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* RE: [PATCH] can: flexcan: add TX support for variable payload size
  2018-12-12  6:46 [PATCH] can: flexcan: add TX support for variable payload size Joakim Zhang
@ 2019-01-17  6:26 ` Joakim Zhang
  2019-02-14  9:57   ` Joakim Zhang
  2019-02-27 14:03 ` Marc Kleine-Budde
  1 sibling, 1 reply; 5+ messages in thread
From: Joakim Zhang @ 2019-01-17  6:26 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: wg, netdev, linux-kernel, dl-linux-imx


Kindly Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2018年12月12日 14:47
> To: mkl@pengutronix.de; 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>; Joakim
> Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH] can: flexcan: add TX support for variable payload size
> 
> 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);
> +	}
> 
>  	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;
> +	void __iomem *reg_iflag;
>  	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;
> 
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH] can: flexcan: add TX support for variable payload size
  2019-01-17  6:26 ` Joakim Zhang
@ 2019-02-14  9:57   ` Joakim Zhang
  0 siblings, 0 replies; 5+ messages in thread
From: Joakim Zhang @ 2019-02-14  9:57 UTC (permalink / raw)
  To: mkl, linux-can; +Cc: wg, netdev, linux-kernel, dl-linux-imx


Kindly Ping...

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang
> Sent: 2019年1月17日 14:26
> To: mkl@pengutronix.de; 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
> 
> 
> Kindly Ping...
> 
> Best Regards,
> Joakim Zhang
> 
> > -----Original Message-----
> > From: Joakim Zhang
> > Sent: 2018年12月12日 14:47
> > To: mkl@pengutronix.de; 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>; Joakim
> > Zhang <qiangqing.zhang@nxp.com>
> > Subject: [PATCH] can: flexcan: add TX support for variable payload
> > size
> >
> > 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);
> > +	}
> >
> >  	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;
> > +	void __iomem *reg_iflag;
> >  	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;
> >
> > --
> > 2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] can: flexcan: add TX support for variable payload size
  2018-12-12  6:46 [PATCH] can: flexcan: add TX support for variable payload size Joakim Zhang
  2019-01-17  6:26 ` Joakim Zhang
@ 2019-02-27 14:03 ` Marc Kleine-Budde
  2019-02-28  3:43   ` Joakim Zhang
  1 sibling, 1 reply; 5+ messages in thread
From: Marc Kleine-Budde @ 2019-02-27 14:03 UTC (permalink / raw)
  To: Joakim Zhang, linux-can; +Cc: wg, netdev, linux-kernel, dl-linux-imx


[-- Attachment #1.1: Type: text/plain, Size: 4992 bytes --]

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?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH] can: flexcan: add TX support for variable payload size
  2019-02-27 14:03 ` Marc Kleine-Budde
@ 2019-02-28  3:43   ` Joakim Zhang
  0 siblings, 0 replies; 5+ messages in thread
From: Joakim Zhang @ 2019-02-28  3:43 UTC (permalink / raw)
  To: Marc Kleine-Budde, linux-can; +Cc: wg, netdev, linux-kernel, dl-linux-imx


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


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-28  3:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12  6:46 [PATCH] can: flexcan: add TX support for variable payload size Joakim Zhang
2019-01-17  6:26 ` Joakim Zhang
2019-02-14  9:57   ` Joakim Zhang
2019-02-27 14:03 ` Marc Kleine-Budde
2019-02-28  3:43   ` Joakim Zhang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.