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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

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.

>    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.

Thanks,
Miquèl