From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vivek Yadav <vivek.2311@samsung.com>
Cc: rcsekar@samsung.com, wg@grandegger.com, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
pankaj.dubey@samsung.com, ravi.patel@samsung.com,
alim.akhtar@samsung.com, linux-can@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] can: m_can: Add ECC functionality for message RAM
Date: Tue, 25 Oct 2022 10:16:10 +0200 [thread overview]
Message-ID: <20221025081610.h6bjg6nuqdhkupvg@pengutronix.de> (raw)
In-Reply-To: <20221021095833.62406-7-vivek.2311@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 7320 bytes --]
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.
>
> 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.
> Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
> ---
> 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.
> +
> + mraminit->syscon = syscon_regmap_lookup_by_phandle(np,
> + "mram-ecc-cfg");
Please check if syscon_regmap_lookup_by_phandle_args() is better suited.
You probably want to do the syscon lookup once during
m_can_class_register().
> + 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
> + regmap_clear_bits(mraminit->syscon, mraminit->reg, val);
> +
> + if (enable) {
> + val = (MRAM_ECC_ENABLE_MASK | MRAM_INIT_ENABLE_MASK) << offset;
same here
> + 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
> + 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().
> +
> + 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.
>
> /* 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
> /* 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.
Add a common prefix M_CAN_ to them.
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?
> + 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
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2022-10-25 8:16 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20221021102614epcas5p18bcb932e697a378a8244bd91065c5496@epcas5p1.samsung.com>
2022-10-21 9:58 ` [PATCH 0/7] can: mcan: Add MCAN support for FSD SoC Vivek Yadav
[not found] ` <CGME20221021102621epcas5p3f10b063ae972bee587eb5beb6e1b5617@epcas5p3.samsung.com>
2022-10-21 9:58 ` [PATCH 1/7] dt-bindings: Document the SYSREG specific compatibles found on " Vivek Yadav
[not found] ` <CGME20221021102625epcas5p4e1c5900b9425e41909e82072f2c1713d@epcas5p4.samsung.com>
2022-10-21 9:58 ` [PATCH 2/7] dt-bindings: can: mcan: Add ECC functionality to message ram Vivek Yadav
2022-10-25 7:24 ` Marc Kleine-Budde
2022-11-09 8:47 ` Vivek Yadav
2022-10-25 7:31 ` Marc Kleine-Budde
2022-11-09 8:52 ` Vivek Yadav
[not found] ` <CGME20221021102628epcas5p1ecf91523d9db65b066bc4f2cb693ea45@epcas5p1.samsung.com>
2022-10-21 9:58 ` [PATCH 3/7] arm64: dts: fsd: add sysreg device node Vivek Yadav
2022-11-09 11:13 ` Krzysztof Kozlowski
[not found] ` <CGME20221021102632epcas5p29333840201aacbae42bc90f651ac85cd@epcas5p2.samsung.com>
2022-10-21 9:58 ` [PATCH 4/7] can: mcan: enable peripheral clk to access mram Vivek Yadav
2022-10-25 7:44 ` Marc Kleine-Budde
2022-11-09 9:55 ` Vivek Yadav
[not found] ` <CGME20221021102635epcas5p33623e6b6ed02d3fb663da9ec253585ad@epcas5p3.samsung.com>
2022-10-21 9:58 ` [PATCH 5/7] arm64: dts: fsd: Add MCAN device node Vivek Yadav
2022-10-25 7:44 ` Marc Kleine-Budde
2022-11-09 9:16 ` Vivek Yadav
[not found] ` <CGME20221021102639epcas5p2241192d3ac873d1262372eeae948b401@epcas5p2.samsung.com>
2022-10-21 9:58 ` [PATCH 6/7] can: m_can: Add ECC functionality for message RAM Vivek Yadav
2022-10-21 15:28 ` kernel test robot
2022-10-25 8:16 ` Marc Kleine-Budde [this message]
2022-11-09 9:59 ` Vivek Yadav
[not found] ` <CGME20221021102642epcas5p24fec1e1e90632f6d9e768f7d25dc5480@epcas5p2.samsung.com>
2022-10-21 9:58 ` [PATCH 7/7] arm64: dts: fsd: Add support for error correction code " Vivek Yadav
2022-10-25 7:32 ` [PATCH 0/7] can: mcan: Add MCAN support for FSD SoC Marc Kleine-Budde
2022-11-09 8:55 ` Vivek Yadav
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221025081610.h6bjg6nuqdhkupvg@pengutronix.de \
--to=mkl@pengutronix.de \
--cc=alim.akhtar@samsung.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pankaj.dubey@samsung.com \
--cc=ravi.patel@samsung.com \
--cc=rcsekar@samsung.com \
--cc=vivek.2311@samsung.com \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).