[PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

Hal Feng hal.feng at linux.starfivetech.com
Tue Oct 15 02:30:50 PDT 2024


On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> Hi Hal,
> 
> A few more comments on top of what Andrew already wrote.
> 
> On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng at starfivetech.com> wrote:
>> From: William Qiu <william.qiu at starfivetech.com>
>>
>> Add driver for CAST CAN Bus Controller used on
>> StarFive JH7110 SoC.
>>
>> Signed-off-by: William Qiu <william.qiu at starfivetech.com>
>> Co-developed-by: Hal Feng <hal.feng at starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng at starfivetech.com>
>> ---
>>  MAINTAINERS                |   8 +
>>  drivers/net/can/Kconfig    |   7 +
>>  drivers/net/can/Makefile   |   1 +
>>  drivers/net/can/cast_can.c | 936 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 952 insertions(+)
>>  create mode 100644 drivers/net/can/cast_can.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cc40a9d9b8cd..9313b1a69e48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5010,6 +5010,14 @@ S:       Maintained
>>  W:     https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
>>  F:     drivers/net/wireless/ath/carl9170/
>>
>> +CAST CAN DRIVER
>> +M:     William Qiu <william.qiu at starfivetech.com>
>> +M:     Hal Feng <hal.feng at starfivetech.com>
>> +L:     linux-can at vger.kernel.org
>> +S:     Supported
>> +F:     Documentation/devicetree/bindings/net/can/cast,can-ctrl.yaml
>> +F:     drivers/net/can/cast_can.c
>> +
>>  CAVIUM I2C DRIVER
>>  M:     Robert Richter <rric at kernel.org>
>>  S:     Odd Fixes
>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index 7f9b60a42d29..a7ae8be5876f 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -124,6 +124,13 @@ config CAN_CAN327
>>
>>           If this driver is built as a module, it will be called can327.
>>
>> +config CAN_CASTCAN
>> +       tristate "CAST CAN"
>> +       depends on ARCH_STARFIVE || COMPILE_TEST
>> +       depends on COMMON_CLK && HAS_IOMEM
>> +       help
>> +         CAST CAN driver. This driver supports both CAN and CANFD IP.
> 
> Nitpick, maybe briefly mention the module name:
> 
>   If built as a module, it will be named cast_can.

OK.

> 
>>  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 4669cd51e7bf..2f1ebd7c0efe 100644
>> --- a/drivers/net/can/Makefile
>> +++ b/drivers/net/can/Makefile
>> @@ -17,6 +17,7 @@ obj-y                         += softing/
>>  obj-$(CONFIG_CAN_AT91)         += at91_can.o
>>  obj-$(CONFIG_CAN_BXCAN)                += bxcan.o
>>  obj-$(CONFIG_CAN_CAN327)       += can327.o
>> +obj-$(CONFIG_CAN_CASTCAN)      += cast_can.o
>>  obj-$(CONFIG_CAN_CC770)                += cc770/
>>  obj-$(CONFIG_CAN_C_CAN)                += c_can/
>>  obj-$(CONFIG_CAN_CTUCANFD)     += ctucanfd/
>> diff --git a/drivers/net/can/cast_can.c b/drivers/net/can/cast_can.c
>> new file mode 100644
>> index 000000000000..020a2eaa236b
>> --- /dev/null
>> +++ b/drivers/net/can/cast_can.c
>> @@ -0,0 +1,936 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * CAST Controller Area Network Bus Controller Driver
>> + *
>> + * Copyright (c) 2022-2024 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <linux/can/dev.h>
>> +#include <linux/can/error.h>
>> +#include <linux/clk.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +#define DRIVER_NAME "cast_can"
>> +
>> +enum ccan_reg {
>> +       CCAN_RUBF       = 0x00, /* Receive Buffer Registers 0x00-0x4f */
>> +       CCAN_RUBF_ID    = 0x00,
>> +       CCAN_RBUF_CTL   = 0x04,
>> +       CCAN_RBUF_DATA  = 0x08,
>> +       CCAN_TBUF       = 0x50, /* Transmit Buffer Registers 0x50-0x97 */
>> +       CCAN_TBUF_ID    = 0x50,
>> +       CCAN_TBUF_CTL   = 0x54,
>> +       CCAN_TBUF_DATA  = 0x58,
>> +       CCAN_TTS        = 0x98, /* Transmission Time Stamp 0x98-0x9f */
>> +       CCAN_CFG_STAT   = 0xa0,
>> +       CCAN_TCMD       = 0xa1,
>> +       CCAN_TCTRL      = 0xa2,
>> +       CCAN_RCTRL      = 0xa3,
>> +       CCAN_RTIE       = 0xa4,
>> +       CCAN_RTIF       = 0xa5,
>> +       CCAN_ERRINT     = 0xa6,
>> +       CCAN_LIMIT      = 0xa7,
>> +       CCAN_S_SEG_1    = 0xa8,
>> +       CCAN_S_SEG_2    = 0xa9,
>> +       CCAN_S_SJW      = 0xaa,
>> +       CCAN_S_PRESC    = 0xab,
>> +       CCAN_F_SEG_1    = 0xac,
>> +       CCAN_F_SEG_2    = 0xad,
>> +       CCAN_F_SJW      = 0xae,
>> +       CCAN_F_PRESC    = 0xaf,
>> +       CCAN_EALCAP     = 0xb0,
>> +       CCAN_RECNT      = 0xb2,
>> +       CCAN_TECNT      = 0xb3,
>> +};
>> +
>> +enum ccan_reg_bit_mask {
>> +       CCAN_RST_MASK             = BIT(7), /* Set Reset Bit */
>> +       CCAN_FULLCAN_MASK         = BIT(4),
>> +       CCAN_FIFO_MASK            = BIT(5),
>> +       CCAN_TSONE_MASK           = BIT(2),
>> +       CCAN_TSALL_MASK           = BIT(1),
>> +       CCAN_LBMEMOD_MASK         = BIT(6), /* Set loopback external mode */
>> +       CCAN_LBMIMOD_MASK         = BIT(5), /* Set loopback internal mode */
>> +       CCAN_BUSOFF_MASK          = BIT(0),
>> +       CCAN_TTSEN_MASK           = BIT(7),
>> +       CCAN_BRS_MASK             = BIT(4), /* CAN-FD Bit Rate Switch mask */
>> +       CCAN_EDL_MASK             = BIT(5), /* Extended Data Length */
>> +       CCAN_DLC_MASK             = GENMASK(3, 0),
>> +       CCAN_TENEXT_MASK          = BIT(6),
>> +       CCAN_IDE_MASK             = BIT(7),
>> +       CCAN_RTR_MASK             = BIT(6),
>> +       CCAN_INTR_ALL_MASK        = GENMASK(7, 0), /* All interrupts enable mask */
>> +       CCAN_RIE_MASK             = BIT(7),
>> +       CCAN_RFIE_MASK            = BIT(5),
>> +       CCAN_RAFIE_MASK           = BIT(4),
>> +       CCAN_EIE_MASK             = BIT(1),
>> +       CCAN_TASCTIVE_MASK        = BIT(1),
>> +       CCAN_RASCTIVE_MASK        = BIT(2),
>> +       CCAN_TBSEL_MASK           = BIT(7), /* Message writen in STB */
>                                                          ^^^^^^
> 
> Typo: writen -> written

Will fix it later.

> 
>> +       CCAN_STBY_MASK            = BIT(5),
>> +       CCAN_TPE_MASK             = BIT(4), /* Transmit primary enable */
>> +       CCAN_TPA_MASK             = BIT(3),
>> +       CCAN_SACK_MASK            = BIT(7),
>> +       CCAN_RREL_MASK            = BIT(4),
>> +       CCAN_RSTAT_NOT_EMPTY_MASK = GENMASK(1, 0),
>> +       CCAN_RIF_MASK             = BIT(7),
>> +       CCAN_RAFIF_MASK           = BIT(4),
>> +       CCAN_RFIF_MASK            = BIT(5),
>> +       CCAN_TPIF_MASK            = BIT(3), /* Transmission Primary Interrupt Flag */
>> +       CCAN_TSIF_MASK            = BIT(2),
>> +       CCAN_EIF_MASK             = BIT(1),
>> +       CCAN_AIF_MASK             = BIT(0),
>> +       CCAN_EWARN_MASK           = BIT(7),
>> +       CCAN_EPASS_MASK           = BIT(6),
>> +       CCAN_EPIE_MASK            = BIT(5),
>> +       CCAN_EPIF_MASK            = BIT(4),
>> +       CCAN_ALIE_MASK            = BIT(3),
>> +       CCAN_ALIF_MASK            = BIT(2),
>> +       CCAN_BEIE_MASK            = BIT(1),
>> +       CCAN_BEIF_MASK            = BIT(0),
>> +       CCAN_AFWL_MASK            = BIT(6),
>> +       CCAN_EWL_MASK             = (BIT(3) | GENMASK(1, 0)),
>> +       CCAN_KOER_MASK            = GENMASK(7, 5),
>> +       CCAN_BIT_ERROR_MASK       = BIT(5),
>> +       CCAN_FORM_ERROR_MASK      = BIT(6),
>> +       CCAN_STUFF_ERROR_MASK     = GENMASK(6, 5),
>> +       CCAN_ACK_ERROR_MASK       = BIT(7),
>> +       CCAN_CRC_ERROR_MASK       = (BIT(7) | BIT(5)),
>> +       CCAN_OTH_ERROR_MASK       = GENMASK(7, 6),
>> +};
>> +
>> +/* CCAN_S/F_SEG_1 bitfield shift */
>> +#define SEG_1_SHIFT            0
>> +#define SEG_2_SHIFT            8
>> +#define SJW_SHIFT              16
>> +#define PRESC_SHIFT            24
>> +
>> +enum cast_can_type {
>> +       CAST_CAN_TYPE_CAN = 0,
>> +       CAST_CAN_TYPE_CANFD,
>> +};
>> +
>> +struct ccan_priv {
>> +       struct can_priv can;
>> +       struct napi_struct napi;
>> +       struct device *dev;
>> +       void __iomem *reg_base;
>> +       struct clk_bulk_data clks[3];
>> +       struct reset_control *resets;
>> +       u32 cantype;
>> +};
>> +
>> +struct cast_can_data {
>> +       enum cast_can_type cantype;
>> +       const struct can_bittiming_const *bittime_const;
>> +       int (*syscon_update)(struct ccan_priv *priv);
>> +};
>> +
>> +static struct can_bittiming_const ccan_bittiming_const = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 2,
>> +       .tseg1_max = 16,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 8,
>> +       .sjw_max = 4,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static struct can_bittiming_const ccan_bittiming_const_canfd = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 2,
>> +       .tseg1_max = 64,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 16,
>> +       .sjw_max = 16,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static struct can_bittiming_const ccan_data_bittiming_const_canfd = {
>> +       .name = DRIVER_NAME,
>> +       .tseg1_min = 1,
>> +       .tseg1_max = 16,
>> +       .tseg2_min = 2,
>> +       .tseg2_max = 8,
>> +       .sjw_max = 8,
>> +       .brp_min = 1,
>> +       .brp_max = 256,
>> +       .brp_inc = 1,
>> +};
>> +
>> +static inline u32 ccan_read_reg(const struct ccan_priv *priv, u8 reg)
>> +{
>> +       return ioread32(priv->reg_base + reg);
>> +}
>> +
>> +static inline void ccan_write_reg(const struct ccan_priv *priv, u8 reg, u32 value)
>> +{
>> +       iowrite32(value, priv->reg_base + reg);
>> +}
>> +
>> +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
>> +                                   enum ccan_reg reg)
>> +{
>> +       u8 reg_down;
>> +       union val {
>> +               u8 val_8[4];
>> +               u32 val_32;
>> +       } val;
>> +
>> +       reg_down = ALIGN_DOWN(reg, 4);
>> +       val.val_32 = ccan_read_reg(priv, reg_down);
>> +       return val.val_8[reg - reg_down];
>> +}
>> +
>> +static inline void ccan_write_reg_8bit(const struct ccan_priv *priv,
>> +                                      enum ccan_reg reg, u8 value)
>> +{
>> +       u8 reg_down;
>> +       union val {
>> +               u8 val_8[4];
>> +               u32 val_32;
>> +       } val;
>> +
>> +       reg_down = ALIGN_DOWN(reg, 4);
>> +       val.val_32 = ccan_read_reg(priv, reg_down);
>> +       val.val_8[reg - reg_down] = value;
>> +       ccan_write_reg(priv, reg_down, val.val_32);
>> +}
>> +
>> +static void ccan_reg_set_bits(const struct ccan_priv *priv,
>> +                             enum ccan_reg reg,
>> +                             enum ccan_reg_bit_mask bits)
>> +{
>> +       u8 val;
>> +
>> +       val = ccan_read_reg_8bit(priv, reg);
>> +       val |= bits;
>> +       ccan_write_reg_8bit(priv, reg, val);
>> +}
>> +
>> +static void ccan_reg_clear_bits(const struct ccan_priv *priv,
>> +                               enum ccan_reg reg,
>> +                               enum ccan_reg_bit_mask bits)
>> +{
>> +       u8 val;
>> +
>> +       val = ccan_read_reg_8bit(priv, reg);
>> +       val &= ~bits;
>> +       ccan_write_reg_8bit(priv, reg, val);
>> +}
>> +
>> +static void ccan_set_reset_mode(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
>> +}
>> +
>> +static int ccan_bittime_configuration(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct can_bittiming *bt = &priv->can.bittiming;
>> +       struct can_bittiming *dbt = &priv->can.data_bittiming;
>> +       u32 bittiming, data_bittiming;
>> +       u8 reset_test;
>> +
>> +       reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +
>> +       if (!(reset_test & CCAN_RST_MASK)) {
>> +               netdev_alert(ndev, "Not in reset mode, cannot set bit timing\n");
>> +               return -EPERM;
>> +       }
>> +
>> +       /* Check the bittime parameter */
>> +       if ((((int)(bt->phase_seg1 + bt->prop_seg + 1) - 2) < 0) ||
>> +           (((int)(bt->phase_seg2) - 1) < 0) ||
>> +           (((int)(bt->sjw) - 1) < 0) ||
>> +           (((int)(bt->brp) - 1) < 0))
>> +               return -EINVAL;
> 
> Here, you are checking for wraparounds. But can this really happen? We
> know for sure that:
> 
>   - bt->phase_seg1 + bt->prop_seg1 <= ccan_bittiming_const->tseg1_max
>   - bt->phase_seg1 >= ccan_bittiming_const->tseg2_min
> 
> and so on.
> 
> Unless there is a condition under which this issue can show up, remove
> the check.

OK, will drop it.

> 
>> +       bittiming = ((bt->phase_seg1 + bt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
>> +                        ((bt->phase_seg2 - 1) << SEG_2_SHIFT) |
>> +                        ((bt->sjw - 1) << SJW_SHIFT) |
>> +                        ((bt->brp - 1) << PRESC_SHIFT);
>> +
>> +       ccan_write_reg(priv, CCAN_S_SEG_1, bittiming);
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>> +               if ((((int)(dbt->phase_seg1 + dbt->prop_seg + 1) - 2) < 0) ||
>> +                   (((int)(dbt->phase_seg2) - 1) < 0) ||
>> +                   (((int)(dbt->sjw) - 1) < 0) ||
>> +                   (((int)(dbt->brp) - 1) < 0))
>> +                       return -EINVAL;
>> +
>> +               data_bittiming = ((dbt->phase_seg1 + dbt->prop_seg + 1 - 2) << SEG_1_SHIFT) |
>> +                                ((dbt->phase_seg2 - 1) << SEG_2_SHIFT) |
>> +                                ((dbt->sjw - 1) << SJW_SHIFT) |
>> +                                ((dbt->brp - 1) << PRESC_SHIFT);
> 
> Nitpick: the calculation for the bittiming and the data_bittiming is
> the same. Can you factorize this calculation in a helper function?

Yes, will add a helper function to calculate.

> 
>> +               ccan_write_reg(priv, CCAN_F_SEG_1, data_bittiming);
>> +       }
>> +
>> +       ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_RST_MASK);
>> +
>> +       netdev_dbg(ndev, "Slow bit rate: %08x\n", ccan_read_reg(priv, CCAN_S_SEG_1));
>> +       netdev_dbg(ndev, "Fast bit rate: %08x\n", ccan_read_reg(priv, CCAN_F_SEG_1));
>> +
>> +       return 0;
>> +}
>> +
>> +static int ccan_chip_start(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int err;
>> +
>> +       ccan_set_reset_mode(ndev);
>> +
>> +       err = ccan_bittime_configuration(ndev);
>> +       if (err) {
>> +               netdev_err(ndev, "Bittime setting failed!\n");
>> +               return err;
>> +       }
>> +
>> +       /* Set Almost Full Warning Limit */
>> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_AFWL_MASK);
>> +
>> +       /* Programmable Error Warning Limit = (EWL+1)*8. Set EWL=11->Error Warning=96 */
>> +       ccan_reg_set_bits(priv, CCAN_LIMIT, CCAN_EWL_MASK);
>> +
>> +       /* Interrupts enable */
>> +       ccan_write_reg_8bit(priv, CCAN_RTIE, CCAN_INTR_ALL_MASK);
>> +
>> +       /* Error Interrupts enable(Error Passive and Bus Error) */
>> +       ccan_reg_set_bits(priv, CCAN_ERRINT, CCAN_EPIE_MASK);
>> +
>> +       /* Check whether it is loopback mode or normal mode */
>> +       if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
>> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_LBMIMOD_MASK);
>> +       else
>> +               ccan_reg_clear_bits(priv, CCAN_CFG_STAT, CCAN_LBMEMOD_MASK | CCAN_LBMIMOD_MASK);
>> +
>> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +
>> +       return 0;
>> +}
>> +
>> +static int ccan_do_set_mode(struct net_device *ndev, enum can_mode mode)
>> +{
>> +       int ret;
>> +
>> +       switch (mode) {
>> +       case CAN_MODE_START:
>> +               ret = ccan_chip_start(ndev);
>> +               if (ret) {
>> +                       netdev_err(ndev, "Could not start CAN device !\n");
>> +                       return ret;
>> +               }
>> +               netif_wake_queue(ndev);
>> +               break;
>> +       default:
>> +               ret = -EOPNOTSUPP;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static void ccan_tx_interrupt(struct net_device *ndev, u8 isr)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       /* wait till transmission of the PTB or STB finished */
>> +       while (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
>> +               if (isr & CCAN_TPIF_MASK)
>> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TPIF_MASK);
>> +
>> +               if (isr & CCAN_TSIF_MASK)
>> +                       ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_TSIF_MASK);
>> +
>> +               isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
>> +       }
>> +
>> +       ndev->stats.tx_bytes += can_get_echo_skb(ndev, 0, NULL);
>> +       ndev->stats.tx_packets++;
>> +       netif_wake_queue(ndev);
>> +}
>> +
>> +static void ccan_rxfull_interrupt(struct net_device *ndev, u8 isr)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       if (isr & CCAN_RAFIF_MASK)
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK);
>> +
>> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK))
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
>> +}
>> +
>> +static enum can_state ccan_get_chip_status(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       u8 can_stat, eir;
>> +
>> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
>> +
>> +       if (can_stat & CCAN_BUSOFF_MASK)
>> +               return CAN_STATE_BUS_OFF;
>> +
>> +       if (eir & CCAN_EPASS_MASK)
>> +               return CAN_STATE_ERROR_PASSIVE;
>> +
>> +       if (eir & CCAN_EWARN_MASK)
>> +               return CAN_STATE_ERROR_WARNING;
>> +
>> +       return CAN_STATE_ERROR_ACTIVE;
>> +}
>> +
>> +static void ccan_error_interrupt(struct net_device *ndev, u8 isr, u8 eir)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct net_device_stats *stats = &ndev->stats;
>> +       struct can_frame *cf;
>> +       struct sk_buff *skb;
>> +       u8 koer, recnt = 0, tecnt = 0, can_stat = 0;
>> +
>> +       skb = alloc_can_err_skb(ndev, &cf);
>> +
>> +       koer = ccan_read_reg_8bit(priv, CCAN_EALCAP) & CCAN_KOER_MASK;
>> +       recnt = ccan_read_reg_8bit(priv, CCAN_RECNT);
>> +       tecnt = ccan_read_reg_8bit(priv, CCAN_TECNT);
>> +
>> +       /* Read CAN status */
>> +       can_stat = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
>> +
>> +       /* Bus off ---> active error mode */
>> +       if ((isr & CCAN_EIF_MASK) && priv->can.state == CAN_STATE_BUS_OFF)
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +
>> +       /* State selection */
>> +       if (can_stat & CCAN_BUSOFF_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.bus_off++;
>> +               ccan_reg_set_bits(priv, CCAN_CFG_STAT, CCAN_BUSOFF_MASK);
>> +               can_bus_off(ndev);
>> +               if (skb)
>> +                       cf->can_id |= CAN_ERR_BUSOFF;
>> +       } else if (eir & CCAN_EPASS_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.error_passive++;
>> +               if (skb) {
>> +                       cf->can_id |= CAN_ERR_CRTL;
>> +                       cf->data[1] |= (recnt > 127) ? CAN_ERR_CRTL_RX_PASSIVE : 0;
>> +                       cf->data[1] |= (tecnt > 127) ? CAN_ERR_CRTL_TX_PASSIVE : 0;
> 
> Use the CAN state thresholds from include/uapi/linux/can/error.h:
> 
>   recnt >= CAN_ERROR_PASSIVE_THRESHOLD
> 
> and so on.

Get it.

> 
> Replace the ternary operator by an if.

OK. It will be more readable.

> 
>> +                       cf->data[6] = tecnt;
>> +                       cf->data[7] = recnt;
>> +               }
>> +       } else if (eir & CCAN_EWARN_MASK) {
>> +               priv->can.state = ccan_get_chip_status(ndev);
>> +               priv->can.can_stats.error_warning++;
>> +               if (skb) {
>> +                       cf->can_id |= CAN_ERR_CRTL;
>> +                       cf->data[1] |= (recnt > 95) ? CAN_ERR_CRTL_RX_WARNING : 0;
>> +                       cf->data[1] |= (tecnt > 95) ? CAN_ERR_CRTL_TX_WARNING : 0;
> 
> Ditto with CAN_ERROR_WARNING_THRESHOLD.

Get it.

> 
>> +                       cf->data[6] = tecnt;
>> +                       cf->data[7] = recnt;
>> +               }
>> +       }
>> +
>> +       /* Check for in protocol defined error interrupt */
>> +       if (eir & CCAN_BEIF_MASK) {
>> +               if (skb)
>> +                       cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
>> +
>> +               if (koer == CCAN_BIT_ERROR_MASK) {
>> +                       stats->tx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_BIT;
>> +               } else if (koer == CCAN_FORM_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_FORM;
>> +               } else if (koer == CCAN_STUFF_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[3] = CAN_ERR_PROT_STUFF;
>> +               } else if (koer == CCAN_ACK_ERROR_MASK) {
>> +                       stats->tx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_LOC_ACK;
>> +               } else if (koer == CCAN_CRC_ERROR_MASK) {
>> +                       stats->rx_errors++;
>> +                       if (skb)
>> +                               cf->data[2] = CAN_ERR_PROT_LOC_CRC_SEQ;
>> +               }
>> +               priv->can.can_stats.bus_error++;
>> +       }
>> +
>> +       if (skb) {
>> +               stats->rx_packets++;
>> +               stats->rx_bytes += cf->can_dlc;
> 
> Do not increase the reception statistics for an error frame. Refer to:
> 
>   https://git.kernel.org/torvalds/c/676068db69b8
> 
> for the reason why.

Will fix it accordingly.

> 
>> +               netif_rx(skb);
>> +       }
>> +
>> +       netdev_dbg(ndev, "Recnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_RECNT));
>> +       netdev_dbg(ndev, "Tecnt is 0x%02x", ccan_read_reg_8bit(priv, CCAN_TECNT));
> 
> Either remove this error message or print something more explicit. The
> end-user may not know what a Recnt is. Something like this is better:
> 
>   netdev_dbg(ndev, "Rx error count: 0x%02x", ccan_read_reg_8bit(priv,
> CCAN_RECNT));
> 
> If there are a lot of errors on the but, this can create a lot of
> spam. If you decide to keep, encapsulate this in a:
> 
>   if (net_ratelimit())

I prefer removing this error message. Thanks for your suggestions.

> 
>> +}
>> +
>> +static irqreturn_t ccan_interrupt(int irq, void *dev_id)
>> +{
>> +       struct net_device *ndev = (struct net_device *)dev_id;
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       u8 isr, eir;
>> +       u8 isr_handled = 0, eir_handled = 0;
>> +
>> +       /* Read the value of interrupt status register */
>> +       isr = ccan_read_reg_8bit(priv, CCAN_RTIF);
>> +
>> +       /* Read the value of error interrupt register */
>> +       eir = ccan_read_reg_8bit(priv, CCAN_ERRINT);
>> +
>> +       /* Check for Tx interrupt and processing it */
>> +       if (isr & (CCAN_TPIF_MASK | CCAN_TSIF_MASK)) {
>> +               ccan_tx_interrupt(ndev, isr);
>> +               isr_handled |= (CCAN_TPIF_MASK | CCAN_TSIF_MASK);
>> +       }
>> +
>> +       if (isr & (CCAN_RAFIF_MASK | CCAN_RFIF_MASK)) {
>> +               ccan_rxfull_interrupt(ndev, isr);
>> +               isr_handled |= (CCAN_RAFIF_MASK | CCAN_RFIF_MASK);
>> +       }
>> +
>> +       /* Check Rx interrupt and processing the receive interrupt routine */
>> +       if (isr & CCAN_RIF_MASK) {
>> +               ccan_reg_clear_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, CCAN_RIF_MASK);
>> +
>> +               napi_schedule(&priv->napi);
>> +               isr_handled |= CCAN_RIF_MASK;
>> +       }
>> +
>> +       if ((isr & CCAN_EIF_MASK) | (eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK))) {
>> +               /* Reset EPIF and BEIF. Reset EIF */
>> +               ccan_reg_set_bits(priv, CCAN_ERRINT, eir & (CCAN_EPIF_MASK | CCAN_BEIF_MASK));
>> +               ccan_reg_set_bits(priv, CCAN_RTIF, isr & CCAN_EIF_MASK);
>> +
>> +               ccan_error_interrupt(ndev, isr, eir);
>> +
>> +               isr_handled |= CCAN_EIF_MASK;
>> +               eir_handled |= (CCAN_EPIF_MASK | CCAN_BEIF_MASK);
>> +       }
>> +
>> +       if (isr_handled == 0 && eir_handled == 0) {
>> +               netdev_err(ndev, "Unhandled interrupt!\n");
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static int ccan_open(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int ret;
>> +
>> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               netdev_err(ndev, "Failed to enable CAN clocks\n");
>> +               return ret;
>> +       }
>> +
>> +       /* Set chip into reset mode */
>> +       ccan_set_reset_mode(ndev);
>> +
>> +       /* Common open */
>> +       ret = open_candev(ndev);
>> +       if (ret)
>> +               goto clk_exit;
>> +
>> +       /* Register interrupt handler */
>> +       ret = devm_request_irq(priv->dev, ndev->irq, ccan_interrupt, IRQF_SHARED,
>> +                              ndev->name, ndev);
>> +       if (ret) {
>> +               netdev_err(ndev, "Request_irq err: %d\n", ret);
>> +               goto candev_exit;
>> +       }
>> +
>> +       ret = ccan_chip_start(ndev);
>> +       if (ret) {
>> +               netdev_err(ndev, "Could not start CAN device !\n");
> 
> Nipick: in English punctuation rules, there an no spaces before the
> exclamation mark:
> 
>   netdev_err(ndev, "Could not start CAN device!\n");
> 
> (or you may consider just removing the exclamation mark. No need to be
> so emotional for an error message).

Yeah, just drop the space and the exclamation.

> 
>> +               goto candev_exit;
>> +       }
>> +
>> +       napi_enable(&priv->napi);
>> +       netif_start_queue(ndev);
>> +
>> +       return 0;
>> +
>> +candev_exit:
>> +       close_candev(ndev);
>> +clk_exit:
>> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
>> +       return ret;
>> +}
>> +
>> +static int ccan_close(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +
>> +       netif_stop_queue(ndev);
>> +       napi_disable(&priv->napi);
>> +
>> +       ccan_set_reset_mode(ndev);
>> +       priv->can.state = CAN_STATE_STOPPED;
>> +
>> +       close_candev(ndev);
>> +       clk_bulk_disable_unprepare(ARRAY_SIZE(priv->clks), priv->clks);
>> +
>> +       return 0;
>> +}
>> +
>> +static netdev_tx_t ccan_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct canfd_frame *cf = (struct canfd_frame *)skb->data;
>> +       u32 id, ctl, addr_off = CCAN_TBUF_DATA;
>> +       int i;
>> +
>> +       if (can_dropped_invalid_skb(ndev, skb))
>> +               return NETDEV_TX_OK;
>> +
>> +       netif_stop_queue(ndev);
> 
> Does your device only allow sending one frame at a time? Doesn't it
> have a transmission queue?

