Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Brian Norris]

+ Huang, Mark, SPI list

There seem to be multiple efforts going on that are vaguely related. I'd
like to see more of the same people appearing on the CC list, to keep
better coordinated.

On that topic, is the SPI dev list relevant, or would anybody working on
the intersection of SPI and MTD be on the MTD list?

On Fri, Nov 22, 2013 at 04:22:37PM +0000, Lee Jones wrote:
> 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.

What happened to integrating a SPI NOR framework? I see you copied some
of the m25p80 commands, but you don't share them with m25p80. I also
understand your argument (from the first version of this patch series)
that much of the actual controller operation is not shared with m25p80,
but you'd be surprised how this might evolve. There are several others
with QSPI hardware that are trying to get patches merged, some of whom
(Huang, at least) are actively submitting patches to help with this
abstraction.

Now, I haven't had a lot of time yet to look at both yours and Huang's
work and see what, if anything, can be shared (will do soon). But I
think you need to be aware.

BTW, can you include version markers (i.e., [PATCH v2 X/Y]) in the
subject from now on?

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Lee Jones]

> There seem to be multiple efforts going on that are vaguely related. I'd
> like to see more of the same people appearing on the CC list, to keep
> better coordinated.
> 
> On that topic, is the SPI dev list relevant, or would anybody working on
> the intersection of SPI and MTD be on the MTD list?

I think Mark is the prominent figure that this would interest.

> On Fri, Nov 22, 2013 at 04:22:37PM +0000, Lee Jones wrote:
> > 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.
> 
> What happened to integrating a SPI NOR framework? I see you copied some
> of the m25p80 commands, but you don't share them with m25p80. I also
> understand your argument (from the first version of this patch series)
> that much of the actual controller operation is not shared with m25p80,
> but you'd be surprised how this might evolve. There are several others
> with QSPI hardware that are trying to get patches merged, some of whom
> (Huang, at least) are actively submitting patches to help with this
> abstraction.
> 
> Now, I haven't had a lot of time yet to look at both yours and Huang's
> work and see what, if anything, can be shared (will do soon). But I
> think you need to be aware.

I've taken a look at Huang's framework and it doesn't provide us with
anything extra to what the m25p80 would provide. We did have the idea
of splitting our driver into two distinct areas; the MTD part, which
we planned on using the m25p80 for and an SPI Controller portion.

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.

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
is mostly pointless. We'd be better of just using the MTD Framework's
_read, _write and _erase call-backs as we do now.

For these reasons and a bunch of others I think the best solution for
our particular use-case would be to stick with our stand-alone driver
implementation.

> BTW, can you include version markers (i.e., [PATCH v2 X/Y]) in the
> subject from now on?

Yes of course, no problem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Huang Shijie]

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

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


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.

thanks
Huang Shijie










______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Angus Clark]

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.clark@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-------------------------------------

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

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

Unfortunately not, it's ST company confidential.

> we can store the opcode to a field, such as spi_nor_write_op.

The OPCODE isn't the issue here.

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

I _fully_ understand your implementation, but it just won't work for
our controller. Or it would, but it would mean writing _more_ code to
bend it into your framework, not less. Let me try to explain in
another way by implementing what you're suggesting. For your JEDEC
example above, we would have to implement [1] which to my mind
completely defeats the purpose.

Most controllers just take an OPCODE and pass it on to the controller
and have done with it. The issue that you're attempting to rectify is
that the m25p80 expects every controller to be an SPI controller
registered to the SPI framework, but as we both know that's not always
practical as the SPI framework doesn't allow all configuration
information to be passed back to the controller driver. Our issue is
not the same. We are required to send entire 'message sequences', to
the controller rather than just opcodes. The JEDEC message sequence
can be seen below. Bear in mind that this is also one of the more
simple message sequences. Some of them even vary depending on which
chip is present.

[1]:

static struct stfsm_seq stfsm_seq_read_jedec = {
	.data_size = TRANSFER_SIZE(8),
	.seq_opc[0] = (SEQ_OPC_PADS_1 |
		       SEQ_OPC_CYCLES(8) |
		       SEQ_OPC_OPCODE(FLASH_CMD_RDID)),
	.seq = {
		STFSM_INST_CMD1,
		STFSM_INST_DATA_READ,
		STFSM_INST_STOP,
	},
	.seq_cfg = (SEQ_CFG_PADS_1 |
		    SEQ_CFG_READNOTWRITE |
		    SEQ_CFG_CSDEASSERT |
		    SEQ_CFG_STARTSEQ),
};

static inline int stfsm_is_idle(struct stfsm *fsm)
{
	return readl(fsm->base + SPI_FAST_SEQ_STA) & 0x10;
}

static inline uint32_t stfsm_fifo_available(struct stfsm *fsm)
{
	return (readl(fsm->base + SPI_FAST_SEQ_STA) >> 5) & 0x7f;
}

static inline void stfsm_load_seq(struct stfsm *fsm,
				  const struct stfsm_seq *seq)
{
	void __iomem *dst = fsm->base + SPI_FAST_SEQ_TRANSFER_SIZE;
	const uint32_t *src = (const uint32_t *)seq;
	int words = STFSM_SEQ_SIZE / sizeof(uint32_t);

	while (words--) {
		writel(*src, dst);
		src++;
		dst += 4;
	}
}

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();
	}
}

