[PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Venkatesh Yadav Abbarapu]

Update the spi_nor_read() function only for parallel configuration
when config SPI_STACKED_PARALLEL is enabled and keep the default
code as is. For parallel configurations fix the read issue for
4byte address width by passing the entire length to the read
function, split the memory of 16MB size banks only when the
address width is 3byte. Also update the size when the
configuration is stacked.

Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories support")
Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Tested-by: Jonathan Humphreys <j-humphreys@ti.com>
---
Changes in v2:
- Update the nor read for parallel configuration and for other case
  keep the code as is.
- Fixed the commit description.
 - Tested the changes with s25fl128s flash
 Zynq> sf probe 0 0 0
 SF: Detected s25fl128s with page size 256 Bytes, erase size 64 KiB, 
 total 16 MiB
 Zynq> time sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;time 
 Zynq> sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;time 
 Zynq> sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x1000000
 SF: 16777216 bytes @ 0x0 Erased: OK
 
 time: 32.464 seconds
 device 0 whole chip
 SF: 16777216 bytes @ 0x0 Written: OK
 
 time: 15.515 seconds
 device 0 whole chip
 SF: 16777216 bytes @ 0x0 Read: OK
 
 time: 1.557 seconds
 Total of 16777216 byte(s) were the same

Changes in v3:
- Compile out the parallel code in spi_nor_erase() if the SPI_STACKED_PARALLEL 
  is enabled. 
---

 drivers/mtd/spi/spi-nor-core.c | 68 +++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index ec841fb13b..691962ef27 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1130,17 +1130,19 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 			goto erase_err;
 		}
 		offset = addr;
-		if (nor->flags & SNOR_F_HAS_PARALLEL)
-			offset /= 2;
-
-		if (nor->flags & SNOR_F_HAS_STACKED) {
-			if (offset >= (mtd->size / 2))
-				nor->spi->flags |= SPI_XFER_U_PAGE;
-			else
-				nor->spi->flags &= ~SPI_XFER_U_PAGE;
+		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
+			if (nor->flags & SNOR_F_HAS_PARALLEL)
+				offset /= 2;
+
+			if (nor->flags & SNOR_F_HAS_STACKED) {
+				if (offset >= (mtd->size / 2))
+					nor->spi->flags |= SPI_XFER_U_PAGE;
+				else
+					nor->spi->flags &= ~SPI_XFER_U_PAGE;
+			}
 		}
 #ifdef CONFIG_SPI_FLASH_BAR
-		ret = write_bar(nor, addr);
+		ret = write_bar(nor, offset);
 		if (ret < 0)
 			goto erase_err;
 #endif
@@ -1152,7 +1154,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		    !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
 			ret = spi_nor_erase_chip(nor);
 		} else {
-			ret = spi_nor_erase_sector(nor, addr);
+			ret = spi_nor_erase_sector(nor, offset);
 		}
 		if (ret < 0)
 			goto erase_err;
@@ -1578,8 +1580,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
 	int ret;
 	loff_t offset = from;
-	u32 read_len = 0;
 	u32 rem_bank_len = 0;
+	u32 stack_shift = 0;
 	u8 bank;
 	bool is_ofst_odd = false;
 
@@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 
 	while (len) {
-		bank = (u32)from / SZ_16M;
-		if (nor->flags & SNOR_F_HAS_PARALLEL)
-			bank /= 2;
-
-		rem_bank_len = SZ_16M * (bank + 1);
-		if (nor->flags & SNOR_F_HAS_PARALLEL)
-			rem_bank_len *= 2;
-		rem_bank_len -= from;
-
+		size_t read_len = len;
 		offset = from;
 
+	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
+		if (nor->addr_width == 3) {
+			bank = (u32)from / SZ_16M;
+			if (nor->flags & SNOR_F_HAS_PARALLEL)
+				bank /= 2;
+
+			rem_bank_len = SZ_16M * (bank + 1);
+			if (nor->flags & SNOR_F_HAS_PARALLEL)
+				rem_bank_len *= 2;
+			rem_bank_len -= from;
+		}
+
 		if (nor->flags & SNOR_F_HAS_STACKED) {
+			stack_shift = 1;
 			if (offset >= (mtd->size / 2)) {
 				offset = offset - (mtd->size / 2);
 				nor->spi->flags |= SPI_XFER_U_PAGE;
@@ -1613,20 +1620,31 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 			}
 		}
 
+		if (nor->addr_width == 4)
+			rem_bank_len = (mtd->size >> stack_shift) - offset;
+
 		if (nor->flags & SNOR_F_HAS_PARALLEL)
 			offset /= 2;
+	}
 
 #ifdef CONFIG_SPI_FLASH_BAR