The hardware has a transmission queue, but the current driver doesn't
use it. Let me use the transmission queue in the next version.

> 
>> +       /* Work in XMIT_PTB mode */
>> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_TBSEL_MASK);
>> +
>> +       ccan_reg_clear_bits(priv, CCAN_TCMD, CCAN_STBY_MASK);
>> +
>> +       id = cf->can_id & ((cf->can_id & CAN_EFF_FLAG) ? CAN_EFF_MASK : CAN_SFF_MASK);
>> +
>> +       ctl = can_fd_len2dlc(cf->len);
>> +       ctl = (cf->can_id & CAN_EFF_FLAG) ? (ctl | CCAN_IDE_MASK) : (ctl & ~CCAN_IDE_MASK);
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD && can_is_canfd_skb(skb)) {
>> +               ctl |= (cf->flags & CANFD_BRS) ? (CCAN_BRS_MASK | CCAN_EDL_MASK) : CCAN_EDL_MASK;
> 
> Replace those long ternary operations with some il/else. It is easier to read.

OK.

> 
>> +               for (i = 0; i < cf->len; i += 4) {
>> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + i)));
>> +                       addr_off += 4;
> 
> Nitpick, what about:
> 
>   ccan_write_reg(priv, CCAN_TBUF_DATA + i, *((u32 *)(cf->data + i)));
> 
> and then remove the declaration of addr_off.

