On 12/08/2016 05:36 PM, Cédric Le Goater wrote:
> Hello Marek,
Hi!
[...]
>>> @@ -0,0 +1,72 @@
>>> +* Aspeed Static Memory controller
>>> +* Aspeed SPI Flash Controller
>>> +
>>> +The Static memory controller in the ast2400 supports 5 chip selects
>>> +each can be attached to NAND, parallel NOR, or SPI NOR attached flash.
>>
>> So this controller is supported by this driver, which behaves like a SPI
>> controller driver, yet the block can also do NAND and parallel NOR ?
>
> I think that was answered in a previous email.
Yep, thanks!
>>> +The Firmware Memory Controller in the ast2500 supports 3 chip selects,
>>> +two of which are always in SPI-NOR mode and the third can be SPI-NOR
>>> +or parallel flash. The SPI flash controller in the ast2400 supports
>>> +one of 2 chip selects selected by pinmux. The two SPI flash
>>> +controllers in the ast2500 each support two chip selects.
>>
>> This paragraph is confusing, it's hard to grok down how many different
>> controllers does this driver support and what are their properties from
>> it. It is all there, it's just hard to read.
>
> I will start with the AST2500 controllers only, which are consistent.
That works too :-)
[...]
>>> + 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?
>
> Some left over from the patch we have been keeping for too long (+1year)
> in our tree.
Hehe, I see, so it's fixed now.
>>> + 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).
>
> I agree, I will try to clarify the naming in the next version. I will keep
> the smc suffix for the driver name though.
Thanks!
[...]
>>> +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip);
>>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
>>> +
>>> +static const struct aspeed_smc_info fmc_2400_info = {
>>> + .maxsize = 64 * 1024 * 1024,
>>> + .nce = 5,
>>> + .maxwidth = 4,
>>> + .hastype = true,
>>
>> Shouldn't all this be specified in DT ?
>
> I am not sure, these values are not configurable. I will remove a few
> of them in the next version as they are unused.
Sooo, I had a discussion with Boris (which we didn't finish yet).
Please ignore my comment for now and yes please, drop unused params.
>>> + .we0 = 16,
>>> + .ctl0 = 0x10,
>>> + .time = 0x94,
>>> + .misc = 0x54,
>>> + .set_4b = aspeed_smc_chip_set_4b,
>>> +};
[...]
>>> +static u32 spi_control_fill_opcode(u8 opcode)
>>> +{
>>> + return ((u32)(opcode)) << CONTROL_SPI_COMMAND_SHIFT;
>>
>> return opcode << CONTROL... , fix these horrible casts and parenthesis
>> globally.
>
> I killed the helper.
Nice, thanks for all the cleanups :)
>>> +}
>>> +
>>> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
>>> +{
>>> + return ((u32)1 << (chip->controller->info->we0 + chip->cs));
>>
>> return BIT(...)
>>
>> I'm not sure these microfunctions are even needed.
>
> well, this one is used in a couple of places.
Ah all right, then just return BIT(...) and be done with it.
>>> +}
[...]
>>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + mutex_lock(&chip->controller->mutex);
>>
>> Won't this have a horrid overhead ?
>
> Shall I use the prepare() and unprepare() ops instead ?
I think that'd trim down the amount of locking, yes.
>>> + aspeed_smc_start_user(nor);
>>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1);
>>> + aspeed_smc_read_from_ahb(buf, chip->base, len);
>>> + aspeed_smc_stop_user(nor);
>>> +
>>> + mutex_unlock(&chip->controller->mutex);
>>> +
>>> + return 0;
>>> +}
[...]
>>> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>>> + struct resource *r)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + const struct aspeed_smc_info *info = controller->info;
>>> + u32 reg, base_reg;
>>> +
>>> + /*
>>> + * Always turn on the write enable bit to allow opcodes to be
>>> + * sent in user mode.
>>> + */
>>> + aspeed_smc_chip_enable_write(chip);
>>> +
>>> + /* The driver only supports SPI type flash for the moment */
>>> + if (info->hastype)
>>> + aspeed_smc_chip_set_type(chip, smc_type_spi);
>>> +
>>> + /*
>>> + * Configure chip base address in memory
>>> + */
>>> + chip->base = window_start(controller, r, chip->cs);
>>> + if (!chip->base) {
>>> + dev_warn(chip->nor.dev, "CE segment window closed.\n");
>>> + return -1;
>>> + }
>>> +
>>> + /*
>>> + * Read the existing control register to get basic values.
>>> + *
>>> + * XXX This register probably needs more sanitation.
>>
>> What's this comment about ?
>
> This is an initial comment about settings being done by U-Boot
> before the kernel is loaded, and some optimisations should be
> nice to keep, for the FMC controller. I will rephrase.
Shouldn't that be passed via DT instead ? We want to be bootloader
agnostic in Linux.
btw off-topic, but is U-Boot support for these aspeed devices ever be
upstreamed ?
>>> + * Do we need support for mode 3 vs mode 0 clock phasing?
>>> + */
>>> + reg = readl(chip->ctl);
>>> + dev_dbg(controller->dev, "control register: %08x\n", reg);
>>> +
>>> + base_reg = reg & CONTROL_SPI_KEEP_MASK;
>>> + if (base_reg != reg) {
>>> + dev_info(controller->dev,
>>> + "control register changed to: %08x\n",
>>> + base_reg);
>>> + }
[...]
>>> + err = mtd_device_register(&chip->nor.mtd, NULL, 0);
>>> + if (err)
>>> + continue;
>>
>> What happens if some chip fails to register ?
>
> That's not correctly handled ... I have a fix for it.
>
> Thanks,
Thanks for all the work :)
--
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
