From: Md Sadre Alam <quic_mdalam@quicinc.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: <broonie@kernel.org>, <robh@kernel.org>, <krzk+dt@kernel.org>,
<conor+dt@kernel.org>, <andersson@kernel.org>,
<konrad.dybcio@linaro.org>, <richard@nod.at>, <vigneshr@ti.com>,
<manivannan.sadhasivam@linaro.org>,
<linux-arm-msm@vger.kernel.org>, <linux-spi@vger.kernel.org>,
<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-mtd@lists.infradead.org>, <quic_srichara@quicinc.com>,
<quic_varada@quicinc.com>,
Alexandru Gagniuc <mr.nuke.me@gmail.com>
Subject: Re: [PATCH v5 5/7] spi: spi-qpic: add driver for QCOM SPI NAND flash Interface
Date: Mon, 20 May 2024 20:55:58 +0530 [thread overview]
Message-ID: <05657afe-0c09-a199-00bb-abc864d78780@quicinc.com> (raw)
In-Reply-To: <20240520144334.79c754e3@xps-13>
On 5/20/2024 6:13 PM, Miquel Raynal wrote:
> Hi,
>
>>>> +static int qcom_spi_io_op(struct qcom_nand_controller *snandc, const struct spi_mem_op *op)
>>>> +{
>>>> + int ret, val, opcode;
>>>> + bool copy = false, copy_ftr = false;
>>>> +
>>>> + ret = qcom_spi_send_cmdaddr(snandc, op);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + snandc->buf_count = 0;
>>>> + snandc->buf_start = 0;
>>>> + qcom_clear_read_regs(snandc);
>>>> + qcom_clear_bam_transaction(snandc);
>>>> + opcode = op->cmd.opcode;
>>>> +
>>>> + switch (opcode) {
>>>> + case SPINAND_READID:
>>>> + snandc->buf_count = 4;
>>>> + qcom_read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL);
>>>> + copy = true;
>>>> + break;
>>>> + case SPINAND_GET_FEATURE:
>>>> + snandc->buf_count = 4;
>>>> + qcom_read_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL);
>>>> + copy_ftr = true;
>>>> + break;
>>>> + case SPINAND_SET_FEATURE:
>>>> + snandc->regs->flash_feature = *(u32 *)op->data.buf.out;
>>>> + qcom_write_reg_dma(snandc, &snandc->regs->flash_feature,
>>>> + NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL);
>>>> + break;
>>>> + default:
>>>> + return 0;
>>>
>>> No error state?
>> We can't return return error here , since this API is not for checking supported command.
>
> I no longer remember exactly where this is called, but if there are
> possible unhandled cases, I want an error to be returned.
Ok
>
>> We can return error only if we submitted the descriptor. That already we are handling.
>
> ...
>
>>>> --- a/include/linux/mtd/nand-qpic-common.h
>>>> +++ b/include/linux/mtd/nand-qpic-common.h
>>>> @@ -315,11 +315,56 @@ struct nandc_regs {
>>>> __le32 read_location_last1;
>>>> __le32 read_location_last2;
>>>> __le32 read_location_last3;
>>>> + __le32 spi_cfg;
>>>> + __le32 num_addr_cycle;
>>>> + __le32 busy_wait_cnt;
>>>> + __le32 flash_feature;
>>>> >> __le32 erased_cw_detect_cfg_clr;
>>>> __le32 erased_cw_detect_cfg_set;
>>>> };
>>>> >> +/*
>>>> + * ECC state struct
>>>> + * @corrected: ECC corrected
>>>> + * @bitflips: Max bit flip
>>>> + * @failed: ECC failed
>>>> + */
>>>> +struct qcom_ecc_stats {
>>>> + u32 corrected;
>>>> + u32 bitflips;
>>>> + u32 failed;
>>>> +};
>>>> +
>>>> +struct qpic_ecc {
>>>> + struct device *dev;
>>>> + const struct qpic_ecc_caps *caps;
>>>> + struct completion done;
>>>> + u32 sectors;
>>>> + u8 *eccdata;
>>>> + bool use_ecc;
>>>> + u32 ecc_modes;
>>>> + int ecc_bytes_hw;
>>>> + int spare_bytes;
>>>> + int bbm_size;
>>>> + int ecc_mode;
>>>> + int bytes;
>>>> + int steps;
>>>> + int step_size;
>>>> + int strength;
>>>> + int cw_size;
>>>> + int cw_data;
>>>> + u32 cfg0, cfg1;
>>>> + u32 cfg0_raw, cfg1_raw;
>>>> + u32 ecc_buf_cfg;
>>>> + u32 ecc_bch_cfg;
>>>> + u32 clrflashstatus;
>>>> + u32 clrreadstatus;
>>>> + bool bch_enabled;
>>>> +};
>>>> +
>>>> +struct qpic_ecc;
>>>> +
>>>> /*
>>>> * NAND controller data struct
>>>> *
>>>> @@ -329,6 +374,7 @@ struct nandc_regs {
>>>> *
>>>> * @core_clk: controller clock
>>>> * @aon_clk: another controller clock
>>>> + * @iomacro_clk: io macro clock
>>>> *
>>>> * @regs: a contiguous chunk of memory for DMA register
>>>> * writes. contains the register values to be
>>>> @@ -338,6 +384,7 @@ struct nandc_regs {
>>>> * initialized via DT match data
>>>> *
>>>> * @controller: base controller structure
>>>> + * @ctlr: spi controller structure
>>>> * @host_list: list containing all the chips attached to the
>>>> * controller
>>>> *
>>>> @@ -375,6 +422,7 @@ struct qcom_nand_controller {
>>>> >> struct clk *core_clk;
>>>> struct clk *aon_clk;
>>>> + struct clk *iomacro_clk;
>>>> >> struct nandc_regs *regs;
>>>> struct bam_transaction *bam_txn;
>>>> @@ -382,6 +430,7 @@ struct qcom_nand_controller {
>>>> const struct qcom_nandc_props *props;
>>>> >> struct nand_controller controller;
>>>> + struct spi_controller *ctlr;
>>>> struct list_head host_list;
>>>> >> union {
>>>> @@ -418,6 +467,21 @@ struct qcom_nand_controller {
>>>> >> u32 cmd1, vld;
>>>> bool exec_opwrite;
>>>> + struct qpic_ecc *ecc;
>>>> + struct qcom_ecc_stats ecc_stats;
>>>> + struct nand_ecc_engine ecc_eng;
>>>> + u8 *data_buf;
>>>> + u8 *oob_buf;
>>>> + u32 wlen;
>>>> + u32 addr1;
>>>> + u32 addr2;
>>>> + u32 cmd;
>>>> + u32 num_cw;
>>>> + u32 pagesize;
>>>> + bool oob_rw;
>>>> + bool page_rw;
>>>> + bool raw_rw;
>>>> + bool read_last_cw;
>>>> };
>>>
>>> If all these definitions are only used by the spi controller, I don't
>>> see why you want to put them in the common file.
>> We are using qcom_nand_controller{..} structure as common b/w raw nand
>> and spi nand. These all variables will be used by spi nand only , but
>> qcom_nand_controller structure is passed across all the SPI API, thats
>> why define these all variables inside qcom_nand_controller structure.
>> so that i can access directlty.
>
> Maybe you can move the spi-nand specific variables in a struct, and the
> raw NAND specific variables in another, and then use an enum in this
> structure. This way only the useful fields are available. Or maybe you
> can have two pointers and only populate the relevant one from the
> relevant driver with the fields that are missing. But this is a generic
> include, so don't put specific fields there just because it is
> convenient.
Ok , will do in next patch.
>
> Thanks,
> Miquèl
next prev parent reply other threads:[~2024-05-20 15:26 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 8:36 [PATCH v5 0/7] Add QPIC SPI NAND driver Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 1/7] spi: dt-bindings: Introduce qcom,spi-qpic-snand Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 2/7] mtd: rawnand: qcom: cleanup qcom_nandc driver Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 3/7] mtd: rawnand: qcom: Add qcom prefix to common api Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 4/7] drivers: mtd: nand: Add qpic_common API file Md Sadre Alam
2024-05-16 12:37 ` Miquel Raynal
2024-05-20 9:51 ` Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 5/7] spi: spi-qpic: add driver for QCOM SPI NAND flash Interface Md Sadre Alam
2024-05-16 12:56 ` Miquel Raynal
2024-05-20 10:07 ` Md Sadre Alam
2024-05-20 12:43 ` Miquel Raynal
2024-05-20 15:25 ` Md Sadre Alam [this message]
2024-05-08 8:36 ` [PATCH v5 6/7] arm64: dts: qcom: ipq9574: Add SPI nand support Md Sadre Alam
2024-05-08 8:36 ` [PATCH v5 7/7] arm64: dts: qcom: ipq9574: Disable eMMC node Md Sadre Alam
2024-05-16 12:59 ` Miquel Raynal
2024-05-20 10:15 ` Md Sadre Alam
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=05657afe-0c09-a199-00bb-abc864d78780@quicinc.com \
--to=quic_mdalam@quicinc.com \
--cc=andersson@kernel.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=miquel.raynal@bootlin.com \
--cc=mr.nuke.me@gmail.com \
--cc=quic_srichara@quicinc.com \
--cc=quic_varada@quicinc.com \
--cc=richard@nod.at \
--cc=robh@kernel.org \
--cc=vigneshr@ti.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 read-only IMAP folder(s) and NNTP newsgroup(s).