static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
			    const uint32_t size)
{
	uint32_t remaining = size >> 2;
	uint32_t avail;
	uint32_t words;

	while (remaining) {
		for (;;) {
			avail = stfsm_fifo_available(fsm);
			if (avail)
				break;
			udelay(1);
		}
		words = min(avail, remaining);
		remaining -= words;

		readsl(fsm->base + SPI_FAST_SEQ_DATA_REG, buf, words);
		buf += words;
	}
}

static void stfsm_read_jedec(struct stfsm *fsm, u8 *jedec)
{
	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
	uint32_t tmp[2];

	stfsm_load_seq(fsm, seq);

	stfsm_read_fifo(fsm, tmp, 8);

	memcpy(jedec, tmp, 5);

	stfsm_wait_seq(fsm);
}

static int stfsm_read_reg(struct spi_nor *flash, u8 opcode, u8 *buf, int len)
{
	struct stfsm *fsm = dev_get_drvdata(flash->mtd->dev.parent);
	
	switch (opcode) {
	case OPCODE_RDID :
		stfsm_read_jedec(fsm, buf);
		break;

	case OPCODE_A :
		stfsm_do_a();
		break;

	/******** SNIP ********/

	case OPCODE_Z :
		stfsm_do_z();
		break;

	case default :
		return -EINVAL;
	}
}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Huang Shijie]

Hi Jones & Augus:

thanks for your explanations.
> Most controllers just take an OPCODE and pass it on to the controller
> and have done with it. The issue that you're attempting to rectify is
> that the m25p80 expects every controller to be an SPI controller
> registered to the SPI framework, but as we both know that's not always
> practical as the SPI framework doesn't allow all configuration
> information to be passed back to the controller driver. Our issue is
> not the same. We are required to send entire 'message sequences', to
> the controller rather than just opcodes. The JEDEC message sequence
> can be seen below. Bear in mind that this is also one of the more
> simple message sequences. Some of them even vary depending on which
> chip is present.
Frankly speaking, my quadspi driver's code is just like Jones's code.
Yes, a big "switch".

The opcode is just like an index to trigger the proper operation.
That's why i add this hook @->read_reg(). (the hook acts as the ioctl)

If we do not use this hooks, we should add more hooks such as
@->read_id, @->read_sr, @->read_cr...

That's make the interface not graceful enough.


I read the your patch implementing the read_id:

http://lists.infradead.org/pipermail/linux-mtd/2013-November/050221.html


it's more readable. But i think Jones's stfsm_read_reg() is workable too.


If you do not like the read_reg() hook, do you have any better idea?



thanks
Huang Shijie




--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/23] mtd: st_spi_fsm: Add new device

[Lee Jones]

> thanks for your explanations.
> >Most controllers just take an OPCODE and pass it on to the controller
> >and have done with it. The issue that you're attempting to rectify is
> >that the m25p80 expects every controller to be an SPI controller
> >registered to the SPI framework, but as we both know that's not always
> >practical as the SPI framework doesn't allow all configuration
> >information to be passed back to the controller driver. Our issue is
> >not the same. We are required to send entire 'message sequences', to
> >the controller rather than just opcodes. The JEDEC message sequence
> >can be seen below. Bear in mind that this is also one of the more
> >simple message sequences. Some of them even vary depending on which
> >chip is present.
> Frankly speaking, my quadspi driver's code is just like Jones's code.
> Yes, a big "switch".

That sounds awful.

It begs the question, what are you saving/improving by using this
framework then?

> The opcode is just like an index to trigger the proper operation.
> That's why i add this hook @->read_reg(). (the hook acts as the ioctl)

As I've said before, the framework you're suggesting is likely to aid
a Controller which shares a great deal of functionality with the
m25p80 where the back-end controller driver doesn't use the SPI
Subsystem. That won't apply to some Serial Flash Controller drivers,
ours included.

The idea of providing a framework like this is to reduce unnecessary
duplication of code by consolidating shared routines into a central
point. mp25p80 is a good place for that, as a great many Controllers
share the majority of its code. As the only commonality we have with
the m25p80 is a small piece of the JEDEC extraction and a sub-section
of the supported device table (which A. our Controller doesn't support
all of the devices and C. we've extended the table to make it more
useful for our use-case); what you're suggesting would not only
create a larger code-base, but execution would also take more cycles,
which to me sounds completely counter-intuitive. It's lose-lose!

Using the m25p80 as a pass-through, then creating an OPCODE based
switch statement to then go and do the _real_ work sound absurd.

> If we do not use this hooks, we should add more hooks such as
> @->read_id, @->read_sr, @->read_cr...
> 
> That's make the interface not graceful enough.

I agree.

> I read the your patch implementing the read_id:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2013-November/050221.html
> 
> it's more readable. But i think Jones's stfsm_read_reg() is workable too.

It's 'workable', but not efficient. I wrote that example to show you
how bad it would look and to show that is was not a good idea, not
as a good example to be followed.

> If you do not like the read_reg() hook, do you have any better idea?

I don't have a better idea. I think your framework will work just fine
for some controllers. I just don't think bending ours to use it (by
basically adding _extra_ code, and subsequently extra cycles) is the
best way to go.

Kind regards,
Lee

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog