by Abhishek Sahu

[permalink] [raw]

Subject: Re: [PATCH v4 13/20] mtd: nand: qcom: support for different DEV_CMD register offsets

On 2017-08-16 11:22, Archit Taneja wrote:
> On 08/11/2017 05:09 PM, Abhishek Sahu wrote:
>> The FLASH_DEV_CMD registers starting offset is not same in
>> different QPIC NAND controller versions. This patch adds
>> the starting offset in NAND controller properties and uses
>> the same for calculating the actual offset of these registers.
>>
>> Signed-off-by: Abhishek Sahu <[email protected]>
>> ---
>> drivers/mtd/nand/qcom_nandc.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/qcom_nandc.c
>> b/drivers/mtd/nand/qcom_nandc.c
>> index 85fbe00..c0c140b 100644
>> --- a/drivers/mtd/nand/qcom_nandc.c
>> +++ b/drivers/mtd/nand/qcom_nandc.c
>> @@ -193,6 +193,9 @@
>> ((size) << READ_LOCATION_SIZE) | \
>> ((is_last) << READ_LOCATION_LAST))
>> +/* Returns the actual register address for NAND_FLASH_DEV_* */
>
> There aren't any registers starting with NAND_FLASH_DEV_* in the
> registers
> defined above, it might get confusing for someone who doesn't have
> access
> to the HW docs. Could you explicitly mention in this comment all the
> register
> names that are required to go through this translation, it should make
> things
> more readable. With that:

Thanks Archit. Following are the registers

#define NAND_DEV_CMD0 0xa0
#define NAND_DEV_CMD1 0xa4
#define NAND_DEV_CMD2 0xa8
#define NAND_DEV_CMD_VLD 0xac

I will update the comment.

>
> Reviewed-by: Archit Taneja <[email protected]>
>
> Thanks,
> Archit
>
>
>> +#define nandc_dev_addr(nandc, reg) ((nandc)->props->flash_dev_offset
>> + (reg))
>> +
>> #define QPIC_PER_CW_CMD_SGL 32
>> #define QPIC_PER_CW_DATA_SGL 8
>> @@ -426,10 +429,12 @@ struct qcom_nand_host {
>> * among different NAND controllers.
>> * @ecc_modes - ecc mode for NAND
>> * @is_bam - whether NAND controller is using BAM
>> + * @flash_dev_offset - NAND_FLASH_DEV_* registers start offset
>> */
>> struct qcom_nandc_props {
>> u32 ecc_modes;
>> bool is_bam;
>> + u32 flash_dev_offset;
>> };
>> /* Frees the BAM transaction memory */
>> @@ -844,6 +849,9 @@ static int read_reg_dma(struct
>> qcom_nand_controller *nandc, int first,
>> if (first == NAND_READ_ID || first == NAND_FLASH_STATUS)
>> flow_control = true;
>> + if (first == NAND_DEV_CMD_VLD || first == NAND_DEV_CMD1)
>> + first = nandc_dev_addr(nandc, first);
>> +
>> size = num_regs * sizeof(u32);
>> vaddr = nandc->reg_read_buf + nandc->reg_read_pos;
>> nandc->reg_read_pos += num_regs;
>> @@ -881,11 +889,11 @@ static int write_reg_dma(struct
>> qcom_nand_controller *nandc, int first,
>> if (first == NAND_EXEC_CMD)
>> flags |= NAND_BAM_NWD;
>> - if (first == NAND_DEV_CMD1_RESTORE)
>> - first = NAND_DEV_CMD1;
>> + if (first == NAND_DEV_CMD1_RESTORE || first == NAND_DEV_CMD1)
>> + first = nandc_dev_addr(nandc, NAND_DEV_CMD1);
>> - if (first == NAND_DEV_CMD_VLD_RESTORE)
>> - first = NAND_DEV_CMD_VLD;
>> + if (first == NAND_DEV_CMD_VLD_RESTORE || first == NAND_DEV_CMD_VLD)
>> + first = nandc_dev_addr(nandc, NAND_DEV_CMD_VLD);
>> size = num_regs * sizeof(u32);
>> @@ -2492,7 +2500,8 @@ static int qcom_nandc_setup(struct
>> qcom_nand_controller *nandc)
>> /* kill onenand */
>> nandc_write(nandc, SFLASHC_BURST_CFG, 0);
>> - nandc_write(nandc, NAND_DEV_CMD_VLD, NAND_DEV_CMD_VLD_VAL);
>> + nandc_write(nandc, nandc_dev_addr(nandc, NAND_DEV_CMD_VLD),
>> + NAND_DEV_CMD_VLD_VAL);
>> /* enable ADM or BAM DMA */
>> if (nandc->props->is_bam) {
>> @@ -2503,7 +2512,7 @@ static int qcom_nandc_setup(struct
>> qcom_nand_controller *nandc)
>> }
>> /* save the original values of these registers */
>> - nandc->cmd1 = nandc_read(nandc, NAND_DEV_CMD1);
>> + nandc->cmd1 = nandc_read(nandc, nandc_dev_addr(nandc,
>> NAND_DEV_CMD1));
>> nandc->vld = NAND_DEV_CMD_VLD_VAL;
>> return 0;
>> @@ -2752,6 +2761,7 @@ static int qcom_nandc_remove(struct
>> platform_device *pdev)
>> static const struct qcom_nandc_props ipq806x_nandc_props = {
>> .ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT),
>> .is_bam = false,
>> + .flash_dev_offset = 0x0,
>> };
>> /*
>>