Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-24 22:06, Miquel Raynal wrote:

Hello,

mda...@codeaurora.org wrote on Wed, 24 Feb 2021 22:00:05 +0530:


On 2021-02-24 12:18, Miquel Raynal wrote:
> Hello,
>
> mda...@codeaurora.org wrote on Wed, 24 Feb 2021 10:09:48 +0530:
>
>> On 2021-02-24 01:13, mda...@codeaurora.org wrote:
>> > On 2021-02-23 22:04, Miquel Raynal wrote:
>> >> Hello,
>> >> >> Md Sadre Alam  wrote on Tue, 23 Feb 2021
>> >> 01:34:27 +0530:
>> >> >>> From QPIC version 2.0 onwards new register got added to read last
>> >> >>a new
>> >> >>> codeword. This change will add the READ_LOCATION_LAST_CW_n register.
>> >> >> Add support for this READ_LOCATION_LAST_CW_n register.
>> >> >>> >>> For first three code word READ_LOCATION_n register will be
>> >>> use.For last code word READ_LOCATION_LAST_CW_n register will be
>> >>> use.
>> >> >> "
>> >> In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
>> >> READ_LOCATION_n, while codeword 3 will be accessed through
>> >> READ_LOCATION_LAST_CW_n.
>> >> "
>> >> >> When I read my own sentence, I feel that there is something wrong.
>> >> If there are only 4 codewords, I guess a QPIC v2 is able to use
>> >> READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?
>> >> >> I guess the point of having these "last_cw_n" registers is to support
>> >> up to 8 codewords, am I wrong? If this the case, the current patch
>> >> completely fails doing that I don't get the point of such change.
>> >
>> > This register is only use to read last code word.
>> >
>> > I have address all the comments from all the previous sub sequent
>> > patches and pushed
>> > all patches in only one series.
>> >
>> > Please check.
>> >>   The registers READ_LOCATION & READ_LOCATION_LAST are not associated >> 
with number of code words.
>>   These two registers are used to access the location inside a code >> word.
>
> Ok. Can you please explain what is a location then? Or point me to a
> datasheet that explains it.

   The location is the position inside a code word.

>
> Bottom line question: why having READ_LOCATION_0, _1,... an
> READ_LOCATION_LAST_0, _1, etc?

  READ_LOCATION_0, _1,... are used to extract multiple chunks from a 
code word.


  e.g If we wanted to extract first 100 bytes from a code word then 
(0...99) READ_LOCATION_0 will be configured.
  if we wanted to extract next 100 bytes (100...199) then 
READ_LOCATION_1 will be configured.


  same way for last code word READ_LOCATION_LAST_0, _1, will be 
used.




Nice explanation, and thanks for the below figures. So I guess there
is some kind of "small SRAM" that is
directly addressable perhaps?

I think I'm fine with your series now. Just a small nit: next time you
send a series, please update the version number "[PATCH v6]"
(automatically added with the -v6 parameter in git-format-patch). But
no need to resend just for that.



  Thanks Miquel. So now no need to test these patches further. I have 
already tested these patches on IPQ5018 SoC with mtd_test module & 
nand-utils tool.




Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-24 12:18, Miquel Raynal wrote:

Hello,

mda...@codeaurora.org wrote on Wed, 24 Feb 2021 10:09:48 +0530:


On 2021-02-24 01:13, mda...@codeaurora.org wrote:
> On 2021-02-23 22:04, Miquel Raynal wrote:
>> Hello,
>> >> Md Sadre Alam  wrote on Tue, 23 Feb 2021
>> 01:34:27 +0530:
>> >>> From QPIC version 2.0 onwards new register got added to read last
>> >>a new
>> >>> codeword. This change will add the READ_LOCATION_LAST_CW_n register.
>> >> Add support for this READ_LOCATION_LAST_CW_n register.
>> >>> >>> For first three code word READ_LOCATION_n register will be
>>> use.For last code word READ_LOCATION_LAST_CW_n register will be
>>> use.
>> >> "
>> In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
>> READ_LOCATION_n, while codeword 3 will be accessed through
>> READ_LOCATION_LAST_CW_n.
>> "
>> >> When I read my own sentence, I feel that there is something wrong.
>> If there are only 4 codewords, I guess a QPIC v2 is able to use
>> READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?
>> >> I guess the point of having these "last_cw_n" registers is to support
>> up to 8 codewords, am I wrong? If this the case, the current patch
>> completely fails doing that I don't get the point of such change.
>
> This register is only use to read last code word.
>
> I have address all the comments from all the previous sub sequent
> patches and pushed
> all patches in only one series.
>
> Please check.

  The registers READ_LOCATION & READ_LOCATION_LAST are not associated 
with number of code words.
  These two registers are used to access the location inside a code 
word.


Ok. Can you please explain what is a location then? Or point me to a
datasheet that explains it.


  The location is the position inside a code word.



Bottom line question: why having READ_LOCATION_0, _1,... an
READ_LOCATION_LAST_0, _1, etc?


 READ_LOCATION_0, _1,... are used to extract multiple chunks from a code 
word.


 e.g If we wanted to extract first 100 bytes from a code word then 
(0...99) READ_LOCATION_0 will be configured.
 if we wanted to extract next 100 bytes (100...199) then 
READ_LOCATION_1 will be configured.


 same way for last code word READ_LOCATION_LAST_0, _1, will be used.


  Below is the mapping example for a 2K page size.

 
<-2048><--64-->2111
 
___
|
  |   |   |
|
  |   |   |
|
  |   |   |
|
  |   |   |

|__|___|___|
 
  2048-bytes
 
  factory BBM



One code word:

 1-byte BBM   7-bytes ECC
__
||  ||   ||
||  ||   ||
||  ||   ||
||  ||   ||
||  ||   ||
||__||___||
 <---512 bytes--->4-bytes spare
 data


 
<-2048-->2047<--64-->2111
 

|
 |   | ||
|
 |   | ||
|
 |   | ||
|
 |   | ||

|_|___|_||
 
|   |
 
|   |
 
|   |<-mapping
 
|   |
 
   

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-24 01:13, mda...@codeaurora.org wrote:

On 2021-02-23 22:04, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Tue, 23 Feb 2021
01:34:27 +0530:


From QPIC version 2.0 onwards new register got added to read last


   a new


codeword. This change will add the READ_LOCATION_LAST_CW_n register.


Add support for this READ_LOCATION_LAST_CW_n register.



For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.


"
In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
READ_LOCATION_n, while codeword 3 will be accessed through
READ_LOCATION_LAST_CW_n.
"

When I read my own sentence, I feel that there is something wrong.
If there are only 4 codewords, I guess a QPIC v2 is able to use
READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?

I guess the point of having these "last_cw_n" registers is to support
up to 8 codewords, am I wrong? If this the case, the current patch
completely fails doing that I don't get the point of such change.


This register is only use to read last code word.

I have address all the comments from all the previous sub sequent
patches and pushed
all patches in only one series.

Please check.


 The registers READ_LOCATION & READ_LOCATION_LAST are not associated 
with number of code words.
 These two registers are used to access the location inside a code word. 
So whether we are having 4 code words
 or 8 code words it doesn't matter. If we wanted access the location 
within normal code word we have to
 use READ_LOCATION register and if we wanted to access location in last 
code word then we have to use

 READ_LOCATION_LAST.





Signed-off-by: Md Sadre Alam 
---


[...]


 /* helper to configure address register values */
@@ -700,8 +727,9 @@ static void set_address(struct qcom_nand_host 
*host, u16 column, int page)

  *
  * @num_cw:number of steps for the read/write operation
  * @read:  read or write operation
+ * @cw :   which code word
  */
-static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
bool read)
+static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
bool read, int cw)

 {
struct nand_chip *chip = &host->chip;
 	struct qcom_nand_controller *nandc = 
get_qcom_nand_controller(chip);
@@ -740,7 +768,7 @@ static void update_rw_regs(struct qcom_nand_host 
*host, int num_cw, bool read)

nandc_set_reg(nandc, NAND_EXEC_CMD, 1);

if (read)
-   nandc_set_read_loc(chip, 0, 0, 0, host->use_ecc ?
+   nandc_set_read_loc(chip, cw, 0, 0, host->use_ecc ?
   host->cw_data : host->cw_size, 1);
 }

@@ -,18 +1139,34 @@ static void config_nand_page_read(struct 
nand_chip *chip)

  NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL);
 }

+/* helper to check which location register should be use for this


/*
 * Check which location...


+ * code word. NAND_READ_LOCATION or NAND_READ_LOCATION_LAST_CW
+ */
+static bool config_loc_last_reg(struct nand_chip *chip, int cw)
+{
+	struct qcom_nand_controller *nandc = 
get_qcom_nand_controller(chip);

+   struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+   if (nandc->props->qpic_v2 && qcom_nandc_is_last_cw(ecc, cw))
+   return true;


Not sure this is really useful, it's probably better to drop this
helper and just use...


+
+   return false;
+}
 /*
  * Helper to prepare DMA descriptors for configuring registers
  * before reading each codeword in NAND page.
  */
 static void
-config_nand_cw_read(struct nand_chip *chip, bool use_ecc)
+config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
 {
 	struct qcom_nand_controller *nandc = 
get_qcom_nand_controller(chip);

+   int reg = NAND_READ_LOCATION_0;
+
+   if (config_loc_last_reg(chip, cw))


... if (nandc->props->qpic_v2 && qcom_nandc_is_lastcw()) here.


+   reg = NAND_READ_LOCATION_LAST_CW_0;

if (nandc->props->is_bam)
-   write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
- NAND_BAM_NEXT_SGL);
+   write_reg_dma(nandc, reg, 4, NAND_BAM_NEXT_SGL);

write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
@@ -1142,12 +1186,12 @@ config_nand_cw_read(struct nand_chip *chip, 
bool use_ecc)


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-23 22:04, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Tue, 23 Feb 2021
01:34:27 +0530:


From QPIC version 2.0 onwards new register got added to read last


   a new


codeword. This change will add the READ_LOCATION_LAST_CW_n register.


Add support for this READ_LOCATION_LAST_CW_n register.



For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.


"
In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
READ_LOCATION_n, while codeword 3 will be accessed through
READ_LOCATION_LAST_CW_n.
"

When I read my own sentence, I feel that there is something wrong.
If there are only 4 codewords, I guess a QPIC v2 is able to use
READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?

I guess the point of having these "last_cw_n" registers is to support
up to 8 codewords, am I wrong? If this the case, the current patch
completely fails doing that I don't get the point of such change.


This register is only use to read last code word.

I have address all the comments from all the previous sub sequent 
patches and pushed

all patches in only one series.

Please check.




Signed-off-by: Md Sadre Alam 
---


[...]


 /* helper to configure address register values */
@@ -700,8 +727,9 @@ static void set_address(struct qcom_nand_host 
*host, u16 column, int page)

  *
  * @num_cw:number of steps for the read/write operation
  * @read:  read or write operation
+ * @cw :   which code word
  */
-static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
bool read)
+static void update_rw_regs(struct qcom_nand_host *host, int num_cw, 
bool read, int cw)

 {
struct nand_chip *chip = &host->chip;
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
@@ -740,7 +768,7 @@ static void update_rw_regs(struct qcom_nand_host 
*host, int num_cw, bool read)

nandc_set_reg(nandc, NAND_EXEC_CMD, 1);

if (read)
-   nandc_set_read_loc(chip, 0, 0, 0, host->use_ecc ?
+   nandc_set_read_loc(chip, cw, 0, 0, host->use_ecc ?
   host->cw_data : host->cw_size, 1);
 }

@@ -,18 +1139,34 @@ static void config_nand_page_read(struct 
nand_chip *chip)

  NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL);
 }

+/* helper to check which location register should be use for this


/*
 * Check which location...


+ * code word. NAND_READ_LOCATION or NAND_READ_LOCATION_LAST_CW
+ */
+static bool config_loc_last_reg(struct nand_chip *chip, int cw)
+{
+   struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+   struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+   if (nandc->props->qpic_v2 && qcom_nandc_is_last_cw(ecc, cw))
+   return true;


Not sure this is really useful, it's probably better to drop this
helper and just use...


+
+   return false;
+}
 /*
  * Helper to prepare DMA descriptors for configuring registers
  * before reading each codeword in NAND page.
  */
 static void
-config_nand_cw_read(struct nand_chip *chip, bool use_ecc)
+config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
 {
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+   int reg = NAND_READ_LOCATION_0;
+
+   if (config_loc_last_reg(chip, cw))


... if (nandc->props->qpic_v2 && qcom_nandc_is_lastcw()) here.


+   reg = NAND_READ_LOCATION_LAST_CW_0;

if (nandc->props->is_bam)
-   write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
- NAND_BAM_NEXT_SGL);
+   write_reg_dma(nandc, reg, 4, NAND_BAM_NEXT_SGL);

write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
@@ -1142,12 +1186,12 @@ config_nand_cw_read(struct nand_chip *chip, 
bool use_ecc)


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-18 21:59, mda...@codeaurora.org wrote:

On 2021-02-18 14:50, Miquel Raynal wrote:

Hello,


>> >> +/* helper to configure location register values */
>> +static void nandc_set_read_loc(struct nand_chip *chip, int cw, int >> reg,
>> + int offset, int size, int is_last)
>
> You know cw, you have access to chip->ecc.steps, so you can derive by
> yourself if is_last is set or not. No need to forward it through
> function calls.


   This "is_last" is not for last code word, it will indicate the 
Location register "NAND_READ_LOCATION_n" last bit.


Ok, I've mixed two things. Let's keep this boolean as it is for now 
and

just do the minimum changes to support the LOCATION_LAST_cw registers.

Nevertheless, can't you calculate is_last from nandc_set_read_loc() ?

I also think a bit of renaming (in a different patch) would be welcome
to avoid such confusions.

Just to be clear: I think you should take a step back, and try to
simplify a bit this driver. I understand you know every character by
heart but with an external eye it's not that easy to understand what
you want to do and why:
- write small commits with a single, atomic change
- try to reduce the number of parameters when it is possible
- try to use meaningful names (is_last vs. LAST_CW)
- try to avoid extra indentation level when possible



  Sure , I will try to split these changes in multiple patches
  and re-pushed again.


[...]


>> @@ -1094,11 +1144,19 @@ static void

config_nand_page_read(struct

qcom_nand_controller *nandc)

>>   * before reading each codeword in NAND page.
>>   */
>>  static void
>> -config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
>> +config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
>>  {
>> -  if (nandc->props->is_bam)
>> -  write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
>> -NAND_BAM_NEXT_SGL);
>> +  struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> +  struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +
>> +  if (nandc->props->is_bam) {
>> +  if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
>> +  write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
>> +NAND_BAM_NEXT_SGL);
>> +  else
>> +  write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
>> +NAND_BAM_NEXT_SGL);
>> +  }
>
> Same here, I am pretty sure we can abstract the complexity.
>
Here I did this because , i need pointer to struct nand_ecc_ctrl 
structure
to access ecc->steps for CW comparison for last code word. cw == 
(ecc->steps - 1)


So i think no separate patch needed for conversion of 
nanc-->chip.
Please let me know if still separate patch needed for nanc-->chip 
conversion.


I was talking about the extra indentation level.

the "qpic_v2 && cv == ..." condition can be checked by write_reg_dma
directly.

You could even introduce a helper returning the boolean value of which
register should be used.

Regarding the use of nand_chip instead of nandc, if there are too many
changes involved, I prefer a separate patch.


  I will push separate patch for nandc to chip conversion.


I have pushed separate patch for nandc to chip conversion.
Please check.





>> >>   write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
>>write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
>> @@ -1117,11 +1175,11 @@ config_nand_cw_read(struct >> qcom_nand_controller 
*nandc, bool use_ecc)
>>   * single codeword in page
>>   */
>>  static void
>> -config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
>> -  bool use_ecc)
>> +config_nand_single_cw_page_read(struct nand_chip *chip,
>> +  bool use_ecc, int cw)
>>  {
>> -  config_nand_page_read(nandc);
>> -  config_nand_cw_read(nandc, use_ecc);
>> +  config_nand_page_read(chip);
>> +  config_nand_cw_read(chip, use_ecc, cw);
>>  }
>> >>  /*
>> @@ -1205,7 +1263,7 @@ static int nandc_param(struct qcom_nand_host >> *host)
>>nandc_set_reg(nandc, NAND_DEV_CMD_VLD_RESTORE, nandc->vld);
>>}
>> >> - nandc_set_read_loc(nandc, 0, 0, 512, 1);
>> +  nandc_set_read_loc(chip, 0, 0, 0, 512, 1);
>> >>   if (!nandc->props->qpic_v2) {
>>write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
>> @@ -1215,7 +1273,7 @@ static int nandc_param(struct qcom_nand_host >> *host)
>>nandc->buf_count = 512;
>>memset(nandc->data_buffer, 0xff, nandc->buf_count);
>> >> - config_nand_single_cw_page_read(nandc, false);
>> +  config_nand_single_cw_page_read(chip, false, 0);
>> >>   read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
>>  nandc->buf_count, 0);
>> @@ -1617,7 +1675,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, >> struct 
nand_chip *chip,
>>clear_bam_t

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-18 14:50, Miquel Raynal wrote:

Hello,


>> >> +/* helper to configure location register values */
>> +static void nandc_set_read_loc(struct nand_chip *chip, int cw, int >> reg,
>> + int offset, int size, int is_last)
>
> You know cw, you have access to chip->ecc.steps, so you can derive by
> yourself if is_last is set or not. No need to forward it through
> function calls.


   This "is_last" is not for last code word, it will indicate the 
Location register "NAND_READ_LOCATION_n" last bit.


Ok, I've mixed two things. Let's keep this boolean as it is for now and
just do the minimum changes to support the LOCATION_LAST_cw registers.

Nevertheless, can't you calculate is_last from nandc_set_read_loc() ?

I also think a bit of renaming (in a different patch) would be welcome
to avoid such confusions.

Just to be clear: I think you should take a step back, and try to
simplify a bit this driver. I understand you know every character by
heart but with an external eye it's not that easy to understand what
you want to do and why:
- write small commits with a single, atomic change
- try to reduce the number of parameters when it is possible
- try to use meaningful names (is_last vs. LAST_CW)
- try to avoid extra indentation level when possible



  Sure , I will try to split these changes in multiple patches
  and re-pushed again.


[...]


>> @@ -1094,11 +1144,19 @@ static void

config_nand_page_read(struct

qcom_nand_controller *nandc)

>>   * before reading each codeword in NAND page.
>>   */
>>  static void
>> -config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
>> +config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
>>  {
>> -  if (nandc->props->is_bam)
>> -  write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
>> -NAND_BAM_NEXT_SGL);
>> +  struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> +  struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +
>> +  if (nandc->props->is_bam) {
>> +  if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
>> +  write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
>> +NAND_BAM_NEXT_SGL);
>> +  else
>> +  write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
>> +NAND_BAM_NEXT_SGL);
>> +  }
>
> Same here, I am pretty sure we can abstract the complexity.
>
Here I did this because , i need pointer to struct nand_ecc_ctrl 
structure
to access ecc->steps for CW comparison for last code word. cw == 
(ecc->steps - 1)


So i think no separate patch needed for conversion of nanc-->chip.
Please let me know if still separate patch needed for nanc-->chip 
conversion.


I was talking about the extra indentation level.

the "qpic_v2 && cv == ..." condition can be checked by write_reg_dma
directly.

You could even introduce a helper returning the boolean value of which
register should be used.

Regarding the use of nand_chip instead of nandc, if there are too many
changes involved, I prefer a separate patch.


  I will push separate patch for nandc to chip conversion.




>> >>   write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
>>write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
>> @@ -1117,11 +1175,11 @@ config_nand_cw_read(struct >> qcom_nand_controller 
*nandc, bool use_ecc)
>>   * single codeword in page
>>   */
>>  static void
>> -config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
>> -  bool use_ecc)
>> +config_nand_single_cw_page_read(struct nand_chip *chip,
>> +  bool use_ecc, int cw)
>>  {
>> -  config_nand_page_read(nandc);
>> -  config_nand_cw_read(nandc, use_ecc);
>> +  config_nand_page_read(chip);
>> +  config_nand_cw_read(chip, use_ecc, cw);
>>  }
>> >>  /*
>> @@ -1205,7 +1263,7 @@ static int nandc_param(struct qcom_nand_host >> *host)
>>nandc_set_reg(nandc, NAND_DEV_CMD_VLD_RESTORE, nandc->vld);
>>}
>> >> - nandc_set_read_loc(nandc, 0, 0, 512, 1);
>> +  nandc_set_read_loc(chip, 0, 0, 0, 512, 1);
>> >>   if (!nandc->props->qpic_v2) {
>>write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
>> @@ -1215,7 +1273,7 @@ static int nandc_param(struct qcom_nand_host >> *host)
>>nandc->buf_count = 512;
>>memset(nandc->data_buffer, 0xff, nandc->buf_count);
>> >> - config_nand_single_cw_page_read(nandc, false);
>> +  config_nand_single_cw_page_read(chip, false, 0);
>> >>   read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
>>  nandc->buf_count, 0);
>> @@ -1617,7 +1675,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, >> struct 
nand_chip *chip,
>>clear_bam_transaction(nandc);
>>set_address(host, host->cw_size * cw, page);
>>update_rw_regs(host, 1, true);
>> -  config_nand_p

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-16 13:46, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Tue, 16 Feb 2021
00:46:42 +0530:


From QPIC version 2.0 onwards new register got added to
read last codeword. This change will add the READ_LOCATION_LAST_CW_n
register.

For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam 
---
[V6]


It is also very important that you mark the version in the subject:

[PATCH v6] mtd: rawnand: etc

Otherwise it is difficult to keep track on the changes.


  Sure I will follow the same.



 * Updated write_reg_dma() function in "v6"
 * Removed extra indentation level in read_page_ecc() to read last 
code word in "v6"

 * Removed boolean check in config_nand_cw_read() in "v6"
 drivers/mtd/nand/raw/qcom_nandc.c | 118 
--

 1 file changed, 87 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c

index 667e4bf..bae352f 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
 #defineNAND_READ_LOCATION_10xf24
 #defineNAND_READ_LOCATION_20xf28
 #defineNAND_READ_LOCATION_30xf2c
+#defineNAND_READ_LOCATION_LAST_CW_00xf40
+#defineNAND_READ_LOCATION_LAST_CW_10xf44
+#defineNAND_READ_LOCATION_LAST_CW_20xf48
+#defineNAND_READ_LOCATION_LAST_CW_30xf4c

 /* dummy register offsets, used by write_reg_dma */
 #defineNAND_DEV_CMD1_RESTORE   0xdead
@@ -181,8 +185,14 @@
 #defineECC_BCH_4BITBIT(2)
 #defineECC_BCH_8BITBIT(3)

-#define nandc_set_read_loc(nandc, reg, offset, size, is_last)  \
-nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
+#define nandc_set_read_loc_first(nandc, reg, offset, size, is_last)\
+nandc_set_reg(nandc, reg,  \
+ ((offset) << READ_LOCATION_OFFSET) |\
+ ((size) << READ_LOCATION_SIZE) |\
+ ((is_last) << READ_LOCATION_LAST))
+
+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, reg,  \
  ((offset) << READ_LOCATION_OFFSET) |\
  ((size) << READ_LOCATION_SIZE) |\
  ((is_last) << READ_LOCATION_LAST))
@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+   __le32 read_location_last0;
+   __le32 read_location_last1;
+   __le32 read_location_last2;
+   __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct 
nandc_regs *regs, int offset)

return ®s->read_location2;
case NAND_READ_LOCATION_3:
return ®s->read_location3;
+   case NAND_READ_LOCATION_LAST_CW_0:
+   return ®s->read_location_last0;
+   case NAND_READ_LOCATION_LAST_CW_1:
+   return ®s->read_location_last1;
+   case NAND_READ_LOCATION_LAST_CW_2:
+   return ®s->read_location_last2;
+   case NAND_READ_LOCATION_LAST_CW_3:
+   return ®s->read_location_last3;
default:
return NULL;
}
@@ -661,6 +683,26 @@ static void nandc_set_reg(struct 
qcom_nand_controller *nandc, int offset,

*reg = cpu_to_le32(val);
 }

+/* helper to configure location register values */
+static void nandc_set_read_loc(struct nand_chip *chip, int cw, int 
reg,

+  int offset, int size, int is_last)


You know cw, you have access to chip->ecc.steps, so you can derive by
yourself if is_last is set or not. No need to forward it through
function calls.



  This "is_last" is not for last code word, it will indicate the 
Location register "NAND_READ_LOCATION_n" last bit.


  bit[31]: Indicate this is the last Read location needing processing.




+{
+   struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+   struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+   int loc = NAND_READ_LOCATION_0;
+
+   if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
+   loc = NAND_READ_LOCATION_LAST_CW_0;
+
+   loc += reg * 4;
+
+   if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))


Just compute is_last a single time at the top of the helper and use it.


   This "is_last" is not for last code word, it will indicate the 
Location register last bit.


   bit[31]: Indicate this is the last Read location needing processing.



+   return nandc_set_read_loc_last(nandc, loc, offset, size, 
is_last);
+   else
+   return nandc_set_read_loc_first(nandc, loc, offset, size, 

Re: [PATCH] mtd: spi-nor: gigadevice: Add support for gd25lb256e

[mdalam]

On 2021-02-09 16:41, Md Sadre Alam wrote:

Add support for gd25lb256e. This device tested on IPQ5018
platform with dd from/to the flash for read/write respectly,
and mtd erase for erasing the flash.

Signed-off-by: Md Sadre Alam 
---
 drivers/mtd/spi-nor/gigadevice.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/gigadevice.c 
b/drivers/mtd/spi-nor/gigadevice.c

index 447d84b..8f1e7ec 100644
--- a/drivers/mtd/spi-nor/gigadevice.c
+++ b/drivers/mtd/spi-nor/gigadevice.c
@@ -50,6 +50,9 @@ static const struct flash_info gigadevice_parts[] = {
   SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK |
   SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6)
.fixups = &gd25q256_fixups },
+   { "gd25lb256e", INFO(0xc86719, 0, 64 * 1024, 512,
+ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+ SPI_NOR_4B_OPCODES) },
 };

 const struct spi_nor_manufacturer spi_nor_gigadevice = {


ping! Is there any additional info needed for this ?

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-15 14:10, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Mon, 15 Feb 2021
02:47:31 +0530:


From QPIC version 2.0 onwards new register got added to
read last codeword. This change will add the READ_LOCATION_LAST_CW_n
register.

For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam 
---
[V5]
 * Added helper function to update location register value.



Please don't forget the "v5" in the message object.


 /*
@@ -1094,11 +1141,16 @@ static void config_nand_page_read(struct 
qcom_nand_controller *nandc)

  * before reading each codeword in NAND page.
  */
 static void
-config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
+config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc, 
bool last_cw)

 {
-   if (nandc->props->is_bam)
-   write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
- NAND_BAM_NEXT_SGL);
+   if (nandc->props->is_bam) {
+   if (nandc->props->qpic_v2 && last_cw)
+   write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
+ NAND_BAM_NEXT_SGL);
+   else
+   write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
+ NAND_BAM_NEXT_SGL);


I guess write_reg_dma should be updated as well.


  Updated in V6 patch , please check.



[...]



-   config_nand_cw_read(nandc, false);
+	config_nand_cw_read(nandc, false, cw == ecc->steps - 1 ? true : 
false);