+		u32 remain_len;
+
 		ret = write_bar(nor, offset);
 		if (ret < 0)
 			return log_ret(ret);
-#endif
-
-		if (len < rem_bank_len)
+		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
+		if (len < remain_len)
 			read_len = len;
 		else
-			read_len = rem_bank_len;
-
+			read_len = remain_len;
+#endif
+		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
+			if (len < rem_bank_len)
+				read_len = len;
+			else
+				read_len = rem_bank_len;
+		}
 		if (read_len == 0)
 			return -EIO;
 
-- 
2.34.1

Re: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Marek Vasut]

On 12/20/24 3:30 PM, Venkatesh Yadav Abbarapu wrote:

[...]

> @@ -1578,8 +1580,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>   	int ret;
>   	loff_t offset = from;
> -	u32 read_len = 0;
>   	u32 rem_bank_len = 0;
> +	u32 stack_shift = 0;
>   	u8 bank;
>   	bool is_ofst_odd = false;
>   
> @@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	}
>   
>   	while (len) {
> -		bank = (u32)from / SZ_16M;
> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> -			bank /= 2;
> -
> -		rem_bank_len = SZ_16M * (bank + 1);
> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> -			rem_bank_len *= 2;
> -		rem_bank_len -= from;
> -
> +		size_t read_len = len;

size_t read_len should be declared at the beginning of this function.

>   		offset = from;
>   
> +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {

The indent here needs to be pushed on tab RIGHT -> .

Do you need this functionality in SPL ? If so, you might need a matching 
Kconfig SPL_SPI_STACKED_PARALLEL symbol .

> +		if (nor->addr_width == 3) {
> +			bank = (u32)from / SZ_16M;
> +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> +				bank /= 2;
> +
> +			rem_bank_len = SZ_16M * (bank + 1);
> +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> +				rem_bank_len *= 2;
> +			rem_bank_len -= from;
> +		}
> +
>   		if (nor->flags & SNOR_F_HAS_STACKED) {
> +			stack_shift = 1;
>   			if (offset >= (mtd->size / 2)) {
>   				offset = offset - (mtd->size / 2);
>   				nor->spi->flags |= SPI_XFER_U_PAGE;
> @@ -1613,20 +1620,31 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   			}
>   		}
>   
> +		if (nor->addr_width == 4)
> +			rem_bank_len = (mtd->size >> stack_shift) - offset;
> +
>   		if (nor->flags & SNOR_F_HAS_PARALLEL)
>   			offset /= 2;
> +	}
>   
>   #ifdef CONFIG_SPI_FLASH_BAR
> +		u32 remain_len;
> +
>   		ret = write_bar(nor, offset);
>   		if (ret < 0)
>   			return log_ret(ret);
> -#endif
> -
> -		if (len < rem_bank_len)
> +		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;

Is this calculation correct?

It used to be calculated as:

bank = (u32)from / SZ_16M;
rem_bank_len = SZ_16M * (bank + 1);
rem_bank_len -= from;

Now the bank is calculated as:

bank = nor->bank_curr + 1;

?

Why not keep calculating bank from offset ?

Maybe this will work ?

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 691962ef271..bdfbd44f30c 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1578,10 +1578,11 @@ static int spi_nor_read(struct mtd_info *mtd, 
loff_t from, size_t len,
  			size_t *retlen, u_char *buf)
  {
  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
-	int ret;
  	loff_t offset = from;
  	u32 rem_bank_len = 0;
  	u32 stack_shift = 0;
+	size_t read_len;
+	int ret;
  	u8 bank;
  	bool is_ofst_odd = false;

@@ -1595,7 +1596,7 @@ static int spi_nor_read(struct mtd_info *mtd, 
loff_t from, size_t len,
  	}

  	while (len) {
-		size_t read_len = len;
+		read_len = len;
  		offset = from;

  		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
@@ -1627,24 +1628,24 @@ static int spi_nor_read(struct mtd_info *mtd, 
loff_t from, size_t len,
  				offset /= 2;
  		}

-#ifdef CONFIG_SPI_FLASH_BAR
-		u32 remain_len;
+		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
+			bank = (u32)from / SZ_16M;
+			rem_bank_len = SZ_16M * (bank + 1);
+			rem_bank_len -= from;

  			ret = write_bar(nor, offset);
  			if (ret < 0)
  				return log_ret(ret);
-		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
-		if (len < remain_len)
-			read_len = len;
-		else
-			read_len = remain_len;
-#endif
-		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
+		}
+
+		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) ||
+		    CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
  			if (len < rem_bank_len)
  				read_len = len;
  			else
  				read_len = rem_bank_len;
  		}
