| Message ID | 20231125092137.2948-1-amit.kumar-mahapatra@amd.com |
|---|---|
| Headers | show |
| Series | spi: Add support for stacked/parallel memories | expand |
On Sat, 25 Nov 2023 14:51:28 +0530, Amit Kumar Mahapatra wrote: > In preparation for adding multiple CS support for a device, set/get > functions were introduces accessing spi->chip_select in > 'commit 303feb3cc06a ("spi: Add APIs in spi core to set/get > spi->chip_select and spi->cs_gpiod")'. > Replace spi->chip_select with spi_get_chipselect() API. > > > [...] Applied, thanks! [01/10] mfd: tps6594: Use set/get APIs to access spi->chip_select commit: dd636638446c87c95c5beddcd367d95ac6764c6c -- Lee Jones [李琼斯]
On Fri, Dec 01, 2023 at 09:57:36AM +0000, Lee Jones wrote: > On Sat, 25 Nov 2023 14:51:28 +0530, Amit Kumar Mahapatra wrote: > > In preparation for adding multiple CS support for a device, set/get > > functions were introduces accessing spi->chip_select in > > 'commit 303feb3cc06a ("spi: Add APIs in spi core to set/get > > spi->chip_select and spi->cs_gpiod")'. > > Replace spi->chip_select with spi_get_chipselect() API. > Applied, thanks! > [01/10] mfd: tps6594: Use set/get APIs to access spi->chip_select > commit: dd636638446c87c95c5beddcd367d95ac6764c6c Is there a signed tag available for this - without this change the subsequent SPI changes introduce a build breakage.
On Fri, 01 Dec 2023, Mark Brown wrote: > On Fri, Dec 01, 2023 at 09:57:36AM +0000, Lee Jones wrote: > > On Sat, 25 Nov 2023 14:51:28 +0530, Amit Kumar Mahapatra wrote: > > > In preparation for adding multiple CS support for a device, set/get > > > functions were introduces accessing spi->chip_select in > > > 'commit 303feb3cc06a ("spi: Add APIs in spi core to set/get > > > spi->chip_select and spi->cs_gpiod")'. > > > Replace spi->chip_select with spi_get_chipselect() API. > > > Applied, thanks! > > > [01/10] mfd: tps6594: Use set/get APIs to access spi->chip_select > > commit: dd636638446c87c95c5beddcd367d95ac6764c6c > > Is there a signed tag available for this - without this change the > subsequent SPI changes introduce a build breakage. Not yet, but I can get around to making one.
Hi, Amit, On 11/25/23 09:21, Amit Kumar Mahapatra wrote: > Each flash that is connected in stacked mode should have a separate > parameter structure. So, the flash parameter member(*params) of the spi_nor > structure is changed to an array (*params[2]). The array is used to store > the parameters of each flash connected in stacked configuration. > > The current implementation assumes that a maximum of two flashes are > connected in stacked mode and both the flashes are of same make but can > differ in sizes. So, except the sizes all other flash parameters of both > the flashes are identical. Do you plan to add support for different flashes in stacked mode? If not, wouldn't it be simpler to have just an array of flash sizes instead of duplicating the entire params struct? > > SPI-NOR is not aware of the chip_select values, for any incoming request > SPI-NOR will decide the flash index with the help of individual flash size > and the configuration type (single/stacked). SPI-NOR will pass on the flash > index information to the SPI core & SPI driver by setting the appropriate > bit in nor->spimem->spi->cs_index_mask. For example, if nth bit of > nor->spimem->spi->cs_index_mask is set then the driver would > assert/de-assert spi->chip_slect[n]. > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > --- > drivers/mtd/spi-nor/core.c | 272 +++++++++++++++++++++++++++++------- > drivers/mtd/spi-nor/core.h | 4 + > include/linux/mtd/spi-nor.h | 15 +- > 3 files changed, 240 insertions(+), 51 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 93ae69b7ff83..e990be7c7eb6 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c cut > @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor) > static int spi_nor_late_init_params(struct spi_nor *nor) > { > struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0); > - int ret; > + struct device_node *np = spi_nor_get_flash_node(nor); > + u64 flash_size[SNOR_FLASH_CNT_MAX]; > + u32 idx = 0; > + int rc, ret; > > if (nor->manufacturer && nor->manufacturer->fixups && > nor->manufacturer->fixups->late_init) { > @@ -2937,6 +3042,44 @@ static int spi_nor_late_init_params(struct spi_nor *nor) > if (params->n_banks > 1) > params->bank_size = div64_u64(params->size, params->n_banks); > > + nor->num_flash = 0; > + > + /* > + * The flashes that are connected in stacked mode should be of same make. > + * Except the flash size all other properties are identical for all the > + * flashes connected in stacked mode. > + * The flashes that are connected in parallel mode should be identical. > + */ > + while (idx < SNOR_FLASH_CNT_MAX) { > + rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[idx]); This is a little late in my opinion, as we don't have any sanity check on the flashes that are stacked on top of the first. We shall at least read and compare the ID for all. Cheers, ta
On 12/6/23 14:30, Tudor Ambarus wrote: > Hi, Amit, > > On 11/25/23 09:21, Amit Kumar Mahapatra wrote: >> Each flash that is connected in stacked mode should have a separate >> parameter structure. So, the flash parameter member(*params) of the spi_nor >> structure is changed to an array (*params[2]). The array is used to store >> the parameters of each flash connected in stacked configuration. >> >> The current implementation assumes that a maximum of two flashes are >> connected in stacked mode and both the flashes are of same make but can >> differ in sizes. So, except the sizes all other flash parameters of both >> the flashes are identical. > > Do you plan to add support for different flashes in stacked mode? If > not, wouldn't it be simpler to have just an array of flash sizes instead > of duplicating the entire params struct? > >> >> SPI-NOR is not aware of the chip_select values, for any incoming request >> SPI-NOR will decide the flash index with the help of individual flash size >> and the configuration type (single/stacked). SPI-NOR will pass on the flash >> index information to the SPI core & SPI driver by setting the appropriate >> bit in nor->spimem->spi->cs_index_mask. For example, if nth bit of >> nor->spimem->spi->cs_index_mask is set then the driver would >> assert/de-assert spi->chip_slect[n]. >> >> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> >> --- >> drivers/mtd/spi-nor/core.c | 272 +++++++++++++++++++++++++++++------- >> drivers/mtd/spi-nor/core.h | 4 + >> include/linux/mtd/spi-nor.h | 15 +- >> 3 files changed, 240 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 93ae69b7ff83..e990be7c7eb6 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c > > cut > >> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor) >> static int spi_nor_late_init_params(struct spi_nor *nor) >> { >> struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0); >> - int ret; >> + struct device_node *np = spi_nor_get_flash_node(nor); >> + u64 flash_size[SNOR_FLASH_CNT_MAX]; >> + u32 idx = 0; >> + int rc, ret; >> >> if (nor->manufacturer && nor->manufacturer->fixups && >> nor->manufacturer->fixups->late_init) { >> @@ -2937,6 +3042,44 @@ static int spi_nor_late_init_params(struct spi_nor *nor) >> if (params->n_banks > 1) >> params->bank_size = div64_u64(params->size, params->n_banks); >> >> + nor->num_flash = 0; >> + >> + /* >> + * The flashes that are connected in stacked mode should be of same make. >> + * Except the flash size all other properties are identical for all the >> + * flashes connected in stacked mode. >> + * The flashes that are connected in parallel mode should be identical. >> + */ >> + while (idx < SNOR_FLASH_CNT_MAX) { >> + rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[idx]); also, it's not clear to me why you read this property multiple times. Have you sent a device tree patch somewhere? It will help me understand what you're trying to achieve. > > This is a little late in my opinion, as we don't have any sanity check > on the flashes that are stacked on top of the first. We shall at least > read and compare the ID for all. > > Cheers, > ta
On 11/25/23 09:21, Amit Kumar Mahapatra wrote: > The current implementation assumes that a maximum of two flashes are > connected in stacked mode and both the flashes are of same make but can > differ in sizes. So, except the sizes all other flash parameters of both > the flashes are identical. Too much restrictions, isn't it? Have you thought about adding a layer on top of SPI NOR managing the stacked/parallel flashes?
On Sat, 25 Nov 2023 14:51:27 +0530, Amit Kumar Mahapatra wrote: > This patch series updated the spi-nor, spi core and the AMD-Xilinx GQSPI > driver to add stacked and parallel memories support. > > The first nine patches > https://lore.kernel.org/all/20230119185342.2093323-1-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-2-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-3-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-4-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-5-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-6-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-7-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-8-amit.kumar-mahapatra@amd.com/ > https://lore.kernel.org/all/20230310173217.3429788-9-amit.kumar-mahapatra@amd.com/ > of the previous series got applied to > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git > for-next But the rest of the patches in the series did not get applied as > failure was reported for spi driver with GPIO CS, so send the remaining > patches in the series after rebasing it on top of for-next branch and > fixing the issue. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [02/10] ALSA: hda/cs35l56: Use set/get APIs to access spi->chip_select commit: f05e2f61fe88092e0d341ea27644a84e3386358d [03/10] spi: Add multi-cs memories support in SPI core commit: 4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hello Tudor, > -----Original Message----- > From: Tudor Ambarus <tudor.ambarus@linaro.org> > Sent: Wednesday, December 6, 2023 8:00 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; > richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; > lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; > rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; > patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- > Xilinx) <git@amd.com>; amitrkcian2002@gmail.com > Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support > in spi-nor > > Hi, Amit, > > On 11/25/23 09:21, Amit Kumar Mahapatra wrote: > > Each flash that is connected in stacked mode should have a separate > > parameter structure. So, the flash parameter member(*params) of the > > spi_nor structure is changed to an array (*params[2]). The array is > > used to store the parameters of each flash connected in stacked > configuration. > > > > The current implementation assumes that a maximum of two flashes are > > connected in stacked mode and both the flashes are of same make but > > can differ in sizes. So, except the sizes all other flash parameters > > of both the flashes are identical. > > Do you plan to add support for different flashes in stacked mode? If not, No, according to the current implementation, in stacked mode, both flashes must be of the same make. > wouldn't it be simpler to have just an array of flash sizes instead of > duplicating the entire params struct? Yes, that is accurate. In alignment with our current stacked support use case we can have an array of flash sizes instead. The primary purpose of having an array of params struct was to facilitate potential future extensions, allowing the addition of stacked support for different flashes > > > > > SPI-NOR is not aware of the chip_select values, for any incoming > > request SPI-NOR will decide the flash index with the help of > > individual flash size and the configuration type (single/stacked). > > SPI-NOR will pass on the flash index information to the SPI core & SPI > > driver by setting the appropriate bit in > > nor->spimem->spi->cs_index_mask. For example, if nth bit of > > nor->spimem->spi->cs_index_mask is set then the driver would > > assert/de-assert spi->chip_slect[n]. > > > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > > --- > > drivers/mtd/spi-nor/core.c | 272 +++++++++++++++++++++++++++++------- > > drivers/mtd/spi-nor/core.h | 4 + > > include/linux/mtd/spi-nor.h | 15 +- > > 3 files changed, 240 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 93ae69b7ff83..e990be7c7eb6 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > cut > > > @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct > > spi_nor *nor) static int spi_nor_late_init_params(struct spi_nor > > *nor) { > > struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, > 0); > > - int ret; > > + struct device_node *np = spi_nor_get_flash_node(nor); > > + u64 flash_size[SNOR_FLASH_CNT_MAX]; > > + u32 idx = 0; > > + int rc, ret; > > > > if (nor->manufacturer && nor->manufacturer->fixups && > > nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@ > > static int spi_nor_late_init_params(struct spi_nor *nor) > > if (params->n_banks > 1) > > params->bank_size = div64_u64(params->size, params- > >n_banks); > > > > + nor->num_flash = 0; > > + > > + /* > > + * The flashes that are connected in stacked mode should be of same > make. > > + * Except the flash size all other properties are identical for all the > > + * flashes connected in stacked mode. > > + * The flashes that are connected in parallel mode should be identical. > > + */ > > + while (idx < SNOR_FLASH_CNT_MAX) { > > + rc = of_property_read_u64_index(np, "stacked-memories", > idx, > > +&flash_size[idx]); > > This is a little late in my opinion, as we don't have any sanity check on the > flashes that are stacked on top of the first. We shall at least read and compare > the ID for all. Alright, I will incorporate a sanity check for reading and comparing the ID of the stacked flash. Subsequently, I believe this stacked logic should be relocated to spi_nor_get_flash_info() where we identify the first flash. Please share your thoughts on this. Additionally, do you anticipate that SPI-NOR should throw an error if the second or any subsequent flash within the stacked connection is different? Or would you prefer it to print a warning and operate in single mode (i.e., only the first flash)? Regards, Amit > > Cheers, > ta
On 12/8/23 17:05, Mahapatra, Amit Kumar wrote: > Hello Tudor, Hi! > >> -----Original Message----- >> From: Tudor Ambarus <tudor.ambarus@linaro.org> >> Sent: Wednesday, December 6, 2023 8:00 PM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> michael@walle.cc; linux-mtd@lists.infradead.org; >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- >> arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; >> patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- >> Xilinx) <git@amd.com>; amitrkcian2002@gmail.com >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support >> in spi-nor >> >> Hi, Amit, >> >> On 11/25/23 09:21, Amit Kumar Mahapatra wrote: >>> Each flash that is connected in stacked mode should have a separate >>> parameter structure. So, the flash parameter member(*params) of the >>> spi_nor structure is changed to an array (*params[2]). The array is >>> used to store the parameters of each flash connected in stacked >> configuration. >>> >>> The current implementation assumes that a maximum of two flashes are >>> connected in stacked mode and both the flashes are of same make but >>> can differ in sizes. So, except the sizes all other flash parameters >>> of both the flashes are identical. >> >> Do you plan to add support for different flashes in stacked mode? If not, > > No, according to the current implementation, in stacked mode, both flashes > must be of the same make. > >> wouldn't it be simpler to have just an array of flash sizes instead of >> duplicating the entire params struct? > > Yes, that is accurate. In alignment with our current stacked support use case we > can have an array of flash sizes instead. > The primary purpose of having an array of params struct was to facilitate > potential future extensions, allowing the addition of stacked support for > different flashes > right. Don't do this change yet, let's decide on the overall architecture first. >> >>> >>> SPI-NOR is not aware of the chip_select values, for any incoming >>> request SPI-NOR will decide the flash index with the help of >>> individual flash size and the configuration type (single/stacked). >>> SPI-NOR will pass on the flash index information to the SPI core & SPI >>> driver by setting the appropriate bit in >>> nor->spimem->spi->cs_index_mask. For example, if nth bit of >>> nor->spimem->spi->cs_index_mask is set then the driver would >>> assert/de-assert spi->chip_slect[n]. >>> >>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> >>> --- >>> drivers/mtd/spi-nor/core.c | 272 +++++++++++++++++++++++++++++------- >>> drivers/mtd/spi-nor/core.h | 4 + >>> include/linux/mtd/spi-nor.h | 15 +- >>> 3 files changed, 240 insertions(+), 51 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index 93ae69b7ff83..e990be7c7eb6 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >> >> cut >> >>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct >>> spi_nor *nor) static int spi_nor_late_init_params(struct spi_nor >>> *nor) { >>> struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, >> 0); >>> - int ret; >>> + struct device_node *np = spi_nor_get_flash_node(nor); >>> + u64 flash_size[SNOR_FLASH_CNT_MAX]; >>> + u32 idx = 0; >>> + int rc, ret; >>> >>> if (nor->manufacturer && nor->manufacturer->fixups && >>> nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@ >>> static int spi_nor_late_init_params(struct spi_nor *nor) >>> if (params->n_banks > 1) >>> params->bank_size = div64_u64(params->size, params- >>> n_banks); >>> >>> + nor->num_flash = 0; >>> + >>> + /* >>> + * The flashes that are connected in stacked mode should be of same >> make. >>> + * Except the flash size all other properties are identical for all the >>> + * flashes connected in stacked mode. >>> + * The flashes that are connected in parallel mode should be identical. >>> + */ >>> + while (idx < SNOR_FLASH_CNT_MAX) { >>> + rc = of_property_read_u64_index(np, "stacked-memories", >> idx, >>> +&flash_size[idx]); >> >> This is a little late in my opinion, as we don't have any sanity check on the >> flashes that are stacked on top of the first. We shall at least read and compare >> the ID for all. > > Alright, I will incorporate a sanity check for reading and comparing the > ID of the stacked flash. Subsequently, I believe this stacked logic > should be relocated to spi_nor_get_flash_info() where we identify the > first flash. Please share your thoughts on this. Additionally, do you I'm wondering whether we can add a layer on top of the flash type to handle the stacked/parallel modes. This way everything will become flash type independent. Would it be possible to stack 2 SPI NANDs? How about a SPI NOR and a SPI NAND? Is the datasheet of the controller public? > anticipate that SPI-NOR should throw an error if the second or any > subsequent flash within the stacked connection is different? Or would you > prefer it to print a warning and operate in single mode (i.e., only the > first flash)? Both options are fine, but I haven't yet decided on the overall architecture. Cheers, ta
On 12/11/23 06:56, Mahapatra, Amit Kumar wrote: > > >> -----Original Message----- >> From: Tudor Ambarus <tudor.ambarus@linaro.org> >> Sent: Monday, December 11, 2023 9:03 AM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> michael@walle.cc; linux-mtd@lists.infradead.org; >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- >> arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; >> patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- >> Xilinx) <git@amd.com>; amitrkcian2002@gmail.com >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support >> in spi-nor >> >> >> >> On 12/8/23 17:05, Mahapatra, Amit Kumar wrote: >>> Hello Tudor, >> >> Hi! > > Hello Tudor, > Hi! >> >>> >>>> -----Original Message----- >>>> From: Tudor Ambarus <tudor.ambarus@linaro.org> >>>> Sent: Wednesday, December 6, 2023 8:00 PM >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >>>> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >>>> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >>>> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >>>> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> michael@walle.cc; linux-mtd@lists.infradead.org; >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; >>>> linux- arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; >>>> patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- >>>> Xilinx) <git@amd.com>; amitrkcian2002@gmail.com >>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories >>>> support in spi-nor >>>> >>>> Hi, Amit, >>>> >>>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote: >>>>> Each flash that is connected in stacked mode should have a separate >>>>> parameter structure. So, the flash parameter member(*params) of the >>>>> spi_nor structure is changed to an array (*params[2]). The array is >>>>> used to store the parameters of each flash connected in stacked >>>> configuration. >>>>> >>>>> The current implementation assumes that a maximum of two flashes are >>>>> connected in stacked mode and both the flashes are of same make but >>>>> can differ in sizes. So, except the sizes all other flash parameters >>>>> of both the flashes are identical. >>>> >>>> Do you plan to add support for different flashes in stacked mode? If >>>> not, >>> >>> No, according to the current implementation, in stacked mode, both >>> flashes must be of the same make. >>> >>>> wouldn't it be simpler to have just an array of flash sizes instead >>>> of duplicating the entire params struct? >>> >>> Yes, that is accurate. In alignment with our current stacked support >>> use case we can have an array of flash sizes instead. >>> The primary purpose of having an array of params struct was to >>> facilitate potential future extensions, allowing the addition of >>> stacked support for different flashes >>> >> >> right. Don't do this change yet, let's decide on the overall architecture first. > > Sure. >> >>>> >>>>> >>>>> SPI-NOR is not aware of the chip_select values, for any incoming >>>>> request SPI-NOR will decide the flash index with the help of >>>>> individual flash size and the configuration type (single/stacked). >>>>> SPI-NOR will pass on the flash index information to the SPI core & >>>>> SPI driver by setting the appropriate bit in >>>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of >>>>> nor->spimem->spi->cs_index_mask is set then the driver would >>>>> assert/de-assert spi->chip_slect[n]. >>>>> >>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar- >> mahapatra@amd.com> >>>>> --- >>>>> drivers/mtd/spi-nor/core.c | 272 +++++++++++++++++++++++++++++----- >> -- >>>>> drivers/mtd/spi-nor/core.h | 4 + >>>>> include/linux/mtd/spi-nor.h | 15 +- >>>>> 3 files changed, 240 insertions(+), 51 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>>> index 93ae69b7ff83..e990be7c7eb6 100644 >>>>> --- a/drivers/mtd/spi-nor/core.c >>>>> +++ b/drivers/mtd/spi-nor/core.c >>>> >>>> cut >>>> >>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct >>>>> spi_nor *nor) static int spi_nor_late_init_params(struct spi_nor >>>>> *nor) { >>>>> struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, >>>> 0); >>>>> - int ret; >>>>> + struct device_node *np = spi_nor_get_flash_node(nor); >>>>> + u64 flash_size[SNOR_FLASH_CNT_MAX]; >>>>> + u32 idx = 0; >>>>> + int rc, ret; >>>>> >>>>> if (nor->manufacturer && nor->manufacturer->fixups && >>>>> nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@ >>>>> static int spi_nor_late_init_params(struct spi_nor *nor) >>>>> if (params->n_banks > 1) >>>>> params->bank_size = div64_u64(params->size, params- >> n_banks); >>>>> >>>>> + nor->num_flash = 0; >>>>> + >>>>> + /* >>>>> + * The flashes that are connected in stacked mode should be of >>>>> +same >>>> make. >>>>> + * Except the flash size all other properties are identical for all the >>>>> + * flashes connected in stacked mode. >>>>> + * The flashes that are connected in parallel mode should be identical. >>>>> + */ >>>>> + while (idx < SNOR_FLASH_CNT_MAX) { >>>>> + rc = of_property_read_u64_index(np, "stacked-memories", >>>> idx, >>>>> +&flash_size[idx]); >>>> >>>> This is a little late in my opinion, as we don't have any sanity >>>> check on the flashes that are stacked on top of the first. We shall >>>> at least read and compare the ID for all. >>> >>> Alright, I will incorporate a sanity check for reading and comparing >>> the ID of the stacked flash. Subsequently, I believe this stacked >>> logic should be relocated to spi_nor_get_flash_info() where we >>> identify the first flash. Please share your thoughts on this. >>> Additionally, do you >> >> I'm wondering whether we can add a layer on top of the flash type to handle > > When you mention "on top," are you referring to incorporating it into > the MTD layer? Initially, Miquel had submitted this patch to address I mean something above SPI MEM flashes, be it NOR, NANDs or whatever. Instead of treating the stacked flashes as a monolithic device and treat in SPI NOR some array of flashes, to have a layer above which probes the SPI MEM flash driver for each stacked flash. In your case SPI NOR would be probed twice, as you use 2 SPI NOR flashes. > stacked/parallel handling in the MTD layer. > https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/ > However, the Device Tree bindings were initially not accepted. Okay, thanks for the pointer. I'll take a look. > Following a series of discussions, the below bindings were > eventually merged. > https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@bootlin.com/ I saw, thanks. > >> the stacked/parallel modes. This way everything will become flash type >> independent. Would it be possible to stack 2 SPI NANDs? How about a SPI >> NOR and a SPI NAND? >> >> Is the datasheet of the controller public? > > Yes, https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Controller > Wonderful, I'll take a look. I see two clocks there. Does this mean that the stacked flashes can be operated at different frequencies? Do you know if we can combine a SPI NOR with a SPI NAND in stacked configuration? I need to study this a bit. I'll try to involve Michael and Pratyush too. Cheers, ta
> This patch series updated the spi-nor, spi core and the AMD-Xilinx > GQSPI > driver to add stacked and parallel memories support. Honestly, I'm not thrilled about how this is implemented in the core and what the restrictions are. First, the pattern "if (n==1) then { old behavior } else { copy old code modify for n==2 }" is hard to maintain. There should be no copy and the old code shall be adapted to work for both n=1 and n>1. But I agree with Tudor, some kind of abstraction (layer) would be nice. Also, you hardcode n=2 everywhere. Please generalize that one. Are you aware of any other controller supporting such a feature? I've seen you also need to modify the spi controller and intercept some commands. Can everything be moved there? I'm not sure we are implementing controller specific things in the core. Hard to judge without seeing other controllers doing a similar thing. I'd like to avoid that. If we had some kind of abstraction here, that might be easier to adapt in the future, but just putting everything into the core will make it really hard to maintain. So if everything related to stacked and parallel memory would be in drivers/mtd/spi-nor/stacked.c, we'd have at least everything in one place with a proper interface between that and the core. -michael
On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote: >> Thanks! Can you share with us what flashes you used for testing in the >> stacked and parallel configurations? > I used SPI-NOR QSPI flashes for testing stacked and parallel. I got that, I wanted the flash name or device ID. What I'm interested is if each flash is in its own package. Are they? > >>>> can combine a SPI NOR with a SPI NAND in stacked configuration? >>> No, Xilinx/AMD QSPI controllers doesn't support this configuration. >>> >> 2 SPI NANDs shall work with the AMD controller, right? > We never tested 2 SPI-NAND with AMD controller. I was asking because I think the stacked layer shall be SPI MEM generic, and not particular to SPI NOR. >> I skimmed over the QSPI controller datasheet and wondered why one would >> get complicated with 2 Quad Flashes when we have Octal. But then I saw that >> the same SoC can configure an Octal controller (the Octal and Quad >> controllers seems mutual exclusive) and that the Octal one can operate 2 >> octal flashes in stacked mode 🙂. > Thats correct . > > Please let me know how you want me to proceed on this. I got you. Still need to allocate more time on this. Cheers, ta
On 12/15/23 10:02, Mahapatra, Amit Kumar wrote: > Hello Tudor, Hi, > >> -----Original Message----- >> From: Tudor Ambarus <tudor.ambarus@linaro.org> >> Sent: Friday, December 15, 2023 1:40 PM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> michael@walle.cc; linux-mtd@lists.infradead.org; >> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- >> arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; >> patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- >> Xilinx) <git@amd.com>; amitrkcian2002@gmail.com >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support >> in spi-nor >> >> >> >> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote: >>>> Thanks! Can you share with us what flashes you used for testing in >>>> the stacked and parallel configurations? >>> I used SPI-NOR QSPI flashes for testing stacked and parallel. >> >> I got that, I wanted the flash name or device ID. > > N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI flashes on > which we tested. Additionally, we conducted tests on over 30 different > QSPI flashes from four distinct vendors (Miron, Winbond, Macronix, and ISSI). > Great. >> What I'm interested is if each flash is in its own package. Are they? > > I'm sorry, but I don't quite understand what you mean by "if each flash in > its own package." > There are flashes that are stacked at the physical level. It's a single flash with multiple dies, that are all under a single physical package. As I understand, your stacked flash model is at logical level. You have 2 flashes each in its own package. 2 different entities. Is my understanding correct? Cheers, ta
----- Ursprüngliche Mail ----- > Von: "Amit Kumar Mahapatra" <amit.kumar-mahapatra@amd.com> > This patch series updated the spi-nor, spi core and the AMD-Xilinx GQSPI > driver to add stacked and parallel memories support. I wish the series had a real cover letter which explains the big picture in more detail. What I didn't really get so far, is it really necessary to support multiple chip selects within a single mtd? You changes introduce hard to maintain changes into the spi-nor/mtd core code which alert me. Why can't we have one mtd for each cs and, if needed, combine them later? We have drivers such as mtdconcat for reasons. Thanks, //richard
Hi Richard, richard@nod.at wrote on Mon, 18 Dec 2023 23:10:20 +0100 (CET): > ----- Ursprüngliche Mail ----- > > Von: "Amit Kumar Mahapatra" <amit.kumar-mahapatra@amd.com> > > > This patch series updated the spi-nor, spi core and the AMD-Xilinx GQSPI > > driver to add stacked and parallel memories support. > > I wish the series had a real cover letter which explains the big picture > in more detail. > > What I didn't really get so far, is it really necessary to support multiple > chip selects within a single mtd? > You changes introduce hard to maintain changes into the spi-nor/mtd core code > which alert me. > Why can't we have one mtd for each cs and, if needed, combine them later? > We have drivers such as mtdconcat for reasons. The Xilinx SPI controller is a bit convoluted, there are two ways to address the bits in a memory: * Either your extend the memory range with the second chip "on top" of the first (which would typically be a mtd-concat use case) * Or you use the two chips in parallel and you store the even bits in one device (let's say cs0) and the odd bits in the other (cs1). Extending mtd-concat for this might be another solution, I don't know how feasible it would be. Maybe these bindings will help understanding the logic: e2edd1b64f1c ("spi: dt-bindings: Describe stacked/parallel memories modes") eba5368503b4 ("spi: dt-bindings: Add an example with two stacked flashes") However I agree the changes will likely be hard to maintain given the complexity brought with such a different controller. Thanks, Miquèl
On 15.12.2023 13:20, Mahapatra, Amit Kumar wrote: > Hello Tudor, > Hi! >> -----Original Message----- >> From: Tudor Ambarus <tudor.ambarus@linaro.org> >> Sent: Friday, December 15, 2023 4:03 PM >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; michael@walle.cc; >> linux-mtd@lists.infradead.org; nicolas.ferre@microchip.com; >> alexandre.belloni@bootlin.com; claudiu.beznea@tuxon.dev; Simek, Michal >> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org; alsa- >> devel@alsa-project.org; patches@opensource.cirrus.com; linux- >> sound@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; >> amitrkcian2002@gmail.com >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support >> in spi-nor >> >> >> >> On 12/15/23 10:02, Mahapatra, Amit Kumar wrote: >>> Hello Tudor, >> >> Hi, >> >>> >>>> -----Original Message----- >>>> From: Tudor Ambarus <tudor.ambarus@linaro.org> >>>> Sent: Friday, December 15, 2023 1:40 PM >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; >>>> broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; >>>> richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; >>>> lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; >>>> rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com >>>> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> michael@walle.cc; linux-mtd@lists.infradead.org; >>>> nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; >>>> claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; >>>> linux- arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; >>>> patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- >>>> Xilinx) <git@amd.com>; amitrkcian2002@gmail.com >>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories >>>> support in spi-nor >>>> >>>> >>>> >>>> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote: >>>>>> Thanks! Can you share with us what flashes you used for testing in >>>>>> the stacked and parallel configurations? >>>>> I used SPI-NOR QSPI flashes for testing stacked and parallel. >>>> >>>> I got that, I wanted the flash name or device ID. >>> >>> N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI >> flashes >>> on which we tested. Additionally, we conducted tests on over 30 >>> different QSPI flashes from four distinct vendors (Miron, Winbond, >> Macronix, and ISSI). >>> >> >> Great. >> >>>> What I'm interested is if each flash is in its own package. Are they? >>> >>> I'm sorry, but I don't quite understand what you mean by "if each >>> flash in its own package." >>> >> >> There are flashes that are stacked at the physical level. It's a single flash with >> multiple dies, that are all under a single physical package. > > Got it. The W25H02JV QSPI flash I mentioned earlier is a device with > with four dies that are stacked at the physical level. > >> >> As I understand, your stacked flash model is at logical level. You have >> 2 flashes each in its own package. 2 different entities. Is my understanding >> correct? > > Yes, that’s correct. > > I'd like to contribute to your earlier point regarding the placement of > the stacked layer. As you correctly highlighted, it should be in the > spi-mem generic layer. For instance, when a read/write operation extends > across multiple flashes (whether SPI-NOR or SPI-NAND), the stacked layer > must handle the flash crossover. This requires setting the appropriate CS > index in mem->spi->cs_index_mask to select the correct slave device and > updating the data buffer, address & data length in spi_mem_op struct > variable. Does this align with your understanding? > This was the initial idea, yes, but we'll have to see how mtd concat fits in. Maybe the abstraction can be made at the mtd level, which I suspect mtd concat does. I have to read that driver, never opened it. Something else to consider: I see that Micron has a twin quad mode: https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's dual parallel 8-bit. Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers both as DQ[3:0]. I had a short chat with Michael and he highlighted that instead of the parallel mode, one would be better of with an octal device. I wonder whether the quad parallel is worth the effort. I see AMD can select either quad (single/stacked/parallel) or octal (single/stacked). Is the parallel mode considered obsolete for new IPs? Cheers, ta
On 12/21/23 06:54, Mahapatra, Amit Kumar wrote: >> Something else to consider: I see that Micron has a twin quad mode: >> https://media-www.micron.com/- >> /media/client/global/documents/products/data-sheet/nor-flash/serial- >> nor/mt25t/generation- >> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 >> >> The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's >> dual parallel 8-bit. > Yes, I agree. > >> Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's >> stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers >> both as DQ[3:0]. > Yes, correct. Amit, please help me to assess this. I assume Micron and Microchip is using the same concepts as AMD uses for the "Dual Parallel 8-bit IO mode", but they call it "Twin Quad Mode". I was wrong, the AMD datasheet [1] was misleading [2], it described the IOs for both flashes as IO[3:0], but later on in the "Table QSPI Interface Signals" the second flash is described with IO[7:4]. The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and CLK# lines for each flash. As Micron does, isn't it? Micron says [3] that: "The device contains two quad I/O die, each able to operate independently for a total of eight I/Os. The memory map applies to each die. Each die has internal registers for status, configuration, and device protection that can be set and read independently from one other. Micron recommends that internal configuration settings for the two die be set identically." it also says that: "When using quad commands in XIO-SPI or when using QIO-SPI, DQ[3:0]/DQ[7:4] are I/O." So I guess the upper layers just ask for a chunk of memory to be written and the controller handles the cs# lines automatically. How is the AMD controller working, do you have to drive the cs# lines manually, or you just set the parallel mode and the controller takes care of everything? I assume this is how mchp is handling things, they seem to just set a bit the protocol into the QSPI_IFR.PROTTYP register field and that's all [4]. They even seem to write the registers of both flashes at the same time. In what regards the AMD's "dual stack interface", AMD is sharing the clock and IO lines and uses dedicated CS# lines for the flashes, whereas Micron shares the CS# and CLK# lines with different IO lines. Amit, please study the architectures used by mchp, micron and amd and let us know if they are the same or they differ, and if they differ what are the differences. I added Conor from mchp in cc, I see Nicolas is already there, and Bean from micron. Thanks, ta [1] https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Signals [2] https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv4ry78xM0_EF5kv4mA [3] https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 [4] https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf
On 2/9/24 11:06, Tudor Ambarus wrote: > > > On 12/21/23 06:54, Mahapatra, Amit Kumar wrote: >>> Something else to consider: I see that Micron has a twin quad mode: >>> https://media-www.micron.com/- >>> /media/client/global/documents/products/data-sheet/nor-flash/serial- >>> nor/mt25t/generation- >>> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 >>> >>> The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's >>> dual parallel 8-bit. >> Yes, I agree. >> >>> Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's >>> stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers >>> both as DQ[3:0]. >> Yes, correct. > > Amit, please help me to assess this. I assume Micron and Microchip is > using the same concepts as AMD uses for the "Dual Parallel 8-bit IO > mode", but they call it "Twin Quad Mode". > > I was wrong, the AMD datasheet [1] was misleading [2], it described the > IOs for both flashes as IO[3:0], but later on in the "Table QSPI > Interface Signals" the second flash is described with IO[7:4]. > > The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and > CLK# lines for each flash. As Micron does, isn't it? > > Micron says [3] that: > "The device contains two quad I/O die, each able to operate > independently for a total of eight I/Os. The memory map applies to each > die. Each die has internal registers for status, configuration, and > device protection that can be set and read independently from one other. > Micron recommends that internal configuration settings for the two die > be set identically." Amit, I forgot to say my first conclusion about the quote from above. Even if the dies are in the same physical package, micron asks users to configure each die as it is a self-standing entity, IOW to configure each die as it is a flash on its own. To me it looks like 2 concatenated flashes at first look. Thus identical to how AMD controller works. Please clarify this. > > it also says that: > "When using quad commands in XIO-SPI or when using QIO-SPI, > DQ[3:0]/DQ[7:4] are I/O." and this would be a parallel concatenation of two flashes. Then it would be good if you let us now the similarities and differences between how amd and mchp controller work, I scrawled few ideas below. thanks, ta > > So I guess the upper layers just ask for a chunk of memory to be written > and the controller handles the cs# lines automatically. How is the AMD > controller working, do you have to drive the cs# lines manually, or you > just set the parallel mode and the controller takes care of everything? > > I assume this is how mchp is handling things, they seem to just set a > bit the protocol into the QSPI_IFR.PROTTYP register field and that's all > [4]. They even seem to write the registers of both flashes at the same time. > > In what regards the AMD's "dual stack interface", AMD is sharing the > clock and IO lines and uses dedicated CS# lines for the flashes, whereas > Micron shares the CS# and CLK# lines with different IO lines. > > Amit, please study the architectures used by mchp, micron and amd and > let us know if they are the same or they differ, and if they differ what > are the differences. > > I added Conor from mchp in cc, I see Nicolas is already there, and Bean > from micron. > > Thanks, > ta > > [1] > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Signals > [2] > https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv4ry78xM0_EF5kv4mA > [3] > https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 > [4] > https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf
Hello Tudor, > -----Original Message----- > From: Tudor Ambarus <tudor.ambarus@linaro.org> > Sent: Friday, February 9, 2024 4:36 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; > richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; > lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; > rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; > patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- > Xilinx) <git@amd.com>; amitrkcian2002@gmail.com; Conor Dooley > <conor.dooley@microchip.com>; beanhuo@micron.com > Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support > in spi-nor > > CAUTION: This message has originated from an External Source. Please use > proper judgment and caution when opening attachments, clicking links, or > responding to this email. > > > On 12/21/23 06:54, Mahapatra, Amit Kumar wrote: > >> Something else to consider: I see that Micron has a twin quad mode: > >> https://media-www.micron.com/- > >> /media/client/global/documents/products/data-sheet/nor-flash/serial- > >> nor/mt25t/generation- > >> > b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 > >> > >> The micron's "Separate Chip-Select and Clock Signals" resembles the > >> AMD's dual parallel 8-bit. > > Yes, I agree. > > > >> Micron's "Shared Chip-Select and Clock Signals" differs from the > >> AMD's stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD > >> considers both as DQ[3:0]. > > Yes, correct. > > Amit, please help me to assess this. I assume Micron and Microchip is using > the same concepts as AMD uses for the "Dual Parallel 8-bit IO mode", but > they call it "Twin Quad Mode". That's accurate. It's the same concept. > > I was wrong, the AMD datasheet [1] was misleading [2], it described the IOs > for both flashes as IO[3:0], but later on in the "Table QSPI Interface Signals" > the second flash is described with IO[7:4]. That’s correct. > > The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and CLK# > lines for each flash. As Micron does, isn't it? Correct. > > Micron says [3] that: > "The device contains two quad I/O die, each able to operate independently > for a total of eight I/Os. The memory map applies to each die. Each die has > internal registers for status, configuration, and device protection that can be > set and read independently from one other. > Micron recommends that internal configuration settings for the two die be set > identically." > > it also says that: > "When using quad commands in XIO-SPI or when using QIO-SPI, > DQ[3:0]/DQ[7:4] are I/O." > > So I guess the upper layers just ask for a chunk of memory to be written and > the controller handles the cs# lines automatically. How is the AMD controller > working, do you have to drive the cs# lines manually, or you just set the > parallel mode and the controller takes care of everything? https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Word-Format In parallel mode, the driver sets both the CS_LOWER and CS_UPPER bits in the Cmd_FIFO_Data register(please refer the above link), and sets BUS_SEL to 2’b11 to utilize all 8 IO lines. The controller then manages the assertion and de-assertion of the CS# lines. > > I assume this is how mchp is handling things, they seem to just set a bit the > protocol into the QSPI_IFR.PROTTYP register field and that's all [4]. They even > seem to write the registers of both flashes at the same time. Yes, that's accurate, but the key distinction is that in Microchip, the QSPI controller has one CS# (referred to as QCS), whereas the AMD QSPI controller has 2 CS#(referred to as CS0 & CS1). Regards, Amit > > In what regards the AMD's "dual stack interface", AMD is sharing the clock > and IO lines and uses dedicated CS# lines for the flashes, whereas Micron > shares the CS# and CLK# lines with different IO lines. > > Amit, please study the architectures used by mchp, micron and amd and let > us know if they are the same or they differ, and if they differ what are the > differences. > > I added Conor from mchp in cc, I see Nicolas is already there, and Bean from > micron. > > Thanks, > ta > > [1] > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface- > Signals > [2] > https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv > 4ry78xM0_EF5kv4mA > [3] > https://media-www.micron.com/- > /media/client/global/documents/products/data-sheet/nor-flash/serial- > nor/mt25t/generation- > b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 > [4] > https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ > ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet- > DS60001765.pdf
> -----Original Message----- > From: Tudor Ambarus <tudor.ambarus@linaro.org> > Sent: Friday, February 9, 2024 9:44 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; > broonie@kernel.org; pratyush@kernel.org; miquel.raynal@bootlin.com; > richard@nod.at; vigneshr@ti.com; sbinding@opensource.cirrus.com; > lee@kernel.org; james.schulman@cirrus.com; david.rhodes@cirrus.com; > rf@opensource.cirrus.com; perex@perex.cz; tiwai@suse.com > Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; > michael@walle.cc; linux-mtd@lists.infradead.org; > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com; > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux- > arm-kernel@lists.infradead.org; alsa-devel@alsa-project.org; > patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD- > Xilinx) <git@amd.com>; amitrkcian2002@gmail.com; Conor Dooley > <conor.dooley@microchip.com>; beanhuo@micron.com > Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support > in spi-nor > > > > On 2/9/24 11:06, Tudor Ambarus wrote: > > > > > > On 12/21/23 06:54, Mahapatra, Amit Kumar wrote: > >>> Something else to consider: I see that Micron has a twin quad mode: > >>> https://media-www.micron.com/- > >>> /media/client/global/documents/products/data-sheet/nor-flash/serial- > >>> nor/mt25t/generation- > >>> > b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 > >>> > >>> The micron's "Separate Chip-Select and Clock Signals" resembles the > >>> AMD's dual parallel 8-bit. > >> Yes, I agree. > >> > >>> Micron's "Shared Chip-Select and Clock Signals" differs from the > >>> AMD's stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD > >>> considers both as DQ[3:0]. > >> Yes, correct. > > > > Amit, please help me to assess this. I assume Micron and Microchip is > > using the same concepts as AMD uses for the "Dual Parallel 8-bit IO > > mode", but they call it "Twin Quad Mode". > > > > I was wrong, the AMD datasheet [1] was misleading [2], it described > > the IOs for both flashes as IO[3:0], but later on in the "Table QSPI > > Interface Signals" the second flash is described with IO[7:4]. > > > > The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# > > and CLK# lines for each flash. As Micron does, isn't it? > > > > Micron says [3] that: > > "The device contains two quad I/O die, each able to operate > > independently for a total of eight I/Os. The memory map applies to > > each die. Each die has internal registers for status, configuration, > > and device protection that can be set and read independently from one > other. > > Micron recommends that internal configuration settings for the two die > > be set identically." > Hello Tudor, > Amit, > > I forgot to say my first conclusion about the quote from above. Even if the > dies are in the same physical package, micron asks users to configure each die > as it is a self-standing entity, IOW to configure each die as it is a flash on its > own. To me it looks like 2 concatenated flashes at first look. Thus identical to > how AMD controller works. > Please clarify this. That’s correct, the Micron flash that you referred can communicate with the AMD QSPI controller in both parallel and stacked mode. > > > > > it also says that: > > "When using quad commands in XIO-SPI or when using QIO-SPI, > > DQ[3:0]/DQ[7:4] are I/O." > > and this would be a parallel concatenation of two flashes. That's correct. Regards, Amit > > Then it would be good if you let us now the similarities and differences > between how amd and mchp controller work, I scrawled few ideas below. > > thanks, > ta > > > > So I guess the upper layers just ask for a chunk of memory to be > > written and the controller handles the cs# lines automatically. How is > > the AMD controller working, do you have to drive the cs# lines > > manually, or you just set the parallel mode and the controller takes care of > everything? > > > > I assume this is how mchp is handling things, they seem to just set a > > bit the protocol into the QSPI_IFR.PROTTYP register field and that's > > all [4]. They even seem to write the registers of both flashes at the same > time. > > > > In what regards the AMD's "dual stack interface", AMD is sharing the > > clock and IO lines and uses dedicated CS# lines for the flashes, > > whereas Micron shares the CS# and CLK# lines with different IO lines. > > > > Amit, please study the architectures used by mchp, micron and amd and > > let us know if they are the same or they differ, and if they differ > > what are the differences. > > > > I added Conor from mchp in cc, I see Nicolas is already there, and > > Bean from micron. > > > > Thanks, > > ta > > > > [1] > > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Inter > > face-Signals > > [2] > > > https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv > > 4ry78xM0_EF5kv4mA > > [3] > > https://media-www.micron.com/- > /media/client/global/documents/products/ > > data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xb > > a_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500 > > [4] > > > https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ > Produ > > ctDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf