> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: 25 October 2022 13:46
> To: Vivek Yadav <vivek.2311@xxxxxxxxxxx>
> Cc: rcsekar@xxxxxxxxxxx; wg@xxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> pankaj.dubey@xxxxxxxxxxx; ravi.patel@xxxxxxxxxxx;
> alim.akhtar@xxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 6/7] can: m_can: Add ECC functionality for message RAM
>
> On 21.10.2022 15:28:32, Vivek Yadav wrote:
> > Whenever MCAN Buffers and FIFOs are stored on message ram, there are
> RAM
> > inherent risks of corruption known as single-bit errors.
> >
> > Enable error correction code (ECC) data intigrity check for Message
> > RAM to create valid ECC checksums.
> >
> > ECC uses a respective number of bits, which are added to each word as
> > a parity and that will raise the error signal on the corruption in the
> > Interrupt Register(IR).
> >
> > Message RAM bit error controlled by input signal m_can_aeim_berr[0],
> > generated by an optional external parity / ECC logic attached to the
> > Message RAM.
> >
I will remove this text from commit as this indicates the handling of ECC error.
> > This indicates either Bit Error detected and Corrected(BEC) or No bit
> > error detected when reading from Message RAM.
>
> There is no ECC error handler added to the code.
>
Yes, we are not adding the ECC error handler in the code.
As per my understanding this is already handled in <m_can_handle_other_err> function.
> > Signed-off-by: Vivek Yadav <vivek.2311@xxxxxxxxxxx>
> > ---
> > drivers/net/can/m_can/m_can.c | 73
> > +++++++++++++++++++++++++++++++++++
> > drivers/net/can/m_can/m_can.h | 24 ++++++++++++
> > 2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index dcb582563d5e..578146707d5b
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1535,9 +1535,62 @@ static void m_can_stop(struct net_device *dev)
> > cdev->can.state = CAN_STATE_STOPPED; }
> >
> > +int m_can_config_mram_ecc_check(struct m_can_classdev *cdev, int
> > +enable,
> static ^^^ bool
> > + struct device_node *np)
> > +{
> > + int val = 0;
> > + int offset = 0, ret = 0;
> > + int delay_count = MRAM_INIT_TIMEOUT;
> > + struct m_can_mraminit *mraminit = &cdev->mraminit_sys;
>
> Please sort by reverse Christmas tree.
>
Okay, I will address this in next patch series.
> > +
> > + mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
> > + "mram-ecc-cfg");
>
> Please check if syscon_regmap_lookup_by_phandle_args() is better suited.
>
Okay, I will check and make it better.
> You probably want to do the syscon lookup once during
> m_can_class_register().
>
Yes, It should be once only, I will address this.
> > + if (IS_ERR(mraminit->syscon)) {
> > + /* can fail with -EPROBE_DEFER */
>
> m_can_config_mram_ecc_check() is called from m_can_open() and
> m_can_close(), returning -EPROBE_DEFER makes no sense there.
>
> > + ret = PTR_ERR(mraminit->syscon);
> > + return ret;
> > + }
> > +
> > + if (of_property_read_u32_index(np, "mram-ecc-cfg", 1,
> > + &mraminit->reg)) {
> > + dev_err(cdev->dev, "couldn't get the mraminit reg.
> offset!\n");
> > + return -ENODEV;
> > + }
> > +
> > + val = ((MRAM_ECC_ENABLE_MASK | MRAM_CFG_VALID_MASK |
> > + MRAM_INIT_ENABLE_MASK) << offset);
>
> please make use of FIELD_PREP
>
Okay, I will do.
> > + regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
> > +
> > + if (enable) {
> > + val = (MRAM_ECC_ENABLE_MASK |
> MRAM_INIT_ENABLE_MASK) << offset;
>
> same here
>
okay
> > + regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> > + }
> > +
> > + /* after enable or disable valid flag need to be set*/
> ^^^
> missing space
> > + val = (MRAM_CFG_VALID_MASK << offset);
>
> here, too
>
okay
> > + regmap_set_bits(mraminit->syscon, mraminit->reg, val);
> > +
> > + if (enable) {
> > + do {
> > + regmap_read(mraminit->syscon, mraminit->reg,
> &val);
> > +
> > + if (val & (MRAM_INIT_DONE_MASK << offset))
> > + return 0;
> > +
> > + udelay(1);
> > + } while (delay_count--);
>
> please make use of regmap_read_poll_timeout().
>
Okay, I will add this in next patch series.
> > +
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int m_can_close(struct net_device *dev) {
> > struct m_can_classdev *cdev = netdev_priv(dev);
> > + struct device_node *np;
> > + int err = 0;
> >
> > netif_stop_queue(dev);
> >
> > @@ -1557,6 +1610,15 @@ static int m_can_close(struct net_device *dev)
> > if (cdev->is_peripheral)
> > can_rx_offload_disable(&cdev->offload);
> >
> > + np = cdev->dev->of_node;
> > +
> > + if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> > + err = m_can_config_mram_ecc_check(cdev, ECC_DISABLE,
> np);
> > + if (err < 0)
> > + netdev_err(dev,
> > + "Message RAM ECC disable config
> failed\n");
> > + }
> > +
> > close_candev(dev);
> >
> > phy_power_off(cdev->transceiver);
> > @@ -1754,6 +1816,7 @@ static int m_can_open(struct net_device *dev) {
> > struct m_can_classdev *cdev = netdev_priv(dev);
> > int err;
> > + struct device_node *np;
> >
> > err = phy_power_on(cdev->transceiver);
> > if (err)
> > @@ -1798,6 +1861,16 @@ static int m_can_open(struct net_device *dev)
> > goto exit_irq_fail;
> > }
> >
> > + np = cdev->dev->of_node;
> > +
> > + if (np && of_property_read_bool(np, "mram-ecc-cfg")) {
> > + err = m_can_config_mram_ecc_check(cdev, ECC_ENABLE,
> np);
> > + if (err < 0) {
> > + netdev_err(dev,
> > + "Message RAM ECC enable config
> failed\n");
> > + }
> > + }
> > +
> > /* start the m_can controller */
> > m_can_start(dev);
> >
> > diff --git a/drivers/net/can/m_can/m_can.h
> > b/drivers/net/can/m_can/m_can.h index 4c0267f9f297..3cbfdc64a7db
> > 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -28,6 +28,8 @@
> > #include <linux/can/dev.h>
> > #include <linux/pinctrl/consumer.h>
> > #include <linux/phy/phy.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
>
> please make a separate patch that sorts these includes alphabetically, then
> add the new includes sorted.
>
Okay, I will post the separate patch for this.
> >
> > /* m_can lec values */
> > enum m_can_lec_type {
> > @@ -52,12 +54,33 @@ enum m_can_mram_cfg {
> > MRAM_CFG_NUM,
> > };
> >
> > +enum m_can_ecc_cfg {
> > + ECC_DISABLE = 0,
> > + ECC_ENABLE,
> > +};
> > +
>
> unused
>
Okay, I will remove or make better use of this.
> > /* address offset and element number for each FIFO/Buffer in the
> > Message RAM */ struct mram_cfg {
> > u16 off;
> > u8 num;
> > };
> >
> > +/* MRAM_INIT_BITS */
> > +#define MRAM_CFG_VALID_SHIFT 5
> > +#define MRAM_CFG_VALID_MASK BIT(MRAM_CFG_VALID_SHIFT)
> > +#define MRAM_ECC_ENABLE_SHIFT 3
> > +#define MRAM_ECC_ENABLE_MASK BIT(MRAM_ECC_ENABLE_SHIFT)
> > +#define MRAM_INIT_ENABLE_SHIFT 1
> > +#define MRAM_INIT_ENABLE_MASK BIT(MRAM_INIT_ENABLE_SHIFT)
> > +#define MRAM_INIT_DONE_SHIFT 0
> > +#define MRAM_INIT_DONE_MASK BIT(MRAM_INIT_DONE_SHIFT)
> > +#define MRAM_INIT_TIMEOUT 50
>
> Please move these bits to the m_can.c file.
>
Okay, I will move.
> Add a common prefix M_CAN_ to them.
>
Okay, I will address this in next patch series.
> Remove the SHIFT values, directly define the MASK using BIT() (for single set
> bits) or GEN_MASK() (for multiple set bits).
>
> > +
> > +struct m_can_mraminit {
>
> Is this RAM init or ECC related?
>
This is for ECC only, I will give better naming for this for better understanding.
> > + struct regmap *syscon; /* for mraminit ctrl. reg. access */
> > + unsigned int reg; /* register index within syscon */
> > +};
> > +
> > struct m_can_classdev;
> > struct m_can_ops {
> > /* Device specific call backs */
> > @@ -92,6 +115,7 @@ struct m_can_classdev {
> > int pm_clock_support;
> > int is_peripheral;
> >
> > + struct m_can_mraminit mraminit_sys; /* mraminit via syscon
> regmap */
> > struct mram_cfg mcfg[MRAM_CFG_NUM];
> > };
> >
> > --
> > 2.17.1
> >
> >
>
> Marc
>
Thank you for your feedback and reviewing the patches.
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux |
> https://protect2.fireeye.com/v1/url?k=7f1e79b1-1e956c87-7f1ff2fe-
> 74fe485cbff1-92aa04a06e5e6383&q=1&e=543e935e-4838-4692-b1da-
> d42741c9ad3f&u=https%3A%2F%2Fwww.pengutronix.de%2F |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