+
  		if (read_len == 0)
  			return -EIO;

I am still worried about using stacked parallel and BAR together, is 
that even possible or are they mutually exclusive ?

RE: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, December 23, 2024 10:22 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/20/24 3:30 PM, Venkatesh Yadav Abbarapu wrote:
> 
> [...]
> 
> > @@ -1578,8 +1580,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
> >   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >   	int ret;
> >   	loff_t offset = from;
> > -	u32 read_len = 0;
> >   	u32 rem_bank_len = 0;
> > +	u32 stack_shift = 0;
> >   	u8 bank;
> >   	bool is_ofst_odd = false;
> >
> > @@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> >   	}
> >
> >   	while (len) {
> > -		bank = (u32)from / SZ_16M;
> > -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> > -			bank /= 2;
> > -
> > -		rem_bank_len = SZ_16M * (bank + 1);
> > -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> > -			rem_bank_len *= 2;
> > -		rem_bank_len -= from;
> > -
> > +		size_t read_len = len;
> 
> size_t read_len should be declared at the beginning of this function.
> 
> >   		offset = from;
> >
> > +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> 
> The indent here needs to be pushed on tab RIGHT -> .
> 
> Do you need this functionality in SPL ? If so, you might need a matching Kconfig
> SPL_SPI_STACKED_PARALLEL symbol .

This functionality is not needed for SPL.

> 
> > +		if (nor->addr_width == 3) {
> > +			bank = (u32)from / SZ_16M;
> > +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> > +				bank /= 2;
> > +
> > +			rem_bank_len = SZ_16M * (bank + 1);
> > +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> > +				rem_bank_len *= 2;
> > +			rem_bank_len -= from;
> > +		}
> > +
> >   		if (nor->flags & SNOR_F_HAS_STACKED) {
> > +			stack_shift = 1;
> >   			if (offset >= (mtd->size / 2)) {
> >   				offset = offset - (mtd->size / 2);
> >   				nor->spi->flags |= SPI_XFER_U_PAGE; @@ -
> 1613,20 +1620,31 @@
> > static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> >   			}
> >   		}
> >
> > +		if (nor->addr_width == 4)
> > +			rem_bank_len = (mtd->size >> stack_shift) - offset;
> > +
> >   		if (nor->flags & SNOR_F_HAS_PARALLEL)
> >   			offset /= 2;
> > +	}
> >
> >   #ifdef CONFIG_SPI_FLASH_BAR
> > +		u32 remain_len;
> > +
> >   		ret = write_bar(nor, offset);
> >   		if (ret < 0)
> >   			return log_ret(ret);
> > -#endif
> > -
> > -		if (len < rem_bank_len)
> > +		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
> 
> Is this calculation correct?
> 
> It used to be calculated as:
> 
> bank = (u32)from / SZ_16M;
> rem_bank_len = SZ_16M * (bank + 1);
> rem_bank_len -= from;
> 
> Now the bank is calculated as:
> 
> bank = nor->bank_curr + 1;
> 
> ?
> 
> Why not keep calculating bank from offset ?
> 
> Maybe this will work ?
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index
> 691962ef271..bdfbd44f30c 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1578,10 +1578,11 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>   			size_t *retlen, u_char *buf)
>   {
>   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	int ret;
>   	loff_t offset = from;
>   	u32 rem_bank_len = 0;
>   	u32 stack_shift = 0;
> +	size_t read_len;
> +	int ret;
>   	u8 bank;
>   	bool is_ofst_odd = false;
> 
> @@ -1595,7 +1596,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>   	}
> 
>   	while (len) {
> -		size_t read_len = len;
> +		read_len = len;
>   		offset = from;
> 
>   		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { @@ -
> 1627,24 +1628,24 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>   				offset /= 2;
>   		}
> 
> -#ifdef CONFIG_SPI_FLASH_BAR
> -		u32 remain_len;
> +		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
> +			bank = (u32)from / SZ_16M;
> +			rem_bank_len = SZ_16M * (bank + 1);
> +			rem_bank_len -= from;
> 
>   			ret = write_bar(nor, offset);
>   			if (ret < 0)
>   				return log_ret(ret);
> -		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
> -		if (len < remain_len)
> -			read_len = len;
> -		else
> -			read_len = remain_len;
> -#endif
> -		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> +		}
> +
> +		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) ||
> +		    CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>   			if (len < rem_bank_len)
>   				read_len = len;
>   			else
>   				read_len = rem_bank_len;
>   		}
> +
>   		if (read_len == 0)
>   			return -EIO;
> 
> I am still worried about using stacked parallel and BAR together, is that even
> possible or are they mutually exclusive ?

