On 11/25/2016 04:01 PM, Cédric Le Goater wrote: > Hello Marek, Hi! > Sorry for the late answer. Here are a couple of answers to the naming > problem > > On 11/25/2016 02:57 PM, Marek Vasut wrote: >> On 11/21/2016 05:45 AM, Joel Stanley wrote: >>> Hello Marek, >> >> Hi! >> >>> Thank you for the review. I have answered a few of your questions; >>> I'll leave the rest to Cedric. >>> >>> On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote: >>>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>>>> index 4a682ee0f632..96148600fdab 100644 >>>>> --- a/drivers/mtd/spi-nor/Kconfig >>>>> +++ b/drivers/mtd/spi-nor/Kconfig >>>>> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >>>>> Flash. Enable this option if you have a device with a SPIFI >>>>> controller and want to access the Flash as a mtd device. >>>>> >>>>> +config ASPEED_FLASH_SPI >>>> >>>> Should be SPI_ASPEED , see the other controllers and keep the list sorted. >>> >>> Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus? >> >> But it's not a driver for SPI-NOR only either, it seems it's a driver >> for multiple distinct devices. > > yes and I think it was a mistake to send the whole at once. We have > added the support in qemu controller by controller and it was easier > to understand. I need to split the patchset in the next version. Cool :-) >>>>> + tristate "Aspeed flash controllers in SPI mode" >>>>> + depends on HAS_IOMEM && OF >>>>> + depends on ARCH_ASPEED || COMPILE_TEST >>>>> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >>>>> + depends on !NEED_MACH_IO_H >>>> >>>> Why? >>>> >>>>> + help >>>>> + This enables support for the New Static Memory Controller >>>>> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >>>>> + to SPI nor chips, and support for the SPI Memory controller >>>>> + (SPI) for the BIOS. >>>> >>>> I think there is a naming chaos between FMC, SMC (as in Static MC) and >>>> SMC (as in SPI MC). >>> >>> Yes, you're spot on. This naming chaos comes form the vendor's documentation. >>> >>> I think we could re-work this sentence to make it clearer. >> >> Please do before someone's head explodes from this :) > > Indeed .. :) I will give a try. Here is the status : > > Aspeed SoC AST2400 has a set of SMC (Static Memory Controller) > controllers in which you find : > > - Legacy Static Memory Controller (called SMC in the spec) > . base address at 0x16000000 > . BMC firmware > . old register set > . supports NOR flash, NAND flash and SPI flash memory. All bootable. > . 1 chip select pin (CE0) > > - New Static Memory Controller (called FMC in the spec) > . base address at 0x16200000 > . BMC firmware > . new register set > . supports NOR flash, NAND flash and SPI flash memory. > . 5 chip select pins (CE0 ∼ CE4) > > - SPI Flash Controller (called SPI in the spec) > . base address at 0x16300000 > . host Firmware > . exotic register set, between old and new ... > . supports SPI flash memory > . 1 chip select pin (CE0) This should be (except for the base address) be in some documentation, it helps. > Aspeed SoC AST2500 defines has a similar set of SMC (Static Memory > Controller) controllers, more in the vein of the AST2400 FMC : > > - Legacy Static Memory Controller is gone, NOR and NAND support also > > - Firmware SPI Memory Controller (called FMC in the spec) > . base address at 0x16200000 > . BMC firmware > . new register set > . supports SPI flash memory. > . 3 chip select pins (CE0 ~ CE2) > > - SPI Flash Controller (called SPI1 in the spec) first > . base address at 0x16300000 > . host firmware > . new register set > . supports SPI flash memory. > . 2 chip select pins (CE0 ~ CE1) > > - SPI Flash Controller (called SPI2 in the spec) second > . base address at 0x16310000 > . host firmware > . new register set > . supports SPI flash memory. > . 2 chip select pins (CE0 ~ CE1) > > > So, these are the reasons behind the naming mess. Added to that the > driver considers the acronym SMC to stand for SPI Memory Controller, > which is wrong. I tried to reduce the confusion with some comments but > that was a failure :) The explanation above is awesome though. > In qemu, we have used FMC (Firmware ...) and SPI to name the controllers > and we just dropped the legacy SMC. I think using the same naming scheme > is a good idea. We don't support anything else than SPI either so we can > drop the other types for the moment. One thing which I still ponder about is how do you support those controllers which support NOR and NAND flash and SPI, do you tap into all subsystems ? >>>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >>>>> + size_t len) >>>>> +{ >>>> >>>> What if start of buf is unaligned ? >>>> >>>>> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { >>>> >>>> Uh, should use boolean OR, not bitwise or. Also, if you're testing >>>> pointer for NULL, do if (!ptr) . >>>> >>>> if (!src || !buf || !len) >>>> return; >>> >>> That's a different test. We're testing here that the buffers are >>> aligned to see if we can do a word-at-a-time copy. >>> >>> If not, it falls through to do a byte-at-a-time copy. I think this >>> covers your first question about buf being unaligned. >> >> Ah, I see, thanks for clarifying. Comment in the code would be helpful >> for why what you're doing is OK. And I think you want to cast to >> uintptr_t instead to make this work on 64bit. > > yes > >>> Cedric, perhaps you could create a macro called IS_ALLIGNED to make it >>> clear what this is doing? >> >> Yup, thanks! > > sure. I still need to go through Marek's comments in the initial email, > I will split the pachset and fix the naming in next version. Thanks! -- Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html