Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hi Cédric, On 25/01/19 11:30 PM, Boris Brezillon wrote: > +Vignesh > [...] >> The first is about performing direct accesses on the AHB window on which >> the flash contents is mapped. > > We have introduced the dirmap API/interface exactly for this purpose, > and the SPI NOR layer will use it in 5.1 (see [1]). > >> >> How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command >> for instance ? > > Why do you need to know the access type? > >> Are the drivers expected to check the SPI OP command and >> depending on the target/command redirect to the appropriate address space ? >> > > Definitely not, the SPI MEM layer is supposed to be memory-type > agnostic, so you should not guess the operation type based on the > opcode. For direct mapping accesses, just implement the ->dirmap_xxx > hooks at the controller level and you'll be able to use the feature.>> >> Also, Aspeed SPI controllers have a Read Timing Compensation Register which >> defines different data input delay cycles depending on SPI clock rates. This >> register is supposed to be tuned when the flash chip characteristics are >> known, after the first bus scan. Is there a way to know that our SPI slave >> is alive and well detected before starting hammering successive reads on it >> to see how it behaves. > > Vignesh mentioned that a while back (couldn't find the thread where > this discussion happened) and I suggested adding a new hook to do this > "link training" process where you'd pass a spi_mem_op template + the > expected result so that the controller can test different setups until > it finds a working one. > Right, Cadence QSPI/OSPI needs PHY DLL values to be tuned for operating at higher frequencies. So idea is to use READID to read flash ID at lowest speed and use that as golden reference for tuning/link training at higher frequencies. I am guessing Aspeed SPI controller has similar need. Here is the discussion that Boris was talking about: https://lkml.org/lkml/2018/10/4/468 I haven't been able to get to implement his suggestions yet, but I think idea is generic enough. >> >> >> I think the U-Boot and Linux driver will be very similar wrt the issues >> above ? > I have submitted patches[1] to sync SPI NOR framework from kernel to U-Boot. Once that's merged then Aspeed SPI can make use of spi-mem interface in U-Boot as well. [1] https://patchwork.ozlabs.org/cover/1017335/ -- Regards Vignesh ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
+Vignesh Hi Cédric, On Fri, 25 Jan 2019 18:28:02 +0100 Cédric Le Goater wrote: > Hello > > On 10/10/18 2:02 PM, Cédric Le Goater wrote: > > Hello Boris, > > > > On 10/10/18 9:32 AM, Boris Brezillon wrote: > >> Hi Cédric, > >> > >> On Wed, 10 Oct 2018 11:46:56 +0530 > >> Jagan Teki wrote: > >> > >>> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater wrote: > > On 10/4/18 5:57 PM, Jagan Teki wrote: > > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: > > > >> > >> Hello Simon, > >> > >> > >> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash > >> memory, > >> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some > >> misunderstanding on this driver, it might come from the fact it is > >> closer > >> to a SPI-NOR driver like we have in Linux, than a generic SPI driver. > >> The stm32 SPI driver is somewhat similar. > >> > >> Should we move it under drivers/mtd/spi/ maybe ? > > > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem > > instead of mtd/spi-nor. So I think you can handle this via new > > spi-mem. have you send any patches to Linux? > > No, not yet. The patchset is sent : > > https://patchwork.ozlabs.org/cover/933293/ > > is not using spimem. I was not aware of that change in the spi-nor layer > at the time. I will take a look. > >> > >> Indeed, if you have some time to convert the Linux aspeed driver to > >> the spi-mem interface that would be appreciated. > > > > Yes. That's the plan. I have a series on the way but I will see if I can > > rework a v2 to use spi-mem. > > I took a look at spi-mem and worked on an initial spi-aspeed-smc driver > using the new framework in Linux 5.0.x. It looks good but I have a couple > of questions. Great! > > > The first is about performing direct accesses on the AHB window on which > the flash contents is mapped. We have introduced the dirmap API/interface exactly for this purpose, and the SPI NOR layer will use it in 5.1 (see [1]). > > How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command > for instance ? Why do you need to know the access type? > Are the drivers expected to check the SPI OP command and > depending on the target/command redirect to the appropriate address space ? Definitely not, the SPI MEM layer is supposed to be memory-type agnostic, so you should not guess the operation type based on the opcode. For direct mapping accesses, just implement the ->dirmap_xxx hooks at the controller level and you'll be able to use the feature. > > Also, Aspeed SPI controllers have a Read Timing Compensation Register which > defines different data input delay cycles depending on SPI clock rates. This > register is supposed to be tuned when the flash chip characteristics are > known, after the first bus scan. Is there a way to know that our SPI slave > is alive and well detected before starting hammering successive reads on it > to see how it behaves. Vignesh mentioned that a while back (couldn't find the thread where this discussion happened) and I suggested adding a new hook to do this "link training" process where you'd pass a spi_mem_op template + the expected result so that the controller can test different setups until it finds a working one. > > > I think the U-Boot and Linux driver will be very similar wrt the issues > above ? I hope so. Thanks for working on this conversion. Boris [1]https://patchwork.kernel.org/patch/10670755/ ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hello On 10/10/18 2:02 PM, Cédric Le Goater wrote: > Hello Boris, > > On 10/10/18 9:32 AM, Boris Brezillon wrote: >> Hi Cédric, >> >> On Wed, 10 Oct 2018 11:46:56 +0530 >> Jagan Teki wrote: >> >>> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater wrote: On 10/4/18 5:57 PM, Jagan Teki wrote: > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: >> >> Hello Simon, >> >> >> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash >> memory, >> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some >> misunderstanding on this driver, it might come from the fact it is closer >> to a SPI-NOR driver like we have in Linux, than a generic SPI driver. >> The stm32 SPI driver is somewhat similar. >> >> Should we move it under drivers/mtd/spi/ maybe ? > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem > instead of mtd/spi-nor. So I think you can handle this via new > spi-mem. have you send any patches to Linux? No, not yet. The patchset is sent : https://patchwork.ozlabs.org/cover/933293/ is not using spimem. I was not aware of that change in the spi-nor layer at the time. I will take a look. >> >> Indeed, if you have some time to convert the Linux aspeed driver to >> the spi-mem interface that would be appreciated. > > Yes. That's the plan. I have a series on the way but I will see if I can > rework a v2 to use spi-mem. I took a look at spi-mem and worked on an initial spi-aspeed-smc driver using the new framework in Linux 5.0.x. It looks good but I have a couple of questions. The first is about performing direct accesses on the AHB window on which the flash contents is mapped. How do you distinguish a flash read (fast, dual, etc) from a RDSFPD command for instance ? Are the drivers expected to check the SPI OP command and depending on the target/command redirect to the appropriate address space ? Also, Aspeed SPI controllers have a Read Timing Compensation Register which defines different data input delay cycles depending on SPI clock rates. This register is supposed to be tuned when the flash chip characteristics are known, after the first bus scan. Is there a way to know that our SPI slave is alive and well detected before starting hammering successive reads on it to see how it behaves. I think the U-Boot and Linux driver will be very similar wrt the issues above ? > Same for the u-boot aspeed spi driver which needs a spi-mem refresh if > I understand correctly. U-Boot, I haven't looked at yet but I expect the driver to be very similar. Thanks, C. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hello Boris, On 10/10/18 9:32 AM, Boris Brezillon wrote: > Hi Cédric, > > On Wed, 10 Oct 2018 11:46:56 +0530 > Jagan Teki wrote: > >> On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater wrote: >>> >>> On 10/4/18 5:57 PM, Jagan Teki wrote: On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: > > Hello Simon, > > > The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash > memory, > and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some > misunderstanding on this driver, it might come from the fact it is closer > to a SPI-NOR driver like we have in Linux, than a generic SPI driver. > The stm32 SPI driver is somewhat similar. > > Should we move it under drivers/mtd/spi/ maybe ? Seems with new spi-mem in Linux flash memory driver rely on spi-mem instead of mtd/spi-nor. So I think you can handle this via new spi-mem. have you send any patches to Linux? >>> >>> No, not yet. The patchset is sent : >>> >>> https://patchwork.ozlabs.org/cover/933293/ >>> >>> is not using spimem. I was not aware of that change in the spi-nor layer >>> at the time. I will take a look. > > Indeed, if you have some time to convert the Linux aspeed driver to > the spi-mem interface that would be appreciated. Yes. That's the plan. I have a series on the way but I will see if I can rework a v2 to use spi-mem. Same for the u-boot aspeed spi driver which needs a spi-mem refresh if I understand correctly. Thanks, C. >> >> Yes, but for newly added drivers. added spi-mem guys, may be they can >> comment. > > Jagan, what's the plan for the spi-nor layer in u-boot? I mean, spi-mem > is just the controller side of things, but it requires spi-mem drivers > to support specific SPI memories. We added the spi-nand driver, but > AFAICT, the spi-nor driver does not exist yet. There's the spi-flash > layer already, but IIUC you were trying to replace it by a spi-nor > framework. > > I see 2 options here: > > 1/ copy the spi-nor framework from linux and adjust it to make it work >in uboot > 2/ create a spi-nor driver which interfaces directly with the spi-mem >layer > > I know I usually recommend going for #1, but it might be a bit > different this time around since I'm trying to get rid of the > spi_nor interface in Linux (the one that allows people to implement > spi-nor controller drivers) in favor of a native spi-mem driver. So > I think it's worth considering option #2. > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hi Cédric, On Wed, 10 Oct 2018 11:46:56 +0530 Jagan Teki wrote: > On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater wrote: > > > > On 10/4/18 5:57 PM, Jagan Teki wrote: > > > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: > > >> > > >> Hello Simon, > > >> > > >> > > >> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash > > >> memory, > > >> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some > > >> misunderstanding on this driver, it might come from the fact it is closer > > >> to a SPI-NOR driver like we have in Linux, than a generic SPI driver. > > >> The stm32 SPI driver is somewhat similar. > > >> > > >> Should we move it under drivers/mtd/spi/ maybe ? > > > > > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem > > > instead of mtd/spi-nor. So I think you can handle this via new > > > spi-mem. have you send any patches to Linux? > > > > No, not yet. The patchset is sent : > > > > https://patchwork.ozlabs.org/cover/933293/ > > > > is not using spimem. I was not aware of that change in the spi-nor layer > > at the time. I will take a look. Indeed, if you have some time to convert the Linux aspeed driver to the spi-mem interface that would be appreciated. > > Yes, but for newly added drivers. added spi-mem guys, may be they can comment. Jagan, what's the plan for the spi-nor layer in u-boot? I mean, spi-mem is just the controller side of things, but it requires spi-mem drivers to support specific SPI memories. We added the spi-nand driver, but AFAICT, the spi-nor driver does not exist yet. There's the spi-flash layer already, but IIUC you were trying to replace it by a spi-nor framework. I see 2 options here: 1/ copy the spi-nor framework from linux and adjust it to make it work in uboot 2/ create a spi-nor driver which interfaces directly with the spi-mem layer I know I usually recommend going for #1, but it might be a bit different this time around since I'm trying to get rid of the spi_nor interface in Linux (the one that allows people to implement spi-nor controller drivers) in favor of a native spi-mem driver. So I think it's worth considering option #2. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
On Mon, Oct 8, 2018 at 11:32 AM Cédric Le Goater wrote: > > On 10/4/18 5:57 PM, Jagan Teki wrote: > > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: > >> > >> Hello Simon, > >> > >> > >> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash > >> memory, > >> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some > >> misunderstanding on this driver, it might come from the fact it is closer > >> to a SPI-NOR driver like we have in Linux, than a generic SPI driver. > >> The stm32 SPI driver is somewhat similar. > >> > >> Should we move it under drivers/mtd/spi/ maybe ? > > > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem > > instead of mtd/spi-nor. So I think you can handle this via new > > spi-mem. have you send any patches to Linux? > > No, not yet. The patchset is sent : > > https://patchwork.ozlabs.org/cover/933293/ > > is not using spimem. I was not aware of that change in the spi-nor layer > at the time. I will take a look. Yes, but for newly added drivers. added spi-mem guys, may be they can comment. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hello Simon,
[...]
+/* CEx Control Register */
+#define CE_CTRL_IO_MODE_MASK GENMASK(30, 28)
+#define CE_CTRL_IO_DUAL_DATA BIT(29)
+#define CE_CTRL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28))
+#define CE_CTRL_CMD_SHIFT 16
+#define CE_CTRL_CMD_MASK 0xff
+#define CE_CTRL_CMD(cmd) \
+ (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT)
+#define CE_CTRL_DUMMY_HIGH_SHIFT 14
+#define CE_CTRL_CLOCK_FREQ_SHIFT 8
+#define CE_CTRL_CLOCK_FREQ_MASK0xf
+#define CE_CTRL_CLOCK_FREQ(div)
\
+ (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT)
>>>
>>> Do you need this, and other macros like it? I suppose you do use them
>>> twice in the code, at least.
>>
>> CE_CTRL_CLOCK_FREQ() is used twice in aspeed_spi_flash_init().
>>
>> Are you suggesting that we should not use such macros ? I agree this one is
>> not very useful.
>
> Yes I think it is better to just spell it out in the code. Defining
> shifts and masks is good but creating macros with them can make the
> code more confusing I think and it is less common to do that in
> U-Boot.
ok.
> BTW the mask is normally a shifted mask:
>
> +#define CE_CTRL_CLOCK_FREQ_SHIFT 8
> +#define CE_CTRL_CLOCK_FREQ_MASK(0xf <<
> CE_CTRL_CLOCK_FREQ_SHIFT)
>
> and you can put it in an anonymous enum if you prefer.
Let's cleanup the defines then.
[...]
+static int aspeed_spi_flash_init(struct aspeed_spi_priv *priv,
+struct aspeed_spi_flash *flash,
+struct udevice *dev)
+{
+ struct spi_flash *spi_flash = dev_get_uclass_priv(dev);
+ struct spi_slave *slave = dev_get_parent_priv(dev);
+ u32 user_hclk;
+ u32 read_hclk;
+
+ /*
+* The SPI flash device slave should not change, so initialize
+* it only once.
+*/
+ if (flash->init)
+ return 0;
>>>
>>> Why does the init happen here?
>>
>> I would rather do it under the probe routine but the flash device is probed
>> later in the sequence. I do agree it's a bit awkard and looks like a work
>> around.
>>
>> We could also do the setting each time the device claims the bus, like in
>> the stm32_qspi driver ?
>
> I think the init option is OK, basd on what you said. But please add a
> comment to the var explaining why it is needed.
ok. will do.
> How come the flash is not available when the driver is probed?
The flash device OF nodes are available, this is how we get the count
of CS, but when controller is probed, the flash device properties are
unknown which makes sense as we need a working SPI controller to probe
the SPI slave flash devices.
> Could you probe whatever else is needed first, so that this IS available?
I would love to but I don't have much control on device_probe() and
spi_get_bus_and_cs() ... :/
> [..]
>>>
>>> This is a SPI driver so it should implement the SPI interface. It
>>> should not be messing with SPI flash things.
>>
>> I would agree but the Aspeed SoC FMC controller and the two SPI controllers
>> are designed for SPI flash memory devices (and also NOR flash memory for the
>> FMC)
>>
>>> I have a hard time understanding why this driver knows about SPI
>>> flash devices?
>>
>> because I made look like a SPI-NOR driver I suppose. Should it be in its
>> own uclass category ?
>>
>
> As above, let's check with Jagan on SPI nor.
>
> With the x86 driver (ich.c) we got around it by trying to make the SPI
> controller look like a normal SPI controller.
Ah. I will take a look.
Thanks,
C.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
On 10/4/18 5:57 PM, Jagan Teki wrote: > On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: >> >> Hello Simon, >> >> >> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory, >> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some >> misunderstanding on this driver, it might come from the fact it is closer >> to a SPI-NOR driver like we have in Linux, than a generic SPI driver. >> The stm32 SPI driver is somewhat similar. >> >> Should we move it under drivers/mtd/spi/ maybe ? > > Seems with new spi-mem in Linux flash memory driver rely on spi-mem > instead of mtd/spi-nor. So I think you can handle this via new > spi-mem. have you send any patches to Linux? No, not yet. The patchset is sent : https://patchwork.ozlabs.org/cover/933293/ is not using spimem. I was not aware of that change in the spi-nor layer at the time. I will take a look. Is there a similar framework in u-boot ? Thanks, C. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
On Fri, Sep 28, 2018 at 5:20 PM Cédric Le Goater wrote: > > Hello Simon, > > > The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory, > and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some > misunderstanding on this driver, it might come from the fact it is closer > to a SPI-NOR driver like we have in Linux, than a generic SPI driver. > The stm32 SPI driver is somewhat similar. > > Should we move it under drivers/mtd/spi/ maybe ? Seems with new spi-mem in Linux flash memory driver rely on spi-mem instead of mtd/spi-nor. So I think you can handle this via new spi-mem. have you send any patches to Linux? ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hi Cedric,
On 28 September 2018 at 04:42, Cédric Le Goater wrote:
> Hello Simon,
>
>
> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory,
> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some
> misunderstanding on this driver, it might come from the fact it is closer
> to a SPI-NOR driver like we have in Linux, than a generic SPI driver.
> The stm32 SPI driver is somewhat similar.
>
> Should we move it under drivers/mtd/spi/ maybe ?
>
> Nevertheless, I think I can improve the dependency on the spi_flash driver
> and probably remove the aspeed_spi_flash struct.
OK, I am not sure of the best thing to do.
Jagan (on cc) has been working on SPI NOR, but I'm not sure of the status.
If you use UCLASS_SPI_FLASH then you can put in drivers/mtd/spi, but
if UCLASS_SPI then drivers/spi
[..]
>>> +/* CEx Control Register */
>>> +#define CE_CTRL_IO_MODE_MASK GENMASK(30, 28)
>>> +#define CE_CTRL_IO_DUAL_DATA BIT(29)
>>> +#define CE_CTRL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28))
>>> +#define CE_CTRL_CMD_SHIFT 16
>>> +#define CE_CTRL_CMD_MASK 0xff
>>> +#define CE_CTRL_CMD(cmd) \
>>> + (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT)
>>> +#define CE_CTRL_DUMMY_HIGH_SHIFT 14
>>> +#define CE_CTRL_CLOCK_FREQ_SHIFT 8
>>> +#define CE_CTRL_CLOCK_FREQ_MASK0xf
>>> +#define CE_CTRL_CLOCK_FREQ(div)
>>> \
>>> + (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT)
>>
>> Do you need this, and other macros like it? I suppose you do use them
>> twice in the code, at least.
>
> CE_CTRL_CLOCK_FREQ() is used twice in aspeed_spi_flash_init().
>
> Are you suggesting that we should not use such macros ? I agree this one is
> not very useful.
Yes I think it is better to just spell it out in the code. Defining
shifts and masks is good but creating macros with them can make the
code more confusing I think and it is less common to do that in
U-Boot.
BTW the mask is normally a shifted mask:
+#define CE_CTRL_CLOCK_FREQ_SHIFT 8
+#define CE_CTRL_CLOCK_FREQ_MASK(0xf <<
CE_CTRL_CLOCK_FREQ_SHIFT)
and you can put it in an anonymous enum if you prefer.
[...]
>
>>> +/* DMA Control/Status Register */
>>> +#define DMA_CTRL_DELAY_SHIFT 8
>>> +#define DMA_CTRL_DELAY_MASK0xf
>>> +#define DMA_CTRL_FREQ_SHIFT4
>>> +#define DMA_CTRL_FREQ_MASK 0xf
>>> +#define TIMING_MASK(div, delay) \
>>> + (((delay & DMA_CTRL_DELAY_MASK) << DMA_CTRL_DELAY_SHIFT) | \
>>> +((div & DMA_CTRL_FREQ_MASK) << DMA_CTRL_FREQ_SHIFT))
>>
>> Just move this code down below?
>
> 'below' as 'closer' to aspeed_spi_fmc_checksum() ?
I mean remove the #define if it is only used once
[...]
>>> +
>>> +struct aspeed_spi_flash {
>>
>> struct comment - what is this for?
>>
>>> + u8 cs;
>>> + boolinit; /* Initialized when the SPI bus is
>>> +* first claimed
>>> +*/
>>
>> Please avoid this type of comment - either put it before the line, or
>> add a struct comment with each member listed.
>
> yes. This will be better.
>
>> Also, can you drop this variable and do the init in the probe() method
>> instead?
>
> I haven't found a good way to do this.
>
> The problem is that the SPI slave flash devices are not available yet when
> the controller is probed. So I am using the claim_bus() method to initialize
> the settings related to each SPI device.
>
> This is what the struct aspeed_spi_flash is about.
>
>>> + void __iomem*ahb_base; /* AHB Window for this device */
>>
>> Should that be a struct *?
>
> no. This is really just the memory address where the flash contents is mapped.
> Depending on the configured controller's mode, accesses will be interpreted
> as direct to the flash contents or as a command/control way to do SPI
> transfers.
>
> But the controller has no registers behind this address.
>
>>> + u32 ahb_size; /* AHB Window segment size */
>>> + u32 ce_ctrl_user; /* CE Control Register for USER
>>> mode */
>>> + u32 ce_ctrl_fread; /* CE Control Register for FREAD
>>> mode */
>>> + u32 iomode;
>>> +
>>> + struct spi_flash *spi; /* Associated SPI Flash device */
>>
>> You should not need this struct here with driver model.
>
> OK. I think I can simplify as I only need the size and the dummy_bytes from
> the spi_flash struct. It felt convenient at the time.
But you should be able to access it from the struct udevice *. Thie
struct spi_flash is available using dev_get_uclass_priv(dev) where dev
is the SPI flash device.
>
>>
>>> +};
>>> +
>>> +struct aspeed_spi_priv {
>>> + struct aspeed_spi_regs
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hello Simon, The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory, and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some misunderstanding on this driver, it might come from the fact it is closer to a SPI-NOR driver like we have in Linux, than a generic SPI driver. The stm32 SPI driver is somewhat similar. Should we move it under drivers/mtd/spi/ maybe ? Nevertheless, I think I can improve the dependency on the spi_flash driver and probably remove the aspeed_spi_flash struct. On 9/27/18 3:41 PM, Simon Glass wrote: > Hi Cedric, > > On 10 September 2018 at 07:16, Cédric Le Goater wrote: >> The Aspeed AST2500 SoC comes with three static memory controllers, all >> with a similar interface : >> >> * Firmware SPI Memory Controller (FMC) >>. BMC firmware >>. 3 chip select pins (CE0 ~ CE2) >>. supports SPI type flash memory (CE0 ~ CE1) >>. CE2 can be of NOR type flash but this is not supported by the >> driver >> >> * SPI Flash Controller (SPI1 and SPI2) >>. host firmware >>. 2 chip select pins (CE0 ~ CE1) >> >> Each controller has a defined AHB window for its registers and another >> AHB window on which all the flash devices are mapped. Each device is >> assigned a memory range through a set of registers called the Segment >> Address Registers. >> >> Devices are controlled using two different modes: the USER command >> mode or the READ/WRITE command mode. When in USER command mode, >> accesses to the AHB window of the SPI flash device are translated into >> SPI command transfers. When in READ/WRITE command mode, the HW >> generates the SPI commands depending on the setting of the CE control >> register. >> >> The following driver supports the FMC and the SPI controllers with the >> attached SPI flash devices. When the controller is probed, the driver >> performs a read timing calibration using specific DMA control >> registers (FMC only). The driver's primary goal is to support the >> first device of the FMC controller on which reside U-Boot but it >> should support the other controllers also. >> >> The Aspeed FMC controller automatically detects at boot time if a >> flash device is in 4BYTE address mode and self configures to use the >> appropriate address width. This can be a problem for the U-Boot SPI >> Flash layer which only supports 3 byte addresses. In such a case, a >> warning is emitted and the address width is fixed when sent on the >> bus. >> >> Signed-off-by: Cédric Le Goater >> --- >> arch/arm/include/asm/arch-aspeed/spi.h | 114 >> drivers/spi/aspeed_spi.c | 788 + >> drivers/spi/Kconfig| 8 + >> drivers/spi/Makefile | 1 + >> 4 files changed, 911 insertions(+) >> create mode 100644 arch/arm/include/asm/arch-aspeed/spi.h >> create mode 100644 drivers/spi/aspeed_spi.c >> >> diff --git a/arch/arm/include/asm/arch-aspeed/spi.h >> b/arch/arm/include/asm/arch-aspeed/spi.h >> new file mode 100644 >> index ..9e952933e1f1 >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-aspeed/spi.h >> @@ -0,0 +1,114 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2018, IBM Corporation. >> + */ >> + >> +#ifndef _ASM_ARCH_ASPEED_SPI_H >> +#define _ASM_ARCH_ASPEED_SPI_H >> + >> +/* CE Type Setting Register */ >> +#define CONF_ENABLE_W2 BIT(18) >> +#define CONF_ENABLE_W1 BIT(17) >> +#define CONF_ENABLE_W0 BIT(16) >> +#define CONF_FLASH_TYPE2 4 >> +#define CONF_FLASH_TYPE1 2 /* Hardwired to SPI */ >> +#define CONF_FLASH_TYPE0 0 /* Hardwired to SPI */ >> +#define CONF_FLASH_TYPE_NOR 0x0 >> +#define CONF_FLASH_TYPE_SPI 0x2 >> + >> +/* CE Control Register */ >> +#define CTRL_EXTENDED2 BIT(2) /* 32 bit addressing for SPI >> */ >> +#define CTRL_EXTENDED1 BIT(1) /* 32 bit addressing for SPI >> */ >> +#define CTRL_EXTENDED0 BIT(0) /* 32 bit addressing for SPI >> */ >> + >> +/* Interrupt Control and Status Register */ >> +#define INTR_CTRL_DMA_STATUS BIT(11) >> +#define INTR_CTRL_CMD_ABORT_STATUS BIT(10) >> +#define INTR_CTRL_WRITE_PROTECT_STATUS BIT(9) >> +#define INTR_CTRL_DMA_EN BIT(3) >> +#define INTR_CTRL_CMD_ABORT_EN BIT(2) >> +#define INTR_CTRL_WRITE_PROTECT_EN BIT(1) >> + >> +/* CEx Control Register */ >> +#define CE_CTRL_IO_MODE_MASK GENMASK(30, 28) >> +#define CE_CTRL_IO_DUAL_DATA BIT(29) >> +#define CE_CTRL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) >> +#define CE_CTRL_CMD_SHIFT 16 >> +#define CE_CTRL_CMD_MASK 0xff >> +#define CE_CTRL_CMD(cmd) \ >> + (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT) >> +#define CE_CTRL_DUMMY_HIGH_SHIFT 14 >> +#define CE_CTRL_CLOCK_FREQ_SHIFT 8 >>
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
Hi Cedric, On 10 September 2018 at 07:16, Cédric Le Goater wrote: > The Aspeed AST2500 SoC comes with three static memory controllers, all > with a similar interface : > > * Firmware SPI Memory Controller (FMC) >. BMC firmware >. 3 chip select pins (CE0 ~ CE2) >. supports SPI type flash memory (CE0 ~ CE1) >. CE2 can be of NOR type flash but this is not supported by the > driver > > * SPI Flash Controller (SPI1 and SPI2) >. host firmware >. 2 chip select pins (CE0 ~ CE1) > > Each controller has a defined AHB window for its registers and another > AHB window on which all the flash devices are mapped. Each device is > assigned a memory range through a set of registers called the Segment > Address Registers. > > Devices are controlled using two different modes: the USER command > mode or the READ/WRITE command mode. When in USER command mode, > accesses to the AHB window of the SPI flash device are translated into > SPI command transfers. When in READ/WRITE command mode, the HW > generates the SPI commands depending on the setting of the CE control > register. > > The following driver supports the FMC and the SPI controllers with the > attached SPI flash devices. When the controller is probed, the driver > performs a read timing calibration using specific DMA control > registers (FMC only). The driver's primary goal is to support the > first device of the FMC controller on which reside U-Boot but it > should support the other controllers also. > > The Aspeed FMC controller automatically detects at boot time if a > flash device is in 4BYTE address mode and self configures to use the > appropriate address width. This can be a problem for the U-Boot SPI > Flash layer which only supports 3 byte addresses. In such a case, a > warning is emitted and the address width is fixed when sent on the > bus. > > Signed-off-by: Cédric Le Goater > --- > arch/arm/include/asm/arch-aspeed/spi.h | 114 > drivers/spi/aspeed_spi.c | 788 + > drivers/spi/Kconfig| 8 + > drivers/spi/Makefile | 1 + > 4 files changed, 911 insertions(+) > create mode 100644 arch/arm/include/asm/arch-aspeed/spi.h > create mode 100644 drivers/spi/aspeed_spi.c > > diff --git a/arch/arm/include/asm/arch-aspeed/spi.h > b/arch/arm/include/asm/arch-aspeed/spi.h > new file mode 100644 > index ..9e952933e1f1 > --- /dev/null > +++ b/arch/arm/include/asm/arch-aspeed/spi.h > @@ -0,0 +1,114 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2018, IBM Corporation. > + */ > + > +#ifndef _ASM_ARCH_ASPEED_SPI_H > +#define _ASM_ARCH_ASPEED_SPI_H > + > +/* CE Type Setting Register */ > +#define CONF_ENABLE_W2 BIT(18) > +#define CONF_ENABLE_W1 BIT(17) > +#define CONF_ENABLE_W0 BIT(16) > +#define CONF_FLASH_TYPE2 4 > +#define CONF_FLASH_TYPE1 2 /* Hardwired to SPI */ > +#define CONF_FLASH_TYPE0 0 /* Hardwired to SPI */ > +#define CONF_FLASH_TYPE_NOR 0x0 > +#define CONF_FLASH_TYPE_SPI 0x2 > + > +/* CE Control Register */ > +#define CTRL_EXTENDED2 BIT(2) /* 32 bit addressing for SPI > */ > +#define CTRL_EXTENDED1 BIT(1) /* 32 bit addressing for SPI > */ > +#define CTRL_EXTENDED0 BIT(0) /* 32 bit addressing for SPI > */ > + > +/* Interrupt Control and Status Register */ > +#define INTR_CTRL_DMA_STATUS BIT(11) > +#define INTR_CTRL_CMD_ABORT_STATUS BIT(10) > +#define INTR_CTRL_WRITE_PROTECT_STATUS BIT(9) > +#define INTR_CTRL_DMA_EN BIT(3) > +#define INTR_CTRL_CMD_ABORT_EN BIT(2) > +#define INTR_CTRL_WRITE_PROTECT_EN BIT(1) > + > +/* CEx Control Register */ > +#define CE_CTRL_IO_MODE_MASK GENMASK(30, 28) > +#define CE_CTRL_IO_DUAL_DATA BIT(29) > +#define CE_CTRL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) > +#define CE_CTRL_CMD_SHIFT 16 > +#define CE_CTRL_CMD_MASK 0xff > +#define CE_CTRL_CMD(cmd) \ > + (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT) > +#define CE_CTRL_DUMMY_HIGH_SHIFT 14 > +#define CE_CTRL_CLOCK_FREQ_SHIFT 8 > +#define CE_CTRL_CLOCK_FREQ_MASK0xf > +#define CE_CTRL_CLOCK_FREQ(div) > \ > + (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT) Do you need this, and other macros like it? I suppose you do use them twice in the code, at least. > +#define CE_CTRL_DUMMY_LOW_SHIFT6 /* 2 bits [7:6] */ > +#define CE_CTRL_DUMMY(dummy)\ > + (dummy) >> 2) & 0x1) << CE_CTRL_DUMMY_HIGH_SHIFT) | \ > +(((dummy) & 0x3) << CE_CTRL_DUMMY_LOW_SHIFT)) I think this needs MASK values instead of open-coding them. > +#define CE_CTRL_STOP_ACTIVE
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
On 09/12/2018 12:38 AM, Joel Stanley wrote:
> On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater wrote:
>>
>> The Aspeed AST2500 SoC comes with three static memory controllers, all
>> with a similar interface :
>>
>> * Firmware SPI Memory Controller (FMC)
>>. BMC firmware
>>. 3 chip select pins (CE0 ~ CE2)
>>. supports SPI type flash memory (CE0 ~ CE1)
>>. CE2 can be of NOR type flash but this is not supported by the
>> driver
>>
>> * SPI Flash Controller (SPI1 and SPI2)
>>. host firmware
>>. 2 chip select pins (CE0 ~ CE1)
>>
>> Each controller has a defined AHB window for its registers and another
>> AHB window on which all the flash devices are mapped. Each device is
>> assigned a memory range through a set of registers called the Segment
>> Address Registers.
>>
>> Devices are controlled using two different modes: the USER command
>> mode or the READ/WRITE command mode. When in USER command mode,
>> accesses to the AHB window of the SPI flash device are translated into
>> SPI command transfers. When in READ/WRITE command mode, the HW
>> generates the SPI commands depending on the setting of the CE control
>> register.
>>
>> The following driver supports the FMC and the SPI controllers with the
>> attached SPI flash devices. When the controller is probed, the driver
>> performs a read timing calibration using specific DMA control
>> registers (FMC only). The driver's primary goal is to support the
>> first device of the FMC controller on which reside U-Boot but it
>> should support the other controllers also.
>>
>> The Aspeed FMC controller automatically detects at boot time if a
>> flash device is in 4BYTE address mode and self configures to use the
>> appropriate address width. This can be a problem for the U-Boot SPI
>> Flash layer which only supports 3 byte addresses. In such a case, a
>> warning is emitted and the address width is fixed when sent on the
>> bus.
>>
>> Signed-off-by: Cédric Le Goater
>
> Looks good. I compared some things against the data sheet, and read
> through the driver. There were to small questions I had, so please
> clear those up and add my:
>
> Reviewed-by: Joel Stanley
>
>> +static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)
>> +{
>> + /* HCLK/1 ..HCLK/16 */
>> + const u8 hclk_divisors[] = {
>> + 15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
>> + };
>> +
>> + u32 i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
>> + if (max_hz >= (hclk_rate / (i + 1)))
>
> We spoke offline about why the values in hclk_divisors are not used in
> this calculation. I think we decided to change the name of
> hclk_divisors to something like hclk_masks?
yes. I have changed the name everywhere as it makes more sense.
>> + break;
>> + }
>> +
>> + debug("hclk=%d required=%d divisor is %d (mask %x) speed=%d\n",
>> + hclk_rate, max_hz, i + 1, hclk_divisors[i], hclk_rate / (i +
>> 1));
>> +
>> + return hclk_divisors[i];
>> +}
>
>> +static u32 aspeed_spi_read_checksum(struct aspeed_spi_priv *priv, u8 div,
>> + u8 delay)
>> +{
>> + /* TODO: the SPI controllers do not have the DMA registers.
>> +* The algorithm is the same.
>> +*/
>> + if (!priv->is_fmc) {
>> + pr_warn("No timing calibration support for SPI controllers");
>> + return 0xbadc0de;
>> + }
>> +
>> + return aspeed_spi_fmc_checksum(priv, div, delay);
>> +}
>> +
>> +static int aspeed_spi_timing_calibration(struct aspeed_spi_priv *priv)
>> +{
>> + /* HCLK/5 .. HCLK/1 */
>> + const u8 hclk_divisors[] = { 13, 6, 14, 7, 15 };
>> +
>> + u32 timing_reg = 0;
>> + u32 checksum, gold_checksum;
>> + int i, j;
>> +
>> + debug("Read timing calibration :\n");
>> +
>> + /* Compute reference checksum at lowest freq HCLK/16 */
>> + gold_checksum = aspeed_spi_read_checksum(priv, 0, 0);
>> +
>> + /*
>> +* Set CE0 Control Register to FAST READ command mode. The
>> +* HCLK divisor will be set through the DMA Control Register.
>> +*/
>> + writel(CE_CTRL_CMD(0xB) | CE_CTRL_DUMMY(1) | CE_CTRL_FREADMODE,
>> + >regs->ce_ctrl[0]);
>> +
>> + /* Increase HCLK freq */
>> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
>> + u32 hdiv = 5 - i;
>> + u32 hshift = (hdiv - 1) << 2;
>> + bool pass = false;
>> + u8 delay;
>> +
>> + if (priv->hclk_rate / hdiv > priv->max_hz) {
>> + debug("skipping freq %ld\n", priv->hclk_rate / hdiv);
>> + continue;
>> + }
>> +
>> + /* Increase HCLK delays until read succeeds */
>> + for (j = 0; j < 6; j++) {
>
> 6 here is the number of items in hclk_divisors?
no. these are counts of HCLK cycles
Re: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers
On Mon, 10 Sep 2018 at 23:48, Cédric Le Goater wrote:
>
> The Aspeed AST2500 SoC comes with three static memory controllers, all
> with a similar interface :
>
> * Firmware SPI Memory Controller (FMC)
>. BMC firmware
>. 3 chip select pins (CE0 ~ CE2)
>. supports SPI type flash memory (CE0 ~ CE1)
>. CE2 can be of NOR type flash but this is not supported by the
> driver
>
> * SPI Flash Controller (SPI1 and SPI2)
>. host firmware
>. 2 chip select pins (CE0 ~ CE1)
>
> Each controller has a defined AHB window for its registers and another
> AHB window on which all the flash devices are mapped. Each device is
> assigned a memory range through a set of registers called the Segment
> Address Registers.
>
> Devices are controlled using two different modes: the USER command
> mode or the READ/WRITE command mode. When in USER command mode,
> accesses to the AHB window of the SPI flash device are translated into
> SPI command transfers. When in READ/WRITE command mode, the HW
> generates the SPI commands depending on the setting of the CE control
> register.
>
> The following driver supports the FMC and the SPI controllers with the
> attached SPI flash devices. When the controller is probed, the driver
> performs a read timing calibration using specific DMA control
> registers (FMC only). The driver's primary goal is to support the
> first device of the FMC controller on which reside U-Boot but it
> should support the other controllers also.
>
> The Aspeed FMC controller automatically detects at boot time if a
> flash device is in 4BYTE address mode and self configures to use the
> appropriate address width. This can be a problem for the U-Boot SPI
> Flash layer which only supports 3 byte addresses. In such a case, a
> warning is emitted and the address width is fixed when sent on the
> bus.
>
> Signed-off-by: Cédric Le Goater
Looks good. I compared some things against the data sheet, and read
through the driver. There were to small questions I had, so please
clear those up and add my:
Reviewed-by: Joel Stanley
> +static u32 aspeed_spi_hclk_divisor(u32 hclk_rate, u32 max_hz)
> +{
> + /* HCLK/1 ..HCLK/16 */
> + const u8 hclk_divisors[] = {
> + 15, 7, 14, 6, 13, 5, 12, 4, 11, 3, 10, 2, 9, 1, 8, 0
> + };
> +
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
> + if (max_hz >= (hclk_rate / (i + 1)))
We spoke offline about why the values in hclk_divisors are not used in
this calculation. I think we decided to change the name of
hclk_divisors to something like hclk_masks?
> + break;
> + }
> +
> + debug("hclk=%d required=%d divisor is %d (mask %x) speed=%d\n",
> + hclk_rate, max_hz, i + 1, hclk_divisors[i], hclk_rate / (i +
> 1));
> +
> + return hclk_divisors[i];
> +}
> +static u32 aspeed_spi_read_checksum(struct aspeed_spi_priv *priv, u8 div,
> + u8 delay)
> +{
> + /* TODO: the SPI controllers do not have the DMA registers.
> +* The algorithm is the same.
> +*/
> + if (!priv->is_fmc) {
> + pr_warn("No timing calibration support for SPI controllers");
> + return 0xbadc0de;
> + }
> +
> + return aspeed_spi_fmc_checksum(priv, div, delay);
> +}
> +
> +static int aspeed_spi_timing_calibration(struct aspeed_spi_priv *priv)
> +{
> + /* HCLK/5 .. HCLK/1 */
> + const u8 hclk_divisors[] = { 13, 6, 14, 7, 15 };
> +
> + u32 timing_reg = 0;
> + u32 checksum, gold_checksum;
> + int i, j;
> +
> + debug("Read timing calibration :\n");
> +
> + /* Compute reference checksum at lowest freq HCLK/16 */
> + gold_checksum = aspeed_spi_read_checksum(priv, 0, 0);
> +
> + /*
> +* Set CE0 Control Register to FAST READ command mode. The
> +* HCLK divisor will be set through the DMA Control Register.
> +*/
> + writel(CE_CTRL_CMD(0xB) | CE_CTRL_DUMMY(1) | CE_CTRL_FREADMODE,
> + >regs->ce_ctrl[0]);
> +
> + /* Increase HCLK freq */
> + for (i = 0; i < ARRAY_SIZE(hclk_divisors); i++) {
> + u32 hdiv = 5 - i;
> + u32 hshift = (hdiv - 1) << 2;
> + bool pass = false;
> + u8 delay;
> +
> + if (priv->hclk_rate / hdiv > priv->max_hz) {
> + debug("skipping freq %ld\n", priv->hclk_rate / hdiv);
> + continue;
> + }
> +
> + /* Increase HCLK delays until read succeeds */
> + for (j = 0; j < 6; j++) {
6 here is the number of items in hclk_divisors?
> + /* Try first with a 4ns DI delay */
> + delay = 0x8 | j;
> + checksum = aspeed_spi_read_checksum(
> + priv, hclk_divisors[i], delay);
> + pass = (checksum ==