Will check the above implementation and update the patch.
FLASH_BAR and stacked parallel configuration doesn’t depend on each other.

Thanks
Venkatesh

Re: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Marek Vasut]

On 12/24/24 4:34 PM, Abbarapu, Venkatesh wrote:

[...]

>>> @@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
>> from, size_t len,
>>>    	}
>>>
>>>    	while (len) {
>>> -		bank = (u32)from / SZ_16M;
>>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
>>> -			bank /= 2;
>>> -
>>> -		rem_bank_len = SZ_16M * (bank + 1);
>>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
>>> -			rem_bank_len *= 2;
>>> -		rem_bank_len -= from;
>>> -
>>> +		size_t read_len = len;
>>
>> size_t read_len should be declared at the beginning of this function.
>>
>>>    		offset = from;
>>>
>>> +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>>
>> The indent here needs to be pushed on tab RIGHT -> .
>>
>> Do you need this functionality in SPL ? If so, you might need a matching Kconfig
>> SPL_SPI_STACKED_PARALLEL symbol .
> 
> This functionality is not needed for SPL.

So SPL does use SPI NOR in non-stacked/parallel mode, and U-Boot does 
use it in stacked/parallel mode ? Doesn't that pose a problem ?

[...]

> FLASH_BAR and stacked parallel configuration doesn’t depend on each other.
Is the use of BAR register and stacked/parallel mode mutually exclusive 
or can they be used together ?

RE: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Abbarapu, Venkatesh]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Thursday, December 26, 2024 1:51 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/24/24 4:34 PM, Abbarapu, Venkatesh wrote:
> 
> [...]
> 
> >>> @@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info
> >>> *mtd, loff_t
> >> from, size_t len,
> >>>    	}
> >>>
> >>>    	while (len) {
> >>> -		bank = (u32)from / SZ_16M;
> >>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> >>> -			bank /= 2;
> >>> -
> >>> -		rem_bank_len = SZ_16M * (bank + 1);
> >>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> >>> -			rem_bank_len *= 2;
> >>> -		rem_bank_len -= from;
> >>> -
> >>> +		size_t read_len = len;
> >>
> >> size_t read_len should be declared at the beginning of this function.
> >>
> >>>    		offset = from;
> >>>
> >>> +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> >>
> >> The indent here needs to be pushed on tab RIGHT -> .
> >>
> >> Do you need this functionality in SPL ? If so, you might need a
> >> matching Kconfig SPL_SPI_STACKED_PARALLEL symbol .
> >
> > This functionality is not needed for SPL.
> 
> So SPL does use SPI NOR in non-stacked/parallel mode, and U-Boot does use it in
> stacked/parallel mode ? Doesn't that pose a problem ?
> 
> [...]
> 
> > FLASH_BAR and stacked parallel configuration doesn’t depend on each other.
> Is the use of BAR register and stacked/parallel mode mutually exclusive or can they
> be used together ?
The FLASH_BAR and stacked/parallel mode can be used together.

Thanks
Venkatesh