RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Vignesh, Thanks for your information. > -Original Message- > From: Vignesh Raghavendra > Sent: Wednesday, November 11, 2020 1:44 PM > To: Chin-Ting Kuo ; Boris Brezillon > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > driver > > Hi Chin-Ting, > > On 11/6/20 11:57 PM, Chin-Ting Kuo wrote: > > Hi Boris, > > > >> -Original Message- > >> From: Boris Brezillon > >> Sent: Friday, November 6, 2020 7:30 PM > >> To: Chin-Ting Kuo > >> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > >> controller driver > >> > >> +Tudor and Vignesh > >> > >> On Fri, 6 Nov 2020 10:21:06 + > >> Chin-Ting Kuo wrote: > >> > >>> Hi Boris, > >>> > >>> Thanks for your comments and suggestions. > >>> > >>>> -Original Message- > >>>> From: Boris Brezillon > >>>> Sent: Friday, November 6, 2020 5:06 PM > >>>> To: Chin-Ting Kuo > >>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > >>>> controller driver > >>>> > >>>> On Fri, 6 Nov 2020 08:58:23 + > >>>> Chin-Ting Kuo wrote: > >>>> > >>>>> Hi Boris, > >>>>> > >>>>> Thanks for your quick reply. > >>>>> > >>>>>> -Original Message----- > >>>>>> From: Boris Brezillon > >>>>>> Sent: Thursday, November 5, 2020 11:12 PM > >>>>>> To: Cédric Le Goater ; robh...@kernel.org > >>>>>> Cc: Chin-Ting Kuo ; > >>>>>> broo...@kernel.org; j...@jms.id.au; and...@aj.id.au; > >>>>>> bbrezil...@kernel.org; devicet...@vger.kernel.org; > >>>>>> linux-kernel@vger.kernel.org; linux-asp...@lists.ozlabs.org; > >>>>>> linux-...@vger.kernel.org; BMC-SW > >>>>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > >>>>>> controller driver > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater > >>>>>> wrote: > >>>>>> > >>>>>>> Hello Chin-Ting, > >>>>>>> > >>>>>>> Thanks for this driver. It's much cleaner than the previous and > >>>>>>> we should try adding support for the AST2500 SoC also. I guess > >>>>>>> we can keep the old driver for the AST2400 which has a > >> different register layout. > >>>>>>> > >>>>>>> On the patchset, I think we should split this patch in three : > >>>>>>> > >>>>>>> - basic support > >>>>>>> - AHB window calculation depending on the flash size > >>>>>>> - read training support > >>>>>> > >>>>>> I didn't look closely at the implementation, but if the read > >>>>>> training tries to read a section of the NOR, I'd recommend > >>>>>> exposing that feature through spi-mem and letting the SPI-NOR > >>>>>> framework trigger the training instead of doing that at dirmap > >>>>>> creation time (remember that spi-mem is also used for SPI NANDs > >>>>>> which use the dirmap > >>>> API too, and this training is unlikely to work there). > >>>>>> > >>>>>> The SPI-NOR framework could pass a read op template and a > >>>>>> reference pattern such that all the spi-mem driver has to do is > >>>>>> execute the template op and compare the output to the reference > >> buffer. > >>>>>> > >>>>> > >>>>> I agree it. Before, I were not able to find a suitable location to > >>>>> implement > >>>> read training feature. > >>>>> I think that I can add a SPI timing training function in > >>>>> "spi_controller_mem_ops" struct and call it by a wrapper function > >>>>> called at > >>>> the bottom of spi_nor_probe() in spi-nor.c. > >>>>> Maybe, SPI-NOR framework does not need to pass reference buffer > >>>>> since calibration method depends on each SoC itself and buffer > >>&
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Chin-Ting, On 11/6/20 11:57 PM, Chin-Ting Kuo wrote: > Hi Boris, > >> -Original Message- >> From: Boris Brezillon >> Sent: Friday, November 6, 2020 7:30 PM >> To: Chin-Ting Kuo >> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller >> driver >> >> +Tudor and Vignesh >> >> On Fri, 6 Nov 2020 10:21:06 + >> Chin-Ting Kuo wrote: >> >>> Hi Boris, >>> >>> Thanks for your comments and suggestions. >>> >>>> -Original Message----- >>>> From: Boris Brezillon >>>> Sent: Friday, November 6, 2020 5:06 PM >>>> To: Chin-Ting Kuo >>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory >>>> controller driver >>>> >>>> On Fri, 6 Nov 2020 08:58:23 + >>>> Chin-Ting Kuo wrote: >>>> >>>>> Hi Boris, >>>>> >>>>> Thanks for your quick reply. >>>>> >>>>>> -Original Message- >>>>>> From: Boris Brezillon >>>>>> Sent: Thursday, November 5, 2020 11:12 PM >>>>>> To: Cédric Le Goater ; robh...@kernel.org >>>>>> Cc: Chin-Ting Kuo ; >>>>>> broo...@kernel.org; j...@jms.id.au; and...@aj.id.au; >>>>>> bbrezil...@kernel.org; devicet...@vger.kernel.org; >>>>>> linux-kernel@vger.kernel.org; linux-asp...@lists.ozlabs.org; >>>>>> linux-...@vger.kernel.org; BMC-SW >>>>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory >>>>>> controller driver >>>>>> >>>>>> Hi, >>>>>> >>>>>> On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater >>>>>> wrote: >>>>>> >>>>>>> Hello Chin-Ting, >>>>>>> >>>>>>> Thanks for this driver. It's much cleaner than the previous >>>>>>> and we should try adding support for the AST2500 SoC also. I >>>>>>> guess we can keep the old driver for the AST2400 which has a >> different register layout. >>>>>>> >>>>>>> On the patchset, I think we should split this patch in three : >>>>>>> >>>>>>> - basic support >>>>>>> - AHB window calculation depending on the flash size >>>>>>> - read training support >>>>>> >>>>>> I didn't look closely at the implementation, but if the read >>>>>> training tries to read a section of the NOR, I'd recommend >>>>>> exposing that feature through spi-mem and letting the SPI-NOR >>>>>> framework trigger the training instead of doing that at dirmap >>>>>> creation time (remember that spi-mem is also used for SPI NANDs >>>>>> which use the dirmap >>>> API too, and this training is unlikely to work there). >>>>>> >>>>>> The SPI-NOR framework could pass a read op template and a >>>>>> reference pattern such that all the spi-mem driver has to do is >>>>>> execute the template op and compare the output to the reference >> buffer. >>>>>> >>>>> >>>>> I agree it. Before, I were not able to find a suitable location to >>>>> implement >>>> read training feature. >>>>> I think that I can add a SPI timing training function in >>>>> "spi_controller_mem_ops" struct and call it by a wrapper function >>>>> called at >>>> the bottom of spi_nor_probe() in spi-nor.c. >>>>> Maybe, SPI-NOR framework does not need to pass reference buffer >>>>> since calibration method depends on each SoC itself and buffer >>>>> size may be >>>> variant. >>>>> The detail calibration method may be implemented in each SoC SPI >> driver. >>>> >>>> That's a real problem IMO. What makes this pattern SoC specific? I >>>> can see why the location in flash could be *board* specific, but the >>>> pattern should be pretty common, right? As for the spi-mem operation >>>> to be executed, it's definitely memory specific (I can imagine some >>>> flash vendors providing a specific command returning a fixed pattern >>>> that's not actually stored on a visible portion of the flash). >>> >>> You are right, the pattern should be prett
RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Boris, > -Original Message- > From: Boris Brezillon > Sent: Friday, November 6, 2020 7:30 PM > To: Chin-Ting Kuo > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > driver > > +Tudor and Vignesh > > On Fri, 6 Nov 2020 10:21:06 + > Chin-Ting Kuo wrote: > > > Hi Boris, > > > > Thanks for your comments and suggestions. > > > > > -Original Message- > > > From: Boris Brezillon > > > Sent: Friday, November 6, 2020 5:06 PM > > > To: Chin-Ting Kuo > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > > > controller driver > > > > > > On Fri, 6 Nov 2020 08:58:23 + > > > Chin-Ting Kuo wrote: > > > > > > > Hi Boris, > > > > > > > > Thanks for your quick reply. > > > > > > > > > -Original Message- > > > > > From: Boris Brezillon > > > > > Sent: Thursday, November 5, 2020 11:12 PM > > > > > To: Cédric Le Goater ; robh...@kernel.org > > > > > Cc: Chin-Ting Kuo ; > > > > > broo...@kernel.org; j...@jms.id.au; and...@aj.id.au; > > > > > bbrezil...@kernel.org; devicet...@vger.kernel.org; > > > > > linux-kernel@vger.kernel.org; linux-asp...@lists.ozlabs.org; > > > > > linux-...@vger.kernel.org; BMC-SW > > > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > > > > > controller driver > > > > > > > > > > Hi, > > > > > > > > > > On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater > > > > > wrote: > > > > > > > > > > > Hello Chin-Ting, > > > > > > > > > > > > Thanks for this driver. It's much cleaner than the previous > > > > > > and we should try adding support for the AST2500 SoC also. I > > > > > > guess we can keep the old driver for the AST2400 which has a > different register layout. > > > > > > > > > > > > On the patchset, I think we should split this patch in three : > > > > > > > > > > > > - basic support > > > > > > - AHB window calculation depending on the flash size > > > > > > - read training support > > > > > > > > > > I didn't look closely at the implementation, but if the read > > > > > training tries to read a section of the NOR, I'd recommend > > > > > exposing that feature through spi-mem and letting the SPI-NOR > > > > > framework trigger the training instead of doing that at dirmap > > > > > creation time (remember that spi-mem is also used for SPI NANDs > > > > > which use the dirmap > > > API too, and this training is unlikely to work there). > > > > > > > > > > The SPI-NOR framework could pass a read op template and a > > > > > reference pattern such that all the spi-mem driver has to do is > > > > > execute the template op and compare the output to the reference > buffer. > > > > > > > > > > > > > I agree it. Before, I were not able to find a suitable location to > > > > implement > > > read training feature. > > > > I think that I can add a SPI timing training function in > > > > "spi_controller_mem_ops" struct and call it by a wrapper function > > > > called at > > > the bottom of spi_nor_probe() in spi-nor.c. > > > > Maybe, SPI-NOR framework does not need to pass reference buffer > > > > since calibration method depends on each SoC itself and buffer > > > > size may be > > > variant. > > > > The detail calibration method may be implemented in each SoC SPI > driver. > > > > > > That's a real problem IMO. What makes this pattern SoC specific? I > > > can see why the location in flash could be *board* specific, but the > > > pattern should be pretty common, right? As for the spi-mem operation > > > to be executed, it's definitely memory specific (I can imagine some > > > flash vendors providing a specific command returning a fixed pattern > > > that's not actually stored on a visible portion of the flash). > > > > You are right, the pattern should be pretty common. The thing I was > > worried about is the size of that buffer since, maybe, some > > controllers need to read more data than others in order to get good training &
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
+Tudor and Vignesh On Fri, 6 Nov 2020 10:21:06 + Chin-Ting Kuo wrote: > Hi Boris, > > Thanks for your comments and suggestions. > > > -Original Message- > > From: Boris Brezillon > > Sent: Friday, November 6, 2020 5:06 PM > > To: Chin-Ting Kuo > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > > driver > > > > On Fri, 6 Nov 2020 08:58:23 + > > Chin-Ting Kuo wrote: > > > > > Hi Boris, > > > > > > Thanks for your quick reply. > > > > > > > -Original Message- > > > > From: Boris Brezillon > > > > Sent: Thursday, November 5, 2020 11:12 PM > > > > To: Cédric Le Goater ; robh...@kernel.org > > > > Cc: Chin-Ting Kuo ; > > > > broo...@kernel.org; j...@jms.id.au; and...@aj.id.au; > > > > bbrezil...@kernel.org; devicet...@vger.kernel.org; > > > > linux-kernel@vger.kernel.org; linux-asp...@lists.ozlabs.org; > > > > linux-...@vger.kernel.org; BMC-SW > > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > > > > controller driver > > > > > > > > Hi, > > > > > > > > On Thu, 5 Nov 2020 15:09:11 +0100 > > > > Cédric Le Goater wrote: > > > > > > > > > Hello Chin-Ting, > > > > > > > > > > Thanks for this driver. It's much cleaner than the previous and we > > > > > should try adding support for the AST2500 SoC also. I guess we can > > > > > keep the old driver for the AST2400 which has a different register > > > > > layout. > > > > > > > > > > On the patchset, I think we should split this patch in three : > > > > > > > > > > - basic support > > > > > - AHB window calculation depending on the flash size > > > > > - read training support > > > > > > > > I didn't look closely at the implementation, but if the read > > > > training tries to read a section of the NOR, I'd recommend exposing > > > > that feature through spi-mem and letting the SPI-NOR framework > > > > trigger the training instead of doing that at dirmap creation time > > > > (remember that spi-mem is also used for SPI NANDs which use the dirmap > > API too, and this training is unlikely to work there). > > > > > > > > The SPI-NOR framework could pass a read op template and a reference > > > > pattern such that all the spi-mem driver has to do is execute the > > > > template op and compare the output to the reference buffer. > > > > > > > > > > I agree it. Before, I were not able to find a suitable location to > > > implement > > read training feature. > > > I think that I can add a SPI timing training function in > > > "spi_controller_mem_ops" struct and call it by a wrapper function called > > > at > > the bottom of spi_nor_probe() in spi-nor.c. > > > Maybe, SPI-NOR framework does not need to pass reference buffer since > > > calibration method depends on each SoC itself and buffer size may be > > variant. > > > The detail calibration method may be implemented in each SoC SPI driver. > > > > That's a real problem IMO. What makes this pattern SoC specific? I can see > > why the location in flash could be *board* specific, but the pattern should > > be > > pretty common, right? As for the spi-mem operation to be executed, it's > > definitely memory specific (I can imagine some flash vendors providing a > > specific command returning a fixed pattern that's not actually stored on a > > visible portion of the flash). > > You are right, the pattern should be pretty common. The thing I was worried > about is the size of > that buffer since, maybe, some controllers need to read more data than others > in order to get good > training result. It would be good to see how other controllers implement that. I know that the Cadence controller had something similar. Vignesh might be able to share his thoughts on this. > > > > > > > Besides, I am thinking about the possibility for adding a > > > "spi_mem_post_init" function in spi-mem framework sine for some SoCs, > > > SPI controller needs to adjust some settings after getting SPI flash > > information. > > > > I don't think that's a good idea. The spi-mem interface should stay > > memory-type agnostic and doing that means we somehow pass N
RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Boris, Thanks for your comments and suggestions. > -Original Message- > From: Boris Brezillon > Sent: Friday, November 6, 2020 5:06 PM > To: Chin-Ting Kuo > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > driver > > On Fri, 6 Nov 2020 08:58:23 + > Chin-Ting Kuo wrote: > > > Hi Boris, > > > > Thanks for your quick reply. > > > > > -Original Message- > > > From: Boris Brezillon > > > Sent: Thursday, November 5, 2020 11:12 PM > > > To: Cédric Le Goater ; robh...@kernel.org > > > Cc: Chin-Ting Kuo ; > > > broo...@kernel.org; j...@jms.id.au; and...@aj.id.au; > > > bbrezil...@kernel.org; devicet...@vger.kernel.org; > > > linux-kernel@vger.kernel.org; linux-asp...@lists.ozlabs.org; > > > linux-...@vger.kernel.org; BMC-SW > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory > > > controller driver > > > > > > Hi, > > > > > > On Thu, 5 Nov 2020 15:09:11 +0100 > > > Cédric Le Goater wrote: > > > > > > > Hello Chin-Ting, > > > > > > > > Thanks for this driver. It's much cleaner than the previous and we > > > > should try adding support for the AST2500 SoC also. I guess we can > > > > keep the old driver for the AST2400 which has a different register > > > > layout. > > > > > > > > On the patchset, I think we should split this patch in three : > > > > > > > > - basic support > > > > - AHB window calculation depending on the flash size > > > > - read training support > > > > > > I didn't look closely at the implementation, but if the read > > > training tries to read a section of the NOR, I'd recommend exposing > > > that feature through spi-mem and letting the SPI-NOR framework > > > trigger the training instead of doing that at dirmap creation time > > > (remember that spi-mem is also used for SPI NANDs which use the dirmap > API too, and this training is unlikely to work there). > > > > > > The SPI-NOR framework could pass a read op template and a reference > > > pattern such that all the spi-mem driver has to do is execute the > > > template op and compare the output to the reference buffer. > > > > > > > I agree it. Before, I were not able to find a suitable location to implement > read training feature. > > I think that I can add a SPI timing training function in > > "spi_controller_mem_ops" struct and call it by a wrapper function called at > the bottom of spi_nor_probe() in spi-nor.c. > > Maybe, SPI-NOR framework does not need to pass reference buffer since > > calibration method depends on each SoC itself and buffer size may be > variant. > > The detail calibration method may be implemented in each SoC SPI driver. > > That's a real problem IMO. What makes this pattern SoC specific? I can see > why the location in flash could be *board* specific, but the pattern should be > pretty common, right? As for the spi-mem operation to be executed, it's > definitely memory specific (I can imagine some flash vendors providing a > specific command returning a fixed pattern that's not actually stored on a > visible portion of the flash). You are right, the pattern should be pretty common. The thing I was worried about is the size of that buffer since, maybe, some controllers need to read more data than others in order to get good training result. > > > > Besides, I am thinking about the possibility for adding a > > "spi_mem_post_init" function in spi-mem framework sine for some SoCs, > > SPI controller needs to adjust some settings after getting SPI flash > information. > > I don't think that's a good idea. The spi-mem interface should stay > memory-type agnostic and doing that means we somehow pass NOR specific > info. What is it that you need exactly, and why? Yes, as you mention, the spi-mem interface should stay memory-type agnostic. Thus, currently, I just think about this, not implementation. Why did I need this exactly? Take ASPEED SPI controller for example, ASPEED SPI controller is designed for SPI NOR flash especially. When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash through SPI controller. But, MCU ROM does not know the current address mode of SPI NOR flash when SoC was reset (SPI flash is not reset). Therefore, SPI flash driver needs to set related flag to notify MCU ROM when flash is set to 4B address mode and 4B read opcode is used. Besides, for other SoCs connected to ASPEED SoC, they can read/write SPI NOR flash connected to ASPEED SoC by a pure HW channel without any interaction of SW driver. But, before trigger this feature, flash read/write/erase opcode, dummy cycle and other information should be filled in the related registers in advance because that HW channel does not know accurate information about connected SPI NOR flash. Best Wishes, Chin-Ting
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
On Fri, 6 Nov 2020 08:58:23 + Chin-Ting Kuo wrote: > Hi Boris, > > Thanks for your quick reply. > > > -Original Message- > > From: Boris Brezillon > > Sent: Thursday, November 5, 2020 11:12 PM > > To: Cédric Le Goater ; robh...@kernel.org > > Cc: Chin-Ting Kuo ; broo...@kernel.org; > > j...@jms.id.au; and...@aj.id.au; bbrezil...@kernel.org; > > devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-asp...@lists.ozlabs.org; linux-...@vger.kernel.org; BMC-SW > > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > > driver > > > > Hi, > > > > On Thu, 5 Nov 2020 15:09:11 +0100 > > Cédric Le Goater wrote: > > > > > Hello Chin-Ting, > > > > > > Thanks for this driver. It's much cleaner than the previous and we > > > should try adding support for the AST2500 SoC also. I guess we can > > > keep the old driver for the AST2400 which has a different register layout. > > > > > > On the patchset, I think we should split this patch in three : > > > > > > - basic support > > > - AHB window calculation depending on the flash size > > > - read training support > > > > I didn't look closely at the implementation, but if the read training tries > > to read > > a section of the NOR, I'd recommend exposing that feature through spi-mem > > and letting the SPI-NOR framework trigger the training instead of doing > > that at > > dirmap creation time (remember that spi-mem is also used for SPI NANDs > > which use the dirmap API too, and this training is unlikely to work there). > > > > The SPI-NOR framework could pass a read op template and a reference pattern > > such that all the spi-mem driver has to do is execute the template op and > > compare the output to the reference buffer. > > > > I agree it. Before, I were not able to find a suitable location to implement > read training feature. > I think that I can add a SPI timing training function in > "spi_controller_mem_ops" struct and > call it by a wrapper function called at the bottom of spi_nor_probe() in > spi-nor.c. > Maybe, SPI-NOR framework does not need to pass reference buffer since > calibration > method depends on each SoC itself and buffer size may be variant. > The detail calibration method may be implemented in each SoC SPI driver. That's a real problem IMO. What makes this pattern SoC specific? I can see why the location in flash could be *board* specific, but the pattern should be pretty common, right? As for the spi-mem operation to be executed, it's definitely memory specific (I can imagine some flash vendors providing a specific command returning a fixed pattern that's not actually stored on a visible portion of the flash). > > Besides, I am thinking about the possibility for adding a "spi_mem_post_init" > function in > spi-mem framework sine for some SoCs, SPI controller needs to adjust some > settings > after getting SPI flash information. I don't think that's a good idea. The spi-mem interface should stay memory-type agnostic and doing that means we somehow pass NOR specific info. What is it that you need exactly, and why?
RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Mark, > -Original Message- > From: Mark Brown > Sent: Friday, November 6, 2020 12:43 AM > To: Boris Brezillon > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > driver > > On Thu, Nov 05, 2020 at 04:11:32PM +0100, Boris Brezillon wrote: > > Cédric Le Goater wrote: > > > > Thanks for this driver. It's much cleaner than the previous and we > > > should try adding support for the AST2500 SoC also. I guess we can > > > keep the old driver for the AST2400 which has a different register layout. > > > > > > On the patchset, I think we should split this patch in three : > > > > > > - basic support > > > - AHB window calculation depending on the flash size > > > - read training support > > > I didn't look closely at the implementation, but if the read training > > tries to read a section of the NOR, I'd recommend exposing that > > feature through spi-mem and letting the SPI-NOR framework trigger the > > training instead of doing that at dirmap creation time (remember that > > spi-mem is also used for SPI NANDs which use the dirmap API too, and > > this training is unlikely to work there). > > > The SPI-NOR framework could pass a read op template and a reference > > pattern such that all the spi-mem driver has to do is execute the > > template op and compare the output to the reference buffer. > > That seems like a good idea. Yes, this idea will be implemented on the next patch version. > > > > We should avoid magic values when setting registers. This is > > > confusing and defines are much better. > > It does depend a bit on documentation though, it's not a hard requirement. I will update it on the next patch version. Thanks. Best Wishes, Chin-Ting
RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi Boris,
Thanks for your quick reply.
> -Original Message-
> From: Boris Brezillon
> Sent: Thursday, November 5, 2020 11:12 PM
> To: Cédric Le Goater ; robh...@kernel.org
> Cc: Chin-Ting Kuo ; broo...@kernel.org;
> j...@jms.id.au; and...@aj.id.au; bbrezil...@kernel.org;
> devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-asp...@lists.ozlabs.org; linux-...@vger.kernel.org; BMC-SW
>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
>
> Hi,
>
> On Thu, 5 Nov 2020 15:09:11 +0100
> Cédric Le Goater wrote:
>
> > Hello Chin-Ting,
> >
> > Thanks for this driver. It's much cleaner than the previous and we
> > should try adding support for the AST2500 SoC also. I guess we can
> > keep the old driver for the AST2400 which has a different register layout.
> >
> > On the patchset, I think we should split this patch in three :
> >
> > - basic support
> > - AHB window calculation depending on the flash size
> > - read training support
>
> I didn't look closely at the implementation, but if the read training tries
> to read
> a section of the NOR, I'd recommend exposing that feature through spi-mem
> and letting the SPI-NOR framework trigger the training instead of doing that
> at
> dirmap creation time (remember that spi-mem is also used for SPI NANDs
> which use the dirmap API too, and this training is unlikely to work there).
>
> The SPI-NOR framework could pass a read op template and a reference pattern
> such that all the spi-mem driver has to do is execute the template op and
> compare the output to the reference buffer.
>
I agree it. Before, I were not able to find a suitable location to implement
read training feature.
I think that I can add a SPI timing training function in
"spi_controller_mem_ops" struct and
call it by a wrapper function called at the bottom of spi_nor_probe() in
spi-nor.c.
Maybe, SPI-NOR framework does not need to pass reference buffer since
calibration
method depends on each SoC itself and buffer size may be variant.
The detail calibration method may be implemented in each SoC SPI driver.
Besides, I am thinking about the possibility for adding a "spi_mem_post_init"
function in
spi-mem framework sine for some SoCs, SPI controller needs to adjust some
settings
after getting SPI flash information.
>
> >
> > We should avoid magic values when setting registers. This is confusing
> > and defines are much better.
> >
> > AST2500 support will be a bit challenging because HW does not allow
> > to configure a 128MB AHB window, max is 120MB This is a bug and the
> > work around is to use user mode for the remaining 8MB. Something to
> > keep in mind.
>
> Most SPI-MEM controllers don't have such a big dirmap window anyway, and
> that shouldn't be a problem, because we don't expose the direct mapping
> directly (as would be done if we were trying to support something like XIP).
> That means that, assuming your controller allows controlling the base spi-mem
> address the direct mapping points to, you should be able to adjust the window
> at runtime and make it point where you requested.
>
> Note that dirmap_{read,write}() are allowed to return less data than requested
> thus simplifying the case where a specific access requires a window
> adjustment in the middle of an read/write operation.
Thanks for your remainder.
Best Wishes,
Chin-Ting
RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi C, Thanks for your reply. > -Original Message- > From: Cédric Le Goater > Sent: Thursday, November 5, 2020 10:09 PM > To: Chin-Ting Kuo ; broo...@kernel.org; > robh...@kernel.org; j...@jms.id.au; and...@aj.id.au; bbrezil...@kernel.org; > devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-asp...@lists.ozlabs.org; linux-...@vger.kernel.org > Cc: BMC-SW > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller > driver > > Hello Chin-Ting, > > Thanks for this driver. It's much cleaner than the previous and we should try > adding support for the AST2500 SoC also. I guess we can keep the old driver > for the AST2400 which has a different register layout. > I will also add AST2500 part into next patch version. > On the patchset, I think we should split this patch in three : > > - basic support > - AHB window calculation depending on the flash size > - read training support Oaky, it is more clear. > We should avoid magic values when setting registers. This is confusing and > defines are much better. Okay, it will be modified at next patch. > AST2500 support will be a bit challenging because HW does not allow > to configure a 128MB AHB window, max is 120MB This is a bug and the work > around is to use user mode for the remaining 8MB. Something to keep in > mind. This is why there is a "min_decode_sz" element in "aspeed_spi_info" struct. Thanks for your reminder. Some replies for the comments below. > I gave it a try on QEMU. It looks good. When I can revive my EVB, I will do > the > same. > > More comments below, > > Thanks, > > C. > > > On 11/5/20 1:03 PM, Chin-Ting Kuo wrote: > > Add driver for ASPEED BMC FMC/SPI memory controller which supports > > spi-mem interface. > > > > There are three SPI memory controllers embedded in an ASPEED SoC. > > Each of them can connect to two or three SPI NOR flashes. The first > > SPI memory controller is also named as Firmware Memory Controller > > (FMC), which is similar to SPI memory controller. After device AC on, > > MCU ROM can fetch device boot code from FMC CS 0. Thus, there exists > > additional registers for boot process control in FMC. > > > > ASPEED SPI memory controller supports single, dual and quad mode for > > SPI NOR flash. It also supports two types of command mode, user mode > > and command read/write mode. User mode is traditional pure SPI > > operations where all transmission is controlled by CPU. Contrarily, > > with command read/write mode, SPI controller can send command and > > address automatically when CPU read/write related remapped address. > > > > Besides, different wafer processes of SPI NOR flash result in > > different signal response time. This phenomenon will be enlarged when > > SPI clock frequency increases. ASPEED SPI memory controller provides a > > mechanism for timing compensation in order to satisfy various SPI NOR > > flash parts and PCB layout. > > > > Signed-off-by: Chin-Ting Kuo > > --- > > v2: Fix sparse warnings reported by kernel test robot . > > v3: Fix build warnings with x86 allmodconfig. > > > > drivers/spi/Kconfig | 10 + > > drivers/spi/Makefile | 1 + > > drivers/spi/spi-aspeed.c | 969 > > +++ > > 3 files changed, 980 insertions(+) > > create mode 100644 drivers/spi/spi-aspeed.c > > > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index > > 5cff60de8e83..c848f2f7b694 100644 > > --- a/drivers/spi/Kconfig > > +++ b/drivers/spi/Kconfig > > @@ -70,6 +70,16 @@ config SPI_AR934X > > This enables support for the SPI controller present on the > > Qualcomm Atheros AR934X/QCA95XX SoCs. > > > > +config SPI_ASPEED > > + tristate "ASPEED FMC/SPI Memory Controller" > > + depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST) > > We will need to do something about the other driver. For the moment, we can > select both but that won't be the case anymore when we add > AST2500 support. > Yes, maybe, "!SPI_ASPEED_SMC" should be added and also add "!SPI_ASPEED" for the other driver. Thanks for reminder. > > + help > > + Enable driver for ASPEED FMC/SPI Memory Controller. > > + > > + This driver is not a generic pure SPI driver, which > > + is especially designed for spi-mem framework with > > + SPI NOR flash direct read and write features. > > + > > config SPI_ATH79 > > tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver" &
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
On Thu, Nov 05, 2020 at 04:11:32PM +0100, Boris Brezillon wrote: > Cédric Le Goater wrote: > > Thanks for this driver. It's much cleaner than the previous and we should > > try adding support for the AST2500 SoC also. I guess we can keep the old > > driver for the AST2400 which has a different register layout. > > > > On the patchset, I think we should split this patch in three : > > > > - basic support > > - AHB window calculation depending on the flash size > > - read training support > I didn't look closely at the implementation, but if the read training > tries to read a section of the NOR, I'd recommend exposing that feature > through spi-mem and letting the SPI-NOR framework trigger the training > instead of doing that at dirmap creation time (remember that spi-mem is > also used for SPI NANDs which use the dirmap API too, and this training > is unlikely to work there). > The SPI-NOR framework could pass a read op template and a reference > pattern such that all the spi-mem driver has to do is execute the > template op and compare the output to the reference buffer. That seems like a good idea. > > We should avoid magic values when setting registers. This is confusing > > and defines are much better. It does depend a bit on documentation though, it's not a hard requirement. signature.asc Description: PGP signature
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hi,
On Thu, 5 Nov 2020 15:09:11 +0100
Cédric Le Goater wrote:
> Hello Chin-Ting,
>
> Thanks for this driver. It's much cleaner than the previous and we should
> try adding support for the AST2500 SoC also. I guess we can keep the old
> driver for the AST2400 which has a different register layout.
>
> On the patchset, I think we should split this patch in three :
>
> - basic support
> - AHB window calculation depending on the flash size
> - read training support
I didn't look closely at the implementation, but if the read training
tries to read a section of the NOR, I'd recommend exposing that feature
through spi-mem and letting the SPI-NOR framework trigger the training
instead of doing that at dirmap creation time (remember that spi-mem is
also used for SPI NANDs which use the dirmap API too, and this training
is unlikely to work there).
The SPI-NOR framework could pass a read op template and a reference
pattern such that all the spi-mem driver has to do is execute the
template op and compare the output to the reference buffer.
>
> We should avoid magic values when setting registers. This is confusing
> and defines are much better.
>
> AST2500 support will be a bit challenging because HW does not allow
> to configure a 128MB AHB window, max is 120MB This is a bug and the work
> around is to use user mode for the remaining 8MB. Something to keep in
> mind.
Most SPI-MEM controllers don't have such a big dirmap window anyway, and
that shouldn't be a problem, because we don't expose the direct mapping
directly (as would be done if we were trying to support something like
XIP). That means that, assuming your controller allows controlling the
base spi-mem address the direct mapping points to, you should be able
to adjust the window at runtime and make it point where you requested.
Note that dirmap_{read,write}() are allowed to return less data than
requested thus simplifying the case where a specific access requires a
window adjustment in the middle of an read/write operation.
Regards,
Boris
Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Hello Chin-Ting, Thanks for this driver. It's much cleaner than the previous and we should try adding support for the AST2500 SoC also. I guess we can keep the old driver for the AST2400 which has a different register layout. On the patchset, I think we should split this patch in three : - basic support - AHB window calculation depending on the flash size - read training support We should avoid magic values when setting registers. This is confusing and defines are much better. AST2500 support will be a bit challenging because HW does not allow to configure a 128MB AHB window, max is 120MB This is a bug and the work around is to use user mode for the remaining 8MB. Something to keep in mind. I gave it a try on QEMU. It looks good. When I can revive my EVB, I will do the same. More comments below, Thanks, C. On 11/5/20 1:03 PM, Chin-Ting Kuo wrote: > Add driver for ASPEED BMC FMC/SPI memory controller which > supports spi-mem interface. > > There are three SPI memory controllers embedded in an ASPEED SoC. > Each of them can connect to two or three SPI NOR flashes. The first > SPI memory controller is also named as Firmware Memory Controller (FMC), > which is similar to SPI memory controller. After device AC on, MCU ROM > can fetch device boot code from FMC CS 0. Thus, there exists additional > registers for boot process control in FMC. > > ASPEED SPI memory controller supports single, dual and quad mode for > SPI NOR flash. It also supports two types of command mode, user mode > and command read/write mode. User mode is traditional pure SPI operations > where all transmission is controlled by CPU. Contrarily, with command > read/write mode, SPI controller can send command and address automatically > when CPU read/write related remapped address. > > Besides, different wafer processes of SPI NOR flash result in different > signal response time. This phenomenon will be enlarged when SPI clock > frequency increases. ASPEED SPI memory controller provides a mechanism > for timing compensation in order to satisfy various SPI NOR flash parts > and PCB layout. > > Signed-off-by: Chin-Ting Kuo > --- > v2: Fix sparse warnings reported by kernel test robot . > v3: Fix build warnings with x86 allmodconfig. > > drivers/spi/Kconfig | 10 + > drivers/spi/Makefile | 1 + > drivers/spi/spi-aspeed.c | 969 +++ > 3 files changed, 980 insertions(+) > create mode 100644 drivers/spi/spi-aspeed.c > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > index 5cff60de8e83..c848f2f7b694 100644 > --- a/drivers/spi/Kconfig > +++ b/drivers/spi/Kconfig > @@ -70,6 +70,16 @@ config SPI_AR934X > This enables support for the SPI controller present on the > Qualcomm Atheros AR934X/QCA95XX SoCs. > > +config SPI_ASPEED > + tristate "ASPEED FMC/SPI Memory Controller" > + depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST) We will need to do something about the other driver. For the moment, we can select both but that won't be the case anymore when we add AST2500 support. > + help > + Enable driver for ASPEED FMC/SPI Memory Controller. > + > + This driver is not a generic pure SPI driver, which > + is especially designed for spi-mem framework with > + SPI NOR flash direct read and write features. > + > config SPI_ATH79 > tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver" > depends on ATH79 || COMPILE_TEST > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > index 6fea5821662e..9e62c650fca0 100644 > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) += > spi-loopback-test.o > obj-$(CONFIG_SPI_ALTERA) += spi-altera.o > obj-$(CONFIG_SPI_AR934X) += spi-ar934x.o > obj-$(CONFIG_SPI_ARMADA_3700)+= spi-armada-3700.o > +obj-$(CONFIG_SPI_ASPEED) += spi-aspeed.o > obj-$(CONFIG_SPI_ATMEL) += spi-atmel.o > obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o > obj-$(CONFIG_SPI_AT91_USART) += spi-at91-usart.o > diff --git a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c > new file mode 100644 > index ..cfaaa0d5bac6 > --- /dev/null > +++ b/drivers/spi/spi-aspeed.c > @@ -0,0 +1,969 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * ASPEED FMC/SPI Memory Controller Driver > + * > + * Copyright (c) 2020, ASPEED Corporation. > + * Copyright (c) 2015-2016, IBM Corporation. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* ASPEED FMC/SPI memory control register related */ > +#define OFFSET_CE_TYPE_SETTING 0x00 > +#define OFFSET_CE_ADDR_MODE_CTRL 0x04 > +#define OFFSET_INTR_CTRL_STATUS 0x08 > +#define
[v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
Add driver for ASPEED BMC FMC/SPI memory controller which
supports spi-mem interface.
There are three SPI memory controllers embedded in an ASPEED SoC.
Each of them can connect to two or three SPI NOR flashes. The first
SPI memory controller is also named as Firmware Memory Controller (FMC),
which is similar to SPI memory controller. After device AC on, MCU ROM
can fetch device boot code from FMC CS 0. Thus, there exists additional
registers for boot process control in FMC.
ASPEED SPI memory controller supports single, dual and quad mode for
SPI NOR flash. It also supports two types of command mode, user mode
and command read/write mode. User mode is traditional pure SPI operations
where all transmission is controlled by CPU. Contrarily, with command
read/write mode, SPI controller can send command and address automatically
when CPU read/write related remapped address.
Besides, different wafer processes of SPI NOR flash result in different
signal response time. This phenomenon will be enlarged when SPI clock
frequency increases. ASPEED SPI memory controller provides a mechanism
for timing compensation in order to satisfy various SPI NOR flash parts
and PCB layout.
Signed-off-by: Chin-Ting Kuo
---
v2: Fix sparse warnings reported by kernel test robot .
v3: Fix build warnings with x86 allmodconfig.
drivers/spi/Kconfig | 10 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-aspeed.c | 969 +++
3 files changed, 980 insertions(+)
create mode 100644 drivers/spi/spi-aspeed.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 5cff60de8e83..c848f2f7b694 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -70,6 +70,16 @@ config SPI_AR934X
This enables support for the SPI controller present on the
Qualcomm Atheros AR934X/QCA95XX SoCs.
+config SPI_ASPEED
+ tristate "ASPEED FMC/SPI Memory Controller"
+ depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST)
+ help
+ Enable driver for ASPEED FMC/SPI Memory Controller.
+
+ This driver is not a generic pure SPI driver, which
+ is especially designed for spi-mem framework with
+ SPI NOR flash direct read and write features.
+
config SPI_ATH79
tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
depends on ATH79 || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 6fea5821662e..9e62c650fca0 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) +=
spi-loopback-test.o
obj-$(CONFIG_SPI_ALTERA) += spi-altera.o
obj-$(CONFIG_SPI_AR934X) += spi-ar934x.o
obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o
+obj-$(CONFIG_SPI_ASPEED) += spi-aspeed.o
obj-$(CONFIG_SPI_ATMEL)+= spi-atmel.o
obj-$(CONFIG_SPI_ATMEL_QUADSPI)+= atmel-quadspi.o
obj-$(CONFIG_SPI_AT91_USART) += spi-at91-usart.o
diff --git a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c
new file mode 100644
index ..cfaaa0d5bac6
--- /dev/null
+++ b/drivers/spi/spi-aspeed.c
@@ -0,0 +1,969 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * ASPEED FMC/SPI Memory Controller Driver
+ *
+ * Copyright (c) 2020, ASPEED Corporation.
+ * Copyright (c) 2015-2016, IBM Corporation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* ASPEED FMC/SPI memory control register related */
+#define OFFSET_CE_TYPE_SETTING 0x00
+#define OFFSET_CE_ADDR_MODE_CTRL 0x04
+#define OFFSET_INTR_CTRL_STATUS0x08
+#define OFFSET_ADDR_DATA_MASK 0x0c
+#define OFFSET_CE0_CTRL_REG0x10
+#define OFFSET_CE0_DECODE_RANGE_REG0x30
+#define OFFSET_DMA_CTRL0x80
+#define OFFSET_DMA_FLASH_ADDR_REG 0x84
+#define OFFSET_DMA_RAM_ADDR_REG0x88
+#define OFFSET_DMA_LEN_REG 0x8c
+#define OFFSET_DMA_CHECKSUM_RESULT 0x90
+#define OFFSET_CE0_TIMING_COMPENSATION 0x94
+
+#define CTRL_IO_SINGLE_DATA0
+#define CTRL_IO_DUAL_DATA BIT(29)
+#define CTRL_IO_QUAD_DATA BIT(30)
+
+#define CTRL_IO_MODE_USER GENMASK(1, 0)
+#define CTRL_IO_MODE_CMD_READ BIT(0)
+#define CTRL_IO_MODE_CMD_WRITE BIT(1)
+#define CTRL_STOP_ACTIVE BIT(2)
+
+#define CALIBRATION_LEN0x400
+#define SPI_DAM_REQUESTBIT(31)
+#define SPI_DAM_GRANT BIT(30)
+#define SPI_DMA_CALIB_MODE BIT(3)
+#define SPI_DMA_CALC_CKSUM BIT(2)
+#define SPI_DMA_ENABLE BIT(0)
+#define SPI_DMA_STATUS BIT(11)
+
+enum aspeed_spi_ctl_reg_value {
+ ASPEED_SPI_BASE,
+ ASPEED_SPI_READ,
+ ASPEED_SPI_WRITE,
+ ASPEED_SPI_MAX,
+};
+
+#define ASPEED_SPI_MAX_CS 5
+
+struct aspeed_spi_controller;
+struct aspeed_spi_chip;
+