Yeah, it will be clearer.

> 
>> +               }
>> +       } else {
>> +               ctl &= ~(CCAN_EDL_MASK | CCAN_BRS_MASK);
>> +
>> +               if (cf->can_id & CAN_RTR_FLAG) {
>> +                       ctl |= CCAN_RTR_MASK;
>> +               } else {
>> +                       ctl &= ~CCAN_RTR_MASK;
>> +                       ccan_write_reg(priv, addr_off, *((u32 *)(cf->data + 0)));
>> +                       ccan_write_reg(priv, addr_off + 4, *((u32 *)(cf->data + 4)));
> 
> In this else block, addr_off is just an alias of CCAN_TBUF_DATA. So,
> directly do:
> 
>   ccan_write_reg(priv, CCAN_TBUF_DATA, *((u32 *)(cf->data + 0)));
>   ccan_write_reg(priv, CCAN_TBUF_DATA + 4, *((u32 *)(cf->data + 4)));

OK.

> 
>> +               }
>> +       }
>> +
>> +       ccan_write_reg(priv, CCAN_TBUF_ID, id);
>> +       ccan_write_reg(priv, CCAN_TBUF_CTL, ctl);
>> +       ccan_reg_set_bits(priv, CCAN_TCMD, CCAN_TPE_MASK);
>> +
>> +       can_put_echo_skb(skb, ndev, 0, 0);
>> +
>> +       return NETDEV_TX_OK;
>> +}
>> +
>> +static const struct net_device_ops ccan_netdev_ops = {
>> +       .ndo_open = ccan_open,
>> +       .ndo_stop = ccan_close,
>> +       .ndo_start_xmit = ccan_start_xmit,
>> +       .ndo_change_mtu = can_change_mtu,
>> +};
> 
> Also add a struct ethtool_ops for the default timestamps:
> 
>   static const struct ethtool_ops ccan_ethtool_ops = {
>           .get_ts_info = ethtool_op_get_ts_info,
>   };
> 
> This assumes that your device does not support hardware timestamps. If
> you do have hardware timestamping support, please adjust accordingly.

Will add this struct ethtool_ops later.

> 
>> +static int ccan_rx(struct net_device *ndev)
>> +{
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       struct net_device_stats *stats = &ndev->stats;
>> +       struct canfd_frame *cf_fd;
>> +       struct can_frame *cf;
>> +       struct sk_buff *skb;
>> +       u32 can_id;
>> +       u8 dlc, control;
>> +       int i;
>> +
>> +       control = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL);
>> +       can_id = ccan_read_reg(priv, CCAN_RUBF_ID);
>> +       dlc = ccan_read_reg_8bit(priv, CCAN_RBUF_CTL) & CCAN_DLC_MASK;
>> +
>> +       if (control & CCAN_EDL_MASK)
>> +               /* allocate sk_buffer for canfd frame */
>> +               skb = alloc_canfd_skb(ndev, &cf_fd);
>> +       else
>> +               /* allocate sk_buffer for can frame */
>> +               skb = alloc_can_skb(ndev, &cf);
>> +
>> +       if (!skb) {
>> +               stats->rx_dropped++;
>> +               return 0;
>> +       }
>> +
>> +       /* Change the CANFD or CAN2.0 data into socketcan data format */
>> +       if (control & CCAN_EDL_MASK)
>> +               cf_fd->len = can_fd_dlc2len(dlc);
>> +       else
>> +               cf->can_dlc = can_cc_dlc2len(dlc);
> 
> cf->can_dlc is deprecated. Use cf->len instead.

