Hi Simon,
On 11 December 2015 at 07:35, Simon Glass <s...@chromium.org> wrote:
> Hi Jagan,
>
> On 8 December 2015 at 08:36, Jagan Teki <jt...@openedev.com> wrote:
>> Hi Simon,
>>
>> On 8 December 2015 at 08:22, Simon Glass <s...@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 3 December 2015 at 03:10, Jagan Teki <jt...@openedev.com> wrote:
>>>> Hi Simon,
>>>>
>>>> I re-phrase all the question from previous thread and continue in this for
>>>> more discussion on spi-nor development.
>>>>
>>>>> Is it intended that SPI flash should be a driver for the MTD uclass?
>>>>> Others would be NAND and CFI. From what I can tell MTD has the same
>>>>> operations as SPI flash (erase, read, write) and adds a lot more.
>>>>>
>>>>> Or is SPI flash really a separate uclass from MTD, with SPI flash
>>>>> being at a higher level?
>>>>
>>>> Based on my "sf: MTD support" series SPI flash is a separate uclass from
>>>> MTD
>>>> and it uses mtd_info operations.
>>>>
>>>>>> cmd_sf
>>>>>> =======
>>>>>> MTD
>>>>>> =======
>>>>>> spi-nor or spi-flash
>>>>>> ===============
>>>>>> "spi-nor to spi drivers" and spi-nor controller driver
>>>>>> =======================================
>>>>> Your diagram above suggests that MTD calls into SPI NOR or SPI flash,
>>>>> but when I look at (for exampe) spi_flash_erase(), it is calling
>>>>> mtd_erase(), suggesting that it is above MTD in the software stack,
>>>>> the opposite of your diagram above.
>>>>>
>>>>
>>>> Will explain this clearly in below description.
>>>>
>>>>> Conceptually this seems problematic.
>>>>>
>>>>> SPI flash is a uclass and supports driver model. It has operations,
>>>>> etc. Your patches remove the operations in favour of calling MTD. But
>>>>> MTD does not support driver model. This is getting really messy.
>>>>>
>>>>
>>>> Will explain this clearly in below description.
>>>>
>>>>
>>>> Introducing SPI-NOR:
>>>> ********************
>>>>
>>>> Some of the spi drivers or spi controllers at drivers/spi/*
>>>> not a real spi controllers, unlike normal spi controllers
>>>> those operates varienty of spi devices among spi-nor flash is
>>>> one of them but instead these were specially designed for
>>>> to operate spi-nor flash devices - these are spi-nor controllers.
>>>> example: drivers/spi/fsl_qspi.c
>>>>
>>>> The problem with these were sitting at drivers/spi is entire
>>>> spi layer becomes spi-nor flash oriented which is absolutely
>>>> wrong indication where spi layer gets effected more with
>>>> flash operations - So this SPI-NOR will resolve this issue
>>>> by separating all spi-nor flash operations from spi layer
>>>> and by creating a generic layer called SPI-NOR core where it
>>>> deals with all SPI-NOR flash activities. The basic idea is
>>>> taken from Linux spi-nor framework.
>>>>
>>>> SPI-NOR Block diagram:
>>>> *********************
>>>>
>>>> =========
>>>> cmd_sf.c
>>>> ===========
>>>> spi_flash.h
>>>> ===========
>>>> mtdcore.c
>>>> ===========
>>>> spi-nor.c
>>>> ========================
>>>> m25p80.c fsl_qspi.c
>>>> ========== ===========
>>>> spi-uclass spi-nor hw
>>>> ========== ===========
>>>> spi_drivers
>>>> ===========
>>>> spi-bus hw
>>>> ==========
>>>>
>>>> Note:
>>>> - spi-nor.c is a common core for m25p80.c which is a spi-nor to spi driver
>>>> interface layer and for fsl_qspi.c which is a typical spi-nor
>>>> controller driver.
>>>> - Challenging task is about probe.
>>>>
>>>> SPI-NOR Development plan:
>>>> *************************
>>>>
>>>> a) Adding MTD core support:
>>>> From command point of view the spi-flash is like a mtd device having
>>>> erase/write/read ops with offset, addr and size, but from lower layers the
>>>> spi-flash becomes either spi-nor or spi-nand so they have their own
>>>> specific
>>>> features like struct spi_nor {}.
>>>>
>>>> This is the reason for calling MTD from command layer and the lower
>>>> layer
>>>> as SPI_NOR uclass.
>>>>
>>>> b) Drop spi_flash uclass:
>>>> Since spi_flash is a generic term for serial flash devices among
>>>> these spi-nor and spi-nand are the real device categories so add
>>>> driver model to these categories.
>>>>
>>>> I sent the series [1] for above a) and b)
>>>
>>> It doesn't look like that series drops the SPI flash uclass.
>>
>> Yes, I have not dropped SPI flash uclass fully on the series only ops.
>>
>>>
>>>>
>>>> c) Add spi-nor support (mean Adding SPI-NOR dm drivers) the next step.
>>>
>>> I think this is what I am missing. If you are moving to SPI NOR, what
>>> is the API? Is it the roughly the same as the current SPI flash API?
>>> (read, write, erase)
>>
>> For now SPI NOR api's are same as SPI flash in terms of structure
>> members but spi-nor hooks, definition and the calling context are
>> different. Usually SPI flash ops are getting called from cmd_sf ==>
>> spi_flash.h ==> sf-uclass.c and then finally goes into sf_ops.c for
>> actual function definitions, but in case of SPI NOR spi-nor ops
>> (defined in spi-nor controller drivers - sf_probe.c, fsl_qspi.c) are
>> getting called from mtd_info ops definitions which are defined at
>> spi-nor core (which is sf_ops.c as of now), and from cmd_sf =>
>> spi_flash.h => mtdcore.c and the finally goes into spi-nor for actual
>> mtd ops definitions.
>>
>> Unlike SPI flash, SPI nor framework will separate the flow from
>> generic spi controller which one of the connected slave as spi nor
>> flash (through sf_probe.c) vs spi-nor controller drivers like
>> fsl_qspi.c
>>
>>>
>>>>
>>>> Detailed SPI-NOR with examples:
>>>> ******************************
>>>>
>>>> include/spi_flash.h
>>>> -------------------
>>>>
>>>> struct spi_flash {
>>>> struct spi_nor *nor;
>>>> };
>>>>
>>>> include/linux/mtd/spi-nor.h
>>>> ---------------------------
>>>>
>>>> struct spi_nor {
>>>> struct udevice *dev;
>>>> struct mtd_info *info;
>>>>
>>>> // spi-nor hooks
>>>> int (*read_reg)(struct udevice *dev, u8 cmd, u8 *val, int len);
>>>> int (*write_reg)(struct udevice *dev, u8 cmd, u8 *data, int len);
>>>>
>>>> int (*read_mmap)(struct udevice *dev, void *data, void *offset,
>>>> size_t len);
>>>> int (*read)(struct udevice *dev, const u8 *opcode, size_t cmd_len,
>>>> void *data, size_t data_len);
>>>> int (*write)(struct udevice *dev, const u8 *cmd, size_t cmd_len,
>>>> const void *data, size_t data_len);
>>>
>>> But here you show SPI nor with its own operations. So does SPI nor
>>> call into MTD? What is your plan to move MTD to driver model and
>>> create a MTD uclass? Also, does this mean that each SPI nor device
>>> will have an MTD sub-device?
>>
>> Yes, SPI NOR is different from MTD and MTD operations which are
>> defined at spi-nor will call the respective spi-nor controller driver
>> spi-nor operations. which I explained above.
>>
>> I think using MTD uclass is another approach where we can remove the
>> udevice from SPI NOR and making all spi-nor controller driver as
>> MTD_UCLASS - do you prefer this?
>
> I think so, so far as I understand it. My initial concern with your
> approach was that you had a driver model uclass making calls into an
> API that was not driver-model-compatible. So creating an MTD uclass
> definitely seems cleaner to me. Also I've found that driver model
> conversion is best done starting from the bottom of the stack.
MTD uclass looks ok to me as well and with this I may drop spi_flash
{} and will use mtd_info since from cmd_sf and below is sample code
snippet, just written for understanding things will clear when patches
submitted. Let me know your inputs on this.
cmd_sf.c
---------
spi_flash_t *flash;
spi_flash.h
-----------
typedef struct mtd_info spi_flash_t;
static inline int spi_flash_read(spi_flash_t *flash, u32 offset,
size_t len, void *buf)
{
// mtd_read
}
static inline int spi_flash_write(spi_flash_t *flash, u32 offset,
size_t len, const void *buf)
{
// mtd_write
}
static inline int spi_flash_erase(spi_flash_t *flash, u32 offset,
size_t len)
{
// mtd_erase
}
static inline int spi_flash_protect(spi_flash_t *flash, u32 ofs, u32 len,
bool prot)
{
// mtd_lock
// mtd_unlock
}
include/linux/mtd/spi-nor.h
---------------------------
struct spi_nor {
struct mtd_info *mtd;
// spi-nor hooks
int (*read_reg)(struct spi_nor *nor, u8 cmd, u8 *val, int len);
int (*write_reg)(struct spi_nor *nor, u8 cmd, u8 *data, int len);
int (*read_mmap)(struct spi_nor *nor, void *data, void *offset,
size_t len);
int (*read)(struct spi_nor *nor, const u8 *opcode, size_t cmd_len,
void *data, size_t data_len);
int (*write)(struct spi_nor *nor, const u8 *cmd, size_t cmd_len,
const void *data, size_t data_len);
};
drivers/mtd/spi-nor/spi-nor.c
-----------------------------
static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
{
// call to spi-nor erase nor->erase()
}
static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
size_t *retlen, u_char *buf)
{
// call to spi-nor erase nor->read()
}
static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
size_t *retlen, const u_char *buf)
{
// call to spi-nor erase nor->erase()
}
int spi_nor_scan(struct spi_nor *nor)
{
struct mtd_info *mtd = nor->mtd;
// read_jedec
// assign mtd hooks
// other spi-nor stuff
}
drivers/mtd/spi-nor/m25p80.c
----------------------------
struct m25p80 {
struct spi_nor nor;
};
static int m25p80_probe(struct udevice *dev)
{
struct spi_slave *spi = dev_get_parentdata(dev);
struct mtd_info *mtd = dev_get_uclass_priv(dev);
struct m25p80 *flash = dev_get_priv(dev);
struct spi_nor *nor;
nor = &flash->nor;
nor->mtd = mtd;
flash->spi = spi;
// spi-nor hooks
nor->read =
nor->write =
nor->read_reg =
nor->write_reg =
spi_nor_scan(nor);
add_mtd_device(mtd);
}
static const struct udevice_id m25p80_ids[] = {
{ .compatible = "jedec,spi-nor" },
{ }
};
U_BOOT_DRIVER(m25p80) = {
.name = "m25p80",
.id = UCLASS_MTD,
.of_match = m25p80_ids,
.probe = m25p80_probe,
.priv_auto_alloc_size = sizeof(struct m25p80),
};
drivers/mtd/spi-nor/fsl-qspi.c
------------------------------
struct fsl_qspi {
struct spi_nor nor;
};
static int fsl_probe(struct udevice *dev)
{
struct mtd_info *mtd = dev_get_uclass_priv(dev);
struct fsl_qspi *q = dev_get_priv(dev);
struct spi_nor *nor;
nor = &q->nor;
nor->mtd = mtd;
// spi-nor hooks
nor->read =
nor->write =
nor->read_reg =
nor->write_reg =
spi_nor_scan(nor);
add_mtd_device(&q->mtd);
}
static const struct udevice_id fsl_qspi_ids[] = {
{ .compatible = "fsl,vf610-qspi" },
{ }
};
U_BOOT_DRIVER(fsl_qspi) = {
.name = "fsl_qspi",
.id = UCLASS_MTD,
.of_match = fsl_qspi_ids,
.probe = fsl_qspi_probe,
.priv_auto_alloc_size = sizeof(struct fsl_qspi),
};
>
>>
>>>
>>>> };
>>>>
>>>> drivers/mtd/spi-nor/spi-nor.c
>>>> -----------------------------
>>>>
>>>> static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>>>> {
>>>> // call to spi-nor erase nor->erase()
>>>> }
>>>>
>>>> static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>> size_t *retlen, u_char *buf)
>>>> {
>>>> // call to spi-nor erase nor->read()
>>>> }
>>>>
>>>> static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>>> size_t *retlen, const u_char *buf)
>>>> {
>>>> // call to spi-nor erase nor->erase()
>>>> }
>>>>
>>>> int spi_nor_scan(struct spi_nor *nor)
>>>> {
>>>> struct mtd_info *mtd = nor->mtd;
>>>>
>>>> // read_jedec
>>>>
>>>> // assign mtd hooks
>>>>
>>>> // other spi-nor stuff
>>>>
>>>> }
>>>>
>>>> drivers/mtd/spi-nor/m25p80.c
>>>> ----------------------------
>>>>
>>>> struct m25p80 {
>>>> struct spi_slave *spi;
>>>> struct mtd_info mtd;
>>>> };
>>>>
>>>> static int m25p80_probe(struct udevice *dev)
>>>> {
>>>> struct spi_slave *spi = dev_get_parentdata(dev);
>>>> struct spi_nor *nor = dev_get_uclass_priv(dev);
>>>> struct m25p80 *flash = dev_get_priv(dev);
>>>> struct spi_flash *sflash;
>>>>
>>>> spi_nor_scan(nor);
>>>>
>>>> add_mtd_device(&flash->mtd);
>>>>
>>>> }
>>>>
>>>> // spi-nor hooks
>>>> static const struct dm_spi_flash_ops m25p80_ops = {
>>>> .read = m25p80_read,
>>>> .write = m25p80_write,
>>>> .erase = m25p80_erase,
>>>> };
>>>>
>>>> static const struct udevice_id m25p80_ids[] = {
>>>> { .compatible = "jedec,spi-nor" },
>>>> { }
>>>> };
>>>>
>>>> U_BOOT_DRIVER(m25p80) = {
>>>> .name = "m25p80",
>>>> .id = UCLASS_SPI_NOR,
>>>> .of_match = m25p80_ids,
>>>> .probe = m25p80_probe,
>>>> .priv_auto_alloc_size = sizeof(struct m25p80),
>>>> .ops = &m25p80_ops,
>>>> };
>>>>
>>>> drivers/mtd/spi-nor/fsl-qspi.c
>>>> ------------------------------
>>>>
>>>> struct fsl_qspi {
>>>> struct mtd_info mtd;
>>>> };
>>>>
>>>> static int fsl_probe(struct udevice *dev)
>>>> {
>>>> struct spi_nor *nor = dev_get_uclass_priv(dev);
>>>> struct fsl_qspi *q = dev_get_priv(dev);
>>>> struct spi_flash *sflash;
>>>>
>>>> spi_nor_scan(nor);
>>>>
>>>> add_mtd_device(&q->mtd);
>>>>
>>>> }
>>>>
>>>> // spi-nor hooks
>>>> static const struct dm_spi_nor_ops fsl_qspi_ops = {
>>>> .read = fsl_qspi_read,
>>>> .write = fsl_qspi_write,
>>>> .erase = fsl_qspi_erase,
>>>> };
>>>>
>>>> static const struct udevice_id fsl_qspi_ids[] = {
>>>> { .compatible = "fsl,vf610-qspi" },
>>>> { }
>>>> };
>>>>
>>>> U_BOOT_DRIVER(fsl_qspi) = {
>>>> .name = "fsl_qspi",
>>>> .id = UCLASS_SPI_NOR,
>>>> .of_match = fsl_qspi_ids,
>>>> .probe = fsl_qspi_probe,
>>>> .priv_auto_alloc_size = sizeof(struct fsl_qspi),
>>>> ops = &fsl_qspi_ops,
>>>> };
>>>>
>>>> About non-dm handling
>>>> *********************
>>>>
>>>> We still maintain the driver similar to m25p80.c for non-dm interface till
>>>> all spi-drivers converted into dm.
>>>>
>>>> [1] https://www.mail-archive.com/u-boot@lists.denx.de/msg193802.html
thanks!
--
Jagan.
Sent with MailTrack
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
