diff mbox series

[v7,1/4] can: m_can: Create a m_can platform framework

Message ID 20190305155220.14037-1-dmurphy@ti.com
State Superseded
Headers show
Series [v7,1/4] can: m_can: Create a m_can platform framework | expand

Commit Message

Dan Murphy March 5, 2019, 3:52 p.m. UTC
Create a m_can platform framework that peripherial
devices can register to and use common code and register sets.
The peripherial devices may provide read/write and configuration
support of the IP.

Signed-off-by: Dan Murphy <dmurphy@ti.com>

---


v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard
start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

v6 - Squashed platform patch to this patch for bissectablity, fixed coding style
issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed
priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -
Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/
Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

 drivers/net/can/m_can/Kconfig          |  13 +-
 drivers/net/can/m_can/Makefile         |   1 +
 drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------
 drivers/net/can/m_can/m_can.h          | 110 ++++
 drivers/net/can/m_can/m_can_platform.c | 202 +++++++
 5 files changed, 682 insertions(+), 344 deletions(-)
 create mode 100644 drivers/net/can/m_can/m_can.h
 create mode 100644 drivers/net/can/m_can/m_can_platform.c

-- 
2.20.1.390.gb5101f9297

Comments

Wolfgang Grandegger March 8, 2019, 10:10 a.m. UTC | #1
Hallo Dan,

Am 05.03.19 um 16:52 schrieb Dan Murphy:
> Create a m_can platform framework that peripherial

> devices can register to and use common code and register sets.

> The peripherial devices may provide read/write and configuration

> support of the IP.

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> 

> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

> 

> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

> 

>  drivers/net/can/m_can/Kconfig          |  13 +-

>  drivers/net/can/m_can/Makefile         |   1 +

>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>  drivers/net/can/m_can/m_can.h          | 110 ++++

>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>  5 files changed, 682 insertions(+), 344 deletions(-)

>  create mode 100644 drivers/net/can/m_can/m_can.h

>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

> 

> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

> index 04f20dd39007..f7119fd72df4 100644

> --- a/drivers/net/can/m_can/Kconfig

> +++ b/drivers/net/can/m_can/Kconfig

> @@ -1,5 +1,14 @@

>  config CAN_M_CAN

> +	tristate "Bosch M_CAN support"

> +	---help---

> +	  Say Y here if you want support for Bosch M_CAN controller framework.

> +	  This is common support for devices that embed the Bosch M_CAN IP.

> +

> +config CAN_M_CAN_PLATFORM

> +	tristate "Bosch M_CAN support for io-mapped devices"

>  	depends on HAS_IOMEM

> -	tristate "Bosch M_CAN devices"

> +	depends on CAN_M_CAN

>  	---help---

> -	  Say Y here if you want to support for Bosch M_CAN controller.

> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

> +	  This support is for devices that have the Bosch M_CAN controller

> +	  IP embedded into the device and the IP is IO Mapped to the processor.

> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

> index 8bbd7f24f5be..057bbcdb3c74 100644

> --- a/drivers/net/can/m_can/Makefile

> +++ b/drivers/net/can/m_can/Makefile

> @@ -3,3 +3,4 @@

>  #

>  

>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

> index 9b449400376b..a60278d94126 100644

> --- a/drivers/net/can/m_can/m_can.c

> +++ b/drivers/net/can/m_can/m_can.c


... snip...

> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

> +				    struct net_device *dev)

> +{

> +	struct m_can_priv *priv = netdev_priv(dev);

> +

> +	if (can_dropped_invalid_skb(dev, skb))

> +		return NETDEV_TX_OK;

> +

> +	if (priv->is_peripherial) {

> +		if (priv->tx_skb) {

> +			netdev_err(dev, "hard_xmit called while tx busy\n");

> +			return NETDEV_TX_BUSY;

> +		}


The problem with that approach is, that the upper layer will try to
resubmit the current "skb" but not the previous "tx_skb". And the
previous "tx_skb" has not been freed yet. I would just drop and free the
skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices
(like can_dropped_invalid_skb() does).

> +

> +		priv->tx_skb = skb;

> +		netif_stop_queue(priv->net);

> +		queue_work(priv->tx_wq, &priv->tx_work);

> +	} else {

> +		priv->tx_skb = skb;

> +		return m_can_tx_handler(priv);

>  	}

>  

>  	return NETDEV_TX_OK;

>  }


Wolfgang.
Dan Murphy March 8, 2019, 12:44 p.m. UTC | #2
Wolfgang

On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:
> Hallo Dan,

> 

> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>> Create a m_can platform framework that peripherial

>> devices can register to and use common code and register sets.

>> The peripherial devices may provide read/write and configuration

>> support of the IP.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>

>>

>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>

>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>

>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>  drivers/net/can/m_can/Makefile         |   1 +

>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>

>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>> index 04f20dd39007..f7119fd72df4 100644

>> --- a/drivers/net/can/m_can/Kconfig

>> +++ b/drivers/net/can/m_can/Kconfig

>> @@ -1,5 +1,14 @@

>>  config CAN_M_CAN

>> +	tristate "Bosch M_CAN support"

>> +	---help---

>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>> +

>> +config CAN_M_CAN_PLATFORM

>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>  	depends on HAS_IOMEM

>> -	tristate "Bosch M_CAN devices"

>> +	depends on CAN_M_CAN

>>  	---help---

>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>> +	  This support is for devices that have the Bosch M_CAN controller

>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>> index 8bbd7f24f5be..057bbcdb3c74 100644

>> --- a/drivers/net/can/m_can/Makefile

>> +++ b/drivers/net/can/m_can/Makefile

>> @@ -3,3 +3,4 @@

>>  #

>>  

>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>> index 9b449400376b..a60278d94126 100644

>> --- a/drivers/net/can/m_can/m_can.c

>> +++ b/drivers/net/can/m_can/m_can.c

> 

> ... snip...

> 

>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>> +				    struct net_device *dev)

>> +{

>> +	struct m_can_priv *priv = netdev_priv(dev);

>> +

>> +	if (can_dropped_invalid_skb(dev, skb))

>> +		return NETDEV_TX_OK;

>> +

>> +	if (priv->is_peripherial) {

>> +		if (priv->tx_skb) {

>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>> +			return NETDEV_TX_BUSY;

>> +		}

> 

> The problem with that approach is, that the upper layer will try to

> resubmit the current "skb" but not the previous "tx_skb". And the

> previous "tx_skb" has not been freed yet. I would just drop and free the

> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

> (like can_dropped_invalid_skb() does).

> 


OK.

So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length
this is how these drivers are written.

In addition in the peripheral context the work queue does not report up to the upper layer the status.
Again the hi3110 and mcp251x drivers are written this way.

The only issue I see here is that the dropped and invalid check needs to come after the tx_skb check.

Dan


>> +

>> +		priv->tx_skb = skb;

>> +		netif_stop_queue(priv->net);

>> +		queue_work(priv->tx_wq, &priv->tx_work);

>> +	} else {

>> +		priv->tx_skb = skb;

>> +		return m_can_tx_handler(priv);

>>  	}

>>  

>>  	return NETDEV_TX_OK;

>>  }

> 

> Wolfgang.

> 



-- 
------------------
Dan Murphy
Wolfgang Grandegger March 8, 2019, 1:29 p.m. UTC | #3
Hello Dan,

Am 08.03.19 um 13:44 schrieb Dan Murphy:
> Wolfgang

> 

> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>> Hallo Dan,

>>

>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>> Create a m_can platform framework that peripherial

>>> devices can register to and use common code and register sets.

>>> The peripherial devices may provide read/write and configuration

>>> support of the IP.

>>>

>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>> ---

>>>

>>>

>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>

>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>

>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>

>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>> index 04f20dd39007..f7119fd72df4 100644

>>> --- a/drivers/net/can/m_can/Kconfig

>>> +++ b/drivers/net/can/m_can/Kconfig

>>> @@ -1,5 +1,14 @@

>>>  config CAN_M_CAN

>>> +	tristate "Bosch M_CAN support"

>>> +	---help---

>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>> +

>>> +config CAN_M_CAN_PLATFORM

>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>  	depends on HAS_IOMEM

>>> -	tristate "Bosch M_CAN devices"

>>> +	depends on CAN_M_CAN

>>>  	---help---

>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>> +	  This support is for devices that have the Bosch M_CAN controller

>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>> --- a/drivers/net/can/m_can/Makefile

>>> +++ b/drivers/net/can/m_can/Makefile

>>> @@ -3,3 +3,4 @@

>>>  #

>>>  

>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>> index 9b449400376b..a60278d94126 100644

>>> --- a/drivers/net/can/m_can/m_can.c

>>> +++ b/drivers/net/can/m_can/m_can.c

>>

>> ... snip...

>>

>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>> +				    struct net_device *dev)

>>> +{

>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>> +

>>> +	if (can_dropped_invalid_skb(dev, skb))

>>> +		return NETDEV_TX_OK;

>>> +

>>> +	if (priv->is_peripherial) {

>>> +		if (priv->tx_skb) {

>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>> +			return NETDEV_TX_BUSY;

>>> +		}

>>

>> The problem with that approach is, that the upper layer will try to

>> resubmit the current "skb" but not the previous "tx_skb". And the

>> previous "tx_skb" has not been freed yet. I would just drop and free the

>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>> (like can_dropped_invalid_skb() does).

>>

> 

> OK.

> 

> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

> this is how these drivers are written.


This is different. When entering the "start_xmit" routine, the previous
TX is still in progress. It will (hopefully) complete soon. Therefore
returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be
recalled soon with the same "skb". That scenario should/could also not
happen.

In contrast, in "m_can_tx_handler()", the skb could not be handled
because the FIFO is full. The "start_xmit" routine for peripheral
devices for that skb already returned NETDEV_TX_OK. Therefore the only
meaningful action is to drop the skb. Also this error should not happen
and if, something is going really wrong. Therefore I think, a
WARN_ONCE() would be even more appropriate. But that should be a
separate patch.

> 

> In addition in the peripheral context the work queue does not report up to the upper layer the status.

> Again the hi3110 and mcp251x drivers are written this way.

> 

> The only issue I see here is that the dropped and invalid check needs to come after the tx_skb check.


See above.

Wolfgang.
Wolfgang Grandegger March 8, 2019, 2:41 p.m. UTC | #4
Hello Dan,

thinking more about it...

Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:
> Hello Dan,

> 

> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>> Wolfgang

>>

>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>> Hallo Dan,

>>>

>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>> Create a m_can platform framework that peripherial

>>>> devices can register to and use common code and register sets.

>>>> The peripherial devices may provide read/write and configuration

>>>> support of the IP.

>>>>

>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>> ---

>>>>

>>>>

>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>

>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>

>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>

>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>> index 04f20dd39007..f7119fd72df4 100644

>>>> --- a/drivers/net/can/m_can/Kconfig

>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>> @@ -1,5 +1,14 @@

>>>>  config CAN_M_CAN

>>>> +	tristate "Bosch M_CAN support"

>>>> +	---help---

>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>> +

>>>> +config CAN_M_CAN_PLATFORM

>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>  	depends on HAS_IOMEM

>>>> -	tristate "Bosch M_CAN devices"

>>>> +	depends on CAN_M_CAN

>>>>  	---help---

>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>> --- a/drivers/net/can/m_can/Makefile

>>>> +++ b/drivers/net/can/m_can/Makefile

>>>> @@ -3,3 +3,4 @@

>>>>  #

>>>>  

>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>> index 9b449400376b..a60278d94126 100644

>>>> --- a/drivers/net/can/m_can/m_can.c

>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>

>>> ... snip...

>>>

>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>> +				    struct net_device *dev)

>>>> +{

>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>> +

>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>> +		return NETDEV_TX_OK;

>>>> +

>>>> +	if (priv->is_peripherial) {

>>>> +		if (priv->tx_skb) {

>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>> +			return NETDEV_TX_BUSY;

>>>> +		}

>>>

>>> The problem with that approach is, that the upper layer will try to

>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>> (like can_dropped_invalid_skb() does).

>>>

>>

>> OK.

>>

>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>> this is how these drivers are written.

> 

> This is different. When entering the "start_xmit" routine, the previous

> TX is still in progress. It will (hopefully) complete soon. Therefore

> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

> recalled soon with the same "skb". That scenario should/could also not

> happen.


In principle, this also applies to the m_can peripheral devices. If
tx_skb is not NULL, the TX is still in progress and returning
NETDEV_TX_BUSY is just fine.

> 

> In contrast, in "m_can_tx_handler()", the skb could not be handled

> because the FIFO is full. The "start_xmit" routine for peripheral

> devices for that skb already returned NETDEV_TX_OK. Therefore the only

> meaningful action is to drop the skb. Also this error should not happen

> and if, something is going really wrong. Therefore I think, a

> WARN_ONCE() would be even more appropriate. But that should be a

> separate patch.


But that's a different issue/error. The tx_skb cannot be processed in
"m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>

>> In addition in the peripheral context the work queue does not report up to the upper layer the status.

>> Again the hi3110 and mcp251x drivers are written this way.

>>

>> The only issue I see here is that the dropped and invalid check needs to come after the tx_skb check.

> 

> See above.


Wolfgang.
Dan Murphy March 8, 2019, 3:48 p.m. UTC | #5
Wolfgang

On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:
> Hello Dan,

> 

> thinking more about it...

> 

> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>> Hello Dan,

>>

>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>> Wolfgang

>>>

>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>> Hallo Dan,

>>>>

>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>> Create a m_can platform framework that peripherial

>>>>> devices can register to and use common code and register sets.

>>>>> The peripherial devices may provide read/write and configuration

>>>>> support of the IP.

>>>>>

>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>> ---

>>>>>

>>>>>

>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>

>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>

>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>

>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>> @@ -1,5 +1,14 @@

>>>>>  config CAN_M_CAN

>>>>> +	tristate "Bosch M_CAN support"

>>>>> +	---help---

>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>> +

>>>>> +config CAN_M_CAN_PLATFORM

>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>  	depends on HAS_IOMEM

>>>>> -	tristate "Bosch M_CAN devices"

>>>>> +	depends on CAN_M_CAN

>>>>>  	---help---

>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>> @@ -3,3 +3,4 @@

>>>>>  #

>>>>>  

>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>> index 9b449400376b..a60278d94126 100644

>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>

>>>> ... snip...

>>>>

>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>> +				    struct net_device *dev)

>>>>> +{

>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>> +

>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>> +		return NETDEV_TX_OK;

>>>>> +

>>>>> +	if (priv->is_peripherial) {

>>>>> +		if (priv->tx_skb) {

>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>> +			return NETDEV_TX_BUSY;

>>>>> +		}

>>>>

>>>> The problem with that approach is, that the upper layer will try to

>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>> (like can_dropped_invalid_skb() does).

>>>>

>>>

>>> OK.

>>>

>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>> this is how these drivers are written.

>>

>> This is different. When entering the "start_xmit" routine, the previous

>> TX is still in progress. It will (hopefully) complete soon. Therefore

>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>> recalled soon with the same "skb". That scenario should/could also not

>> happen.

> 

> In principle, this also applies to the m_can peripheral devices. If

> tx_skb is not NULL, the TX is still in progress and returning

> NETDEV_TX_BUSY is just fine.

> 

>>

>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>> because the FIFO is full. The "start_xmit" routine for peripheral

>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>> meaningful action is to drop the skb. Also this error should not happen

>> and if, something is going really wrong. Therefore I think, a

>> WARN_ONCE() would be even more appropriate. But that should be a

>> separate patch.

> 

> But that's a different issue/error. The tx_skb cannot be processed in

> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

> 


OK I am a bit confused on this.  Are you saying this is not an issue?
Or are you saying I need to check for tx_len like the other code?

Again if this code is an issue here I believe this is an issue in the hi3110 and mcp251x

Dan


>>>

>>> In addition in the peripheral context the work queue does not report up to the upper layer the status.

>>> Again the hi3110 and mcp251x drivers are written this way.

>>>

>>> The only issue I see here is that the dropped and invalid check needs to come after the tx_skb check.

>>

>> See above.

> 

> Wolfgang.

> 



-- 
------------------
Dan Murphy
Wolfgang Grandegger March 8, 2019, 5:08 p.m. UTC | #6
Hello,

Am 08.03.19 um 16:48 schrieb Dan Murphy:
> Wolfgang

> 

> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>> Hello Dan,

>>

>> thinking more about it...

>>

>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>> Hello Dan,

>>>

>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>> Wolfgang

>>>>

>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>> Hallo Dan,

>>>>>

>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>> Create a m_can platform framework that peripherial

>>>>>> devices can register to and use common code and register sets.

>>>>>> The peripherial devices may provide read/write and configuration

>>>>>> support of the IP.

>>>>>>

>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>> ---

>>>>>>

>>>>>>

>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>

>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>

>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>

>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>> @@ -1,5 +1,14 @@

>>>>>>  config CAN_M_CAN

>>>>>> +	tristate "Bosch M_CAN support"

>>>>>> +	---help---

>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>> +

>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>  	depends on HAS_IOMEM

>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>> +	depends on CAN_M_CAN

>>>>>>  	---help---

>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>> @@ -3,3 +3,4 @@

>>>>>>  #

>>>>>>  

>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>

>>>>> ... snip...

>>>>>

>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>> +				    struct net_device *dev)

>>>>>> +{

>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>> +

>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>> +		return NETDEV_TX_OK;

>>>>>> +

>>>>>> +	if (priv->is_peripherial) {

>>>>>> +		if (priv->tx_skb) {

>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>> +			return NETDEV_TX_BUSY;

>>>>>> +		}

>>>>>

>>>>> The problem with that approach is, that the upper layer will try to

>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>> (like can_dropped_invalid_skb() does).

>>>>>

>>>>

>>>> OK.

>>>>

>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>> this is how these drivers are written.

>>>

>>> This is different. When entering the "start_xmit" routine, the previous

>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>> recalled soon with the same "skb". That scenario should/could also not

>>> happen.

>>

>> In principle, this also applies to the m_can peripheral devices. If

>> tx_skb is not NULL, the TX is still in progress and returning

>> NETDEV_TX_BUSY is just fine.

>>

>>>

>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>> meaningful action is to drop the skb. Also this error should not happen

>>> and if, something is going really wrong. Therefore I think, a

>>> WARN_ONCE() would be even more appropriate. But that should be a

>>> separate patch.

>>

>> But that's a different issue/error. The tx_skb cannot be processed in

>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>

> 

> OK I am a bit confused on this.  Are you saying this is not an issue?

> Or are you saying I need to check for tx_len like the other code?


If you check for tx_skb in the "start_xmit" routine like the hi3110 and
mcp251x, it will work the same way. But only, if the "tx_handler()" has
fully processed the message. It simple means, the TX is still in
progress and will complete soon. But in "m_can_tx_handler()" we return
without handling the message! It will never be sent and freed. Or will
the "m_can_tx_handler()" retry?

> Again if this code is an issue here I believe this is an issue in the hi3110 and mcp251x


I don't think so.

Sorry for confusion.

Wolfgang.
Dan Murphy March 8, 2019, 5:25 p.m. UTC | #7
On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:
> Hello,

> 

> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>> Wolfgang

>>

>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>> Hello Dan,

>>>

>>> thinking more about it...

>>>

>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>> Hello Dan,

>>>>

>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>> Wolfgang

>>>>>

>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>> Hallo Dan,

>>>>>>

>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>> Create a m_can platform framework that peripherial

>>>>>>> devices can register to and use common code and register sets.

>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>> support of the IP.

>>>>>>>

>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>> ---

>>>>>>>

>>>>>>>

>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>

>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>

>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>

>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>  config CAN_M_CAN

>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>> +	---help---

>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>> +

>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>  	depends on HAS_IOMEM

>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>> +	depends on CAN_M_CAN

>>>>>>>  	---help---

>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>  #

>>>>>>>  

>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>

>>>>>> ... snip...

>>>>>>

>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>> +				    struct net_device *dev)

>>>>>>> +{

>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>> +

>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>> +		return NETDEV_TX_OK;

>>>>>>> +

>>>>>>> +	if (priv->is_peripherial) {

>>>>>>> +		if (priv->tx_skb) {

>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>> +		}

>>>>>>

>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>

>>>>>

>>>>> OK.

>>>>>

>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>> this is how these drivers are written.

>>>>

>>>> This is different. When entering the "start_xmit" routine, the previous

>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>> recalled soon with the same "skb". That scenario should/could also not

>>>> happen.

>>>

>>> In principle, this also applies to the m_can peripheral devices. If

>>> tx_skb is not NULL, the TX is still in progress and returning

>>> NETDEV_TX_BUSY is just fine.

>>>

>>>>

>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>> meaningful action is to drop the skb. Also this error should not happen

>>>> and if, something is going really wrong. Therefore I think, a

>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>> separate patch.

>>>

>>> But that's a different issue/error. The tx_skb cannot be processed in

>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>

>>

>> OK I am a bit confused on this.  Are you saying this is not an issue?

>> Or are you saying I need to check for tx_len like the other code?

> 

> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

> mcp251x, it will work the same way. But only, if the "tx_handler()" has

> fully processed the message. It simple means, the TX is still in

> progress and will complete soon. But in "m_can_tx_handler()" we return

> without handling the message! It will never be sent and freed. Or will

> the "m_can_tx_handler()" retry?

> 


I am not seeing where we are not handling the message in the m_can_tx_handler()

In the peripheral code the work is queued up.  And the work thread is m_can_tx_work_queue.

This in turn calls the m_can_tx_handler and the worker is blocked until return which means the message
would have been processed.

If there is no issue and the handler returns OK then the skb is set to null.
Otherwise the only other time that the skb will not be null is if the FIFO was full.

Plus there can only be one work queue at a time so the processing is synchronous.
If the upper layer decides to send another packet before the prior one is complete then it will get
a TX busy return.

IOmapped calls are blocked on return so this is not an issue.  We cannot do it the same way with peripherals due to the
atomic context of the request.

Dan

>> Again if this code is an issue here I believe this is an issue in the hi3110 and mcp251x

> 

> I don't think so.

> 

> Sorry for confusion.

> 

> Wolfgang.

> 



-- 
------------------
Dan Murphy
Wolfgang Grandegger March 8, 2019, 5:40 p.m. UTC | #8
Hello Dan,

Am 08.03.19 um 18:25 schrieb Dan Murphy:
> On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:

>> Hello,

>>

>> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>>> Wolfgang

>>>

>>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>>> Hello Dan,

>>>>

>>>> thinking more about it...

>>>>

>>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>>> Hello Dan,

>>>>>

>>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>>> Wolfgang

>>>>>>

>>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>>> Hallo Dan,

>>>>>>>

>>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>>> Create a m_can platform framework that peripherial

>>>>>>>> devices can register to and use common code and register sets.

>>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>>> support of the IP.

>>>>>>>>

>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>> ---

>>>>>>>>

>>>>>>>>

>>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>>

>>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>>

>>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>>

>>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>>  config CAN_M_CAN

>>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>>> +	---help---

>>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>>> +

>>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>>  	depends on HAS_IOMEM

>>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>>> +	depends on CAN_M_CAN

>>>>>>>>  	---help---

>>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>>  #

>>>>>>>>  

>>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>>

>>>>>>> ... snip...

>>>>>>>

>>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>>> +				    struct net_device *dev)

>>>>>>>> +{

>>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>>> +

>>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>>> +		return NETDEV_TX_OK;

>>>>>>>> +

>>>>>>>> +	if (priv->is_peripherial) {

>>>>>>>> +		if (priv->tx_skb) {

>>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>>> +		}

>>>>>>>

>>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>>

>>>>>>

>>>>>> OK.

>>>>>>

>>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>>> this is how these drivers are written.

>>>>>

>>>>> This is different. When entering the "start_xmit" routine, the previous

>>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>>> recalled soon with the same "skb". That scenario should/could also not

>>>>> happen.

>>>>

>>>> In principle, this also applies to the m_can peripheral devices. If

>>>> tx_skb is not NULL, the TX is still in progress and returning

>>>> NETDEV_TX_BUSY is just fine.

>>>>

>>>>>

>>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>>> meaningful action is to drop the skb. Also this error should not happen

>>>>> and if, something is going really wrong. Therefore I think, a

>>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>>> separate patch.

>>>>

>>>> But that's a different issue/error. The tx_skb cannot be processed in

>>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>>

>>>

>>> OK I am a bit confused on this.  Are you saying this is not an issue?

>>> Or are you saying I need to check for tx_len like the other code?

>>

>> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

>> mcp251x, it will work the same way. But only, if the "tx_handler()" has

>> fully processed the message. It simple means, the TX is still in

>> progress and will complete soon. But in "m_can_tx_handler()" we return

>> without handling the message! It will never be sent and freed. Or will

>> the "m_can_tx_handler()" retry?

>>

> 

> I am not seeing where we are not handling the message in the m_can_tx_handler()


static void m_can_tx_handler(struct m_can_classdev *priv)
{
		...
		/* Check if FIFO full */
		if (m_can_tx_fifo_full(priv)) {
			/* This shouldn't happen */
			netif_stop_queue(dev);
			netdev_warn(dev,
				    "TX queue active although FIFO is full.");
			return;
		}

We simply return here. When is the message (tx_skb) processed (sent or freed)?
What happens with tx_skb?


For the hi3110, we have:

static void hi3110_tx_work_handler(struct work_struct *ws)
{
        struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,
                                                tx_work);
        struct spi_device *spi = priv->spi;
        struct net_device *net = priv->net;
        struct can_frame *frame;

        mutex_lock(&priv->hi3110_lock);
        if (priv->tx_skb) {
                if (priv->can.state == CAN_STATE_BUS_OFF) {
                        hi3110_clean(net);
                } else {
                        frame = (struct can_frame *)priv->tx_skb->data;
                        hi3110_hw_tx(spi, frame);
                        priv->tx_len = 1 + frame->can_dlc;
                        can_put_echo_skb(priv->tx_skb, net, 0);
                        priv->tx_skb = NULL;
                }
        }
        mutex_unlock(&priv->hi3110_lock);
}

Either the tx_skb is sent or cleanup (dropped and freed) in case of bus-off.
Also "hi3110_clean" sets "priv->tx_skb = NULL"! The "tx_len" handles a pending 
"echo_skb".

> 

> In the peripheral code the work is queued up.  And the work thread is m_can_tx_work_queue.

> 

> This in turn calls the m_can_tx_handler and the worker is blocked until return which means the message

> would have been processed.

> 

> If there is no issue and the handler returns OK then the skb is set to null.

> Otherwise the only other time that the skb will not be null is if the FIFO was full.

> 

> Plus there can only be one work queue at a time so the processing is synchronous.

> If the upper layer decides to send another packet before the prior one is complete then it will get

> a TX busy return.

> 

> IOmapped calls are blocked on return so this is not an issue.  We cannot do it the same way with peripherals due to the

> atomic context of the request.

Wolfgang.
Dan Murphy March 8, 2019, 5:52 p.m. UTC | #9
On 3/8/19 11:40 AM, Wolfgang Grandegger wrote:
> Hello Dan,

> 

> Am 08.03.19 um 18:25 schrieb Dan Murphy:

>> On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:

>>> Hello,

>>>

>>> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>>>> Wolfgang

>>>>

>>>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>>>> Hello Dan,

>>>>>

>>>>> thinking more about it...

>>>>>

>>>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>>>> Hello Dan,

>>>>>>

>>>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>>>> Wolfgang

>>>>>>>

>>>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>>>> Hallo Dan,

>>>>>>>>

>>>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>>>> Create a m_can platform framework that peripherial

>>>>>>>>> devices can register to and use common code and register sets.

>>>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>>>> support of the IP.

>>>>>>>>>

>>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>>> ---

>>>>>>>>>

>>>>>>>>>

>>>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>>>

>>>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>>>

>>>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>>>

>>>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>>>  config CAN_M_CAN

>>>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>>>> +	---help---

>>>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>>>> +

>>>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>>>  	depends on HAS_IOMEM

>>>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>>>> +	depends on CAN_M_CAN

>>>>>>>>>  	---help---

>>>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>>>  #

>>>>>>>>>  

>>>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>>>

>>>>>>>> ... snip...

>>>>>>>>

>>>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>>>> +				    struct net_device *dev)

>>>>>>>>> +{

>>>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>>>> +

>>>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>>>> +		return NETDEV_TX_OK;

>>>>>>>>> +

>>>>>>>>> +	if (priv->is_peripherial) {

>>>>>>>>> +		if (priv->tx_skb) {

>>>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>>>> +		}

>>>>>>>>

>>>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>>>

>>>>>>>

>>>>>>> OK.

>>>>>>>

>>>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>>>> this is how these drivers are written.

>>>>>>

>>>>>> This is different. When entering the "start_xmit" routine, the previous

>>>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>>>> recalled soon with the same "skb". That scenario should/could also not

>>>>>> happen.

>>>>>

>>>>> In principle, this also applies to the m_can peripheral devices. If

>>>>> tx_skb is not NULL, the TX is still in progress and returning

>>>>> NETDEV_TX_BUSY is just fine.

>>>>>

>>>>>>

>>>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>>>> meaningful action is to drop the skb. Also this error should not happen

>>>>>> and if, something is going really wrong. Therefore I think, a

>>>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>>>> separate patch.

>>>>>

>>>>> But that's a different issue/error. The tx_skb cannot be processed in

>>>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>>>

>>>>

>>>> OK I am a bit confused on this.  Are you saying this is not an issue?

>>>> Or are you saying I need to check for tx_len like the other code?

>>>

>>> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

>>> mcp251x, it will work the same way. But only, if the "tx_handler()" has

>>> fully processed the message. It simple means, the TX is still in

>>> progress and will complete soon. But in "m_can_tx_handler()" we return

>>> without handling the message! It will never be sent and freed. Or will

>>> the "m_can_tx_handler()" retry?

>>>

>>

>> I am not seeing where we are not handling the message in the m_can_tx_handler()

> 

> static void m_can_tx_handler(struct m_can_classdev *priv)

> {

> 		...

> 		/* Check if FIFO full */

> 		if (m_can_tx_fifo_full(priv)) {

> 			/* This shouldn't happen */

> 			netif_stop_queue(dev);

> 			netdev_warn(dev,

> 				    "TX queue active although FIFO is full.");

> 			return;

> 		}

> 

> We simply return here. When is the message (tx_skb) processed (sent or freed)?

> What happens with tx_skb?

> 


Are you sure you are looking at the right code?

For patch version v7 I have the following

		/* Check if FIFO full */
		if (m_can_tx_fifo_full(cdev)) {
			/* This shouldn't happen */
			netif_stop_queue(dev);
			netdev_warn(dev,
				    "TX queue active although FIFO is full.");
			return NETDEV_TX_BUSY;
		}

Which is no change from the original source code.

> 

> For the hi3110, we have:

> 

> static void hi3110_tx_work_handler(struct work_struct *ws)

> {

>         struct hi3110_priv *priv = container_of(ws, struct hi3110_priv,

>                                                 tx_work);

>         struct spi_device *spi = priv->spi;

>         struct net_device *net = priv->net;

>         struct can_frame *frame;

> 

>         mutex_lock(&priv->hi3110_lock);

>         if (priv->tx_skb) {

>                 if (priv->can.state == CAN_STATE_BUS_OFF) {

>                         hi3110_clean(net);

>                 } else {

>                         frame = (struct can_frame *)priv->tx_skb->data;

>                         hi3110_hw_tx(spi, frame);

>                         priv->tx_len = 1 + frame->can_dlc;

>                         can_put_echo_skb(priv->tx_skb, net, 0);

>                         priv->tx_skb = NULL;

>                 }

>         }

>         mutex_unlock(&priv->hi3110_lock);

> }

> 

> Either the tx_skb is sent or cleanup (dropped and freed) in case of bus-off.

> Also "hi3110_clean" sets "priv->tx_skb = NULL"! The "tx_len" handles a pending 

> "echo_skb".

> 

>>

>> In the peripheral code the work is queued up.  And the work thread is m_can_tx_work_queue.

>>

>> This in turn calls the m_can_tx_handler and the worker is blocked until return which means the message

>> would have been processed.

>>

>> If there is no issue and the handler returns OK then the skb is set to null.

>> Otherwise the only other time that the skb will not be null is if the FIFO was full.

>>

>> Plus there can only be one work queue at a time so the processing is synchronous.

>> If the upper layer decides to send another packet before the prior one is complete then it will get

>> a TX busy return.

>>

>> IOmapped calls are blocked on return so this is not an issue.  We cannot do it the same way with peripherals due to the

>> atomic context of the request.

> Wolfgang.

> 



-- 
------------------
Dan Murphy
Wolfgang Grandegger March 8, 2019, 6:06 p.m. UTC | #10
Am 08.03.19 um 18:52 schrieb Dan Murphy:
> On 3/8/19 11:40 AM, Wolfgang Grandegger wrote:

>> Hello Dan,

>>

>> Am 08.03.19 um 18:25 schrieb Dan Murphy:

>>> On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:

>>>> Hello,

>>>>

>>>> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>>>>> Wolfgang

>>>>>

>>>>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>>>>> Hello Dan,

>>>>>>

>>>>>> thinking more about it...

>>>>>>

>>>>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>>>>> Hello Dan,

>>>>>>>

>>>>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>>>>> Wolfgang

>>>>>>>>

>>>>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>>>>> Hallo Dan,

>>>>>>>>>

>>>>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>>>>> Create a m_can platform framework that peripherial

>>>>>>>>>> devices can register to and use common code and register sets.

>>>>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>>>>> support of the IP.

>>>>>>>>>>

>>>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>>>> ---

>>>>>>>>>>

>>>>>>>>>>

>>>>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>>>>

>>>>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>>>>

>>>>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>>>>

>>>>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>>>>  config CAN_M_CAN

>>>>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>>>>> +	---help---

>>>>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>>>>> +

>>>>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>>>>  	depends on HAS_IOMEM

>>>>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>>>>> +	depends on CAN_M_CAN

>>>>>>>>>>  	---help---

>>>>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>>>>  #

>>>>>>>>>>  

>>>>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>>>>

>>>>>>>>> ... snip...

>>>>>>>>>

>>>>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>>>>> +				    struct net_device *dev)

>>>>>>>>>> +{

>>>>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>>>>> +

>>>>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>>>>> +		return NETDEV_TX_OK;

>>>>>>>>>> +

>>>>>>>>>> +	if (priv->is_peripherial) {

>>>>>>>>>> +		if (priv->tx_skb) {

>>>>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>>>>> +		}

>>>>>>>>>

>>>>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>>>>

>>>>>>>>

>>>>>>>> OK.

>>>>>>>>

>>>>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>>>>> this is how these drivers are written.

>>>>>>>

>>>>>>> This is different. When entering the "start_xmit" routine, the previous

>>>>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>>>>> recalled soon with the same "skb". That scenario should/could also not

>>>>>>> happen.

>>>>>>

>>>>>> In principle, this also applies to the m_can peripheral devices. If

>>>>>> tx_skb is not NULL, the TX is still in progress and returning

>>>>>> NETDEV_TX_BUSY is just fine.

>>>>>>

>>>>>>>

>>>>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>>>>> meaningful action is to drop the skb. Also this error should not happen

>>>>>>> and if, something is going really wrong. Therefore I think, a

>>>>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>>>>> separate patch.

>>>>>>

>>>>>> But that's a different issue/error. The tx_skb cannot be processed in

>>>>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>>>>

>>>>>

>>>>> OK I am a bit confused on this.  Are you saying this is not an issue?

>>>>> Or are you saying I need to check for tx_len like the other code?

>>>>

>>>> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

>>>> mcp251x, it will work the same way. But only, if the "tx_handler()" has

>>>> fully processed the message. It simple means, the TX is still in

>>>> progress and will complete soon. But in "m_can_tx_handler()" we return

>>>> without handling the message! It will never be sent and freed. Or will

>>>> the "m_can_tx_handler()" retry?

>>>>

>>>

>>> I am not seeing where we are not handling the message in the m_can_tx_handler()

>>

>> static void m_can_tx_handler(struct m_can_classdev *priv)

>> {

>> 		...

>> 		/* Check if FIFO full */

>> 		if (m_can_tx_fifo_full(priv)) {

>> 			/* This shouldn't happen */

>> 			netif_stop_queue(dev);

>> 			netdev_warn(dev,

>> 				    "TX queue active although FIFO is full.");

>> 			return;

>> 		}

>>

>> We simply return here. When is the message (tx_skb) processed (sent or freed)?

>> What happens with tx_skb?

>>

> 

> Are you sure you are looking at the right code?

> 

> For patch version v7 I have the following

> 

> 		/* Check if FIFO full */

> 		if (m_can_tx_fifo_full(cdev)) {

> 			/* This shouldn't happen */

> 			netif_stop_queue(dev);

> 			netdev_warn(dev,

> 				    "TX queue active although FIFO is full.");

> 			return NETDEV_TX_BUSY;

> 		}

> 

> Which is no change from the original source code.


I know,  but for the peripheral devices you have:

  static void m_can_tx_work_queue(struct work_struct *ws)
  {
	struct m_can_priv *priv = container_of(ws, struct m_can_priv,
						tx_work);
	netdev_tx_t ret;

	ret = m_can_tx_handler(priv);
	if (ret == NETDEV_TX_OK)
		priv->tx_skb = NULL;
  }

What will happen with tx_skb if NETDEV_TX_BUSY? It has not been
dropped/freed yet?

Wolfgang.
Dan Murphy March 8, 2019, 8:36 p.m. UTC | #11
On 3/8/19 12:06 PM, Wolfgang Grandegger wrote:
> 

> 

> Am 08.03.19 um 18:52 schrieb Dan Murphy:

>> On 3/8/19 11:40 AM, Wolfgang Grandegger wrote:

>>> Hello Dan,

>>>

>>> Am 08.03.19 um 18:25 schrieb Dan Murphy:

>>>> On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:

>>>>> Hello,

>>>>>

>>>>> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>>>>>> Wolfgang

>>>>>>

>>>>>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>>>>>> Hello Dan,

>>>>>>>

>>>>>>> thinking more about it...

>>>>>>>

>>>>>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>>>>>> Hello Dan,

>>>>>>>>

>>>>>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>>>>>> Wolfgang

>>>>>>>>>

>>>>>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>>>>>> Hallo Dan,

>>>>>>>>>>

>>>>>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>>>>>> Create a m_can platform framework that peripherial

>>>>>>>>>>> devices can register to and use common code and register sets.

>>>>>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>>>>>> support of the IP.

>>>>>>>>>>>

>>>>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>>>>> ---

>>>>>>>>>>>

>>>>>>>>>>>

>>>>>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>>>>>

>>>>>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>>>>>

>>>>>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>>>>>

>>>>>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>>>>>  config CAN_M_CAN

>>>>>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>>>>>> +	---help---

>>>>>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>>>>>> +

>>>>>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>>>>>  	depends on HAS_IOMEM

>>>>>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>>>>>> +	depends on CAN_M_CAN

>>>>>>>>>>>  	---help---

>>>>>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>>>>>  #

>>>>>>>>>>>  

>>>>>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>>>>>

>>>>>>>>>> ... snip...

>>>>>>>>>>

>>>>>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>>>>>> +				    struct net_device *dev)

>>>>>>>>>>> +{

>>>>>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>>>>>> +

>>>>>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>>>>>> +		return NETDEV_TX_OK;

>>>>>>>>>>> +

>>>>>>>>>>> +	if (priv->is_peripherial) {

>>>>>>>>>>> +		if (priv->tx_skb) {

>>>>>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>>>>>> +		}

>>>>>>>>>>

>>>>>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>>>>>

>>>>>>>>>

>>>>>>>>> OK.

>>>>>>>>>

>>>>>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>>>>>> this is how these drivers are written.

>>>>>>>>

>>>>>>>> This is different. When entering the "start_xmit" routine, the previous

>>>>>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>>>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>>>>>> recalled soon with the same "skb". That scenario should/could also not

>>>>>>>> happen.

>>>>>>>

>>>>>>> In principle, this also applies to the m_can peripheral devices. If

>>>>>>> tx_skb is not NULL, the TX is still in progress and returning

>>>>>>> NETDEV_TX_BUSY is just fine.

>>>>>>>

>>>>>>>>

>>>>>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>>>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>>>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>>>>>> meaningful action is to drop the skb. Also this error should not happen

>>>>>>>> and if, something is going really wrong. Therefore I think, a

>>>>>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>>>>>> separate patch.

>>>>>>>

>>>>>>> But that's a different issue/error. The tx_skb cannot be processed in

>>>>>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>>>>>

>>>>>>

>>>>>> OK I am a bit confused on this.  Are you saying this is not an issue?

>>>>>> Or are you saying I need to check for tx_len like the other code?

>>>>>

>>>>> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

>>>>> mcp251x, it will work the same way. But only, if the "tx_handler()" has

>>>>> fully processed the message. It simple means, the TX is still in

>>>>> progress and will complete soon. But in "m_can_tx_handler()" we return

>>>>> without handling the message! It will never be sent and freed. Or will

>>>>> the "m_can_tx_handler()" retry?

>>>>>

>>>>

>>>> I am not seeing where we are not handling the message in the m_can_tx_handler()

>>>

>>> static void m_can_tx_handler(struct m_can_classdev *priv)

>>> {

>>> 		...

>>> 		/* Check if FIFO full */

>>> 		if (m_can_tx_fifo_full(priv)) {

>>> 			/* This shouldn't happen */

>>> 			netif_stop_queue(dev);

>>> 			netdev_warn(dev,

>>> 				    "TX queue active although FIFO is full.");

>>> 			return;

>>> 		}

>>>

>>> We simply return here. When is the message (tx_skb) processed (sent or freed)?

>>> What happens with tx_skb?

>>>

>>

>> Are you sure you are looking at the right code?

>>

>> For patch version v7 I have the following

>>

>> 		/* Check if FIFO full */

>> 		if (m_can_tx_fifo_full(cdev)) {

>> 			/* This shouldn't happen */

>> 			netif_stop_queue(dev);

>> 			netdev_warn(dev,

>> 				    "TX queue active although FIFO is full.");

>> 			return NETDEV_TX_BUSY;

>> 		}

>>

>> Which is no change from the original source code.

> 

> I know,  but for the peripheral devices you have:

> 

>   static void m_can_tx_work_queue(struct work_struct *ws)

>   {

> 	struct m_can_priv *priv = container_of(ws, struct m_can_priv,

> 						tx_work);

> 	netdev_tx_t ret;

> 

> 	ret = m_can_tx_handler(priv);

> 	if (ret == NETDEV_TX_OK)

> 		priv->tx_skb = NULL;

>   }

> 

> What will happen with tx_skb if NETDEV_TX_BUSY? It has not been

> dropped/freed yet?

> 


OK I think I see the issue there.

I should probably add can_put_echo_skb if NETDEV_TX_BUSY and always NULL out the SKB.

This appears to be the way the other perp drivers do it as they just put and null the skb
regardless of the return of the handlers.

And clean is called when the BUS is off or coming out of suspend.

Dan

> Wolfgang.

> 



-- 
------------------
Dan Murphy
Wolfgang Grandegger March 11, 2019, 11:19 a.m. UTC | #12
Hello Dan,

Am 08.03.19 um 21:36 schrieb Dan Murphy:
> On 3/8/19 12:06 PM, Wolfgang Grandegger wrote:

>>

>>

>> Am 08.03.19 um 18:52 schrieb Dan Murphy:

>>> On 3/8/19 11:40 AM, Wolfgang Grandegger wrote:

>>>> Hello Dan,

>>>>

>>>> Am 08.03.19 um 18:25 schrieb Dan Murphy:

>>>>> On 3/8/19 11:08 AM, Wolfgang Grandegger wrote:

>>>>>> Hello,

>>>>>>

>>>>>> Am 08.03.19 um 16:48 schrieb Dan Murphy:

>>>>>>> Wolfgang

>>>>>>>

>>>>>>> On 3/8/19 8:41 AM, Wolfgang Grandegger wrote:

>>>>>>>> Hello Dan,

>>>>>>>>

>>>>>>>> thinking more about it...

>>>>>>>>

>>>>>>>> Am 08.03.19 um 14:29 schrieb Wolfgang Grandegger:

>>>>>>>>> Hello Dan,

>>>>>>>>>

>>>>>>>>> Am 08.03.19 um 13:44 schrieb Dan Murphy:

>>>>>>>>>> Wolfgang

>>>>>>>>>>

>>>>>>>>>> On 3/8/19 4:10 AM, Wolfgang Grandegger wrote:

>>>>>>>>>>> Hallo Dan,

>>>>>>>>>>>

>>>>>>>>>>> Am 05.03.19 um 16:52 schrieb Dan Murphy:

>>>>>>>>>>>> Create a m_can platform framework that peripherial

>>>>>>>>>>>> devices can register to and use common code and register sets.

>>>>>>>>>>>> The peripherial devices may provide read/write and configuration

>>>>>>>>>>>> support of the IP.

>>>>>>>>>>>>

>>>>>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>>>>>>>>>>>> ---

>>>>>>>>>>>>

>>>>>>>>>>>>

>>>>>>>>>>>> v7 - Fixed remaining new checkpatch issues, removed CSR setting, fixed tx hard

>>>>>>>>>>>> start function to return tx_busy, and renamed device callbacks - https://lore.kernel.org/patchwork/patch/1047220/

>>>>>>>>>>>>

>>>>>>>>>>>> v6 - Squashed platform patch to this patch for bissectablity, fixed coding style

>>>>>>>>>>>> issues, updated Kconfig help, placed mcan reg offsets back into c file, renamed

>>>>>>>>>>>> priv->skb to priv->tx_skb and cleared perp interrupts at ISR start -

>>>>>>>>>>>> Patch 1 comments - https://lore.kernel.org/patchwork/patch/1042446/

>>>>>>>>>>>> Patch 2 comments - https://lore.kernel.org/patchwork/patch/1042442/

>>>>>>>>>>>>

>>>>>>>>>>>>  drivers/net/can/m_can/Kconfig          |  13 +-

>>>>>>>>>>>>  drivers/net/can/m_can/Makefile         |   1 +

>>>>>>>>>>>>  drivers/net/can/m_can/m_can.c          | 700 +++++++++++++------------

>>>>>>>>>>>>  drivers/net/can/m_can/m_can.h          | 110 ++++

>>>>>>>>>>>>  drivers/net/can/m_can/m_can_platform.c | 202 +++++++

>>>>>>>>>>>>  5 files changed, 682 insertions(+), 344 deletions(-)

>>>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can.h

>>>>>>>>>>>>  create mode 100644 drivers/net/can/m_can/m_can_platform.c

>>>>>>>>>>>>

>>>>>>>>>>>> diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig

>>>>>>>>>>>> index 04f20dd39007..f7119fd72df4 100644

>>>>>>>>>>>> --- a/drivers/net/can/m_can/Kconfig

>>>>>>>>>>>> +++ b/drivers/net/can/m_can/Kconfig

>>>>>>>>>>>> @@ -1,5 +1,14 @@

>>>>>>>>>>>>  config CAN_M_CAN

>>>>>>>>>>>> +	tristate "Bosch M_CAN support"

>>>>>>>>>>>> +	---help---

>>>>>>>>>>>> +	  Say Y here if you want support for Bosch M_CAN controller framework.

>>>>>>>>>>>> +	  This is common support for devices that embed the Bosch M_CAN IP.

>>>>>>>>>>>> +

>>>>>>>>>>>> +config CAN_M_CAN_PLATFORM

>>>>>>>>>>>> +	tristate "Bosch M_CAN support for io-mapped devices"

>>>>>>>>>>>>  	depends on HAS_IOMEM

>>>>>>>>>>>> -	tristate "Bosch M_CAN devices"

>>>>>>>>>>>> +	depends on CAN_M_CAN

>>>>>>>>>>>>  	---help---

>>>>>>>>>>>> -	  Say Y here if you want to support for Bosch M_CAN controller.

>>>>>>>>>>>> +	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.

>>>>>>>>>>>> +	  This support is for devices that have the Bosch M_CAN controller

>>>>>>>>>>>> +	  IP embedded into the device and the IP is IO Mapped to the processor.

>>>>>>>>>>>> diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile

>>>>>>>>>>>> index 8bbd7f24f5be..057bbcdb3c74 100644

>>>>>>>>>>>> --- a/drivers/net/can/m_can/Makefile

>>>>>>>>>>>> +++ b/drivers/net/can/m_can/Makefile

>>>>>>>>>>>> @@ -3,3 +3,4 @@

>>>>>>>>>>>>  #

>>>>>>>>>>>>  

>>>>>>>>>>>>  obj-$(CONFIG_CAN_M_CAN) += m_can.o

>>>>>>>>>>>> +obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o

>>>>>>>>>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c

>>>>>>>>>>>> index 9b449400376b..a60278d94126 100644

>>>>>>>>>>>> --- a/drivers/net/can/m_can/m_can.c

>>>>>>>>>>>> +++ b/drivers/net/can/m_can/m_can.c

>>>>>>>>>>>

>>>>>>>>>>> ... snip...

>>>>>>>>>>>

>>>>>>>>>>>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,

>>>>>>>>>>>> +				    struct net_device *dev)

>>>>>>>>>>>> +{

>>>>>>>>>>>> +	struct m_can_priv *priv = netdev_priv(dev);

>>>>>>>>>>>> +

>>>>>>>>>>>> +	if (can_dropped_invalid_skb(dev, skb))

>>>>>>>>>>>> +		return NETDEV_TX_OK;

>>>>>>>>>>>> +

>>>>>>>>>>>> +	if (priv->is_peripherial) {

>>>>>>>>>>>> +		if (priv->tx_skb) {

>>>>>>>>>>>> +			netdev_err(dev, "hard_xmit called while tx busy\n");

>>>>>>>>>>>> +			return NETDEV_TX_BUSY;

>>>>>>>>>>>> +		}

>>>>>>>>>>>

>>>>>>>>>>> The problem with that approach is, that the upper layer will try to

>>>>>>>>>>> resubmit the current "skb" but not the previous "tx_skb". And the

>>>>>>>>>>> previous "tx_skb" has not been freed yet. I would just drop and free the

>>>>>>>>>>> skb and return NETDEV_TX_OK in m_can_tx_handler() for peripheral devices

>>>>>>>>>>> (like can_dropped_invalid_skb() does).

>>>>>>>>>>>

>>>>>>>>>>

>>>>>>>>>> OK.

>>>>>>>>>>

>>>>>>>>>> So would this also be a bug in the hi3110 and mcp251x drivers (line 521) as well because besides checking tx_length

>>>>>>>>>> this is how these drivers are written.

>>>>>>>>>

>>>>>>>>> This is different. When entering the "start_xmit" routine, the previous

>>>>>>>>> TX is still in progress. It will (hopefully) complete soon. Therefore

>>>>>>>>> returning NETDEV_TX_BUSY is OK. The "start_xmit" routine will be

>>>>>>>>> recalled soon with the same "skb". That scenario should/could also not

>>>>>>>>> happen.

>>>>>>>>

>>>>>>>> In principle, this also applies to the m_can peripheral devices. If

>>>>>>>> tx_skb is not NULL, the TX is still in progress and returning

>>>>>>>> NETDEV_TX_BUSY is just fine.

>>>>>>>>

>>>>>>>>>

>>>>>>>>> In contrast, in "m_can_tx_handler()", the skb could not be handled

>>>>>>>>> because the FIFO is full. The "start_xmit" routine for peripheral

>>>>>>>>> devices for that skb already returned NETDEV_TX_OK. Therefore the only

>>>>>>>>> meaningful action is to drop the skb. Also this error should not happen

>>>>>>>>> and if, something is going really wrong. Therefore I think, a

>>>>>>>>> WARN_ONCE() would be even more appropriate. But that should be a

>>>>>>>>> separate patch.

>>>>>>>>

>>>>>>>> But that's a different issue/error. The tx_skb cannot be processed in

>>>>>>>> "m_can_tx_handler()". Either we drop it or we re-queue it (retry later).

>>>>>>>>

>>>>>>>

>>>>>>> OK I am a bit confused on this.  Are you saying this is not an issue?

>>>>>>> Or are you saying I need to check for tx_len like the other code?

>>>>>>

>>>>>> If you check for tx_skb in the "start_xmit" routine like the hi3110 and

>>>>>> mcp251x, it will work the same way. But only, if the "tx_handler()" has

>>>>>> fully processed the message. It simple means, the TX is still in

>>>>>> progress and will complete soon. But in "m_can_tx_handler()" we return

>>>>>> without handling the message! It will never be sent and freed. Or will

>>>>>> the "m_can_tx_handler()" retry?

>>>>>>

>>>>>

>>>>> I am not seeing where we are not handling the message in the m_can_tx_handler()

>>>>

>>>> static void m_can_tx_handler(struct m_can_classdev *priv)

>>>> {

>>>> 		...

>>>> 		/* Check if FIFO full */

>>>> 		if (m_can_tx_fifo_full(priv)) {

>>>> 			/* This shouldn't happen */

>>>> 			netif_stop_queue(dev);

>>>> 			netdev_warn(dev,

>>>> 				    "TX queue active although FIFO is full.");

>>>> 			return;

>>>> 		}

>>>>

>>>> We simply return here. When is the message (tx_skb) processed (sent or freed)?

>>>> What happens with tx_skb?

>>>>

>>>

>>> Are you sure you are looking at the right code?

>>>

>>> For patch version v7 I have the following

>>>

>>> 		/* Check if FIFO full */

>>> 		if (m_can_tx_fifo_full(cdev)) {

>>> 			/* This shouldn't happen */

>>> 			netif_stop_queue(dev);

>>> 			netdev_warn(dev,

>>> 				    "TX queue active although FIFO is full.");

>>> 			return NETDEV_TX_BUSY;

>>> 		}

>>>

>>> Which is no change from the original source code.

>>

>> I know,  but for the peripheral devices you have:

>>

>>   static void m_can_tx_work_queue(struct work_struct *ws)

>>   {

>> 	struct m_can_priv *priv = container_of(ws, struct m_can_priv,

>> 						tx_work);

>> 	netdev_tx_t ret;

>>

>> 	ret = m_can_tx_handler(priv);

>> 	if (ret == NETDEV_TX_OK)

>> 		priv->tx_skb = NULL;

>>   }

>>

>> What will happen with tx_skb if NETDEV_TX_BUSY? It has not been

>> dropped/freed yet?

>>

> 

> OK I think I see the issue there.


The key point is that the "skb" entered by the "start_xmit" must be
released/free when it's processed (with NETDEV_TX_OK). This is more
tricky for perp devices because the "skb" is handled deferred.

> 

> I should probably add can_put_echo_skb if NETDEV_TX_BUSY and always NULL out the SKB.


can_put_echo_skb() should only be called after the TX has been
initiated. The normal flow for the skb is:

 start-xmit -> initiate tx -> can_put_echo_skb -> return NETDEV_TX_OK...
   tx done interrupt -> can_get_echo_skb -> free skb

I would just drop the message/skb:

		/* Check if FIFO full */
		if (m_can_tx_fifo_full(cdev)) {
			/* This shouldn't happen */
			netif_stop_queue(dev);
			netdev_warn(dev,
				    "TX queue active although FIFO is full.");
			if (cdev->is_peripherial) {
				kfree_skb(skb);
				dev->stats.tx_dropped++;
				return NETDEV_TX_OK;
			} else {
				return NETDEV_TX_BUSY;
			}
		}

> This appears to be the way the other perp drivers do it as they just put and null the skb

> regardless of the return of the handlers.


You can also use:

		if (cdev->tx_skb) {
			netdev_err(dev, "hard_xmit called while tx busy\n");
			return NETDEV_TX_BUSY;
		}

to check for "hard_xmit called while tx busy". But you still need to
handle the "m_can_tx_fifo_full(cdev)" case properly. See above.

> And clean is called when the BUS is off or coming out of suspend.


Probably we need that as well even if other drivers don't care if
the device goes bus-off while TX messages are pending.

Wolfgang
diff mbox series

Patch

diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig
index 04f20dd39007..f7119fd72df4 100644
--- a/drivers/net/can/m_can/Kconfig
+++ b/drivers/net/can/m_can/Kconfig
@@ -1,5 +1,14 @@ 
 config CAN_M_CAN
+	tristate "Bosch M_CAN support"
+	---help---
+	  Say Y here if you want support for Bosch M_CAN controller framework.
+	  This is common support for devices that embed the Bosch M_CAN IP.
+
+config CAN_M_CAN_PLATFORM
+	tristate "Bosch M_CAN support for io-mapped devices"
 	depends on HAS_IOMEM
-	tristate "Bosch M_CAN devices"
+	depends on CAN_M_CAN
 	---help---
-	  Say Y here if you want to support for Bosch M_CAN controller.
+	  Say Y here if you want support for IO Mapped Bosch M_CAN controller.
+	  This support is for devices that have the Bosch M_CAN controller
+	  IP embedded into the device and the IP is IO Mapped to the processor.
diff --git a/drivers/net/can/m_can/Makefile b/drivers/net/can/m_can/Makefile
index 8bbd7f24f5be..057bbcdb3c74 100644
--- a/drivers/net/can/m_can/Makefile
+++ b/drivers/net/can/m_can/Makefile
@@ -3,3 +3,4 @@ 
 #
 
 obj-$(CONFIG_CAN_M_CAN) += m_can.o
+obj-$(CONFIG_CAN_M_CAN_PLATFORM) += m_can_platform.o
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 9b449400376b..a60278d94126 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1,20 +1,14 @@ 
-/*
- * CAN bus driver for Bosch M_CAN controller
- *
- * Copyright (C) 2014 Freescale Semiconductor, Inc.
- *	Dong Aisheng <b29396@freescale.com>
- *
- * Bosch M_CAN user manual can be obtained from:
+// SPDX-License-Identifier: GPL-2.0
+// CAN bus driver for Bosch M_CAN controller
+// Copyright (C) 2014 Freescale Semiconductor, Inc.
+//      Dong Aisheng <b29396@freescale.com>
+// Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
+
+/* Bosch M_CAN user manual can be obtained from:
  * http://www.bosch-semiconductors.de/media/pdf_1/ipmodules_1/m_can/
  * mcan_users_manual_v302.pdf
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
  */
 
-#include <linux/clk.h>
-#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -28,11 +22,7 @@ 
 #include <linux/can/dev.h>
 #include <linux/pinctrl/consumer.h>
 
-/* napi related */
-#define M_CAN_NAPI_WEIGHT	64
-
-/* message ram configuration data length */
-#define MRAM_CFG_LEN	8
+#include "m_can.h"
 
 /* registers definition */
 enum m_can_reg {
@@ -86,28 +76,11 @@  enum m_can_reg {
 	M_CAN_TXEFA	= 0xf8,
 };
 
-/* m_can lec values */
-enum m_can_lec_type {
-	LEC_NO_ERROR = 0,
-	LEC_STUFF_ERROR,
-	LEC_FORM_ERROR,
-	LEC_ACK_ERROR,
-	LEC_BIT1_ERROR,
-	LEC_BIT0_ERROR,
-	LEC_CRC_ERROR,
-	LEC_UNUSED,
-};
+/* napi related */
+#define M_CAN_NAPI_WEIGHT	64
 
-enum m_can_mram_cfg {
-	MRAM_SIDF = 0,
-	MRAM_XIDF,
-	MRAM_RXF0,
-	MRAM_RXF1,
-	MRAM_RXB,
-	MRAM_TXE,
-	MRAM_TXB,
-	MRAM_CFG_NUM,
-};
+/* message ram configuration data length */
+#define MRAM_CFG_LEN	8
 
 /* Core Release Register (CREL) */
 #define CREL_REL_SHIFT		28
@@ -347,68 +320,72 @@  enum m_can_mram_cfg {
 #define TX_EVENT_MM_SHIFT	TX_BUF_MM_SHIFT
 #define TX_EVENT_MM_MASK	(0xff << TX_EVENT_MM_SHIFT)
 
-/* address offset and element number for each FIFO/Buffer in the Message RAM */
-struct mram_cfg {
-	u16 off;
-	u8  num;
-};
-
-/* m_can private data structure */
-struct m_can_priv {
-	struct can_priv can;	/* must be the first member */
-	struct napi_struct napi;
-	struct net_device *dev;
-	struct device *device;
-	struct clk *hclk;
-	struct clk *cclk;
-	void __iomem *base;
-	u32 irqstatus;
-	int version;
-
-	/* message ram configuration */
-	void __iomem *mram_base;
-	struct mram_cfg mcfg[MRAM_CFG_NUM];
-};
+static u32 m_can_read(struct m_can_priv *priv, enum m_can_reg reg)
+{
+	if (priv->ops->read_reg)
+		return priv->ops->read_reg(priv, reg);
+	else
+		return -EINVAL;
+}
 
-static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg)
+static int m_can_write(struct m_can_priv *priv, enum m_can_reg reg, u32 val)
 {
-	return readl(priv->base + reg);
+	if (priv->ops->write_reg)
+		return priv->ops->write_reg(priv, reg, val);
+	else
+		return -EINVAL;
 }
 
-static inline void m_can_write(const struct m_can_priv *priv,
-			       enum m_can_reg reg, u32 val)
+static u32 m_can_fifo_read(struct m_can_priv *priv,
+			   u32 fgi, unsigned int offset)
 {
-	writel(val, priv->base + reg);
+	u32 addr_offset = priv->mcfg[MRAM_RXF0].off + fgi * RXF0_ELEMENT_SIZE +
+			  offset;
+
+	if (priv->ops->read_fifo)
+		return priv->ops->read_fifo(priv, addr_offset);
+	else
+		return -EINVAL;
 }
 
-static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
-				  u32 fgi, unsigned int offset)
+static u32 m_can_fifo_write(struct m_can_priv *priv,
+			    u32 fpi, unsigned int offset, u32 val)
 {
-	return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
-		     fgi * RXF0_ELEMENT_SIZE + offset);
+	u32 addr_offset = priv->mcfg[MRAM_TXB].off + fpi * TXB_ELEMENT_SIZE +
+			  offset;
+
+	if (priv->ops->write_fifo)
+		return priv->ops->write_fifo(priv, addr_offset, val);
+	else
+		return -EINVAL;
 }
 
-static inline void m_can_fifo_write(const struct m_can_priv *priv,
-				    u32 fpi, unsigned int offset, u32 val)
+static u32 m_can_fifo_write_no_off(struct m_can_priv *priv,
+				   u32 fpi, u32 val)
 {
-	writel(val, priv->mram_base + priv->mcfg[MRAM_TXB].off +
-	       fpi * TXB_ELEMENT_SIZE + offset);
+	if (priv->ops->write_fifo)
+		return priv->ops->write_fifo(priv, fpi, val);
+	else
+		return 0;
 }
 
-static inline u32 m_can_txe_fifo_read(const struct m_can_priv *priv,
-				      u32 fgi,
-				      u32 offset) {
-	return readl(priv->mram_base + priv->mcfg[MRAM_TXE].off +
-			fgi * TXE_ELEMENT_SIZE + offset);
+static u32 m_can_txe_fifo_read(struct m_can_priv *priv, u32 fgi, u32 offset)
+{
+	u32 addr_offset = priv->mcfg[MRAM_TXE].off + fgi * TXE_ELEMENT_SIZE +
+			  offset;
+
+	if (priv->ops->read_fifo)
+		return priv->ops->read_fifo(priv, addr_offset);
+	else
+		return -EINVAL;
 }
 
-static inline bool m_can_tx_fifo_full(const struct m_can_priv *priv)
+static inline bool m_can_tx_fifo_full(struct m_can_priv *priv)
 {
 		return !!(m_can_read(priv, M_CAN_TXFQS) & TXFQS_TFQF);
 }
 
-static inline void m_can_config_endisable(const struct m_can_priv *priv,
-					  bool enable)
+void m_can_config_endisable(struct m_can_priv *priv, bool enable)
 {
 	u32 cccr = m_can_read(priv, M_CAN_CCCR);
 	u32 timeout = 10;
@@ -430,7 +407,7 @@  static inline void m_can_config_endisable(const struct m_can_priv *priv,
 
 	while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE)) != val) {
 		if (timeout == 0) {
-			netdev_warn(priv->dev, "Failed to init module\n");
+			netdev_warn(priv->net, "Failed to init module\n");
 			return;
 		}
 		timeout--;
@@ -438,13 +415,13 @@  static inline void m_can_config_endisable(const struct m_can_priv *priv,
 	}
 }
 
-static inline void m_can_enable_all_interrupts(const struct m_can_priv *priv)
+static inline void m_can_enable_all_interrupts(struct m_can_priv *priv)
 {
 	/* Only interrupt line 0 is used in this driver */
 	m_can_write(priv, M_CAN_ILE, ILE_EINT0);
 }
 
-static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
+static inline void m_can_disable_all_interrupts(struct m_can_priv *priv)
 {
 	m_can_write(priv, M_CAN_ILE, 0x0);
 }
@@ -633,9 +610,12 @@  static int m_can_clk_start(struct m_can_priv *priv)
 {
 	int err;
 
-	err = pm_runtime_get_sync(priv->device);
+	if (priv->pm_clock_support == 0)
+		return 0;
+
+	err = pm_runtime_get_sync(priv->dev);
 	if (err < 0) {
-		pm_runtime_put_noidle(priv->device);
+		pm_runtime_put_noidle(priv->dev);
 		return err;
 	}
 
@@ -644,7 +624,8 @@  static int m_can_clk_start(struct m_can_priv *priv)
 
 static void m_can_clk_stop(struct m_can_priv *priv)
 {
-	pm_runtime_put_sync(priv->device);
+	if (priv->pm_clock_support)
+		pm_runtime_put_sync(priv->dev);
 }
 
 static int m_can_get_berr_counter(const struct net_device *dev,
@@ -811,9 +792,8 @@  static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
 	return work_done;
 }
 
-static int m_can_poll(struct napi_struct *napi, int quota)
+static int m_can_rx_handler(struct net_device *dev, int quota)
 {
-	struct net_device *dev = napi->dev;
 	struct m_can_priv *priv = netdev_priv(dev);
 	int work_done = 0;
 	u32 irqstatus, psr;
@@ -831,13 +811,33 @@  static int m_can_poll(struct napi_struct *napi, int quota)
 
 	if (irqstatus & IR_RF0N)
 		work_done += m_can_do_rx_poll(dev, (quota - work_done));
+end:
+	return work_done;
+}
+
+static int m_can_rx_peripherial(struct net_device *dev)
+{
+	struct m_can_priv *priv = netdev_priv(dev);
+
+	m_can_rx_handler(dev, 1);
+
+	m_can_enable_all_interrupts(priv);
+
+	return 0;
+}
 
+static int m_can_poll(struct napi_struct *napi, int quota)
+{
+	struct net_device *dev = napi->dev;
+	struct m_can_priv *priv = netdev_priv(dev);
+	int work_done;
+
+	work_done = m_can_rx_handler(dev, quota);
 	if (work_done < quota) {
 		napi_complete_done(napi, work_done);
 		m_can_enable_all_interrupts(priv);
 	}
 
-end:
 	return work_done;
 }
 
@@ -894,6 +894,9 @@  static irqreturn_t m_can_isr(int irq, void *dev_id)
 	if (ir & IR_ALL_INT)
 		m_can_write(priv, M_CAN_IR, ir);
 
+	if (priv->ops->clear_interrupts)
+		priv->ops->clear_interrupts(priv);
+
 	/* schedule NAPI in case of
 	 * - rx IRQ
 	 * - state change IRQ
@@ -902,7 +905,10 @@  static irqreturn_t m_can_isr(int irq, void *dev_id)
 	if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X)) {
 		priv->irqstatus = ir;
 		m_can_disable_all_interrupts(priv);
-		napi_schedule(&priv->napi);
+		if (!priv->is_peripherial)
+			napi_schedule(&priv->napi);
+		else
+			m_can_rx_peripherial(dev);
 	}
 
 	if (priv->version == 30) {
@@ -1155,6 +1161,9 @@  static void m_can_chip_config(struct net_device *dev)
 	m_can_set_bittiming(dev);
 
 	m_can_config_endisable(priv, false);
+
+	if (priv->ops->init)
+		priv->ops->init(priv);
 }
 
 static void m_can_start(struct net_device *dev)
@@ -1188,20 +1197,17 @@  static int m_can_set_mode(struct net_device *dev, enum can_mode mode)
  * else it returns the release and step coded as:
  * return value = 10 * <release> + 1 * <step>
  */
-static int m_can_check_core_release(void __iomem *m_can_base)
+static int m_can_check_core_release(struct m_can_priv *priv)
 {
 	u32 crel_reg;
 	u8 rel;
 	u8 step;
 	int res;
-	struct m_can_priv temp_priv = {
-		.base = m_can_base
-	};
 
 	/* Read Core Release Version and split into version number
 	 * Example: Version 3.2.1 => rel = 3; step = 2; substep = 1;
 	 */
-	crel_reg = m_can_read(&temp_priv, M_CAN_CREL);
+	crel_reg = m_can_read(priv, M_CAN_CREL);
 	rel = (u8)((crel_reg & CREL_REL_MASK) >> CREL_REL_SHIFT);
 	step = (u8)((crel_reg & CREL_STEP_MASK) >> CREL_STEP_SHIFT);
 
@@ -1219,18 +1225,26 @@  static int m_can_check_core_release(void __iomem *m_can_base)
 /* Selectable Non ISO support only in version 3.2.x
  * This function checks if the bit is writable.
  */
-static bool m_can_niso_supported(const struct m_can_priv *priv)
+static bool m_can_niso_supported(struct m_can_priv *priv)
 {
-	u32 cccr_reg, cccr_poll;
-	int niso_timeout;
+	u32 cccr_reg, cccr_poll = 0;
+	int niso_timeout = -ETIMEDOUT;
+	int i;
 
 	m_can_config_endisable(priv, true);
 	cccr_reg = m_can_read(priv, M_CAN_CCCR);
 	cccr_reg |= CCCR_NISO;
 	m_can_write(priv, M_CAN_CCCR, cccr_reg);
 
-	niso_timeout = readl_poll_timeout((priv->base + M_CAN_CCCR), cccr_poll,
-					  (cccr_poll == cccr_reg), 0, 10);
+	for (i = 0; i <= 10; i++) {
+		cccr_poll = m_can_read(priv, M_CAN_CCCR);
+		if (cccr_poll == cccr_reg) {
+			niso_timeout = 0;
+			break;
+		}
+
+		usleep_range(1, 5);
+	}
 
 	/* Clear NISO */
 	cccr_reg &= ~(CCCR_NISO);
@@ -1242,107 +1256,79 @@  static bool m_can_niso_supported(const struct m_can_priv *priv)
 	return !niso_timeout;
 }
 
-static int m_can_dev_setup(struct platform_device *pdev, struct net_device *dev,
-			   void __iomem *addr)
+static int m_can_dev_setup(struct m_can_priv *m_can_dev)
 {
-	struct m_can_priv *priv;
+	struct net_device *dev = m_can_dev->net;
 	int m_can_version;
 
-	m_can_version = m_can_check_core_release(addr);
+	m_can_version = m_can_check_core_release(m_can_dev);
 	/* return if unsupported version */
 	if (!m_can_version) {
-		dev_err(&pdev->dev, "Unsupported version number: %2d",
+		dev_err(m_can_dev->dev, "Unsupported version number: %2d",
 			m_can_version);
 		return -EINVAL;
 	}
 
-	priv = netdev_priv(dev);
-	netif_napi_add(dev, &priv->napi, m_can_poll, M_CAN_NAPI_WEIGHT);
+	if (!m_can_dev->is_peripherial)
+		netif_napi_add(dev, &m_can_dev->napi,
+			       m_can_poll, M_CAN_NAPI_WEIGHT);
 
 	/* Shared properties of all M_CAN versions */
-	priv->version = m_can_version;
-	priv->dev = dev;
-	priv->base = addr;
-	priv->can.do_set_mode = m_can_set_mode;
-	priv->can.do_get_berr_counter = m_can_get_berr_counter;
+	m_can_dev->version = m_can_version;
+	m_can_dev->can.do_set_mode = m_can_set_mode;
+	m_can_dev->can.do_get_berr_counter = m_can_get_berr_counter;
 
 	/* Set M_CAN supported operations */
-	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+	m_can_dev->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
 					CAN_CTRLMODE_LISTENONLY |
 					CAN_CTRLMODE_BERR_REPORTING |
 					CAN_CTRLMODE_FD;
 
 	/* Set properties depending on M_CAN version */
-	switch (priv->version) {
+	switch (m_can_dev->version) {
 	case 30:
 		/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.0.x */
 		can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
-		priv->can.bittiming_const = &m_can_bittiming_const_30X;
-		priv->can.data_bittiming_const =
-				&m_can_data_bittiming_const_30X;
+		m_can_dev->can.bittiming_const = m_can_dev->bit_timing ?
+			m_can_dev->bit_timing : &m_can_bittiming_const_30X;
+
+		m_can_dev->can.data_bittiming_const = m_can_dev->data_timing ?
+						m_can_dev->data_timing :
+						&m_can_data_bittiming_const_30X;
 		break;
 	case 31:
 		/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.1.x */
 		can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
-		priv->can.bittiming_const = &m_can_bittiming_const_31X;
-		priv->can.data_bittiming_const =
-				&m_can_data_bittiming_const_31X;
+		m_can_dev->can.bittiming_const = m_can_dev->bit_timing ?
+			m_can_dev->bit_timing : &m_can_bittiming_const_31X;
+
+		m_can_dev->can.data_bittiming_const = m_can_dev->data_timing ?
+						m_can_dev->data_timing :
+						&m_can_data_bittiming_const_31X;
 		break;
 	case 32:
-		priv->can.bittiming_const = &m_can_bittiming_const_31X;
-		priv->can.data_bittiming_const =
-				&m_can_data_bittiming_const_31X;
-		priv->can.ctrlmode_supported |= (m_can_niso_supported(priv)
+		m_can_dev->can.bittiming_const = m_can_dev->bit_timing ?
+			m_can_dev->bit_timing : &m_can_bittiming_const_31X;
+
+		m_can_dev->can.data_bittiming_const = m_can_dev->data_timing ?
+						m_can_dev->data_timing :
+						&m_can_data_bittiming_const_31X;
+
+		m_can_dev->can.ctrlmode_supported |=
+						(m_can_niso_supported(m_can_dev)
 						? CAN_CTRLMODE_FD_NON_ISO
 						: 0);
 		break;
 	default:
-		dev_err(&pdev->dev, "Unsupported version number: %2d",
-			priv->version);
+		dev_err(m_can_dev->dev, "Unsupported version number: %2d",
+			m_can_dev->version);
 		return -EINVAL;
 	}
 
-	return 0;
-}
-
-static int m_can_open(struct net_device *dev)
-{
-	struct m_can_priv *priv = netdev_priv(dev);
-	int err;
-
-	err = m_can_clk_start(priv);
-	if (err)
-		return err;
-
-	/* open the can device */
-	err = open_candev(dev);
-	if (err) {
-		netdev_err(dev, "failed to open can device\n");
-		goto exit_disable_clks;
-	}
-
-	/* register interrupt handler */
-	err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
-			  dev);
-	if (err < 0) {
-		netdev_err(dev, "failed to request interrupt\n");
-		goto exit_irq_fail;
-	}
-
-	/* start the m_can controller */
-	m_can_start(dev);
-
-	can_led_event(dev, CAN_LED_EVENT_OPEN);
-	napi_enable(&priv->napi);
-	netif_start_queue(dev);
+	if (m_can_dev->ops->init)
+		m_can_dev->ops->init(m_can_dev);
 
 	return 0;
-
-exit_irq_fail:
-	close_candev(dev);
-exit_disable_clks:
-	m_can_clk_stop(priv);
-	return err;
 }
 
 static void m_can_stop(struct net_device *dev)
@@ -1361,10 +1347,18 @@  static int m_can_close(struct net_device *dev)
 	struct m_can_priv *priv = netdev_priv(dev);
 
 	netif_stop_queue(dev);
-	napi_disable(&priv->napi);
+	if (!priv->is_peripherial)
+		napi_disable(&priv->napi);
 	m_can_stop(dev);
 	m_can_clk_stop(priv);
 	free_irq(dev->irq, dev);
+
+	if (priv->is_peripherial) {
+		priv->tx_skb = NULL;
+		destroy_workqueue(priv->tx_wq);
+		priv->tx_wq = NULL;
+	}
+
 	close_candev(dev);
 	can_led_event(dev, CAN_LED_EVENT_STOP);
 
@@ -1385,18 +1379,15 @@  static int m_can_next_echo_skb_occupied(struct net_device *dev, int putidx)
 	return !!priv->can.echo_skb[next_idx];
 }
 
-static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
-				    struct net_device *dev)
+static netdev_tx_t m_can_tx_handler(struct m_can_priv *priv)
 {
-	struct m_can_priv *priv = netdev_priv(dev);
-	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	struct canfd_frame *cf = (struct canfd_frame *)priv->tx_skb->data;
+	struct net_device *dev = priv->net;
+	struct sk_buff *skb = priv->tx_skb;
 	u32 id, cccr, fdflags;
 	int i;
 	int putidx;
 
-	if (can_dropped_invalid_skb(dev, skb))
-		return NETDEV_TX_OK;
-
 	/* Generate ID field for TX buffer Element */
 	/* Common to all supported M_CAN versions */
 	if (cf->can_id & CAN_EFF_FLAG) {
@@ -1492,14 +1483,113 @@  static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
 		m_can_write(priv, M_CAN_TXBAR, (1 << putidx));
 
 		/* stop network queue if fifo full */
-			if (m_can_tx_fifo_full(priv) ||
-			    m_can_next_echo_skb_occupied(dev, putidx))
-				netif_stop_queue(dev);
+		if (m_can_tx_fifo_full(priv) ||
+		    m_can_next_echo_skb_occupied(dev, putidx))
+			netif_stop_queue(dev);
+	}
+
+	return NETDEV_TX_OK;
+}
+
+static void m_can_tx_work_queue(struct work_struct *ws)
+{
+	struct m_can_priv *priv = container_of(ws, struct m_can_priv,
+						tx_work);
+	netdev_tx_t ret;
+
+	ret = m_can_tx_handler(priv);
+	if (ret == NETDEV_TX_OK)
+		priv->tx_skb = NULL;
+}
+
+static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	struct m_can_priv *priv = netdev_priv(dev);
+
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
+	if (priv->is_peripherial) {
+		if (priv->tx_skb) {
+			netdev_err(dev, "hard_xmit called while tx busy\n");
+			return NETDEV_TX_BUSY;
+		}
+
+		priv->tx_skb = skb;
+		netif_stop_queue(priv->net);
+		queue_work(priv->tx_wq, &priv->tx_work);
+	} else {
+		priv->tx_skb = skb;
+		return m_can_tx_handler(priv);
 	}
 
 	return NETDEV_TX_OK;
 }
 
+static int m_can_open(struct net_device *dev)
+{
+	struct m_can_priv *priv = netdev_priv(dev);
+	int err;
+
+	err = m_can_clk_start(priv);
+	if (err)
+		return err;
+
+	/* open the can device */
+	err = open_candev(dev);
+	if (err) {
+		netdev_err(dev, "failed to open can device\n");
+		goto exit_disable_clks;
+	}
+
+	/* register interrupt handler */
+	if (priv->is_peripherial) {
+		priv->tx_skb = NULL;
+		priv->tx_wq = alloc_workqueue("mcan_wq",
+					      WQ_FREEZABLE | WQ_MEM_RECLAIM, 0);
+		if (!priv->tx_wq) {
+			err = -ENOMEM;
+			goto out_wq_fail;
+		}
+
+		INIT_WORK(&priv->tx_work, m_can_tx_work_queue);
+
+		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
+					   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+					   dev->name, dev);
+	} else {
+		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
+				  dev);
+	}
+
+	if (err < 0) {
+		netdev_err(dev, "failed to request interrupt\n");
+		goto exit_irq_fail;
+	}
+
+	/* start the m_can controller */
+	m_can_start(dev);
+
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
+	if (!priv->is_peripherial)
+		napi_enable(&priv->napi);
+
+	netif_start_queue(dev);
+
+	return 0;
+
+exit_irq_fail:
+	if (priv->is_peripherial)
+		destroy_workqueue(priv->tx_wq);
+out_wq_fail:
+	close_candev(dev);
+exit_disable_clks:
+	m_can_clk_stop(priv);
+	return err;
+}
+
 static const struct net_device_ops m_can_netdev_ops = {
 	.ndo_open = m_can_open,
 	.ndo_stop = m_can_close,
@@ -1515,20 +1605,6 @@  static int register_m_can_dev(struct net_device *dev)
 	return register_candev(dev);
 }
 
-static void m_can_init_ram(struct m_can_priv *priv)
-{
-	int end, i, start;
-
-	/* initialize the entire Message RAM in use to avoid possible
-	 * ECC/parity checksum errors when reading an uninitialized buffer
-	 */
-	start = priv->mcfg[MRAM_SIDF].off;
-	end = priv->mcfg[MRAM_TXB].off +
-		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
-	for (i = start; i < end; i += 4)
-		writel(0x0, priv->mram_base + i);
-}
-
 static void m_can_of_parse_mram(struct m_can_priv *priv,
 				const u32 *mram_config_vals)
 {
@@ -1556,9 +1632,8 @@  static void m_can_of_parse_mram(struct m_can_priv *priv,
 	priv->mcfg[MRAM_TXB].num = mram_config_vals[7] &
 			(TXBC_NDTB_MASK >> TXBC_NDTB_SHIFT);
 
-	dev_dbg(priv->device,
-		"mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
-		priv->mram_base,
+	dev_dbg(priv->dev,
+		"sidf 0x%x %d xidf 0x%x %d rxf0 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
 		priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num,
 		priv->mcfg[MRAM_XIDF].off, priv->mcfg[MRAM_XIDF].num,
 		priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num,
@@ -1566,63 +1641,55 @@  static void m_can_of_parse_mram(struct m_can_priv *priv,
 		priv->mcfg[MRAM_RXB].off, priv->mcfg[MRAM_RXB].num,
 		priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
 		priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
-
-	m_can_init_ram(priv);
 }
 
-static int m_can_plat_probe(struct platform_device *pdev)
+void m_can_init_ram(struct m_can_priv *priv)
 {
-	struct net_device *dev;
-	struct m_can_priv *priv;
-	struct resource *res;
-	void __iomem *addr;
-	void __iomem *mram_addr;
-	struct clk *hclk, *cclk;
-	int irq, ret;
-	struct device_node *np;
-	u32 mram_config_vals[MRAM_CFG_LEN];
-	u32 tx_fifo_size;
-
-	np = pdev->dev.of_node;
+	int end, i, start;
 
-	hclk = devm_clk_get(&pdev->dev, "hclk");
-	cclk = devm_clk_get(&pdev->dev, "cclk");
+	/* initialize the entire Message RAM in use to avoid possible
+	 * ECC/parity checksum errors when reading an uninitialized buffer
+	 */
+	start = priv->mcfg[MRAM_SIDF].off;
+	end = priv->mcfg[MRAM_TXB].off +
+		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
 
-	if (IS_ERR(hclk) || IS_ERR(cclk)) {
-		dev_err(&pdev->dev, "no clock found\n");
-		ret = -ENODEV;
-		goto failed_ret;
-	}
+	for (i = start; i < end; i += 4)
+		m_can_fifo_write_no_off(priv, i, 0x0);
+}
+EXPORT_SYMBOL_GPL(m_can_init_ram);
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
-	addr = devm_ioremap_resource(&pdev->dev, res);
-	irq = platform_get_irq_byname(pdev, "int0");
+int m_can_class_get_clocks(struct m_can_priv *m_can_dev)
+{
+	int ret = 0;
 
-	if (IS_ERR(addr) || irq < 0) {
-		ret = -EINVAL;
-		goto failed_ret;
-	}
+	m_can_dev->hclk = devm_clk_get(m_can_dev->dev, "hclk");
+	m_can_dev->cclk = devm_clk_get(m_can_dev->dev, "cclk");
 
-	/* message ram could be shared */
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
-	if (!res) {
+	if (IS_ERR(m_can_dev->cclk)) {
+		dev_err(m_can_dev->dev, "no clock found\n");
 		ret = -ENODEV;
-		goto failed_ret;
 	}
 
-	mram_addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!mram_addr) {
-		ret = -ENOMEM;
-		goto failed_ret;
-	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(m_can_class_get_clocks);
 
-	/* get message ram configuration */
-	ret = of_property_read_u32_array(np, "bosch,mram-cfg",
-					 mram_config_vals,
-					 sizeof(mram_config_vals) / 4);
+struct m_can_priv *m_can_class_allocate_dev(struct device *dev)
+{
+	struct m_can_priv *class_dev = NULL;
+	u32 mram_config_vals[MRAM_CFG_LEN];
+	struct net_device *net_dev;
+	u32 tx_fifo_size;
+	int ret;
+
+	ret = fwnode_property_read_u32_array(dev_fwnode(dev),
+					     "bosch,mram-cfg",
+					     mram_config_vals,
+					     sizeof(mram_config_vals) / 4);
 	if (ret) {
-		dev_err(&pdev->dev, "Could not get Message RAM configuration.");
-		goto failed_ret;
+		dev_err(dev, "Could not get Message RAM configuration.");
+		goto out;
 	}
 
 	/* Get TX FIFO size
@@ -1631,66 +1698,74 @@  static int m_can_plat_probe(struct platform_device *pdev)
 	tx_fifo_size = mram_config_vals[7];
 
 	/* allocate the m_can device */
-	dev = alloc_candev(sizeof(*priv), tx_fifo_size);
-	if (!dev) {
-		ret = -ENOMEM;
-		goto failed_ret;
+	net_dev = alloc_candev(sizeof(*class_dev), tx_fifo_size);
+	if (!net_dev) {
+		dev_err(dev, "Failed to allocate CAN device");
+		goto out;
 	}
 
-	priv = netdev_priv(dev);
-	dev->irq = irq;
-	priv->device = &pdev->dev;
-	priv->hclk = hclk;
-	priv->cclk = cclk;
-	priv->can.clock.freq = clk_get_rate(cclk);
-	priv->mram_base = mram_addr;
+	class_dev = netdev_priv(net_dev);
+	if (!class_dev) {
+		dev_err(dev, "Failed to init netdev private");
+		goto out;
+	}
 
-	platform_set_drvdata(pdev, dev);
-	SET_NETDEV_DEV(dev, &pdev->dev);
+	class_dev->net = net_dev;
+	class_dev->dev = dev;
+	SET_NETDEV_DEV(net_dev, dev);
 
-	/* Enable clocks. Necessary to read Core Release in order to determine
-	 * M_CAN version
-	 */
-	pm_runtime_enable(&pdev->dev);
-	ret = m_can_clk_start(priv);
-	if (ret)
-		goto pm_runtime_fail;
+	m_can_of_parse_mram(class_dev, mram_config_vals);
+out:
+	return class_dev;
+}
+EXPORT_SYMBOL_GPL(m_can_class_allocate_dev);
+
+int m_can_class_register(struct m_can_priv *m_can_dev)
+{
+	int ret;
 
-	ret = m_can_dev_setup(pdev, dev, addr);
+	if (m_can_dev->pm_clock_support) {
+		pm_runtime_enable(m_can_dev->dev);
+		ret = m_can_clk_start(m_can_dev);
+		if (ret)
+			goto pm_runtime_fail;
+	}
+
+	ret = m_can_dev_setup(m_can_dev);
 	if (ret)
 		goto clk_disable;
 
-	ret = register_m_can_dev(dev);
+	ret = register_m_can_dev(m_can_dev->net);
 	if (ret) {
-		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
-			KBUILD_MODNAME, ret);
+		dev_err(m_can_dev->dev, "registering %s failed (err=%d)\n",
+			m_can_dev->net->name, ret);
 		goto clk_disable;
 	}
 
-	m_can_of_parse_mram(priv, mram_config_vals);
-
-	devm_can_led_init(dev);
+	devm_can_led_init(m_can_dev->net);
 
-	of_can_transceiver(dev);
+	of_can_transceiver(m_can_dev->net);
 
-	dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
-		 KBUILD_MODNAME, dev->irq, priv->version);
+	dev_info(m_can_dev->dev, "%s device registered (irq=%d, version=%d)\n",
+		 KBUILD_MODNAME, m_can_dev->net->irq, m_can_dev->version);
 
 	/* Probe finished
 	 * Stop clocks. They will be reactivated once the M_CAN device is opened
 	 */
 clk_disable:
-	m_can_clk_stop(priv);
+	m_can_clk_stop(m_can_dev);
 pm_runtime_fail:
 	if (ret) {
-		pm_runtime_disable(&pdev->dev);
-		free_candev(dev);
+		if (m_can_dev->pm_clock_support)
+			pm_runtime_disable(m_can_dev->dev);
+		free_candev(m_can_dev->net);
 	}
-failed_ret:
+
 	return ret;
 }
+EXPORT_SYMBOL_GPL(m_can_class_register);
 
-static __maybe_unused int m_can_suspend(struct device *dev)
+int m_can_class_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct m_can_priv *priv = netdev_priv(ndev);
@@ -1708,8 +1783,9 @@  static __maybe_unused int m_can_suspend(struct device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(m_can_class_suspend);
 
-static __maybe_unused int m_can_resume(struct device *dev)
+int m_can_class_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct m_can_priv *priv = netdev_priv(ndev);
@@ -1733,79 +1809,19 @@  static __maybe_unused int m_can_resume(struct device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(m_can_class_resume);
 
-static void unregister_m_can_dev(struct net_device *dev)
+void m_can_class_unregister(struct m_can_priv *m_can_dev)
 {
-	unregister_candev(dev);
-}
+	unregister_candev(m_can_dev->net);
 
-static int m_can_plat_remove(struct platform_device *pdev)
-{
-	struct net_device *dev = platform_get_drvdata(pdev);
+	m_can_clk_stop(m_can_dev);
 
-	unregister_m_can_dev(dev);
-
-	pm_runtime_disable(&pdev->dev);
-
-	platform_set_drvdata(pdev, NULL);
-
-	free_candev(dev);
-
-	return 0;
-}
-
-static int __maybe_unused m_can_runtime_suspend(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-	struct m_can_priv *priv = netdev_priv(ndev);
-
-	clk_disable_unprepare(priv->cclk);
-	clk_disable_unprepare(priv->hclk);
-
-	return 0;
-}
-
-static int __maybe_unused m_can_runtime_resume(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-	struct m_can_priv *priv = netdev_priv(ndev);
-	int err;
-
-	err = clk_prepare_enable(priv->hclk);
-	if (err)
-		return err;
-
-	err = clk_prepare_enable(priv->cclk);
-	if (err)
-		clk_disable_unprepare(priv->hclk);
-
-	return err;
+	free_candev(m_can_dev->net);
 }
-
-static const struct dev_pm_ops m_can_pmops = {
-	SET_RUNTIME_PM_OPS(m_can_runtime_suspend,
-			   m_can_runtime_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
-};
-
-static const struct of_device_id m_can_of_table[] = {
-	{ .compatible = "bosch,m_can", .data = NULL },
-	{ /* sentinel */ },
-};
-MODULE_DEVICE_TABLE(of, m_can_of_table);
-
-static struct platform_driver m_can_plat_driver = {
-	.driver = {
-		.name = KBUILD_MODNAME,
-		.of_match_table = m_can_of_table,
-		.pm     = &m_can_pmops,
-	},
-	.probe = m_can_plat_probe,
-	.remove = m_can_plat_remove,
-};
-
-module_platform_driver(m_can_plat_driver);
+EXPORT_SYMBOL_GPL(m_can_class_unregister);
 
 MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
new file mode 100644
index 000000000000..b60f27dd9f33
--- /dev/null
+++ b/drivers/net/can/m_can/m_can.h
@@ -0,0 +1,110 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* CAN bus driver for Bosch M_CAN controller
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef _CAN_M_CAN_H_
+#define _CAN_M_CAN_H_
+
+#include <linux/can/core.h>
+#include <linux/can/led.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/freezer.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/iopoll.h>
+#include <linux/can/dev.h>
+#include <linux/pinctrl/consumer.h>
+
+/* m_can lec values */
+enum m_can_lec_type {
+	LEC_NO_ERROR = 0,
+	LEC_STUFF_ERROR,
+	LEC_FORM_ERROR,
+	LEC_ACK_ERROR,
+	LEC_BIT1_ERROR,
+	LEC_BIT0_ERROR,
+	LEC_CRC_ERROR,
+	LEC_UNUSED,
+};
+
+enum m_can_mram_cfg {
+	MRAM_SIDF = 0,
+	MRAM_XIDF,
+	MRAM_RXF0,
+	MRAM_RXF1,
+	MRAM_RXB,
+	MRAM_TXE,
+	MRAM_TXB,
+	MRAM_CFG_NUM,
+};
+
+/* address offset and element number for each FIFO/Buffer in the Message RAM */
+struct mram_cfg {
+	u16 off;
+	u8  num;
+};
+
+struct m_can_priv;
+struct m_can_ops {
+	/* Device specific call backs */
+	int (*clear_interrupts)(struct m_can_priv *m_can_class);
+	u32 (*read_reg)(struct m_can_priv *m_can_class, int reg);
+	int (*write_reg)(struct m_can_priv *m_can_class, int reg, int val);
+	u32 (*read_fifo)(struct m_can_priv *m_can_class, int addr_offset);
+	int (*write_fifo)(struct m_can_priv *m_can_class, int addr_offset,
+			  int val);
+	int (*init)(struct m_can_priv *m_can_class);
+};
+
+struct m_can_priv {
+	struct can_priv can;
+	struct napi_struct napi;
+	struct net_device *net;
+	struct device *dev;
+	struct clk *hclk;
+	struct clk *cclk;
+
+	struct workqueue_struct *tx_wq;
+	struct work_struct tx_work;
+	struct sk_buff *tx_skb;
+
+	struct can_bittiming_const *bit_timing;
+	struct can_bittiming_const *data_timing;
+
+	struct m_can_ops *ops;
+
+	void *device_data;
+
+	int version;
+	int freq;
+	u32 irqstatus;
+
+	int pm_clock_support;
+	int is_peripherial;
+
+	struct mram_cfg mcfg[MRAM_CFG_NUM];
+};
+
+struct m_can_priv *m_can_class_allocate_dev(struct device *dev);
+int m_can_class_register(struct m_can_priv *m_can_dev);
+void m_can_class_unregister(struct m_can_priv *m_can_dev);
+int m_can_class_get_clocks(struct m_can_priv *m_can_dev);
+void m_can_init_ram(struct m_can_priv *priv);
+void m_can_config_endisable(struct m_can_priv *priv, bool enable);
+
+int m_can_class_suspend(struct device *dev);
+int m_can_class_resume(struct device *dev);
+#endif	/* _CAN_M_H_ */
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
new file mode 100644
index 000000000000..b02d49f9ee4c
--- /dev/null
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -0,0 +1,202 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// IOMapped CAN bus driver for Bosch M_CAN controller
+// Copyright (C) 2014 Freescale Semiconductor, Inc.
+//	Dong Aisheng <b29396@freescale.com>
+//
+// Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/platform_device.h>
+
+#include "m_can.h"
+
+struct m_can_plat_priv {
+	void __iomem *base;
+	void __iomem *mram_base;
+};
+
+static u32 iomap_read_reg(struct m_can_priv *cdev, int reg)
+{
+	struct m_can_plat_priv *priv =
+			(struct m_can_plat_priv *)cdev->device_data;
+
+	return readl(priv->base + reg);
+}
+
+static u32 iomap_read_fifo(struct m_can_priv *cdev, int offset)
+{
+	struct m_can_plat_priv *priv =
+			(struct m_can_plat_priv *)cdev->device_data;
+
+	return readl(priv->mram_base + offset);
+}
+
+static int iomap_write_reg(struct m_can_priv *cdev, int reg, int val)
+{
+	struct m_can_plat_priv *priv =
+			(struct m_can_plat_priv *)cdev->device_data;
+
+	writel(val, priv->base + reg);
+
+	return 0;
+}
+
+static int iomap_write_fifo(struct m_can_priv *cdev, int offset, int val)
+{
+	struct m_can_plat_priv *priv =
+			(struct m_can_plat_priv *)cdev->device_data;
+
+	writel(val, priv->mram_base + offset);
+
+	return 0;
+}
+
+static struct m_can_ops m_can_plat_ops = {
+	.read_reg = iomap_read_reg,
+	.write_reg = iomap_write_reg,
+	.write_fifo = iomap_write_fifo,
+	.read_fifo = iomap_read_fifo,
+};
+
+static int m_can_plat_probe(struct platform_device *pdev)
+{
+	struct m_can_priv *mcan_class;
+	struct m_can_plat_priv *priv;
+	struct resource *res;
+	void __iomem *addr;
+	void __iomem *mram_addr;
+	int irq, ret = 0;
+
+	mcan_class = m_can_class_allocate_dev(&pdev->dev);
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mcan_class->device_data = priv;
+
+	m_can_class_get_clocks(mcan_class);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
+	addr = devm_ioremap_resource(&pdev->dev, res);
+	irq = platform_get_irq_byname(pdev, "int0");
+	if (IS_ERR(addr) || irq < 0) {
+		ret = -EINVAL;
+		goto failed_ret;
+	}
+
+	/* message ram could be shared */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
+	if (!res) {
+		ret = -ENODEV;
+		goto failed_ret;
+	}
+
+	mram_addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!mram_addr) {
+		ret = -ENOMEM;
+		goto failed_ret;
+	}
+
+	priv->base = addr;
+	priv->mram_base = mram_addr;
+
+	mcan_class->net->irq = irq;
+	mcan_class->pm_clock_support = 1;
+	mcan_class->can.clock.freq = clk_get_rate(mcan_class->cclk);
+	mcan_class->dev = &pdev->dev;
+
+	mcan_class->ops = &m_can_plat_ops;
+
+	mcan_class->is_peripherial = false;
+
+	platform_set_drvdata(pdev, mcan_class->dev);
+
+	m_can_init_ram(mcan_class);
+
+	ret = m_can_class_register(mcan_class);
+
+failed_ret:
+	return ret;
+}
+
+static __maybe_unused int m_can_suspend(struct device *dev)
+{
+	return m_can_class_suspend(dev);
+}
+
+static __maybe_unused int m_can_resume(struct device *dev)
+{
+	return m_can_class_resume(dev);
+}
+
+static int m_can_plat_remove(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct m_can_priv *mcan_class = netdev_priv(dev);
+
+	m_can_class_unregister(mcan_class);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static int __maybe_unused m_can_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct m_can_priv *mcan_class = netdev_priv(ndev);
+
+	m_can_class_suspend(dev);
+
+	clk_disable_unprepare(mcan_class->cclk);
+	clk_disable_unprepare(mcan_class->hclk);
+
+	return 0;
+}
+
+static int __maybe_unused m_can_runtime_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct m_can_priv *mcan_class = netdev_priv(ndev);
+	int err;
+
+	err = clk_prepare_enable(mcan_class->hclk);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(mcan_class->cclk);
+	if (err)
+		clk_disable_unprepare(mcan_class->hclk);
+
+	m_can_class_resume(dev);
+
+	return err;
+}
+
+static const struct dev_pm_ops m_can_pmops = {
+	SET_RUNTIME_PM_OPS(m_can_runtime_suspend,
+			   m_can_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
+};
+
+static const struct of_device_id m_can_of_table[] = {
+	{ .compatible = "bosch,m_can", .data = NULL },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, m_can_of_table);
+
+static struct platform_driver m_can_plat_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = m_can_of_table,
+		.pm     = &m_can_pmops,
+	},
+	.probe = m_can_plat_probe,
+	.remove = m_can_plat_remove,
+};
+
+module_platform_driver(m_can_plat_driver);
+
+MODULE_AUTHOR("Dong Aisheng <b29396@freescale.com>");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("M_CAN driver for IO Mapped Bosch controllers");