Get it.

> 
>> +       /* Change the CANFD or CAN2.0 id into socketcan id format */
>> +       if (control & CCAN_EDL_MASK) {
>> +               cf_fd->can_id = can_id;
>> +               cf_fd->can_id = (control & CCAN_IDE_MASK) ? (cf_fd->can_id | CAN_EFF_FLAG) :
>> +                            (cf_fd->can_id & ~CAN_EFF_FLAG);
>> +       } else {
>> +               cf->can_id = can_id;
>> +               cf->can_id = (control & CCAN_IDE_MASK) ? (cf->can_id | CAN_EFF_FLAG) :
>> +                            (cf->can_id & ~CAN_EFF_FLAG);
>> +       }
> 
> struct can_frame and struct canfd_frame have overlapping fields. You
> can just have one stack variable for both and then, no need for an
> if/else here.

Get it.

> 
>> +       if (!(control & CCAN_EDL_MASK))
>> +               if (control & CCAN_RTR_MASK)
>> +                       cf->can_id |= CAN_RTR_FLAG;
>> +
>> +       if (control & CCAN_EDL_MASK) {
> 
> You are checking for if (!(control & CCAN_EDL_MASK)) and just after
> for if (control & CCAN_EDL_MASK). Factorize those two together.

OK.

> 
>> +               for (i = 0; i < cf_fd->len; i += 4)
>> +                       *((u32 *)(cf_fd->data + i)) = ccan_read_reg(priv, CCAN_RBUF_DATA + i);
>> +       } else {
>> +               /* skb reads the received datas, if the RTR bit not set */
>> +               if (!(control & CCAN_RTR_MASK)) {
>> +                       *((u32 *)(cf->data + 0)) = ccan_read_reg(priv, CCAN_RBUF_DATA);
>> +                       *((u32 *)(cf->data + 4)) = ccan_read_reg(priv, CCAN_RBUF_DATA + 4);
>> +               }
>> +       }
>> +
>> +       ccan_reg_set_bits(priv, CCAN_RCTRL, CCAN_RREL_MASK);
>> +
>> +       stats->rx_bytes += (control & CCAN_EDL_MASK) ? cf_fd->len : cf->can_dlc;
> 
> No, cf_fd->len and cf->can_dlc are the same byte (these are parts of
> an union). It is just that cf->can_dlc is deprecated and is kept to
> not break the UAPI. In the kernel it should never be used.
> 
> Here, what you have to do is just to not increase stats->rx_bytes for
> RTR frames. Try to factorize this with the other checks which you did
> above.

