[PATCH openbmc 3/6] linux-obmc: apply the aspeed-spi-nor controller patchset

[Milton Miller II]

-----Jeremy Kerr <jk at ozlabs.org> wrote: -----
> On  01/20/2016 11:41PM, Milton Miller  wrote:
> 
> Hi Milton,
> 
>> Thie applies the apseed-spi-nor driver and patches the dts for
>> palmetto and barreleye when building the linux image.  It also
>> enables the config options for the driver and some file systems to
>> support the overlay filesystem stack used by obmc-phosphor-initfs.
>> 
>> It also applies the rtc month fix patch.
> 
> OK, these should be separate.
> 
> However, I'd prefer we apply the patches directly to our kernel repo.
> Can you post a patch for that, then this change should just be an
> updated kernel reference.

Actually both patches were submitted as separate pull requests to the
openbmc/linux repository.  I submitted them as patching via openbmc 
bitbake recipe to make it easier for others to test; it avoids 
having bitbake transfer a tarball for each new repository and me keeping
a merged branch with all the fixes.  Someday we can teach the git fetcher 
about reference repositories.

> 
> Some minor comments on the kernel patch though:
> 
>> +diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> +new file mode 100644
>> +index 0000000..2497648
>> +--- /dev/null
>> ++++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> +@@ -0,0 +1,539 @@
>> ++/* ASPEED Static Memory Controller driver
>> >++ * Copyright 2015
>> ++ * Milton Miller
>> ++ *
>> ++ */
>
> Make sure you have the right copyright headers on this one.
 
This is a literal post of what I had submitted in 2015.  However I 
admit I am a noob at copyright headers and detailed advice will 
be gladly accepted.

>
>> ++#define DEBUG
> 
> Remove on the final version; we have dynamic debug enabled in the
> OpenBMC kernels.

Sure.

> 
>> ++
>> ++#include <linux/device.h>
>> ++#include <linux/io.h>
>> ++#include <linux/module.h>
>> ++#include <linux/mutex.h>
>> ++#include <linux/mtd/mtd.h>
>> ++#include <linux/mtd/partitions.h>
>> ++#include <linux/mtd/spi-nor.h>
>> ++#include <linux/of.h>
>> ++#include <linux/of_platform.h>
>> ++#include <linux/sysfs.h>
>> ++
>> ++enum smc_controller_type {
>> ++	smc_type_leg,		/* legacy register mode and map */
>> ++	smc_type_fmc,		/* new FMC 5 CEs multiple flash types */
>> ++	smc_type_smc,		/* SPI memory controller for BIOS / host */
>> ++};
>> ++
>> ++enum smc_flash_type {
>> ++	smc_type_nor = 0,	/* controller connected to nor flash */
>> ++	smc_type_nand = 1,	/* controller connected to nand flash */
>> ++	smc_type_spi = 2,	/* controller connected to spi flash */
>> ++};
>> ++
>> ++struct aspeed_smc_info {
>> ++	unsigned long nce : 3;		/* number of chip enables */
>> ++	unsigned long hasdma : 1;	/* has dma engine */
>> ++	unsigned long maxwidth : 3;	/* max width of spi bus */
>> ++	unsigned long we0 : 5;		/* we shift for ce 0 in cfg reg */
>> ++	unsigned long hastype : 1;	/* type shift for ce 0 in cfg reg */
>> ++	u8 ctl0;			/* offset in regs of ctl for ce 0 */
>> ++	u8 cfg;				/* offset in regs of cfg */
>> ++	u8 time;			/* offset in regs of timing */
>> ++};
>
> I wouldn't worry about bitfields here.

I can adapt.

> 
>> ++
>> ++#define E(_ce, _dma, _w, _ctl, _cfg, _we0, _hastype, _time, _misc) \
>> ++	.nce = _ce, .ctl0 = _ctl, .cfg = 0, .we0 = _we0, .hastype = _hastype, \
>> ++	.hasdma = _dma, .maxwidth = _w, .time = _time
>> ++
>> ++struct aspeed_smc_info aspeed_table[] = {
>> ++	[smc_type_fmc] = { E(5, 1, 4, 0x10, 0x00, 16, 1, 0x54, 0x50) },
>> ++	[smc_type_smc] = { E(1, 0, 2, 0x04, 0x00, 0,  0, 0x14, 0x10) },
>> ++};
> 
> And it'll be a little more readable to expand this:
> 
> struct aspeed_smc_info aspeed_table[] = {
> 	[smc_type_fmc] = {
> 		.nce = 5,
> 		.hasdma = 1,
> 		.maxwidth = 0x10,
> 		/* ... etc */
> 	},
> };

I think I'll go a step further, and make them separate structs, there
is not much in keeping them in an array.

> 
>> ++#if 0 /* ATTR */
>> ++static u32 spi_control_to_freq_div(u32 control)
>> ++{
>> ++	u32 sel;
>> ++
>> ++	sel = control & CONTROL_SPI_CLOCK_FREQ_SEL_MASK;
>> ++	sel >>= CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT;
>> ++
>> ++	/* 16, 14, 12, 10, 8, 6, 4, 2, 15, 13, 11, 9, 7, 5, 3, 1 */
>> ++	return 16 - (((sel & 7) << 1) + ((sel & 8) >> 1));
>> ++}
>> ++
>> ++static u32 spi_control_to_dummy_bytes(u32 control)
>> ++{
>> ++
>> ++	return ((control & CONTROL_SPI_IO_DUMMY_CYCLES_LO) >>
>> ++			CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT) |
>> ++		((control & CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT) >>
>> ++			CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT);
>> ++}
>> ++
>> ++static size_t show_ctl_div(u32 control, char *buf)
>> ++{
>> ++	return sprintf(buf, "%u\n", spi_control_to_freq_div(control));
>> ++}
>> ++
>> ++static size_t show_ctl_dummy_bytes(u32 control, char *buf)
>> ++{
>> ++	return sprintf(buf, "%u\n", spi_control_to_dummy_bytes(control));
>> ++}
>> ++#endif /* ATTR */
> 
> There are a couple of `#if 0`-ed blocks in this patch. Was this just for
> development, or something we want to keep?

Actually I wrote them and made them compile except for the unused static 
function warning.  I intended to hook them up at least to debugfs but 
have not yet written that code.  I think they will be useful when developing 
the code to tune the transfer speeds.

So with that background, should I remove it or wait until we have done the 
next phase of tuning?


There are also a couple of functions that use more of the hardware to handle
sending the address, which should reduce the overhead.  I chose to wait to
test those until the base access was known to work.

[snip]

>> ++
>> ++const struct of_device_id aspeed_smc_matches[] = {
>> ++	{ .compatible = "aspeed,fmc", .data = &aspeed_table[smc_type_fmc] },
>> ++	{ .compatible = "aspeed,smc", .data = &aspeed_table[smc_type_smc] },
>> ++	{ }
>> ++};
>> ++MODULE_DEVICE_TABLE(of, aspeed_smc_matches);
> 
> I'd suggest grouping this with the platform_device definition below.

I put it before probe() to allow the  table to be matched to retrieve
the data pointer.  Moving it next to the the platform_driver struct 
would require a forward declaration of either the match table or the 
probe function.

> 
> But in general, all looks good to me. Ben - any thoughts about the
> smc controller implementation?
> 
> Cheers,
> Jeremy

milton