| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02a67a6d-30fd-4b15-349d-1cd8afab13ca@quicinc.com>
Date: Fri, 3 Nov 2023 17:36:09 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
CC: <agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <robh+dt@...nel.org>,
<conor+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<richard@....at>, <vigneshr@...com>, <broonie@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>,
<linux-spi@...r.kernel.org>, <quic_srichara@...cinc.com>,
<qpic_varada@...cinc.com>
Subject: Re: [RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine
Driver
On 10/31/2023 8:58 PM, Miquel Raynal wrote:
> Hi,
>
> quic_mdalam@...cinc.com wrote on Tue, 31 Oct 2023 17:33:03 +0530:
>
> Commit log is missing.
Having a separate device node for ECC was NAK-ed
https://www.spinics.net/lists/linux-arm-msm/msg177596.html
It is fine to drop this patch ? keep ECC support inlined in both
raw nand and Serial nand driver.
>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@...cinc.com>
>
> If Sricharan is a co developer you need to use the right tags. Please
> have a look at the documentation. Using the two SoB here does not mean
> anything
Ok will fix
>
>> ---
>> drivers/mtd/nand/Kconfig | 7 ++
>> drivers/mtd/nand/Makefile | 1 +
>> drivers/mtd/nand/ecc-qcom.c | 198 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 206 insertions(+)
>> create mode 100644 drivers/mtd/nand/ecc-qcom.c
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 5b0c2c95f10c..333cec8187c8 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -61,6 +61,13 @@ config MTD_NAND_ECC_MEDIATEK
>> help
>> This enables support for the hardware ECC engine from Mediatek.
>>
>> +config MTD_NAND_ECC_QCOM
>> + tristate "Qualcomm hardware ECC engine"
>> + depends on ARCH_QCOM
>
> Same comment as Mark regarding COMPILE_TEST
Ok
>
>> + select MTD_NAND_ECC
>> + help
>> + This enables support for the hardware ECC engine from Qualcomm.
>> +
>> endmenu
>>
>> endmenu
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index 19e1291ac4d5..c73b8a3456ec 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -3,6 +3,7 @@
>> nandcore-objs := core.o bbt.o
>> obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
>> obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o
>> +obj-$(CONFIG_MTD_NAND_ECC_QCOM) += ecc-qcom.o qpic_common.o
>>
>> obj-y += onenand/
>> obj-y += raw/
>> diff --git a/drivers/mtd/nand/ecc-qcom.c b/drivers/mtd/nand/ecc-qcom.c
>> new file mode 100644
>> index 000000000000..a85423ed368a
>> --- /dev/null
>> +++ b/drivers/mtd/nand/ecc-qcom.c
>> @@ -0,0 +1,198 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * QCOM ECC Engine Driver.
>> + * Copyright (C) 2023 Qualcomm Inc.
>> + * Authors: Md sadre Alam <quic_mdalam@...cinc.com>
>> + * Sricharan R <quic_srichara@...cinc.com>
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/mutex.h>
>> +#include <linux/mtd/nand-qpic-common.h>
>> +
>> +
>> +
>> +/* ECC modes supported by the controller */
>> +#define ECC_NONE BIT(0)
>> +#define ECC_RS_4BIT BIT(1)
>> +#define ECC_BCH_4BIT BIT(2)
>> +#define ECC_BCH_8BIT BIT(3)
>> +
>> +struct qpic_ecc_caps {
>> + u32 err_mask;
>> + u32 err_shift;
>> + const u8 *ecc_strength;
>> + const u32 *ecc_regs;
>> + u8 num_ecc_strength;
>> + u8 ecc_mode_shift;
>> + u32 parity_bits;
>> + int pg_irq_sel;
>> +};
>> +
>> +
>> +struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
>> +{
>> + return container_of(chip, struct qcom_nand_host, chip);
>> +}
>> +EXPORT_SYMBOL(to_qcom_nand_host);
>> +
>> +struct qcom_nand_controller *
>> +get_qcom_nand_controller(struct nand_chip *chip)
>> +{
>> + return container_of(chip->controller, struct qcom_nand_controller,
>> + controller);
>> +}
>> +EXPORT_SYMBOL(get_qcom_nand_controller);
>> +
>> +static struct qpic_ecc *qpic_ecc_get(struct device_node *np)
>> +{
>> + struct platform_device *pdev;
>> + struct qpic_ecc *ecc;
>> +
>> + pdev = of_find_device_by_node(np);
>> + if (!pdev)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + ecc = platform_get_drvdata(pdev);
>> + if (!ecc) {
>> + put_device(&pdev->dev);
>> + return ERR_PTR(-EPROBE_DEFER);
>> + }
>> +
>> + return ecc;
>> +}
>> +
>> +struct qpic_ecc *of_qpic_ecc_get(struct device_node *of_node)
>> +{
>> + struct qpic_ecc *ecc = NULL;
>> + struct device_node *np;
>> +
>> + np = of_parse_phandle(of_node, "nand-ecc-engine", 0);
>> + /* for backward compatibility */
>
> There is no backward compatibility to handle upstream
Ok will fix in V1
>
>> + if (!np)
>> + np = of_parse_phandle(of_node, "ecc-engine", 0);
>> + if (np) {
>> + ecc = qpic_ecc_get(np);
>> + of_node_put(np);
>> + }
>> +
>> + return ecc;
>> +}
>> +EXPORT_SYMBOL(of_qpic_ecc_get);
>> +
>> +int qcom_ecc_config(struct qpic_ecc *ecc, int ecc_strength,
>> + bool wide_bus)
>> +{
>> + ecc->ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT);
>> +
>> + if (ecc_strength >= 8) {
>
> If your engine does not support more than an 8-bit strength this
> condition seems a bit strange.
Max ECC supported 8-bit only, forcing it to 8-bit.
>
>> + /* 8 bit ECC defaults to BCH ECC on all platforms */
>> + ecc->bch_enabled = true;
>> + ecc->ecc_mode = 1;
>
> ecc_modes above, ecc_mode here, not very clear what this is.
> Please give meaningful names to your variables, not just the bit name
> that this is capturing because here it's unclear what this is.
ok will fix in V1
>
>> +
>> + if (wide_bus) {
>> + ecc->ecc_bytes_hw = 14;
>> + ecc->spare_bytes = 0;
>
> Spare bytes depend on the flash, you can't use constant values like
> that.
Ok will fix in V1
>
> I also don't understand what wide_bus is and why it has an impact of
> only 1 on the number of ECC bytes. Please make all this more explicit.
wide_bus 1 means 16-bit wide and wide_bus 0 means 8-bit wide.
there different configuration for ecc config for 16-bit wide bus
and 8-bit wide bus. This is recommended configuration by IP team,
Will reconfirm this with IP folks and come back.
Regards,
Alam.
Powered by blists - more mailing lists