Get it.

> 
>> +       stats->rx_packets++;
>> +       netif_receive_skb(skb);
>> +
>> +       return 1;
> 
> Why return 1 on success and 0 on failure? The convention in the kernel
> is that 0 means success. If you really want to keep 0 for failure, at
> least make this return boolean true or boolean false, but overall, try
> to follow the return conventions.

The return value here represents the number of successfully received packets.
It is used in ccan_rx_poll() for counting the number of successfully
received packets.

> 
>> +}
>> +
>> +static int ccan_rx_poll(struct napi_struct *napi, int quota)
>> +{
>> +       struct net_device *ndev = napi->dev;
>> +       struct ccan_priv *priv = netdev_priv(ndev);
>> +       int work_done = 0;
>> +       u8 rx_status = 0;
>> +
>> +       rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
>> +
>> +       /* Clear receive interrupt and deal with all the received frames */
>> +       while ((rx_status & CCAN_RSTAT_NOT_EMPTY_MASK) && (work_done < quota)) {
>> +               work_done += ccan_rx(ndev);
>> +
>> +               rx_status = ccan_read_reg_8bit(priv, CCAN_RCTRL);
>> +       }
>> +
>> +       napi_complete(napi);
>> +       ccan_reg_set_bits(priv, CCAN_RTIE, CCAN_RIE_MASK);
>> +
>> +       return work_done;
>> +}
>> +
>> +static int ccan_driver_probe(struct platform_device *pdev)
>> +{
>> +       struct net_device *ndev;
>> +       struct ccan_priv *priv;
>> +       const struct cast_can_data *ddata;
>> +       void __iomem *addr;
>> +       int ret;
>> +
>> +       addr = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(addr)) {
>> +               ret = PTR_ERR(addr);
>> +               goto exit;
> 
> No need for goto here:
> 
>   return PTR_ERR(addr);

OK.

> 
>> +       }
>> +
>> +       ddata = of_device_get_match_data(&pdev->dev);
>> +       if (!ddata)
>> +               return -ENODEV;
>> +
>> +       ndev = alloc_candev(sizeof(struct ccan_priv), 1);
>> +       if (!ndev) {
>> +               ret = -ENOMEM;
>> +               goto exit;
> 
> Same:
> 
>   return -ENOMEM;
> 
> (this done, remove the exit label).

OK.

> 
>> +       }
>> +
>> +       priv = netdev_priv(ndev);
>> +       priv->dev = &pdev->dev;
>> +       priv->cantype = ddata->cantype;
>> +       priv->can.bittiming_const = ddata->bittime_const;
>> +
>> +       if (ddata->syscon_update) {
>> +               ret = ddata->syscon_update(priv);
>> +               if (ret)
>> +                       goto free_exit;
>> +       }
>> +
>> +       priv->clks[0].id = "apb";
>> +       priv->clks[1].id = "timer";
>> +       priv->clks[2].id = "core";
>> +
>> +       ret = devm_clk_bulk_get(&pdev->dev, ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to get CAN clocks\n");
>> +               goto free_exit;
>> +       }
>> +
>> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(priv->clks), priv->clks);
>> +       if (ret) {
>> +               ret = dev_err_probe(&pdev->dev, ret, "Failed to enable CAN clocks\n");
>> +               goto free_exit;
>> +       }
>> +
>> +       priv->resets = devm_reset_control_array_get_exclusive(&pdev->dev);
>> +       if (IS_ERR(priv->resets)) {
>> +               ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->resets),
>> +                                   "Failed to get CAN resets");
>> +               goto clk_exit;
>> +       }
>> +
>> +       ret = reset_control_deassert(priv->resets);
>> +       if (ret)
>> +               goto clk_exit;
>> +
>> +       if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
>> +               priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
>> +       } else {
>> +               priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
>> +       }
> 
> Nitpick, consider doing this:
> 
>   priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
>   if (priv->cantype == CAST_CAN_TYPE_CANFD) {
>           priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
>           priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
>   }