read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
reg_off += data_size1;
@@ -1873,18 +1938,31 @@ static int read_page_ecc(struct qcom_nand_host 
*host, u8 *data_buf,


if (nandc->props->is_bam) {
if (data_buf && oob_buf) {
-   nandc_set_read_loc(nandc, 0, 0, data_size, 0);
-   nandc_set_read_loc(nandc, 1, data_size,
-  oob_size, 1);
+   if (nandc->props->qpic_v2 && i == (ecc->steps - 
1)) {


I would like the helper to handle this condition. I would prefer to
avoid yet an extra indentation level.


  Updated in V6 patch , please check.



+   nandc_set_read_loc(chip, i, 0, 0, 
data_size, 0);
+   nandc_set_read_loc(chip, i, 1, 
data_size,
+  oob_size, 1);
+   } else {
+   nandc_set_read_loc(chip, i, 0, 0, 
data_size, 0);
+   nandc_set_read_loc(chip, i, 1, 
data_size,
+  oob_size, 1);
+   }
} else if (data_buf) {
-   nandc_set_read_loc(nandc, 0, 0, data_size, 1);
+   if (nandc->props->qpic_v2 && i == (ecc->steps - 
1))
+   nandc_set_read_loc(chip, i, 0, 0, 
data_size, 1);
+   else
+   nandc_set_read_loc(chip, i, 0, 0, 
data_size, 1);
} else {
-   nandc_set_read_loc(nandc, 0, data_size,
-  oob_size, 1);
+   if (nandc->props->qpic_v2 && i == (ecc->steps - 
1))
+   nandc_set_read_loc(chip, i, 0, 
data_size,
+  oob_size, 1);
+   else
+   nandc_set_read_loc(chip, i, 0, 
data_size,
+  oob_size, 1);
}
}

-   config_nand_cw_read(nandc, true);
+		config_nand_cw_read(nandc, true, i == ecc->steps - 1 ? true : 
false);


i == (ecc->steps - 1)

is already a boolean, you don't need

"? true : false"



   Updated in V6 patch.



if (data_buf)
read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
@@ -1946,7 +2024,7 @@ static int copy_last_cw(struct qcom_nand_host 
*host, int page)

set_address(host, host->cw_size * (ecc->steps - 1), page);
update_rw_regs(host, 1, true);

-   config_nand_single_cw_page_read(nandc, host->use_ecc);
+   config_nand_single_cw_page_read(nandc, host->use_ecc, true);


Maybe it's best to just forward the codeword and let the code that
needs to know if it is the last one or not do the comparison.



  Updated in V6 patch, please check.


read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);



Thanks,
Miquèl

Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-12 13:49, Miquel Raynal wrote:

Hello,

mda...@codeaurora.org wrote on Fri, 12 Feb 2021 01:00:47 +0530:


On 2021-02-11 19:37, Miquel Raynal wrote:
> Hello,
>
> Manivannan Sadhasivam  wrote on Wed,
> 10 Feb 2021 14:31:44 +0530:
>
>> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
>> > From QPIC version 2.0 onwards new register got added to
>> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
>> > register.
>> >
>> > For first three code word READ_LOCATION_n register will be
>> > use.For last code word READ_LOCATION_LAST_CW_n register will be
>> > use.
>
> Sorry for the late notice, I think the patch is fine but if you don't
> mind I would like to propose a small change that should simplify your
> patch a lot, see below.
>
>> >
>> > Signed-off-by: Md Sadre Alam 
>> >> Reviewed-by: Manivannan Sadhasivam 
>> >> Thanks,
>> Mani
>> >> > ---
>> > [V4]
>> >  * Modified condition for nandc_set_read_loc_last() in 
qcom_nandc_read_cw_raw().
>> >  * Added one additional argument "last_cw" to the function 
config_nand_cw_read()
>> >to handle last code word condition.
>> >  * Changed total number of last code word register 
"NAND_READ_LOCATION_LAST_CW_0" to 4
>> >while doing code word configuration.
>> >  drivers/mtd/nand/raw/qcom_nandc.c | 110 
+-
>> >  1 file changed, 84 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c
>> > index 667e4bf..9484be8 100644
>> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> > @@ -48,6 +48,10 @@
>> >  #define  NAND_READ_LOCATION_10xf24
>> >  #define  NAND_READ_LOCATION_20xf28
>> >  #define  NAND_READ_LOCATION_30xf2c
>> > +#define  NAND_READ_LOCATION_LAST_CW_00xf40
>> > +#define  NAND_READ_LOCATION_LAST_CW_10xf44
>> > +#define  NAND_READ_LOCATION_LAST_CW_20xf48
>> > +#define  NAND_READ_LOCATION_LAST_CW_30xf4c
>> >
>> >  /* dummy register offsets, used by write_reg_dma */
>> >  #define  NAND_DEV_CMD1_RESTORE   0xdead
>> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg,
   \
>> > ((size) << READ_LOCATION_SIZE) |\
>> > ((is_last) << READ_LOCATION_LAST))
>> >
>> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last)   \
>> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg,   \
>> > +   ((offset) << READ_LOCATION_OFFSET) |\
>> > +   ((size) << READ_LOCATION_SIZE) |\
>> > +   ((is_last) << READ_LOCATION_LAST))
>> > +
>
> You could rename the macro nandc_set_read_loc() into
> nandc_set_read_loc_first() or anything else that make sense, then have
> a helper which does:
>
> nandc_set_read_loc()
> {
>if (condition for first)
>return nandc_set_read_loc_first();
>else
>return nandc_set_read_loc_last();
> }
>

   Yes this is more precise way & simplify the patch a lot.
   But for this i have to change these two macro as a function.

   nandc_set_read_loc() & nandc_set_read_loc_last().

   Since for last code word register we are using Token Pasting 
Operator##.


   So if i am implementing like the below.

   /* helper to configure location register values */
   static void nandc_set_read_loc(struct qcom_nand_controller *nandc, 
int reg,

   int offset, int size, int is_last, bool last_cw)
   {
   if (last_cw)
   return nandc_set_read_loc_last(nandc, reg, offset, 
size, is_last);

   else
   return nandc_set_read_loc_first(nandc, reg, offset, 
size, is_last);

  }

   So here for macro expansion reg should be a value not a variable 
else it will be expended like
   NAND_READ_LOCATION_LAST_CW_reg instead of 
NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.


I know it involves a little bit more computation but I wonder if using
funcs instead of macros here would not be nicer? Perhaps something 
like:


loc = is_last ? NAND_READ_LOCATION /* 0xf20 */ :
NAND_READ_LOCATION_LAST /* 0xf40 */;
loc += reg * 2;


  I have added a helper function to update location register value in V5 
patch.

  Please check the patch.


  the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0, 
read_loc, data_size1, 0, true); ---> for last code word.
  nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> 
for first three code wrod.


I think it's best to forward 'cw' as a parameter and do the
computation of is_last locally.


  So is this ok for you to convert these two macro into function ?

> And in the rest of your patch you won't have to touch anything else.
>
> Thanks,
> Miquèl


Thanks,
Miquèl

Re: [PATCH V4] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-02-11 19:37, Miquel Raynal wrote:

Hello,

Manivannan Sadhasivam  wrote on Wed,
10 Feb 2021 14:31:44 +0530:


On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> From QPIC version 2.0 onwards new register got added to
> read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> register.
>
> For first three code word READ_LOCATION_n register will be
> use.For last code word READ_LOCATION_LAST_CW_n register will be
> use.


Sorry for the late notice, I think the patch is fine but if you don't
mind I would like to propose a small change that should simplify your
patch a lot, see below.


>
> Signed-off-by: Md Sadre Alam 

Reviewed-by: Manivannan Sadhasivam 

Thanks,
Mani

> ---
> [V4]
>  * Modified condition for nandc_set_read_loc_last() in 
qcom_nandc_read_cw_raw().
>  * Added one additional argument "last_cw" to the function 
config_nand_cw_read()
>to handle last code word condition.
>  * Changed total number of last code word register 
"NAND_READ_LOCATION_LAST_CW_0" to 4
>while doing code word configuration.
>  drivers/mtd/nand/raw/qcom_nandc.c | 110 
+-
>  1 file changed, 84 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c
> index 667e4bf..9484be8 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -48,6 +48,10 @@
>  #define   NAND_READ_LOCATION_10xf24
>  #define   NAND_READ_LOCATION_20xf28
>  #define   NAND_READ_LOCATION_30xf2c
> +#define   NAND_READ_LOCATION_LAST_CW_00xf40
> +#define   NAND_READ_LOCATION_LAST_CW_10xf44
> +#define   NAND_READ_LOCATION_LAST_CW_20xf48
> +#define   NAND_READ_LOCATION_LAST_CW_30xf4c
>
>  /* dummy register offsets, used by write_reg_dma */
>  #define   NAND_DEV_CMD1_RESTORE   0xdead
> @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg,   
 \
>  ((size) << READ_LOCATION_SIZE) |\
>  ((is_last) << READ_LOCATION_LAST))
>
> +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last)\
> +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg,\
> +((offset) << READ_LOCATION_OFFSET) |\
> +((size) << READ_LOCATION_SIZE) |\
> +((is_last) << READ_LOCATION_LAST))
> +


You could rename the macro nandc_set_read_loc() into
nandc_set_read_loc_first() or anything else that make sense, then have
a helper which does:

nandc_set_read_loc()
{
if (condition for first)
return nandc_set_read_loc_first();
else
return nandc_set_read_loc_last();
}



  Yes this is more precise way & simplify the patch a lot.
  But for this i have to change these two macro as a function.

  nandc_set_read_loc() & nandc_set_read_loc_last().

  Since for last code word register we are using Token Pasting 
Operator##.


  So if i am implementing like the below.

  /* helper to configure location register values */
  static void nandc_set_read_loc(struct qcom_nand_controller *nandc, int 
reg,

  int offset, int size, int is_last, bool last_cw)
  {
  if (last_cw)
  return nandc_set_read_loc_last(nandc, reg, offset, 
size, is_last);

  else
  return nandc_set_read_loc_first(nandc, reg, offset, 
size, is_last);

 }

  So here for macro expansion reg should be a value not a variable else 
it will be expended like
  NAND_READ_LOCATION_LAST_CW_reg instead of 
NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.


 the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0, 
read_loc, data_size1, 0, true); ---> for last code word.
 nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> for 
first three code wrod.



 So is this ok for you to convert these two macro into function ?


And in the rest of your patch you won't have to touch anything else.

Thanks,
Miquèl

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-02-09 23:05, Bjorn Andersson wrote:

On Mon 01 Feb 09:50 CST 2021, mda...@codeaurora.org wrote:


On 2021-02-01 12:13, Vinod Koul wrote:
> On 01-02-21, 11:52, mda...@codeaurora.org wrote:
> > On 2021-02-01 11:35, Vinod Koul wrote:
> > > On 27-01-21, 23:56, mda...@codeaurora.org wrote:
>
> > > >   The actual LOCK/UNLOCK flag should be set on hardware command
> > > > descriptor.
> > > >   so this flag setting should be done in DMA engine driver. The user
> > > > of the
> > > > DMA
> > > >   driver like (in case of IPQ5018) Crypto can use flag
> > > > "DMA_PREP_LOCK" &
> > > > "DMA_PREP_UNLOCK"
> > > >   while preparing CMD descriptor before submitting to the DMA
> > > > engine. In DMA
> > > > engine driver
> > > >   we are checking these flasgs on CMD descriptor and setting actual
> > > > LOCK/UNLOCK flag on hardware
> > > >   descriptor.
> > >
> > >
> > > I am not sure I comprehend this yet.. when is that we would need to do
> > > this... is this for each txn submitted to dmaengine.. or something
> > > else..
> >
> >  Its not for each transaction submitted to dmaengine. We have to set
> > this
> > only
> >  once on CMD descriptor. So when A53 crypto driver need to change
> > the crypto
> > configuration
> >  then first it will lock the all other pipes using setting the LOCK
> > flag bit
> > on CMD
> >  descriptor and then it can start the transaction , on data
> > descriptor this
> > flag will
> >  not get set once all transaction will be completed the A53 crypto
> > driver
> > release the lock on
> >  all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK
> > will be
> > only once and not for
> >  the each transaction.
>
> Okay so why cant the bam driver check cmd descriptor and do lock/unlock
> as below, why do we need users to do this.
>
> if (flags & DMA_PREP_CMD) {
> do_lock_bam();

 User will not decide to do this LOCK/UNLOCK mechanism. It depends on
 use case.  This LOCK/UNLOCK mechanism not required always. It needs
 only when hardware will be shared between different core with
 different driver.


So you have a single piece of crypto hardware and you're using the 
BAM's

LOCK/UNLOCK feature to implement a "mutex" on a particular BAM channel?


  Yes, In IPQ5018 SoC we are having only one Crypto and it will be 
shared between
  UBI32 core & A53 core, and these two cores are running different 
driver to use Crypto.

  The LOCK/UNLOCK flag can be set only on CMD descriptor.



 The LOCK/UNLOCK flags provides SW to enter ordering between pipes
execution.
 (Generally, the BAM pipes are total independent from each other and 
work in

parallel manner).
 This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.

 Pipe descriptor having the following flags:
 INT : Interrupt
 EOT: End of transfer
 EOB: End of block
 NWD: Notify when done
 CMD: Command
 LOCK: Lock
 UNLOCK: Unlock
 etc.

 Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in
IPQ5018)
 So here only Crypto will be shared b/w multiple cores so For crypto 
request

only the LOCK/UNLOCK
 mechanism required.
 For other request like for QPIC driver, QUPT driver etc. its not 
required.

So Crypto driver has to raise the flag for
 LOCK/UNLOCK while preparing CMD descriptor. The actual locking will 
happen

in BAM driver only using condition
 if (flags & DMA_PREP_CMD) {
 if (flags & DMA_PREP_LOCK)
desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
 }

 So Crypto driver should set this flag DMA_PREP_LOCK while preparing 
CMD

descriptor.
 So LOCK should be set on actual hardware pipe descriptor with 
descriptor

type CMD.



It sounds fairly clear that the actual descriptor modification must
happen in the BAM driver, but the question in my mind is how this is
exposed to the DMAengine clients (e.g. crypto, QPIC etc).


  I have added these two flags "DMA_PREP_LOCK" & "DMA_PREP_UNLOCK" In 
enum dma_ctrl_flags.


  enum dma_ctrl_flags {
DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,8 @@ enum dma_ctrl_flags {
DMA_PREP_CMD = (1 << 7),
DMA_PREP_REPEAT = (1 << 8),
DMA_PREP_LOAD_EOT = (1 << 9),
+   DMA_PREP_LOCK = (1 << 10),
+   DMA_PREP_UNLOCK = (1 << 11),
 };

 So these flags we get passed while preparing CMD descriptor in Crypto 
driver. Based on these
 flags only i am setting LOCK/UNLOCK flags on actual hardware descriptor 
in BAM driver.


  if (flags & DMA_PREP_CMD) {
 if (flags & DMA_PREP_LOCK)
 desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);



What is the life span of the locked state? Do you always provide a
series of descriptors that starts with a LOCK and ends with an UNLOCK?
Or do you envision that the crypto driver provides a LOCK descriptor 
and

at some later point provides a UNLOCK descriptor?



  While preparing CMD descriptor we will use this LOCK/UNLOCK flags. So 
if i wanted to write
  some 20 registers of Crypto HW via BAM then i will prepare multiple 
command descriptor
  let's say 20 CMD descriptor so in the very fir

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-02-01 21:20, mda...@codeaurora.org wrote:

On 2021-02-01 12:13, Vinod Koul wrote:

On 01-02-21, 11:52, mda...@codeaurora.org wrote:

On 2021-02-01 11:35, Vinod Koul wrote:
> On 27-01-21, 23:56, mda...@codeaurora.org wrote:



> >   The actual LOCK/UNLOCK flag should be set on hardware command
> > descriptor.
> >   so this flag setting should be done in DMA engine driver. The user
> > of the
> > DMA
> >   driver like (in case of IPQ5018) Crypto can use flag
> > "DMA_PREP_LOCK" &
> > "DMA_PREP_UNLOCK"
> >   while preparing CMD descriptor before submitting to the DMA
> > engine. In DMA
> > engine driver
> >   we are checking these flasgs on CMD descriptor and setting actual
> > LOCK/UNLOCK flag on hardware
> >   descriptor.
>
>
> I am not sure I comprehend this yet.. when is that we would need to do
> this... is this for each txn submitted to dmaengine.. or something
> else..

 Its not for each transaction submitted to dmaengine. We have to set 
this

only
 once on CMD descriptor. So when A53 crypto driver need to change the 
crypto

configuration
 then first it will lock the all other pipes using setting the LOCK 
flag bit

on CMD
 descriptor and then it can start the transaction , on data 
descriptor this

flag will
 not get set once all transaction will be completed the A53 crypto 
driver

release the lock on
 all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK 
will be

only once and not for
 the each transaction.


Okay so why cant the bam driver check cmd descriptor and do 
lock/unlock

as below, why do we need users to do this.

if (flags & DMA_PREP_CMD) {
do_lock_bam();


 User will not decide to do this LOCK/UNLOCK mechanism. It depends on 
use case.

 This LOCK/UNLOCK mechanism not required always. It needs only when
hardware will be shared
 between different core with different driver.
 The LOCK/UNLOCK flags provides SW to enter ordering between pipes 
execution.

 (Generally, the BAM pipes are total independent from each other and
work in parallel manner).
 This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.

 Pipe descriptor having the following flags:
 INT : Interrupt
 EOT: End of transfer
 EOB: End of block
 NWD: Notify when done
 CMD: Command
 LOCK: Lock
 UNLOCK: Unlock
 etc.

 Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in 
IPQ5018)

 So here only Crypto will be shared b/w multiple cores so For crypto
request only the LOCK/UNLOCK
 mechanism required.
 For other request like for QPIC driver, QUPT driver etc. its not
required. So Crypto driver has to raise the flag for
 LOCK/UNLOCK while preparing CMD descriptor. The actual locking will
happen in BAM driver only using condition
 if (flags & DMA_PREP_CMD) {
 if (flags & DMA_PREP_LOCK)
desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
 }

 So Crypto driver should set this flag DMA_PREP_LOCK while preparing
CMD descriptor.
 So LOCK should be set on actual hardware pipe descriptor with
descriptor type CMD.



The point here is that this seems to be internal to dma and should be
handled by dma driver.


  This LOCK/UNLOK flags are part of actual hardware descriptor so this
should be handled by BAM driver only.
  If we set condition like this
  if (flags & DMA_PREP_CMD) {
do_lock_bam();
  Then LOCK/UNLOCK will be applied for all the CMD descriptor
including (QPIC driver, QUP driver , Crypto driver etc.).
  So this is not our intension. So we need to set this LOCK/UNLOCK
only for the drivers it needs. So Crypto driver needs
  locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver
request only for other driver request like QPIC driver,
  QUP driver will not set this.



   ping! Do you need any further info on this?


Also if we do this, it needs to be done for specific platforms..










Thanks

Re: [PATCH V3] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-01-29 10:41, mda...@codeaurora.org wrote:

On 2021-01-28 13:22, Manivannan Sadhasivam wrote:

On Sun, Jan 10, 2021 at 09:31:45AM +0530, Md Sadre Alam wrote:

From QPIC version 2.0 onwards new register got added to
read last codeword. This change will update the same.

For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam 


I gave this patch a try on SDX55 board but not able to resolve an 
issue and
I think it is related to reading the last code word which this patch 
is trying
to address. For my patch on supporting QPIC v2 IP, I tested with 
SDX55-MTP board
and I never hit any issue. But on my new dev board (Telit FN980), 
there seems to
be an issue while populating the partitions and tracing down that bug 
lands me

in copy_last_cw() function.

The issue only happens while creating the 3rd partition on Telit board 
whose
size differs when compared with MTP. The board just reboots into QDL 
mode

whenever it tries to read the last code word.

Below is the snippet of partition layout:

Telit partitions:

[1.082015] 0: sbl offs=0x size=0x000a attr:0x00ff
[1.082702] 1: mibib offs=0x000a size=0x000a 
attr:0x00ff

[1.083488] 2: ico offs=0x0014 size=0x0014 attr:0x00ff
[1.084572] 3: efs2 offs=0x0028 size=0x002c attr:0x00ff
[1.085316] 4: tz offs=0x0054 size=0x0007 attr:0x00ff
[1.086089] 5: tz_devcfg offs=0x005b size=0x0004 
attr:0x00ff



MTP partitions:

[1.573871] 0: sbl offs=0x size=0x000a attr:0x00ff
[1.581139] 1: mibib offs=0x000a size=0x000a 
attr:0x00ff

[1.587362] 2: efs2 offs=0x0014 size=0x002c attr:0x00ff
[1.593853] 3: tz offs=0x0040 size=0x0007 attr:0x00ff
[1.599860] 4: tz_devcfg offs=0x0047 size=0x0004 
attr:0x00ff

...

So until I figure this out, please keep this patch on hold!


  There was some corner case I missed in V3 patch. I have fixed this
in V4 patch.
  I have done some tress testing as well with V4 patch on IPQ5018
platform using "nand-utils"
  and "mtd_test.ko" , Now its working fine. Can you test with V4 patch 
once.


   ping! Do you need some more info for the V4 patch ?


Thanks,
Mani


---
[V3]
 * Added else condition for last code word in update_rw_regs().
 drivers/mtd/nand/raw/qcom_nandc.c | 84 
---

 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c

index 667e4bf..50ff6e3 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
 #defineNAND_READ_LOCATION_10xf24
 #defineNAND_READ_LOCATION_20xf28
 #defineNAND_READ_LOCATION_30xf2c
+#defineNAND_READ_LOCATION_LAST_CW_00xf40
+#defineNAND_READ_LOCATION_LAST_CW_10xf44
+#defineNAND_READ_LOCATION_LAST_CW_20xf48
+#defineNAND_READ_LOCATION_LAST_CW_30xf4c

 /* dummy register offsets, used by write_reg_dma */
 #defineNAND_DEV_CMD1_RESTORE   0xdead
@@ -187,6 +191,12 @@ nandc_set_reg(nandc, 
NAND_READ_LOCATION_##reg,			\

  ((size) << READ_LOCATION_SIZE) |\
  ((is_last) << READ_LOCATION_LAST))

+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
+ ((offset) << READ_LOCATION_OFFSET) |\
+ ((size) << READ_LOCATION_SIZE) |\
+ ((is_last) << READ_LOCATION_LAST))
+
 /*
  * Returns the actual register address for all NAND_DEV_ registers
  * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and 
NAND_DEV_CMD_VLD)

@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+   __le32 read_location_last0;
+   __le32 read_location_last1;
+   __le32 read_location_last2;
+   __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct 
nandc_regs *regs, int offset)

return ®s->read_location2;
case NAND_READ_LOCATION_3:
return ®s->read_location3;
+   case NAND_READ_LOCATION_LAST_CW_0:
+   return ®s->read_location_last0;
+   case NAND_READ_LOCATION_LAST_CW_1:
+   return ®s->read_location_last1;
+   case NAND_READ_LOCATION_LAST_CW_2:
+   return ®s->read_location_last2;
+   case NAND_READ_LOCATION_LAST_CW_3:
+   return ®s->read_location_last3;
default:
return NULL;
}
@@ -719,9 +741,14 @@ static void update_rw_regs(struct qcom_nand_host 
*

Re: [PATCH] mtd: rawnand: qcom: Update register macro name for 0x2c offset

[mdalam]

On 2021-01-31 01:37, Md Sadre Alam wrote:

This change will remove unused register name macro NAND_DEV1_ECC_CFG.
Since this register was only available in QPIC version 1.4.20 ipq40xx
and it was not used. In QPIC version 1.5 on wards this register got
removed.In QPIC version 2.0 0x2c offset is updated with register
NAND_AUTO_STATUS_EN So adding this register macro NAND_AUTO_STATUS_EN
with offset 0x2c.

Signed-off-by: Md Sadre Alam 


  Ping! Is any additional info needed for this patch ?


---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
b/drivers/mtd/nand/raw/qcom_nandc.c
index 9484be8..c238a35 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -27,7 +27,7 @@
 #defineNAND_DEV0_CFG0  0x20
 #defineNAND_DEV0_CFG1  0x24
 #defineNAND_DEV0_ECC_CFG   0x28
-#defineNAND_DEV1_ECC_CFG   0x2c
+#defineNAND_AUTO_STATUS_EN 0x2c
 #defineNAND_DEV1_CFG0  0x30
 #defineNAND_DEV1_CFG1  0x34
 #defineNAND_READ_ID0x40

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-02-01 12:13, Vinod Koul wrote:

On 01-02-21, 11:52, mda...@codeaurora.org wrote:

On 2021-02-01 11:35, Vinod Koul wrote:
> On 27-01-21, 23:56, mda...@codeaurora.org wrote:



> >   The actual LOCK/UNLOCK flag should be set on hardware command
> > descriptor.
> >   so this flag setting should be done in DMA engine driver. The user
> > of the
> > DMA
> >   driver like (in case of IPQ5018) Crypto can use flag
> > "DMA_PREP_LOCK" &
> > "DMA_PREP_UNLOCK"
> >   while preparing CMD descriptor before submitting to the DMA
> > engine. In DMA
> > engine driver
> >   we are checking these flasgs on CMD descriptor and setting actual
> > LOCK/UNLOCK flag on hardware
> >   descriptor.
>
>
> I am not sure I comprehend this yet.. when is that we would need to do
> this... is this for each txn submitted to dmaengine.. or something
> else..

 Its not for each transaction submitted to dmaengine. We have to set 
this

only
 once on CMD descriptor. So when A53 crypto driver need to change the 
crypto

configuration
 then first it will lock the all other pipes using setting the LOCK 
flag bit

on CMD
 descriptor and then it can start the transaction , on data descriptor 
this

flag will
 not get set once all transaction will be completed the A53 crypto 
driver

release the lock on
 all other pipes using UNLOCK flag on CMD descriptor. So LOCK/UNLOCK 
will be

only once and not for
 the each transaction.


Okay so why cant the bam driver check cmd descriptor and do lock/unlock
as below, why do we need users to do this.

if (flags & DMA_PREP_CMD) {
do_lock_bam();


 User will not decide to do this LOCK/UNLOCK mechanism. It depends on 
use case.
 This LOCK/UNLOCK mechanism not required always. It needs only when 
hardware will be shared

 between different core with different driver.
 The LOCK/UNLOCK flags provides SW to enter ordering between pipes 
execution.
 (Generally, the BAM pipes are total independent from each other and 
work in parallel manner).

 This LOCK/UNLOCK flags are part of actual pipe hardware descriptor.

 Pipe descriptor having the following flags:
 INT : Interrupt
 EOT: End of transfer
 EOB: End of block
 NWD: Notify when done
 CMD: Command
 LOCK: Lock
 UNLOCK: Unlock
 etc.

 Here the BAM driver is common driver for (QPIC, Crypto, QUP etc. in 
IPQ5018)
 So here only Crypto will be shared b/w multiple cores so For crypto 
request only the LOCK/UNLOCK

 mechanism required.
 For other request like for QPIC driver, QUPT driver etc. its not 
required. So Crypto driver has to raise the flag for
 LOCK/UNLOCK while preparing CMD descriptor. The actual locking will 
happen in BAM driver only using condition

 if (flags & DMA_PREP_CMD) {
 if (flags & DMA_PREP_LOCK)
desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
 }

 So Crypto driver should set this flag DMA_PREP_LOCK while preparing CMD 
descriptor.
 So LOCK should be set on actual hardware pipe descriptor with 
descriptor type CMD.




The point here is that this seems to be internal to dma and should be
handled by dma driver.

  This LOCK/UNLOK flags are part of actual hardware descriptor so this 
should be handled by BAM driver only.

  If we set condition like this
  if (flags & DMA_PREP_CMD) {
do_lock_bam();
  Then LOCK/UNLOCK will be applied for all the CMD descriptor including 
(QPIC driver, QUP driver , Crypto driver etc.).
  So this is not our intension. So we need to set this LOCK/UNLOCK only 
for the drivers it needs. So Crypto driver needs
  locking mechanism so we will set LOCK/UNLOCK flag on Crypto driver 
request only for other driver request like QPIC driver,

  QUP driver will not set this.


Also if we do this, it needs to be done for specific platforms..










Thanks

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-02-01 11:35, Vinod Koul wrote:

On 27-01-21, 23:56, mda...@codeaurora.org wrote:

On 2021-01-19 22:15, Vinod Koul wrote:
> On 18-01-21, 09:21, mda...@codeaurora.org wrote:
> > On 2021-01-15 11:28, Vinod Koul wrote:
> > > On 14-01-21, 01:20, mda...@codeaurora.org wrote:
> > > > On 2021-01-12 15:40, Vinod Koul wrote:
> > > > > On 12-01-21, 15:01, mda...@codeaurora.org wrote:
> > > > > > On 2020-12-21 23:03, mda...@codeaurora.org wrote:
> > > > > > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > > > > > This change will add support for LOCK & UNLOCK flag bit 
support
> > > > > > > > > on CMD descriptor.
> > > > > > > > >
> > > > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester 
of this
> > > > > > > > > transaction wanted to lock the DMA controller for this 
transaction so
> > > > > > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > > > > > >
> > > > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > > > > > of this
> > > > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > > > > > should set
> > > > > > > > > UNLOCK bit for the HW descriptor.
> > > > > > > >
> > > > > > > > Can you explain why would we need to first lock and then 
unlock..? How
> > > > > > > > would this be used in real world.
> > > > > > > >
> > > > > > > > I have read a bit of documentation but is unclear to me. Also 
should
> > > > > > > > this be exposed as an API to users, sounds like internal to 
driver..?
> > > > > > > >
> > > > > > >
> > > > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto 
Hardware
> > > > > > > Engine
> > > > > > > will be shared between A53 core & ubi32 core. There is two 
separate
> > > > > > > driver dedicated
> > > > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > > > > > parallelly for encryption/description
> > > > > > > we need bam locking mechanism. if one driver will submit the 
request
> > > > > > > for encryption/description
> > > > > > > to Crypto then first it has to set LOCK flag bit on command 
descriptor
> > > > > > > so that other pipes will
> > > > > > > get locked.
> > > > > > >
> > > > > > > The Pipe Locking/Unlocking will be only on command-descriptor. 
Upon
> > > > > > > encountering a command descriptor
> > > > >
> > > > > Can you explain what is a cmd descriptor?
> > > >
> > > >   In BAM pipe descriptor structure there is a field called CMD
> > > > (Command
> > > > descriptor).
> > > >   CMD allows the SW to create descriptors of type Command which does
> > > > not
> > > > generate any data transmissions
> > > >   but configures registers in the Peripheral (write operations, and
> > > > read
> > > > registers operations ).
> > > >   Using command descriptor enables the SW to queue new configurations
> > > > between data transfers in advance.
> > >
> > > What and when is the CMD descriptor used for..?
> >
> >   CMD descriptor is mainly used for configuring controller register.
> >   We can read/write controller register via BAM using CMD descriptor
> > only.
> >   CMD descriptor use command pipe for the transaction.
>
> In which use cases would you need to issue cmd descriptors..?

  In IPQ5018 there is only one Crypto engine and it will get shared
  between UBI32 core & A53 core. So here we need to use command
  descriptor in-order to perform LOCKING/UNLOCKING mechanism. Since
  LOCK/ULOCK flag we can set only on CMD descriptor.


So when will lock/unlock be performed? Can you please explain that..


  LOCK/UNLOCK will be performed when two different driver wanted to use 
the
  same HW. eg. In IPQ5018 there is only one Crypto engine and it will be 
shared b/w

  UBI32 core driver and A53 core driver.

  When A53 core wanted to submit request to crypto engine via BAM then 
first it has to
  LOCK all other pipes (pipe dedicated to UBI32 core) and then trigger 
the transaction start.
  Once all the transaction will be completed the A53 core crypto driver 
will release the LOCK
  from all the pipes. Same sequence will be applicable for UBI32 core 
crypto driver as well.

  It depends whose request will come first to the crypto HW.





>
> > >
> > > > >
> > > > > > > with LOCK bit set, The BAM will lock all other pipes not related 
to
> > > > > > > the current pipe group, and keep
> > > > > > > handling the current pipe only until it sees the UNLOCK set then 
it
> > > > > > > will release all locked pipes.
> > > > > > > locked pipe will not fetch new descriptors even if it got 
event/events
> > > > > > > adding more descriptors for
> > > > > > > this pipe (locked pipe).
> > > > > > >
> > > > > > > No need to expose as an API to user because its internal to 
driver, so
> > > > > > > while preparing command descriptor
> > > > > > > just we have to update the LOCK/UNLOCK flag.
> > > > >
> > > > > So IIUC, no api right? it would be internal to dr

Re: [PATCH V3] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-01-28 13:22, Manivannan Sadhasivam wrote:

On Sun, Jan 10, 2021 at 09:31:45AM +0530, Md Sadre Alam wrote:

From QPIC version 2.0 onwards new register got added to
read last codeword. This change will update the same.

For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam 


I gave this patch a try on SDX55 board but not able to resolve an issue 
and
I think it is related to reading the last code word which this patch is 
trying
to address. For my patch on supporting QPIC v2 IP, I tested with 
SDX55-MTP board
and I never hit any issue. But on my new dev board (Telit FN980), there 
seems to
be an issue while populating the partitions and tracing down that bug 
lands me

in copy_last_cw() function.

The issue only happens while creating the 3rd partition on Telit board 
whose
size differs when compared with MTP. The board just reboots into QDL 
mode

whenever it tries to read the last code word.

Below is the snippet of partition layout:

Telit partitions:

[1.082015] 0: sbl offs=0x size=0x000a attr:0x00ff
[1.082702] 1: mibib offs=0x000a size=0x000a attr:0x00ff
[1.083488] 2: ico offs=0x0014 size=0x0014 attr:0x00ff
[1.084572] 3: efs2 offs=0x0028 size=0x002c attr:0x00ff
[1.085316] 4: tz offs=0x0054 size=0x0007 attr:0x00ff
[1.086089] 5: tz_devcfg offs=0x005b size=0x0004 
attr:0x00ff



MTP partitions:

[1.573871] 0: sbl offs=0x size=0x000a attr:0x00ff
[1.581139] 1: mibib offs=0x000a size=0x000a attr:0x00ff
[1.587362] 2: efs2 offs=0x0014 size=0x002c attr:0x00ff
[1.593853] 3: tz offs=0x0040 size=0x0007 attr:0x00ff
[1.599860] 4: tz_devcfg offs=0x0047 size=0x0004 
attr:0x00ff

...

So until I figure this out, please keep this patch on hold!


  There was some corner case I missed in V3 patch. I have fixed this in 
V4 patch.
  I have done some tress testing as well with V4 patch on IPQ5018 
platform using "nand-utils"
  and "mtd_test.ko" , Now its working fine. Can you test with V4 patch 
once.


Thanks,
Mani


---
[V3]
 * Added else condition for last code word in update_rw_regs().
 drivers/mtd/nand/raw/qcom_nandc.c | 84 
---

 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c

index 667e4bf..50ff6e3 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
 #defineNAND_READ_LOCATION_10xf24
 #defineNAND_READ_LOCATION_20xf28
 #defineNAND_READ_LOCATION_30xf2c
+#defineNAND_READ_LOCATION_LAST_CW_00xf40
+#defineNAND_READ_LOCATION_LAST_CW_10xf44
+#defineNAND_READ_LOCATION_LAST_CW_20xf48
+#defineNAND_READ_LOCATION_LAST_CW_30xf4c

 /* dummy register offsets, used by write_reg_dma */
 #defineNAND_DEV_CMD1_RESTORE   0xdead
@@ -187,6 +191,12 @@ nandc_set_reg(nandc, 
NAND_READ_LOCATION_##reg,			\

  ((size) << READ_LOCATION_SIZE) |\
  ((is_last) << READ_LOCATION_LAST))

+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
+ ((offset) << READ_LOCATION_OFFSET) |\
+ ((size) << READ_LOCATION_SIZE) |\
+ ((is_last) << READ_LOCATION_LAST))
+
 /*
  * Returns the actual register address for all NAND_DEV_ registers
  * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and 
NAND_DEV_CMD_VLD)

@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+   __le32 read_location_last0;
+   __le32 read_location_last1;
+   __le32 read_location_last2;
+   __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct 
nandc_regs *regs, int offset)

return ®s->read_location2;
case NAND_READ_LOCATION_3:
return ®s->read_location3;
+   case NAND_READ_LOCATION_LAST_CW_0:
+   return ®s->read_location_last0;
+   case NAND_READ_LOCATION_LAST_CW_1:
+   return ®s->read_location_last1;
+   case NAND_READ_LOCATION_LAST_CW_2:
+   return ®s->read_location_last2;
+   case NAND_READ_LOCATION_LAST_CW_3:
+   return ®s->read_location_last3;
default:
return NULL;
}
@@ -719,9 +741,14 @@ static void update_rw_regs(struct qcom_nand_host 
*host, int num_cw, bool read)

nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
nan

Re: [PATCH V3] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-01-29 03:41, Miquel Raynal wrote:

Hello,

mda...@codeaurora.org wrote on Fri, 29 Jan 2021 03:18:46 +0530:


On 2021-01-14 21:23, Miquel Raynal wrote:
> Hello,
>
> Md Sadre Alam  wrote on Sun, 10 Jan 2021
> 09:31:45 +0530:
>
>> From QPIC version 2.0 onwards new register got added to
>
> a
>
>> read last codeword. This change will update the same.
>
>the?   
>   Please reword this sentence.

   Fixed this in V4 patch.
>
>> For first three code word READ_LOCATION_n register will be
>> use.For last code word READ_LOCATION_LAST_CW_n register will be
>> use.
>
> "For the first three codewords, READ_LOCATION_n registers will be used.
> The last codeword register will be accessed through
> READ_LOCATION_LAST_CW_n."
>
> Also, please specify what these registers store.

   The location register is mainly use for reading controller
   buffer via BAM mode. The bits of the register 
"NAND_READ_LOCATION_LAST_CW_n, n=0..4"

   as follow:


Perhaps what I do not understand is: when is this "last_cw" register
more useful than the previous set?


  From QPIC Version 2.0 onwards it is mandatory to use 
"NAND_READ_LOCATION_LAST_CW_n, n=0..4"
  register to extract last code word data from controller buffer. Using 
register "NAND_READ_LOCATION_n, n=0..4"

  we can extract all code words except last code word.


   [9:0]-bits : (OFFSET) This bit defines the offset from the buffer 
base address to be picked up for DMA.
   [25:16]-bits: (SIZE) This bit of every register will define the 
size of the chunk for DMA.
   31-bit :  (LAST) If this bit is set, the controller takes the 
particular register to specify the last chunk
   of data made available for DMA. This chunk is 
part of the internal buffer of the controller.


>


Thanks,
Miquèl

Re: [PATCH V3] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-01-14 21:23, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Sun, 10 Jan 2021
09:31:45 +0530:


From QPIC version 2.0 onwards new register got added to


a


read last codeword. This change will update the same.


   the?   
  Please reword this sentence.


  Fixed this in V4 patch.



For first three code word READ_LOCATION_n register will be
use.For last code word READ_LOCATION_LAST_CW_n register will be
use.


"For the first three codewords, READ_LOCATION_n registers will be used.
The last codeword register will be accessed through
READ_LOCATION_LAST_CW_n."

Also, please specify what these registers store.


  The location register is mainly use for reading controller
  buffer via BAM mode. The bits of the register 
"NAND_READ_LOCATION_LAST_CW_n, n=0..4"

  as follow:
  [9:0]-bits : (OFFSET) This bit defines the offset from the buffer base 
address to be picked up for DMA.
  [25:16]-bits: (SIZE) This bit of every register will define the size 
of the chunk for DMA.
  31-bit :  (LAST) If this bit is set, the controller takes the 
particular register to specify the last chunk
  of data made available for DMA. This chunk is part 
of the internal buffer of the controller.





Signed-off-by: Md Sadre Alam 


Could someone please test this patch?


  I have tested this patch on IPQ5018 platform and its working fine.



---
[V3]
 * Added else condition for last code word in update_rw_regs().
 drivers/mtd/nand/raw/qcom_nandc.c | 84 
---

 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c

index 667e4bf..50ff6e3 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
 #defineNAND_READ_LOCATION_10xf24
 #defineNAND_READ_LOCATION_20xf28
 #defineNAND_READ_LOCATION_30xf2c
+#defineNAND_READ_LOCATION_LAST_CW_00xf40
+#defineNAND_READ_LOCATION_LAST_CW_10xf44
+#defineNAND_READ_LOCATION_LAST_CW_20xf48
+#defineNAND_READ_LOCATION_LAST_CW_30xf4c

 /* dummy register offsets, used by write_reg_dma */
 #defineNAND_DEV_CMD1_RESTORE   0xdead
@@ -187,6 +191,12 @@ nandc_set_reg(nandc, 
NAND_READ_LOCATION_##reg,			\

  ((size) << READ_LOCATION_SIZE) |\
  ((is_last) << READ_LOCATION_LAST))

+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
+ ((offset) << READ_LOCATION_OFFSET) |\
+ ((size) << READ_LOCATION_SIZE) |\
+ ((is_last) << READ_LOCATION_LAST))
+
 /*
  * Returns the actual register address for all NAND_DEV_ registers
  * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and 
NAND_DEV_CMD_VLD)

@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+   __le32 read_location_last0;
+   __le32 read_location_last1;
+   __le32 read_location_last2;
+   __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct 
nandc_regs *regs, int offset)

return ®s->read_location2;
case NAND_READ_LOCATION_3:
return ®s->read_location3;
+   case NAND_READ_LOCATION_LAST_CW_0:
+   return ®s->read_location_last0;
+   case NAND_READ_LOCATION_LAST_CW_1:
+   return ®s->read_location_last1;
+   case NAND_READ_LOCATION_LAST_CW_2:
+   return ®s->read_location_last2;
+   case NAND_READ_LOCATION_LAST_CW_3:
+   return ®s->read_location_last3;
default:
return NULL;
}
@@ -719,9 +741,14 @@ static void update_rw_regs(struct qcom_nand_host 
*host, int num_cw, bool read)

nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
nandc_set_reg(nandc, NAND_EXEC_CMD, 1);

-   if (read)
-   nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
-  host->cw_data : host->cw_size, 1);
+   if (read) {
+   if (nandc->props->qpic_v2)
+   nandc_set_read_loc_last(nandc, 0, 0, host->use_ecc ?
+   host->cw_data : host->cw_size, 1);
+   else
+   nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
+   host->cw_data : host->cw_size, 1);
+   }
 }

 /*
@@ -1096,9 +1123,13 @@ static void config_nand_page_read(struct 
qcom_nand_controller *nandc)

 static void
 config_nand_cw_read(struct qcom_nand_controller *nandc, bool 

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-01-19 22:15, Vinod Koul wrote:

On 18-01-21, 09:21, mda...@codeaurora.org wrote:

On 2021-01-15 11:28, Vinod Koul wrote:
> On 14-01-21, 01:20, mda...@codeaurora.org wrote:
> > On 2021-01-12 15:40, Vinod Koul wrote:
> > > On 12-01-21, 15:01, mda...@codeaurora.org wrote:
> > > > On 2020-12-21 23:03, mda...@codeaurora.org wrote:
> > > > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > > > Hello,
> > > > > >
> > > > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > > > > on CMD descriptor.
> > > > > > >
> > > > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of 
this
> > > > > > > transaction wanted to lock the DMA controller for this 
transaction so
> > > > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > > > >
> > > > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > > > of this
> > > > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > > > should set
> > > > > > > UNLOCK bit for the HW descriptor.
> > > > > >
> > > > > > Can you explain why would we need to first lock and then unlock..? 
How
> > > > > > would this be used in real world.
> > > > > >
> > > > > > I have read a bit of documentation but is unclear to me. Also should
> > > > > > this be exposed as an API to users, sounds like internal to 
driver..?
> > > > > >
> > > > >
> > > > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto 
Hardware
> > > > > Engine
> > > > > will be shared between A53 core & ubi32 core. There is two separate
> > > > > driver dedicated
> > > > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > > > parallelly for encryption/description
> > > > > we need bam locking mechanism. if one driver will submit the request
> > > > > for encryption/description
> > > > > to Crypto then first it has to set LOCK flag bit on command descriptor
> > > > > so that other pipes will
> > > > > get locked.
> > > > >
> > > > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > > > > encountering a command descriptor
> > >
> > > Can you explain what is a cmd descriptor?
> >
> >   In BAM pipe descriptor structure there is a field called CMD
> > (Command
> > descriptor).
> >   CMD allows the SW to create descriptors of type Command which does
> > not
> > generate any data transmissions
> >   but configures registers in the Peripheral (write operations, and
> > read
> > registers operations ).
> >   Using command descriptor enables the SW to queue new configurations
> > between data transfers in advance.
>
> What and when is the CMD descriptor used for..?

  CMD descriptor is mainly used for configuring controller register.
  We can read/write controller register via BAM using CMD descriptor 
only.

  CMD descriptor use command pipe for the transaction.


In which use cases would you need to issue cmd descriptors..?


  In IPQ5018 there is only one Crypto engine and it will get shared 
between
  UBI32 core & A53 core. So here we need to use command descriptor 
in-order to
  perform LOCKING/UNLOCKING mechanism. Since LOCK/ULOCK flag we can set 
only on

  CMD descriptor.



>
> > >
> > > > > with LOCK bit set, The BAM will lock all other pipes not related to
> > > > > the current pipe group, and keep
> > > > > handling the current pipe only until it sees the UNLOCK set then it
> > > > > will release all locked pipes.
> > > > > locked pipe will not fetch new descriptors even if it got event/events
> > > > > adding more descriptors for
> > > > > this pipe (locked pipe).
> > > > >
> > > > > No need to expose as an API to user because its internal to driver, so
> > > > > while preparing command descriptor
> > > > > just we have to update the LOCK/UNLOCK flag.
> > >
> > > So IIUC, no api right? it would be internal to driver..?
> >
> >   Yes its totally internal to deriver.
>
> So no need for this patch then, right?

  This patch is needed , because if some hardware will shared between
  multiple core like A53 and ubi32 for example. In IPQ5018 there is
  only one crypto engine and this will be shared between A53 core and
  ubi32 core and in A53 core & ubi32 core there are different drivers
  is getting used. So if encryption/decryption request come at same
  time from both the driver then things will get messed up. So here we
  need LOCKING mechanism.  If first request is from A53 core driver
  then this driver should lock all the pipes other than pipe dedicated
  to A53 core. So while preparing CMD descriptor driver should used
  this flag "DMA_PREP_LOCK", Since LOCK and UNLOCK flag bit we can set
  only on CMD descriptor. Once request processed then driver will set
  UNLOCK flag on CMD descriptor. Driver should use this flag
  "DMA_PREP_UNLOCK" while preparing CMD descriptor. Same logic will be
  apply for ubi32 core driver as well.


Why cant this be applied at driver level, based on txn being issued it
can lock issue the

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-01-15 11:28, Vinod Koul wrote:

On 14-01-21, 01:20, mda...@codeaurora.org wrote:

On 2021-01-12 15:40, Vinod Koul wrote:
> On 12-01-21, 15:01, mda...@codeaurora.org wrote:
> > On 2020-12-21 23:03, mda...@codeaurora.org wrote:
> > > On 2020-12-21 14:53, Vinod Koul wrote:
> > > > Hello,
> > > >
> > > > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > > > This change will add support for LOCK & UNLOCK flag bit support
> > > > > on CMD descriptor.
> > > > >
> > > > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > > > transaction wanted to lock the DMA controller for this transaction so
> > > > > BAM driver should set LOCK bit for the HW descriptor.
> > > > >
> > > > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > > > of this
> > > > > transaction wanted to unlock the DMA controller.so BAM driver
> > > > > should set
> > > > > UNLOCK bit for the HW descriptor.
> > > >
> > > > Can you explain why would we need to first lock and then unlock..? How
> > > > would this be used in real world.
> > > >
> > > > I have read a bit of documentation but is unclear to me. Also should
> > > > this be exposed as an API to users, sounds like internal to driver..?
> > > >
> > >
> > > IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> > > Engine
> > > will be shared between A53 core & ubi32 core. There is two separate
> > > driver dedicated
> > > to A53 core and ubi32 core. So to use Crypto Hardware Engine
> > > parallelly for encryption/description
> > > we need bam locking mechanism. if one driver will submit the request
> > > for encryption/description
> > > to Crypto then first it has to set LOCK flag bit on command descriptor
> > > so that other pipes will
> > > get locked.
> > >
> > > The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> > > encountering a command descriptor
>
> Can you explain what is a cmd descriptor?

  In BAM pipe descriptor structure there is a field called CMD 
(Command

descriptor).
  CMD allows the SW to create descriptors of type Command which does 
not

generate any data transmissions
  but configures registers in the Peripheral (write operations, and 
read

registers operations ).
  Using command descriptor enables the SW to queue new configurations
between data transfers in advance.


What and when is the CMD descriptor used for..?


  CMD descriptor is mainly used for configuring controller register.
  We can read/write controller register via BAM using CMD descriptor 
only.

  CMD descriptor use command pipe for the transaction.



>
> > > with LOCK bit set, The BAM will lock all other pipes not related to
> > > the current pipe group, and keep
> > > handling the current pipe only until it sees the UNLOCK set then it
> > > will release all locked pipes.
> > > locked pipe will not fetch new descriptors even if it got event/events
> > > adding more descriptors for
> > > this pipe (locked pipe).
> > >
> > > No need to expose as an API to user because its internal to driver, so
> > > while preparing command descriptor
> > > just we have to update the LOCK/UNLOCK flag.
>
> So IIUC, no api right? it would be internal to driver..?

  Yes its totally internal to deriver.


So no need for this patch then, right?


  This patch is needed , because if some hardware will shared between 
multiple core
  like A53 and ubi32 for example. In IPQ5018 there is only one crypto 
engine and this will
  be shared between A53 core and ubi32 core and in A53 core & ubi32 core 
there are different
  drivers is getting used. So if encryption/decryption request come at 
same time from both the
  driver then things will get messed up. So here we need LOCKING 
mechanism. If first request is
  from A53 core driver then this driver should lock all the pipes other 
than pipe dedicated to
  A53 core. So while preparing CMD descriptor driver should used this 
flag "DMA_PREP_LOCK",
  Since LOCK and UNLOCK flag bit we can set only on CMD descriptor. Once 
request processed then
  driver will set UNLOCK flag on CMD descriptor. Driver should use this 
flag "DMA_PREP_UNLOCK"
  while preparing CMD descriptor. Same logic will be apply for ubi32 
core driver as well.

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2021-01-12 15:40, Vinod Koul wrote:

On 12-01-21, 15:01, mda...@codeaurora.org wrote:

On 2020-12-21 23:03, mda...@codeaurora.org wrote:
> On 2020-12-21 14:53, Vinod Koul wrote:
> > Hello,
> >
> > On 17-12-20, 20:07, Md Sadre Alam wrote:
> > > This change will add support for LOCK & UNLOCK flag bit support
> > > on CMD descriptor.
> > >
> > > If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
> > > transaction wanted to lock the DMA controller for this transaction so
> > > BAM driver should set LOCK bit for the HW descriptor.
> > >
> > > If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester
> > > of this
> > > transaction wanted to unlock the DMA controller.so BAM driver
> > > should set
> > > UNLOCK bit for the HW descriptor.
> >
> > Can you explain why would we need to first lock and then unlock..? How
> > would this be used in real world.
> >
> > I have read a bit of documentation but is unclear to me. Also should
> > this be exposed as an API to users, sounds like internal to driver..?
> >
>
> IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware
> Engine
> will be shared between A53 core & ubi32 core. There is two separate
> driver dedicated
> to A53 core and ubi32 core. So to use Crypto Hardware Engine
> parallelly for encryption/description
> we need bam locking mechanism. if one driver will submit the request
> for encryption/description
> to Crypto then first it has to set LOCK flag bit on command descriptor
> so that other pipes will
> get locked.
>
> The Pipe Locking/Unlocking will be only on command-descriptor. Upon
> encountering a command descriptor


Can you explain what is a cmd descriptor?


  In BAM pipe descriptor structure there is a field called CMD (Command 
descriptor).
  CMD allows the SW to create descriptors of type Command which does not 
generate any data transmissions
  but configures registers in the Peripheral (write operations, and read 
registers operations ).
  Using command descriptor enables the SW to queue new configurations 
between data transfers in advance.





> with LOCK bit set, The BAM will lock all other pipes not related to
> the current pipe group, and keep
> handling the current pipe only until it sees the UNLOCK set then it
> will release all locked pipes.
> locked pipe will not fetch new descriptors even if it got event/events
> adding more descriptors for
> this pipe (locked pipe).
>
> No need to expose as an API to user because its internal to driver, so
> while preparing command descriptor
> just we have to update the LOCK/UNLOCK flag.


So IIUC, no api right? it would be internal to driver..?


  Yes its totally internal to deriver.

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2020-12-21 23:03, mda...@codeaurora.org wrote:

On 2020-12-21 14:53, Vinod Koul wrote:

Hello,

On 17-12-20, 20:07, Md Sadre Alam wrote:

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
transaction wanted to lock the DMA controller for this transaction so
BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
this
transaction wanted to unlock the DMA controller.so BAM driver should 
set

UNLOCK bit for the HW descriptor.


Can you explain why would we need to first lock and then unlock..? How
would this be used in real world.

I have read a bit of documentation but is unclear to me. Also should
this be exposed as an API to users, sounds like internal to driver..?



IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto 
Hardware Engine

will be shared between A53 core & ubi32 core. There is two separate
driver dedicated
to A53 core and ubi32 core. So to use Crypto Hardware Engine
parallelly for encryption/description
we need bam locking mechanism. if one driver will submit the request
for encryption/description
to Crypto then first it has to set LOCK flag bit on command descriptor
so that other pipes will
get locked.

The Pipe Locking/Unlocking will be only on command-descriptor. Upon
encountering a command descriptor
with LOCK bit set, The BAM will lock all other pipes not related to
the current pipe group, and keep
handling the current pipe only until it sees the UNLOCK set then it
will release all locked pipes.
locked pipe will not fetch new descriptors even if it got event/events
adding more descriptors for
this pipe (locked pipe).

No need to expose as an API to user because its internal to driver, so
while preparing command descriptor
just we have to update the LOCK/UNLOCK flag.



ping! Is there any update on this ? Do you need any further info ?







Signed-off-by: Md Sadre Alam 
---
 Documentation/driver-api/dmaengine/provider.rst | 9 +
 drivers/dma/qcom/bam_dma.c  | 9 +
 include/linux/dmaengine.h   | 5 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/driver-api/dmaengine/provider.rst 
b/Documentation/driver-api/dmaengine/provider.rst

index ddb0a81..d7516e2 100644
--- a/Documentation/driver-api/dmaengine/provider.rst
+++ b/Documentation/driver-api/dmaengine/provider.rst
@@ -599,6 +599,15 @@ DMA_CTRL_REUSE
   - This flag is only supported if the channel reports the 
DMA_LOAD_EOT

 capability.

+- DMA_PREP_LOCK
+
+  - If set , the client driver tells DMA controller I am locking you 
for

+this transcation.
+
+- DMA_PREP_UNLOCK
+
+  - If set, the client driver will tells DMA controller I am 
releasing the lock

+
 General Design Notes
 

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 4eeb8bb..cdbe395 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -58,6 +58,8 @@ struct bam_desc_hw {
 #define DESC_FLAG_EOB BIT(13)
 #define DESC_FLAG_NWD BIT(12)
 #define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)

 struct bam_async_desc {
struct virt_dma_desc vd;
@@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
*bam_prep_slave_sg(struct dma_chan *chan,


/* fill in temporary descriptors */
desc = async_desc->desc;
+   if (flags & DMA_PREP_CMD) {
+   if (flags & DMA_PREP_LOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
+   if (flags & DMA_PREP_UNLOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
+   }
+
for_each_sg(sgl, sg, sg_len, i) {
unsigned int remainder = sg_dma_len(sg);
unsigned int curr_offset = 0;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index dd357a7..79ccadb4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -190,6 +190,9 @@ struct dma_interleaved_template {
  *  transaction is marked with DMA_PREP_REPEAT will cause the new 
transaction
  *  to never be processed and stay in the issued queue forever. The 
flag is
  *  ignored if the previous transaction is not a repeated 
transaction.
+ * @DMA_PREP_LOCK: tell the driver that DMA HW engine going to be 
locked for this

+ *  transaction , until not seen DMA_PREP_UNLOCK flag set.
+ * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
  */
 enum dma_ctrl_flags {
DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,8 @@ enum dma_ctrl_flags {
DMA_PREP_CMD = (1 << 7),
DMA_PREP_REPEAT = (1 << 8),
DMA_PREP_LOAD_EOT = (1 << 9),
+   DMA_PREP_LOCK = (1 << 10),
+   DMA_PREP_UNLOCK = (1 << 11),
 };

 /**
--
2.7.4

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2020-12-22 17:48, mda...@codeaurora.org wrote:

On 2020-12-21 23:39, Thara Gopinath wrote:

On 12/21/20 2:35 AM, mda...@codeaurora.org wrote:

On 2020-12-19 09:05, Thara Gopinath wrote:

On 12/17/20 9:37 AM, Md Sadre Alam wrote:

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of 
this
transaction wanted to lock the DMA controller for this transaction 
so

BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
this
transaction wanted to unlock the DMA controller.so BAM driver 
should set

UNLOCK bit for the HW descriptor.

Hi,

This is a generic question. What is the point of LOCK/UNLOCK with
allocating LOCK groups to the individual dma channels? By default
doesn't all channels fall in the same group. This would mean that
a lock does not prevent the dma controller from not executing a
transaction on the other channels.



The Pipe Locking/Unlocking will be only on command-descriptor.
Upon encountering a command descriptor with LOCK bit set, the BAM
will lock all other pipes not related to the current pipe group, and 
keep
handling the current pipe only until it sees the UNLOCK set then it 
will

release all locked pipes.


So unless you assign pipe groups, this will not work as intended
right? So this patch is only half of the solution. There should also
be a patch allowing pipe groups to be assigned. Without that extra bit
this patch does nothing , right ?


Yes you are right.
We are having some register which will configure the pipe lock group.
But these registers are not exposed to non-secure world. These 
registers

only accessible through secure world. Currently in IPQ5018 SoC we are
configuring
these register in secure world to configure pipe lock group.


ping! Is there any update on this ? Do you need any further info ?

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2021-01-05 21:15, Manivannan Sadhasivam wrote:

On Tue, Jan 05, 2021 at 12:24:45AM +0530, mda...@codeaurora.org wrote:

On 2020-12-31 16:23, Manivannan Sadhasivam wrote:
> On Thu, Dec 17, 2020 at 07:32:56PM +0530, Md Sadre Alam wrote:
> > From QPIC version 2.0 onwards new register got added to
> > read last codeword. This change will update the same.
> >
> > For first three code word READ_LOCATION_n register will be
> > use.For last code wrod READ_LOCATION_LAST_CW_n register will be
> > use.
> >
> > Signed-off-by: Md Sadre Alam 
> > ---
> >  drivers/mtd/nand/raw/qcom_nandc.c | 79
> > +--
> >  1 file changed, 67 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> > b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 667e4bf..eaef51d 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -48,6 +48,10 @@
> >  #define  NAND_READ_LOCATION_10xf24
> >  #define  NAND_READ_LOCATION_20xf28
> >  #define  NAND_READ_LOCATION_30xf2c
> > +#define NAND_READ_LOCATION_LAST_CW_00xf40
> > +#define NAND_READ_LOCATION_LAST_CW_10xf44
> > +#define NAND_READ_LOCATION_LAST_CW_20xf48
> > +#define NAND_READ_LOCATION_LAST_CW_30xf4c
>
> Please keep the alignment as before.
>
 Fixed alignment in V2 patch
> >
> >  /* dummy register offsets, used by write_reg_dma */
> >  #define  NAND_DEV_CMD1_RESTORE   0xdead
> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc,
> > NAND_READ_LOCATION_##reg, \
> > ((size) << READ_LOCATION_SIZE) |\
> > ((is_last) << READ_LOCATION_LAST))
> >
> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last)   \
> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, 
  \
> > +   ((offset) << READ_LOCATION_OFFSET) |\
> > +   ((size) << READ_LOCATION_SIZE) |\
> > +   ((is_last) << READ_LOCATION_LAST))
> > +
> >  /*
> >   * Returns the actual register address for all NAND_DEV_ registers
> >   * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and
> > NAND_DEV_CMD_VLD)
> > @@ -316,6 +326,10 @@ struct nandc_regs {
> >   __le32 read_location1;
> >   __le32 read_location2;
> >   __le32 read_location3;
> > + __le32 read_location_last0;
> > + __le32 read_location_last1;
> > + __le32 read_location_last2;
> > + __le32 read_location_last3;
> >
> >   __le32 erased_cw_detect_cfg_clr;
> >   __le32 erased_cw_detect_cfg_set;
> > @@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct
> > nandc_regs *regs, int offset)
> >   return ®s->read_location2;
> >   case NAND_READ_LOCATION_3:
> >   return ®s->read_location3;
> > + case NAND_READ_LOCATION_LAST_CW_0:
> > + return ®s->read_location_last0;
> > + case NAND_READ_LOCATION_LAST_CW_1:
> > + return ®s->read_location_last1;
> > + case NAND_READ_LOCATION_LAST_CW_2:
> > + return ®s->read_location_last2;
> > + case NAND_READ_LOCATION_LAST_CW_3:
> > + return ®s->read_location_last3;
> >   default:
> >   return NULL;
> >   }
> > @@ -719,9 +741,13 @@ static void update_rw_regs(struct
> > qcom_nand_host *host, int num_cw, bool read)
> >   nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
> >   nandc_set_reg(nandc, NAND_EXEC_CMD, 1);
> >
> > - if (read)
> > + if (read) {
> > + if (nandc->props->qpic_v2)
> > + nandc_set_read_loc_last(nandc, 0, 0, host->use_ecc ?
> > + host->cw_data : host->cw_size, 1);
>
> Forgot to add else? Otherwise both NAND_READ_LOCATION_n and
> NAND_READ_LOCATION_LAST_CW_n
> will be used.

  Here else is not needed , because to read last code word we need to
configure
  NAND_READ_LOCATION_LAST_CW_n register. Any way here we are doing 
only

register configuration.
  for all the code words. Earlier version of QPIC we were using
nandc_set_read_loc()
  for all the code words, but in qpic V2 onwards for last code word we 
have

to use
  NAND_READ_LOCATION_LAST_CW_n register. So configuring here the same.



nandc_set_read_loc() has the last argument "is_last". This is used to 
convey
whether we need to set READ_LOCATION_LAST bit or not. This is fine for 
QPIC
IP < 2, but for >=2 we need to use nandc_set_read_loc_last() only. My 
point
is why do you need to still use nandc_set_read_loc() here for QPIC v2? 
That's

why I asked you about using else().


  Got it. I fixed this in V3 patch.


>
> >   nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
> >  host->cw_data : host->cw_size, 1);
> > + }
> >  }
> >
> >  /*
> > @@ -1096,9 +1122,13 @@ static void config_nand_page_read(struct
> > qcom_nand_controller *nandc)
> >  static void
> >  config_nand_cw

Re: [PATCH] mtd: rawnand: qcom: update last code word register

[mdalam]

On 2020-12-31 16:23, Manivannan Sadhasivam wrote:

On Thu, Dec 17, 2020 at 07:32:56PM +0530, Md Sadre Alam wrote:

From QPIC version 2.0 onwards new register got added to
read last codeword. This change will update the same.

For first three code word READ_LOCATION_n register will be
use.For last code wrod READ_LOCATION_LAST_CW_n register will be
use.

Signed-off-by: Md Sadre Alam 
---
 drivers/mtd/nand/raw/qcom_nandc.c | 79 
+--

 1 file changed, 67 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c 
b/drivers/mtd/nand/raw/qcom_nandc.c

index 667e4bf..eaef51d 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -48,6 +48,10 @@
 #defineNAND_READ_LOCATION_10xf24
 #defineNAND_READ_LOCATION_20xf28
 #defineNAND_READ_LOCATION_30xf2c
+#define NAND_READ_LOCATION_LAST_CW_00xf40
+#define NAND_READ_LOCATION_LAST_CW_10xf44
+#define NAND_READ_LOCATION_LAST_CW_20xf48
+#define NAND_READ_LOCATION_LAST_CW_30xf4c


Please keep the alignment as before.


 Fixed alignment in V2 patch


 /* dummy register offsets, used by write_reg_dma */
 #defineNAND_DEV_CMD1_RESTORE   0xdead
@@ -187,6 +191,12 @@ nandc_set_reg(nandc, 
NAND_READ_LOCATION_##reg,			\

  ((size) << READ_LOCATION_SIZE) |\
  ((is_last) << READ_LOCATION_LAST))

+#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
+nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg, \
+ ((offset) << READ_LOCATION_OFFSET) |\
+ ((size) << READ_LOCATION_SIZE) |\
+ ((is_last) << READ_LOCATION_LAST))
+
 /*
  * Returns the actual register address for all NAND_DEV_ registers
  * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and 
NAND_DEV_CMD_VLD)

@@ -316,6 +326,10 @@ struct nandc_regs {
__le32 read_location1;
__le32 read_location2;
__le32 read_location3;
+   __le32 read_location_last0;
+   __le32 read_location_last1;
+   __le32 read_location_last2;
+   __le32 read_location_last3;

__le32 erased_cw_detect_cfg_clr;
__le32 erased_cw_detect_cfg_set;
@@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct 
nandc_regs *regs, int offset)

return ®s->read_location2;
case NAND_READ_LOCATION_3:
return ®s->read_location3;
+   case NAND_READ_LOCATION_LAST_CW_0:
+   return ®s->read_location_last0;
+   case NAND_READ_LOCATION_LAST_CW_1:
+   return ®s->read_location_last1;
+   case NAND_READ_LOCATION_LAST_CW_2:
+   return ®s->read_location_last2;
+   case NAND_READ_LOCATION_LAST_CW_3:
+   return ®s->read_location_last3;
default:
return NULL;
}
@@ -719,9 +741,13 @@ static void update_rw_regs(struct qcom_nand_host 
*host, int num_cw, bool read)

nandc_set_reg(nandc, NAND_READ_STATUS, host->clrreadstatus);
nandc_set_reg(nandc, NAND_EXEC_CMD, 1);

-   if (read)
+   if (read) {
+   if (nandc->props->qpic_v2)
+   nandc_set_read_loc_last(nandc, 0, 0, host->use_ecc ?
+   host->cw_data : host->cw_size, 1);


Forgot to add else? Otherwise both NAND_READ_LOCATION_n and
NAND_READ_LOCATION_LAST_CW_n
will be used.


  Here else is not needed , because to read last code word we need to 
configure
  NAND_READ_LOCATION_LAST_CW_n register. Any way here we are doing only 
register configuration.
  for all the code words. Earlier version of QPIC we were using  
nandc_set_read_loc()
  for all the code words, but in qpic V2 onwards for last code word we 
have to use

  NAND_READ_LOCATION_LAST_CW_n register. So configuring here the same.





nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
   host->cw_data : host->cw_size, 1);
+   }
 }

 /*
@@ -1096,9 +1122,13 @@ static void config_nand_page_read(struct 
qcom_nand_controller *nandc)

 static void
 config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
 {
-   if (nandc->props->is_bam)
+   if (nandc->props->is_bam) {
+   if (nandc->props->qpic_v2)
+   write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0,
+ 4, NAND_BAM_NEXT_SGL);
write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
  NAND_BAM_NEXT_SGL);


Don't you need to modify the number of registers to write? It can't be 
4 all the

time if NAND_READ_LOCATION_LAST_CW_0 is used.


  Changed number of registers to write from 4 to 1 in V2 patch for 
register NAND_READ_LOCATION_LAST_CW_0 .



+   }

write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_B

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2020-12-21 23:39, Thara Gopinath wrote:

On 12/21/20 2:35 AM, mda...@codeaurora.org wrote:

On 2020-12-19 09:05, Thara Gopinath wrote:

On 12/17/20 9:37 AM, Md Sadre Alam wrote:

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
transaction wanted to lock the DMA controller for this transaction 
so

BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of 
this
transaction wanted to unlock the DMA controller.so BAM driver should 
set

UNLOCK bit for the HW descriptor.

Hi,

This is a generic question. What is the point of LOCK/UNLOCK with
allocating LOCK groups to the individual dma channels? By default
doesn't all channels fall in the same group. This would mean that
a lock does not prevent the dma controller from not executing a
transaction on the other channels.



The Pipe Locking/Unlocking will be only on command-descriptor.
Upon encountering a command descriptor with LOCK bit set, the BAM
will lock all other pipes not related to the current pipe group, and 
keep
handling the current pipe only until it sees the UNLOCK set then it 
will

release all locked pipes.


So unless you assign pipe groups, this will not work as intended
right? So this patch is only half of the solution. There should also
be a patch allowing pipe groups to be assigned. Without that extra bit
this patch does nothing , right ?


Yes you are right.
We are having some register which will configure the pipe lock group.
But these registers are not exposed to non-secure world. These registers
only accessible through secure world. Currently in IPQ5018 SoC we are 
configuring

these register in secure world to configure pipe lock group.

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2020-12-21 14:53, Vinod Koul wrote:

Hello,

On 17-12-20, 20:07, Md Sadre Alam wrote:

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
transaction wanted to lock the DMA controller for this transaction so
BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
transaction wanted to unlock the DMA controller.so BAM driver should 
set

UNLOCK bit for the HW descriptor.


Can you explain why would we need to first lock and then unlock..? How
would this be used in real world.

I have read a bit of documentation but is unclear to me. Also should
this be exposed as an API to users, sounds like internal to driver..?



IPQ5018 SoC having only one Crypto Hardware Engine. This Crypto Hardware 
Engine
will be shared between A53 core & ubi32 core. There is two separate 
driver dedicated
to A53 core and ubi32 core. So to use Crypto Hardware Engine parallelly 
for encryption/description
we need bam locking mechanism. if one driver will submit the request for 
encryption/description
to Crypto then first it has to set LOCK flag bit on command descriptor 
so that other pipes will

get locked.

The Pipe Locking/Unlocking will be only on command-descriptor. Upon 
encountering a command descriptor
with LOCK bit set, The BAM will lock all other pipes not related to the 
current pipe group, and keep
handling the current pipe only until it sees the UNLOCK set then it will 
release all locked pipes.
locked pipe will not fetch new descriptors even if it got event/events 
adding more descriptors for

this pipe (locked pipe).

No need to expose as an API to user because its internal to driver, so 
while preparing command descriptor

just we have to update the LOCK/UNLOCK flag.






Signed-off-by: Md Sadre Alam 
---
 Documentation/driver-api/dmaengine/provider.rst | 9 +
 drivers/dma/qcom/bam_dma.c  | 9 +
 include/linux/dmaengine.h   | 5 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/driver-api/dmaengine/provider.rst 
b/Documentation/driver-api/dmaengine/provider.rst

index ddb0a81..d7516e2 100644
--- a/Documentation/driver-api/dmaengine/provider.rst
+++ b/Documentation/driver-api/dmaengine/provider.rst
@@ -599,6 +599,15 @@ DMA_CTRL_REUSE
   - This flag is only supported if the channel reports the 
DMA_LOAD_EOT

 capability.

+- DMA_PREP_LOCK
+
+  - If set , the client driver tells DMA controller I am locking you 
for

+this transcation.
+
+- DMA_PREP_UNLOCK
+
+  - If set, the client driver will tells DMA controller I am 
releasing the lock

+
 General Design Notes
 

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 4eeb8bb..cdbe395 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -58,6 +58,8 @@ struct bam_desc_hw {
 #define DESC_FLAG_EOB BIT(13)
 #define DESC_FLAG_NWD BIT(12)
 #define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)

 struct bam_async_desc {
struct virt_dma_desc vd;
@@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
*bam_prep_slave_sg(struct dma_chan *chan,


/* fill in temporary descriptors */
desc = async_desc->desc;
+   if (flags & DMA_PREP_CMD) {
+   if (flags & DMA_PREP_LOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
+   if (flags & DMA_PREP_UNLOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
+   }
+
for_each_sg(sgl, sg, sg_len, i) {
unsigned int remainder = sg_dma_len(sg);
unsigned int curr_offset = 0;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index dd357a7..79ccadb4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -190,6 +190,9 @@ struct dma_interleaved_template {
  *  transaction is marked with DMA_PREP_REPEAT will cause the new 
transaction
  *  to never be processed and stay in the issued queue forever. The 
flag is
  *  ignored if the previous transaction is not a repeated 
transaction.
+ * @DMA_PREP_LOCK: tell the driver that DMA HW engine going to be 
locked for this

+ *  transaction , until not seen DMA_PREP_UNLOCK flag set.
+ * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
  */
 enum dma_ctrl_flags {
DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,8 @@ enum dma_ctrl_flags {
DMA_PREP_CMD = (1 << 7),
DMA_PREP_REPEAT = (1 << 8),
DMA_PREP_LOAD_EOT = (1 << 9),
+   DMA_PREP_LOCK = (1 << 10),
+   DMA_PREP_UNLOCK = (1 << 11),
 };

 /**
--
2.7.4

Re: [PATCH] dmaengine: qcom: bam_dma: Add LOCK and UNLOCK flag bit support

[mdalam]

On 2020-12-19 09:05, Thara Gopinath wrote:

On 12/17/20 9:37 AM, Md Sadre Alam wrote:

This change will add support for LOCK & UNLOCK flag bit support
on CMD descriptor.

If DMA_PREP_LOCK flag passed in prep_slave_sg then requester of this
transaction wanted to lock the DMA controller for this transaction so
BAM driver should set LOCK bit for the HW descriptor.

If DMA_PREP_UNLOCK flag passed in prep_slave_sg then requester of this
transaction wanted to unlock the DMA controller.so BAM driver should 
set

UNLOCK bit for the HW descriptor.

Hi,

This is a generic question. What is the point of LOCK/UNLOCK with
allocating LOCK groups to the individual dma channels? By default
doesn't all channels fall in the same group. This would mean that
a lock does not prevent the dma controller from not executing a
transaction on the other channels.



The Pipe Locking/Unlocking will be only on command-descriptor.
Upon encountering a command descriptor with LOCK bit set, the BAM
will lock all other pipes not related to the current pipe group, and 
keep

handling the current pipe only until it sees the UNLOCK set then it will
release all locked pipes.

The actual locking is done on the new descriptor fetching for 
publishing,
i.e. locked pipe will not fetch new descriptors even if it got 
event/events

adding more descriptors for this pipe (locked pipe).

The bam LOCKING mechanism is needed where different cores needs to share
same hardware block which use bam for their transaction. So if both 
cores
wanted to access the hardware block in parallel via bam, then locking 
mechanism

is needed for bam pipes.


--
Warm Regards
Thara



Signed-off-by: Md Sadre Alam 
---
  Documentation/driver-api/dmaengine/provider.rst | 9 +
  drivers/dma/qcom/bam_dma.c  | 9 +
  include/linux/dmaengine.h   | 5 +
  3 files changed, 23 insertions(+)

diff --git a/Documentation/driver-api/dmaengine/provider.rst 
b/Documentation/driver-api/dmaengine/provider.rst

index ddb0a81..d7516e2 100644
--- a/Documentation/driver-api/dmaengine/provider.rst
+++ b/Documentation/driver-api/dmaengine/provider.rst
@@ -599,6 +599,15 @@ DMA_CTRL_REUSE
- This flag is only supported if the channel reports the 
DMA_LOAD_EOT

  capability.
  +- DMA_PREP_LOCK
+
+  - If set , the client driver tells DMA controller I am locking you 
for

+this transcation.
+
+- DMA_PREP_UNLOCK
+
+  - If set, the client driver will tells DMA controller I am 
releasing the lock

+
  General Design Notes
  
  diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 4eeb8bb..cdbe395 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -58,6 +58,8 @@ struct bam_desc_hw {
  #define DESC_FLAG_EOB BIT(13)
  #define DESC_FLAG_NWD BIT(12)
  #define DESC_FLAG_CMD BIT(11)
+#define DESC_FLAG_LOCK BIT(10)
+#define DESC_FLAG_UNLOCK BIT(9)
struct bam_async_desc {
struct virt_dma_desc vd;
@@ -644,6 +646,13 @@ static struct dma_async_tx_descriptor 
*bam_prep_slave_sg(struct dma_chan *chan,

/* fill in temporary descriptors */
desc = async_desc->desc;
+   if (flags & DMA_PREP_CMD) {
+   if (flags & DMA_PREP_LOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_LOCK);
+   if (flags & DMA_PREP_UNLOCK)
+   desc->flags |= cpu_to_le16(DESC_FLAG_UNLOCK);
+   }
+
for_each_sg(sgl, sg, sg_len, i) {
unsigned int remainder = sg_dma_len(sg);
unsigned int curr_offset = 0;
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index dd357a7..79ccadb4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -190,6 +190,9 @@ struct dma_interleaved_template {
   *  transaction is marked with DMA_PREP_REPEAT will cause the new 
transaction
   *  to never be processed and stay in the issued queue forever. The 
flag is
   *  ignored if the previous transaction is not a repeated 
transaction.
+ * @DMA_PREP_LOCK: tell the driver that DMA HW engine going to be 
locked for this

+ *  transaction , until not seen DMA_PREP_UNLOCK flag set.
+ * @DMA_PREP_UNLOCK: tell the driver to unlock the DMA HW engine.
   */
  enum dma_ctrl_flags {
DMA_PREP_INTERRUPT = (1 << 0),
@@ -202,6 +205,8 @@ enum dma_ctrl_flags {
DMA_PREP_CMD = (1 << 7),
DMA_PREP_REPEAT = (1 << 8),
DMA_PREP_LOAD_EOT = (1 << 9),
+   DMA_PREP_LOCK = (1 << 10),
+   DMA_PREP_UNLOCK = (1 << 11),
  };
/**

Re: [PATCH 2/5] mtd: rawnand: qcom: Add initial support for qspi nand

[mdalam]

On 2020-10-29 14:37, Boris Brezillon wrote:

Hello,

On Sat, 10 Oct 2020 11:01:39 +0530
Md Sadre Alam  wrote:


This change will add initial support for qspi (serial nand).

QPIC Version v.2.0 onwards supports serial nand as well so this
change will initialize all required register to enable qspi (serial
nand).

This change is supporting very basic functionality of qspi nand flash.

1. Reset device (Reset QSPI NAND device).

2. Device detection (Read id QSPI NAND device).


Unfortunately, that's not going to work in the long term. You're
basically hacking the raw NAND framework to make SPI NANDs fit. I do
understand the rationale behind this decision (re-using the code for
ECC and probably other things), but that's not going to work. So I'd
recommend doing the following instead:

1/ implement a SPI-mem controller driver
2/ implement an ECC engine driver so the ECC logic can be shared
   between the SPI controller and raw NAND controller drivers
3/ convert the raw NAND driver to the exec_op() interface (none of
   this hack would have been possible if the driver was using the new
   API)


Sorry for late reply. I think I mixup the serial nand support and 
QPIC_V2.0 support.

Only patches [2/5] mtd: rawnand: qcom: Add initial support for qspi nand
and [5/5] mtd: rawnand: qcom: Add support for serial training. are for 
serial

nand. Other patches [3/5] mtd: rawnand: qcom: Read QPIC version &
[4/5] mtd: rawnand: qcom: Enable support for erase,read & write for 
serial nand.
are to support QPIC_V2.0. In QPIC_V2.0 onwards some additional registers 
and features
got added. QPIC_NAND_READ_LOCATION_LAST_CW_n register got added to read 
last code word.
Page scope read & multi page read feature got added to read single and 
multiple
pages. QPIC_NAND_AUTO_STATUS_EN register got added to read status in 
page scope read

& multi page read etc.

I will take out QPIC_V2.0 support patches and will push it separately.
For serial nand support few lines of codes are there around 50 lines
to initalize QPIC serial block and serial training code. So can I put 
this
this as a separate file inside drivers/mtd/nand/raw/qpic_serial_nand.c. 
Would it be ok ?
Because there is no dedicated spi controller for serial nand. QPIC 
controller having

one serial interface block to deal with serial nand device.



Regards,

Boris

Re: [PATCH 0/5] mtd: rawnand: qcom: Add support for QSPI nand

[mdalam]

On 2020-10-28 15:18, Miquel Raynal wrote:

Hello,

Md Sadre Alam  wrote on Sat, 10 Oct 2020
11:01:37 +0530:


QPIC 2.0 supports Serial NAND support in addition to all features and
commands in QPIC 1.0 for parallel NAND. Parallel and Serial NAND 
cannot
operate simultaneously. QSPI nand devices will connect to QPIC 
IO_MACRO
block of QPIC controller. There is a separate IO_MACRO clock for 
IO_MACRO
block. Default IO_MACRO block divide the input clock by 4. so if 
IO_MACRO
input clock is 320MHz then on bus it will be 80MHz, so QSPI nand 
device

should also support this frequency.

QPIC provides 4 data pins to QSPI nand. In standard SPI mode (x1 mode) 
data
transfer will occur on only 2 pins one pin for Serial data in and one 
for
serial data out. In QUAD SPI mode (x4 mode) data transfer will occur 
at all
the four data lines. QPIC controller supports command for x1 mode and 
x4 mode.


Md Sadre Alam (5):
  dt-bindings: qcom_nandc: IPQ5018 QPIC NAND documentation
  mtd: rawnand: qcom: Add initial support for qspi nand
  mtd: rawnand: qcom: Read QPIC version
  mtd: rawnand: qcom: Enable support for erase,read & write for serial
nand.
  mtd: rawnand: qcom: Add support for serial training.

 .../devicetree/bindings/mtd/qcom_nandc.txt |   3 +
 drivers/mtd/nand/raw/nand_ids.c|  13 +
 drivers/mtd/nand/raw/qcom_nandc.c  | 502 
-

 3 files changed, 494 insertions(+), 24 deletions(-)



I'm sorry but this series clearly breaks the current layering. I cannot
authorize SPI-NAND code to fall into the raw NAND subsystem.



I am agree with you, we should not add SPI-NAND changes inside
raw NAND subsystem.


As both typologies cannot be used at the same time, I guess you should
have another driver handling this feature under the spi/ subsystem +
a few declarations in the SPI-NAND devices list.



Initially I was started writing separate driver under SPI-NAND subsystem 
then I

realized that more than 85% of raw/qcom_nand.c code getting duplicated.

That's why I have added this SPI-NAND change in raw/qcom_nand.c since
more than 85% of code will be reused.

If I will add this change inside SPI-NAND subsystem then much of
raw/qcom_nand.c code will get duplicated. Would it be ok ?


Thanks,
Miquèl