Re: [PATCH v2] mtd/spi-nor: Add SPI memory controllers for Aspeed SoCs

On 12/08/2016 05:36 PM, Cédric Le Goater wrote:

Hello Marek,

Hi!

[...]

quoted

quoted

@@ -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!

quoted

quoted

+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 :-)

[...]

quoted

quoted

+	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.

quoted

quoted

+	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!

[...]

quoted

quoted

+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.

quoted

quoted

+	.we0 = 16,
+	.ctl0 = 0x10,
+	.time = 0x94,
+	.misc = 0x54,
+	.set_4b = aspeed_smc_chip_set_4b,
+};

[...]

quoted

quoted

+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 :)

quoted

quoted

+}
+
+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.

quoted

quoted

+}

[...]

quoted

quoted

+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.

quoted

quoted

+	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;
+}

[...]

quoted

quoted

+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 ?

quoted

quoted

+	 * 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);
+	}

[...]

quoted

quoted

+		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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html