From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 2 Oct 2018 04:22:03 -0700 Subject: [U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers In-Reply-To: <09c407b3-be92-6ded-ac87-1e8b79a59761@kaod.org> References: <20180910141655.20944-1-clg@kaod.org> <20180910141655.20944-3-clg@kaod.org> <09c407b3-be92-6ded-ac87-1e8b79a59761@kaod.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de Hi Cedric, On 28 September 2018 at 04:42, C=C3=A9dric Le Goater wrote: > Hello Simon, > > > The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memo= ry, > 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_MASK 0xf >>> +#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_MASK 0xf >>> +#define DMA_CTRL_FREQ_SHIFT 4 >>> +#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; >>> + bool init; /* 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 i= nstead? > > 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 initial= ize > 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 ma= pped. > Depending on the configured controller's mode, accesses will be interpret= ed > as direct to the flash contents or as a command/control way to do SPI tra= nsfers. > > 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 FREA= D 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 fr= om > 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 *regs; >>> + void __iomem *ahb_base; /* AHB Window for all flash dev= ices */ >>> + u32 ahb_size; /* AHB Window segments size */ >>> + >>> + ulong hclk_rate; /* AHB clock rate */ >>> + u32 max_hz; >>> + u8 num_cs; >>> + bool is_fmc; >>> + >>> + struct aspeed_spi_flash flashes[ASPEED_SPI_MAX_CS]; >> >> SPI flash should be modelled as UCLASS_SPI_FLASH devices. Much of the >> code in here looks like it should move to a separate driver. > > This struct aspeed_spi_flash holds all the parameters related to a SPI sl= ave > flash device. The confusion surely comes from the fact the driver looks l= ike > the SPI-NOR driver we have in Linux. Yes, I think I have to defer to Jagan on this one. [..] >>> +static int aspeed_spi_flash_init(struct aspeed_spi_priv *priv, >>> + struct aspeed_spi_flash *flash, >>> + struct udevice *dev) >>> +{ >>> + struct spi_flash *spi_flash =3D dev_get_uclass_priv(dev); >>> + struct spi_slave *slave =3D 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 prob= ed > 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. How come the flash is not available when the driver is probed? Could you probe whatever else is needed first, so that this IS available? [..] >> >> 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 controlle= rs > 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. Regards, Simon