Re: [PATCH v5 5/7] spi: spi-qpic: add driver for QCOM SPI NAND flash Interface

[Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>]

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