* [RFC PATCH 01/14] can: dev: add struct data_bittiming_params to group FD parameters
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 02/14] can: netlink: replace tabulation by space in assignement Vincent Mailhol
` (14 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
This is a preparation patch for the introduction of CAN XL.
CAN FD and CAN XL uses similar bittiming parameters. Add one level of
nesting for all the CAN FD parameters. Typically:
priv->can.data_bittiming;
becomes:
priv->can.fd.data_bittiming;
This way, the CAN XL equivalent (to be introduced later) would be:
priv->can.xl.data_bittiming;
Add the new struct data_bittiming_params which contains all the data
bittiming parameters, including the TDC and the callback functions.
This done, all the drivers are updated to make use of the new layout.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/ctucanfd/ctucanfd_base.c | 8 +-
drivers/net/can/dev/dev.c | 12 +--
drivers/net/can/dev/netlink.c | 74 +++++++++----------
drivers/net/can/flexcan/flexcan-core.c | 4 +-
drivers/net/can/ifi_canfd/ifi_canfd.c | 10 +--
drivers/net/can/kvaser_pciefd.c | 6 +-
drivers/net/can/m_can/m_can.c | 8 +-
drivers/net/can/peak_canfd/peak_canfd.c | 6 +-
drivers/net/can/rcar/rcar_canfd.c | 4 +-
.../net/can/rockchip/rockchip_canfd-core.c | 4 +-
.../can/rockchip/rockchip_canfd-timestamp.c | 2 +-
.../net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +-
drivers/net/can/usb/esd_usb.c | 6 +-
drivers/net/can/usb/etas_es58x/es58x_core.c | 4 +-
drivers/net/can/usb/etas_es58x/es58x_fd.c | 6 +-
drivers/net/can/usb/gs_usb.c | 8 +-
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 2 +-
.../net/can/usb/kvaser_usb/kvaser_usb_core.c | 6 +-
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 6 +-
drivers/net/can/xilinx_can.c | 6 +-
include/linux/can/dev.h | 28 ++++---
21 files changed, 109 insertions(+), 105 deletions(-)
diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
index 64c349fd4600..18bd7e4d23a5 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_base.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
@@ -275,7 +275,7 @@ static int ctucan_set_bittiming(struct net_device *ndev)
static int ctucan_set_data_bittiming(struct net_device *ndev)
{
struct ctucan_priv *priv = netdev_priv(ndev);
- struct can_bittiming *dbt = &priv->can.data_bittiming;
+ struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
/* Note that dbt may be modified here */
return ctucan_set_btr(ndev, dbt, false);
@@ -290,7 +290,7 @@ static int ctucan_set_data_bittiming(struct net_device *ndev)
static int ctucan_set_secondary_sample_point(struct net_device *ndev)
{
struct ctucan_priv *priv = netdev_priv(ndev);
- struct can_bittiming *dbt = &priv->can.data_bittiming;
+ struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
int ssp_offset = 0;
u32 ssp_cfg = 0; /* No SSP by default */
@@ -1356,12 +1356,12 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
priv->ntxbufs = ntxbufs;
priv->dev = dev;
priv->can.bittiming_const = &ctu_can_fd_bit_timing_max;
- priv->can.data_bittiming_const = &ctu_can_fd_bit_timing_data_max;
+ priv->can.fd.data_bittiming_const = &ctu_can_fd_bit_timing_data_max;
priv->can.do_set_mode = ctucan_do_set_mode;
/* Needed for timing adjustment to be performed as soon as possible */
priv->can.do_set_bittiming = ctucan_set_bittiming;
- priv->can.do_set_data_bittiming = ctucan_set_data_bittiming;
+ priv->can.fd.do_set_data_bittiming = ctucan_set_data_bittiming;
priv->can.do_get_berr_counter = ctucan_get_berr_counter;
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 6792c14fd7eb..6aeed870815f 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -406,8 +406,8 @@ int open_candev(struct net_device *dev)
/* For CAN FD the data bitrate has to be >= the arbitration bitrate */
if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
- (!priv->data_bittiming.bitrate ||
- priv->data_bittiming.bitrate < priv->bittiming.bitrate)) {
+ (!priv->fd.data_bittiming.bitrate ||
+ priv->fd.data_bittiming.bitrate < priv->bittiming.bitrate)) {
netdev_err(dev, "incorrect/missing data bit-timing\n");
return -EINVAL;
}
@@ -545,16 +545,16 @@ int register_candev(struct net_device *dev)
if (!priv->bitrate_const != !priv->bitrate_const_cnt)
return -EINVAL;
- if (!priv->data_bitrate_const != !priv->data_bitrate_const_cnt)
+ if (!priv->fd.data_bitrate_const != !priv->fd.data_bitrate_const_cnt)
return -EINVAL;
/* We only support either fixed bit rates or bit timing const. */
- if ((priv->bitrate_const || priv->data_bitrate_const) &&
- (priv->bittiming_const || priv->data_bittiming_const))
+ if ((priv->bitrate_const || priv->fd.data_bitrate_const) &&
+ (priv->bittiming_const || priv->fd.data_bittiming_const))
return -EINVAL;
if (!can_bittiming_const_valid(priv->bittiming_const) ||
- !can_bittiming_const_valid(priv->data_bittiming_const))
+ !can_bittiming_const_valid(priv->fd.data_bittiming_const))
return -EINVAL;
if (!priv->termination_const) {
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 01aacdcda260..7455a7c5a383 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -141,7 +141,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
{
struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
struct can_tdc tdc = { 0 };
- const struct can_tdc_const *tdc_const = priv->tdc_const;
+ const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
int err;
if (!tdc_const || !can_tdc_is_enabled(priv))
@@ -179,7 +179,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
tdc.tdcf = tdcf;
}
- priv->tdc = tdc;
+ priv->fd.tdc = tdc;
return 0;
}
@@ -228,10 +228,10 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
dev->mtu = CANFD_MTU;
} else {
dev->mtu = CAN_MTU;
- memset(&priv->data_bittiming, 0,
- sizeof(priv->data_bittiming));
+ memset(&priv->fd.data_bittiming, 0,
+ sizeof(priv->fd.data_bittiming));
priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
- memset(&priv->tdc, 0, sizeof(priv->tdc));
+ memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
}
tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
@@ -312,16 +312,16 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
* directly via do_set_bitrate(). Bail out if neither
* is given.
*/
- if (!priv->data_bittiming_const && !priv->do_set_data_bittiming &&
- !priv->data_bitrate_const)
+ if (!priv->fd.data_bittiming_const && !priv->fd.do_set_data_bittiming &&
+ !priv->fd.data_bitrate_const)
return -EOPNOTSUPP;
memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
sizeof(dbt));
err = can_get_bittiming(dev, &dbt,
- priv->data_bittiming_const,
- priv->data_bitrate_const,
- priv->data_bitrate_const_cnt,
+ priv->fd.data_bittiming_const,
+ priv->fd.data_bitrate_const,
+ priv->fd.data_bitrate_const_cnt,
extack);
if (err)
return err;
@@ -333,7 +333,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
return -EINVAL;
}
- memset(&priv->tdc, 0, sizeof(priv->tdc));
+ memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
if (data[IFLA_CAN_TDC]) {
/* TDC parameters are provided: use them */
err = can_tdc_changelink(priv, data[IFLA_CAN_TDC],
@@ -346,17 +346,17 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
/* Neither of TDC parameters nor TDC flags are
* provided: do calculation
*/
- can_calc_tdco(&priv->tdc, priv->tdc_const, &dbt,
+ can_calc_tdco(&priv->fd.tdc, priv->fd.tdc_const, &dbt,
&priv->ctrlmode, priv->ctrlmode_supported);
} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
* turned off. TDC is disabled: do nothing
*/
- memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
+ memcpy(&priv->fd.data_bittiming, &dbt, sizeof(dbt));
- if (priv->do_set_data_bittiming) {
+ if (priv->fd.do_set_data_bittiming) {
/* Finally, set the bit-timing registers */
- err = priv->do_set_data_bittiming(dev);
+ err = priv->fd.do_set_data_bittiming(dev);
if (err)
return err;
}
@@ -394,7 +394,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
struct can_priv *priv = netdev_priv(dev);
size_t size;
- if (!priv->tdc_const)
+ if (!priv->fd.tdc_const)
return 0;
size = nla_total_size(0); /* nest IFLA_CAN_TDC */
@@ -404,17 +404,17 @@ static size_t can_tdc_get_size(const struct net_device *dev)
}
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MIN */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
- if (priv->tdc_const->tdcf_max) {
+ if (priv->fd.tdc_const->tdcf_max) {
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MIN */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
}
if (can_tdc_is_enabled(priv)) {
if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL ||
- priv->do_get_auto_tdcv)
+ priv->fd.do_get_auto_tdcv)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
- if (priv->tdc_const->tdcf_max)
+ if (priv->fd.tdc_const->tdcf_max)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
}
@@ -442,9 +442,9 @@ static size_t can_get_size(const struct net_device *dev)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_RESTART_MS */
if (priv->do_get_berr_counter) /* IFLA_CAN_BERR_COUNTER */
size += nla_total_size(sizeof(struct can_berr_counter));
- if (priv->data_bittiming.bitrate) /* IFLA_CAN_DATA_BITTIMING */
+ if (priv->fd.data_bittiming.bitrate) /* IFLA_CAN_DATA_BITTIMING */
size += nla_total_size(sizeof(struct can_bittiming));
- if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
+ if (priv->fd.data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
size += nla_total_size(sizeof(struct can_bittiming_const));
if (priv->termination_const) {
size += nla_total_size(sizeof(priv->termination)); /* IFLA_CAN_TERMINATION */
@@ -454,9 +454,9 @@ static size_t can_get_size(const struct net_device *dev)
if (priv->bitrate_const) /* IFLA_CAN_BITRATE_CONST */
size += nla_total_size(sizeof(*priv->bitrate_const) *
priv->bitrate_const_cnt);
- if (priv->data_bitrate_const) /* IFLA_CAN_DATA_BITRATE_CONST */
- size += nla_total_size(sizeof(*priv->data_bitrate_const) *
- priv->data_bitrate_const_cnt);
+ if (priv->fd.data_bitrate_const) /* IFLA_CAN_DATA_BITRATE_CONST */
+ size += nla_total_size(sizeof(*priv->fd.data_bitrate_const) *
+ priv->fd.data_bitrate_const_cnt);
size += sizeof(priv->bitrate_max); /* IFLA_CAN_BITRATE_MAX */
size += can_tdc_get_size(dev); /* IFLA_CAN_TDC */
size += can_ctrlmode_ext_get_size(); /* IFLA_CAN_CTRLMODE_EXT */
@@ -468,8 +468,8 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
{
struct nlattr *nest;
struct can_priv *priv = netdev_priv(dev);
- struct can_tdc *tdc = &priv->tdc;
- const struct can_tdc_const *tdc_const = priv->tdc_const;
+ struct can_tdc *tdc = &priv->fd.tdc;
+ const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
if (!tdc_const)
return 0;
@@ -497,8 +497,8 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
tdcv = tdc->tdcv;
err = 0;
- } else if (priv->do_get_auto_tdcv) {
- err = priv->do_get_auto_tdcv(dev, &tdcv);
+ } else if (priv->fd.do_get_auto_tdcv) {
+ err = priv->fd.do_get_auto_tdcv(dev, &tdcv);
}
if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
goto err_cancel;
@@ -564,14 +564,14 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
!priv->do_get_berr_counter(dev, &bec) &&
nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
- (priv->data_bittiming.bitrate &&
+ (priv->fd.data_bittiming.bitrate &&
nla_put(skb, IFLA_CAN_DATA_BITTIMING,
- sizeof(priv->data_bittiming), &priv->data_bittiming)) ||
+ sizeof(priv->fd.data_bittiming), &priv->fd.data_bittiming)) ||
- (priv->data_bittiming_const &&
+ (priv->fd.data_bittiming_const &&
nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
- sizeof(*priv->data_bittiming_const),
- priv->data_bittiming_const)) ||
+ sizeof(*priv->fd.data_bittiming_const),
+ priv->fd.data_bittiming_const)) ||
(priv->termination_const &&
(nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination) ||
@@ -586,11 +586,11 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
priv->bitrate_const_cnt,
priv->bitrate_const)) ||
- (priv->data_bitrate_const &&
+ (priv->fd.data_bitrate_const &&
nla_put(skb, IFLA_CAN_DATA_BITRATE_CONST,
- sizeof(*priv->data_bitrate_const) *
- priv->data_bitrate_const_cnt,
- priv->data_bitrate_const)) ||
+ sizeof(*priv->fd.data_bitrate_const) *
+ priv->fd.data_bitrate_const_cnt,
+ priv->fd.data_bitrate_const)) ||
(nla_put(skb, IFLA_CAN_BITRATE_MAX,
sizeof(priv->bitrate_max),
diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index d11411029330..61e5d4b0775b 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -1211,7 +1211,7 @@ static void flexcan_set_bittiming_cbt(const struct net_device *dev)
{
struct flexcan_priv *priv = netdev_priv(dev);
struct can_bittiming *bt = &priv->can.bittiming;
- struct can_bittiming *dbt = &priv->can.data_bittiming;
+ struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
struct flexcan_regs __iomem *regs = priv->regs;
u32 reg_cbt, reg_fdctrl;
@@ -2191,7 +2191,7 @@ static int flexcan_probe(struct platform_device *pdev)
priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
CAN_CTRLMODE_FD_NON_ISO;
priv->can.bittiming_const = &flexcan_fd_bittiming_const;
- priv->can.data_bittiming_const =
+ priv->can.fd.data_bittiming_const =
&flexcan_fd_data_bittiming_const;
} else {
priv->can.bittiming_const = &flexcan_bittiming_const;
diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 72307297d75e..0875bac2330f 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -647,7 +647,7 @@ static void ifi_canfd_set_bittiming(struct net_device *ndev)
{
struct ifi_canfd_priv *priv = netdev_priv(ndev);
const struct can_bittiming *bt = &priv->can.bittiming;
- const struct can_bittiming *dbt = &priv->can.data_bittiming;
+ const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
u16 brp, sjw, tseg1, tseg2, tdc;
/* Configure bit timing */
@@ -978,10 +978,10 @@ static int ifi_canfd_plat_probe(struct platform_device *pdev)
priv->can.clock.freq = readl(addr + IFI_CANFD_CANCLOCK);
- priv->can.bittiming_const = &ifi_canfd_bittiming_const;
- priv->can.data_bittiming_const = &ifi_canfd_bittiming_const;
- priv->can.do_set_mode = ifi_canfd_set_mode;
- priv->can.do_get_berr_counter = ifi_canfd_get_berr_counter;
+ priv->can.bittiming_const = &ifi_canfd_bittiming_const;
+ priv->can.fd.data_bittiming_const = &ifi_canfd_bittiming_const;
+ priv->can.do_set_mode = ifi_canfd_set_mode;
+ priv->can.do_get_berr_counter = ifi_canfd_get_berr_counter;
/* IFI CANFD can do both Bosch FD and ISO FD */
priv->can.ctrlmode = CAN_CTRLMODE_FD;
diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 9283408d1b29..6f267c816602 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -856,7 +856,7 @@ static int kvaser_pciefd_set_bittiming(struct kvaser_pciefd_can *can, bool data)
struct can_bittiming *bt;
if (data)
- bt = &can->can.data_bittiming;
+ bt = &can->can.fd.data_bittiming;
else
bt = &can->can.bittiming;
@@ -991,9 +991,9 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
spin_lock_init(&can->lock);
can->can.bittiming_const = &kvaser_pciefd_bittiming_const;
- can->can.data_bittiming_const = &kvaser_pciefd_bittiming_const;
+ can->can.fd.data_bittiming_const = &kvaser_pciefd_bittiming_const;
can->can.do_set_bittiming = kvaser_pciefd_set_nominal_bittiming;
- can->can.do_set_data_bittiming = kvaser_pciefd_set_data_bittiming;
+ can->can.fd.do_set_data_bittiming = kvaser_pciefd_set_data_bittiming;
can->can.do_set_mode = kvaser_pciefd_set_mode;
can->can.do_get_berr_counter = kvaser_pciefd_get_berr_counter;
can->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 012c3d22b01d..ec8522567d97 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1347,7 +1347,7 @@ static int m_can_set_bittiming(struct net_device *dev)
{
struct m_can_classdev *cdev = netdev_priv(dev);
const struct can_bittiming *bt = &cdev->can.bittiming;
- const struct can_bittiming *dbt = &cdev->can.data_bittiming;
+ const struct can_bittiming *dbt = &cdev->can.fd.data_bittiming;
u16 brp, sjw, tseg1, tseg2;
u32 reg_btp;
@@ -1704,7 +1704,7 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
if (err)
return err;
cdev->can.bittiming_const = &m_can_bittiming_const_30X;
- cdev->can.data_bittiming_const = &m_can_data_bittiming_const_30X;
+ cdev->can.fd.data_bittiming_const = &m_can_data_bittiming_const_30X;
break;
case 31:
/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.1.x */
@@ -1712,13 +1712,13 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
if (err)
return err;
cdev->can.bittiming_const = &m_can_bittiming_const_31X;
- cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
+ cdev->can.fd.data_bittiming_const = &m_can_data_bittiming_const_31X;
break;
case 32:
case 33:
/* Support both MCAN version v3.2.x and v3.3.0 */
cdev->can.bittiming_const = &m_can_bittiming_const_31X;
- cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
+ cdev->can.fd.data_bittiming_const = &m_can_data_bittiming_const_31X;
niso = m_can_niso_supported(cdev);
if (niso < 0)
diff --git a/drivers/net/can/peak_canfd/peak_canfd.c b/drivers/net/can/peak_canfd/peak_canfd.c
index 28f3fd805273..77292afaed22 100644
--- a/drivers/net/can/peak_canfd/peak_canfd.c
+++ b/drivers/net/can/peak_canfd/peak_canfd.c
@@ -624,7 +624,7 @@ static int peak_canfd_set_data_bittiming(struct net_device *ndev)
{
struct peak_canfd_priv *priv = netdev_priv(ndev);
- return pucan_set_timing_fast(priv, &priv->can.data_bittiming);
+ return pucan_set_timing_fast(priv, &priv->can.fd.data_bittiming);
}
static int peak_canfd_close(struct net_device *ndev)
@@ -813,12 +813,12 @@ struct net_device *alloc_peak_canfd_dev(int sizeof_priv, int index,
/* complete now socket-can initialization side */
priv->can.state = CAN_STATE_STOPPED;
priv->can.bittiming_const = &peak_canfd_nominal_const;
- priv->can.data_bittiming_const = &peak_canfd_data_const;
+ priv->can.fd.data_bittiming_const = &peak_canfd_data_const;
priv->can.do_set_mode = peak_canfd_set_mode;
priv->can.do_get_berr_counter = peak_canfd_get_berr_counter;
priv->can.do_set_bittiming = peak_canfd_set_bittiming;
- priv->can.do_set_data_bittiming = peak_canfd_set_data_bittiming;
+ priv->can.fd.do_set_data_bittiming = peak_canfd_set_data_bittiming;
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_LISTENONLY |
CAN_CTRLMODE_3_SAMPLES |
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index c919668bbe7a..7b11dc5a66e4 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1312,7 +1312,7 @@ static void rcar_canfd_set_bittiming(struct net_device *dev)
struct rcar_canfd_channel *priv = netdev_priv(dev);
struct rcar_canfd_global *gpriv = priv->gpriv;
const struct can_bittiming *bt = &priv->can.bittiming;
- const struct can_bittiming *dbt = &priv->can.data_bittiming;
+ const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
u16 brp, sjw, tseg1, tseg2;
u32 cfg;
u32 ch = priv->channel;
@@ -1791,7 +1791,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
if (gpriv->fdmode) {
priv->can.bittiming_const = &rcar_canfd_nom_bittiming_const;
- priv->can.data_bittiming_const =
+ priv->can.fd.data_bittiming_const =
&rcar_canfd_data_bittiming_const;
/* Controller starts in CAN FD only mode */
diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 6883153e8fc1..791a70913391 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-core.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
@@ -118,7 +118,7 @@ static void rkcanfd_chip_set_work_mode(const struct rkcanfd_priv *priv)
static int rkcanfd_set_bittiming(struct rkcanfd_priv *priv)
{
- const struct can_bittiming *dbt = &priv->can.data_bittiming;
+ const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
const struct can_bittiming *bt = &priv->can.bittiming;
u32 reg_nbt, reg_dbt, reg_tdc;
u32 tdco;
@@ -906,7 +906,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
priv->can.clock.freq = clk_get_rate(priv->clks[0].clk);
priv->can.bittiming_const = &rkcanfd_bittiming_const;
- priv->can.data_bittiming_const = &rkcanfd_data_bittiming_const;
+ priv->can.fd.data_bittiming_const = &rkcanfd_data_bittiming_const;
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_BERR_REPORTING;
if (!(priv->devtype_data.quirks & RKCANFD_QUIRK_CANFD_BROKEN))
diff --git a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
index fb1a8f4e6217..dc4c9643aa34 100644
--- a/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
+++ b/drivers/net/can/rockchip/rockchip_canfd-timestamp.c
@@ -39,7 +39,7 @@ static void rkcanfd_timestamp_work(struct work_struct *work)
void rkcanfd_timestamp_init(struct rkcanfd_priv *priv)
{
- const struct can_bittiming *dbt = &priv->can.data_bittiming;
+ const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
const struct can_bittiming *bt = &priv->can.bittiming;
struct cyclecounter *cc = &priv->cc;
u32 bitrate, div, reg, rate;
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 3e7526274e34..fad2d23a60ca 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -509,7 +509,7 @@ static int mcp251xfd_chip_timestamp_init(const struct mcp251xfd_priv *priv)
static int mcp251xfd_set_bittiming(const struct mcp251xfd_priv *priv)
{
const struct can_bittiming *bt = &priv->can.bittiming;
- const struct can_bittiming *dbt = &priv->can.data_bittiming;
+ const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
u32 val = 0;
s8 tdco;
int err;
@@ -2082,7 +2082,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
priv->can.do_set_mode = mcp251xfd_set_mode;
priv->can.do_get_berr_counter = mcp251xfd_get_berr_counter;
priv->can.bittiming_const = &mcp251xfd_bittiming_const;
- priv->can.data_bittiming_const = &mcp251xfd_data_bittiming_const;
+ priv->can.fd.data_bittiming_const = &mcp251xfd_data_bittiming_const;
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING |
CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 41a0e4261d15..b0f3daa85b13 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -1098,7 +1098,7 @@ static int esd_usb_3_set_bittiming(struct net_device *netdev)
const struct can_bittiming_const *data_btc = &esd_usb_3_data_bittiming_const;
struct esd_usb_net_priv *priv = netdev_priv(netdev);
struct can_bittiming *nom_bt = &priv->can.bittiming;
- struct can_bittiming *data_bt = &priv->can.data_bittiming;
+ struct can_bittiming *data_bt = &priv->can.fd.data_bittiming;
struct esd_usb_3_set_baudrate_msg_x *baud_x;
union esd_usb_msg *msg;
u16 flags = 0;
@@ -1222,9 +1222,9 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
priv->can.bittiming_const = &esd_usb_3_nom_bittiming_const;
- priv->can.data_bittiming_const = &esd_usb_3_data_bittiming_const;
+ priv->can.fd.data_bittiming_const = &esd_usb_3_data_bittiming_const;
priv->can.do_set_bittiming = esd_usb_3_set_bittiming;
- priv->can.do_set_data_bittiming = esd_usb_3_set_bittiming;
+ priv->can.fd.do_set_data_bittiming = esd_usb_3_set_bittiming;
break;
case ESD_USB_CANUSBM_PRODUCT_ID:
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 5e3a72b7c469..6fbfdc2fa00d 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2059,8 +2059,8 @@ static int es58x_init_priv(struct es58x_device *es58x_dev,
can->bittiming_const = param->bittiming_const;
if (param->ctrlmode_supported & CAN_CTRLMODE_FD) {
- can->data_bittiming_const = param->data_bittiming_const;
- can->tdc_const = param->tdc_const;
+ can->fd.data_bittiming_const = param->data_bittiming_const;
+ can->fd.tdc_const = param->tdc_const;
}
can->bitrate_max = param->bitrate_max;
can->clock = param->clock;
diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
index fa87b0b78e3e..ab67fe0508ec 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
@@ -427,12 +427,12 @@ static int es58x_fd_enable_channel(struct es58x_priv *priv)
if (tx_conf_msg.canfd_enabled) {
es58x_fd_convert_bittiming(&tx_conf_msg.data_bittiming,
- &priv->can.data_bittiming);
+ &priv->can.fd.data_bittiming);
if (can_tdc_is_enabled(&priv->can)) {
tx_conf_msg.tdc_enabled = 1;
- tx_conf_msg.tdco = cpu_to_le16(priv->can.tdc.tdco);
- tx_conf_msg.tdcf = cpu_to_le16(priv->can.tdc.tdcf);
+ tx_conf_msg.tdco = cpu_to_le16(priv->can.fd.tdc.tdco);
+ tx_conf_msg.tdcf = cpu_to_le16(priv->can.fd.tdc.tdcf);
}
conf_len = ES58X_FD_CANFD_CONF_LEN;
diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index bc86e9b329fd..a84152453809 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -725,7 +725,7 @@ static int gs_usb_set_bittiming(struct net_device *netdev)
static int gs_usb_set_data_bittiming(struct net_device *netdev)
{
struct gs_can *dev = netdev_priv(netdev);
- struct can_bittiming *bt = &dev->can.data_bittiming;
+ struct can_bittiming *bt = &dev->can.fd.data_bittiming;
struct gs_device_bittiming dbt = {
.prop_seg = cpu_to_le32(bt->prop_seg),
.phase_seg1 = cpu_to_le32(bt->phase_seg1),
@@ -1298,8 +1298,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
/* The data bit timing will be overwritten, if
* GS_CAN_FEATURE_BT_CONST_EXT is set.
*/
- dev->can.data_bittiming_const = &dev->bt_const;
- dev->can.do_set_data_bittiming = gs_usb_set_data_bittiming;
+ dev->can.fd.data_bittiming_const = &dev->bt_const;
+ dev->can.fd.do_set_data_bittiming = gs_usb_set_data_bittiming;
}
if (feature & GS_CAN_FEATURE_TERMINATION) {
@@ -1379,7 +1379,7 @@ static struct gs_can *gs_make_candev(unsigned int channel,
dev->data_bt_const.brp_max = le32_to_cpu(bt_const_extended.dbrp_max);
dev->data_bt_const.brp_inc = le32_to_cpu(bt_const_extended.dbrp_inc);
- dev->can.data_bittiming_const = &dev->data_bt_const;
+ dev->can.fd.data_bittiming_const = &dev->data_bt_const;
}
can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index 078496d9b7ba..f6c77eca9f43 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -137,7 +137,7 @@ struct kvaser_usb_net_priv {
* @dev_set_mode: used for can.do_set_mode
* @dev_set_bittiming: used for can.do_set_bittiming
* @dev_get_busparams: readback arbitration busparams
- * @dev_set_data_bittiming: used for can.do_set_data_bittiming
+ * @dev_set_data_bittiming: used for can.fd.do_set_data_bittiming
* @dev_get_data_busparams: readback data busparams
* @dev_get_berr_counter: used for can.do_get_berr_counter
*
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 7d12776ab63e..bd565168bd0e 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -592,7 +592,7 @@ static int kvaser_usb_set_data_bittiming(struct net_device *netdev)
struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
struct kvaser_usb *dev = priv->dev;
const struct kvaser_usb_dev_ops *ops = dev->driver_info->ops;
- struct can_bittiming *dbt = &priv->can.data_bittiming;
+ struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
struct kvaser_usb_busparams busparams;
int tseg1 = dbt->prop_seg + dbt->phase_seg1;
int tseg2 = dbt->phase_seg2;
@@ -841,8 +841,8 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev, int channel)
priv->can.ctrlmode_supported |= dev->card_data.ctrlmode_supported;
if (priv->can.ctrlmode_supported & CAN_CTRLMODE_FD) {
- priv->can.data_bittiming_const = dev->cfg->data_bittiming_const;
- priv->can.do_set_data_bittiming = kvaser_usb_set_data_bittiming;
+ priv->can.fd.data_bittiming_const = dev->cfg->data_bittiming_const;
+ priv->can.fd.do_set_data_bittiming = kvaser_usb_set_data_bittiming;
}
netdev->flags |= IFF_ECHO;
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 59f7cd8ceb39..117637b9b995 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -770,7 +770,7 @@ static int peak_usb_set_data_bittiming(struct net_device *netdev)
const struct peak_usb_adapter *pa = dev->adapter;
if (pa->dev_set_data_bittiming) {
- struct can_bittiming *bt = &dev->can.data_bittiming;
+ struct can_bittiming *bt = &dev->can.fd.data_bittiming;
int err = pa->dev_set_data_bittiming(dev, bt);
if (err)
@@ -954,8 +954,8 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
dev->can.clock = peak_usb_adapter->clock;
dev->can.bittiming_const = peak_usb_adapter->bittiming_const;
dev->can.do_set_bittiming = peak_usb_set_bittiming;
- dev->can.data_bittiming_const = peak_usb_adapter->data_bittiming_const;
- dev->can.do_set_data_bittiming = peak_usb_set_data_bittiming;
+ dev->can.fd.data_bittiming_const = peak_usb_adapter->data_bittiming_const;
+ dev->can.fd.do_set_data_bittiming = peak_usb_set_data_bittiming;
dev->can.do_set_mode = peak_usb_set_mode;
dev->can.do_get_berr_counter = peak_usb_adapter->do_get_berr_counter;
dev->can.ctrlmode_supported = peak_usb_adapter->ctrlmode_supported;
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index d944911d7f05..88dddb627a15 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -481,7 +481,7 @@ static int xcan_set_bittiming(struct net_device *ndev)
{
struct xcan_priv *priv = netdev_priv(ndev);
struct can_bittiming *bt = &priv->can.bittiming;
- struct can_bittiming *dbt = &priv->can.data_bittiming;
+ struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
u32 btr0, btr1;
u32 is_config_mode;
@@ -1967,13 +1967,13 @@ static int xcan_probe(struct platform_device *pdev)
goto err_free;
if (devtype->cantype == XAXI_CANFD) {
- priv->can.data_bittiming_const =
+ priv->can.fd.data_bittiming_const =
&xcan_data_bittiming_const_canfd;
priv->can.tdc_const = &xcan_tdc_const_canfd;
}
if (devtype->cantype == XAXI_CANFD_2_0) {
- priv->can.data_bittiming_const =
+ priv->can.fd.data_bittiming_const =
&xcan_data_bittiming_const_canfd2;
priv->can.tdc_const = &xcan_tdc_const_canfd2;
}
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 23492213ea35..492d23bec7be 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -38,6 +38,17 @@ enum can_termination_gpio {
CAN_TERMINATION_GPIO_MAX,
};
+struct data_bittiming_params {
+ const struct can_bittiming_const *data_bittiming_const;
+ struct can_bittiming data_bittiming;
+ const struct can_tdc_const *tdc_const;
+ struct can_tdc tdc;
+ const u32 *data_bitrate_const;
+ unsigned int data_bitrate_const_cnt;
+ int (*do_set_data_bittiming)(struct net_device *dev);
+ int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
+};
+
/*
* CAN common private data
*/
@@ -45,16 +56,11 @@ struct can_priv {
struct net_device *dev;
struct can_device_stats can_stats;
- const struct can_bittiming_const *bittiming_const,
- *data_bittiming_const;
- struct can_bittiming bittiming, data_bittiming;
- const struct can_tdc_const *tdc_const;
- struct can_tdc tdc;
-
+ const struct can_bittiming_const *bittiming_const;
+ struct can_bittiming bittiming;
+ struct data_bittiming_params fd;
unsigned int bitrate_const_cnt;
const u32 *bitrate_const;
- const u32 *data_bitrate_const;
- unsigned int data_bitrate_const_cnt;
u32 bitrate_max;
struct can_clock clock;
@@ -77,14 +83,12 @@ struct can_priv {
struct delayed_work restart_work;
int (*do_set_bittiming)(struct net_device *dev);
- int (*do_set_data_bittiming)(struct net_device *dev);
int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
int (*do_set_termination)(struct net_device *dev, u16 term);
int (*do_get_state)(const struct net_device *dev,
enum can_state *state);
int (*do_get_berr_counter)(const struct net_device *dev,
struct can_berr_counter *bec);
- int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
};
static inline bool can_tdc_is_enabled(const struct can_priv *priv)
@@ -114,11 +118,11 @@ static inline bool can_tdc_is_enabled(const struct can_priv *priv)
*/
static inline s32 can_get_relative_tdco(const struct can_priv *priv)
{
- const struct can_bittiming *dbt = &priv->data_bittiming;
+ const struct can_bittiming *dbt = &priv->fd.data_bittiming;
s32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
dbt->phase_seg1) * dbt->brp;
- return (s32)priv->tdc.tdco - sample_point_in_tc;
+ return (s32)priv->fd.tdc.tdco - sample_point_in_tc;
}
/* helper to define static CAN controller features at device creation time */
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 02/14] can: netlink: replace tabulation by space in assignement
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 01/14] can: dev: add struct data_bittiming_params to group FD parameters Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 03/14] can: bittiming: rename CAN_CTRLMODE_TDC_MASK into CAN_CTRLMODE_FD_TDC_MASK Vincent Mailhol
` (13 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
commit cfd98c838cbe ("can: netlink: move '=' operators back to
previous line (checkpatch fix)") inadvertedly introduced a tabulation
between the IFLA_CAN_DATA_BITTIMING_CONST array index and the equal
sign.
Remove it.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 7455a7c5a383..df8b7ba68b6e 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -18,7 +18,7 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
[IFLA_CAN_CLOCK] = { .len = sizeof(struct can_clock) },
[IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct can_berr_counter) },
[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
- [IFLA_CAN_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
+ [IFLA_CAN_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
[IFLA_CAN_TDC] = { .type = NLA_NESTED },
[IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 03/14] can: bittiming: rename CAN_CTRLMODE_TDC_MASK into CAN_CTRLMODE_FD_TDC_MASK
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 01/14] can: dev: add struct data_bittiming_params to group FD parameters Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 02/14] can: netlink: replace tabulation by space in assignement Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 04/14] can: bittiming: rename can_tdc_is_enabled() into can_fd_tdc_is_enabled() Vincent Mailhol
` (12 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
With the introduction of CAN XL, a new CAN_CTRLMODE_XL_TDC_MASK will
be introduced later on. Because CAN_CTRLMODE_TDC_MASK is not part of
the uapi, rename it to CAN_CTRLMODE_FD_TDC_MASK to make it more
explicit that this mask is meant for CAN FD.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/calc_bittiming.c | 2 +-
drivers/net/can/dev/netlink.c | 12 ++++++------
include/linux/can/bittiming.h | 2 +-
include/linux/can/dev.h | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 3809c148fb88..a94bd67c670c 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -179,7 +179,7 @@ void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
return;
- *ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+ *ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
/* As specified in ISO 11898-1 section 11.3.3 "Transmitter
* delay compensation" (TDC) is only applicable if data BRP is
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index df8b7ba68b6e..72a60e8186aa 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -67,12 +67,12 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
if (data[IFLA_CAN_CTRLMODE]) {
struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
- u32 tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
+ u32 tdc_flags = cm->flags & CAN_CTRLMODE_FD_TDC_MASK;
is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
- if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
+ if (tdc_flags == CAN_CTRLMODE_FD_TDC_MASK)
return -EOPNOTSUPP;
/* If one of the CAN_CTRLMODE_TDC_* flag is set then
* TDC must be set and vice-versa
@@ -230,16 +230,16 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
dev->mtu = CAN_MTU;
memset(&priv->fd.data_bittiming, 0,
sizeof(priv->fd.data_bittiming));
- priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+ priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
}
- tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
+ tdc_mask = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually
* exclusive: make sure to turn the other one off
*/
if (tdc_mask)
- priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_TDC_MASK;
+ priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_FD_TDC_MASK;
}
if (data[IFLA_CAN_BITTIMING]) {
@@ -339,7 +339,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
err = can_tdc_changelink(priv, data[IFLA_CAN_TDC],
extack);
if (err) {
- priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+ priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
return err;
}
} else if (!tdc_mask) {
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 9b8a9c39614b..5dfdbb63b1d5 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -14,7 +14,7 @@
#define CAN_BITRATE_UNSET 0
#define CAN_BITRATE_UNKNOWN (-1U)
-#define CAN_CTRLMODE_TDC_MASK \
+#define CAN_CTRLMODE_FD_TDC_MASK \
(CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
/*
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 492d23bec7be..e492dfa8a472 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -93,7 +93,7 @@ struct can_priv {
static inline bool can_tdc_is_enabled(const struct can_priv *priv)
{
- return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK);
+ return !!(priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
}
/*
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 04/14] can: bittiming: rename can_tdc_is_enabled() into can_fd_tdc_is_enabled()
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (2 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 03/14] can: bittiming: rename CAN_CTRLMODE_TDC_MASK into CAN_CTRLMODE_FD_TDC_MASK Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 05/14] can: netlink: can_changelink(): rename tdc_mask into fd_tdc_flag_provided Vincent Mailhol
` (11 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
With the introduction of CAN XL, a new can_xl_tdc_is_enabled() helper
function will be introduced later on. Rename can_tdc_is_enabled() into
can_fd_tdc_is_enabled() to make it more explicit that this helper is
meant for CAN FD.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 6 +++---
drivers/net/can/usb/etas_es58x/es58x_fd.c | 2 +-
include/linux/can/dev.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 72a60e8186aa..27168aa6db20 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -144,7 +144,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
int err;
- if (!tdc_const || !can_tdc_is_enabled(priv))
+ if (!tdc_const || !can_fd_tdc_is_enabled(priv))
return -EOPNOTSUPP;
err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
@@ -409,7 +409,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
}
- if (can_tdc_is_enabled(priv)) {
+ if (can_fd_tdc_is_enabled(priv)) {
if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL ||
priv->fd.do_get_auto_tdcv)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
@@ -490,7 +490,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
goto err_cancel;
- if (can_tdc_is_enabled(priv)) {
+ if (can_fd_tdc_is_enabled(priv)) {
u32 tdcv;
int err = -EINVAL;
diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
index ab67fe0508ec..f169d764a1aa 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
@@ -429,7 +429,7 @@ static int es58x_fd_enable_channel(struct es58x_priv *priv)
es58x_fd_convert_bittiming(&tx_conf_msg.data_bittiming,
&priv->can.fd.data_bittiming);
- if (can_tdc_is_enabled(&priv->can)) {
+ if (can_fd_tdc_is_enabled(&priv->can)) {
tx_conf_msg.tdc_enabled = 1;
tx_conf_msg.tdco = cpu_to_le16(priv->can.fd.tdc.tdco);
tx_conf_msg.tdcf = cpu_to_le16(priv->can.fd.tdc.tdcf);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index e492dfa8a472..9a92cbe5b2cb 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -91,7 +91,7 @@ struct can_priv {
struct can_berr_counter *bec);
};
-static inline bool can_tdc_is_enabled(const struct can_priv *priv)
+static inline bool can_fd_tdc_is_enabled(const struct can_priv *priv)
{
return !!(priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
}
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 05/14] can: netlink: can_changelink(): rename tdc_mask into fd_tdc_flag_provided
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (3 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 04/14] can: bittiming: rename can_tdc_is_enabled() into can_fd_tdc_is_enabled() Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 06/14] can: netlink: make can_tdc_changelink() FD agnostic Vincent Mailhol
` (10 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
The only purpose of the tdc_mask variable is to check whether or not
any tdc flags (CAN_CTRLMODE_TDC_{AUTO,MANUAL}) were provided. At this
point, the actual value of the flags do no matter anymore because
these can be deduced from some other information.
Rename the tdc_mask variable into fd_tdc_flag_provided to make this
more explicit. Note that the fd_ prefix is added in preparation of the
introduction of CAN XL.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 27168aa6db20..f346b4208f1c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -189,7 +189,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
struct netlink_ext_ack *extack)
{
struct can_priv *priv = netdev_priv(dev);
- u32 tdc_mask = 0;
+ bool fd_tdc_flag_provided = false;
int err;
/* We need synchronization with dev->stop() */
@@ -234,11 +234,11 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
}
- tdc_mask = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
+ fd_tdc_flag_provided = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually
* exclusive: make sure to turn the other one off
*/
- if (tdc_mask)
+ if (fd_tdc_flag_provided)
priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_FD_TDC_MASK;
}
@@ -342,7 +342,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
return err;
}
- } else if (!tdc_mask) {
+ } else if (!fd_tdc_flag_provided) {
/* Neither of TDC parameters nor TDC flags are
* provided: do calculation
*/
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 06/14] can: netlink: make can_tdc_changelink() FD agnostic
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (4 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 05/14] can: netlink: can_changelink(): rename tdc_mask into fd_tdc_flag_provided Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 07/14] can: netlink: add can_dtb_changelink() Vincent Mailhol
` (9 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
This is a preparation patch for the introduction of CAN XL.
can_tdc_changelink() depends on some varibles which are specific to
CAN FD. Move these to the function parameters list so that, later on,
this function can be reused for the CAN XL TDC.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index f346b4208f1c..50c1658f17a4 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -136,15 +136,16 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
return 0;
}
-static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
+static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
+ bool tdc_is_enabled, const struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
struct can_tdc tdc = { 0 };
- const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
+ const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
int err;
- if (!tdc_const || !can_fd_tdc_is_enabled(priv))
+ if (!tdc_const || !tdc_is_enabled)
return -EOPNOTSUPP;
err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
@@ -179,7 +180,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
tdc.tdcf = tdcf;
}
- priv->fd.tdc = tdc;
+ dbt_params->tdc = tdc;
return 0;
}
@@ -336,8 +337,8 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
if (data[IFLA_CAN_TDC]) {
/* TDC parameters are provided: use them */
- err = can_tdc_changelink(priv, data[IFLA_CAN_TDC],
- extack);
+ err = can_tdc_changelink(&priv->fd, can_fd_tdc_is_enabled(priv),
+ data[IFLA_CAN_TDC], extack);
if (err) {
priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
return err;
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 07/14] can: netlink: add can_dtb_changelink()
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (5 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 06/14] can: netlink: make can_tdc_changelink() FD agnostic Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 08/14] can: netlink: add can_validate_tdc() Vincent Mailhol
` (8 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
Factorize the dattabittiming parsing out of can_changelink() and move
it in the new can_dtb_changelink() function. This is a preparation
patch for the introduction of CAN XL because the databittiming
changelink logic will be reused later on.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
I am not fully happy that can_dbt_changelink() requires 8 parameters.
I will probably revisit this later on. But for the moment, I think
this is acceptable for an RFC.
---
drivers/net/can/dev/netlink.c | 139 +++++++++++++++++++---------------
1 file changed, 78 insertions(+), 61 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 50c1658f17a4..354846e3a0d0 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -185,6 +185,77 @@ static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
return 0;
}
+static int can_dbt_changelink(struct net_device *dev,
+ struct nlattr *data_databittiming,
+ struct data_bittiming_params *dbt_params,
+ struct nlattr *data_tdc, bool tdc_flags_provided,
+ bool tdc_is_enabled, u32 tdc_mask,
+ struct netlink_ext_ack *extack)
+{
+ struct can_priv *priv = netdev_priv(dev);
+ struct can_bittiming dbt;
+ int err;
+
+ if (!data_databittiming)
+ return 0;
+
+ /* Do not allow changing bittiming while running */
+ if (dev->flags & IFF_UP)
+ return -EBUSY;
+
+ /* Calculate bittiming parameters based on data_bittiming_const
+ * if set, otherwise pass bitrate directly via do_set_bitrate().
+ * Bail out if neither is given.
+ */
+ if (!dbt_params->data_bittiming_const && !dbt_params->do_set_data_bittiming &&
+ !dbt_params->data_bitrate_const)
+ return -EOPNOTSUPP;
+
+ memcpy(&dbt, nla_data(data_databittiming), sizeof(dbt));
+ err = can_get_bittiming(dev, &dbt, dbt_params->data_bittiming_const,
+ dbt_params->data_bitrate_const,
+ dbt_params->data_bitrate_const_cnt, extack);
+ if (err)
+ return err;
+
+ if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "CAN data bitrate %u bps surpasses transceiver capabilities of %u bps",
+ dbt.bitrate, priv->bitrate_max);
+ return -EINVAL;
+ }
+
+ memset(&dbt_params->tdc, 0, sizeof(dbt_params->tdc));
+ if (data_tdc) {
+ /* TDC parameters are provided: use them */
+ err = can_tdc_changelink(dbt_params, tdc_is_enabled, data_tdc,
+ extack);
+ if (err) {
+ priv->ctrlmode &= ~tdc_mask;
+ return err;
+ }
+ } else if (!tdc_flags_provided) {
+ /* Neither of TDC parameters nor TDC flags are provided:
+ * do calculation
+ */
+ can_calc_tdco(&dbt_params->tdc, dbt_params->tdc_const, &dbt,
+ &priv->ctrlmode, priv->ctrlmode_supported);
+ } /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
+ * turned off. TDC is disabled: do nothing
+ */
+
+ memcpy(&dbt_params->data_bittiming, &dbt, sizeof(dbt));
+
+ if (dbt_params->do_set_data_bittiming) {
+ /* Finally, set the bit-timing registers */
+ err = dbt_params->do_set_data_bittiming(dev);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
static int can_changelink(struct net_device *dev, struct nlattr *tb[],
struct nlattr *data[],
struct netlink_ext_ack *extack)
@@ -301,67 +372,13 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
return err;
}
- if (data[IFLA_CAN_DATA_BITTIMING]) {
- struct can_bittiming dbt;
-
- /* Do not allow changing bittiming while running */
- if (dev->flags & IFF_UP)
- return -EBUSY;
-
- /* Calculate bittiming parameters based on
- * data_bittiming_const if set, otherwise pass bitrate
- * directly via do_set_bitrate(). Bail out if neither
- * is given.
- */
- if (!priv->fd.data_bittiming_const && !priv->fd.do_set_data_bittiming &&
- !priv->fd.data_bitrate_const)
- return -EOPNOTSUPP;
-
- memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
- sizeof(dbt));
- err = can_get_bittiming(dev, &dbt,
- priv->fd.data_bittiming_const,
- priv->fd.data_bitrate_const,
- priv->fd.data_bitrate_const_cnt,
- extack);
- if (err)
- return err;
-
- if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
- NL_SET_ERR_MSG_FMT(extack,
- "CANFD data bitrate %u bps surpasses transceiver capabilities of %u bps",
- dbt.bitrate, priv->bitrate_max);
- return -EINVAL;
- }
-
- memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
- if (data[IFLA_CAN_TDC]) {
- /* TDC parameters are provided: use them */
- err = can_tdc_changelink(&priv->fd, can_fd_tdc_is_enabled(priv),
- data[IFLA_CAN_TDC], extack);
- if (err) {
- priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
- return err;
- }
- } else if (!fd_tdc_flag_provided) {
- /* Neither of TDC parameters nor TDC flags are
- * provided: do calculation
- */
- can_calc_tdco(&priv->fd.tdc, priv->fd.tdc_const, &dbt,
- &priv->ctrlmode, priv->ctrlmode_supported);
- } /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
- * turned off. TDC is disabled: do nothing
- */
-
- memcpy(&priv->fd.data_bittiming, &dbt, sizeof(dbt));
-
- if (priv->fd.do_set_data_bittiming) {
- /* Finally, set the bit-timing registers */
- err = priv->fd.do_set_data_bittiming(dev);
- if (err)
- return err;
- }
- }
+ /* CAN FD */
+ err = can_dbt_changelink(dev, data[IFLA_CAN_DATA_BITTIMING], &priv->fd,
+ data[IFLA_CAN_TDC], fd_tdc_flag_provided,
+ can_fd_tdc_is_enabled(priv),
+ CAN_CTRLMODE_FD_TDC_MASK, extack);
+ if (err)
+ return err;
if (data[IFLA_CAN_TERMINATION]) {
const u16 termval = nla_get_u16(data[IFLA_CAN_TERMINATION]);
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 08/14] can: netlink: add can_validate_tdc()
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (6 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 07/14] can: netlink: add can_dtb_changelink() Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 09/14] can: netlink: make can_tdc_get_size() FD agnostic Vincent Mailhol
` (7 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
Factorize the TDC validation out of can_validate() and move it in the
new add can_validate_tdc() function. This is a preparation patch for
the introduction of CAN XL because this TDC validation will be reused
later on.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 81 +++++++++++++++++++++--------------
1 file changed, 48 insertions(+), 33 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 354846e3a0d0..511f721f4c80 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -49,6 +49,48 @@ static int can_validate_bittiming(const struct can_bittiming *bt,
return 0;
}
+static int can_validate_tdc(struct nlattr *data_tdc,
+ bool tdc_auto, bool tdc_manual,
+ struct netlink_ext_ack *extack)
+{
+ int err;
+
+ /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
+ if (tdc_auto == tdc_manual)
+ return -EOPNOTSUPP;
+
+ /* If one of the CAN_CTRLMODE_TDC_* flag is set then
+ * TDC must be set and vice-versa
+ */
+ if ((tdc_auto || tdc_manual) != !!data_tdc)
+ return -EOPNOTSUPP;
+
+ /* If providing TDC parameters, at least TDCO is needed. TDCV
+ * is needed if and only if CAN_CTRLMODE_TDC_MANUAL is set
+ */
+ if (!data_tdc) {
+ struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
+
+ err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX,
+ data_tdc, can_tdc_policy, extack);
+ if (err)
+ return err;
+
+ if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
+ if (tdc_auto)
+ return -EOPNOTSUPP;
+ } else {
+ if (tdc_manual)
+ return -EOPNOTSUPP;
+ }
+
+ if (!tb_tdc[IFLA_CAN_TDC_TDCO])
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
static int can_validate(struct nlattr *tb[], struct nlattr *data[],
struct netlink_ext_ack *extack)
{
@@ -67,42 +109,15 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
if (data[IFLA_CAN_CTRLMODE]) {
struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
- u32 tdc_flags = cm->flags & CAN_CTRLMODE_FD_TDC_MASK;
is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
- /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
- if (tdc_flags == CAN_CTRLMODE_FD_TDC_MASK)
- return -EOPNOTSUPP;
- /* If one of the CAN_CTRLMODE_TDC_* flag is set then
- * TDC must be set and vice-versa
- */
- if (!!tdc_flags != !!data[IFLA_CAN_TDC])
- return -EOPNOTSUPP;
- /* If providing TDC parameters, at least TDCO is
- * needed. TDCV is needed if and only if
- * CAN_CTRLMODE_TDC_MANUAL is set
- */
- if (data[IFLA_CAN_TDC]) {
- struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
-
- err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX,
- data[IFLA_CAN_TDC],
- can_tdc_policy, extack);
- if (err)
- return err;
-
- if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
- if (tdc_flags & CAN_CTRLMODE_TDC_AUTO)
- return -EOPNOTSUPP;
- } else {
- if (tdc_flags & CAN_CTRLMODE_TDC_MANUAL)
- return -EOPNOTSUPP;
- }
-
- if (!tb_tdc[IFLA_CAN_TDC_TDCO])
- return -EOPNOTSUPP;
- }
+ err = can_validate_tdc(data[IFLA_CAN_TDC],
+ cm->flags & CAN_CTRLMODE_TDC_AUTO,
+ cm->flags & CAN_CTRLMODE_TDC_MANUAL,
+ extack);
+ if (err)
+ return err;
}
if (data[IFLA_CAN_BITTIMING]) {
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 09/14] can: netlink: make can_tdc_get_size() FD agnostic
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (7 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 08/14] can: netlink: add can_validate_tdc() Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:55 ` [RFC PATCH 10/14] can: netlink: make can_tdc_fill_info() " Vincent Mailhol
` (6 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
This is a preparation patch for the introduction of CAN XL.
can_tdc_get_size() depends on some varibles which are specific to CAN
FD. Move these to the function parameters lest so that, later on, this
function can be reused for the CAN XL TDC.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 511f721f4c80..1809f5e53a75 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -422,32 +422,31 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
return 0;
}
-static size_t can_tdc_get_size(const struct net_device *dev)
+static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
+ bool tdc_is_enabled, bool tdc_manual)
{
- struct can_priv *priv = netdev_priv(dev);
size_t size;
- if (!priv->fd.tdc_const)
+ if (!dbt_params->tdc_const)
return 0;
size = nla_total_size(0); /* nest IFLA_CAN_TDC */
- if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL) {
+ if (tdc_manual) {
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MIN */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */
}
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MIN */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */
- if (priv->fd.tdc_const->tdcf_max) {
+ if (dbt_params->tdc_const->tdcf_max) {
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MIN */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */
}
- if (can_fd_tdc_is_enabled(priv)) {
- if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL ||
- priv->fd.do_get_auto_tdcv)
+ if (tdc_is_enabled) {
+ if (tdc_manual || dbt_params->do_get_auto_tdcv)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV */
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO */
- if (priv->fd.tdc_const->tdcf_max)
+ if (dbt_params->tdc_const->tdcf_max)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF */
}
@@ -491,7 +490,9 @@ static size_t can_get_size(const struct net_device *dev)
size += nla_total_size(sizeof(*priv->fd.data_bitrate_const) *
priv->fd.data_bitrate_const_cnt);
size += sizeof(priv->bitrate_max); /* IFLA_CAN_BITRATE_MAX */
- size += can_tdc_get_size(dev); /* IFLA_CAN_TDC */
+ size += can_tdc_get_size(&priv->fd, /* IFLA_CAN_TDC */
+ can_fd_tdc_is_enabled(priv),
+ priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL);
size += can_ctrlmode_ext_get_size(); /* IFLA_CAN_CTRLMODE_EXT */
return size;
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 10/14] can: netlink: make can_tdc_fill_info() FD agnostic
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (8 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 09/14] can: netlink: make can_tdc_get_size() FD agnostic Vincent Mailhol
@ 2024-11-10 15:55 ` Vincent Mailhol
2024-11-10 15:56 ` [RFC PATCH 11/14] can: netlink: document which symbols are FD specific Vincent Mailhol
` (5 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:55 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
This is a preparation patch for the introduction of CAN XL.
can_tdc_fill_info() depends on some varibles which are specific to CAN
FD. Move these to the function parameters list so that, later on, this
function can be reused for the CAN XL TDC.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 1809f5e53a75..6c3fa5aa22cf 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -498,12 +498,13 @@ static size_t can_get_size(const struct net_device *dev)
return size;
}
-static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
+static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
+ struct data_bittiming_params *dbt_params,
+ bool tdc_is_enabled, bool tdc_manual)
{
struct nlattr *nest;
- struct can_priv *priv = netdev_priv(dev);
- struct can_tdc *tdc = &priv->fd.tdc;
- const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
+ struct can_tdc *tdc = &dbt_params->tdc;
+ const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
if (!tdc_const)
return 0;
@@ -512,7 +513,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
if (!nest)
return -EMSGSIZE;
- if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL &&
+ if (tdc_manual &&
(nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max)))
goto err_cancel;
@@ -524,15 +525,15 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
goto err_cancel;
- if (can_fd_tdc_is_enabled(priv)) {
+ if (tdc_is_enabled) {
u32 tdcv;
int err = -EINVAL;
- if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+ if (tdc_manual) {
tdcv = tdc->tdcv;
err = 0;
- } else if (priv->fd.do_get_auto_tdcv) {
- err = priv->fd.do_get_auto_tdcv(dev, &tdcv);
+ } else if (dbt_params->do_get_auto_tdcv) {
+ err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
}
if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
goto err_cancel;
@@ -630,7 +631,8 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
sizeof(priv->bitrate_max),
&priv->bitrate_max)) ||
- can_tdc_fill_info(skb, dev) ||
+ can_tdc_fill_info(skb, dev, &priv->fd, can_fd_tdc_is_enabled(priv),
+ priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) ||
can_ctrlmode_ext_fill_info(skb, priv)
)
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 11/14] can: netlink: document which symbols are FD specific
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (9 preceding siblings ...)
2024-11-10 15:55 ` [RFC PATCH 10/14] can: netlink: make can_tdc_fill_info() " Vincent Mailhol
@ 2024-11-10 15:56 ` Vincent Mailhol
2024-11-10 15:56 ` [RFC PATCH 12/14] can: netlink: add CAN XL support Vincent Mailhol
` (4 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:56 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
The CAN XL netlink interface will also have data bitrate and TDC
parameters. The current FD parameters do not have a prefix in their
names to differentiate them.
Because the netlink interface is part of the UAPI, it is unfortunately
not feasible to rename the existing symbols to add an FD_ prefix. The
best alternative is to add a comment for each of the symbols to notify
the reader of which parts are CAN FD specific.
While at it, fix a typo: transiver -> transceiver.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
include/uapi/linux/can/netlink.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 02ec32d69474..ef62f56eaaef 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -101,8 +101,8 @@ struct can_ctrlmode {
#define CAN_CTRLMODE_PRESUME_ACK 0x40 /* Ignore missing CAN ACKs */
#define CAN_CTRLMODE_FD_NON_ISO 0x80 /* CAN FD in non-ISO mode */
#define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */
-#define CAN_CTRLMODE_TDC_AUTO 0x200 /* CAN transiver automatically calculates TDCV */
-#define CAN_CTRLMODE_TDC_MANUAL 0x400 /* TDCV is manually set up by user */
+#define CAN_CTRLMODE_TDC_AUTO 0x200 /* FD transceiver automatically calculates TDCV */
+#define CAN_CTRLMODE_TDC_MANUAL 0x400 /* FD TDCV is manually set up by user */
/*
* CAN device statistics
@@ -129,14 +129,14 @@ enum {
IFLA_CAN_RESTART_MS,
IFLA_CAN_RESTART,
IFLA_CAN_BERR_COUNTER,
- IFLA_CAN_DATA_BITTIMING,
- IFLA_CAN_DATA_BITTIMING_CONST,
+ IFLA_CAN_DATA_BITTIMING, /* FD */
+ IFLA_CAN_DATA_BITTIMING_CONST, /* FD */
IFLA_CAN_TERMINATION,
IFLA_CAN_TERMINATION_CONST,
IFLA_CAN_BITRATE_CONST,
- IFLA_CAN_DATA_BITRATE_CONST,
+ IFLA_CAN_DATA_BITRATE_CONST, /* FD */
IFLA_CAN_BITRATE_MAX,
- IFLA_CAN_TDC,
+ IFLA_CAN_TDC, /* FD */
IFLA_CAN_CTRLMODE_EXT,
/* add new constants above here */
@@ -145,7 +145,7 @@ enum {
};
/*
- * CAN FD Transmitter Delay Compensation (TDC)
+ * CAN FD/XL Transmitter Delay Compensation (TDC)
*
* Please refer to struct can_tdc_const and can_tdc in
* include/linux/can/bittiming.h for further details.
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (10 preceding siblings ...)
2024-11-10 15:56 ` [RFC PATCH 11/14] can: netlink: document which symbols are FD specific Vincent Mailhol
@ 2024-11-10 15:56 ` Vincent Mailhol
2024-11-12 8:09 ` Marc Kleine-Budde
2024-11-10 15:56 ` [RFC PATCH 13/14] can: netlink: add userland error messages Vincent Mailhol
` (3 subsequent siblings)
15 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:56 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
Add the netlink interface for CAN XL.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
include/linux/can/bittiming.h | 2 +
include/linux/can/dev.h | 13 ++++--
include/uapi/linux/can/netlink.h | 7 +++
4 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 6c3fa5aa22cf..3c89b304c5b8 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
[IFLA_CAN_TDC] = { .type = NLA_NESTED },
[IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
+ [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
+ [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
+ [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
};
static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
@@ -55,6 +58,10 @@ static int can_validate_tdc(struct nlattr *data_tdc,
{
int err;
+ /* TDC is optional */
+ if (!data_tdc)
+ return 0;
+
/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
if (tdc_auto == tdc_manual)
return -EOPNOTSUPP;
@@ -94,7 +101,7 @@ static int can_validate_tdc(struct nlattr *data_tdc,
static int can_validate(struct nlattr *tb[], struct nlattr *data[],
struct netlink_ext_ack *extack)
{
- bool is_can_fd = false;
+ bool is_can_fd = false, is_can_xl = false;
int err;
/* Make sure that valid CAN FD configurations always consist of
@@ -111,6 +118,7 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
+ is_can_xl = cm->flags & cm->mask & CAN_CTRLMODE_XL;
err = can_validate_tdc(data[IFLA_CAN_TDC],
cm->flags & CAN_CTRLMODE_TDC_AUTO,
@@ -133,11 +141,19 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
return -EOPNOTSUPP;
}
+ if (is_can_xl) {
+ if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_XL_DATA_BITTIMING])
+ return -EOPNOTSUPP;
+ }
if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
if (!is_can_fd)
return -EOPNOTSUPP;
}
+ if (data[IFLA_CAN_XL_DATA_BITTIMING] || data[IFLA_CAN_XL_TDC]) {
+ if (!is_can_xl)
+ return -EOPNOTSUPP;
+ }
if (data[IFLA_CAN_DATA_BITTIMING]) {
struct can_bittiming bt;
@@ -147,6 +163,14 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
if (err)
return err;
}
+ if (data[IFLA_CAN_XL_DATA_BITTIMING]) {
+ struct can_bittiming bt;
+
+ memcpy(&bt, nla_data(data[IFLA_CAN_XL_DATA_BITTIMING]), sizeof(bt));
+ err = can_validate_bittiming(&bt, extack);
+ if (err)
+ return err;
+ }
return 0;
}
@@ -275,8 +299,8 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
struct nlattr *data[],
struct netlink_ext_ack *extack)
{
+ bool fd_tdc_flag_provided = false, xl_tdc_flag_provided = false;
struct can_priv *priv = netdev_priv(dev);
- bool fd_tdc_flag_provided = false;
int err;
/* We need synchronization with dev->stop() */
@@ -310,8 +334,10 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->ctrlmode &= ~cm->mask;
priv->ctrlmode |= maskedflags;
- /* CAN_CTRLMODE_FD can only be set when driver supports FD */
- if (priv->ctrlmode & CAN_CTRLMODE_FD) {
+ /* CAN_CTRLMODE_{FD,XL} can only be set when driver supports FD/XL */
+ if (priv->ctrlmode & CAN_CTRLMODE_XL) {
+ dev->mtu = CANXL_MAX_MTU;
+ } else if (priv->ctrlmode & CAN_CTRLMODE_FD) {
dev->mtu = CANFD_MTU;
} else {
dev->mtu = CAN_MTU;
@@ -322,11 +348,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
}
fd_tdc_flag_provided = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
- /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually
+ xl_tdc_flag_provided = cm->mask & CAN_CTRLMODE_XL_TDC_MASK;
+ /* CAN_CTRLMODE_(XL_)TDC_{AUTO,MANUAL} are mutually
* exclusive: make sure to turn the other one off
*/
if (fd_tdc_flag_provided)
priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_FD_TDC_MASK;
+ if (xl_tdc_flag_provided)
+ priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_XL_TDC_MASK;
}
if (data[IFLA_CAN_BITTIMING]) {
@@ -395,6 +424,15 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
if (err)
return err;
+ /* CAN XL */
+ err = can_dbt_changelink(dev,
+ data[IFLA_CAN_XL_DATA_BITTIMING], &priv->xl,
+ data[IFLA_CAN_XL_TDC], xl_tdc_flag_provided,
+ can_xl_tdc_is_enabled(priv),
+ CAN_CTRLMODE_XL_TDC_MASK, extack);
+ if (err)
+ return err;
+
if (data[IFLA_CAN_TERMINATION]) {
const u16 termval = nla_get_u16(data[IFLA_CAN_TERMINATION]);
const unsigned int num_term = priv->termination_const_cnt;
@@ -494,6 +532,16 @@ static size_t can_get_size(const struct net_device *dev)
can_fd_tdc_is_enabled(priv),
priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL);
size += can_ctrlmode_ext_get_size(); /* IFLA_CAN_CTRLMODE_EXT */
+ if (priv->xl.data_bittiming.bitrate) /* IFLA_CAN_XL_DATA_BITTIMING */
+ size += nla_total_size(sizeof(struct can_bittiming));
+ if (priv->xl.data_bittiming_const) /* IFLA_CAN_XL_DATA_BITTIMING_CONST */
+ size += nla_total_size(sizeof(struct can_bittiming_const));
+ if (priv->xl.data_bitrate_const) /* IFLA_CAN_DATA_BITRATE_CONST */
+ size += nla_total_size(sizeof(*priv->xl.data_bitrate_const) *
+ priv->xl.data_bitrate_const_cnt);
+ size += can_tdc_get_size(&priv->xl, /* IFLA_CAN_XL_TDC */
+ can_xl_tdc_is_enabled(priv),
+ priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL);
return size;
}
@@ -634,7 +682,25 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
can_tdc_fill_info(skb, dev, &priv->fd, can_fd_tdc_is_enabled(priv),
priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) ||
- can_ctrlmode_ext_fill_info(skb, priv)
+ can_ctrlmode_ext_fill_info(skb, priv) ||
+
+ (priv->xl.data_bittiming.bitrate &&
+ nla_put(skb, IFLA_CAN_XL_DATA_BITTIMING,
+ sizeof(priv->xl.data_bittiming), &priv->xl.data_bittiming)) ||
+
+ (priv->xl.data_bittiming_const &&
+ nla_put(skb, IFLA_CAN_XL_DATA_BITTIMING_CONST,
+ sizeof(*priv->xl.data_bittiming_const),
+ priv->xl.data_bittiming_const)) ||
+
+ (priv->xl.data_bitrate_const &&
+ nla_put(skb, IFLA_CAN_XL_DATA_BITRATE_CONST,
+ sizeof(*priv->xl.data_bitrate_const) *
+ priv->xl.data_bitrate_const_cnt,
+ priv->xl.data_bitrate_const)) ||
+
+ can_tdc_fill_info(skb, dev, &priv->xl, can_xl_tdc_is_enabled(priv),
+ priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL)
)
return -EMSGSIZE;
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 5dfdbb63b1d5..2053b9dff0ad 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -16,6 +16,8 @@
#define CAN_CTRLMODE_FD_TDC_MASK \
(CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
+#define CAN_CTRLMODE_XL_TDC_MASK \
+ (CAN_CTRLMODE_XL_TDC_AUTO | CAN_CTRLMODE_XL_TDC_MANUAL)
/*
* struct can_tdc - CAN FD Transmission Delay Compensation parameters
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 9a92cbe5b2cb..1ac98914f351 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -58,7 +58,7 @@ struct can_priv {
const struct can_bittiming_const *bittiming_const;
struct can_bittiming bittiming;
- struct data_bittiming_params fd;
+ struct data_bittiming_params fd, xl;
unsigned int bitrate_const_cnt;
const u32 *bitrate_const;
u32 bitrate_max;
@@ -96,6 +96,11 @@ static inline bool can_fd_tdc_is_enabled(const struct can_priv *priv)
return !!(priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
}
+static inline bool can_xl_tdc_is_enabled(const struct can_priv *priv)
+{
+ return !!(priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MASK);
+}
+
/*
* can_get_relative_tdco() - TDCO relative to the sample point
*
@@ -116,13 +121,13 @@ static inline bool can_fd_tdc_is_enabled(const struct can_priv *priv)
* | |<->| relative TDCO
* |<------------- Secondary Sample Point ------------>|
*/
-static inline s32 can_get_relative_tdco(const struct can_priv *priv)
+static inline s32 can_get_relative_tdco(const struct data_bittiming_params *dbt_params)
{
- const struct can_bittiming *dbt = &priv->fd.data_bittiming;
+ const struct can_bittiming *dbt = &dbt_params->data_bittiming;
s32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
dbt->phase_seg1) * dbt->brp;
- return (s32)priv->fd.tdc.tdco - sample_point_in_tc;
+ return (s32)dbt_params->tdc.tdco - sample_point_in_tc;
}
/* helper to define static CAN controller features at device creation time */
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index ef62f56eaaef..c81fd153a07f 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -103,6 +103,9 @@ struct can_ctrlmode {
#define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */
#define CAN_CTRLMODE_TDC_AUTO 0x200 /* FD transceiver automatically calculates TDCV */
#define CAN_CTRLMODE_TDC_MANUAL 0x400 /* FD TDCV is manually set up by user */
+#define CAN_CTRLMODE_XL 0x800 /* CAN XL mode */
+#define CAN_CTRLMODE_XL_TDC_AUTO 0x200 /* XL transceiver automatically calculates TDCV */
+#define CAN_CTRLMODE_XL_TDC_MANUAL 0x400 /* XL TDCV is manually set up by user */
/*
* CAN device statistics
@@ -138,6 +141,10 @@ enum {
IFLA_CAN_BITRATE_MAX,
IFLA_CAN_TDC, /* FD */
IFLA_CAN_CTRLMODE_EXT,
+ IFLA_CAN_XL_DATA_BITTIMING,
+ IFLA_CAN_XL_DATA_BITTIMING_CONST,
+ IFLA_CAN_XL_DATA_BITRATE_CONST,
+ IFLA_CAN_XL_TDC,
/* add new constants above here */
__IFLA_CAN_MAX,
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-11-10 15:56 ` [RFC PATCH 12/14] can: netlink: add CAN XL support Vincent Mailhol
@ 2024-11-12 8:09 ` Marc Kleine-Budde
2024-11-12 8:31 ` Vincent Mailhol
0 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-11-12 8:09 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]
On 11.11.2024 00:56:01, Vincent Mailhol wrote:
> Add the netlink interface for CAN XL.
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
> include/linux/can/bittiming.h | 2 +
> include/linux/can/dev.h | 13 ++++--
> include/uapi/linux/can/netlink.h | 7 +++
> 4 files changed, 90 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index 6c3fa5aa22cf..3c89b304c5b8 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> [IFLA_CAN_TDC] = { .type = NLA_NESTED },
> [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
> + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
> + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
> + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
I haven't looked at the can_xl IP-core docs yet.
I don't want to pass "struct can_bittiming_const" via netlink to user
space. It's not sufficient to fully describe the CAN-FD controllers, as
tseg1_min cannot equally divided into prop_seg and phase_seg1.
Better make it a NLA_NESTED, as you did for the TDC.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-11-12 8:09 ` Marc Kleine-Budde
@ 2024-11-12 8:31 ` Vincent Mailhol
2024-11-12 8:41 ` Marc Kleine-Budde
2024-12-04 10:56 ` Oliver Hartkopp
0 siblings, 2 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-12 8:31 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 11.11.2024 00:56:01, Vincent Mailhol wrote:
> > Add the netlink interface for CAN XL.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
> > include/linux/can/bittiming.h | 2 +
> > include/linux/can/dev.h | 13 ++++--
> > include/uapi/linux/can/netlink.h | 7 +++
> > 4 files changed, 90 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > index 6c3fa5aa22cf..3c89b304c5b8 100644
> > --- a/drivers/net/can/dev/netlink.c
> > +++ b/drivers/net/can/dev/netlink.c
> > @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> > [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> > [IFLA_CAN_TDC] = { .type = NLA_NESTED },
> > [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
> > + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
> > + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
> > + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
>
> I haven't looked at the can_xl IP-core docs yet.
>
> I don't want to pass "struct can_bittiming_const" via netlink to user
> space. It's not sufficient to fully describe the CAN-FD controllers, as
> tseg1_min cannot equally divided into prop_seg and phase_seg1.
>
> Better make it a NLA_NESTED, as you did for the TDC.
I considered this point. The fact is that the code to handle the "struct
can_bittiming_const" already exists. And so here, I am left with two
choices:
- small code refactor and reuse the existing
- rewrite the full thing just for CAN XL and have two different ways
to handle the constant bittiming: one for Classical CAN and CAN FD
and the other for CAN XL.
For consistency, I chose the former approach which is the least
disruptive. If you want this nested, what about an in-between
solution:
IFLA_CAN_XL /* NLA_NESTED */
+ IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
+ IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
+ IFLA_CAN_TDC /* NLA_NESTED */
+ IFLA_CAN_TDC_TDCV_MIN
+ IFLA_CAN_TDC_TDCV_MAX
+ (all other TDC nested values)...
This way, we can still keep most of the current CAN(-FD) logic, and if
we want to add one value, we can add it as a standalone within the
IFLA_CAN_XL nest.
Also, the main reason for not creating the nest was that I thought
that the current bittiming API was stable. I was not aware of the
current flaw on how to divide tseg1_min. Maybe we should first discuss
how to solve this issue for CAN FD?
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-11-12 8:31 ` Vincent Mailhol
@ 2024-11-12 8:41 ` Marc Kleine-Budde
2024-12-04 10:56 ` Oliver Hartkopp
1 sibling, 0 replies; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-11-12 8:41 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]
On 12.11.2024 17:31:38, Vincent Mailhol wrote:
> On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 11.11.2024 00:56:01, Vincent Mailhol wrote:
> > > Add the netlink interface for CAN XL.
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > > drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
> > > include/linux/can/bittiming.h | 2 +
> > > include/linux/can/dev.h | 13 ++++--
> > > include/uapi/linux/can/netlink.h | 7 +++
> > > 4 files changed, 90 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > > index 6c3fa5aa22cf..3c89b304c5b8 100644
> > > --- a/drivers/net/can/dev/netlink.c
> > > +++ b/drivers/net/can/dev/netlink.c
> > > @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> > > [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> > > [IFLA_CAN_TDC] = { .type = NLA_NESTED },
> > > [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
> > > + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
> > > + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
> > > + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
> >
> > I haven't looked at the can_xl IP-core docs yet.
> >
> > I don't want to pass "struct can_bittiming_const" via netlink to user
> > space. It's not sufficient to fully describe the CAN-FD controllers, as
> > tseg1_min cannot equally divided into prop_seg and phase_seg1.
> >
> > Better make it a NLA_NESTED, as you did for the TDC.
>
> I considered this point. The fact is that the code to handle the "struct
> can_bittiming_const" already exists. And so here, I am left with two
> choices:
>
> - small code refactor and reuse the existing
>
> - rewrite the full thing just for CAN XL and have two different ways
> to handle the constant bittiming: one for Classical CAN and CAN FD
> and the other for CAN XL.
>
> For consistency, I chose the former approach which is the least
> disruptive. If you want this nested, what about an in-between
> solution:
>
> IFLA_CAN_XL /* NLA_NESTED */
> + IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
> + IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
> + IFLA_CAN_TDC /* NLA_NESTED */
> + IFLA_CAN_TDC_TDCV_MIN
> + IFLA_CAN_TDC_TDCV_MAX
> + (all other TDC nested values)...
>
> This way, we can still keep most of the current CAN(-FD) logic, and if
> we want to add one value, we can add it as a standalone within the
> IFLA_CAN_XL nest.
>
> Also, the main reason for not creating the nest was that I thought
> that the current bittiming API was stable. I was not aware of the
> current flaw on how to divide tseg1_min. Maybe we should first discuss
> how to solve this issue for CAN FD?
The bittiming API is stable. I thought of adding an additional nested
type to hold all relevant information. Luckily it's only from kernel ->
user space. So we can pass both, the old struct can_bittiming_const and
the new nested type and the user space can decide which one to use.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-11-12 8:31 ` Vincent Mailhol
2024-11-12 8:41 ` Marc Kleine-Budde
@ 2024-12-04 10:56 ` Oliver Hartkopp
2024-12-04 11:15 ` Marc Kleine-Budde
1 sibling, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-04 10:56 UTC (permalink / raw)
To: Vincent Mailhol, Marc Kleine-Budde; +Cc: linux-can, Robert Nawrath
On 12.11.24 09:31, Vincent Mailhol wrote:
> On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 11.11.2024 00:56:01, Vincent Mailhol wrote:
>>> Add the netlink interface for CAN XL.
>>>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> ---
>>> drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
>>> include/linux/can/bittiming.h | 2 +
>>> include/linux/can/dev.h | 13 ++++--
>>> include/uapi/linux/can/netlink.h | 7 +++
>>> 4 files changed, 90 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
>>> index 6c3fa5aa22cf..3c89b304c5b8 100644
>>> --- a/drivers/net/can/dev/netlink.c
>>> +++ b/drivers/net/can/dev/netlink.c
>>> @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>>> [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
>>> [IFLA_CAN_TDC] = { .type = NLA_NESTED },
>>> [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
>>> + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
>>> + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
>>> + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
>>
>> I haven't looked at the can_xl IP-core docs yet.
>>
>> I don't want to pass "struct can_bittiming_const" via netlink to user
>> space. It's not sufficient to fully describe the CAN-FD controllers, as
>> tseg1_min cannot equally divided into prop_seg and phase_seg1.
>>
>> Better make it a NLA_NESTED, as you did for the TDC.
>
> I considered this point. The fact is that the code to handle the "struct
> can_bittiming_const" already exists. And so here, I am left with two
> choices:
>
> - small code refactor and reuse the existing
>
> - rewrite the full thing just for CAN XL and have two different ways
> to handle the constant bittiming: one for Classical CAN and CAN FD
> and the other for CAN XL.
>
> For consistency, I chose the former approach which is the least
> disruptive. If you want this nested, what about an in-between
> solution:
>
> IFLA_CAN_XL /* NLA_NESTED */
> + IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
> + IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
> + IFLA_CAN_TDC /* NLA_NESTED */
> + IFLA_CAN_TDC_TDCV_MIN
> + IFLA_CAN_TDC_TDCV_MAX
> + (all other TDC nested values)...
>
> This way, we can still keep most of the current CAN(-FD) logic, and if
> we want to add one value, we can add it as a standalone within the
> IFLA_CAN_XL nest.
>
> Also, the main reason for not creating the nest was that I thought
> that the current bittiming API was stable. I was not aware of the
> current flaw on how to divide tseg1_min. Maybe we should first discuss
> how to solve this issue for CAN FD?
I like the current way how you added the CAN XL support.
It maintains the known usage pattern - and the way how CAN XL bit
timings are defined is identical to CAN FD (including TDC).
Is the separation of propseg and tseg1 that relevant?
Does it really need to be exposed to the user?
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-04 10:56 ` Oliver Hartkopp
@ 2024-12-04 11:15 ` Marc Kleine-Budde
2024-12-04 11:35 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-12-04 11:15 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 4143 bytes --]
On 04.12.2024 11:56:02, Oliver Hartkopp wrote:
>
>
> On 12.11.24 09:31, Vincent Mailhol wrote:
> > On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 11.11.2024 00:56:01, Vincent Mailhol wrote:
> > > > Add the netlink interface for CAN XL.
> > > >
> > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > ---
> > > > drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
> > > > include/linux/can/bittiming.h | 2 +
> > > > include/linux/can/dev.h | 13 ++++--
> > > > include/uapi/linux/can/netlink.h | 7 +++
> > > > 4 files changed, 90 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > > > index 6c3fa5aa22cf..3c89b304c5b8 100644
> > > > --- a/drivers/net/can/dev/netlink.c
> > > > +++ b/drivers/net/can/dev/netlink.c
> > > > @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> > > > [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> > > > [IFLA_CAN_TDC] = { .type = NLA_NESTED },
> > > > [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
> > > > + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
> > > > + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
> > > > + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
> > >
> > > I haven't looked at the can_xl IP-core docs yet.
> > >
> > > I don't want to pass "struct can_bittiming_const" via netlink to user
> > > space. It's not sufficient to fully describe the CAN-FD controllers, as
> > > tseg1_min cannot equally divided into prop_seg and phase_seg1.
> > >
> > > Better make it a NLA_NESTED, as you did for the TDC.
> >
> > I considered this point. The fact is that the code to handle the "struct
> > can_bittiming_const" already exists. And so here, I am left with two
> > choices:
> >
> > - small code refactor and reuse the existing
> >
> > - rewrite the full thing just for CAN XL and have two different ways
> > to handle the constant bittiming: one for Classical CAN and CAN FD
> > and the other for CAN XL.
> >
> > For consistency, I chose the former approach which is the least
> > disruptive. If you want this nested, what about an in-between
> > solution:
> >
> > IFLA_CAN_XL /* NLA_NESTED */
> > + IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
> > + IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
> > + IFLA_CAN_TDC /* NLA_NESTED */
> > + IFLA_CAN_TDC_TDCV_MIN
> > + IFLA_CAN_TDC_TDCV_MAX
> > + (all other TDC nested values)...
> >
> > This way, we can still keep most of the current CAN(-FD) logic, and if
> > we want to add one value, we can add it as a standalone within the
> > IFLA_CAN_XL nest.
> >
> > Also, the main reason for not creating the nest was that I thought
> > that the current bittiming API was stable. I was not aware of the
> > current flaw on how to divide tseg1_min. Maybe we should first discuss
> > how to solve this issue for CAN FD?
>
> I like the current way how you added the CAN XL support.
> It maintains the known usage pattern - and the way how CAN XL bit timings
> are defined is identical to CAN FD (including TDC).
>
> Is the separation of propseg and tseg1 that relevant?
> Does it really need to be exposed to the user?
There are IIRC at least 2 CAN-FD cores where the prop segment and phase
segment 1 for the data bit timing have not the same width. This means we
have to change the bittiming_const in the kernel.
A struct in netlink means we cannot change it. It makes IMHO no sense to
me to add any new structs into netlink, especially if we know the
bittiming struct is going to change.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-04 11:15 ` Marc Kleine-Budde
@ 2024-12-04 11:35 ` Oliver Hartkopp
2024-12-04 11:44 ` Marc Kleine-Budde
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-04 11:35 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
On 04.12.24 12:15, Marc Kleine-Budde wrote:
> On 04.12.2024 11:56:02, Oliver Hartkopp wrote:
>>
>>
>> On 12.11.24 09:31, Vincent Mailhol wrote:
>>> On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>> On 11.11.2024 00:56:01, Vincent Mailhol wrote:
>>>>> Add the netlink interface for CAN XL.
>>>>>
>>>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>>> ---
>>>>> drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
>>>>> include/linux/can/bittiming.h | 2 +
>>>>> include/linux/can/dev.h | 13 ++++--
>>>>> include/uapi/linux/can/netlink.h | 7 +++
>>>>> 4 files changed, 90 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
>>>>> index 6c3fa5aa22cf..3c89b304c5b8 100644
>>>>> --- a/drivers/net/can/dev/netlink.c
>>>>> +++ b/drivers/net/can/dev/netlink.c
>>>>> @@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>>>>> [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
>>>>> [IFLA_CAN_TDC] = { .type = NLA_NESTED },
>>>>> [IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
>>>>> + [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
>>>>> + [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
>>>>> + [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
>>>>
>>>> I haven't looked at the can_xl IP-core docs yet.
>>>>
>>>> I don't want to pass "struct can_bittiming_const" via netlink to user
>>>> space. It's not sufficient to fully describe the CAN-FD controllers, as
>>>> tseg1_min cannot equally divided into prop_seg and phase_seg1.
>>>>
>>>> Better make it a NLA_NESTED, as you did for the TDC.
>>>
>>> I considered this point. The fact is that the code to handle the "struct
>>> can_bittiming_const" already exists. And so here, I am left with two
>>> choices:
>>>
>>> - small code refactor and reuse the existing
>>>
>>> - rewrite the full thing just for CAN XL and have two different ways
>>> to handle the constant bittiming: one for Classical CAN and CAN FD
>>> and the other for CAN XL.
>>>
>>> For consistency, I chose the former approach which is the least
>>> disruptive. If you want this nested, what about an in-between
>>> solution:
>>>
>>> IFLA_CAN_XL /* NLA_NESTED */
>>> + IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
>>> + IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
>>> + IFLA_CAN_TDC /* NLA_NESTED */
>>> + IFLA_CAN_TDC_TDCV_MIN
>>> + IFLA_CAN_TDC_TDCV_MAX
>>> + (all other TDC nested values)...
>>>
>>> This way, we can still keep most of the current CAN(-FD) logic, and if
>>> we want to add one value, we can add it as a standalone within the
>>> IFLA_CAN_XL nest.
>>>
>>> Also, the main reason for not creating the nest was that I thought
>>> that the current bittiming API was stable. I was not aware of the
>>> current flaw on how to divide tseg1_min. Maybe we should first discuss
>>> how to solve this issue for CAN FD?
>>
>> I like the current way how you added the CAN XL support.
>> It maintains the known usage pattern - and the way how CAN XL bit timings
>> are defined is identical to CAN FD (including TDC).
>>
>> Is the separation of propseg and tseg1 that relevant?
>> Does it really need to be exposed to the user?
>
> There are IIRC at least 2 CAN-FD cores where the prop segment and phase
> segment 1 for the data bit timing have not the same width. This means we
> have to change the bittiming_const in the kernel.
>
> A struct in netlink means we cannot change it.
But are we not already in this situation with CAN FD that we can not
change the bittiming (const) in the userspace API?
Best regards,
Oliver
> It makes IMHO no sense to
> me to add any new structs into netlink, especially if we know the
> bittiming struct is going to change.
>
> regards,
> Marc
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-04 11:35 ` Oliver Hartkopp
@ 2024-12-04 11:44 ` Marc Kleine-Budde
2024-12-05 8:16 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-12-04 11:44 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]
On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
> > > > Also, the main reason for not creating the nest was that I thought
> > > > that the current bittiming API was stable. I was not aware of the
> > > > current flaw on how to divide tseg1_min. Maybe we should first discuss
> > > > how to solve this issue for CAN FD?
> > >
> > > I like the current way how you added the CAN XL support.
> > > It maintains the known usage pattern - and the way how CAN XL bit timings
> > > are defined is identical to CAN FD (including TDC).
> > >
> > > Is the separation of propseg and tseg1 that relevant?
> > > Does it really need to be exposed to the user?
> >
> > There are IIRC at least 2 CAN-FD cores where the prop segment and phase
> > segment 1 for the data bit timing have not the same width. This means we
> > have to change the bittiming_const in the kernel.
> >
> > A struct in netlink means we cannot change it.
>
> But are we not already in this situation with CAN FD that we can not change
> the bittiming (const) in the userspace API?
Yes, we have to support it. But we can add a new nested type that
serializes the individual members of an improved struct bittiming_const.
The old user space tools will just keep working, iproute2 can be updated
to use the new bittiming_const if it's available and fall back to the
existing one.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-04 11:44 ` Marc Kleine-Budde
@ 2024-12-05 8:16 ` Oliver Hartkopp
2024-12-05 9:15 ` Marc Kleine-Budde
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-05 8:16 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol; +Cc: linux-can, Robert Nawrath
On 04.12.24 12:44, Marc Kleine-Budde wrote:
> On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
>>>>> Also, the main reason for not creating the nest was that I thought
>>>>> that the current bittiming API was stable. I was not aware of the
>>>>> current flaw on how to divide tseg1_min. Maybe we should first discuss
>>>>> how to solve this issue for CAN FD?
>>>>
>>>> I like the current way how you added the CAN XL support.
>>>> It maintains the known usage pattern - and the way how CAN XL bit timings
>>>> are defined is identical to CAN FD (including TDC).
>>>>
>>>> Is the separation of propseg and tseg1 that relevant?
>>>> Does it really need to be exposed to the user?
>>>
>>> There are IIRC at least 2 CAN-FD cores where the prop segment and phase
>>> segment 1 for the data bit timing have not the same width. This means we
>>> have to change the bittiming_const in the kernel.
Sure?
In the end (almost) every CAN controller has the tseg1 register which
contains prop_seg + phase_seg1 as a sum of these.
The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure
these "2 CAN-FD cores" will add the values internally too.
I'm a bit concerned that after 40 years someone shows up with the idea
to spend two registers for the tseg1 value instead of one.
As a both values rely on the same tq can't we just split the tseg1 into
prop_seg + phase_seg1 values with some common e.g. 70:30 pattern?
IMO changing the bittiming API has no value for the user just to satisfy
the "2 CAN-FD cores" that came late to the party.
>>> A struct in netlink means we cannot change it.
>>
>> But are we not already in this situation with CAN FD that we can not change
>> the bittiming (const) in the userspace API?
>
> Yes, we have to support it. But we can add a new nested type that
> serializes the individual members of an improved struct bittiming_const.
> The old user space tools will just keep working, iproute2 can be updated
> to use the new bittiming_const if it's available and fall back to the
> existing one.
Ok. Nice - but maybe obsolete due to my question above ;-)
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-05 8:16 ` Oliver Hartkopp
@ 2024-12-05 9:15 ` Marc Kleine-Budde
2024-12-09 13:13 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-12-05 9:15 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 3890 bytes --]
On 05.12.2024 09:16:44, Oliver Hartkopp wrote:
> On 04.12.24 12:44, Marc Kleine-Budde wrote:
> > On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
> > > > > > Also, the main reason for not creating the nest was that I thought
> > > > > > that the current bittiming API was stable. I was not aware of the
> > > > > > current flaw on how to divide tseg1_min. Maybe we should first discuss
> > > > > > how to solve this issue for CAN FD?
> > > > >
> > > > > I like the current way how you added the CAN XL support.
> > > > > It maintains the known usage pattern - and the way how CAN XL bit timings
> > > > > are defined is identical to CAN FD (including TDC).
> > > > >
> > > > > Is the separation of propseg and tseg1 that relevant?
> > > > > Does it really need to be exposed to the user?
> > > >
> > > > There are IIRC at least 2 CAN-FD cores where the prop segment and phase
> > > > segment 1 for the data bit timing have not the same width. This means we
> > > > have to change the bittiming_const in the kernel.
>
> Sure?
I'm sure the registers don't have the same width. And I'm sure about my
conclusion, but that's up for discussion :)
https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L197
https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/flexcan/flexcan-core.c#L1210
> In the end (almost) every CAN controller has the tseg1 register which
> contains prop_seg + phase_seg1 as a sum of these.
Some do (just a short grep): bxcan, esdacc, rcar_can, softing, hi311x,
ti_hecc. More controllers haven evenly divided prop_seg + phase_seg1.
> The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure these
> "2 CAN-FD cores" will add the values internally too.
As the ctucanfd is open you can have a look :)
> I'm a bit concerned that after 40 years someone shows up with the idea to
> spend two registers for the tseg1 value instead of one.
It doesn't matter if prop_seg and phase_seg1 are in the same register or
not, what matters is:
a) 1. does the IP core want separate prop_seg and phase_seg1 values
- or -
2. does the IP core want a single "prop_seg + phase_seg1", a.k.a.
tseg1 value?
b) 1. what's the width of the prop_seg and phase_seg1?
2. what's the width of tseg1?
Currently the CAN infrastructure allows the driver to specify tseg1 only
and assumes the width of prop_seg and phase_seg1 to be the same, as it
distributes tseg1 evenly between prop_seg and phase_seg1:
https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/dev/calc_bittiming.c#L155
This leads to the workarounds in the CAN drivers, see above for links.
> As a both values rely on the same tq can't we just split the tseg1 into
> prop_seg + phase_seg1 values with some common e.g. 70:30 pattern?
We currently split 50:50 (hard-coded).
> IMO changing the bittiming API has no value for the user just to satisfy the
> "2 CAN-FD cores" that came late to the party.
>
> > > > A struct in netlink means we cannot change it.
> > >
> > > But are we not already in this situation with CAN FD that we can not change
> > > the bittiming (const) in the userspace API?
> >
> > Yes, we have to support it. But we can add a new nested type that
> > serializes the individual members of an improved struct bittiming_const.
> > The old user space tools will just keep working, iproute2 can be updated
> > to use the new bittiming_const if it's available and fall back to the
> > existing one.
>
> Ok. Nice - but maybe obsolete due to my question above ;-)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-05 9:15 ` Marc Kleine-Budde
@ 2024-12-09 13:13 ` Oliver Hartkopp
2024-12-10 11:58 ` Marc Kleine-Budde
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-09 13:13 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
On 05.12.24 10:15, Marc Kleine-Budde wrote:
> On 05.12.2024 09:16:44, Oliver Hartkopp wrote:
>> On 04.12.24 12:44, Marc Kleine-Budde wrote:
>>> On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
>>>>>>> Also, the main reason for not creating the nest was that I thought
>>>>>>> that the current bittiming API was stable. I was not aware of the
>>>>>>> current flaw on how to divide tseg1_min. Maybe we should first discuss
>>>>>>> how to solve this issue for CAN FD?
>>>>>>
>>>>>> I like the current way how you added the CAN XL support.
>>>>>> It maintains the known usage pattern - and the way how CAN XL bit timings
>>>>>> are defined is identical to CAN FD (including TDC).
>>>>>>
>>>>>> Is the separation of propseg and tseg1 that relevant?
>>>>>> Does it really need to be exposed to the user?
>>>>>
>>>>> There are IIRC at least 2 CAN-FD cores where the prop segment and phase
>>>>> segment 1 for the data bit timing have not the same width. This means we
>>>>> have to change the bittiming_const in the kernel.
>>
>> Sure?
>
> I'm sure the registers don't have the same width. And I'm sure about my
> conclusion, but that's up for discussion :)
>
> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L197
> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/flexcan/flexcan-core.c#L1210
>
>> In the end (almost) every CAN controller has the tseg1 register which
>> contains prop_seg + phase_seg1 as a sum of these.
>
> Some do (just a short grep): bxcan, esdacc, rcar_can, softing, hi311x,
> ti_hecc. More controllers haven evenly divided prop_seg + phase_seg1.
>
>> The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure these
>> "2 CAN-FD cores" will add the values internally too.
>
> As the ctucanfd is open you can have a look :)
>
>> I'm a bit concerned that after 40 years someone shows up with the idea to
>> spend two registers for the tseg1 value instead of one.
>
> It doesn't matter if prop_seg and phase_seg1 are in the same register or
> not, what matters is:
> a) 1. does the IP core want separate prop_seg and phase_seg1 values
> - or -
> 2. does the IP core want a single "prop_seg + phase_seg1", a.k.a.
> tseg1 value?
> b) 1. what's the width of the prop_seg and phase_seg1?
> 2. what's the width of tseg1?
>
> Currently the CAN infrastructure allows the driver to specify tseg1 only
> and assumes the width of prop_seg and phase_seg1 to be the same, as it
> distributes tseg1 evenly between prop_seg and phase_seg1:
>
> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/dev/calc_bittiming.c#L155
>
> This leads to the workarounds in the CAN drivers, see above for links.
Yes. But why don't we just let this as-is then?
Even if prop_seg phase_seg1 registers have a different size, this split
up can be done easily without changing the current bittiming API.
Maybe a common helper function to split up the values based on given
register sizes could simplify the handling for those CAN drivers.
I'm still not convinced that it brings some benefits for the user to
extend the bittiming API. IMHO it just complicates the bitrate settings.
Best regards,
Oliver
>> As a both values rely on the same tq can't we just split the tseg1 into
>> prop_seg + phase_seg1 values with some common e.g. 70:30 pattern?
>
> We currently split 50:50 (hard-coded).
>
>> IMO changing the bittiming API has no value for the user just to satisfy the
>> "2 CAN-FD cores" that came late to the party.
>>
>>>>> A struct in netlink means we cannot change it.
>>>>
>>>> But are we not already in this situation with CAN FD that we can not change
>>>> the bittiming (const) in the userspace API?
>>>
>>> Yes, we have to support it. But we can add a new nested type that
>>> serializes the individual members of an improved struct bittiming_const.
>>> The old user space tools will just keep working, iproute2 can be updated
>>> to use the new bittiming_const if it's available and fall back to the
>>> existing one.
>>
>> Ok. Nice - but maybe obsolete due to my question above ;-)
>
> regards,
> Marc
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-09 13:13 ` Oliver Hartkopp
@ 2024-12-10 11:58 ` Marc Kleine-Budde
2024-12-15 8:05 ` Vincent Mailhol
0 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-12-10 11:58 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Vincent Mailhol, linux-can, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 4237 bytes --]
On 09.12.2024 14:13:29, Oliver Hartkopp wrote:
>
>
> On 05.12.24 10:15, Marc Kleine-Budde wrote:
> > On 05.12.2024 09:16:44, Oliver Hartkopp wrote:
> > > On 04.12.24 12:44, Marc Kleine-Budde wrote:
> > > > On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
> > > > > > > > Also, the main reason for not creating the nest was that I thought
> > > > > > > > that the current bittiming API was stable. I was not aware of the
> > > > > > > > current flaw on how to divide tseg1_min. Maybe we should first discuss
> > > > > > > > how to solve this issue for CAN FD?
> > > > > > >
> > > > > > > I like the current way how you added the CAN XL support.
> > > > > > > It maintains the known usage pattern - and the way how CAN XL bit timings
> > > > > > > are defined is identical to CAN FD (including TDC).
> > > > > > >
> > > > > > > Is the separation of propseg and tseg1 that relevant?
> > > > > > > Does it really need to be exposed to the user?
> > > > > >
> > > > > > There are IIRC at least 2 CAN-FD cores where the prop segment and phase
> > > > > > segment 1 for the data bit timing have not the same width. This means we
> > > > > > have to change the bittiming_const in the kernel.
> > >
> > > Sure?
> >
> > I'm sure the registers don't have the same width. And I'm sure about my
> > conclusion, but that's up for discussion :)
> >
> > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L197
> > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/flexcan/flexcan-core.c#L1210
> >
> > > In the end (almost) every CAN controller has the tseg1 register which
> > > contains prop_seg + phase_seg1 as a sum of these.
> >
> > Some do (just a short grep): bxcan, esdacc, rcar_can, softing, hi311x,
> > ti_hecc. More controllers haven evenly divided prop_seg + phase_seg1.
> >
> > > The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure these
> > > "2 CAN-FD cores" will add the values internally too.
> >
> > As the ctucanfd is open you can have a look :)
As far as I understand, it internally adds sync + prop + phase1:
https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/src/prescaler/bit_time_cfg_capture.vhd?ref_type=heads#L242
> > > I'm a bit concerned that after 40 years someone shows up with the idea to
> > > spend two registers for the tseg1 value instead of one.
> >
> > It doesn't matter if prop_seg and phase_seg1 are in the same register or
> > not, what matters is:
> > a) 1. does the IP core want separate prop_seg and phase_seg1 values
> > - or -
> > 2. does the IP core want a single "prop_seg + phase_seg1", a.k.a.
> > tseg1 value?
> > b) 1. what's the width of the prop_seg and phase_seg1?
> > 2. what's the width of tseg1?
> >
> > Currently the CAN infrastructure allows the driver to specify tseg1 only
> > and assumes the width of prop_seg and phase_seg1 to be the same, as it
> > distributes tseg1 evenly between prop_seg and phase_seg1:
> >
> > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/dev/calc_bittiming.c#L155
> >
> > This leads to the workarounds in the CAN drivers, see above for links.
>
> Yes. But why don't we just let this as-is then?
>
> Even if prop_seg phase_seg1 registers have a different size, this split up
> can be done easily without changing the current bittiming API.
>
> Maybe a common helper function to split up the values based on given
> register sizes could simplify the handling for those CAN drivers.
Good idea!
What about adding the information about prop_seg and phase_seg1 to
bittiming_const and let the can_calc_bittiming() calculate it?
> I'm still not convinced that it brings some benefits for the user to extend
> the bittiming API. IMHO it just complicates the bitrate settings.
The benefit is, that the user knows about the limitation of prop_seg and
phase_seg1.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 12/14] can: netlink: add CAN XL support
2024-12-10 11:58 ` Marc Kleine-Budde
@ 2024-12-15 8:05 ` Vincent Mailhol
0 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-12-15 8:05 UTC (permalink / raw)
To: Marc Kleine-Budde, Oliver Hartkopp; +Cc: linux-can, Robert Nawrath
On 10/12/2024 at 20:58, Marc Kleine-Budde wrote:
> On 09.12.2024 14:13:29, Oliver Hartkopp wrote:
>>
>>
>> On 05.12.24 10:15, Marc Kleine-Budde wrote:
>>> On 05.12.2024 09:16:44, Oliver Hartkopp wrote:
>>>> On 04.12.24 12:44, Marc Kleine-Budde wrote:
>>>>> On 04.12.2024 12:35:43, Oliver Hartkopp wrote:
>>>>>>>>> Also, the main reason for not creating the nest was that I thought
>>>>>>>>> that the current bittiming API was stable. I was not aware of the
>>>>>>>>> current flaw on how to divide tseg1_min. Maybe we should first discuss
>>>>>>>>> how to solve this issue for CAN FD?
>>>>>>>>
>>>>>>>> I like the current way how you added the CAN XL support.
>>>>>>>> It maintains the known usage pattern - and the way how CAN XL bit timings
>>>>>>>> are defined is identical to CAN FD (including TDC).
>>>>>>>>
>>>>>>>> Is the separation of propseg and tseg1 that relevant?
>>>>>>>> Does it really need to be exposed to the user?
>>>>>>>
>>>>>>> There are IIRC at least 2 CAN-FD cores where the prop segment and phase
>>>>>>> segment 1 for the data bit timing have not the same width. This means we
>>>>>>> have to change the bittiming_const in the kernel.
>>>>
>>>> Sure?
>>>
>>> I'm sure the registers don't have the same width. And I'm sure about my
>>> conclusion, but that's up for discussion :)
>>>
>>> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L197
>>> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/flexcan/flexcan-core.c#L1210
>>>
>>>> In the end (almost) every CAN controller has the tseg1 register which
>>>> contains prop_seg + phase_seg1 as a sum of these.
>>>
>>> Some do (just a short grep): bxcan, esdacc, rcar_can, softing, hi311x,
>>> ti_hecc. More controllers haven evenly divided prop_seg + phase_seg1.
>>>
>>>> The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure these
>>>> "2 CAN-FD cores" will add the values internally too.
>>>
>>> As the ctucanfd is open you can have a look :)
>
> As far as I understand, it internally adds sync + prop + phase1:
>
> https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/src/prescaler/bit_time_cfg_capture.vhd?ref_type=heads#L242
>
>>>> I'm a bit concerned that after 40 years someone shows up with the idea to
>>>> spend two registers for the tseg1 value instead of one.
>>>
>>> It doesn't matter if prop_seg and phase_seg1 are in the same register or
>>> not, what matters is:
>>> a) 1. does the IP core want separate prop_seg and phase_seg1 values
>>> - or -
>>> 2. does the IP core want a single "prop_seg + phase_seg1", a.k.a.
>>> tseg1 value?
>>> b) 1. what's the width of the prop_seg and phase_seg1?
>>> 2. what's the width of tseg1?
>>>
>>> Currently the CAN infrastructure allows the driver to specify tseg1 only
>>> and assumes the width of prop_seg and phase_seg1 to be the same, as it
>>> distributes tseg1 evenly between prop_seg and phase_seg1:
>>>
>>> https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/dev/calc_bittiming.c#L155
>>>
>>> This leads to the workarounds in the CAN drivers, see above for links.
>>
>> Yes. But why don't we just let this as-is then?
>>
>> Even if prop_seg phase_seg1 registers have a different size, this split up
>> can be done easily without changing the current bittiming API.
>>
>> Maybe a common helper function to split up the values based on given
>> register sizes could simplify the handling for those CAN drivers.
>
> Good idea!
>
> What about adding the information about prop_seg and phase_seg1 to
> bittiming_const and let the can_calc_bittiming() calculate it?
>
>> I'm still not convinced that it brings some benefits for the user to extend
>> the bittiming API. IMHO it just complicates the bitrate settings.
>
> The benefit is, that the user knows about the limitation of prop_seg and
> phase_seg1.
I finally caught up with this thread. As I said before I was divided on
this topic two weeks ago, and still I am today.
One part of me tells me that if the ISO mandates the prop_seg and the
phase_seg1 as two different configurable variables, then so shall it be.
The other part of me tells me that the benefit is small. The transceiver
does not need to know the tseg1 split to figure out the sample point. So
this is only useful for the user.
One thing I am sure of: I am against deprecating the struct
can_bittiming_const and creating a new nested structure with all the
configuration ranges. I do not want to have two branches of code: one
for the legacy struct can_bittiming_const, one for the new nest. It will
needlessly complicate the logic.
If we want to add the prop_seg to the configuration, the best is to just
reuse what we have and only add an extension for what is missing:
/*
* IFLA_CAN_BITTIMING_CONST_EXT nest: additional bit-timing constants
* Extension of the struct can_bittiming_const.
*/
enum {
IFLA_CAN_BITTIMING_CONST_PROP_SEG_MAX, /* u32 */
/* add new constants above here */
__IFLA_CAN_BITTIMING_CONST,
IFLA_CAN_BITTIMING_CONST_MAX = __IFLA_CAN_BITTIMING_CONST - 1
};
I purposely omitted IFLA_CAN_BITTIMING_CONST_PROP_SEG_MIN, because the
standard already specifies that this minimum value is one.
The CAN netlink interface then gets three additional entries:
IFLA_CAN_CC_BITRATE_CONST_EXT,
IFLA_CAN_FD_BITRATE_CONST_EXT,
IFLA_CAN_XL_BITRATE_CONST_EXT,
all pointing the the new IFLA_CAN_BITTIMING_CONST_EXT nest.
Finally, we create a new *internal* structure (i.e. not visible from
uapi) that will replace the previous one in can_priv.
/*
* CAN internal bit-timing constant
*
* Used for calculating and checking bit-timing parameters,
* extended from struct can_bittiming_const.
*/
struct can_bittiming_const_ext {
char name[16]; /* Name of the CAN controller hardware */
u32 tseg1_min; /* Time segment 1 = prop_seg + phase_seg1 */
u32 tseg1_max;
u32 tseg2_min; /* Time segment 2 = phase_seg2 */
u32 tseg2_max;
u32 sjw_max; /* Synchronisation jump width */
u32 brp_min; /* Bit-rate prescaler */
u32 brp_max;
u32 brp_inc;
u32 prop_seg_max; /* 0: not set, legacy behaviour */
};
The first fields up to the brp_inc matches the struct
can_bittiming_const, the last field, prop_seg_max, matches the
IFLA_CAN_BITTIMING_CONST_EXT nest. If prop_seg_max is zero, we keep the
old behaviour: 50/50 split.
This gives the same results for less work and all of the Classical CAN,
CAN FD and CAN XL will use the same logic. And biggest point: no need to
deprecate any existing code.
Note that with this approach, it does not matter if we do it before or
after the introduction on CAN XL.
@Marc, I am fine to do the netlink part if you are willing to the
bittiming and the driver part. Deal?
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread
* [RFC PATCH 13/14] can: netlink: add userland error messages
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (11 preceding siblings ...)
2024-11-10 15:56 ` [RFC PATCH 12/14] can: netlink: add CAN XL support Vincent Mailhol
@ 2024-11-10 15:56 ` Vincent Mailhol
2024-11-10 15:56 ` [RFC PATCH 14/14] !!! DO NOT MERGE !!! can: add dummyxl driver Vincent Mailhol
` (2 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:56 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
Use NL_SET_ERR_MSG_FMT() to return meaningfull error messages to the
userland whenever the validation of the CAN netlink arguments fails.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
See this one as a WIP. I wrote this quickly. I will revisit and
rephrase when dropping the RFC tag of this series.
Also, do not try to imagine that I wrote this because I forgot how to
provide to can bittiming arguments to the ip tool. This patch is only
meant to help the newcommers, nothing else :D
---
drivers/net/can/dev/netlink.c | 40 ++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 3c89b304c5b8..0a6700194ab0 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -124,8 +124,11 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
cm->flags & CAN_CTRLMODE_TDC_AUTO,
cm->flags & CAN_CTRLMODE_TDC_MANUAL,
extack);
- if (err)
+ if (err) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "TDC parameters are incorrect");
return err;
+ }
}
if (data[IFLA_CAN_BITTIMING]) {
@@ -133,26 +136,41 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
err = can_validate_bittiming(&bt, extack);
- if (err)
+ if (err) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "Nominal bittiming parameters are incorrect");
return err;
+ }
}
if (is_can_fd) {
- if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
+ if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING]) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "Provide both nominal and FD data bittiming");
return -EOPNOTSUPP;
+ }
}
if (is_can_xl) {
- if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_XL_DATA_BITTIMING])
+ if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_XL_DATA_BITTIMING]) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "Provide both nominal and XL data bittiming");
return -EOPNOTSUPP;
+ }
}
if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
- if (!is_can_fd)
+ if (!is_can_fd) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "CAN FD is required to use FD data bittiming or FD TDC");
return -EOPNOTSUPP;
+ }
}
if (data[IFLA_CAN_XL_DATA_BITTIMING] || data[IFLA_CAN_XL_TDC]) {
- if (!is_can_xl)
+ if (!is_can_xl) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "CAN XL is required to use XL data bittiming or XL TDC");
return -EOPNOTSUPP;
+ }
}
if (data[IFLA_CAN_DATA_BITTIMING]) {
@@ -160,16 +178,22 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]), sizeof(bt));
err = can_validate_bittiming(&bt, extack);
- if (err)
+ if (err) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "FD data bittiming parameters are incorrect");
return err;
+ }
}
if (data[IFLA_CAN_XL_DATA_BITTIMING]) {
struct can_bittiming bt;
memcpy(&bt, nla_data(data[IFLA_CAN_XL_DATA_BITTIMING]), sizeof(bt));
err = can_validate_bittiming(&bt, extack);
- if (err)
+ if (err) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "FD data bittiming parameters are incorrect");
return err;
+ }
}
return 0;
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [RFC PATCH 14/14] !!! DO NOT MERGE !!! can: add dummyxl driver
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (12 preceding siblings ...)
2024-11-10 15:56 ` [RFC PATCH 13/14] can: netlink: add userland error messages Vincent Mailhol
@ 2024-11-10 15:56 ` Vincent Mailhol
2024-11-11 14:08 ` [RFC PATCH 00/14] can: netlink: add CAN XL Oliver Hartkopp
2024-11-12 8:53 ` Marc Kleine-Budde
15 siblings, 0 replies; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-10 15:56 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Oliver Hartkopp
Cc: Robert Nawrath, Vincent Mailhol
A dummy driver to test the CAN XL netlink.
I wrote this in less than one hour. I will drop this from the series
once this is not an RFC anymore.
To test:
1. Apply this series, compile it and load the driver. Example:
modprobe dummyxl
From now on, I will assume that the can device name is
can0. Adjust below commands accordingly to your setup.
2. Apply the other series to the iproute2-next [1].
3. Using the compiled ip tool, send the CAN bittiming and set the
interface up. Example:
./ip/ip link set can0 up type can bitrate 500000 fd on dbitrate 2000000 xl on xl-dbitrate 8000000
4. The kernel message log should show the bittiming parameters of
the dummyxl driver. Like that:
can0: CAN CC nominal bittiming:
can0: bitrate: 500000
can0: sample_point: 875
can0: tq: 12
can0: prop_seg: 69
can0: phase_seg1: 70
can0: phase_seg2: 20
can0: sjw: 10
can0: brp: 1
can0:
can0: CAN FD databittiming:
can0: bitrate: 2000000
can0: sample_point: 750
can0: tq: 12
can0: prop_seg: 14
can0: phase_seg1: 15
can0: phase_seg2: 10
can0: sjw: 5
can0: brp: 1
can0: CAN FD TDC:
can0: tdcv: 0
can0: tdco: 30
can0: tdcf: 0
can0:
can0: CAN XL databittiming:
can0: bitrate: 8000000
can0: sample_point: 700
can0: tq: 12
can0: prop_seg: 3
can0: phase_seg1: 3
can0: phase_seg2: 3
can0: sjw: 1
can0: brp: 1
can0: CAN XL TDC:
can0: tdcv: 0
can0: tdco: 7
can0: tdcf: 0
can0:
can0: dummyxl is up
5. Use a recent version of can-utils [2] to generate some CAN
traffic. For example:
cangen -m can0
should generate a mix of Classical CAN, CAN FD and CAN XL
frames. The dummyxl drivers should then print in the kernel log
that those frames were well received.
Voila, with this, you are now able to send CAN XL frames to a CAN XL
driver.
[1] https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git
[2] https://github.com/linux-can/can-utils
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/Kconfig | 8 ++
drivers/net/can/Makefile | 1 +
drivers/net/can/dummyxl.c | 224 ++++++++++++++++++++++++++++++++++++++
3 files changed, 233 insertions(+)
create mode 100644 drivers/net/can/dummyxl.c
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index cf989bea9aa3..a8bc5f4e752f 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -124,6 +124,14 @@ config CAN_CAN327
If this driver is built as a module, it will be called can327.
+config CAN_DUMMY_XL
+ tristate "Dummy CAN XL"
+ help
+ A dummy module just to check the CAN XL netlink interface. Do not
+ merge this.
+
+ If this driver is built as a module, it will be called dummyxl.
+
config CAN_FLEXCAN
tristate "Support for Freescale FLEXCAN based chips"
depends on OF || COLDFIRE || COMPILE_TEST
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index a71db2cfe990..ff8e00fa26e3 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_CAN_CAN327) += can327.o
obj-$(CONFIG_CAN_CC770) += cc770/
obj-$(CONFIG_CAN_C_CAN) += c_can/
obj-$(CONFIG_CAN_CTUCANFD) += ctucanfd/
+obj-$(CONFIG_CAN_DUMMY_XL) += dummyxl.o
obj-$(CONFIG_CAN_FLEXCAN) += flexcan/
obj-$(CONFIG_CAN_GRCAN) += grcan.o
obj-$(CONFIG_CAN_IFI_CANFD) += ifi_canfd/
diff --git a/drivers/net/can/dummyxl.c b/drivers/net/can/dummyxl.c
new file mode 100644
index 000000000000..9f6a2991b0ec
--- /dev/null
+++ b/drivers/net/can/dummyxl.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Copyright (c) 2024 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */
+
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/units.h>
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/skb.h>
+
+struct dummyxl {
+ struct can_priv can;
+ struct net_device *dev;
+};
+
+static struct dummyxl *dummyxl;
+
+static const struct can_bittiming_const dummyxl_bittiming_const = {
+ .name = "dummyxl nominal",
+ .tseg1_min = 2,
+ .tseg1_max = 256,
+ .tseg2_min = 2,
+ .tseg2_max = 128,
+ .sjw_max = 128,
+ .brp_min = 1,
+ .brp_max = 512,
+ .brp_inc = 1
+};
+
+static const struct can_bittiming_const dummyxl_fd_databittiming_const = {
+ .name = "dummyxl FD",
+ .tseg1_min = 2,
+ .tseg1_max = 256,
+ .tseg2_min = 2,
+ .tseg2_max = 128,
+ .sjw_max = 128,
+ .brp_min = 1,
+ .brp_max = 512,
+ .brp_inc = 1
+};
+
+static const struct can_tdc_const dummyxl_fd_tdc_const = {
+ .tdcv_min = 0,
+ .tdcv_max = 0, /* Manual mode not supported. */
+ .tdco_min = 0,
+ .tdco_max = 127,
+ .tdcf_min = 0,
+ .tdcf_max = 127
+};
+
+static const struct can_bittiming_const dummyxl_xl_databittiming_const = {
+ .name = "dummyxl XL",
+ .tseg1_min = 2,
+ .tseg1_max = 256,
+ .tseg2_min = 2,
+ .tseg2_max = 128,
+ .sjw_max = 128,
+ .brp_min = 1,
+ .brp_max = 512,
+ .brp_inc = 1
+};
+
+static const struct can_tdc_const dummyxl_xl_tdc_const = {
+ .tdcv_min = 0,
+ .tdcv_max = 0, /* Manual mode not supported. */
+ .tdco_min = 0,
+ .tdco_max = 127,
+ .tdcf_min = 0,
+ .tdcf_max = 127
+};
+
+static void dummyxl_print_bittiming(struct net_device *dev, struct can_bittiming *bt)
+{
+ netdev_info(dev, "\tbitrate: %u\n", bt->bitrate);
+ netdev_info(dev, "\tsample_point: %u\n", bt->sample_point);
+ netdev_info(dev, "\ttq: %u\n", bt->tq);
+ netdev_info(dev, "\tprop_seg: %u\n", bt->prop_seg);
+ netdev_info(dev, "\tphase_seg1: %u\n", bt->phase_seg1);
+ netdev_info(dev, "\tphase_seg2: %u\n", bt->phase_seg2);
+ netdev_info(dev, "\tsjw: %u\n", bt->sjw);
+ netdev_info(dev, "\tbrp: %u\n", bt->brp);
+}
+
+static void dummyxl_print_tdc(struct net_device *dev, struct can_tdc *tdc)
+{
+ netdev_info(dev, "\t\ttdcv: %u\n", tdc->tdcv);
+ netdev_info(dev, "\t\ttdco: %u\n", tdc->tdco);
+ netdev_info(dev, "\t\ttdcf: %u\n", tdc->tdcf);
+}
+
+static int dummyxl_netdev_open(struct net_device *dev)
+{
+ struct dummyxl *priv = netdev_priv(dev);
+ struct can_priv *can_priv = &priv->can;
+ int ret;
+
+ netdev_info(dev, "CAN CC nominal bittiming:\n");
+ dummyxl_print_bittiming(dev, &can_priv->bittiming);
+ netdev_info(dev, "\n");
+
+ if (can_priv->ctrlmode & CAN_CTRLMODE_FD) {
+ netdev_info(dev, "CAN FD databittiming:\n");
+ dummyxl_print_bittiming(dev, &can_priv->fd.data_bittiming);
+ if (can_fd_tdc_is_enabled(can_priv)) {
+ netdev_info(dev, "\tCAN FD TDC:\n");
+ dummyxl_print_tdc(dev, &can_priv->fd.tdc);
+ } else {
+ netdev_info(dev, "\tCAN FD TDC is off\n");
+ }
+ } else {
+ netdev_info(dev, "CAN FD is off\n");
+ }
+ netdev_info(dev, "\n");
+
+ if (can_priv->ctrlmode & CAN_CTRLMODE_XL) {
+ netdev_info(dev, "CAN XL databittiming:\n");
+ dummyxl_print_bittiming(dev, &can_priv->xl.data_bittiming);
+ if (can_xl_tdc_is_enabled(can_priv)) {
+ netdev_info(dev, "\tCAN XL TDC:\n");
+ dummyxl_print_tdc(dev, &can_priv->xl.tdc);
+ } else {
+ netdev_info(dev, "\tCAN XL TDC is off\n");
+ }
+ } else {
+ netdev_info(dev, "CAN XL is off\n");
+ }
+ netdev_info(dev, "\n");
+
+ ret = open_candev(dev);
+ if (ret)
+ return ret;
+ netif_start_queue(dev);
+ netdev_info(dev, "dummyxl is up\n");
+
+ return 0;
+}
+
+static int dummyxl_netdev_close(struct net_device *dev)
+{
+ netif_stop_queue(dev);
+ close_candev(dev);
+ netdev_info(dev, "dummyxl is down\n");
+
+ return 0;
+}
+
+static netdev_tx_t dummyxl_start_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ if (can_is_canxl_skb(skb))
+ netdev_info(dev, "Received CAN XL skb\n");
+ else if (can_is_canfd_skb(skb))
+ netdev_info(dev, "Received CAN FD skb\n");
+ else if (can_is_can_skb(skb))
+ netdev_info(dev, "Received Classical CAN skb\n");
+ else
+ netdev_info(dev, "Received an odd skb?!\n");
+ kfree_skb(skb);
+ dev->stats.tx_dropped++;
+
+ return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops dummyxl_netdev_ops = {
+ .ndo_open = dummyxl_netdev_open,
+ .ndo_stop = dummyxl_netdev_close,
+ .ndo_start_xmit = dummyxl_start_xmit,
+};
+
+static int __init dummyxl_init(void)
+{
+ struct net_device *dev;
+ struct dummyxl *priv;
+ int ret;
+
+ dev = alloc_candev(sizeof(struct dummyxl), 0);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->netdev_ops = &dummyxl_netdev_ops;
+ priv = netdev_priv(dev);
+ priv->can.bittiming_const = &dummyxl_bittiming_const;
+ priv->can.bitrate_max = 8 * MEGA /* BPS */;
+ priv->can.clock.freq = 80 * MEGA /* Hz */;
+ priv->can.fd.data_bittiming_const = &dummyxl_fd_databittiming_const;
+ priv->can.fd.tdc_const = &dummyxl_fd_tdc_const;
+ priv->can.xl.data_bittiming_const = &dummyxl_xl_databittiming_const;
+ priv->can.xl.tdc_const = &dummyxl_xl_tdc_const;
+ priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_FD | CAN_CTRLMODE_TDC_AUTO |
+ CAN_CTRLMODE_XL | CAN_CTRLMODE_XL_TDC_AUTO;
+ priv->dev = dev;
+
+ ret = register_candev(priv->dev);
+ if (ret) {
+ free_candev(priv->dev);
+ return ret;
+ }
+
+ dummyxl = priv;
+ netdev_info(dev, "dummyxl OK\n");
+
+ return 0;
+}
+
+static void __exit dummyxl_exit(void)
+{
+ struct net_device *dev = dummyxl->dev;
+
+ netdev_info(dev, "dummyxl bye bye\n");
+ unregister_candev(dev);
+ free_candev(dev);
+}
+
+module_init(dummyxl_init);
+module_exit(dummyxl_exit);
+
+MODULE_DESCRIPTION("A dummy module just to check the CAN XL netlink interface");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Vincent Mailhol <mailhol.vincent@wanadoo.fr>");
--
2.45.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (13 preceding siblings ...)
2024-11-10 15:56 ` [RFC PATCH 14/14] !!! DO NOT MERGE !!! can: add dummyxl driver Vincent Mailhol
@ 2024-11-11 14:08 ` Oliver Hartkopp
2024-11-11 15:17 ` Vincent Mailhol
2024-11-12 8:53 ` Marc Kleine-Budde
15 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-11-11 14:08 UTC (permalink / raw)
To: Vincent Mailhol, linux-can, Marc Kleine-Budde; +Cc: Robert Nawrath
Hello Vincent!
Very impressive! Thanks for the effort.
Together with the example in the dummyxl driver it should become quite
easy to integrate the netlink API into my XCANB driver hack for testing.
Picking up the dummyxl driver I wonder if it would make sense to
mainline this driver probably as can_nltest driver?!?
Of course this driver should be disabled or combined with some kernel
testing Kconfig stuff. But is it a great testing tool.
From what I can see with the bitrate and tdc configurations the
extension for CAN XL is ok.
If you take a look at this manual
https://github.com/linux-can/can-doc/blob/master/x_can/xcan_user_manual_v350.pdf
on page 268/304 you will find the PWM configuration which consists of
three values with 6 bits each. I assume this to be similar in all CAN XL
controllers.
The PWM feature switches the physical layer for a CAN XL transceiver in
the CAN XL data phase. This is a weird feature to do some PWM on the
controllers' TX data pin to be able to switch the physical layer while
maintaining the CAN transceiver package with 8 pin layout.
Additionally to this PWM configuration register, the PWM CAN XL
transceiver switch feature has to be enabled similar to the way we
enable 'fd on' or 'xl on' today.
You can see this bit called XLTR in the Operating Mode section on page
269/304 and 270/304 .
E.g. that might be named 'xltrx [on|off]' (default off)
Best regards,
Oliver
On 10.11.24 16:55, Vincent Mailhol wrote:
> Because of popular request [1] and to celebrate the fact that I joined
> the kernel web of trust this week [2], here is an RFC to introduce the
> CAN XL netlink support.
>
> The logic is simple. The CAN XL basically just reuse the same kind of
> parameters as CAN FD: the databittiming and the TDC. So the series is
> just a bunch of refactor to either:
>
> - factorize code.
> - rename some variable to differentiate between CAN FD and XL.
> - make some function more generic to accept both CAN FD and XL.
>
> The feature is working™: there is a dummy driver at the end of the
> series to show the traffic from the userland to a driver. This said, I
> did close to zero testing. Once I had one CAN XL frame reaching the
> driver, I call it a day, and decided to send the work. Regardless, it
> is Sunday night. If I do not send it now, that would be next week-end.
> Probably some mistakes are hidden here and there, but this should be
> enough for an RFC level.
>
> Also, I am not fully happy that can_dbt_changelink() requires 8
> parameters. I will probably revisit this later on. But for the moment,
> I think this is acceptable for an RFC.
>
> Overall, I do not want to rush this series. Linus should send the rc7
> anytime soon, and the merge window will probably start in eight days.
> I do not think this series will be finalized by then. I still need to
> give a deeper look at ISO 11898-1:2024 [3] to check that everything is
> good. However, if I receive positive feedback on this RFC, I would
> probably like to have the first patch merged so that I do not have to
> rebase that tree wide patch each time someone makes a change.
>
> I will send a second RFC series for iplink2 just after this one. Stay
> tuned!
>
> [1] https://lore.kernel.org/linux-can/CAEQ16vpxthctdrpv0kBKEZJA8VNYffjGGPBGBY93RmKDD49bAQ@mail.gmail.com/
> [2] https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/commit/?id=81d335d3586c
> [3] https://www.iso.org/fr/standard/86384.html
>
> Vincent Mailhol (14):
> can: dev: add struct data_bittiming_params to group FD parameters
> can: netlink: replace tabulation by space in assignement
> can: bittiming: rename CAN_CTRLMODE_TDC_MASK into
> CAN_CTRLMODE_FD_TDC_MASK
> can: bittiming: rename can_tdc_is_enabled() into
> can_fd_tdc_is_enabled()
> can: netlink: can_changelink(): rename tdc_mask into
> fd_tdc_flag_provided
> can: netlink: make can_tdc_changelink() FD agnostic
> can: netlink: add can_dtb_changelink()
> can: netlink: add can_validate_tdc()
> can: netlink: make can_tdc_get_size() FD agnostic
> can: netlink: make can_tdc_fill_info() FD agnostic
> can: netlink: document which symbols are FD specific
> can: netlink: add CAN XL support
> can: netlink: add userland error messages
> !!! DO NOT MERGE !!! can: add dummyxl driver
>
> drivers/net/can/Kconfig | 8 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/ctucanfd/ctucanfd_base.c | 8 +-
> drivers/net/can/dev/calc_bittiming.c | 2 +-
> drivers/net/can/dev/dev.c | 12 +-
> drivers/net/can/dev/netlink.c | 422 ++++++++++++------
> drivers/net/can/dummyxl.c | 224 ++++++++++
> drivers/net/can/flexcan/flexcan-core.c | 4 +-
> drivers/net/can/ifi_canfd/ifi_canfd.c | 10 +-
> drivers/net/can/kvaser_pciefd.c | 6 +-
> drivers/net/can/m_can/m_can.c | 8 +-
> drivers/net/can/peak_canfd/peak_canfd.c | 6 +-
> drivers/net/can/rcar/rcar_canfd.c | 4 +-
> .../net/can/rockchip/rockchip_canfd-core.c | 4 +-
> .../can/rockchip/rockchip_canfd-timestamp.c | 2 +-
> .../net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +-
> drivers/net/can/usb/esd_usb.c | 6 +-
> drivers/net/can/usb/etas_es58x/es58x_core.c | 4 +-
> drivers/net/can/usb/etas_es58x/es58x_fd.c | 8 +-
> drivers/net/can/usb/gs_usb.c | 8 +-
> drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 2 +-
> .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 6 +-
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 6 +-
> drivers/net/can/xilinx_can.c | 6 +-
> include/linux/can/bittiming.h | 4 +-
> include/linux/can/dev.h | 39 +-
> include/uapi/linux/can/netlink.h | 21 +-
> 27 files changed, 606 insertions(+), 229 deletions(-)
> create mode 100644 drivers/net/can/dummyxl.c
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-11 14:08 ` [RFC PATCH 00/14] can: netlink: add CAN XL Oliver Hartkopp
@ 2024-11-11 15:17 ` Vincent Mailhol
2024-11-11 15:32 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-11 15:17 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can, Marc Kleine-Budde; +Cc: Robert Nawrath
On 11/11/2024 at 23:08, Oliver Hartkopp wrote:
> Hello Vincent!
>
> Very impressive! Thanks for the effort.
> Together with the example in the dummyxl driver it should become quite
> easy to integrate the netlink API into my XCANB driver hack for testing.
>
> Picking up the dummyxl driver I wonder if it would make sense to
> mainline this driver probably as can_nltest driver?!?
> Of course this driver should be disabled or combined with some kernel
> testing Kconfig stuff. But is it a great testing tool.
Thanks. I do not recall similar things in the netdev subtree. I wonder
what would be the idiomatic way to introduce such netlink test driver.
@Marc, any thoughts on this?
> From what I can see with the bitrate and tdc configurations the
> extension for CAN XL is ok.
>
> If you take a look at this manual
>
> https://github.com/linux-can/can-doc/blob/master/x_can/xcan_user_manual_v350.pdf
>
>
> on page 268/304 you will find the PWM configuration which consists of
> three values with 6 bits each. I assume this to be similar in all CAN
> XL controllers.
>
> The PWM feature switches the physical layer for a CAN XL transceiver
> in the CAN XL data phase. This is a weird feature to do some PWM on
> the controllers' TX data pin to be able to switch the physical layer
> while maintaining the CAN transceiver package with 8 pin layout.
>
> Additionally to this PWM configuration register, the PWM CAN XL
> transceiver switch feature has to be enabled similar to the way we
> enable 'fd on' or 'xl on' today.
>
> You can see this bit called XLTR in the Operating Mode section on page
> 269/304 and 270/304 .
>
> E.g. that might be named 'xltrx [on|off]' (default off)
Thanks for the information and the link to the documentation. I will
also try to see what ISO 11898-1:2024 has to say on the topic. Just be
patient on this and do not worry if I give no signal for a couple weeks.
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-11 15:17 ` Vincent Mailhol
@ 2024-11-11 15:32 ` Oliver Hartkopp
2024-11-21 20:10 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-11-11 15:32 UTC (permalink / raw)
To: Vincent Mailhol, linux-can, Marc Kleine-Budde; +Cc: Robert Nawrath
On 11.11.24 16:17, Vincent Mailhol wrote:
> On 11/11/2024 at 23:08, Oliver Hartkopp wrote:
>> Hello Vincent!
>>
>> Very impressive! Thanks for the effort.
>> Together with the example in the dummyxl driver it should become quite
>> easy to integrate the netlink API into my XCANB driver hack for testing.
>>
>> Picking up the dummyxl driver I wonder if it would make sense to
>> mainline this driver probably as can_nltest driver?!?
>> Of course this driver should be disabled or combined with some kernel
>> testing Kconfig stuff. But is it a great testing tool.
>
> Thanks. I do not recall similar things in the netdev subtree. I wonder
> what would be the idiomatic way to introduce such netlink test driver.
>
> @Marc, any thoughts on this?
>
>> From what I can see with the bitrate and tdc configurations the
>> extension for CAN XL is ok.
>>
>> If you take a look at this manual
>>
>> https://github.com/linux-can/can-doc/blob/master/x_can/
>> xcan_user_manual_v350.pdf
>>
>> on page 268/304 you will find the PWM configuration which consists of
>> three values with 6 bits each. I assume this to be similar in all CAN
>> XL controllers.
>>
>> The PWM feature switches the physical layer for a CAN XL transceiver
>> in the CAN XL data phase. This is a weird feature to do some PWM on
>> the controllers' TX data pin to be able to switch the physical layer
>> while maintaining the CAN transceiver package with 8 pin layout.
>>
>> Additionally to this PWM configuration register, the PWM CAN XL
>> transceiver switch feature has to be enabled similar to the way we
>> enable 'fd on' or 'xl on' today.
>>
>> You can see this bit called XLTR in the Operating Mode section on page
>> 269/304 and 270/304 .
>>
>> E.g. that might be named 'xltrx [on|off]' (default off)
> Thanks for the information and the link to the documentation. I will
> also try to see what ISO 11898-1:2024 has to say on the topic. Just be
> patient on this and do not worry if I give no signal for a couple weeks.
No problem! I will give some feedback when I managed to integrate the
extended netlink API to my driver.
Maybe Robert also gets his hands on this, so that we can continue to
discuss things even when you are busy for some time.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-11 15:32 ` Oliver Hartkopp
@ 2024-11-21 20:10 ` Oliver Hartkopp
2024-12-01 11:32 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-11-21 20:10 UTC (permalink / raw)
To: Vincent Mailhol, linux-can, Marc Kleine-Budde; +Cc: Robert Nawrath
Hi Vincent,
On 11.11.24 16:32, Oliver Hartkopp wrote:
> No problem! I will give some feedback when I managed to integrate the
> extended netlink API to my driver.
I managed to set up my CAN XL dev board with the latest Linux kernel
(6.13 merge) and upgraded to Ubuntu 24.04 LTS.
Applying your RFC patches for the kernel and the iproute2 package
(including the sed hack) works great.
While the xl transceiver switch and the and the PWM configuration (with
3 values with 6 bit each) are still missing I tried your current code as-is.
# modprobe dummyxl
(created can0)
# ip link set can0 type can bitrate 500000 xbitrate 8000000 xl on
dbitrate 4000000 fd on
# ip -det link show can0
7: can0: <NOARP> mtu 2060 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can promiscuity 0 allmulti 0 minmtu 0 maxmtu 0
can <FD,TDC-AUTO,XL> state STOPPED restart-ms 0
Should there be a XTDC-AUTO too?
bitrate 500000 sample-point 0.875
tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 10 brp 1
dummyxl nominal: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512
brp_inc 1
dbitrate 4000000 dsample-point 0.750
dtq 12 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 2 dbrp 1
tdco 15 tdcf 0
Should the tdc* values be placed behind dbrp 1 without a line break?
The gso/gro stuff below also heavily exceeds the 80 columns. And it is
more in the same shape like with the CAN CC settings.
dummyxl FD: dtseg1 2..256 dtseg2 2..128 dsjw 1..128 dbrp 1..512
dbrp_inc 1
tdco 0..127 tdcf 0..127
same here
xbitrate 8000000 xsample-point 0.700
xtq 12 xprop-seg 3 xphase-seg1 3 xphase-seg2 3 xsjw 1 xbrp 1
xtdco/xtdcf missing here?
dummyxl XL: xtseg1 2..256 xtseg2 2..128 xsjw 1..128 xbrp 1..512
xbrp_inc 1
xtdco/xtdcf missing here?
clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size
65536 gso_ipv4_max_size 65536 gro_ipv4_max_size 65536
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-21 20:10 ` Oliver Hartkopp
@ 2024-12-01 11:32 ` Oliver Hartkopp
2024-12-03 9:45 ` Vincent Mailhol
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-01 11:32 UTC (permalink / raw)
To: Vincent Mailhol, linux-can, Marc Kleine-Budde; +Cc: Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]
Hi Vincent,
I found some issues in the code and fixed up the problems below.
The funniest thing was this copy/paste problem in netlink.h ;-)
(see attached patch with my changes)
The patch descriptions are not finalized - but it becomes usable now.
I will add the CAN XL transceiver switch to the controlmode definitions.
For the PWM configuration we would need some more discussions.
https://lore.kernel.org/linux-can/20241201112333.6950-1-socketcan@hartkopp.net/T/#u
https://lore.kernel.org/linux-can/20241201112230.6917-1-socketcan@hartkopp.net/T/#t
Best regards,
Oliver
On 21.11.24 21:10, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 11.11.24 16:32, Oliver Hartkopp wrote:
>
>> No problem! I will give some feedback when I managed to integrate the
>> extended netlink API to my driver.
>
> I managed to set up my CAN XL dev board with the latest Linux kernel
> (6.13 merge) and upgraded to Ubuntu 24.04 LTS.
>
> Applying your RFC patches for the kernel and the iproute2 package
> (including the sed hack) works great.
>
> While the xl transceiver switch and the and the PWM configuration (with
> 3 values with 6 bit each) are still missing I tried your current code
> as-is.
>
> # modprobe dummyxl
> (created can0)
>
> # ip link set can0 type can bitrate 500000 xbitrate 8000000 xl on
> dbitrate 4000000 fd on
>
> # ip -det link show can0
> 7: can0: <NOARP> mtu 2060 qdisc noop state DOWN mode DEFAULT group
> default qlen 10
> link/can promiscuity 0 allmulti 0 minmtu 0 maxmtu 0
> can <FD,TDC-AUTO,XL> state STOPPED restart-ms 0
>
> Should there be a XTDC-AUTO too?
>
> bitrate 500000 sample-point 0.875
> tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 10 brp 1
> dummyxl nominal: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512
> brp_inc 1
> dbitrate 4000000 dsample-point 0.750
> dtq 12 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 2 dbrp 1
> tdco 15 tdcf 0
>
> Should the tdc* values be placed behind dbrp 1 without a line break?
> The gso/gro stuff below also heavily exceeds the 80 columns. And it is
> more in the same shape like with the CAN CC settings.
>
> dummyxl FD: dtseg1 2..256 dtseg2 2..128 dsjw 1..128 dbrp 1..512
> dbrp_inc 1
> tdco 0..127 tdcf 0..127
>
> same here
>
> xbitrate 8000000 xsample-point 0.700
> xtq 12 xprop-seg 3 xphase-seg1 3 xphase-seg2 3 xsjw 1 xbrp 1
>
> xtdco/xtdcf missing here?
>
> dummyxl XL: xtseg1 2..256 xtseg2 2..128 xsjw 1..128 xbrp 1..512
> xbrp_inc 1
>
> xtdco/xtdcf missing here?
>
> clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
> gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size
> 65536 gso_ipv4_max_size 65536 gro_ipv4_max_size 65536
>
> Best regards,
> Oliver
[-- Attachment #2: fix-for-vincents-rfc.patch --]
[-- Type: text/x-patch, Size: 3542 bytes --]
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 0a6700194ab0..64f675e874f6 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -127,10 +127,19 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
if (err) {
NL_SET_ERR_MSG_FMT(extack,
"TDC parameters are incorrect");
return err;
}
+ err = can_validate_tdc(data[IFLA_CAN_XL_TDC],
+ cm->flags & CAN_CTRLMODE_XL_TDC_AUTO,
+ cm->flags & CAN_CTRLMODE_XL_TDC_MANUAL,
+ extack);
+ if (err) {
+ NL_SET_ERR_MSG_FMT(extack,
+ "XL TDC parameters are incorrect");
+ return err;
+ }
}
if (data[IFLA_CAN_BITTIMING]) {
struct can_bittiming bt;
@@ -570,20 +579,21 @@ static size_t can_get_size(const struct net_device *dev)
return size;
}
static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
struct data_bittiming_params *dbt_params,
- bool tdc_is_enabled, bool tdc_manual)
+ bool tdc_is_enabled, bool tdc_manual,
+ int ifla_can_tdc_type)
{
struct nlattr *nest;
struct can_tdc *tdc = &dbt_params->tdc;
const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
if (!tdc_const)
return 0;
- nest = nla_nest_start(skb, IFLA_CAN_TDC);
+ nest = nla_nest_start(skb, ifla_can_tdc_type);
if (!nest)
return -EMSGSIZE;
if (tdc_manual &&
(nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
@@ -702,11 +712,12 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
(nla_put(skb, IFLA_CAN_BITRATE_MAX,
sizeof(priv->bitrate_max),
&priv->bitrate_max)) ||
can_tdc_fill_info(skb, dev, &priv->fd, can_fd_tdc_is_enabled(priv),
- priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) ||
+ priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL,
+ IFLA_CAN_TDC) ||
can_ctrlmode_ext_fill_info(skb, priv) ||
(priv->xl.data_bittiming.bitrate &&
nla_put(skb, IFLA_CAN_XL_DATA_BITTIMING,
@@ -722,11 +733,12 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
sizeof(*priv->xl.data_bitrate_const) *
priv->xl.data_bitrate_const_cnt,
priv->xl.data_bitrate_const)) ||
can_tdc_fill_info(skb, dev, &priv->xl, can_xl_tdc_is_enabled(priv),
- priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL)
+ priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL,
+ IFLA_CAN_XL_TDC)
)
return -EMSGSIZE;
return 0;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index c81fd153a07f..f4fb8eea8f35 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -102,12 +102,12 @@ struct can_ctrlmode {
#define CAN_CTRLMODE_FD_NON_ISO 0x80 /* CAN FD in non-ISO mode */
#define CAN_CTRLMODE_CC_LEN8_DLC 0x100 /* Classic CAN DLC option */
#define CAN_CTRLMODE_TDC_AUTO 0x200 /* FD transceiver automatically calculates TDCV */
#define CAN_CTRLMODE_TDC_MANUAL 0x400 /* FD TDCV is manually set up by user */
#define CAN_CTRLMODE_XL 0x800 /* CAN XL mode */
-#define CAN_CTRLMODE_XL_TDC_AUTO 0x200 /* XL transceiver automatically calculates TDCV */
-#define CAN_CTRLMODE_XL_TDC_MANUAL 0x400 /* XL TDCV is manually set up by user */
+#define CAN_CTRLMODE_XL_TDC_AUTO 0x1000 /* XL transceiver automatically calculates TDCV */
+#define CAN_CTRLMODE_XL_TDC_MANUAL 0x2000 /* XL TDCV is manually set up by user */
/*
* CAN device statistics
*/
struct can_device_stats {
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-01 11:32 ` Oliver Hartkopp
@ 2024-12-03 9:45 ` Vincent Mailhol
2024-12-04 7:56 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-12-03 9:45 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
On Sun. 1 Dec. 2024 at 20:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> I found some issues in the code and fixed up the problems below.
>
> The funniest thing was this copy/paste problem in netlink.h ;-)
> (see attached patch with my changes)
>
> The patch descriptions are not finalized - but it becomes usable now.
> I will add the CAN XL transceiver switch to the controlmode definitions.
>
> For the PWM configuration we would need some more discussions.
>
> https://lore.kernel.org/linux-can/20241201112333.6950-1-socketcan@hartkopp.net/T/#u
> https://lore.kernel.org/linux-can/20241201112230.6917-1-socketcan@hartkopp.net/T/#t
>
> Best regards,
> Oliver
Hi Oliver,
Thanks for all the testing and the fixes. Because of the lack of
testing of this RFC on my side, I was expecting such issues. But I
really appreciate that you took time to investigate and debug, really
helpful! I will make sure to incorporate these fixes in the next
version.
On my side, the last three weeks were more busy than anticipated but I
finally found some time to do a deep dive in ISO 11898-1:2024, I read
two thirds of it so far, just a few more pages to go. It just takes me
time to digest all the information. Once this is done, things should
be more straightforward.
The next series I send will add the pwm and drop the RFC patch. My
goal is to have this CAN XL series ready for inclusion in linux 6.14.
I don't want to overcommit, but hopefully, I would like to send the v1
either this weekend or next weekend.
I will also rethink whether or not is it worth doing some NLA nesting
as suggested by Marc here:
https://lore.kernel.org/linux-can/20241112-flashy-straight-poodle-9a796d-mkl@pengutronix.de/
(I am still divided on this subject).
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-03 9:45 ` Vincent Mailhol
@ 2024-12-04 7:56 ` Oliver Hartkopp
2024-12-15 9:21 ` Vincent Mailhol
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-04 7:56 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
Hi Vincent,
On 03.12.24 10:45, Vincent Mailhol wrote:
>> https://lore.kernel.org/linux-can/20241201112333.6950-1-socketcan@hartkopp.net/T/#u
>> https://lore.kernel.org/linux-can/20241201112230.6917-1-socketcan@hartkopp.net/T/#t
> Thanks for all the testing and the fixes. Because of the lack of
> testing of this RFC on my side, I was expecting such issues. But I
> really appreciate that you took time to investigate and debug, really
> helpful! I will make sure to incorporate these fixes in the next
> version.
I'll send out an extended RFC V2 which is my current test base for you
in some minutes.
Either for the kernel and iproute2 there are two new patches that add
new controlmode flags.
> The next series I send will add the pwm and drop the RFC patch.
Excellent!
I assume it will follow the TDC pattern with
CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
or something similar?
> My
> goal is to have this CAN XL series ready for inclusion in linux 6.14.
> I don't want to overcommit, but hopefully, I would like to send the v1
> either this weekend or next weekend.
No hurry so far ;-)
> I will also rethink whether or not is it worth doing some NLA nesting
> as suggested by Marc here:
>
> https://lore.kernel.org/linux-can/20241112-flashy-straight-poodle-9a796d-mkl@pengutronix.de/
>
> (I am still divided on this subject).
I'll take a look into this too.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-04 7:56 ` Oliver Hartkopp
@ 2024-12-15 9:21 ` Vincent Mailhol
2024-12-17 9:53 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-12-15 9:21 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
Hi,
I am done reading (and understanding) ISO 11898-1:2024, so let's resume
the work!
On 04/12/2024 at 16:56, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 03.12.24 10:45, Vincent Mailhol wrote:
>
>>> https://lore.kernel.org/linux-can/20241201112333.6950-1-
>>> socketcan@hartkopp.net/T/#u
>>> https://lore.kernel.org/linux-can/20241201112230.6917-1-
>>> socketcan@hartkopp.net/T/#t
>
>> Thanks for all the testing and the fixes. Because of the lack of
>> testing of this RFC on my side, I was expecting such issues. But I
>> really appreciate that you took time to investigate and debug, really
>> helpful! I will make sure to incorporate these fixes in the next
>> version.
>
> I'll send out an extended RFC V2 which is my current test base for you
> in some minutes.
>
> Either for the kernel and iproute2 there are two new patches that add
> new controlmode flags.
>
>> The next series I send will add the pwm and drop the RFC patch.
>
> Excellent!
>
> I assume it will follow the TDC pattern with
> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
> or something similar?
I am not sure why we would need a tristate here. The reason why I put
the tristate for the TDC is because some transceiver (the majority) will
automatically measure TDCV but some may give the option for the user to
manually set it.
From what I understand from the specification, PWMO is not something
which is dynamically measured. If I got it right, it is just the
remainder whenever the bit time is not a multiple of PWM. Something like
this:
pwmo = bit_time_in_minimum_time_quantum % pwm;
Same for the PWMS and PWML. I am not yet sure how to calculate those
(more on this below) but these seem to be calculated once for all (like
any bittimming value other than the TDCs).
Let me know if I missed something, the PWM is very new to me :)
So, for the implementation, I am thinking of:
1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
2. Add these to bittiming.c:
struct can_pwm {
u32 pwms; /* PWM short phase length */
u32 pwml; /* PWM long phase length */
u32 pwmo; /* PWM offset */
};
struct can_pwm_const {
u32 pwms_max;
u32 pwml_max;
u32 pwmo_max;
};
static inline u32 can_get_pwm(const struct can_pwm *pwm)
{
return pwm->pwms + pwm->pwml;
}
The minimum value of all those configurable ranges is already specified
to be one minimum time quantum (tq min), so I do not think we need a
field for the minimums.
At the moment, I am stuck on the PWM calculation. I do not know how to
derive good PWMS and PWML values out of the const ranges.
The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
topic. But getting access to this document requires a subscription
(which I do not have):
https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-guidelines-and-application-notes
Do any of you have access to this document? Or do any of you know a good
formula for the PWMS and PWML calculation?
Thank you,
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-15 9:21 ` Vincent Mailhol
@ 2024-12-17 9:53 ` Oliver Hartkopp
2024-12-17 18:17 ` Vincent Mailhol
0 siblings, 1 reply; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-17 9:53 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
Hi Vincent,
On 15.12.24 10:21, Vincent Mailhol wrote:
> I am done reading (and understanding) ISO 11898-1:2024, so let's resume
> the work!
Good! ;-)
> On 04/12/2024 at 16:56, Oliver Hartkopp wrote:
>> Hi Vincent,
>>
>> On 03.12.24 10:45, Vincent Mailhol wrote:
>>
>>>> https://lore.kernel.org/linux-can/20241201112333.6950-1-
>>>> socketcan@hartkopp.net/T/#u
>>>> https://lore.kernel.org/linux-can/20241201112230.6917-1-
>>>> socketcan@hartkopp.net/T/#t
>>
>>> Thanks for all the testing and the fixes. Because of the lack of
>>> testing of this RFC on my side, I was expecting such issues. But I
>>> really appreciate that you took time to investigate and debug, really
>>> helpful! I will make sure to incorporate these fixes in the next
>>> version.
>>
>> I'll send out an extended RFC V2 which is my current test base for you
>> in some minutes.
>>
>> Either for the kernel and iproute2 there are two new patches that add
>> new controlmode flags.
>>
>>> The next series I send will add the pwm and drop the RFC patch.
>>
>> Excellent!
>>
>> I assume it will follow the TDC pattern with
>> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
>> or something similar?
>
> I am not sure why we would need a tristate here. The reason why I put
> the tristate for the TDC is because some transceiver (the majority) will
> automatically measure TDCV but some may give the option for the user to
> manually set it.
>
> From what I understand from the specification, PWMO is not something
> which is dynamically measured. If I got it right, it is just the
> remainder whenever the bit time is not a multiple of PWM. Something like
> this:
>
> pwmo = bit_time_in_minimum_time_quantum % pwm;
>
> Same for the PWMS and PWML. I am not yet sure how to calculate those
> (more on this below) but these seem to be calculated once for all (like
> any bittimming value other than the TDCs).
>
> Let me know if I missed something, the PWM is very new to me :)
>
>
> So, for the implementation, I am thinking of:
The PWM stuff is some very CAN XL specific therefore I would suggest to
bring this into the naming too:
>
> 1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
CAN_CTRLMODE_XL_PWM or CAN_CTRLMODE_XLPWM
>
> 2. Add these to bittiming.c:
>
> struct can_pwm {
can_xl_pwm or can_xlpwm
> u32 pwms; /* PWM short phase length */
> u32 pwml; /* PWM long phase length */
> u32 pwmo; /* PWM offset */
that's fine
> };
>
> struct can_pwm_const {
dito
> u32 pwms_max;
> u32 pwml_max;
> u32 pwmo_max;
> };
>
> static inline u32 can_get_pwm(const struct can_pwm *pwm)
> {
> return pwm->pwms + pwm->pwml;
> }
??
>
> The minimum value of all those configurable ranges is already specified
> to be one minimum time quantum (tq min), so I do not think we need a
> field for the minimums.
>
>
> At the moment, I am stuck on the PWM calculation. I do not know how to
> derive good PWMS and PWML values out of the const ranges.
>
> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
> topic. But getting access to this document requires a subscription
> (which I do not have):
>
>
> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-guidelines-and-application-notes
>
> Do any of you have access to this document? Or do any of you know a good
> formula for the PWMS and PWML calculation?
I have to check for the referenced document.
Btw. this is some code that shows the idea of how to calculate the PWM
values based on the CAN clock and the XL data bitrate:
if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
// @5Mbit/s
pwmo = 0;
pwms = 6;
pwml = 14;
} else if (CanClock_mhz == 100) {
// @10Mbit/s
pwmo = 0;
pwms = 3;
pwml = 7;
} else if (CanClock_mhz == 160) {
// @10Mbit/s
pwmo = 0;
pwms = 2;
pwml = 6;
}
For that reason I was thinking about some default calculation provided
by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
people want to set the values analog to tseg1 and friends instead of
setting a single bitrate.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-17 9:53 ` Oliver Hartkopp
@ 2024-12-17 18:17 ` Vincent Mailhol
2024-12-17 20:03 ` Oliver Hartkopp
0 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-12-17 18:17 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
> Hi Vincent,
(...)
>>> I assume it will follow the TDC pattern with
>>> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
>>> or something similar?
>>
>> I am not sure why we would need a tristate here. The reason why I put
>> the tristate for the TDC is because some transceiver (the majority) will
>> automatically measure TDCV but some may give the option for the user to
>> manually set it.
>>
>> From what I understand from the specification, PWMO is not something
>> which is dynamically measured. If I got it right, it is just the
>> remainder whenever the bit time is not a multiple of PWM. Something like
>> this:
>>
>> pwmo = bit_time_in_minimum_time_quantum % pwm;
>>
>> Same for the PWMS and PWML. I am not yet sure how to calculate those
>> (more on this below) but these seem to be calculated once for all (like
>> any bittimming value other than the TDCs).
>>
>> Let me know if I missed something, the PWM is very new to me :)
>>
>>
>> So, for the implementation, I am thinking of:
>
> The PWM stuff is some very CAN XL specific therefore I would suggest to
> bring this into the naming too:
It is specific to CAN XL for the moment. But if I do a parallel, before
CAN XL existed, the TDC was specific to CAN FD. But the structure was
named can_tdc, not can_fd_tdc, and now that the CAN XL reuses the TDC,
it comes in handy that the structure did not have fd in the name.
The CAN XL still have a resXL flag for potential future extension, if
one day there is a CAN XXL, then the PWM would not be specific to CAN XL
anymore.
My approach will be to:
- name structures and entries within a netlink nest in a generic
manner
- name struct fields (e.g. in can_priv) and the top level netlink
entries with an XL prefix.
>> 1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
>
> CAN_CTRLMODE_XL_PWM or CAN_CTRLMODE_XLPWM
Ack. This flag is specific to CAN XL. I have a preference for
CAN_CTRLMODE_XL_PWM (with the underscore between XL and PWM).
>> 2. Add these to bittiming.c:
>>
>> struct can_pwm {
> can_xl_pwm or can_xlpwm
NACK. I keep can_pwm, but in can_priv, I will name the field:
struct can_pwm xl_pwm;
>> u32 pwms; /* PWM short phase length */
>> u32 pwml; /* PWM long phase length */
>> u32 pwmo; /* PWM offset */
> that's fine
>
>> };
>>
>> struct can_pwm_const {
> dito
>
>> u32 pwms_max;
>> u32 pwml_max;
>> u32 pwmo_max;
>> };
>>
>> static inline u32 can_get_pwm(const struct can_pwm *pwm)
>> {
>> return pwm->pwms + pwm->pwml;
>> }
> ??
ISO 11898-1:2024 tells me that (quote):
The PWM symbol length is the sum of PWMS and PWML.
This inline function was to calculate this PWM symbol length. That said,
on second thought, I doubt that the drivers will need this information.
I will remove it for the moment. If someone needs it, we can reintroduce
this later.
>> The minimum value of all those configurable ranges is already specified
>> to be one minimum time quantum (tq min), so I do not think we need a
>> field for the minimums.
>>
>>
>> At the moment, I am stuck on the PWM calculation. I do not know how to
>> derive good PWMS and PWML values out of the const ranges.
>>
>> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
>> topic. But getting access to this document requires a subscription
>> (which I do not have):
>>
>>
>> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-
>> guidelines-and-application-notes
>>
>> Do any of you have access to this document? Or do any of you know a good
>> formula for the PWMS and PWML calculation?
>
> I have to check for the referenced document.
>
> Btw. this is some code that shows the idea of how to calculate the PWM
> values based on the CAN clock and the XL data bitrate:
>
> if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
> // @5Mbit/s> pwmo = 0;
> pwms = 6;
> pwml = 14;
one bit is 200 nano seconds.
one minimum time quantum is 10 ns.
pwm = pwms + pwml
= 6 + 14
= 20 minimum time quantum
So PWM duration is 20 * 10 = 200 ns. So there is only one PWM per bit.
> } else if (CanClock_mhz == 100) {
> // @10Mbit/s> pwmo = 0;
> pwms = 3;
> pwml = 7;
1 bit is 100 ns.
one minimum time quantum is 10 ns.
pwm = pwms + pwml
= 3 + 7
= 10 minimum time quantum
So PWM duration is 10 * 10 = 100 ns. So here also, there is only one PWM
per bit.
> } else if (CanClock_mhz == 160) {
> // @10Mbit/s> pwmo = 0;
> pwms = 2;
> pwml = 6;
1 bit is 100 ns.
one minimum time quantum is 6.25 ns.
pwm = pwms + pwml
= 2 + 6
= 8 minimum time quantum
So PWM duration is 8 * 6.25 = 50 ns. This time, there are two PWM per
bit. Interesting.
> }
So from those value, I derived:
pwms = round_down(30% * pwm)
pwml = round_up(70% * pwm)
PWMO was already established as:
pwmo = bit_time_in_minimum_time_quantum % pwm
(in your example, the bit time is a multiple of PWM, so the PWMO is zero).
This is some progress. What is not clear yet is how to find the number
of PWM per bit (i.e. why is it only one PWM per bit in your first two
examples, why is it two PWM per bit in your second example).
The ISO has one more formula that I did not yet used. Maybe that's the
solution. But it is 3 a.m. here, let's say that this is a problem for
the tomorrow me.
> For that reason I was thinking about some default calculation provided
> by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
> people want to set the values analog to tseg1 and friends instead of
> setting a single bitrate.
Don't worry, we have the same final result in mind. It is just that I
believe that we can use a trick so that we can do it with a single flag :)
Let me illustrate in more details.
Case 1: PWM off
ip link set can0 (...) xl on pwm off
- CAN_CTRLMODE_XL_PWM bit is false
- IFLA_CAN_XL_PWM is not provided
Case 2: PWM on auto
ip link set can0 (...) xl on pwm on
- CAN_CTRLMODE_XL_PWM bit is true
- IFLA_CAN_XL_PWM is not provided
Case 3: PWM on manual
ip link set can0 (...) xl on pwm on pwms xxxx pwml yyyy
- CAN_CTRLMODE_XL_PWM bit is false
- IFLA_CAN_XL_PWM is provided (and contains a nest with both
IFLA_CAN_PWMS and IFLA_CAN_PWML)
So, here, I use the presence or the absence of the IFLA_CAN_XL_PWM as an
hidden flag to know whether the calculation should be done or not.
Note that there is one more case:
Case 4: full auto??? (let the kernel decide between PWM on or off)
ip link set can0 (...) xl on
Here, the user does not tell if PWM should be on or off. I think that
this case should be banned (return error). For what I understand, if one
node uses PWM and the other doesn't, it just doesn't work. So, contrary
to TDC, there is no way to calculate if this should be on or off.
(side note: I will be busy for personal reasons until this Sunday, do
not be surprised if my next answer is delayed).
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-17 18:17 ` Vincent Mailhol
@ 2024-12-17 20:03 ` Oliver Hartkopp
2024-12-18 9:13 ` Oliver Hartkopp
2025-01-30 5:42 ` Vincent Mailhol
0 siblings, 2 replies; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-17 20:03 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
Hi Vincent,
On 17.12.24 19:17, Vincent Mailhol wrote:
> On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
>
> (...)
>
>>>> I assume it will follow the TDC pattern with
>>>> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
>>>> or something similar?
>>>
>>> I am not sure why we would need a tristate here. The reason why I put
>>> the tristate for the TDC is because some transceiver (the majority) will
>>> automatically measure TDCV but some may give the option for the user to
>>> manually set it.
>>>
>>> From what I understand from the specification, PWMO is not something
>>> which is dynamically measured. If I got it right, it is just the
>>> remainder whenever the bit time is not a multiple of PWM. Something like
>>> this:
>>>
>>> pwmo = bit_time_in_minimum_time_quantum % pwm;
>>>
>>> Same for the PWMS and PWML. I am not yet sure how to calculate those
>>> (more on this below) but these seem to be calculated once for all (like
>>> any bittimming value other than the TDCs).
>>>
>>> Let me know if I missed something, the PWM is very new to me :)
>>>
>>>
>>> So, for the implementation, I am thinking of:
>>
>> The PWM stuff is some very CAN XL specific therefore I would suggest to
>> bring this into the naming too:
>
> It is specific to CAN XL for the moment. But if I do a parallel, before
> CAN XL existed, the TDC was specific to CAN FD. But the structure was
> named can_tdc, not can_fd_tdc, and now that the CAN XL reuses the TDC,
> it comes in handy that the structure did not have fd in the name.
>
> The CAN XL still have a resXL flag for potential future extension, if
> one day there is a CAN XXL, then the PWM would not be specific to CAN XL
> anymore.
>
> My approach will be to:
>
> - name structures and entries within a netlink nest in a generic
> manner
> - name struct fields (e.g. in can_priv) and the top level netlink
> entries with an XL prefix.
>
>>> 1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
>>
>> CAN_CTRLMODE_XL_PWM or CAN_CTRLMODE_XLPWM
>
> Ack. This flag is specific to CAN XL. I have a preference for
> CAN_CTRLMODE_XL_PWM (with the underscore between XL and PWM).
ACK
>>> 2. Add these to bittiming.c:
>>>
>>> struct can_pwm {
>> can_xl_pwm or can_xlpwm
>
> NACK. I keep can_pwm, but in can_priv, I will name the field:
Ok. Fine for me.
>
> struct can_pwm xl_pwm;
>
>>> u32 pwms; /* PWM short phase length */
>>> u32 pwml; /* PWM long phase length */
>>> u32 pwmo; /* PWM offset */
>> that's fine
>>
>>> };
>>>
>>> struct can_pwm_const {
>> dito
>>
>>> u32 pwms_max;
>>> u32 pwml_max;
>>> u32 pwmo_max;
>>> };
>>>
>>> static inline u32 can_get_pwm(const struct can_pwm *pwm)
>>> {
>>> return pwm->pwms + pwm->pwml;
>>> }
>> ??
>
> ISO 11898-1:2024 tells me that (quote):
>
> The PWM symbol length is the sum of PWMS and PWML.
>
> This inline function was to calculate this PWM symbol length. That said,
> on second thought, I doubt that the drivers will need this information.
> I will remove it for the moment. If someone needs it, we can reintroduce
> this later.
>
>>> The minimum value of all those configurable ranges is already specified
>>> to be one minimum time quantum (tq min), so I do not think we need a
>>> field for the minimums.
>>>
>>>
>>> At the moment, I am stuck on the PWM calculation. I do not know how to
>>> derive good PWMS and PWML values out of the const ranges.
>>>
>>> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
>>> topic. But getting access to this document requires a subscription
>>> (which I do not have):
>>>
>>>
>>> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-
>>> guidelines-and-application-notes
>>>
>>> Do any of you have access to this document? Or do any of you know a good
>>> formula for the PWMS and PWML calculation?
>>
>> I have to check for the referenced document.
>>
>> Btw. this is some code that shows the idea of how to calculate the PWM
>> values based on the CAN clock and the XL data bitrate:
>>
>> if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
>> // @5Mbit/s> pwmo = 0;
>> pwms = 6;
>> pwml = 14;
>
> one bit is 200 nano seconds.
> one minimum time quantum is 10 ns.
>
> pwm = pwms + pwml
> = 6 + 14
> = 20 minimum time quantum
>
> So PWM duration is 20 * 10 = 200 ns. So there is only one PWM per bit.
>
>> } else if (CanClock_mhz == 100) {
>> // @10Mbit/s> pwmo = 0;
>> pwms = 3;
>> pwml = 7;
>
> 1 bit is 100 ns.
> one minimum time quantum is 10 ns.
>
> pwm = pwms + pwml
> = 3 + 7
> = 10 minimum time quantum
>
> So PWM duration is 10 * 10 = 100 ns. So here also, there is only one PWM
> per bit.
>
>> } else if (CanClock_mhz == 160) {
>> // @10Mbit/s> pwmo = 0;
>> pwms = 2;
>> pwml = 6;
>
> 1 bit is 100 ns.
> one minimum time quantum is 6.25 ns.
>
> pwm = pwms + pwml
> = 2 + 6
> = 8 minimum time quantum
>
> So PWM duration is 8 * 6.25 = 50 ns. This time, there are two PWM per
> bit. Interesting.
>> }
>
> So from those value, I derived:
>
> pwms = round_down(30% * pwm)
> pwml = round_up(70% * pwm)
>
> PWMO was already established as:
>
> pwmo = bit_time_in_minimum_time_quantum % pwm
>
> (in your example, the bit time is a multiple of PWM, so the PWMO is zero).
>
> This is some progress. What is not clear yet is how to find the number
> of PWM per bit (i.e. why is it only one PWM per bit in your first two
> examples, why is it two PWM per bit in your second example).
>
> The ISO has one more formula that I did not yet used. Maybe that's the
> solution. But it is 3 a.m. here, let's say that this is a problem for
> the tomorrow me.
>
>> For that reason I was thinking about some default calculation provided
>> by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
>> people want to set the values analog to tseg1 and friends instead of
>> setting a single bitrate.
>
> Don't worry, we have the same final result in mind. It is just that I
> believe that we can use a trick so that we can do it with a single flag :)
>
> Let me illustrate in more details.
>
> Case 1: PWM off
>
> ip link set can0 (...) xl on pwm off
Ugh.
Did you take a look into all my patches from 2024-12-04, e.g.
https://lore.kernel.org/linux-can/20241204075741.3727-3-socketcan@hartkopp.net/T/#u
which is introducing CAN_CTRLMODE_XL_TRX ??
We are talking about having a CAN XL capable/switchable transceiver
attached to the CAN (XL) controller or not. IMO we should also name it
accordingly with
ip link set can0 (...) xl on xltrx off
That the transceiver switching is done with PWM under the hood is
nothing we should disclose to the user in this way IMHO.
>
> - CAN_CTRLMODE_XL_PWM bit is false
> - IFLA_CAN_XL_PWM is not provided
>
> Case 2: PWM on auto
>
> ip link set can0 (...) xl on pwm on
>
> - CAN_CTRLMODE_XL_PWM bit is true
> - IFLA_CAN_XL_PWM is not provided
>
> Case 3: PWM on manual
>
> ip link set can0 (...) xl on pwm on pwms xxxx pwml yyyy
>
> - CAN_CTRLMODE_XL_PWM bit is false
> - IFLA_CAN_XL_PWM is provided (and contains a nest with both
> IFLA_CAN_PWMS and IFLA_CAN_PWML)
>
> So, here, I use the presence or the absence of the IFLA_CAN_XL_PWM as an
> hidden flag to know whether the calculation should be done or not.
Yes, that's a good idea. Excellent default behavior.
At least when reading IFLA_CAN_XL_PWM the provided or automatically
calculated values would become visible.
> Note that there is one more case:
>
> Case 4: full auto??? (let the kernel decide between PWM on or off)
>
> ip link set can0 (...) xl on
>
> Here, the user does not tell if PWM should be on or off. I think that
> this case should be banned (return error).
Yes! Good idea!
> For what I understand, if one
> node uses PWM and the other doesn't, it just doesn't work. So, contrary
> to TDC, there is no way to calculate if this should be on or off.
>
> (side note: I will be busy for personal reasons until this Sunday, do
> not be surprised if my next answer is delayed).
No problem.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-17 20:03 ` Oliver Hartkopp
@ 2024-12-18 9:13 ` Oliver Hartkopp
2025-01-30 5:42 ` Vincent Mailhol
1 sibling, 0 replies; 44+ messages in thread
From: Oliver Hartkopp @ 2024-12-18 9:13 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Marc Kleine-Budde, Robert Nawrath
Addition ...
On 17.12.24 21:03, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 17.12.24 19:17, Vincent Mailhol wrote:
>> On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
>>
>> (...)
>>
>>>>> I assume it will follow the TDC pattern with
>>>>> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
>>>>> or something similar?
>>>>
>>>> I am not sure why we would need a tristate here. The reason why I put
>>>> the tristate for the TDC is because some transceiver (the majority)
>>>> will
>>>> automatically measure TDCV but some may give the option for the user to
>>>> manually set it.
>>>>
>>>> From what I understand from the specification, PWMO is not something
>>>> which is dynamically measured. If I got it right, it is just the
>>>> remainder whenever the bit time is not a multiple of PWM. Something
>>>> like
>>>> this:
>>>>
>>>> pwmo = bit_time_in_minimum_time_quantum % pwm;
>>>>
>>>> Same for the PWMS and PWML. I am not yet sure how to calculate those
>>>> (more on this below) but these seem to be calculated once for all (like
>>>> any bittimming value other than the TDCs).
>>>>
>>>> Let me know if I missed something, the PWM is very new to me :)
>>>>
>>>>
>>>> So, for the implementation, I am thinking of:
>>>
>>> The PWM stuff is some very CAN XL specific therefore I would suggest to
>>> bring this into the naming too:
>>
>> It is specific to CAN XL for the moment. But if I do a parallel, before
>> CAN XL existed, the TDC was specific to CAN FD. But the structure was
>> named can_tdc, not can_fd_tdc, and now that the CAN XL reuses the TDC,
>> it comes in handy that the structure did not have fd in the name.
>>
>> The CAN XL still have a resXL flag for potential future extension, if
>> one day there is a CAN XXL, then the PWM would not be specific to CAN XL
>> anymore.
>>
>> My approach will be to:
>>
>> - name structures and entries within a netlink nest in a generic
>> manner
>> - name struct fields (e.g. in can_priv) and the top level netlink
>> entries with an XL prefix.
>>
>>>> 1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
>>>
>>> CAN_CTRLMODE_XL_PWM or CAN_CTRLMODE_XLPWM
>>
>> Ack. This flag is specific to CAN XL. I have a preference for
>> CAN_CTRLMODE_XL_PWM (with the underscore between XL and PWM).
>
> ACK
>
>
>>>> 2. Add these to bittiming.c:
>>>>
>>>> struct can_pwm {
>>> can_xl_pwm or can_xlpwm
>>
>> NACK. I keep can_pwm, but in can_priv, I will name the field:
>
> Ok. Fine for me.
>>
>> struct can_pwm xl_pwm;
>>
>>>> u32 pwms; /* PWM short phase length */
>>>> u32 pwml; /* PWM long phase length */
>>>> u32 pwmo; /* PWM offset */
>>> that's fine
>>>
>>>> };
>>>>
>>>> struct can_pwm_const {
>>> dito
>>>
>>>> u32 pwms_max;
>>>> u32 pwml_max;
>>>> u32 pwmo_max;
>>>> };
>>>>
>>>> static inline u32 can_get_pwm(const struct can_pwm *pwm)
>>>> {
>>>> return pwm->pwms + pwm->pwml;
>>>> }
>>> ??
>>
>> ISO 11898-1:2024 tells me that (quote):
>>
>> The PWM symbol length is the sum of PWMS and PWML.
>>
>> This inline function was to calculate this PWM symbol length. That said,
>> on second thought, I doubt that the drivers will need this information.
>> I will remove it for the moment. If someone needs it, we can reintroduce
>> this later.
>>
>>>> The minimum value of all those configurable ranges is already specified
>>>> to be one minimum time quantum (tq min), so I do not think we need a
>>>> field for the minimums.
>>>>
>>>>
>>>> At the moment, I am stuck on the PWM calculation. I do not know how to
>>>> derive good PWMS and PWML values out of the const ranges.
>>>>
>>>> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
>>>> topic. But getting access to this document requires a subscription
>>>> (which I do not have):
>>>>
>>>>
>>>> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-
>>>> guidelines-and-application-notes
>>>>
>>>> Do any of you have access to this document? Or do any of you know a
>>>> good
>>>> formula for the PWMS and PWML calculation?
>>>
>>> I have to check for the referenced document.
>>>
>>> Btw. this is some code that shows the idea of how to calculate the PWM
>>> values based on the CAN clock and the XL data bitrate:
>>>
>>> if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
>>> // @5Mbit/s> pwmo = 0;
>>> pwms = 6;
>>> pwml = 14;
>>
>> one bit is 200 nano seconds.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>> = 6 + 14
>> = 20 minimum time quantum
>>
>> So PWM duration is 20 * 10 = 200 ns. So there is only one PWM per bit.
>>
>>> } else if (CanClock_mhz == 100) {
>>> // @10Mbit/s> pwmo = 0;
>>> pwms = 3;
>>> pwml = 7;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>> = 3 + 7
>> = 10 minimum time quantum
>>
>> So PWM duration is 10 * 10 = 100 ns. So here also, there is only one PWM
>> per bit.
>>
>>> } else if (CanClock_mhz == 160) {
>>> // @10Mbit/s> pwmo = 0;
>>> pwms = 2;
>>> pwml = 6;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 6.25 ns.
>>
>> pwm = pwms + pwml
>> = 2 + 6
>> = 8 minimum time quantum
>>
>> So PWM duration is 8 * 6.25 = 50 ns. This time, there are two PWM per
>> bit. Interesting.
>>> }
>>
>> So from those value, I derived:
>>
>> pwms = round_down(30% * pwm)
>> pwml = round_up(70% * pwm)
>>
>> PWMO was already established as:
>>
>> pwmo = bit_time_in_minimum_time_quantum % pwm
>>
>> (in your example, the bit time is a multiple of PWM, so the PWMO is
>> zero).
>>
>> This is some progress. What is not clear yet is how to find the number
>> of PWM per bit (i.e. why is it only one PWM per bit in your first two
>> examples, why is it two PWM per bit in your second example).
>>
>> The ISO has one more formula that I did not yet used. Maybe that's the
>> solution. But it is 3 a.m. here, let's say that this is a problem for
>> the tomorrow me.
>>
>>> For that reason I was thinking about some default calculation provided
>>> by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
>>> people want to set the values analog to tseg1 and friends instead of
>>> setting a single bitrate.
>>
>> Don't worry, we have the same final result in mind. It is just that I
>> believe that we can use a trick so that we can do it with a single
>> flag :)
>>
>> Let me illustrate in more details.
>>
>> Case 1: PWM off
>>
>> ip link set can0 (...) xl on pwm off
>
> Ugh.
>
> Did you take a look into all my patches from 2024-12-04, e.g.
>
> https://lore.kernel.org/linux-can/20241204075741.3727-3-
> socketcan@hartkopp.net/T/#u
>
> which is introducing CAN_CTRLMODE_XL_TRX ??
It would be nice if you implement the PWM stuff based on these patch
sets from 2024-12-04. I introduced CAN_CTRLMODE_XL_TRX and
CAN_CTRLMODE_XL_RRS either in the kernel and in the iproute2 which is
already operational in my setup.
Thanks,
Oliver
>
> We are talking about having a CAN XL capable/switchable transceiver
> attached to the CAN (XL) controller or not. IMO we should also name it
> accordingly with
>
> ip link set can0 (...) xl on xltrx off
>
> That the transceiver switching is done with PWM under the hood is
> nothing we should disclose to the user in this way IMHO.
>
>>
>> - CAN_CTRLMODE_XL_PWM bit is false
>> - IFLA_CAN_XL_PWM is not provided
>>
>> Case 2: PWM on auto
>>
>> ip link set can0 (...) xl on pwm on
>>
>> - CAN_CTRLMODE_XL_PWM bit is true
>> - IFLA_CAN_XL_PWM is not provided
>>
>> Case 3: PWM on manual
>>
>> ip link set can0 (...) xl on pwm on pwms xxxx pwml yyyy
>>
>> - CAN_CTRLMODE_XL_PWM bit is false
>> - IFLA_CAN_XL_PWM is provided (and contains a nest with both
>> IFLA_CAN_PWMS and IFLA_CAN_PWML)
>>
>> So, here, I use the presence or the absence of the IFLA_CAN_XL_PWM as an
>> hidden flag to know whether the calculation should be done or not.
>
> Yes, that's a good idea. Excellent default behavior.
>
> At least when reading IFLA_CAN_XL_PWM the provided or automatically
> calculated values would become visible.
>
>> Note that there is one more case:
>>
>> Case 4: full auto??? (let the kernel decide between PWM on or off)
>>
>> ip link set can0 (...) xl on
>>
>> Here, the user does not tell if PWM should be on or off. I think that
>> this case should be banned (return error).
>
> Yes! Good idea!
>
>> For what I understand, if one
>> node uses PWM and the other doesn't, it just doesn't work. So, contrary
>> to TDC, there is no way to calculate if this should be on or off.
>>
>> (side note: I will be busy for personal reasons until this Sunday, do
>> not be surprised if my next answer is delayed).
>
> No problem.
>
> Best regards,
> Oliver
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-12-17 20:03 ` Oliver Hartkopp
2024-12-18 9:13 ` Oliver Hartkopp
@ 2025-01-30 5:42 ` Vincent Mailhol
2025-01-30 7:34 ` Marc Kleine-Budde
1 sibling, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2025-01-30 5:42 UTC (permalink / raw)
To: Oliver Hartkopp, Marc Kleine-Budde; +Cc: linux-can, Robert Nawrath
Hi Oliver and Marc,
Sorry for the late feedback… Furthermore, I am coming back with some bad
news…
On 18/12/2024 at 05:03, Oliver Hartkopp wrote:
> Hi Vincent,
>
> On 17.12.24 19:17, Vincent Mailhol wrote:
>> On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
(...)
>>>> At the moment, I am stuck on the PWM calculation. I do not know how to
>>>> derive good PWMS and PWML values out of the const ranges.
>>>>
>>>> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
>>>> topic. But getting access to this document requires a subscription
>>>> (which I do not have):
>>>>
>>>>
>>>> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-
>>>> guidelines-and-application-notes
>>>>
>>>> Do any of you have access to this document? Or do any of you know a
>>>> good
>>>> formula for the PWMS and PWML calculation?
>>>
>>> I have to check for the referenced document.
So, because I am still stuck on the PWMS and PWML calculation, I
directly reached out to the CiA guys. In short, this is what I learned:
- CiA 612-2 is a work draft, it is not available for purchase at the
moment.
- CiA 612-2 will be available for purchase once it reaches the Draft
Specification stage.
- CiA 612-2 will be published as ISO 16845-1 when ready.
- The only method to get access to the current draft right now would
be either to become a CiA member or an ISO Working Group 3 member.
This would mean convincing my employer to become a member of one of
these. None of which is feasible because I am doing all my kernel
contribution as an hobbyist; my employer has no connections with
this.
- Fun fact, when pointing out that it was not feasible to become a
member as an hobbyist, I got the reply that, I quote:
why would not you look into CAN FD where the bit timing and other
stuff is widely available especially for a hobbyist?
- Finally, when trying to negotiate to just get access to the one
relevant formula, I got replied that:
That is not one formula but many things are involved.
Unless anyone of you is a CiA or ISO WG3 member, I am afraid that we are
stuck for the moment. And even if we have access, I am not even sure if
the formula we are looking for is actually ready for use :(
One way to move forward would be to just implement the manual PWM and
have the kernel return an error if automatic mode is requested. I am
still not at ease to do the implementation of something I do not fully
comprehend.
>>> Btw. this is some code that shows the idea of how to calculate the PWM
>>> values based on the CAN clock and the XL data bitrate:
>>>
>>> if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
>>> // @5Mbit/s> pwmo = 0;
>>> pwms = 6;
>>> pwml = 14;
>>
>> one bit is 200 nano seconds.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>> = 6 + 14
>> = 20 minimum time quantum
>>
>> So PWM duration is 20 * 10 = 200 ns. So there is only one PWM per bit.
>>
>>> } else if (CanClock_mhz == 100) {
>>> // @10Mbit/s> pwmo = 0;
>>> pwms = 3;
>>> pwml = 7;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>> = 3 + 7
>> = 10 minimum time quantum
>>
>> So PWM duration is 10 * 10 = 100 ns. So here also, there is only one PWM
>> per bit.
>>
>>> } else if (CanClock_mhz == 160) {
>>> // @10Mbit/s> pwmo = 0;
>>> pwms = 2;
>>> pwml = 6;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 6.25 ns.
>>
>> pwm = pwms + pwml
>> = 2 + 6
>> = 8 minimum time quantum
>>
>> So PWM duration is 8 * 6.25 = 50 ns. This time, there are two PWM per
>> bit. Interesting.
>>> }
>>
>> So from those value, I derived:
>>
>> pwms = round_down(30% * pwm)
>> pwml = round_up(70% * pwm)
>>
>> PWMO was already established as:
>>
>> pwmo = bit_time_in_minimum_time_quantum % pwm
>>
>> (in your example, the bit time is a multiple of PWM, so the PWMO is
>> zero).
>>
>> This is some progress. What is not clear yet is how to find the number
>> of PWM per bit (i.e. why is it only one PWM per bit in your first two
>> examples, why is it two PWM per bit in your third example).
For the context, this is the precise point were I am still stuck on: how
to find out how many PWM symbols we should have in one bit.
>> The ISO has one more formula that I did not yet used. Maybe that's the
>> solution. But it is 3 a.m. here, let's say that this is a problem for
>> the tomorrow me.
>>
>>> For that reason I was thinking about some default calculation provided
>>> by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
>>> people want to set the values analog to tseg1 and friends instead of
>>> setting a single bitrate.
>>
>> Don't worry, we have the same final result in mind. It is just that I
>> believe that we can use a trick so that we can do it with a single
>> flag :)
>>
>> Let me illustrate in more details.
>>
>> Case 1: PWM off
>>
>> ip link set can0 (...) xl on pwm off
>
> Ugh.
>
> Did you take a look into all my patches from 2024-12-04, e.g.
>
> https://lore.kernel.org/linux-can/20241204075741.3727-3-
> socketcan@hartkopp.net/T/#u
>
> which is introducing CAN_CTRLMODE_XL_TRX ??
>
> We are talking about having a CAN XL capable/switchable transceiver
> attached to the CAN (XL) controller or not. IMO we should also name it
> accordingly with
>
> ip link set can0 (...) xl on xltrx off
>
> That the transceiver switching is done with PWM under the hood is
> nothing we should disclose to the user in this way IMHO.
To be honest, all my brain cycle were consumed by the PWM calculation
topic. I was not yet in a phase where I was trying to think of the best
naming.
Back to the topic, when looking again at the ISO, there are actually
three modes, c.f. this table:
CAN transceiver modes signalled PMA operating modes for CAN SIC
to the PMA sub-layer as specified XL transceivers as specified in
in CiA 610-1 & ISO 11898-2 CiA 610-3 & ISO 11898-2
Arbitration mode SIC mode
Data TX mode FAST TX mode
Data RX mode FAST RX mode
The table is also available at:
https://www.can-cia.org/can-knowledge/can-xl
(the CiA and the ISO table are the exact same thing except for the
references).
I am not exactly sure how the thing is supposed to work when you enable
only one of the data TX or RX mode, but the table seems clear about the
fact that the data TX and RX mode can opperate separatly.
And on a side topic, I do not like the naming of the transceiver mode
(the left column of the table). If I say "Data TX mode on" or "Data TX
mode off", I am just thinking of a listen only mode, not of a PWM mode.
I think it is better to use the naming of the PMA operating modes (the
right column of the table): fast_rx and fast_tx. This naming better
conveys what the thing is about.
>>
>> - CAN_CTRLMODE_XL_PWM bit is false
>> - IFLA_CAN_XL_PWM is not provided
>>
>> Case 2: PWM on auto
>>
>> ip link set can0 (...) xl on pwm on
>>
>> - CAN_CTRLMODE_XL_PWM bit is true
>> - IFLA_CAN_XL_PWM is not provided
>>
>> Case 3: PWM on manual
>>
>> ip link set can0 (...) xl on pwm on pwms xxxx pwml yyyy
>>
>> - CAN_CTRLMODE_XL_PWM bit is false
>> - IFLA_CAN_XL_PWM is provided (and contains a nest with both
>> IFLA_CAN_PWMS and IFLA_CAN_PWML)
>>
>> So, here, I use the presence or the absence of the IFLA_CAN_XL_PWM as an
>> hidden flag to know whether the calculation should be done or not.
>
> Yes, that's a good idea. Excellent default behavior.
>
> At least when reading IFLA_CAN_XL_PWM the provided or automatically
> calculated values would become visible.
>
>> Note that there is one more case:
>>
>> Case 4: full auto??? (let the kernel decide between PWM on or off)
>>
>> ip link set can0 (...) xl on
>>
>> Here, the user does not tell if PWM should be on or off. I think that
>> this case should be banned (return error).
>
> Yes! Good idea!
>
>> For what I understand, if one
>> node uses PWM and the other doesn't, it just doesn't work. So, contrary
>> to TDC, there is no way to calculate if this should be on or off.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-10 15:55 [RFC PATCH 00/14] can: netlink: add CAN XL Vincent Mailhol
` (14 preceding siblings ...)
2024-11-11 14:08 ` [RFC PATCH 00/14] can: netlink: add CAN XL Oliver Hartkopp
@ 2024-11-12 8:53 ` Marc Kleine-Budde
2024-11-12 9:24 ` Vincent Mailhol
15 siblings, 1 reply; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-11-12 8:53 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]
On 11.11.2024 00:55:49, Vincent Mailhol wrote:
> Because of popular request [1] and to celebrate the fact that I joined
> the kernel web of trust this week [2], here is an RFC to introduce the
> CAN XL netlink support.
yeay \o/
> The logic is simple. The CAN XL basically just reuse the same kind of
> parameters as CAN FD: the databittiming and the TDC. So the series is
> just a bunch of refactor to either:
>
> - factorize code.
> - rename some variable to differentiate between CAN FD and XL.
> - make some function more generic to accept both CAN FD and XL.
>
> The feature is working™: there is a dummy driver at the end of the
> series to show the traffic from the userland to a driver. This said, I
> did close to zero testing. Once I had one CAN XL frame reaching the
> driver, I call it a day, and decided to send the work. Regardless, it
> is Sunday night. If I do not send it now, that would be next week-end.
> Probably some mistakes are hidden here and there, but this should be
> enough for an RFC level.
>
> Also, I am not fully happy that can_dbt_changelink() requires 8
> parameters. I will probably revisit this later on. But for the moment,
> I think this is acceptable for an RFC.
>
> Overall, I do not want to rush this series. Linus should send the rc7
> anytime soon, and the merge window will probably start in eight days.
> I do not think this series will be finalized by then. I still need to
> give a deeper look at ISO 11898-1:2024 [3] to check that everything is
> good. However, if I receive positive feedback on this RFC, I would
> probably like to have the first patch merged so that I do not have to
> rebase that tree wide patch each time someone makes a change.
>
> I will send a second RFC series for iplink2 just after this one. Stay
> tuned!
What's the base commit of this series? It doesn't apply to
net-next/main. For a series this big, try using 'b4', it also
automatically sends the base commit.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-12 8:53 ` Marc Kleine-Budde
@ 2024-11-12 9:24 ` Vincent Mailhol
2024-11-12 10:12 ` Marc Kleine-Budde
0 siblings, 1 reply; 44+ messages in thread
From: Vincent Mailhol @ 2024-11-12 9:24 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
On 12 nov. 2024 à 17:53, Marc Kleine-Budde <mkl@pengutronix.de> a écrit :
> On 11.11.2024 00:55:49, Vincent Mailhol wrote:
> > Because of popular request [1] and to celebrate the fact that I joined
> > the kernel web of trust this week [2], here is an RFC to introduce the
> > CAN XL netlink support.
>
> yeay \o/
>
> > The logic is simple. The CAN XL basically just reuse the same kind of
> > parameters as CAN FD: the databittiming and the TDC. So the series is
> > just a bunch of refactor to either:
> >
> > - factorize code.
> > - rename some variable to differentiate between CAN FD and XL.
> > - make some function more generic to accept both CAN FD and XL.
> >
> > The feature is working™: there is a dummy driver at the end of the
> > series to show the traffic from the userland to a driver. This said, I
> > did close to zero testing. Once I had one CAN XL frame reaching the
> > driver, I call it a day, and decided to send the work. Regardless, it
> > is Sunday night. If I do not send it now, that would be next week-end.
> > Probably some mistakes are hidden here and there, but this should be
> > enough for an RFC level.
> >
> > Also, I am not fully happy that can_dbt_changelink() requires 8
> > parameters. I will probably revisit this later on. But for the moment,
> > I think this is acceptable for an RFC.
> >
> > Overall, I do not want to rush this series. Linus should send the rc7
> > anytime soon, and the merge window will probably start in eight days.
> > I do not think this series will be finalized by then. I still need to
> > give a deeper look at ISO 11898-1:2024 [3] to check that everything is
> > good. However, if I receive positive feedback on this RFC, I would
> > probably like to have the first patch merged so that I do not have to
> > rebase that tree wide patch each time someone makes a change.
> >
> > I will send a second RFC series for iplink2 just after this one. Stay
> > tuned!
>
> What's the base commit of this series? It doesn't apply to
> net-next/main. For a series this big, try using 'b4', it also
> automatically sends the base commit.
It is linux-can-next/main.
I will rebase at least the first patch and maybe some of the trivial
renaming onto net-next/main before the merge window opens. This way,
we will not have to worry about these anymore. After this, the other
patches should apply smoothly.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [RFC PATCH 00/14] can: netlink: add CAN XL
2024-11-12 9:24 ` Vincent Mailhol
@ 2024-11-12 10:12 ` Marc Kleine-Budde
0 siblings, 0 replies; 44+ messages in thread
From: Marc Kleine-Budde @ 2024-11-12 10:12 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, Oliver Hartkopp, Robert Nawrath
[-- Attachment #1: Type: text/plain, Size: 885 bytes --]
On 12.11.2024 18:24:55, Vincent Mailhol wrote:
> > What's the base commit of this series? It doesn't apply to
> > net-next/main. For a series this big, try using 'b4', it also
> > automatically sends the base commit.
>
> It is linux-can-next/main.
Ohh, apparently I forgot to update it after the last PR was merged. I've
done that now.
> I will rebase at least the first patch and maybe some of the trivial
> renaming onto net-next/main before the merge window opens. This way,
> we will not have to worry about these anymore. After this, the other
> patches should apply smoothly.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread