[PATCH v2 0/6] can: c_can: add support to 64 message objects

[PATCH v2 0/6] can: c_can: add support to 64 message objects

[Dario Binacchi]

The D_CAN controller supports up to 128 messages. Until now the driver
only managed 32 messages although Sitara processors and DRA7 SOC can
handle 64.

The series was tested on a beaglebone board.

Note:
I have not changed the type of tx_field (belonging to the c_can_priv
structure) to atomic64_t because I think the atomic_t type has size
of at least 32 bits on x86 and arm, which is enough to handle 64
messages.
http://marc.info/?l=linux-can&m=139746476821294&w=2 reports the results
of tests performed just on x86 and arm architectures.

Changes in v2:
- Fix compiling error reported by kernel test robot.
- Add Reported-by tag.
- Pass larger size to alloc_candev() routine to avoid an additional
  memory allocation/deallocation.
- Add message objects number to PCI driver data.

Dario Binacchi (6):
  can: c_can: remove unused code
  can: c_can: fix indentation
  can: c_can: fix control interface used by c_can_do_tx
  can: c_can: use 32-bit write to set arbitration register
  can: c_can: prepare to up the message objects number
  can: c_can: add support to 64 message objects

 drivers/net/can/c_can/c_can.c          | 77 +++++++++++++++-----------
 drivers/net/can/c_can/c_can.h          | 32 +++++------
 drivers/net/can/c_can/c_can_pci.c      |  6 +-
 drivers/net/can/c_can/c_can_platform.c |  6 +-
 4 files changed, 68 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] can: c_can: remove unused code

[Dario Binacchi]

Commit 9d23a9818cb1 ("can: c_can: Remove unused inline function") left
behind C_CAN_MSG_OBJ_TX_LAST constant.

Commit fa39b54ccf28 ("can: c_can: Get rid of pointless interrupts") left
behind C_CAN_MSG_RX_LOW_LAST and C_CAN_MSG_OBJ_RX_SPLIT constants.

The removed code also made a comment useless and misleading.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/net/can/c_can/c_can.c | 3 +--
 drivers/net/can/c_can/c_can.h | 4 ----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index ef474bae47a1..a962ceefd44a 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -848,8 +848,7 @@ static inline u32 c_can_get_pending(struct c_can_priv *priv)
  * c_can core saves a received CAN message into the first free message
  * object it finds free (starting with the lowest). Bits NEWDAT and
  * INTPND are set for this message object indicating that a new message
- * has arrived. To work-around this issue, we keep two groups of message
- * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
+ * has arrived.
  *
  * We clear the newdat bit right away.
  *
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 92213d3d96eb..90d3d2e7a086 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -32,11 +32,7 @@
 				C_CAN_MSG_OBJ_RX_NUM - 1)
 
 #define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
-#define C_CAN_MSG_OBJ_TX_LAST	(C_CAN_MSG_OBJ_TX_FIRST + \
-				C_CAN_MSG_OBJ_TX_NUM - 1)
 
-#define C_CAN_MSG_OBJ_RX_SPLIT	9
-#define C_CAN_MSG_RX_LOW_LAST	(C_CAN_MSG_OBJ_RX_SPLIT - 1)
 #define RECEIVE_OBJECT_BITS	0x0000ffff
 
 enum reg {
-- 
2.17.1

[PATCH v2 2/6] can: c_can: fix indentation

[Dario Binacchi]

Commit 524369e2391f ("can: c_can: remove obsolete STRICT_FRAME_ORDERING Kconfig option")
left behind wrong indentation, fix it.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/net/can/c_can/c_can.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index a962ceefd44a..dbcc1c1c92d6 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -786,7 +786,7 @@ static u32 c_can_adjust_pending(u32 pend)
 static inline void c_can_rx_object_get(struct net_device *dev,
 				       struct c_can_priv *priv, u32 obj)
 {
-		c_can_object_get(dev, IF_RX, obj, priv->comm_rcv_high);
+	c_can_object_get(dev, IF_RX, obj, priv->comm_rcv_high);
 }
 
 static inline void c_can_rx_finalize(struct net_device *dev,
-- 
2.17.1

[PATCH v2 3/6] can: c_can: fix control interface used by c_can_do_tx

[Dario Binacchi]

According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use
IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/net/can/c_can/c_can.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index dbcc1c1c92d6..69526c3a671c 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev)
 		idx--;
 		pend &= ~(1 << idx);
 		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
-		c_can_inval_tx_object(dev, IF_RX, obj);
+		c_can_inval_tx_object(dev, IF_TX, obj);
 		can_get_echo_skb(dev, idx, NULL);
 		bytes += priv->dlc[idx];
 		pkts++;
-- 
2.17.1

Re: [PATCH v2 3/6] can: c_can: fix control interface used by c_can_do_tx

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On 25.02.2021 22:51:52, Dario Binacchi wrote:
> According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use
> IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).

Is this a fix?

Marc

> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> ---
> 
> (no changes since v1)
> 
>  drivers/net/can/c_can/c_can.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index dbcc1c1c92d6..69526c3a671c 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev)
>  		idx--;
>  		pend &= ~(1 << idx);
>  		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> -		c_can_inval_tx_object(dev, IF_RX, obj);
> +		c_can_inval_tx_object(dev, IF_TX, obj);
>  		can_get_echo_skb(dev, idx, NULL);
>  		bytes += priv->dlc[idx];
>  		pkts++;
> -- 
> 2.17.1
> 
> 

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

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

Re: [PATCH v2 3/6] can: c_can: fix control interface used by c_can_do_tx

[Dario Binacchi]

Hi Marc,

> Il 26/02/2021 09:44 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:
> 
>  
> On 25.02.2021 22:51:52, Dario Binacchi wrote:
> > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use
> > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).
> 
> Is this a fix?
> 

I think that If I consider what is described in the 640916db2bf7 commit, using 
the IF_RX interface in a tx routine is wrong.

Thanks and regards
Dario

> Marc
> 
> > 
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > ---
> > 
> > (no changes since v1)
> > 
> >  drivers/net/can/c_can/c_can.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index dbcc1c1c92d6..69526c3a671c 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev)
> >  		idx--;
> >  		pend &= ~(1 << idx);
> >  		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > -		c_can_inval_tx_object(dev, IF_RX, obj);
> > +		c_can_inval_tx_object(dev, IF_TX, obj);
> >  		can_get_echo_skb(dev, idx, NULL);
> >  		bytes += priv->dlc[idx];
> >  		pkts++;
> > -- 
> > 2.17.1
> > 
> > 
> 
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 3/6] can: c_can: fix control interface used by c_can_do_tx

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

On 28.02.2021 11:35:31, Dario Binacchi wrote:
> > On 25.02.2021 22:51:52, Dario Binacchi wrote:
> > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use
> > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).
> > 
> > Is this a fix?
> > 
> 
> I think that If I consider what is described in the 640916db2bf7
> commit, using the IF_RX interface in a tx routine is wrong.

Yes, IF_RX is used in c_can_do_tx(), but that's called from
c_can_poll(), which runs ins NAPI.

As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe")
fixes the race condition that c_can_poll() and c_can_start_xmit() both
access the same IF. See again the patch description:

| The hardware has two message control interfaces, but the code only uses the
| first one. So on SMP the following can be observed:
| 
| CPU0            CPU1
| rx_poll()
|   write IF1     xmit()
|                 write IF1
|   write IF1

It's not 100% accurate, as the race condition is not just
c_can_do_rx_poll() against the c_can_start_xmit(), but the whole
c_can_poll() against c_can_start_xmit().

If you think my analysis is correct, please update the patch and add a
comment to clarify why IF_RX is used instead of changing it to IF_TX.

regards,
Marc

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

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

Re: [PATCH v2 3/6] can: c_can: fix control interface used by c_can_do_tx

[Dario Binacchi]

> Il 01/03/2021 12:36 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:
> 
>  
> On 28.02.2021 11:35:31, Dario Binacchi wrote:
> > > On 25.02.2021 22:51:52, Dario Binacchi wrote:
> > > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use
> > > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).
> > > 
> > > Is this a fix?
> > > 
> > 
> > I think that If I consider what is described in the 640916db2bf7
> > commit, using the IF_RX interface in a tx routine is wrong.
> 
> Yes, IF_RX is used in c_can_do_tx(), but that's called from
> c_can_poll(), which runs ins NAPI.

Yes, you are right. I was misled by the name of the function.

> 
> As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe")
> fixes the race condition that c_can_poll() and c_can_start_xmit() both
> access the same IF. See again the patch description:
> 
> | The hardware has two message control interfaces, but the code only uses the
> | first one. So on SMP the following can be observed:
> | 
> | CPU0            CPU1
> | rx_poll()
> |   write IF1     xmit()
> |                 write IF1
> |   write IF1
> 
> It's not 100% accurate, as the race condition is not just
> c_can_do_rx_poll() against the c_can_start_xmit(), but the whole
> c_can_poll() against c_can_start_xmit().
> 
> If you think my analysis is correct, please update the patch and add a
> comment to clarify why IF_RX is used instead of changing it to IF_TX.

I agree with you, I'll do it.

Thanks and regards,
Dario

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

[PATCH v2 4/6] can: c_can: use 32-bit write to set arbitration register

[Dario Binacchi]

The arbitration register is already set up with 32-bit writes in the
other parts of the code except for this point.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/net/can/c_can/c_can.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 69526c3a671c..7081cfaf62e2 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -297,8 +297,7 @@ static void c_can_inval_msg_object(struct net_device *dev, int iface, int obj)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface), 0);
-	priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface), 0);
+	priv->write_reg32(priv, C_CAN_IFACE(ARB1_REG, iface), 0);
 	c_can_inval_tx_object(dev, iface, obj);
 }
 
-- 
2.17.1

[PATCH v2 5/6] can: c_can: prepare to up the message objects number

[Dario Binacchi]

As pointed by commit c0a9f4d396c9 ("can: c_can: Reduce register access")
the "driver casts the 16 message objects in stone, which is completely
braindead as contemporary hardware has up to 128 message objects".

The patch prepares the module to extend the number of message objects
beyond the 32 currently managed. This was achieved by transforming the
constants used to manage RX/TX messages into variables without changing
the driver policy.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Reported-by: kernel test robot <lkp@intel.com>

---

Changes in v2:
- Fix compiling error reported by kernel test robot.
- Add Reported-by tag.
- Pass larger size to alloc_candev() routine to avoid an additional
  memory allocation/deallocation.

 drivers/net/can/c_can/c_can.c          | 50 ++++++++++++++++----------
 drivers/net/can/c_can/c_can.h          | 23 ++++++------
 drivers/net/can/c_can/c_can_pci.c      |  2 +-
 drivers/net/can/c_can/c_can_platform.c |  2 +-
 4 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 7081cfaf62e2..df1ad6b3fd3b 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -173,9 +173,6 @@
 /* Wait for ~1 sec for INIT bit */
 #define INIT_WAIT_MS		1000
 
-/* napi related */
-#define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
-
 /* c_can lec values */
 enum c_can_lec_type {
 	LEC_NO_ERROR = 0,
@@ -325,7 +322,7 @@ static void c_can_setup_tx_object(struct net_device *dev, int iface,
 	 * first, i.e. clear the MSGVAL flag in the arbiter.
 	 */
 	if (rtr != (bool)test_bit(idx, &priv->tx_dir)) {
-		u32 obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+		u32 obj = idx + priv->msg_obj_tx_first;
 
 		c_can_inval_msg_object(dev, iface, obj);
 		change_bit(idx, &priv->tx_dir);
@@ -463,10 +460,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	 * prioritized. The lowest buffer number wins.
 	 */
 	idx = fls(atomic_read(&priv->tx_active));
-	obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+	obj = idx + priv->msg_obj_tx_first;
 
 	/* If this is the last buffer, stop the xmit queue */
-	if (idx == C_CAN_MSG_OBJ_TX_NUM - 1)
+	if (idx == priv->msg_obj_tx_num - 1)
 		netif_stop_queue(dev);
 	/*
 	 * Store the message in the interface so we can call
@@ -549,17 +546,18 @@ static int c_can_set_bittiming(struct net_device *dev)
  */
 static void c_can_configure_msg_objects(struct net_device *dev)
 {
+	struct c_can_priv *priv = netdev_priv(dev);
 	int i;
 
 	/* first invalidate all message objects */
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
+	for (i = priv->msg_obj_rx_first; i <= priv->msg_obj_num; i++)
 		c_can_inval_msg_object(dev, IF_RX, i);
 
 	/* setup receive message objects */
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
+	for (i = priv->msg_obj_rx_first; i < priv->msg_obj_rx_last; i++)
 		c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_RCV);
 
-	c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
+	c_can_setup_receive_object(dev, IF_RX, priv->msg_obj_rx_last, 0, 0,
 				   IF_MCONT_RCV_EOB);
 }
 
@@ -730,7 +728,7 @@ static void c_can_do_tx(struct net_device *dev)
 	while ((idx = ffs(pend))) {
 		idx--;
 		pend &= ~(1 << idx);
-		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
+		obj = idx + priv->msg_obj_tx_first;
 		c_can_inval_tx_object(dev, IF_TX, obj);
 		can_get_echo_skb(dev, idx, NULL);
 		bytes += priv->dlc[idx];
@@ -740,7 +738,7 @@ static void c_can_do_tx(struct net_device *dev)
 	/* Clear the bits in the tx_active mask */
 	atomic_sub(clr, &priv->tx_active);
 
-	if (clr & (1 << (C_CAN_MSG_OBJ_TX_NUM - 1)))
+	if (clr & (1 << (priv->msg_obj_tx_num - 1)))
 		netif_wake_queue(dev);
 
 	if (pkts) {
@@ -755,11 +753,11 @@ static void c_can_do_tx(struct net_device *dev)
  * raced with the hardware or failed to readout all upper
  * objects in the last run due to quota limit.
  */
-static u32 c_can_adjust_pending(u32 pend)
+static u32 c_can_adjust_pending(u32 pend, u32 rx_mask)
 {
 	u32 weight, lasts;
 
-	if (pend == RECEIVE_OBJECT_BITS)
+	if (pend == rx_mask)
 		return pend;
 
 	/*
@@ -862,8 +860,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	 * It is faster to read only one 16bit register. This is only possible
 	 * for a maximum number of 16 objects.
 	 */
-	BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
-			"Implementation does not support more message objects than 16");
+	WARN_ON(priv->msg_obj_rx_last > 16);
 
 	while (quota > 0) {
 		if (!pend) {
@@ -874,7 +871,8 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 			 * If the pending field has a gap, handle the
 			 * bits above the gap first.
 			 */
-			toread = c_can_adjust_pending(pend);
+			toread = c_can_adjust_pending(pend,
+						      priv->msg_obj_rx_mask);
 		} else {
 			toread = pend;
 		}
@@ -1205,17 +1203,31 @@ static int c_can_close(struct net_device *dev)
 	return 0;
 }
 
-struct net_device *alloc_c_can_dev(void)
+struct net_device *alloc_c_can_dev(int msg_obj_num)
 {
 	struct net_device *dev;
 	struct c_can_priv *priv;
+	int msg_obj_tx_num = msg_obj_num / 2;
 
-	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
+	dev = alloc_candev(sizeof(*priv) + sizeof(u32) * msg_obj_tx_num,
+			   msg_obj_tx_num);
 	if (!dev)
 		return NULL;
 
 	priv = netdev_priv(dev);
-	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
+	priv->msg_obj_num = msg_obj_num;
+	priv->msg_obj_rx_num = msg_obj_num - msg_obj_tx_num;
+	priv->msg_obj_rx_first = 1;
+	priv->msg_obj_rx_last =
+		priv->msg_obj_rx_first + priv->msg_obj_rx_num - 1;
+	priv->msg_obj_rx_mask = ((u64)1 << priv->msg_obj_rx_num) - 1;
+
+	priv->msg_obj_tx_num = msg_obj_tx_num;
+	priv->msg_obj_tx_first = priv->msg_obj_rx_last + 1;
+	priv->msg_obj_tx_last =
+		priv->msg_obj_tx_first + priv->msg_obj_tx_num - 1;
+
+	netif_napi_add(dev, &priv->napi, c_can_poll, priv->msg_obj_rx_num);
 
 	priv->dev = dev;
 	priv->can.bittiming_const = &c_can_bittiming_const;
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 90d3d2e7a086..22ae6077b489 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -22,18 +22,7 @@
 #ifndef C_CAN_H
 #define C_CAN_H
 
-/* message object split */
 #define C_CAN_NO_OF_OBJECTS	32
-#define C_CAN_MSG_OBJ_RX_NUM	16
-#define C_CAN_MSG_OBJ_TX_NUM	16
-
-#define C_CAN_MSG_OBJ_RX_FIRST	1
-#define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
-				C_CAN_MSG_OBJ_RX_NUM - 1)
-
-#define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
-
-#define RECEIVE_OBJECT_BITS	0x0000ffff
 
 enum reg {
 	C_CAN_CTRL_REG = 0,
@@ -193,6 +182,14 @@ struct c_can_priv {
 	struct napi_struct napi;
 	struct net_device *dev;
 	struct device *device;
+	int msg_obj_num;
+	int msg_obj_rx_num;
+	int msg_obj_tx_num;
+	int msg_obj_rx_first;
+	int msg_obj_rx_last;
+	int msg_obj_tx_first;
+	int msg_obj_tx_last;
+	u32 msg_obj_rx_mask;
 	atomic_t tx_active;
 	atomic_t sie_pending;
 	unsigned long tx_dir;
@@ -209,10 +206,10 @@ struct c_can_priv {
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
 	u32 comm_rcv_high;
 	u32 rxmasked;
-	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
+	u32 dlc[];
 };
 
-struct net_device *alloc_c_can_dev(void);
+struct net_device *alloc_c_can_dev(int msg_obj_num);
 void free_c_can_dev(struct net_device *dev);
 int register_c_can_dev(struct net_device *dev);
 void unregister_c_can_dev(struct net_device *dev);
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 406b4847e5dc..3752f68d095e 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -149,7 +149,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 	}
 
 	/* allocate the c_can device */
-	dev = alloc_c_can_dev();
+	dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
 	if (!dev) {
 		ret = -ENOMEM;
 		goto out_iounmap;
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index 05f425ceb53a..a5b9b1a93702 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -293,7 +293,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	/* allocate the c_can device */
-	dev = alloc_c_can_dev();
+	dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
 	if (!dev) {
 		ret = -ENOMEM;
 		goto exit;
-- 
2.17.1

Re: [PATCH v2 5/6] can: c_can: prepare to up the message objects number

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 8113 bytes --]

On 25.02.2021 22:51:54, Dario Binacchi wrote:
> As pointed by commit c0a9f4d396c9 ("can: c_can: Reduce register access")
> the "driver casts the 16 message objects in stone, which is completely
> braindead as contemporary hardware has up to 128 message objects".
> 
> The patch prepares the module to extend the number of message objects
> beyond the 32 currently managed. This was achieved by transforming the
> constants used to manage RX/TX messages into variables without changing
> the driver policy.
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> Reported-by: kernel test robot <lkp@intel.com>
> 
> ---
> 
> Changes in v2:
> - Fix compiling error reported by kernel test robot.
> - Add Reported-by tag.
> - Pass larger size to alloc_candev() routine to avoid an additional
>   memory allocation/deallocation.
> 
>  drivers/net/can/c_can/c_can.c          | 50 ++++++++++++++++----------
>  drivers/net/can/c_can/c_can.h          | 23 ++++++------
>  drivers/net/can/c_can/c_can_pci.c      |  2 +-
>  drivers/net/can/c_can/c_can_platform.c |  2 +-
>  4 files changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 7081cfaf62e2..df1ad6b3fd3b 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -173,9 +173,6 @@
>  /* Wait for ~1 sec for INIT bit */
>  #define INIT_WAIT_MS		1000
>  
> -/* napi related */
> -#define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
> -
>  /* c_can lec values */
>  enum c_can_lec_type {
>  	LEC_NO_ERROR = 0,
> @@ -325,7 +322,7 @@ static void c_can_setup_tx_object(struct net_device *dev, int iface,
>  	 * first, i.e. clear the MSGVAL flag in the arbiter.
>  	 */
>  	if (rtr != (bool)test_bit(idx, &priv->tx_dir)) {
> -		u32 obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> +		u32 obj = idx + priv->msg_obj_tx_first;
>  
>  		c_can_inval_msg_object(dev, iface, obj);
>  		change_bit(idx, &priv->tx_dir);
> @@ -463,10 +460,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	 * prioritized. The lowest buffer number wins.
>  	 */
>  	idx = fls(atomic_read(&priv->tx_active));
> -	obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> +	obj = idx + priv->msg_obj_tx_first;
>  
>  	/* If this is the last buffer, stop the xmit queue */
> -	if (idx == C_CAN_MSG_OBJ_TX_NUM - 1)
> +	if (idx == priv->msg_obj_tx_num - 1)
>  		netif_stop_queue(dev);
>  	/*
>  	 * Store the message in the interface so we can call
> @@ -549,17 +546,18 @@ static int c_can_set_bittiming(struct net_device *dev)
>   */
>  static void c_can_configure_msg_objects(struct net_device *dev)
>  {
> +	struct c_can_priv *priv = netdev_priv(dev);
>  	int i;
>  
>  	/* first invalidate all message objects */
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
> +	for (i = priv->msg_obj_rx_first; i <= priv->msg_obj_num; i++)
>  		c_can_inval_msg_object(dev, IF_RX, i);
>  
>  	/* setup receive message objects */
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> +	for (i = priv->msg_obj_rx_first; i < priv->msg_obj_rx_last; i++)
>  		c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_RCV);
>  
> -	c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> +	c_can_setup_receive_object(dev, IF_RX, priv->msg_obj_rx_last, 0, 0,
>  				   IF_MCONT_RCV_EOB);
>  }
>  
> @@ -730,7 +728,7 @@ static void c_can_do_tx(struct net_device *dev)
>  	while ((idx = ffs(pend))) {
>  		idx--;
>  		pend &= ~(1 << idx);
> -		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> +		obj = idx + priv->msg_obj_tx_first;
>  		c_can_inval_tx_object(dev, IF_TX, obj);
>  		can_get_echo_skb(dev, idx, NULL);
>  		bytes += priv->dlc[idx];
> @@ -740,7 +738,7 @@ static void c_can_do_tx(struct net_device *dev)
>  	/* Clear the bits in the tx_active mask */
>  	atomic_sub(clr, &priv->tx_active);
>  
> -	if (clr & (1 << (C_CAN_MSG_OBJ_TX_NUM - 1)))
> +	if (clr & (1 << (priv->msg_obj_tx_num - 1)))
>  		netif_wake_queue(dev);
>  
>  	if (pkts) {
> @@ -755,11 +753,11 @@ static void c_can_do_tx(struct net_device *dev)
>   * raced with the hardware or failed to readout all upper
>   * objects in the last run due to quota limit.
>   */
> -static u32 c_can_adjust_pending(u32 pend)
> +static u32 c_can_adjust_pending(u32 pend, u32 rx_mask)
>  {
>  	u32 weight, lasts;
>  
> -	if (pend == RECEIVE_OBJECT_BITS)
> +	if (pend == rx_mask)
>  		return pend;
>  
>  	/*
> @@ -862,8 +860,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  	 * It is faster to read only one 16bit register. This is only possible
>  	 * for a maximum number of 16 objects.
>  	 */
> -	BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
> -			"Implementation does not support more message objects than 16");
> +	WARN_ON(priv->msg_obj_rx_last > 16);
>  
>  	while (quota > 0) {
>  		if (!pend) {
> @@ -874,7 +871,8 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  			 * If the pending field has a gap, handle the
>  			 * bits above the gap first.
>  			 */
> -			toread = c_can_adjust_pending(pend);
> +			toread = c_can_adjust_pending(pend,
> +						      priv->msg_obj_rx_mask);
>  		} else {
>  			toread = pend;
>  		}
> @@ -1205,17 +1203,31 @@ static int c_can_close(struct net_device *dev)
>  	return 0;
>  }
>  
> -struct net_device *alloc_c_can_dev(void)
> +struct net_device *alloc_c_can_dev(int msg_obj_num)
>  {
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
> +	int msg_obj_tx_num = msg_obj_num / 2;
>  
> -	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
> +	dev = alloc_candev(sizeof(*priv) + sizeof(u32) * msg_obj_tx_num,
> +			   msg_obj_tx_num);
>  	if (!dev)
>  		return NULL;
>  
>  	priv = netdev_priv(dev);
> -	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
> +	priv->msg_obj_num = msg_obj_num;
> +	priv->msg_obj_rx_num = msg_obj_num - msg_obj_tx_num;
> +	priv->msg_obj_rx_first = 1;
> +	priv->msg_obj_rx_last =
> +		priv->msg_obj_rx_first + priv->msg_obj_rx_num - 1;
> +	priv->msg_obj_rx_mask = ((u64)1 << priv->msg_obj_rx_num) - 1;
> +
> +	priv->msg_obj_tx_num = msg_obj_tx_num;
> +	priv->msg_obj_tx_first = priv->msg_obj_rx_last + 1;
> +	priv->msg_obj_tx_last =
> +		priv->msg_obj_tx_first + priv->msg_obj_tx_num - 1;
> +
> +	netif_napi_add(dev, &priv->napi, c_can_poll, priv->msg_obj_rx_num);
>  
>  	priv->dev = dev;
>  	priv->can.bittiming_const = &c_can_bittiming_const;
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 90d3d2e7a086..22ae6077b489 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -22,18 +22,7 @@
>  #ifndef C_CAN_H
>  #define C_CAN_H
>  
> -/* message object split */
>  #define C_CAN_NO_OF_OBJECTS	32
> -#define C_CAN_MSG_OBJ_RX_NUM	16
> -#define C_CAN_MSG_OBJ_TX_NUM	16
> -
> -#define C_CAN_MSG_OBJ_RX_FIRST	1
> -#define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
> -				C_CAN_MSG_OBJ_RX_NUM - 1)
> -
> -#define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
> -
> -#define RECEIVE_OBJECT_BITS	0x0000ffff
>  
>  enum reg {
>  	C_CAN_CTRL_REG = 0,
> @@ -193,6 +182,14 @@ struct c_can_priv {
>  	struct napi_struct napi;
>  	struct net_device *dev;
>  	struct device *device;
> +	int msg_obj_num;
> +	int msg_obj_rx_num;
> +	int msg_obj_tx_num;
> +	int msg_obj_rx_first;
> +	int msg_obj_rx_last;
> +	int msg_obj_tx_first;
> +	int msg_obj_tx_last;

I think these should/could be unsigned int.

> +	u32 msg_obj_rx_mask;

Is this variable big enough after you've extended the driver to use 64
mailboxes?

If you want to support 128 message objects converting the driver to the
linux bitmap API is another option.

Marc

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

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

Re: [PATCH v2 5/6] can: c_can: prepare to up the message objects number

[Dario Binacchi]

Hi Marc,

> Il 26/02/2021 09:33 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:
> 
>  
> On 25.02.2021 22:51:54, Dario Binacchi wrote:
> > As pointed by commit c0a9f4d396c9 ("can: c_can: Reduce register access")
> > the "driver casts the 16 message objects in stone, which is completely
> > braindead as contemporary hardware has up to 128 message objects".
> > 
> > The patch prepares the module to extend the number of message objects
> > beyond the 32 currently managed. This was achieved by transforming the
> > constants used to manage RX/TX messages into variables without changing
> > the driver policy.
> > 
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Fix compiling error reported by kernel test robot.
> > - Add Reported-by tag.
> > - Pass larger size to alloc_candev() routine to avoid an additional
> >   memory allocation/deallocation.
> > 
> >  drivers/net/can/c_can/c_can.c          | 50 ++++++++++++++++----------
> >  drivers/net/can/c_can/c_can.h          | 23 ++++++------
> >  drivers/net/can/c_can/c_can_pci.c      |  2 +-
> >  drivers/net/can/c_can/c_can_platform.c |  2 +-
> >  4 files changed, 43 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index 7081cfaf62e2..df1ad6b3fd3b 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -173,9 +173,6 @@
> >  /* Wait for ~1 sec for INIT bit */
> >  #define INIT_WAIT_MS		1000
> >  
> > -/* napi related */
> > -#define C_CAN_NAPI_WEIGHT	C_CAN_MSG_OBJ_RX_NUM
> > -
> >  /* c_can lec values */
> >  enum c_can_lec_type {
> >  	LEC_NO_ERROR = 0,
> > @@ -325,7 +322,7 @@ static void c_can_setup_tx_object(struct net_device *dev, int iface,
> >  	 * first, i.e. clear the MSGVAL flag in the arbiter.
> >  	 */
> >  	if (rtr != (bool)test_bit(idx, &priv->tx_dir)) {
> > -		u32 obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > +		u32 obj = idx + priv->msg_obj_tx_first;
> >  
> >  		c_can_inval_msg_object(dev, iface, obj);
> >  		change_bit(idx, &priv->tx_dir);
> > @@ -463,10 +460,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> >  	 * prioritized. The lowest buffer number wins.
> >  	 */
> >  	idx = fls(atomic_read(&priv->tx_active));
> > -	obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > +	obj = idx + priv->msg_obj_tx_first;
> >  
> >  	/* If this is the last buffer, stop the xmit queue */
> > -	if (idx == C_CAN_MSG_OBJ_TX_NUM - 1)
> > +	if (idx == priv->msg_obj_tx_num - 1)
> >  		netif_stop_queue(dev);
> >  	/*
> >  	 * Store the message in the interface so we can call
> > @@ -549,17 +546,18 @@ static int c_can_set_bittiming(struct net_device *dev)
> >   */
> >  static void c_can_configure_msg_objects(struct net_device *dev)
> >  {
> > +	struct c_can_priv *priv = netdev_priv(dev);
> >  	int i;
> >  
> >  	/* first invalidate all message objects */
> > -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
> > +	for (i = priv->msg_obj_rx_first; i <= priv->msg_obj_num; i++)
> >  		c_can_inval_msg_object(dev, IF_RX, i);
> >  
> >  	/* setup receive message objects */
> > -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> > +	for (i = priv->msg_obj_rx_first; i < priv->msg_obj_rx_last; i++)
> >  		c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_RCV);
> >  
> > -	c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> > +	c_can_setup_receive_object(dev, IF_RX, priv->msg_obj_rx_last, 0, 0,
> >  				   IF_MCONT_RCV_EOB);
> >  }
> >  
> > @@ -730,7 +728,7 @@ static void c_can_do_tx(struct net_device *dev)
> >  	while ((idx = ffs(pend))) {
> >  		idx--;
> >  		pend &= ~(1 << idx);
> > -		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > +		obj = idx + priv->msg_obj_tx_first;
> >  		c_can_inval_tx_object(dev, IF_TX, obj);
> >  		can_get_echo_skb(dev, idx, NULL);
> >  		bytes += priv->dlc[idx];
> > @@ -740,7 +738,7 @@ static void c_can_do_tx(struct net_device *dev)
> >  	/* Clear the bits in the tx_active mask */
> >  	atomic_sub(clr, &priv->tx_active);
> >  
> > -	if (clr & (1 << (C_CAN_MSG_OBJ_TX_NUM - 1)))
> > +	if (clr & (1 << (priv->msg_obj_tx_num - 1)))
> >  		netif_wake_queue(dev);
> >  
> >  	if (pkts) {
> > @@ -755,11 +753,11 @@ static void c_can_do_tx(struct net_device *dev)
> >   * raced with the hardware or failed to readout all upper
> >   * objects in the last run due to quota limit.
> >   */
> > -static u32 c_can_adjust_pending(u32 pend)
> > +static u32 c_can_adjust_pending(u32 pend, u32 rx_mask)
> >  {
> >  	u32 weight, lasts;
> >  
> > -	if (pend == RECEIVE_OBJECT_BITS)
> > +	if (pend == rx_mask)
> >  		return pend;
> >  
> >  	/*
> > @@ -862,8 +860,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
> >  	 * It is faster to read only one 16bit register. This is only possible
> >  	 * for a maximum number of 16 objects.
> >  	 */
> > -	BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
> > -			"Implementation does not support more message objects than 16");
> > +	WARN_ON(priv->msg_obj_rx_last > 16);
> >  
> >  	while (quota > 0) {
> >  		if (!pend) {
> > @@ -874,7 +871,8 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
> >  			 * If the pending field has a gap, handle the
> >  			 * bits above the gap first.
> >  			 */
> > -			toread = c_can_adjust_pending(pend);
> > +			toread = c_can_adjust_pending(pend,
> > +						      priv->msg_obj_rx_mask);
> >  		} else {
> >  			toread = pend;
> >  		}
> > @@ -1205,17 +1203,31 @@ static int c_can_close(struct net_device *dev)
> >  	return 0;
> >  }
> >  
> > -struct net_device *alloc_c_can_dev(void)
> > +struct net_device *alloc_c_can_dev(int msg_obj_num)
> >  {
> >  	struct net_device *dev;
> >  	struct c_can_priv *priv;
> > +	int msg_obj_tx_num = msg_obj_num / 2;
> >  
> > -	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
> > +	dev = alloc_candev(sizeof(*priv) + sizeof(u32) * msg_obj_tx_num,
> > +			   msg_obj_tx_num);
> >  	if (!dev)
> >  		return NULL;
> >  
> >  	priv = netdev_priv(dev);
> > -	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
> > +	priv->msg_obj_num = msg_obj_num;
> > +	priv->msg_obj_rx_num = msg_obj_num - msg_obj_tx_num;
> > +	priv->msg_obj_rx_first = 1;
> > +	priv->msg_obj_rx_last =
> > +		priv->msg_obj_rx_first + priv->msg_obj_rx_num - 1;
> > +	priv->msg_obj_rx_mask = ((u64)1 << priv->msg_obj_rx_num) - 1;
> > +
> > +	priv->msg_obj_tx_num = msg_obj_tx_num;
> > +	priv->msg_obj_tx_first = priv->msg_obj_rx_last + 1;
> > +	priv->msg_obj_tx_last =
> > +		priv->msg_obj_tx_first + priv->msg_obj_tx_num - 1;
> > +
> > +	netif_napi_add(dev, &priv->napi, c_can_poll, priv->msg_obj_rx_num);
> >  
> >  	priv->dev = dev;
> >  	priv->can.bittiming_const = &c_can_bittiming_const;
> > diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> > index 90d3d2e7a086..22ae6077b489 100644
> > --- a/drivers/net/can/c_can/c_can.h
> > +++ b/drivers/net/can/c_can/c_can.h
> > @@ -22,18 +22,7 @@
> >  #ifndef C_CAN_H
> >  #define C_CAN_H
> >  
> > -/* message object split */
> >  #define C_CAN_NO_OF_OBJECTS	32
> > -#define C_CAN_MSG_OBJ_RX_NUM	16
> > -#define C_CAN_MSG_OBJ_TX_NUM	16
> > -
> > -#define C_CAN_MSG_OBJ_RX_FIRST	1
> > -#define C_CAN_MSG_OBJ_RX_LAST	(C_CAN_MSG_OBJ_RX_FIRST + \
> > -				C_CAN_MSG_OBJ_RX_NUM - 1)
> > -
> > -#define C_CAN_MSG_OBJ_TX_FIRST	(C_CAN_MSG_OBJ_RX_LAST + 1)
> > -
> > -#define RECEIVE_OBJECT_BITS	0x0000ffff
> >  
> >  enum reg {
> >  	C_CAN_CTRL_REG = 0,
> > @@ -193,6 +182,14 @@ struct c_can_priv {
> >  	struct napi_struct napi;
> >  	struct net_device *dev;
> >  	struct device *device;
> > +	int msg_obj_num;
> > +	int msg_obj_rx_num;
> > +	int msg_obj_tx_num;
> > +	int msg_obj_rx_first;
> > +	int msg_obj_rx_last;
> > +	int msg_obj_tx_first;
> > +	int msg_obj_tx_last;
> 
> I think these should/could be unsigned int.
> 
> > +	u32 msg_obj_rx_mask;
> 
> Is this variable big enough after you've extended the driver to use 64
> mailboxes?

Yes. I have kept the message assignment policy unchanged, they are equally 
divided between reception and transmission. Therefore, in the case of 64 
messages, 32 are used for reception and 32 for transmission. So a 32-bit 
variable is enough.

> 
> If you want to support 128 message objects converting the driver to the
> linux bitmap API is another option.
> 

Do you know if any of the hardware managed by Linux use a D_CAN controller 
with 128 message objects?

Thanks and regards,
Dario 

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

Re: [PATCH v2 5/6] can: c_can: prepare to up the message objects number

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On 28.02.2021 11:18:13, Dario Binacchi wrote:
> > > +	u32 msg_obj_rx_mask;
> > 
> > Is this variable big enough after you've extended the driver to use 64
> > mailboxes?
> 
> Yes. I have kept the message assignment policy unchanged, they are equally 
> divided between reception and transmission. Therefore, in the case of 64 
> messages, 32 are used for reception and 32 for transmission. So a 32-bit 
> variable is enough.
> 
> > 
> > If you want to support 128 message objects converting the driver to the
> > linux bitmap API is another option.
> > 
> 
> Do you know if any of the hardware managed by Linux use a D_CAN controller 
> with 128 message objects?

Even the am437x only uses 64 message objects. Ok let's stay with 64 as
max for now.

regards,
Marc

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

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

Re: [PATCH v2 5/6] can: c_can: prepare to up the message objects number

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On 25.02.2021 22:51:54, Dario Binacchi wrote:
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
[...]
> -struct net_device *alloc_c_can_dev(void)
> +struct net_device *alloc_c_can_dev(int msg_obj_num)
>  {
>  	struct net_device *dev;
>  	struct c_can_priv *priv;
> +	int msg_obj_tx_num = msg_obj_num / 2;
>  
> -	dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
> +	dev = alloc_candev(sizeof(*priv) + sizeof(u32) * msg_obj_tx_num,
> +			   msg_obj_tx_num);
>  	if (!dev)
>  		return NULL;
>  
>  	priv = netdev_priv(dev);
> -	netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
> +	priv->msg_obj_num = msg_obj_num;
> +	priv->msg_obj_rx_num = msg_obj_num - msg_obj_tx_num;
> +	priv->msg_obj_rx_first = 1;
> +	priv->msg_obj_rx_last =
> +		priv->msg_obj_rx_first + priv->msg_obj_rx_num - 1;
> +	priv->msg_obj_rx_mask = ((u64)1 << priv->msg_obj_rx_num) - 1;

Here you're casting the 1 to 64bit, but msg_obj_rx_mask is only u32. Use
"1UL", if you are sure that rx_mask would never exceed 32 bit.

Marc

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

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

[PATCH v2 6/6] can: c_can: add support to 64 message objects

[Dario Binacchi]

D_CAN controller supports 16, 32, 64 or 128 message objects, comparing
to 32 on C_CAN.
AM335x/AM437x Sitara processors and DRA7 SOC all instantiate a D_CAN
controller with 64 message objects, as described in the "DCAN features"
subsection of the CAN chapter of their technical reference manuals.

The driver policy has been kept unchanged, and as in the previous
version, the first half of the message objects is used for reception and
the second for transmission.

The I/O load is increased only in the case of 64 message objects,
keeping it unchanged in the case of 32. Two 32-bit read accesses are in
fact required, which however remained at 16-bit for configurations with
32 message objects.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

Changes in v2:
- Add message objects number to PCI driver data.

 drivers/net/can/c_can/c_can.c          | 19 +++++++++++--------
 drivers/net/can/c_can/c_can.h          |  5 +++--
 drivers/net/can/c_can/c_can_pci.c      |  6 +++++-
 drivers/net/can/c_can/c_can_platform.c |  6 +++++-
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index df1ad6b3fd3b..b049e94543cb 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -723,8 +723,12 @@ static void c_can_do_tx(struct net_device *dev)
 	struct net_device_stats *stats = &dev->stats;
 	u32 idx, obj, pkts = 0, bytes = 0, pend, clr;
 
-	clr = pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
+	if (priv->msg_obj_tx_last > 32)
+		pend = priv->read_reg32(priv, C_CAN_INTPND3_REG);
+	else
+		pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
 
+	clr = pend;
 	while ((idx = ffs(pend))) {
 		idx--;
 		pend &= ~(1 << idx);
@@ -834,7 +838,12 @@ static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv,
 
 static inline u32 c_can_get_pending(struct c_can_priv *priv)
 {
-	u32 pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
+	u32 pend;
+
+	if (priv->msg_obj_rx_last > 16)
+		pend = priv->read_reg32(priv, C_CAN_NEWDAT1_REG);
+	else
+		pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
 
 	return pend;
 }
@@ -856,12 +865,6 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	struct c_can_priv *priv = netdev_priv(dev);
 	u32 pkts = 0, pend = 0, toread, n;
 
-	/*
-	 * It is faster to read only one 16bit register. This is only possible
-	 * for a maximum number of 16 objects.
-	 */
-	WARN_ON(priv->msg_obj_rx_last > 16);
-
 	while (quota > 0) {
 		if (!pend) {
 			pend = c_can_get_pending(priv);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 22ae6077b489..cff24597be3c 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -22,8 +22,6 @@
 #ifndef C_CAN_H
 #define C_CAN_H
 
-#define C_CAN_NO_OF_OBJECTS	32
-
 enum reg {
 	C_CAN_CTRL_REG = 0,
 	C_CAN_CTRL_EX_REG,
@@ -61,6 +59,7 @@ enum reg {
 	C_CAN_NEWDAT2_REG,
 	C_CAN_INTPND1_REG,
 	C_CAN_INTPND2_REG,
+	C_CAN_INTPND3_REG,
 	C_CAN_MSGVAL1_REG,
 	C_CAN_MSGVAL2_REG,
 	C_CAN_FUNCTION_REG,
@@ -122,6 +121,7 @@ static const u16 __maybe_unused reg_map_d_can[] = {
 	[C_CAN_NEWDAT2_REG]	= 0x9E,
 	[C_CAN_INTPND1_REG]	= 0xB0,
 	[C_CAN_INTPND2_REG]	= 0xB2,
+	[C_CAN_INTPND3_REG]	= 0xB4,
 	[C_CAN_MSGVAL1_REG]	= 0xC4,
 	[C_CAN_MSGVAL2_REG]	= 0xC6,
 	[C_CAN_IF1_COMREQ_REG]	= 0x100,
@@ -161,6 +161,7 @@ struct raminit_bits {
 
 struct c_can_driver_data {
 	enum c_can_dev_id id;
+	int msg_obj_num;
 
 	/* RAMINIT register description. Optional. */
 	const struct raminit_bits *raminit_bits; /* Array of START/DONE bit positions */
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 3752f68d095e..2cb98ccd04d7 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -31,6 +31,8 @@ enum c_can_pci_reg_align {
 struct c_can_pci_data {
 	/* Specify if is C_CAN or D_CAN */
 	enum c_can_dev_id type;
+	/* Number of message objects */
+	int msg_obj_num;
 	/* Set the register alignment in the memory */
 	enum c_can_pci_reg_align reg_align;
 	/* Set the frequency */
@@ -149,7 +151,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 	}
 
 	/* allocate the c_can device */
-	dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
+	dev = alloc_c_can_dev(c_can_pci_data->msg_obj_num);
 	if (!dev) {
 		ret = -ENOMEM;
 		goto out_iounmap;
@@ -253,6 +255,7 @@ static void c_can_pci_remove(struct pci_dev *pdev)
 
 static const struct c_can_pci_data c_can_sta2x11= {
 	.type = BOSCH_C_CAN,
+	.msg_obj_num = 32,
 	.reg_align = C_CAN_REG_ALIGN_32,
 	.freq = 52000000, /* 52 Mhz */
 	.bar = 0,
@@ -260,6 +263,7 @@ static const struct c_can_pci_data c_can_sta2x11= {
 
 static const struct c_can_pci_data c_can_pch = {
 	.type = BOSCH_C_CAN,
+	.msg_obj_num = 32,
 	.reg_align = C_CAN_REG_32,
 	.freq = 50000000, /* 50 MHz */
 	.init = c_can_pci_reset_pch,
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index a5b9b1a93702..87a145b67a2f 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -192,10 +192,12 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable)
 
 static const struct c_can_driver_data c_can_drvdata = {
 	.id = BOSCH_C_CAN,
+	.msg_obj_num = 32,
 };
 
 static const struct c_can_driver_data d_can_drvdata = {
 	.id = BOSCH_D_CAN,
+	.msg_obj_num = 32,
 };
 
 static const struct raminit_bits dra7_raminit_bits[] = {
@@ -205,6 +207,7 @@ static const struct raminit_bits dra7_raminit_bits[] = {
 
 static const struct c_can_driver_data dra7_dcan_drvdata = {
 	.id = BOSCH_D_CAN,
+	.msg_obj_num = 64,
 	.raminit_num = ARRAY_SIZE(dra7_raminit_bits),
 	.raminit_bits = dra7_raminit_bits,
 	.raminit_pulse = true,
@@ -217,6 +220,7 @@ static const struct raminit_bits am3352_raminit_bits[] = {
 
 static const struct c_can_driver_data am3352_dcan_drvdata = {
 	.id = BOSCH_D_CAN,
+	.msg_obj_num = 64,
 	.raminit_num = ARRAY_SIZE(am3352_raminit_bits),
 	.raminit_bits = am3352_raminit_bits,
 };
@@ -293,7 +297,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 	}
 
 	/* allocate the c_can device */
-	dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
+	dev = alloc_c_can_dev(drvdata->msg_obj_num);
 	if (!dev) {
 		ret = -ENOMEM;
 		goto exit;
-- 
2.17.1

Re: [PATCH v2 6/6] can: c_can: add support to 64 message objects

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

On 25.02.2021 22:51:55, Dario Binacchi wrote:
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -22,8 +22,6 @@
>  #ifndef C_CAN_H
>  #define C_CAN_H
>  
> -#define C_CAN_NO_OF_OBJECTS	32
> -
>  enum reg {
>  	C_CAN_CTRL_REG = 0,
>  	C_CAN_CTRL_EX_REG,
> @@ -61,6 +59,7 @@ enum reg {
>  	C_CAN_NEWDAT2_REG,
>  	C_CAN_INTPND1_REG,
>  	C_CAN_INTPND2_REG,
> +	C_CAN_INTPND3_REG,
>  	C_CAN_MSGVAL1_REG,
>  	C_CAN_MSGVAL2_REG,
>  	C_CAN_FUNCTION_REG,
> @@ -122,6 +121,7 @@ static const u16 __maybe_unused reg_map_d_can[] = {
>  	[C_CAN_NEWDAT2_REG]	= 0x9E,
>  	[C_CAN_INTPND1_REG]	= 0xB0,
>  	[C_CAN_INTPND2_REG]	= 0xB2,
> +	[C_CAN_INTPND3_REG]	= 0xB4,
>  	[C_CAN_MSGVAL1_REG]	= 0xC4,
>  	[C_CAN_MSGVAL2_REG]	= 0xC6,
>  	[C_CAN_IF1_COMREQ_REG]	= 0x100,
> @@ -161,6 +161,7 @@ struct raminit_bits {
>  
>  struct c_can_driver_data {
>  	enum c_can_dev_id id;
> +	int msg_obj_num;

unsigned int

>  
>  	/* RAMINIT register description. Optional. */
>  	const struct raminit_bits *raminit_bits; /* Array of START/DONE bit positions */
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> index 3752f68d095e..2cb98ccd04d7 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -31,6 +31,8 @@ enum c_can_pci_reg_align {
>  struct c_can_pci_data {
>  	/* Specify if is C_CAN or D_CAN */
>  	enum c_can_dev_id type;
> +	/* Number of message objects */
> +	int msg_obj_num;

unsigned int


Marc

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

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