OK.

> 
> Also, does you hardware support dlc greater than 8 (c.f.
> CAN_CTRLMODE_CC_LEN8_DLC)?

The class CAN (CC) mode does not support, but the CAN FD mode supports.

> 
>> +       priv->reg_base = addr;
>> +       priv->can.clock.freq = clk_get_rate(priv->clks[2].clk);
>> +       priv->can.do_set_mode = ccan_do_set_mode;
>> +       ndev->irq = platform_get_irq(pdev, 0);
>> +
>> +       /* We support local echo */
>> +       ndev->flags |= IFF_ECHO;
>> +       ndev->netdev_ops = &ccan_netdev_ops;
>> +
>> +       platform_set_drvdata(pdev, ndev);
>> +       SET_NETDEV_DEV(ndev, &pdev->dev);
>> +
>> +       netif_napi_add_tx_weight(ndev, &priv->napi, ccan_rx_poll, 16);
>> +       ret = register_candev(ndev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Failed to register (err=%d)\n", ret);
> 
>>From the Linux coding style:
> 
>   Printing numbers in parentheses (%d) adds no value and should be avoided.
> 
> Ref: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#printing-kernel-messages

Will fix it accordingly.

Sorry for the late reply. Thank you for your detailed review.

Best regards,
Hal



More information about the linux-riscv mailing list