LinuxLists.cc - [PATCH v6 0/6] Add M_CAN Support for Dra76 platform

[permalink] [raw]

Subject: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate

From: Franklin S Cooper Jr <[email protected]>

Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a can-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
[[email protected]: fix build error with !CONFIG_OF]
Signed-off-by: Sekhar Nori <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
v6 changes:
fix build error with !CONFIG_OF

drivers/net/can/dev.c | 39 +++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 8 ++++++++
2 files changed, 47 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..007cfc0 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include <linux/can/skb.h>
#include <linux/can/netlink.h>
#include <linux/can/led.h>
+#include <linux/of.h>
#include <net/rtnetlink.h>

#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,30 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);

+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np = dev->dev.parent->of_node;
+ int ret;
+
+ dn = of_get_child_by_name(np, "can-transceiver");
+ if (!dn)
+ return;
+
+ ret = of_property_read_u32(dn, "max-bitrate", &priv->max_bitrate);
+ if ((ret && ret != -EINVAL) || (!ret && !priv->max_bitrate))
+ netdev_warn(dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit.\n");
+}
+EXPORT_SYMBOL_GPL(of_can_transceiver);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +938,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_bitrate && bt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));

if (priv->do_set_bittiming) {
@@ -997,6 +1029,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->max_bitrate && dbt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));

if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 61f1cf2..fbb7810 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -48,6 +48,8 @@ struct can_priv {
unsigned int data_bitrate_const_cnt;
struct can_clock clock;

+ unsigned int max_bitrate;
+
enum can_state state;

/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -166,6 +168,12 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);

+#ifdef CONFIG_OF
+void of_can_transceiver(struct net_device *dev);
+#else
+static inline void of_can_transceiver(struct net_device *dev) { }
+#endif
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.7.4

2017-12-22 13:31:22

[permalink] [raw]

Subject: [PATCH v6 3/6] can: m_can: Add PM Runtime

From: Franklin S Cooper Jr <[email protected]>

Add support for PM Runtime which is the new way to handle managing clocks.
However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
management approach in place.

PM_RUNTIME is required by OMAP based devices to handle clock management.
Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
to work with this driver.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
[[email protected]: handle pm_runtime_get_sync() failure, fix some bugs]
Signed-off-by: Sekhar Nori <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f72116e..53e764f 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -23,6 +23,7 @@
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/iopoll.h>
#include <linux/can/dev.h>

@@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
{
int err;

+ err = pm_runtime_get_sync(priv->device);
+ if (err) {
+ pm_runtime_put_noidle(priv->device);
+ return err;
+ }
+
err = clk_prepare_enable(priv->hclk);
if (err)
- return err;
+ goto pm_runtime_put;

err = clk_prepare_enable(priv->cclk);
if (err)
- clk_disable_unprepare(priv->hclk);
+ goto disable_hclk;

return err;
+
+disable_hclk:
+ clk_disable_unprepare(priv->hclk);
+pm_runtime_put:
+ pm_runtime_put_sync(priv->device);
+ return err;
}

static void m_can_clk_stop(struct m_can_priv *priv)
{
+ pm_runtime_put_sync(priv->device);
+
clk_disable_unprepare(priv->cclk);
clk_disable_unprepare(priv->hclk);
}
@@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev)
/* Enable clocks. Necessary to read Core Release in order to determine
* M_CAN version
*/
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret) {
+ pm_runtime_put_noidle(&pdev->dev);
+ goto pm_runtime_fail;
+ }
+
ret = clk_prepare_enable(hclk);
if (ret)
- goto disable_hclk_ret;
+ goto pm_runtime_put;

ret = clk_prepare_enable(cclk);
if (ret)
- goto disable_cclk_ret;
+ goto disable_hclk_ret;

res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
addr = devm_ioremap_resource(&pdev->dev, res);
@@ -1666,6 +1688,11 @@ static int m_can_plat_probe(struct platform_device *pdev)
clk_disable_unprepare(cclk);
disable_hclk_ret:
clk_disable_unprepare(hclk);
+pm_runtime_put:
+ pm_runtime_put_sync(&pdev->dev);
+pm_runtime_fail:
+ if (ret)
+ pm_runtime_disable(&pdev->dev);
failed_ret:
return ret;
}
@@ -1723,6 +1750,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
struct net_device *dev = platform_get_drvdata(pdev);

unregister_m_can_dev(dev);
+
+ pm_runtime_disable(&pdev->dev);
+
platform_set_drvdata(pdev, NULL);

free_m_can_dev(dev);
--
2.7.4

2017-12-22 13:31:43

[permalink] [raw]

Subject: [PATCH v6 5/6] dt-bindings: can: m_can: Document new can transceiver binding

From: Franklin S Cooper Jr <[email protected]>

Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index 63e9042..ed61438 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.

+Optional Subnode:
+- can-transceiver : Can-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+ Documentation/devicetree/bindings/net/can/can-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@20e8000 {
@@ -63,4 +68,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ can-transceiver {
+ max-bitrate = <5000000>;
+ };
};
--
2.7.4

2017-12-22 13:31:52

[permalink] [raw]

Subject: [PATCH v6 4/6] can: m_can: Support higher speed CAN-FD bitrates

From: Franklin S Cooper Jr <[email protected]>

During test transmitting using CAN-FD at high bitrates (> 2 Mbps)
would fail. Scoping the signals I noticed that only a single bit
was being transmitted and with a bit more investigation realized the actual
MCAN IP would go back to initialization mode automatically.

It appears this issue is due to the MCAN needing to use the Transmitter
Delay Compensation Mode with the correct value for the transmitter delay
compensation offset (tdco). What impacts the tdco value isn't 100% clear
but to calculate it you use an equation defined in the MCAN User's Guide.

The user guide mentions that this register needs to be set based on clock
values, secondary sample point and the data bitrate. One of the key
variables that can't automatically be determined is the secondary
sample point (ssp). This ssp is similar to the sp but is specific to this
transmitter delay compensation mode. The guidelines for configuring
ssp is rather vague but via some CAN test it appears for DRA76x that putting
the value same as data sampling point works.

The CAN-CIA's "Bit Time Requirements for CAN FD" paper presented at
the International CAN Conference 2013 indicates that this TDC mode is
only needed for data bit rates above 2.5 Mbps. Therefore, only enable
this mode when the data bit rate is above 2.5 Mbps.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/net/can/m_can/m_can.c | 41 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 53e764f..371ffc1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -127,6 +127,12 @@ enum m_can_mram_cfg {
#define DBTP_DSJW_SHIFT 0
#define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)

+/* Transmitter Delay Compensation Register (TDCR) */
+#define TDCR_TDCO_SHIFT 8
+#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
+#define TDCR_TDCF_SHIFT 0
+#define TDCR_TDCF_MASK (0x7F << TDCR_TDCF_SHIFT)
+
/* Test Register (TEST) */
#define TEST_LBCK BIT(4)

@@ -991,7 +997,8 @@ static int m_can_set_bittiming(struct net_device *dev)
const struct can_bittiming *bt = &priv->can.bittiming;
const struct can_bittiming *dbt = &priv->can.data_bittiming;
u16 brp, sjw, tseg1, tseg2;
- u32 reg_btp;
+ u32 reg_btp, tdco, ssp;
+ bool enable_tdc = false;

brp = bt->brp - 1;
sjw = bt->sjw - 1;
@@ -1006,9 +1013,41 @@ static int m_can_set_bittiming(struct net_device *dev)
sjw = dbt->sjw - 1;
tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
tseg2 = dbt->phase_seg2 - 1;
+
+ /* TDC is only needed for bitrates beyond 2.5 MBit/s.
+ * This is mentioned in the "Bit Time Requirements for CAN FD"
+ * paper presented at the International CAN Conference 2013
+ */
+ if (dbt->bitrate > 2500000) {
+ /* Use the same value of secondary sampling point
+ * as the data sampling point
+ */
+ ssp = dbt->sample_point;
+
+ /* Equation based on Bosch's M_CAN User Manual's
+ * Transmitter Delay Compensation Section
+ */
+ tdco = (priv->can.clock.freq / 1000) *
+ ssp / dbt->bitrate;
+
+ /* Max valid TDCO value is 127 */
+ if (tdco > 127) {
+ netdev_warn(dev, "TDCO value of %u is beyond maximum limit. Disabling Transmitter Delay Compensation mode\n",
+ tdco);
+ } else {
+ enable_tdc = true;
+ m_can_write(priv, M_CAN_TDCR,
+ tdco << TDCR_TDCO_SHIFT);
+ }
+ }
+
reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
(tseg1 << DBTP_DTSEG1_SHIFT) |
(tseg2 << DBTP_DTSEG2_SHIFT);
+
+ if (enable_tdc)
+ reg_btp |= DBTP_TDC;
+
m_can_write(priv, M_CAN_DBTP, reg_btp);
}

--
2.7.4

2017-12-22 13:31:19

[permalink] [raw]

Subject: [PATCH v6 2/6] can: m_can: Add call to of_can_transceiver

From: Franklin S Cooper Jr <[email protected]>

Add call to new generic functions that provides support via a binding
to limit the arbitration rate and/or data rate imposed by the physical
transceiver connected to the MCAN peripheral.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/net/can/m_can/m_can.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..f72116e 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev)

devm_can_led_init(dev);

+ of_can_transceiver(dev);
+
dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
KBUILD_MODNAME, dev->irq, priv->version);

--
2.7.4

2017-12-22 13:32:46

[permalink] [raw]

Subject: [PATCH v6 6/6] dt-bindings: can: can-transceiver: Document new binding

From: Franklin S Cooper Jr <[email protected]>

Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.

Signed-off-by: Franklin S Cooper Jr <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Faiz Abbas <[email protected]>
---
.../bindings/net/can/can-transceiver.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt

diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 0000000..0011f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+------------------------------
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+ ....
+ can-transceiver {
+ max-bitrate = <5000000>;
+ };
+ ...
+};
--
2.7.4

2017-12-29 03:38:19

by Wenyou Yang

[permalink] [raw]

Subject: Re: [PATCH v6 0/6] Add M_CAN Support for Dra76 platform

On 2017/12/22 21:31, Faiz Abbas wrote:
> This patch series adds support for M_CAN on the TI Dra76
> platform. Device tree patches will be sent separately.
> A bunch of patches were sent before by
> Franklin Cooper <[email protected]>. I have clubbed the
> series together and rebased to the latest kernel.
Tested this series on SAMA5D2 Xplained board.

Tested-by: Wenyou Yang <[email protected]>

>
> v6 changes:
> Dropped the patches to make hclk optional. Drivers
> which enable hclk as the interface clock using
> pm_runtime calls must still provide a hclk in the
> clocks property.
>
> Support higher speed CAN-FD bitrate:
> The community decided that data sampling point be used
> for the secondary sampling point here
> https://patchwork.kernel.org/patch/9909845/
>
> Franklin S Cooper Jr (6):
> can: dev: Add support for limiting configured bitrate
> can: m_can: Add call to of_can_transceiver
> can: m_can: Add PM Runtime
> can: m_can: Support higher speed CAN-FD bitrates
> dt-bindings: can: m_can: Document new can transceiver binding
> dt-bindings: can: can-transceiver: Document new binding
>
> .../bindings/net/can/can-transceiver.txt | 24 +++++++
> .../devicetree/bindings/net/can/m_can.txt | 9 +++
> drivers/net/can/dev.c | 39 +++++++++++
> drivers/net/can/m_can/m_can.c | 81 ++++++++++++++++++++--
> include/linux/can/dev.h | 8 +++
> 5 files changed, 156 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
>

Best Regards,
Wenyou Yang

2018-01-02 13:10:23

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate

On 12/22/2017 02:31 PM, Faiz Abbas wrote:
> From: Franklin S Cooper Jr <[email protected]>
>
> Various CAN or CAN-FD IP may be able to run at a faster rate than
> what the transceiver the CAN node is connected to. This can lead to
> unexpected errors. However, CAN transceivers typically have fixed
> limitations and provide no means to discover these limitations at
> runtime. Therefore, add support for a can-transceiver node that
> can be reused by other CAN peripheral drivers to determine for both
> CAN and CAN-FD what the max bitrate that can be used. If the user
> tries to configure CAN to pass these maximum bitrates it will throw
> an error.
>
> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> [[email protected]: fix build error with !CONFIG_OF]
> Signed-off-by: Sekhar Nori <[email protected]>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> v6 changes:
> fix build error with !CONFIG_OF
>
> drivers/net/can/dev.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/linux/can/dev.h | 8 ++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 365a8cc..007cfc0 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -27,6 +27,7 @@
> #include <linux/can/skb.h>
> #include <linux/can/netlink.h>
> #include <linux/can/led.h>
> +#include <linux/of.h>
> #include <net/rtnetlink.h>
>
> #define MOD_DESC "CAN device driver interface"
> @@ -814,6 +815,30 @@ int open_candev(struct net_device *dev)
> }
> EXPORT_SYMBOL_GPL(open_candev);
>
> +#ifdef CONFIG_OF
> +/*
> + * Common function that can be used to understand the limitation of
> + * a transceiver when it provides no means to determine these limitations
> + * at runtime.
> + */
> +void of_can_transceiver(struct net_device *dev)
> +{
> + struct device_node *dn;
> + struct can_priv *priv = netdev_priv(dev);
> + struct device_node *np = dev->dev.parent->of_node;
> + int ret;
> +
> + dn = of_get_child_by_name(np, "can-transceiver");
> + if (!dn)
> + return;
> +
> + ret = of_property_read_u32(dn, "max-bitrate", &priv->max_bitrate);
> + if ((ret && ret != -EINVAL) || (!ret && !priv->max_bitrate))
> + netdev_warn(dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit.\n");
> +}
> +EXPORT_SYMBOL_GPL(of_can_transceiver);
> +#endif
> +
> /*
> * Common close function for cleanup before the device gets closed.
> *
> @@ -913,6 +938,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> priv->bitrate_const_cnt);
> if (err)
> return err;
> +
> + if (priv->max_bitrate && bt.bitrate > priv->max_bitrate) {
> + netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
> + priv->max_bitrate);
> + return -EINVAL;
> + }

What about movong this check into can_get_bittiming()? Although we loose
the "arbitration" vs "canfd data".

> +
> memcpy(&priv->bittiming, &bt, sizeof(bt));
>
> if (priv->do_set_bittiming) {
> @@ -997,6 +1029,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> priv->data_bitrate_const_cnt);
> if (err)
> return err;
> +
> + if (priv->max_bitrate && dbt.bitrate > priv->max_bitrate) {
> + netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
> + priv->max_bitrate);
> + return -EINVAL;
> + }
> +
> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>
> if (priv->do_set_data_bittiming) {
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 61f1cf2..fbb7810 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -48,6 +48,8 @@ struct can_priv {
> unsigned int data_bitrate_const_cnt;
> struct can_clock clock;
>
> + unsigned int max_bitrate;

Please make it an u32, name it "bitrate_max" and ...

>> struct can_priv {
>> struct net_device *dev;
>> struct can_device_stats can_stats;
>>
>> struct can_bittiming bittiming, data_bittiming;
>> const struct can_bittiming_const *bittiming_const,
>> *data_bittiming_const;
>> const u16 *termination_const;
>> unsigned int termination_const_cnt;
>> u16 termination;
>> const u32 *bitrate_const;
>> unsigned int bitrate_const_cnt;
>> const u32 *data_bitrate_const;
>> unsigned int data_bitrate_const_cnt;

...move it here.

>> struct can_clock clock;
>

> +
> enum can_state state;
>
> /* CAN controller features - see include/uapi/linux/can/netlink.h */
> @@ -166,6 +168,12 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
> void can_free_echo_skb(struct net_device *dev, unsigned int idx);
>
> +#ifdef CONFIG_OF
> +void of_can_transceiver(struct net_device *dev);
> +#else
> +static inline void of_can_transceiver(struct net_device *dev) { }
> +#endif
> +
> struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
> struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> struct canfd_frame **cfd);
>

Marc

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

Attachments:

2018-01-02 13:35:35

[permalink] [raw]

Subject: Re: [PATCH v6 4/6] can: m_can: Support higher speed CAN-FD bitrates

On 12/22/2017 02:31 PM, Faiz Abbas wrote:
> From: Franklin S Cooper Jr <[email protected]>
>
> During test transmitting using CAN-FD at high bitrates (> 2 Mbps)
> would fail. Scoping the signals I noticed that only a single bit
> was being transmitted and with a bit more investigation realized the actual
> MCAN IP would go back to initialization mode automatically.
>
> It appears this issue is due to the MCAN needing to use the Transmitter
> Delay Compensation Mode with the correct value for the transmitter delay
> compensation offset (tdco). What impacts the tdco value isn't 100% clear
> but to calculate it you use an equation defined in the MCAN User's Guide.
>
> The user guide mentions that this register needs to be set based on clock
> values, secondary sample point and the data bitrate. One of the key
> variables that can't automatically be determined is the secondary
> sample point (ssp). This ssp is similar to the sp but is specific to this
> transmitter delay compensation mode. The guidelines for configuring
> ssp is rather vague but via some CAN test it appears for DRA76x that putting
> the value same as data sampling point works.
>
> The CAN-CIA's "Bit Time Requirements for CAN FD" paper presented at
> the International CAN Conference 2013 indicates that this TDC mode is
> only needed for data bit rates above 2.5 Mbps. Therefore, only enable
> this mode when the data bit rate is above 2.5 Mbps.
>
> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> Signed-off-by: Sekhar Nori <[email protected]>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> drivers/net/can/m_can/m_can.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 53e764f..371ffc1 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -127,6 +127,12 @@ enum m_can_mram_cfg {
> #define DBTP_DSJW_SHIFT 0
> #define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
>
> +/* Transmitter Delay Compensation Register (TDCR) */
> +#define TDCR_TDCO_SHIFT 8
> +#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
> +#define TDCR_TDCF_SHIFT 0
> +#define TDCR_TDCF_MASK (0x7F << TDCR_TDCF_SHIFT)
> +
> /* Test Register (TEST) */
> #define TEST_LBCK BIT(4)
>
> @@ -991,7 +997,8 @@ static int m_can_set_bittiming(struct net_device *dev)
> const struct can_bittiming *bt = &priv->can.bittiming;
> const struct can_bittiming *dbt = &priv->can.data_bittiming;
> u16 brp, sjw, tseg1, tseg2;
> - u32 reg_btp;
> + u32 reg_btp, tdco, ssp;

Please move the tdco and the ssp into "if (dbt->bitrate > 2500000)" scope.

Initialize "reg_btp = 0", see below.

> + bool enable_tdc = false;

Please remove, see below.

>
> brp = bt->brp - 1;
> sjw = bt->sjw - 1;
> @@ -1006,9 +1013,41 @@ static int m_can_set_bittiming(struct net_device *dev)
> sjw = dbt->sjw - 1;
> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
> tseg2 = dbt->phase_seg2 - 1;
> +
> + /* TDC is only needed for bitrates beyond 2.5 MBit/s.
> + * This is mentioned in the "Bit Time Requirements for CAN FD"
> + * paper presented at the International CAN Conference 2013
> + */
> + if (dbt->bitrate > 2500000) {
> + /* Use the same value of secondary sampling point
> + * as the data sampling point
> + */
> + ssp = dbt->sample_point;
> +
> + /* Equation based on Bosch's M_CAN User Manual's
> + * Transmitter Delay Compensation Section
> + */
> + tdco = (priv->can.clock.freq / 1000) *
> + ssp / dbt->bitrate;
> +
> + /* Max valid TDCO value is 127 */
> + if (tdco > 127) {
> + netdev_warn(dev, "TDCO value of %u is beyond maximum limit. Disabling Transmitter Delay Compensation mode\n",

"maximum limit"? Either "maximum" or "limit" should be enough. If the
value is above 127, does it make sense to disable the tdco completely?

> + tdco);
> + } else {
> + enable_tdc = true;

Why not set "reg_btp |= DBTP_TDC;" here directly?

> + m_can_write(priv, M_CAN_TDCR,
> + tdco << TDCR_TDCO_SHIFT);
> + }
> + }
> +
> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
> (tseg1 << DBTP_DTSEG1_SHIFT) |
> (tseg2 << DBTP_DTSEG2_SHIFT);

Adjust this to "reg_btp |=".

> +
> + if (enable_tdc)
> + reg_btp |= DBTP_TDC;

Please remove.

> +
> m_can_write(priv, M_CAN_DBTP, reg_btp);
> }
>
>

Marc

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

Attachments:

2018-01-02 16:07:27

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

On 12/22/2017 02:31 PM, Faiz Abbas wrote:
> From: Franklin S Cooper Jr <[email protected]>
>
> Add support for PM Runtime which is the new way to handle managing clocks.
> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
> management approach in place.

There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
CONFIG_PM_RUNTIME")

Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :

>> Well, I admit it would be nicer if drivers didn't have to worry about
>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>> from the one outlined above would have the probe routine do this:
>>
>> my_power_up(dev);
>> pm_runtime_set_active(dev);
>> pm_runtime_get_noresume(dev);
>> pm_runtime_enable(dev);

> PM_RUNTIME is required by OMAP based devices to handle clock management.
> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
> to work with this driver.

Who will set the SET_RUNTIME_PM_OPS in this case?

> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> [[email protected]: handle pm_runtime_get_sync() failure, fix some bugs]
> Signed-off-by: Sekhar Nori <[email protected]>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f72116e..53e764f 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -23,6 +23,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/iopoll.h>
> #include <linux/can/dev.h>
>
> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
> {
> int err;
>
> + err = pm_runtime_get_sync(priv->device);
> + if (err) {
> + pm_runtime_put_noidle(priv->device);

Why do you call this in case of an error?

> + return err;
> + }
> +
> err = clk_prepare_enable(priv->hclk);
> if (err)
> - return err;
> + goto pm_runtime_put;
>
> err = clk_prepare_enable(priv->cclk);
> if (err)
> - clk_disable_unprepare(priv->hclk);
> + goto disable_hclk;
>
> return err;
> +
> +disable_hclk:
> + clk_disable_unprepare(priv->hclk);
> +pm_runtime_put:
> + pm_runtime_put_sync(priv->device);
> + return err;
> }
>
> static void m_can_clk_stop(struct m_can_priv *priv)
> {
> + pm_runtime_put_sync(priv->device);
> +
> clk_disable_unprepare(priv->cclk);
> clk_disable_unprepare(priv->hclk);
> }
> @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev)
> /* Enable clocks. Necessary to read Core Release in order to determine
> * M_CAN version
> */
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret) {
> + pm_runtime_put_noidle(&pdev->dev);

Why do you call this in case of error?

> + goto pm_runtime_fail;
> + }
> +
> ret = clk_prepare_enable(hclk);
> if (ret)
> - goto disable_hclk_ret;
> + goto pm_runtime_put;
>
> ret = clk_prepare_enable(cclk);
> if (ret)
> - goto disable_cclk_ret;
> + goto disable_hclk_ret;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
> addr = devm_ioremap_resource(&pdev->dev, res);
> @@ -1666,6 +1688,11 @@ static int m_can_plat_probe(struct platform_device *pdev)
> clk_disable_unprepare(cclk);
> disable_hclk_ret:
> clk_disable_unprepare(hclk);
> +pm_runtime_put:
> + pm_runtime_put_sync(&pdev->dev);
> +pm_runtime_fail:
> + if (ret)
> + pm_runtime_disable(&pdev->dev);
> failed_ret:
> return ret;
> }
> @@ -1723,6 +1750,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
> struct net_device *dev = platform_get_drvdata(pdev);
>
> unregister_m_can_dev(dev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> platform_set_drvdata(pdev, NULL);
>
> free_m_can_dev(dev);
>

regards,
Marc

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

Attachments:

2018-01-02 16:16:22

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate

Attachments:

2018-01-03 12:19:22

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate

Hi Marc,

On Tuesday 02 January 2018 06:30 PM, Marc Kleine-Budde wrote:
> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>> From: Franklin S Cooper Jr <[email protected]>
>>
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>>
>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>> [[email protected]: fix build error with !CONFIG_OF]
>> Signed-off-by: Sekhar Nori <[email protected]>
>> Signed-off-by: Faiz Abbas <[email protected]>
>> ---
>> v6 changes:
>> fix build error with !CONFIG_OF
>>
>> drivers/net/can/dev.c | 39 +++++++++++++++++++++++++++++++++++++++
>> include/linux/can/dev.h | 8 ++++++++
>> 2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..007cfc0 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -27,6 +27,7 @@
>> #include <linux/can/skb.h>
>> #include <linux/can/netlink.h>
>> #include <linux/can/led.h>
>> +#include <linux/of.h>
>> #include <net/rtnetlink.h>
>>
>> #define MOD_DESC "CAN device driver interface"
>> @@ -814,6 +815,30 @@ int open_candev(struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(open_candev);
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> + struct device_node *dn;
>> + struct can_priv *priv = netdev_priv(dev);
>> + struct device_node *np = dev->dev.parent->of_node;
>> + int ret;
>> +
>> + dn = of_get_child_by_name(np, "can-transceiver");
>> + if (!dn)
>> + return;
>> +
>> + ret = of_property_read_u32(dn, "max-bitrate", &priv->max_bitrate);
>> + if ((ret && ret != -EINVAL) || (!ret && !priv->max_bitrate))
>> + netdev_warn(dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit.\n");
>> +}
>> +EXPORT_SYMBOL_GPL(of_can_transceiver);
>> +#endif
>> +
>> /*
>> * Common close function for cleanup before the device gets closed.
>> *
>> @@ -913,6 +938,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>> priv->bitrate_const_cnt);
>> if (err)
>> return err;
>> +
>> + if (priv->max_bitrate && bt.bitrate > priv->max_bitrate) {
>> + netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
>> + priv->max_bitrate);
>> + return -EINVAL;
>> + }
>
> What about movong this check into can_get_bittiming()? Although we loose
> the "arbitration" vs "canfd data".

I would prefer to keep the distinction.

>
>> +
>> memcpy(&priv->bittiming, &bt, sizeof(bt));
>>
>> if (priv->do_set_bittiming) {
>> @@ -997,6 +1029,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>> priv->data_bitrate_const_cnt);
>> if (err)
>> return err;
>> +
>> + if (priv->max_bitrate && dbt.bitrate > priv->max_bitrate) {
>> + netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
>> + priv->max_bitrate);
>> + return -EINVAL;
>> + }
>> +
>> memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
>>
>> if (priv->do_set_data_bittiming) {
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 61f1cf2..fbb7810 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -48,6 +48,8 @@ struct can_priv {
>> unsigned int data_bitrate_const_cnt;
>> struct can_clock clock;
>>
>> + unsigned int max_bitrate;
>
> Please make it an u32, name it "bitrate_max" and ...

Sure.

>
>>> struct can_priv {
>>> struct net_device *dev;
>>> struct can_device_stats can_stats;
>>>
>>> struct can_bittiming bittiming, data_bittiming;
>>> const struct can_bittiming_const *bittiming_const,
>>> *data_bittiming_const;
>>> const u16 *termination_const;
>>> unsigned int termination_const_cnt;
>>> u16 termination;
>>> const u32 *bitrate_const;
>>> unsigned int bitrate_const_cnt;
>>> const u32 *data_bitrate_const;
>>> unsigned int data_bitrate_const_cnt;
>
> ...move it here.

Ok.

Thanks,
Faiz

2018-01-03 12:23:50

[permalink] [raw]

Subject: Re: [PATCH v6 1/6] can: dev: Add support for limiting configured bitrate

Hi,

On Tuesday 02 January 2018 09:45 PM, Marc Kleine-Budde wrote:
> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>> From: Franklin S Cooper Jr <[email protected]>
>>
>> Various CAN or CAN-FD IP may be able to run at a faster rate than
>> what the transceiver the CAN node is connected to. This can lead to
>> unexpected errors. However, CAN transceivers typically have fixed
>> limitations and provide no means to discover these limitations at
>> runtime. Therefore, add support for a can-transceiver node that
>> can be reused by other CAN peripheral drivers to determine for both
>> CAN and CAN-FD what the max bitrate that can be used. If the user
>> tries to configure CAN to pass these maximum bitrates it will throw
>> an error.
>
> Please add support to read the maximum bitrate via netlink. Have a look
> at 12a6075cabc0 ("can: dev: add CAN interface termination API"). I think
> you need to extend the following functions: can_get_size() and
> can_fill_info().

Will add in the next version.

Thanks,
Faiz

>
> Marc
>

2018-01-03 12:40:40

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

Hi,

On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>> From: Franklin S Cooper Jr <[email protected]>
>>
>> Add support for PM Runtime which is the new way to handle managing clocks.
>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>> management approach in place.
>
> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
> CONFIG_PM_RUNTIME")

Ok. Will change the commit message.

>
> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>
>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>> from the one outlined above would have the probe routine do this:
>>>
>>> my_power_up(dev);
>>> pm_runtime_set_active(dev);
>>> pm_runtime_get_noresume(dev);
>>> pm_runtime_enable(dev);

This discussion seems to be about cases in which CONFIG_PM is not
enabled. CONFIG_PM is always selected in the case of omap devices.

>
>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>> to work with this driver.
>
> Who will set the SET_RUNTIME_PM_OPS in this case?

It is set with a common SET_RUNTIME_PM_OPS in the case of omap at
arch/arm/mach-omap2/omap_device.c:632

struct dev_pm_domain omap_device_pm_domain = {
.ops = {
SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
NULL)
USE_PLATFORM_PM_SLEEP_OPS
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq,
_od_resume_noirq)
}
};

>
>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>> [[email protected]: handle pm_runtime_get_sync() failure, fix some bugs]
>> Signed-off-by: Sekhar Nori <[email protected]>
>> Signed-off-by: Faiz Abbas <[email protected]>
>> ---
>> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f72116e..53e764f 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/iopoll.h>
>> #include <linux/can/dev.h>
>>
>> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
>> {
>> int err;
>>
>> + err = pm_runtime_get_sync(priv->device);
>> + if (err) {
>> + pm_runtime_put_noidle(priv->device);
>
> Why do you call this in case of an error?

pm_runtime_get_sync() increments the usage count of the device before
any error is returned. This needs to be decremented using
pm_runtime_put_noidle().

>
>> + return err;
>> + }
>> +
>> err = clk_prepare_enable(priv->hclk);
>> if (err)
>> - return err;
>> + goto pm_runtime_put;
>>
>> err = clk_prepare_enable(priv->cclk);
>> if (err)
>> - clk_disable_unprepare(priv->hclk);
>> + goto disable_hclk;
>>
>> return err;
>> +
>> +disable_hclk:
>> + clk_disable_unprepare(priv->hclk);
>> +pm_runtime_put:
>> + pm_runtime_put_sync(priv->device);
>> + return err;
>> }
>>
>> static void m_can_clk_stop(struct m_can_priv *priv)
>> {
>> + pm_runtime_put_sync(priv->device);
>> +
>> clk_disable_unprepare(priv->cclk);
>> clk_disable_unprepare(priv->hclk);
>> }
>> @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev)
>> /* Enable clocks. Necessary to read Core Release in order to determine
>> * M_CAN version
>> */
>> + pm_runtime_enable(&pdev->dev);
>> + ret = pm_runtime_get_sync(&pdev->dev);
>> + if (ret) {
>> + pm_runtime_put_noidle(&pdev->dev);
>
> Why do you call this in case of error?

Same here.

Thanks,
Faiz

2018-01-03 12:55:17

[permalink] [raw]

Subject: Re: [PATCH v6 4/6] can: m_can: Support higher speed CAN-FD bitrates

Hi,

On Tuesday 02 January 2018 07:05 PM, Marc Kleine-Budde wrote:
> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>> From: Franklin S Cooper Jr <[email protected]>
>>
>> During test transmitting using CAN-FD at high bitrates (> 2 Mbps)
>> would fail. Scoping the signals I noticed that only a single bit
>> was being transmitted and with a bit more investigation realized the actual
>> MCAN IP would go back to initialization mode automatically.
>>
>> It appears this issue is due to the MCAN needing to use the Transmitter
>> Delay Compensation Mode with the correct value for the transmitter delay
>> compensation offset (tdco). What impacts the tdco value isn't 100% clear
>> but to calculate it you use an equation defined in the MCAN User's Guide.
>>
>> The user guide mentions that this register needs to be set based on clock
>> values, secondary sample point and the data bitrate. One of the key
>> variables that can't automatically be determined is the secondary
>> sample point (ssp). This ssp is similar to the sp but is specific to this
>> transmitter delay compensation mode. The guidelines for configuring
>> ssp is rather vague but via some CAN test it appears for DRA76x that putting
>> the value same as data sampling point works.
>>
>> The CAN-CIA's "Bit Time Requirements for CAN FD" paper presented at
>> the International CAN Conference 2013 indicates that this TDC mode is
>> only needed for data bit rates above 2.5 Mbps. Therefore, only enable
>> this mode when the data bit rate is above 2.5 Mbps.
>>
>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>> Signed-off-by: Sekhar Nori <[email protected]>
>> Signed-off-by: Faiz Abbas <[email protected]>
>> ---
>> drivers/net/can/m_can/m_can.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index 53e764f..371ffc1 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -127,6 +127,12 @@ enum m_can_mram_cfg {
>> #define DBTP_DSJW_SHIFT 0
>> #define DBTP_DSJW_MASK (0xf << DBTP_DSJW_SHIFT)
>>
>> +/* Transmitter Delay Compensation Register (TDCR) */
>> +#define TDCR_TDCO_SHIFT 8
>> +#define TDCR_TDCO_MASK (0x7F << TDCR_TDCO_SHIFT)
>> +#define TDCR_TDCF_SHIFT 0
>> +#define TDCR_TDCF_MASK (0x7F << TDCR_TDCF_SHIFT)
>> +
>> /* Test Register (TEST) */
>> #define TEST_LBCK BIT(4)
>>
>> @@ -991,7 +997,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>> const struct can_bittiming *bt = &priv->can.bittiming;
>> const struct can_bittiming *dbt = &priv->can.data_bittiming;
>> u16 brp, sjw, tseg1, tseg2;
>> - u32 reg_btp;
>> + u32 reg_btp, tdco, ssp;
>
> Please move the tdco and the ssp into "if (dbt->bitrate > 2500000)" scope.

Ok.

>
> Initialize "reg_btp = 0", see below.
>
>> + bool enable_tdc = false;
>
> Please remove, see below.
>
>>
>> brp = bt->brp - 1;
>> sjw = bt->sjw - 1;
>> @@ -1006,9 +1013,41 @@ static int m_can_set_bittiming(struct net_device *dev)
>> sjw = dbt->sjw - 1;
>> tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>> tseg2 = dbt->phase_seg2 - 1;
>> +
>> + /* TDC is only needed for bitrates beyond 2.5 MBit/s.
>> + * This is mentioned in the "Bit Time Requirements for CAN FD"
>> + * paper presented at the International CAN Conference 2013
>> + */
>> + if (dbt->bitrate > 2500000) {
>> + /* Use the same value of secondary sampling point
>> + * as the data sampling point
>> + */
>> + ssp = dbt->sample_point;
>> +
>> + /* Equation based on Bosch's M_CAN User Manual's
>> + * Transmitter Delay Compensation Section
>> + */
>> + tdco = (priv->can.clock.freq / 1000) *
>> + ssp / dbt->bitrate;
>> +
>> + /* Max valid TDCO value is 127 */
>> + if (tdco > 127) {
>> + netdev_warn(dev, "TDCO value of %u is beyond maximum limit. Disabling Transmitter Delay Compensation mode\n",
>
> "maximum limit"? Either "maximum" or "limit" should be enough. If the
> value is above 127, does it make sense to disable the tdco completely?

I guess we can put the closest possible value (127) so that the ssp
calculated by the IP is as close to the chosen one as possible.

>
>> + tdco);
>> + } else {
>> + enable_tdc = true;
>
> Why not set "reg_btp |= DBTP_TDC;" here directly?
>
>> + m_can_write(priv, M_CAN_TDCR,
>> + tdco << TDCR_TDCO_SHIFT);
>> + }
>> + }
>> +
>> reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>> (tseg1 << DBTP_DTSEG1_SHIFT) |
>> (tseg2 << DBTP_DTSEG2_SHIFT);
>
> Adjust this to "reg_btp |=".
>
>> +
>> + if (enable_tdc)
>> + reg_btp |= DBTP_TDC;
>
> Please remove.

Sure, will remove.

Thanks,
Faiz

2018-01-03 14:25:51

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

On 01/03/2018 01:39 PM, Faiz Abbas wrote:
> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
>> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>>> From: Franklin S Cooper Jr <[email protected]>
>>>
>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>> management approach in place.
>>
>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
>> CONFIG_PM_RUNTIME")
>
> Ok. Will change the commit message.
>
>>
>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>>
>>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>>> from the one outlined above would have the probe routine do this:
>>>>
>>>> my_power_up(dev);
>>>> pm_runtime_set_active(dev);
>>>> pm_runtime_get_noresume(dev);
>>>> pm_runtime_enable(dev);
>
> This discussion seems to be about cases in which CONFIG_PM is not
> enabled. CONFIG_PM is always selected in the case of omap devices.

Yes, but in the commit message you state that you need to support
systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see
is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select
CONFIG_PM, then we can make the driver much simpler.

>>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>>> to work with this driver.
>>
>> Who will set the SET_RUNTIME_PM_OPS in this case?
>
> It is set with a common SET_RUNTIME_PM_OPS in the case of omap at
> arch/arm/mach-omap2/omap_device.c:632
>
> struct dev_pm_domain omap_device_pm_domain = {
> .ops = {
> SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
> NULL)
> USE_PLATFORM_PM_SLEEP_OPS
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq,
> _od_resume_noirq)
> }
> };
>
>
>>
>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>>> [[email protected]: handle pm_runtime_get_sync() failure, fix some bugs]
>>> Signed-off-by: Sekhar Nori <[email protected]>
>>> Signed-off-by: Faiz Abbas <[email protected]>
>>> ---
>>> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index f72116e..53e764f 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/of.h>
>>> #include <linux/of_device.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> #include <linux/iopoll.h>
>>> #include <linux/can/dev.h>
>>>
>>> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
>>> {
>>> int err;
>>>
>>> + err = pm_runtime_get_sync(priv->device);
>>> + if (err) {
>>> + pm_runtime_put_noidle(priv->device);
>>
>> Why do you call this in case of an error?
>
> pm_runtime_get_sync() increments the usage count of the device before
> any error is returned. This needs to be decremented using
> pm_runtime_put_noidle().

Oh, I'm curious how many drivers don't get this right.

Marc

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

Attachments:

2018-01-03 15:06:51

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

Hi,

On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote:
> On 01/03/2018 01:39 PM, Faiz Abbas wrote:
>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>>>> From: Franklin S Cooper Jr <[email protected]>
>>>>
>>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>>> management approach in place.
>>>
>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
>>> CONFIG_PM_RUNTIME")
>>
>> Ok. Will change the commit message.
>>
>>>
>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>>>
>>>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>>>> from the one outlined above would have the probe routine do this:
>>>>>
>>>>> my_power_up(dev);
>>>>> pm_runtime_set_active(dev);
>>>>> pm_runtime_get_noresume(dev);
>>>>> pm_runtime_enable(dev);
>>
>> This discussion seems to be about cases in which CONFIG_PM is not
>> enabled. CONFIG_PM is always selected in the case of omap devices.
>
> Yes, but in the commit message you state that you need to support
> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see
> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select
> CONFIG_PM, then we can make the driver much simpler.

Actually the old clock management (for hclk which is the interface
clock) is still required as mentioned in the cover letter. Will change
the rather misleading description.

Thanks,
Faiz

>
>>>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>>>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>>>> to work with this driver.
>>>
>>> Who will set the SET_RUNTIME_PM_OPS in this case?
>>
>> It is set with a common SET_RUNTIME_PM_OPS in the case of omap at
>> arch/arm/mach-omap2/omap_device.c:632
>>
>> struct dev_pm_domain omap_device_pm_domain = {
>> .ops = {
>> SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
>> NULL)
>> USE_PLATFORM_PM_SLEEP_OPS
>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq,
>> _od_resume_noirq)
>> }
>> };
>>
>>
>>>
>>>> Signed-off-by: Franklin S Cooper Jr <[email protected]>
>>>> [[email protected]: handle pm_runtime_get_sync() failure, fix some bugs]
>>>> Signed-off-by: Sekhar Nori <[email protected]>
>>>> Signed-off-by: Faiz Abbas <[email protected]>
>>>> ---
>>>> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>>> index f72116e..53e764f 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -23,6 +23,7 @@
>>>> #include <linux/of.h>
>>>> #include <linux/of_device.h>
>>>> #include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/iopoll.h>
>>>> #include <linux/can/dev.h>
>>>>
>>>> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv)
>>>> {
>>>> int err;
>>>>
>>>> + err = pm_runtime_get_sync(priv->device);
>>>> + if (err) {
>>>> + pm_runtime_put_noidle(priv->device);
>>>
>>> Why do you call this in case of an error?
>>
>> pm_runtime_get_sync() increments the usage count of the device before
>> any error is returned. This needs to be decremented using
>> pm_runtime_put_noidle().
>
> Oh, I'm curious how many drivers don't get this right.
>
> Marc
>

2018-01-03 15:17:35

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

On 01/03/2018 04:06 PM, Faiz Abbas wrote:
> Hi,
>
> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote:
>> On 01/03/2018 01:39 PM, Faiz Abbas wrote:
>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>>>>> From: Franklin S Cooper Jr <[email protected]>
>>>>>
>>>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>>>> management approach in place.
>>>>
>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
>>>> CONFIG_PM_RUNTIME")
>>>
>>> Ok. Will change the commit message.
>>>
>>>>
>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>>>>
>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>>>>> from the one outlined above would have the probe routine do this:
>>>>>>
>>>>>> my_power_up(dev);
>>>>>> pm_runtime_set_active(dev);
>>>>>> pm_runtime_get_noresume(dev);
>>>>>> pm_runtime_enable(dev);
>>>
>>> This discussion seems to be about cases in which CONFIG_PM is not
>>> enabled. CONFIG_PM is always selected in the case of omap devices.
>>
>> Yes, but in the commit message you state that you need to support
>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see
>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select
>> CONFIG_PM, then we can make the driver much simpler.
>
> Actually the old clock management (for hclk which is the interface
> clock) is still required as mentioned in the cover letter. Will change
> the rather misleading description.

Ok. So you can use the code as discussed on
https://patchwork.kernel.org/patch/9436507/ ?

Marc

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

Attachments:

2018-01-04 15:16:59

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

Hi,

On Wednesday 03 January 2018 08:47 PM, Marc Kleine-Budde wrote:
> On 01/03/2018 04:06 PM, Faiz Abbas wrote:
>> Hi,
>>
>> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote:
>>> On 01/03/2018 01:39 PM, Faiz Abbas wrote:
>>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
>>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>>>>>> From: Franklin S Cooper Jr <[email protected]>
>>>>>>
>>>>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>>>>> management approach in place.
>>>>>
>>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
>>>>> CONFIG_PM_RUNTIME")
>>>>
>>>> Ok. Will change the commit message.
>>>>
>>>>>
>>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>>>>>
>>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>>>>>> from the one outlined above would have the probe routine do this:
>>>>>>>
>>>>>>> my_power_up(dev);
>>>>>>> pm_runtime_set_active(dev);
>>>>>>> pm_runtime_get_noresume(dev);
>>>>>>> pm_runtime_enable(dev);
>>>>
>>>> This discussion seems to be about cases in which CONFIG_PM is not
>>>> enabled. CONFIG_PM is always selected in the case of omap devices.
>>>
>>> Yes, but in the commit message you state that you need to support
>>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see
>>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select
>>> CONFIG_PM, then we can make the driver much simpler.
>>
>> Actually the old clock management (for hclk which is the interface
>> clock) is still required as mentioned in the cover letter. Will change
>> the rather misleading description.
>
> Ok. So you can use the code as discussed on
> https://patchwork.kernel.org/patch/9436507/ ?

Looking at the kernel configuration, it seems like SAMA5D2 platform
selects CONFIG_PM (Wenyou, please confirm). So, it seems like the only
users of this driver always have CONFIG_PM enabled.

So I guess the best way is to maintain the current code for pm_runtime_*
and move the clock enable/disable to pm_runtime callbacks.

Something like this:

m_can_runtime_resume()
{
clk_prepare_enable(cclk);
clk_prepare_enable(hclk);
}

m_can_runtime_suspend()
{
clk_disable_unprepare(cclk);
clk_disable_unprepare(hclk);
}

SET_RUNTIME_PM_OPS(m_can_runtime_suspend, m_can_runtime_resume, NULL)

static void m_can_start(struct net_device *dev)
{
pm_runtime_get_sync(dev)
...
}

static void m_can_stop(struct net_device *dev)
{
...
pm_runtime_put_sync(dev)
}

Does that sound okay? If yes, I will go work on the implementation.

Thanks,
Faiz

2018-01-04 15:18:31

[permalink] [raw]

Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime

On 01/04/2018 04:17 PM, Faiz Abbas wrote:
> Hi,
>
> On Wednesday 03 January 2018 08:47 PM, Marc Kleine-Budde wrote:
>> On 01/03/2018 04:06 PM, Faiz Abbas wrote:
>>> Hi,
>>>
>>> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote:
>>>> On 01/03/2018 01:39 PM, Faiz Abbas wrote:
>>>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote:
>>>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote:
>>>>>>> From: Franklin S Cooper Jr <[email protected]>
>>>>>>>
>>>>>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>>>>>> management approach in place.
>>>>>>
>>>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate
>>>>>> CONFIG_PM_RUNTIME")
>>>>>
>>>>> Ok. Will change the commit message.
>>>>>
>>>>>>
>>>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ :
>>>>>>
>>>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about
>>>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach
>>>>>>>> from the one outlined above would have the probe routine do this:
>>>>>>>>
>>>>>>>> my_power_up(dev);
>>>>>>>> pm_runtime_set_active(dev);
>>>>>>>> pm_runtime_get_noresume(dev);
>>>>>>>> pm_runtime_enable(dev);
>>>>>
>>>>> This discussion seems to be about cases in which CONFIG_PM is not
>>>>> enabled. CONFIG_PM is always selected in the case of omap devices.
>>>>
>>>> Yes, but in the commit message you state that you need to support
>>>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see
>>>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select
>>>> CONFIG_PM, then we can make the driver much simpler.
>>>
>>> Actually the old clock management (for hclk which is the interface
>>> clock) is still required as mentioned in the cover letter. Will change
>>> the rather misleading description.
>>
>> Ok. So you can use the code as discussed on
>> https://patchwork.kernel.org/patch/9436507/ ?
>
> Looking at the kernel configuration, it seems like SAMA5D2 platform
> selects CONFIG_PM (Wenyou, please confirm). So, it seems like the only
> users of this driver always have CONFIG_PM enabled.
>
> So I guess the best way is to maintain the current code for pm_runtime_*
> and move the clock enable/disable to pm_runtime callbacks.
>
> Something like this:
>
> m_can_runtime_resume()
> {
> clk_prepare_enable(cclk);
> clk_prepare_enable(hclk);
> }
>
> m_can_runtime_suspend()
> {
> clk_disable_unprepare(cclk);
> clk_disable_unprepare(hclk);
> }
>
> SET_RUNTIME_PM_OPS(m_can_runtime_suspend, m_can_runtime_resume, NULL)
>
> static void m_can_start(struct net_device *dev)
> {
> pm_runtime_get_sync(dev)
> ...
> }
>
> static void m_can_stop(struct net_device *dev)
> {
> ...
> pm_runtime_put_sync(dev)
> }
>
> Does that sound okay? If yes, I will go work on the implementation.

ACK + error checking.

Marc

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

Attachments: