Re: [PATCH RESEND v4 00/37] mtd: st_spi_fsm: Add new driver
Hi Lee,
Sorry for the delay. I have now had a quick look through the patches. Just a
couple of points :-)
* stfsm_probe(): stfsm_fetch_platform_configs() needs to be called *before*
config() -- config() is based on platform capabilities. Conceptually,
stfsm_fetch_platform_configs() should be called before stfsm_jedec_probe(), and
FLASH_FLAG_32BIT_ADDR should be moved out of stfsm_fetch_platform_configs(),
placed just after stfsm_jedec_probe() but before config().
* fsm_wait_busy(): logic not quite correct if we get a P_ERR or E_ERR error
after a timeout. I am also not sure about returning (uint8_t)-EIO. For what
its worth, this is what I did in response to Brian's comment about the race
condition:
static uint8_t fsm_wait_busy(struct stm_spi_fsm *fsm, unsigned int max_time_ms)
{
struct fsm_seq *seq = &fsm_seq_read_status_fifo;
unsigned long deadline;
uint32_t status;
int timeout = 0;
/* Use RDRS1 */
seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
SEQ_OPC_CYCLES(8) |
SEQ_OPC_OPCODE(FLASH_CMD_RDSR));
/* Load read_status sequence */
fsm_load_seq(fsm, seq);
/*
* Repeat until busy bit is deasserted, or timeout, or error (S25FLxxxS)
*/
deadline = jiffies + msecs_to_jiffies(max_time_ms);
while (!timeout) {
cond_resched();
if (time_after_eq(jiffies, deadline))
timeout = 1;
fsm_wait_seq(fsm);
fsm_read_fifo(fsm, &status, 4);
if ((status & FLASH_STATUS_BUSY) == 0)
return 0;
if ((fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS) &&
((status & S25FL_STATUS_P_ERR) ||
(status & S25FL_STATUS_E_ERR)))
return (uint8_t)(status & 0xff);
if (!timeout)
/* Restart */
writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
}
dev_err(fsm->dev, "timeout on wait_busy\n");
return FLASH_STATUS_TIMEOUT;
}
and:
static int fsm_wait_seq(struct stm_spi_fsm *fsm)
{
unsigned long deadline = jiffies +
msecs_to_jiffies(FSM_MAX_WAIT_SEQ_MS);
int timeout = 0;
while (!timeout) {
if (time_after_eq(jiffies, deadline))
timeout = 1;
if (fsm_is_idle(fsm))
return 0;
cond_resched();
}
dev_err(fsm->dev, "timeout on sequence completion\n");
return 1;
}
* "MFD" creeps into a few commit logs ;-)
Cheers,
Angus
On 01/23/2014 10:30 AM, Lee Jones wrote:
> Version 4:
> Tended to Brian's review comments
> - Checkpatch acceptance
> - MODULE_DEVICE_TABLE() name slip correction
> - Timeout issue(s) resolved
> - Potential infinite loop mitigated
> - Code clarity suggests heeded
> - Duplication with MTD core code removed
> - Upgraded to using ROUND_UP() helper
> - Moved non-shared header code into main driver
> - Relocated dynamic msg sequence stores into main struct
> - Averted adaption of static (table) data
> - Basic whitespace/spelling/data type/dev_err suggestions applied
>
> Version 3:
> Okay, this thing should be fully functional now. Identify a chip
> based on it's JEDEC ID, Read, Write, Erase (all or by sector).
> Support for various chip quirks added too.
>
> Version 2:
> The first bunch of these patches have been on the MLs before, but
> didn't receive a great deal of attention for the most part. We are
> a little more featureful this time however. We can now successfully
> setup and configure the N25Q256. We still can't read/write/erase
> it though. I'll start work on that next week and will provide it in
> the next instalment.
>
> Version 1:
> First stab at getting this thing Mainlined. It doesn't do a great deal
> yet, but we are able to initialise the device and dynamically set it up
> correctly based on an extracted JEDEC ID.
>
> Documentation/devicetree/bindings/mtd/st-fsm.txt | 26 ++
> arch/arm/boot/dts/stih416-b2105.dts | 14 +
> arch/arm/boot/dts/stih416-pinctrl.dtsi | 12 +
> drivers/mtd/devices/Kconfig |8 +
> drivers/mtd/devices/Makefile |1 +
> drivers/mtd/devices/serial_flash_cmds.h | 81 ++++
> drivers/mtd/devices/st_spi_fsm.c | 2124
> +
> 7 files changed, 2266 insertions(+)
>
>
--
-
Ang
Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device
Hi Huang Shijie,
On 11/28/2013 03:34 AM, Huang Shijie wrote:
> 于 2013年11月27日 19:52, Lee Jones 写道:
>> However, as we send entire 'message sequences' to the FSM Controller
>> as opposed to merely OPCODEs we would have to extract the OPCODE from
>> flash->command[0] and call our own functions to craft the correct
>> 'message sequence' for the task. For this reason we rejected the idea
>> and went with a stand-alone driver.
>>
> could you send me the datasheet of your spi nor controller?
> I can change my code if it really not good enough.
I will reply to the "mtd: spi-nor" thread regarding the proposed framework, but
a couple of answers to your specific questions below.
>
> we can store the opcode to a field, such as spi_nor_write_op.
>> The framework which Huang is proposing suffers from the same issues.
>> Only providing read(), write(), read_reg() and write_reg() doesn't
>> work for our use-case, as we'd have to decode the flash->command[0] and
>> invoke our own internal routines as before.
>>
>> The only framework with would work for us would consist almost all
>> of the important functions such as; read(), write(), erase(),
>> wait_busy(), read_jedec(), read_status_reg(), write_status_reg(),
>> read_control_reg(), write_control_reg(), etc. However, this approach
>>
> read_jedec() can be replaced by read_reg(0x9f);
>
> read_status() can be replaced by read_reg(0x5);
>
>
>
> write_control_reg() can be replaced by write_reg(xx).
Unfortunately the H/W Controller in question comes with a few restrictions. One
restriction is that data must be read in multiples of 4 bytes. As such, it
would not be able to honour a call of "flash->read_reg(flash, OPCODE_RDID, id,
5);"
Of course, if the H/W driver knows that we are issuing a read_jedec() operation,
then it can make the assumption that over-reading is benign, and we can instead
read 8 bytes of data from the Flash device, and return 5 to the caller.
However, without knowing what operation is being requested, no such assumption
can be made.
> Please correct me if i am wrong.
>
> IMHO, the current four hooks for spi-nor{} can do all the things.
>
> read/write/read_reg/write_reg.
As it stands, the spi-nor framework cannot support the requirements of the
st_spi_fsm controller. I will go into further details on the "mtd: spi-nor"
thread.
Cheers,
Angus
--
-
Angus Clark
ST Microelectronics (R&D) Ltd.
1000 Aztec West, Bristol, BS32 4SQ
email: angus.cl...@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 04/36] mtd: st_spi_fsm: Supply framework for device requests
On 12/10/2013 08:19 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:53PM +, Lee Jones wrote:
>> --- a/drivers/mtd/devices/st_spi_fsm.c
>> +static void stfsm_wait_seq(struct stfsm *fsm)
>> +{
>> +unsigned long timeo = jiffies + HZ;
>> +
>> +while (time_before(jiffies, timeo)) {
>> +if (stfsm_is_idle(fsm))
>> +return;
>> +
>> +cond_resched();
>> +}
>> +
>> +dev_err(fsm->dev, "timeout on sequence completion\n");
>
> I believe the timeout logic is incorrect. What if we wait a "long time"
> to call stfsm_wait_seq() (due to scheduling, or otherwise)? Then the
> while loop might not even run once (time_before(x, y) is false). Or what
> if cond_resched() waits for a long time...
>
> So you need an extra check of stfsm_is_idle() after the while loop,
> before you declare a timeout.
Yes, good catch, this needs to updated.
Cheers,
Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands
On 12/10/2013 09:48 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:57PM +, Lee Jones wrote:
>> JEDEC have helped to standardise a great deal of the commands which
>> can be issued to a Serial Flash devices. Many of the OPCODEs and all
>> of the Serial Flash Discoverable Parameters (SFDP) commands are
>> generic across devices. This patch provides a shared point where
>> these commands can be defined.
This comment isn't entirely correct. The SFDP standard (JESD216A) does not go
as far as standardising the OPCODES, but merely a way of determining at runtime
which operations are supported (e.g. READ_1_1_4, READ_1_4_4), and the associated
opcodes; vendors are still free to use a different opcode for a particular
operation.
>> +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */
>> +#define FLASH_CMD_READ 0x03/* READ */
>> +#define FLASH_CMD_READ_FAST 0x0b/* FAST READ */
>> +#define FLASH_CMD_READ_1_1_20x3b/* DUAL OUTPUT READ */
>> +#define FLASH_CMD_READ_1_2_20xbb/* DUAL I/O READ */
>> +#define FLASH_CMD_READ_1_1_40x6b/* QUAD OUTPUT READ */
>> +#define FLASH_CMD_READ_1_4_40xeb/* QUAD I/O READ */
>
> All of these {1,2,4}_{1,2,4}_{1,2,4} suffixes are a little cryptic; I
> ended up referring to the SFDP spec to figure out what the first number
> meant. Can you document them briefly at the top of this SFDP list?
I think the {x,y,z} descriptions offer the simplest way of representing the
myriad of operations supported by various devices -- it was one thing SFDP did
get right. However, I agree, the nomenclature does need explained. We also
need a way of representing 32-bit address commands, and SDR vs DDR commands --
this has not yet been addresses by SFDP.
> Since you list these, does SFDP even describe the 32-bit addressing
> commands?
Not yet, but I believe "JESD216B" will include a parameter table related to
32-bit address instructions.
> It seems like it assumes the device is switched (statefully)
> between 24-bit and 32-bit address modes (or kept permanently in one or
> the other).
And the complexity increases! The st_spi_fsm H/W also supports a memory-mapped
mode for booting. However, the boot controller is hard-coded to issue 24-bit
address cycles. If the Serial Flash device includes a dedicated REST pin, and
this is appropriately routed to the system reset, then we have no issue -- the
device will reset to its power-on state following a warm reset. If a reset pin
is not available, then certain precautions can be followed to minimise problems
with warm resets. If the Serial Flash device supports a 32-bit instruction
set, then this should be used in preference, and we can avoid any statefull
transitions. Failing that, we can minimise the window in which a reset would
fail by issuing EN4B/EX4B before/after each operation. Of course, if we know
the system is not subject to these constraints, then we just need to issue EN4B
at the start, and stay in 4B mode.
All this knowledge needs to be in the spi-nor layer...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands
On 12/10/2013 10:23 PM, Brian Norris wrote: > On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote: >> It doesn't look to me like the dual/quad bitfields are laid out very >> usefully. Can't they be divided so that their bit position is more >> meaningful? [...] > I realized you are packing these tight because you're using them as > combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So > while my scheme is nicer for representing a single opcode w.r.t. > controller requirements, it is not able to represent the exact opcodes > supported by a particular flash. Hence, this is not the right place for > it. There is certainly scope to compact the opcode representation... > But I don't want to imitate your flags in any generic framework; we need > a method (that doesn't involve too many extra tables like in your > driver; or that supports these tables generically, to share with other > drivers) to map the "supported opcodes" flags to useful properties that > the controller/driver can examine. I believe a key function of any spi-nor framework should be to determine which fundamental modes of operation are supported by the device (e.g. READ_1_1_2, READ_1_2_2, READ_1_1_4, READ_1_4_4 etc) and how they are driven (i.e. opcode, number of mode bits, number of dummy cycles). A second stage would be to configure the best read, write, erase configurations based on the combined capabilities of the device and the H/W controller. However, it is not obvious how best to achieve this functionality. Given the amount of information that needs to be represented, a static table is not going to be popular. The current approach in st_spi_fsm assumes some default configurations for supported modes, with the option to override with device/family-specific configurations. To be honest, it is rather ugly, and the result of historic evolutions rather than a clean design! > Is it possible, for instance, to describe a range of opcodes supported > using patterns? Like "this flash supports opcodes for 1-4 data pins and > 1-2 address pins"? Or am I being too idealistic, and most flash really > support a totally arbitrary combination of opcodes? Yes, I am afraid so. For example, the mx25l12836e supports READ_1_1_2(0x3b) and READ_1_1_4(0x6b), whereas the mx25l12845e supports READ_1_2_2(0xbb) and READ_1_4_4(0xeb). Cheers, Angus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table
On 12/10/2013 10:03 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:58PM +, Lee Jones wrote:
>> --- a/drivers/mtd/devices/st_spi_fsm.h
>> +++ b/drivers/mtd/devices/st_spi_fsm.h
>> @@ -253,4 +253,141 @@ struct stfsm_seq {
>> } __attribute__((__packed__, aligned(4)));
>> #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
>>
>> +/* SPI Flash Device Table */
>> +struct flash_info {
>> +char*name;
>> +/*
>> + * JEDEC id zero means "no ID" (most older chips); otherwise it has
>> + * a high byte of zero plus three data bytes: the manufacturer id,
>> + * then a two byte device id.
>> + */
>> +u32 jedec_id;
>> +u16 ext_id;
>
> Will 5 bytes of ID be enough? I think we're running into a need for 6
> bytes of ID in m25p80.c right about now. Might make sense to start with
> the right number of bytes.
Yes, we will need 6 bytes of ID. The "dirty" stm_spi_fsm driver already handles
arbitrary-length IDs. This will need to be pulled into the "upstream" version
at some point.
>> +int (*config)(struct stfsm *);
>
> Do you *really* need a configuration callback? Can the callback be
> represented as simply a flag for the special configuration behavior that
> is needed, then your driver calls the appropriate "callback" when it
> sees the flag?
>
> BTW, your later patches have to introduce static declarations in this
> header, which seems very ugly to me; you are entwining data with your
> driver's implementation.
The config() callback is used to perform device/vendor specific initialisation
(e.g. how to set the QE bit) and configuration of the read, write, and erase
operations. As such, it is related to the device, not to the driver or H/W
controller.
However, I take your point, it is rather ugly. In some sense, the callback is
used to make up for the lack of information in the table that would otherwise
permit a generic configuration to be made. One option would be to extend the
table structure to include the superset of properties required across all
devices, or perhaps an entry for extended, vendor-specific properties.
In any case, I think this discussion relates more to the spi-nor thread, rather
than the st_spi_fsm driver.
>> +static struct flash_info flash_types[] = {
>> +/*
>> + * ST Microelectronics/Numonyx --
>> + * (newer production versions may have feature updates
>> + * (eg faster operating frequency)
>> + */
>> +#define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST)
>
> Please don't define macros in the middle of your table like this. (You
> have several of these.)
Personally, I think it is easier to read with the flag combinations defined near
where they are used. I do not have strong feeling either way though, and it may
become a moot point if the table structure is changed :-)
Cheers,
Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations
On 12/11/2013 01:06 AM, Brian Norris wrote:
> This process sounds very much like something that is needed for other
> controllers: a way to match controller capabilties with the opcodes
> supported by the flash.
Yes, I think this should be a primary aim of any 'spi-nor' framework".
> But it still isn't quite flexible enough for
> others, because it doesn't try to abstract the selection criteria -- it
> just uses lists that you presorted
Sure, the implementation here is very specific to the st_spi_fsm controller.
However, until there is something better or more generic available, I am not
sure what else to suggest. Selecting a configuration based on a presorted list
is probably a reasonable strategy though; READ_1_4_4 is more efficient that
READ_1_1_4, and READ_1_1_4 is more efficient than READ_1_1_2 etc.
>> +/* Search for preferred configuration based on available flags */
>> +static struct seq_rw_config *
>> +stfsm_search_seq_rw_configs(struct stfsm *fsm,
>> +struct seq_rw_config cfgs[])
>> +{
>> +struct seq_rw_config *config;
>> +int flags = fsm->info->flags;
>> +
>> +for (config = cfgs; cfgs->cmd != 0; config++)
>
> This becomes an infinite loop if you can't find a matching config. You
> probably meant 'config->cmd != 0':
>
I will leave Lee to defend himself on this one ;-)
>> +/* Parameters to configure a READ or WRITE FSM sequence */
>> +struct seq_rw_config {
>> +uint8_t mode_data; /* MODE data */
>
> What does this represent? As far as I can see, all the configurations
> you provide so far have this entry as 0x00.
>
>> +uint8_t mode_cycles;/* No. of MODE cycles */
>
> What are MODE cycles? (Sorry if these are dumb questions.)
Mode data cyles are similar to dummy cycles, except they also transmit some
extended information on how the device should perform the transfer. For
example, Spansion devices use mode data to enter/exit a "continuous read mode"
where subsequent read operations do not need to issue the read command, just
supply an address (and further mode/dummy cycles). In practice, we have only
used this particular mode during testing; our boot-controller dislikes any form
of statefull mode!
>
>> +uint8_t dummy_cycles; /* No. of DUMMY cycles */
>> +};
>
> Several pieces of this sequence struct seems suspiciously similar to
> what would be needed by several other controllers, while the remaining
> pieces don't seem to be so special that they cannot be configured
> beneath a SPI NOR framework. I'm becoming less convinced that your
> controller/driver is as unique as you say it is.
Other controllers/drivers would certainly benefit from something similar.
However, I thought the point was that no such framework exists at present and
any spi-nor proposal is going to take time to mature. In the meantime,
integrating the st_spi_fsm driver would provide another example of H/W that
needs to be supported by a spi-nor framework, and maybe give a few ideas along
the way.
Cheers,
Angus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations
On 12/17/2013 10:46 AM, Lee Jones wrote: >> I will leave Lee to defend himself on this one ;-) > > No defence necessary, I think Brian's correct. > > NB: You might want to take a look at the 'dirty' implementation too. ;) > Yes, just done so :-( Angus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework
Hi Lee and Brian, I would also like to thank Brian; as always, very sensible comments. I am tied up most of today and off work tomorrow (Christmas shopping!), but I will set aside Friday to go through the comments in detail. I should also have some time next week if necessary, subject to any panics that might arise! I also intend to respond to Huang's updated 'spi-nor' framework at the same time. At some stage, I would expect some of the device probing code in st_spi_fsm, particularly the configuration of read/write/erase operations based on capabilities, to be pulled into the 'spi-nor' framework. I do not see this an an obstacle to st_spi_fsm being integrated earlier though; it's presence in the kernel would provide another example of a H/W Controller that 'spi-nor' needs to accommodate. Cheers, Angus On 12/11/2013 08:48 AM, Lee Jones wrote: > Hi Brian, > > Thank you for taking the time to review, it's very much appreciated. > You bought out some interesting points that I'm happy to go away and > rectify. Firstly however, as I inherited this code I'd like to give > Angus a chance to comment before we go off on our own tangent and > rework some of the good code which perhaps should remain unchanged. > > Angus, do you have enough time to go through Brian's review comments > and perhaps reply to the ones that you feel would benefit from your > expert knowledge. To be frank, some of the questions that were asked I > wouldn't be able to answer without your guidance in any case. > > Thanks both. > > Kind regards, > Lee > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
