Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> Maxim Levitsky wrote: 
> > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: 
> > > I see two immediate problems with this patch:
> > > 
> > > 1. On cosmetic level, custom debug macros should
> not be employed. Device
> > > core already have this functionality (dynamic
> debug levels and such). Please,
> > > use dev_dbg and friends for print-outs.
> > This allows much easier control for debug.
> > Single module parameter is enough to adjust it.
> > This helps me help users.
> > (Eg, kernel compilation is out of question)

I doubt it will be that useful, but it's not my call to make anyway.


> > 
> > 
> > > 
> > > 2. On a structural level, I'd rather prefer host
> drivers to not start their
> > > own threads. If you look at both current host
> implementations, they operate
> > > in callback fashion. Apart from saving some
> resources, this reduces the
> > > amount of problems encountered during
> suspend/resume and shutdown.
> > This isn't possible.
> > Hardware doesn't support interrupts on memstick bus
> changes, it only
> > supports DMA done from/to internal FIFO, and DMA it
> only possible for
> > 512 byte TPCs.
> > 
> 

How depressing.

> 
> Another question.
> 
> I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT
> Instread mspro_blk.c enables this capability for parallel
> mode, assuming
> that hw supports it. Its true in my case, but might not be
> true in other
> cases.
> I think I should fix that, right?

This is mandated by the spec. INT should be available automatically in
parallel mode, and some hardware does it in serial as well.

> 
> Also I see that you bath
> TPC_READ_LONG_DATA/TPC_READ_LONG_DATA
> Does that mean that every HW sector is larger that 512?
> If so, you are doing copy on write, right?
> I have small caching in my sm_ftl of last sector. It helps
> performance a
> lot.


That's how its called in the spec.
Sectors can be larger than 512b on Pro-HG sticks, and there's additional
TPC_READ/WRITE_QUAD_DATA which operates on larger quantities.

> 
> 
> Also I want to clarify that the only kind of interrupts
> supported by hw
> (besides usual card detection interrupt), is DMA done
> interrupt.
> Thats why I have to use thread.
> Doing polling in r592_submit_req (which runs in atomic
> context is just
> cruel).

Yes, I see you have a timed wait there.


> Besides, under moderate IO load, the IO thread doesn't
> sleep, thus there
> is no overhead of wake/sleep.
> 
> 
> Best regards,
> Maxim Levitsky
> 
>



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Thu, 2010-08-05 at 01:30 -0700, Alex Dubov wrote: 
> > Maxim Levitsky wrote: 
> > > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: 
> > > > I see two immediate problems with this patch:
> > > > 
> > > > 1. On cosmetic level, custom debug macros should
> > not be employed. Device
> > > > core already have this functionality (dynamic
> > debug levels and such). Please,
> > > > use dev_dbg and friends for print-outs.
> > > This allows much easier control for debug.
> > > Single module parameter is enough to adjust it.
> > > This helps me help users.
> > > (Eg, kernel compilation is out of question)
> 
> I doubt it will be that useful, but it's not my call to make anyway.
> 
> 
> > > 
> > > 
> > > > 
> > > > 2. On a structural level, I'd rather prefer host
> > drivers to not start their
> > > > own threads. If you look at both current host
> > implementations, they operate
> > > > in callback fashion. Apart from saving some
> > resources, this reduces the
> > > > amount of problems encountered during
> > suspend/resume and shutdown.
> > > This isn't possible.
> > > Hardware doesn't support interrupts on memstick bus
> > changes, it only
> > > supports DMA done from/to internal FIFO, and DMA it
> > only possible for
> > > 512 byte TPCs.
> > > 
> > 
> 
> How depressing.
> 
> > 
> > Another question.
> > 
> > I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT
> > Instread mspro_blk.c enables this capability for parallel
> > mode, assuming
> > that hw supports it. Its true in my case, but might not be
> > true in other
> > cases.
> > I think I should fix that, right?
> 
> This is mandated by the spec. INT should be available automatically in
> parallel mode, and some hardware does it in serial as well.
> 
> > 
> > Also I see that you bath
> > TPC_READ_LONG_DATA/TPC_READ_LONG_DATA
> > Does that mean that every HW sector is larger that 512?
> > If so, you are doing copy on write, right?
> > I have small caching in my sm_ftl of last sector. It helps
> > performance a
> > lot.
> 
> 
> That's how its called in the spec.
> Sectors can be larger than 512b on Pro-HG sticks, and there's additional
> TPC_READ/WRITE_QUAD_DATA which operates on larger quantities.
But not on ordinary PRO, right?

Small question, can I use Pro-HG stick in my reader that is designed for
Standard/PRO only? Does your subsystem support it?

8-bit mode really isn't supported here, but it is optional I am sure.

> 
> > 
> > 
> > Also I want to clarify that the only kind of interrupts
> > supported by hw
> > (besides usual card detection interrupt), is DMA done
> > interrupt.
> > Thats why I have to use thread.
> > Doing polling in r592_submit_req (which runs in atomic
> > context is just
> > cruel).
> 
> Yes, I see you have a timed wait there.
> 

Alex, how should I proceed in merge of my driver?
Do you have any objections against it now?

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> > 
> > That's how its called in the spec.
> > Sectors can be larger than 512b on Pro-HG sticks, and
> there's additional
> > TPC_READ/WRITE_QUAD_DATA which operates on larger
> quantities.
> But not on ordinary PRO, right?

Pro sectors are normally 512 bytes.

> 
> Small question, can I use Pro-HG stick in my reader that is
> designed for
> Standard/PRO only? Does your subsystem support it?

It should work. It works for me on TI, which has no 8b mode either.

> > 
> 
> Alex, how should I proceed in merge of my driver?
> Do you have any objections against it now?
> 

I may have done a thing or two differently, but otherwise no objection.
I suppose, you should ask Andrew Morton to put it in.




      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Thu, 2010-08-05 at 04:48 -0700, Alex Dubov wrote: 
> > > 
> > > That's how its called in the spec.
> > > Sectors can be larger than 512b on Pro-HG sticks, and
> > there's additional
> > > TPC_READ/WRITE_QUAD_DATA which operates on larger
> > quantities.
> > But not on ordinary PRO, right?
> 
> Pro sectors are normally 512 bytes.
> 
> > 
> > Small question, can I use Pro-HG stick in my reader that is
> > designed for
> > Standard/PRO only? Does your subsystem support it?
> 
> It should work. It works for me on TI, which has no 8b mode either.
In that case, is there an upper limit on sector size?

> 
> > > 
> > 
> > Alex, how should I proceed in merge of my driver?
> > Do you have any objections against it now?
> > 
> 
> I may have done a thing or two differently, but otherwise no objection.
> I suppose, you should ask Andrew Morton to put it in.

Ok, will do.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Thu, 2010-08-05 at 15:30 +0300, Maxim Levitsky wrote: 
> On Thu, 2010-08-05 at 04:48 -0700, Alex Dubov wrote: 
> > > > 
> > > > That's how its called in the spec.
> > > > Sectors can be larger than 512b on Pro-HG sticks, and
> > > there's additional
> > > > TPC_READ/WRITE_QUAD_DATA which operates on larger
> > > quantities.
> > > But not on ordinary PRO, right?
> > 
> > Pro sectors are normally 512 bytes.
> > 
> > > 
> > > Small question, can I use Pro-HG stick in my reader that is
> > > designed for
> > > Standard/PRO only? Does your subsystem support it?
> > 
> > It should work. It works for me on TI, which has no 8b mode either.
> In that case, is there an upper limit on sector size?
Also, you don't use the MS_TPC_READ_QUAD_DATA at all.
So mspro_blk.c won't work with >512 bytes/sector disk?
Or there is some compatibility?


Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> > > > > 
> > > > > That's how its called in the spec.
> > > > > Sectors can be larger than 512b on
> Pro-HG sticks, and
> > > > there's additional
> > > > > TPC_READ/WRITE_QUAD_DATA which operates
> on larger
> > > > quantities.
> > > > But not on ordinary PRO, right?
> > > 
> > > Pro sectors are normally 512 bytes.
> > > 
> > > > 
> > > > Small question, can I use Pro-HG stick in my
> reader that is
> > > > designed for
> > > > Standard/PRO only? Does your subsystem
> support it?
> > > 
> > > It should work. It works for me on TI, which has
> no 8b mode either.
> > In that case, is there an upper limit on sector size?
> Also, you don't use the MS_TPC_READ_QUAD_DATA at all.
> So mspro_blk.c won't work with >512 bytes/sector disk?
> Or there is some compatibility?
> 

The code can work with arbitrarily sized pages, 512b
value is not hard coded. All that is needed is to slightly alter 
h_mspro_block_transfer_data function, given, of course, that adapters
support longer transfer sizes.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Fri, 2010-08-06 at 00:43 -0700, Alex Dubov wrote: 
> > > > > > 
> > > > > > That's how its called in the spec.
> > > > > > Sectors can be larger than 512b on
> > Pro-HG sticks, and
> > > > > there's additional
> > > > > > TPC_READ/WRITE_QUAD_DATA which operates
> > on larger
> > > > > quantities.
> > > > > But not on ordinary PRO, right?
> > > > 
> > > > Pro sectors are normally 512 bytes.
> > > > 
> > > > > 
> > > > > Small question, can I use Pro-HG stick in my
> > reader that is
> > > > > designed for
> > > > > Standard/PRO only? Does your subsystem
> > support it?
> > > > 
> > > > It should work. It works for me on TI, which has
> > no 8b mode either.
> > > In that case, is there an upper limit on sector size?
> > Also, you don't use the MS_TPC_READ_QUAD_DATA at all.
> > So mspro_blk.c won't work with >512 bytes/sector disk?
> > Or there is some compatibility?
> > 
> 
> The code can work with arbitrarily sized pages, 512b
> value is not hard coded. All that is needed is to slightly alter 
> h_mspro_block_transfer_data function, given, of course, that adapters
> support longer transfer 

No, I mean if I go and buy memstick PRO HG, that has > 512 bytes/sector,
will it work with current code?

Btw, there is currently no way of telling core about maximum transfer
length.
Here absolute maximum is 1024 (number of bit that _might_ hold TPC
length), and FIFO size is 512 bytes (maybe its possible to use fifo
twice)

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> > > > > > > 
> > > > > > > That's how its called in the
> spec.
> > > > > > > Sectors can be larger than
> 512b on
> > > Pro-HG sticks, and
> > > > > > there's additional
> > > > > > > TPC_READ/WRITE_QUAD_DATA
> which operates
> > > on larger
> > > > > > quantities.
> > > > > > But not on ordinary PRO, right?
> > > > > 
> > > > > Pro sectors are normally 512 bytes.
> > > > > 
> > > > > > 
> > > > > > Small question, can I use Pro-HG
> stick in my
> > > reader that is
> > > > > > designed for
> > > > > > Standard/PRO only? Does your
> subsystem
> > > support it?
> > > > > 
> > > > > It should work. It works for me on TI,
> which has
> > > no 8b mode either.
> > > > In that case, is there an upper limit on
> sector size?
> > > Also, you don't use the MS_TPC_READ_QUAD_DATA at
> all.
> > > So mspro_blk.c won't work with >512
> bytes/sector disk?
> > > Or there is some compatibility?
> > > 
> > 
> > The code can work with arbitrarily sized pages, 512b
> > value is not hard coded. All that is needed is to
> slightly alter 
> > h_mspro_block_transfer_data function, given, of
> course, that adapters
> > support longer transfer 
> 
> No, I mean if I go and buy memstick PRO HG, that has >
> 512 bytes/sector,
> will it work with current code?
> 
> Btw, there is currently no way of telling core about
> maximum transfer
> length.
> Here absolute maximum is 1024 (number of bit that _might_
> hold TPC
> length), and FIFO size is 512 bytes (maybe its possible to
> use fifo
> twice)
> 

On TI adapters, FIFO can be reused and DMA restarted. Jmicron adapters are
funny beasts, but their team is keen to support MSpro-HG, so at some
point it will be fully possible, though, probably, not with every version
of the chipset.




      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Sat, 2010-08-07 at 06:15 -0700, Alex Dubov wrote: 
> > > > > > > > 
> > > > > > > > That's how its called in the
> > spec.
> > > > > > > > Sectors can be larger than
> > 512b on
> > > > Pro-HG sticks, and
> > > > > > > there's additional
> > > > > > > > TPC_READ/WRITE_QUAD_DATA
> > which operates
> > > > on larger
> > > > > > > quantities.
> > > > > > > But not on ordinary PRO, right?
> > > > > > 
> > > > > > Pro sectors are normally 512 bytes.
> > > > > > 
> > > > > > > 
> > > > > > > Small question, can I use Pro-HG
> > stick in my
> > > > reader that is
> > > > > > > designed for
> > > > > > > Standard/PRO only? Does your
> > subsystem
> > > > support it?
> > > > > > 
> > > > > > It should work. It works for me on TI,
> > which has
> > > > no 8b mode either.
> > > > > In that case, is there an upper limit on
> > sector size?
> > > > Also, you don't use the MS_TPC_READ_QUAD_DATA at
> > all.
> > > > So mspro_blk.c won't work with >512
> > bytes/sector disk?
> > > > Or there is some compatibility?
> > > > 
> > > 
> > > The code can work with arbitrarily sized pages, 512b
> > > value is not hard coded. All that is needed is to
> > slightly alter 
> > > h_mspro_block_transfer_data function, given, of
> > course, that adapters
> > > support longer transfer 
> > 
> > No, I mean if I go and buy memstick PRO HG, that has >
> > 512 bytes/sector,
> > will it work with current code?
> > 
> > Btw, there is currently no way of telling core about
> > maximum transfer
> > length.
> > Here absolute maximum is 1024 (number of bit that _might_
> > hold TPC
> > length), and FIFO size is 512 bytes (maybe its possible to
> > use fifo
> > twice)
> > 
> 
> On TI adapters, FIFO can be reused and DMA restarted. Jmicron adapters are
> funny beasts, but their team is keen to support MSpro-HG, so at some
> point it will be fully possible, though, probably, not with every version
> of the chipset.
> 

I still need an answer for whether HG sticks need MS_TPC_READ_QUAD_DATA
to be used.

But anyway, I buy a HG stick and see how it works.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

--- On Sat, 7/8/10, Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> From: Maxim Levitsky <maximlevitsky@gmail.com>
> Subject: Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.
> To: "Alex Dubov" <oakad@yahoo.com>
> Cc: "LKML" <linux-kernel@vger.kernel.org>
> Received: Saturday, 7 August, 2010, 8:58 AM
> On Sat, 2010-08-07 at 06:15 -0700,
> Alex Dubov wrote: 
> > > > > > > > > 
> > > > > > > > > That's how its
> called in the
> > > spec.
> > > > > > > > > Sectors can be
> larger than
> > > 512b on
> > > > > Pro-HG sticks, and
> > > > > > > > there's additional
> > > > > > > > >
> TPC_READ/WRITE_QUAD_DATA
> > > which operates
> > > > > on larger
> > > > > > > > quantities.
> > > > > > > > But not on ordinary PRO,
> right?
> > > > > > > 
> > > > > > > Pro sectors are normally 512
> bytes.
> > > > > > > 
> > > > > > > > 
> > > > > > > > Small question, can I
> use Pro-HG
> > > stick in my
> > > > > reader that is
> > > > > > > > designed for
> > > > > > > > Standard/PRO only? Does
> your
> > > subsystem
> > > > > support it?
> > > > > > > 
> > > > > > > It should work. It works for
> me on TI,
> > > which has
> > > > > no 8b mode either.
> > > > > > In that case, is there an upper
> limit on
> > > sector size?
> > > > > Also, you don't use the
> MS_TPC_READ_QUAD_DATA at
> > > all.
> > > > > So mspro_blk.c won't work with >512
> > > bytes/sector disk?
> > > > > Or there is some compatibility?
> > > > > 
> > > > 
> > > > The code can work with arbitrarily sized
> pages, 512b
> > > > value is not hard coded. All that is needed
> is to
> > > slightly alter 
> > > > h_mspro_block_transfer_data function, given,
> of
> > > course, that adapters
> > > > support longer transfer 
> > > 
> > > No, I mean if I go and buy memstick PRO HG, that
> has >
> > > 512 bytes/sector,
> > > will it work with current code?
> > > 
> > > Btw, there is currently no way of telling core
> about
> > > maximum transfer
> > > length.
> > > Here absolute maximum is 1024 (number of bit that
> _might_
> > > hold TPC
> > > length), and FIFO size is 512 bytes (maybe its
> possible to
> > > use fifo
> > > twice)
> > > 
> > 
> > On TI adapters, FIFO can be reused and DMA restarted.
> Jmicron adapters are
> > funny beasts, but their team is keen to support
> MSpro-HG, so at some
> > point it will be fully possible, though, probably, not
> with every version
> > of the chipset.
> > 
> 
> I still need an answer for whether HG sticks need
> MS_TPC_READ_QUAD_DATA
> to be used.

No, they work fine with LONG_DATA, at least the ones I tried.

> 
> But anyway, I buy a HG stick and see how it works.

This is always a right thing to do.

> 
> Best regards,
> Maxim Levitsky
> 
> 


      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> > 
> > > 
> > > Small question, can I use Pro-HG stick in my
> reader that is
> > > designed for
> > > Standard/PRO only? Does your subsystem support
> it?
> > 
> > It should work. It works for me on TI, which has no 8b
> mode either.
> In that case, is there an upper limit on sector size?
> 

Good code has no artificially induced limits. Less than machine page.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Thu, 2010-08-05 at 01:30 -0700, Alex Dubov wrote: 
> > Maxim Levitsky wrote: 
> > > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: 
> > > > I see two immediate problems with this patch:
> > > > 
> > > > 1. On cosmetic level, custom debug macros should
> > not be employed. Device
> > > > core already have this functionality (dynamic
> > debug levels and such). Please,
> > > > use dev_dbg and friends for print-outs.
> > > This allows much easier control for debug.
> > > Single module parameter is enough to adjust it.
> > > This helps me help users.
> > > (Eg, kernel compilation is out of question)
> 
> I doubt it will be that useful, but it's not my call to make anyway.
> 
> 
> > > 
> > > 
> > > > 
> > > > 2. On a structural level, I'd rather prefer host
> > drivers to not start their
> > > > own threads. If you look at both current host
> > implementations, they operate
> > > > in callback fashion. Apart from saving some
> > resources, this reduces the
> > > > amount of problems encountered during
> > suspend/resume and shutdown.
> > > This isn't possible.
> > > Hardware doesn't support interrupts on memstick bus
> > changes, it only
> > > supports DMA done from/to internal FIFO, and DMA it
> > only possible for
> > > 512 byte TPCs.
> > > 
> > 
> 
> How depressing.
> 
> > 
> > Another question.
> > 
> > I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT
> > Instread mspro_blk.c enables this capability for parallel
> > mode, assuming
> > that hw supports it. Its true in my case, but might not be
> > true in other
> > cases.
> > I think I should fix that, right?
> 
> This is mandated by the spec. INT should be available automatically in
> parallel mode, and some hardware does it in serial as well.
Thinking again about that I agree with you.
Then I can remove 2 more lines from my driver.

Btw, I want to add a callback from driver to card driver to be able to
reset card in case of error (windows driver does that in case of any
error)

I would use most of the mspro_block_resume for the implementation for
mspro.

Any objections, suggestions?

> 
> > 
> > Also I see that you bath
> > TPC_READ_LONG_DATA/TPC_READ_LONG_DATA
> > Does that mean that every HW sector is larger that 512?
> > If so, you are doing copy on write, right?
> > I have small caching in my sm_ftl of last sector. It helps
> > performance a
> > lot.
> 
> 
> That's how its called in the spec.
> Sectors can be larger than 512b on Pro-HG sticks, and there's additional
> TPC_READ/WRITE_QUAD_DATA which operates on larger quantities.
> 
> > 
> > 
> > Also I want to clarify that the only kind of interrupts
> > supported by hw
> > (besides usual card detection interrupt), is DMA done
> > interrupt.
> > Thats why I have to use thread.
> > Doing polling in r592_submit_req (which runs in atomic
> > context is just
> > cruel).
> 
> Yes, I see you have a timed wait there.
> 
> 
> > Besides, under moderate IO load, the IO thread doesn't
> > sleep, thus there
> > is no overhead of wake/sleep.
> > 
> > 
> > Best regards,
> > Maxim Levitsky
> > 

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> 
> Btw, I want to add a callback from driver to card driver to
> be able to
> reset card in case of error (windows driver does that in
> case of any
> error)
> 
> I would use most of the mspro_block_resume for the
> implementation for
> mspro.
> 
> Any objections, suggestions?
> 

Just toggle a power on it. Power off/power on will do the full reset of
the controller and the media. You don't have to reinitialize it, as you
are sure that it's the same stick.




      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Fri, 2010-08-06 at 00:59 -0700, Alex Dubov wrote: 
> > 
> > Btw, I want to add a callback from driver to card driver to
> > be able to
> > reset card in case of error (windows driver does that in
> > case of any
> > error)
> > 
> > I would use most of the mspro_block_resume for the
> > implementation for
> > mspro.
> > 
> > Any objections, suggestions?
> > 
> 
> Just toggle a power on it. Power off/power on will do the full reset of
> the controller and the media. You don't have to reinitialize it, as you
> are sure that it's the same stick.

Yea, but after such reboot, the device will be in serial mode. So, I
will need to send switch TPC to device. My driver doesn't know how to do
that...., so it would be nice to have callback to the core.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> > > 
> > > Btw, I want to add a callback from driver to card
> driver to
> > > be able to
> > > reset card in case of error (windows driver does
> that in
> > > case of any
> > > error)
> > > 
> > > I would use most of the mspro_block_resume for
> the
> > > implementation for
> > > mspro.
> > > 
> > > Any objections, suggestions?
> > > 
> > 
> > Just toggle a power on it. Power off/power on will do
> the full reset of
> > the controller and the media. You don't have to
> reinitialize it, as you
> > are sure that it's the same stick.
> 
> Yea, but after such reboot, the device will be in serial
> mode. So, I
> will need to send switch TPC to device. My driver doesn't
> know how to do
> that...., so it would be nice to have callback to the
> core.
> 

Lets ask a different question: why do you think this
particular functionality is needed at all? Have you encountered any
problems which require it (I haven't, btw)?

Windows drivers do a lot of things, but it does not mean all of them must
be copied literally.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Sat, 2010-08-07 at 06:12 -0700, Alex Dubov wrote: 
> > > > 
> > > > Btw, I want to add a callback from driver to card
> > driver to
> > > > be able to
> > > > reset card in case of error (windows driver does
> > that in
> > > > case of any
> > > > error)
> > > > 
> > > > I would use most of the mspro_block_resume for
> > the
> > > > implementation for
> > > > mspro.
> > > > 
> > > > Any objections, suggestions?
> > > > 
> > > 
> > > Just toggle a power on it. Power off/power on will do
> > the full reset of
> > > the controller and the media. You don't have to
> > reinitialize it, as you
> > > are sure that it's the same stick.
> > 
> > Yea, but after such reboot, the device will be in serial
> > mode. So, I
> > will need to send switch TPC to device. My driver doesn't
> > know how to do
> > that...., so it would be nice to have callback to the
> > core.
> > 
> 
> Lets ask a different question: why do you think this
> particular functionality is needed at all? Have you encountered any
> problems which require it (I haven't, btw)?

I would image using this to verify that stick is still in place on I/O
error.

If I just power  down/up it, it will loose state, for example IO mode,
reg range, etc...

But anyway, this isn't very important.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

--- On Sat, 7/8/10, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > > > > 
> > > > > Btw, I want to add a callback from
> driver to card
> > > driver to
> > > > > be able to
> > > > > reset card in case of error (windows
> driver does
> > > that in
> > > > > case of any
> > > > > error)
> > > > > 
> > > > > I would use most of the
> mspro_block_resume for
> > > the
> > > > > implementation for
> > > > > mspro.
> > > > > 
> > > > > Any objections, suggestions?
> > > > > 
> > > > 
> > > > Just toggle a power on it. Power off/power
> on will do
> > > the full reset of
> > > > the controller and the media. You don't have
> to
> > > reinitialize it, as you
> > > > are sure that it's the same stick.
> > > 
> > > Yea, but after such reboot, the device will be in
> serial
> > > mode. So, I
> > > will need to send switch TPC to device. My driver
> doesn't
> > > know how to do
> > > that...., so it would be nice to have callback to
> the
> > > core.
> > > 
> > 
> > Lets ask a different question: why do you think this
> > particular functionality is needed at all? Have you
> encountered any
> > problems which require it (I haven't, btw)?
> 
> I would image using this to verify that stick is still in
> place on I/O
> error.
> 
> If I just power  down/up it, it will loose state, for
> example IO mode,
> reg range, etc...
> 
> But anyway, this isn't very important.
> 

My point was, that problems are better solved as they come.
Windows drivers do a lot of things (like SCSI device emulation), but it
doesn't mean all of them are necessary. Adding code "just because" often
means more work later on.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

Hi,

I have few more questions about memsticks.

First of all, I need an explanation of overwrite flag.
You already explained it to me once, but I still not sure about few
things.



#define MEMSTICK_OVERWRITE_UDST  0x10
This one I understand, thinking about xD again, I think it is very
handy.

My idea (from xD of course) is that copyonwrite is done this way:

1. read old sector
2. allocate new sector
2. write what was just read to new sector.
3. erase old sector.

Could you explain when I need to set and reset the
MEMSTICK_OVERWRITE_UDST?


#define MEMSTICK_OVERWRITE_PGST1 0x20
#define MEMSTICK_OVERWRITE_PGST0 0x40
I suppose these indicate that page(sector) contains incorrect data, just
like in xD there is page status?
Again, better explanation is welcome.
Also, should I touch that flag when I update sector?



#define MEMSTICK_OVERWRITE_BKST  0x80
This marks bad blocks?



Another question is about write of oob data.
When I write it, overwrite flag is updated, or I need to use
MEMSTICK_CP_OVERWRITE to update it?
I think former is true.

When I write a sector, I just write 0 to management flag, right?


And last question,
If I use MEMSTICK_CP_BLOCK, can I start reading a block from non-zero
page offset?


And surely last question, what is 'MS_CMD_BLOCK_END'


Thanks again for all help so far,

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> Hi,
> 
> I have few more questions about memsticks.
> 
> First of all, I need an explanation of overwrite flag.
> You already explained it to me once, but I still not sure
> about few
> things.

Overwrite register can be accessed either as part of extra data access
or separately (CP_OVERWRITE access mode).

> 
> #define MEMSTICK_OVERWRITE_UDST  0x10
> This one I understand, thinking about xD again, I think it
> is very
> handy.
> 
> My idea (from xD of course) is that copyonwrite is done
> this way:
> 
> 1. read old sector
> 2. allocate new sector
> 2. write what was just read to new sector.
> 3. erase old sector.

This is correct.

> 
> Could you explain when I need to set and reset the
> MEMSTICK_OVERWRITE_UDST?

UDST flag should be set when you're marking the block for
reallocation during the read/modify/write cycle. You read the existing
physical block, mark it with UDST flag (setting it to zero), then write
different physical block on behalf of the same logical one, then erase the
original block. The UDST flag is supposed to guard against a situation,
whereupon power fails during the write cycle and you're left with two
physical blocks mapped to the same logical one (so the one marked with
zero UDST value is supposedly "known good").


> 
> 
> #define MEMSTICK_OVERWRITE_PGST1 0x20
> #define MEMSTICK_OVERWRITE_PGST0 0x40
> I suppose these indicate that page(sector) contains
> incorrect data, just
> like in xD there is page status?
> Again, better explanation is welcome.
> Also, should I touch that flag when I update sector?
> 
> 
> 
> #define MEMSTICK_OVERWRITE_BKST  0x80
> This marks bad blocks?

BKST set to zero indicates that the whole block is bad and shouldn't be
used.

PGST1:0 has several values:
11:  default, r/w page
10: reserved value, shouldn't be used
01: page is read-only (soft write-protect)
00: page is accessible, but the value is not guaranteed (faulty page that
sort-of works)

That's what the spec says.

> 
> 
> 
> Another question is about write of oob data.
> When I write it, overwrite flag is updated, or I need to
> use
> MEMSTICK_CP_OVERWRITE to update it?
> I think former is true.

As I mentioned above, it can be accessed either as part of extra data
or separately.

> 
> When I write a sector, I just write 0 to management flag,
> right?

You shouldn't touch management_flag at all, as far as I can tell.
It's only used to indicate special purpose blocks, such as factory
written boot blocks, volatile look-up table blocks (for systems with
tight RAM requirements) and DRM marked blocks which I has no info about.

> 
> 
> And last question,
> If I use MEMSTICK_CP_BLOCK, can I start reading a block
> from non-zero
> page offset?

Yes, it starts from the user specified page address and auto increments it
until the current block end is hit.

> 
> 
> And surely last question, what is 'MS_CMD_BLOCK_END'

This command is used to terminate the currently ongoing block operation.
If you are using one of the auto-increment modes (with CP_BLOCK set) but
do not want to access all the pages until the block end, you must issue
this command after the desired number of pages is transferred to return
the media's state machine to the initial state. This command never hurts,
as you can guess.

> 
> 
> Thanks again for all help so far,
> 

You're welcome.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Sun, 2010-08-08 at 07:26 -0700, Alex Dubov wrote: 
> > Hi,
> > 
> > I have few more questions about memsticks.
> > 
> > First of all, I need an explanation of overwrite flag.
> > You already explained it to me once, but I still not sure
> > about few
> > things.
> 
> Overwrite register can be accessed either as part of extra data access
> or separately (CP_OVERWRITE access mode).
> 
> > 
> > #define MEMSTICK_OVERWRITE_UDST  0x10
> > This one I understand, thinking about xD again, I think it
> > is very
> > handy.
> > 
> > My idea (from xD of course) is that copyonwrite is done
> > this way:
> > 
> > 1. read old sector
> > 2. allocate new sector
> > 2. write what was just read to new sector.
> > 3. erase old sector.
> 
> This is correct.
> 
> > 
> > Could you explain when I need to set and reset the
> > MEMSTICK_OVERWRITE_UDST?
> 
> UDST flag should be set when you're marking the block for
> reallocation during the read/modify/write cycle. You read the existing
> physical block, mark it with UDST flag (setting it to zero), then write
> different physical block on behalf of the same logical one, then erase the
> original block. The UDST flag is supposed to guard against a situation,
> whereupon power fails during the write cycle and you're left with two
> physical blocks mapped to the same logical one (so the one marked with
> zero UDST value is supposedly "known good").

> 
> 
> > 
> > 
> > #define MEMSTICK_OVERWRITE_PGST1 0x20
> > #define MEMSTICK_OVERWRITE_PGST0 0x40
> > I suppose these indicate that page(sector) contains
> > incorrect data, just
> > like in xD there is page status?
> > Again, better explanation is welcome.
> > Also, should I touch that flag when I update sector?
> > 
> > 
> > 
> > #define MEMSTICK_OVERWRITE_BKST  0x80
> > This marks bad blocks?
> 
> BKST set to zero indicates that the whole block is bad and shouldn't be
> used.
> 
> PGST1:0 has several values:
> 11:  default, r/w page
> 10: reserved value, shouldn't be used
> 01: page is read-only (soft write-protect)
> 00: page is accessible, but the value is not guaranteed (faulty page that
> sort-of works)
> 
> That's what the spec says.

Thank you very much. 
> 
> > 
> > 
> > 
> > Another question is about write of oob data.
> > When I write it, overwrite flag is updated, or I need to
> > use
> > MEMSTICK_CP_OVERWRITE to update it?
> > I think former is true.
> 
> As I mentioned above, it can be accessed either as part of extra data
> or separately.
> 
> > 
> > When I write a sector, I just write 0 to management flag,
> > right?
> 
> You shouldn't touch management_flag at all, as far as I can tell.
> It's only used to indicate special purpose blocks, such as factory
> written boot blocks, volatile look-up table blocks (for systems with
> tight RAM requirements) and DRM marked blocks which I has no info about.
> 
> > 
> > 
> > And last question,
> > If I use MEMSTICK_CP_BLOCK, can I start reading a block
> > from non-zero
> > page offset?
> 
> Yes, it starts from the user specified page address and auto increments it
> until the current block end is hit.
> 
> > 
> > 
> > And surely last question, what is 'MS_CMD_BLOCK_END'
> 
> This command is used to terminate the currently ongoing block operation.
> If you are using one of the auto-increment modes (with CP_BLOCK set) but
> do not want to access all the pages until the block end, you must issue
> this command after the desired number of pages is transferred to return
> the media's state machine to the initial state. This command never hurts,
> as you can guess.
That what I expected, thanks! 
> 
> > 
> > 
> > Thanks again for all help so far,
> > 
> 
> You're welcome.

Thank you very much!

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Sun, 2010-08-08 at 18:07 +0300, Maxim Levitsky wrote: 
> On Sun, 2010-08-08 at 07:26 -0700, Alex Dubov wrote: 
> > > Hi,
> > > 
> > > I have few more questions about memsticks.
> > > 
> > > First of all, I need an explanation of overwrite flag.
> > > You already explained it to me once, but I still not sure
> > > about few
> > > things.
> > 
> > Overwrite register can be accessed either as part of extra data access
> > or separately (CP_OVERWRITE access mode).
> > 
Few more questions I gathered today.

I currently assume that if I set req->need_card_int, the driver will
wait till device raises MEMSTICK_INT_CED regardless of serial/parallel
mode. This bit is always available on serial line.
Is that OK to assume?


Another thing that I want to ask is about writes to overwrite/managment
flag.
Common sense tells me that I can write the flash many times, but write
can only clear bits. Therefore if I write 0xFF, this just means do
nothing.
Probably same applies to data content, but that isn't much of the use.
Thats why I see that default (good) value of bits in overwrite flag is
1.
This is correct I assume?


Another interesting thing I observed was that MEMSTICK_INT_ERR can mean
a correctable error,  therefore it shouldn't alone  reject read/write of
a sector. (I think that it means that one of
MEMSTICK_STATUS1_UCFG..MEMSTICK_STATUS1_DTER is set)
Same question about this.


Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> 
> I currently assume that if I set req->need_card_int, the
> driver will
> wait till device raises MEMSTICK_INT_CED regardless of
> serial/parallel
> mode. This bit is always available on serial line.
> Is that OK to assume?

I'm not quite sure about this question.
Normally, when you wait for the media interrupt, you want to see the whole
value of the INT register (CED by itself doesn't indicate successful
command completion; in fact it's value is undefined in presence of other
INT flags, like BREQ or CMDNK). In parallel mode, host is required to
fetch all meaningful INT bits from the media bus, while in serial mode
this is only possible, if host supports automatic INT retrieval (the host
will issue GET_INT tpc behind the scenes before alerting the software).

> 
> Another thing that I want to ask is about writes to
> overwrite/managment
> flag.
> Common sense tells me that I can write the flash many
> times, but write
> can only clear bits. Therefore if I write 0xFF, this just
> means do
> nothing.
> Probably same applies to data content, but that isn't much
> of the use.

Yes, all memsticks are NAND flash, so writing 1s has no effect whatsoever.

> Thats why I see that default (good) value of bits in
> overwrite flag is
> 1.
> This is correct I assume?

Yes, a direct consequence of the above.

> 
> 
> Another interesting thing I observed was that
> MEMSTICK_INT_ERR can mean
> a correctable error,  therefore it shouldn't
> alone  reject read/write of
> a sector. (I think that it means that one of
> MEMSTICK_STATUS1_UCFG..MEMSTICK_STATUS1_DTER is set)
> Same question about this.
> 

There are three groups of error flags. While overwrite_flag register is
accessible as part of extra data, being the indicator of block goodness
it earned its own error flag:

DTER (UCDT) - error (uncorrectable) in data area
EXER (UCEX) - error (uncorrectable) in extra data area
FGER (UCFG) - error (uncorrectable) in overwrite_flag register

Uncorrectable error means that you've got some bit errors in the data
you've obtained from the media. If uncorrectable flag is not set, it
means that media's ECC circuit managed to correct the bit flip. The
desired way of action is, of course, to reallocate the block before it
develops an uncorrectable failure.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Sun, 2010-08-08 at 23:31 -0700, Alex Dubov wrote: 
> > 
> > I currently assume that if I set req->need_card_int, the
> > driver will
> > wait till device raises MEMSTICK_INT_CED regardless of
> > serial/parallel
> > mode. This bit is always available on serial line.
> > Is that OK to assume?
> 
> I'm not quite sure about this question.
> Normally, when you wait for the media interrupt, you want to see the whole
> value of the INT register (CED by itself doesn't indicate successful
> command completion; in fact it's value is undefined in presence of other
> INT flags, like BREQ or CMDNK). In parallel mode, host is required to
> fetch all meaningful INT bits from the media bus, while in serial mode
> this is only possible, if host supports automatic INT retrieval (the host
> will issue GET_INT tpc behind the scenes before alerting the software).
Ok, maybe I didn't explain myself correctly.

Device is in serial mode.
I set need_card_int
I send a command.
<here I assume that driver waits for CED bit, which is exposed on data
line, if CED won't be set, driver/hardware will timeout>
I send GET_INT _once_, look at flags. If I see CED, no NACK, then ok,
otherwise I send error result.



> 
> > 
> > Another thing that I want to ask is about writes to
> > overwrite/managment
> > flag.
> > Common sense tells me that I can write the flash many
> > times, but write
> > can only clear bits. Therefore if I write 0xFF, this just
> > means do
> > nothing.
> > Probably same applies to data content, but that isn't much
> > of the use.
> 
> Yes, all memsticks are NAND flash, so writing 1s has no effect whatsoever.
> 
> > Thats why I see that default (good) value of bits in
> > overwrite flag is
> > 1.
> > This is correct I assume?
> 
> Yes, a direct consequence of the above.

I suspect that managment flag also has default value of 0xFF, and these
'special' features (drm, boot block, temporary table) clear bits out of
it.

> 
> > 
> > 
> > Another interesting thing I observed was that
> > MEMSTICK_INT_ERR can mean
> > a correctable error,  therefore it shouldn't
> > alone  reject read/write of
> > a sector. (I think that it means that one of
> > MEMSTICK_STATUS1_UCFG..MEMSTICK_STATUS1_DTER is set)
> > Same question about this.
> > 
> 
> There are three groups of error flags. While overwrite_flag register is
> accessible as part of extra data, being the indicator of block goodness
> it earned its own error flag:
> 
> DTER (UCDT) - error (uncorrectable) in data area
> EXER (UCEX) - error (uncorrectable) in extra data area
> FGER (UCFG) - error (uncorrectable) in overwrite_flag register
> 
> Uncorrectable error means that you've got some bit errors in the data
> you've obtained from the media. If uncorrectable flag is not set, it
> means that media's ECC circuit managed to correct the bit flip. The
> desired way of action is, of course, to reallocate the block before it
> develops an uncorrectable failure.
That I understand clearly.
I ask what the meaning of MEMSTICK_INT_ERR is.
I expect it to be set if any of (un) correctable errors are found.
Therefore if MEMSTICK_INT_ERR, I can't report error instantly, but I
need to look at status1 to see if error is correctable or not.
This is correct?


Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

I have another question.

Looking at ms_block.c, I see that it sometimes changes register window.
This doesn't look good.
I see it does put the register window back, but still its a bit obscure.

I added tracking of current register window, so every time I send
MS_TPC_SET_RW_REG_ADRS I note the ranges.
And read/write functions now always attempt to send
MS_TPC_SET_RW_REG_ADRS. If the window is same as was  set last time, TPC
is skipped.

However, I am thinking, that maybe I should always write both param and
extra register? I just write 0xFF to extra register and thats all.
Windows driver does that partially. It writes 0xFF to managmemt and
0xF8 to overwrite flag (why???), but doesn't touch the LBA. I don't
think that matters.
It also always sends the MS_TPC_SET_RW_REG_ADRS, which I don't like.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> Received: Monday, 9 August, 2010, 8:30 AM
> I have another question.
> 
> Looking at ms_block.c, I see that it sometimes changes
> register window.
> This doesn't look good.
> I see it does put the register window back, but still its a
> bit obscure.

It looks very good, in fact, it is the Sony specified way to operate
the media. MS Pro works quite the same, it just needs fewer operations
to actually access data.

> 
> I added tracking of current register window, so every time
> I send
> MS_TPC_SET_RW_REG_ADRS I note the ranges.
> And read/write functions now always attempt to send
> MS_TPC_SET_RW_REG_ADRS. If the window is same as was 
> set last time, TPC
> is skipped.

Sure it is. The media will remember the window set.
Media has all its registers in a sort of flat file. SET_RW_REG_ADDR
selects the subset of the registers that will receive the data delivered
within TPC. This subset is remembered until power off or until changed.


> 
> However, I am thinking, that maybe I should always write
> both param and
> extra register? I just write 0xFF to extra register and
> thats all.

You should write into a param register when you want to alter the command
parameters. You cannot do so during auto incrementing block access, for
example.

But, if you're using the auto incrementing write, you will have to write
extra register for every page transferred.

That's where changing RW_REG_ADDR comes handy.

> Windows driver does that partially. It writes 0xFF to
> managmemt and
> 0xF8 to overwrite flag (why???)

It's a factory default.
Try to read it from some empty block. :-)
(My theory is that missing bits contain invisible ECC data).


> I don't
> think that matters.
> It also always sends the MS_TPC_SET_RW_REG_ADRS, which I
> don't like.
> 

This only reduces the performance slightly. SET_RW_REG_ADDR does not
influence the media's state machine as far as I can tell, unless you try
to push it during the data transfer cycle (whereupon you will end up
having a literal value of the tpc in the media data buffer).



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Tue, 2010-08-10 at 01:12 -0700, Alex Dubov wrote: 
> > Received: Monday, 9 August, 2010, 8:30 AM
> > I have another question.
> > 
> > Looking at ms_block.c, I see that it sometimes changes
> > register window.
> > This doesn't look good.
> > I see it does put the register window back, but still its a
> > bit obscure.
> 
> It looks very good, in fact, it is the Sony specified way to operate
> the media. MS Pro works quite the same, it just needs fewer operations
> to actually access data.
> 
> > 
> > I added tracking of current register window, so every time
> > I send
> > MS_TPC_SET_RW_REG_ADRS I note the ranges.
> > And read/write functions now always attempt to send
> > MS_TPC_SET_RW_REG_ADRS. If the window is same as was 
> > set last time, TPC
> > is skipped.
> 
> Sure it is. The media will remember the window set.
> Media has all its registers in a sort of flat file. SET_RW_REG_ADDR
> selects the subset of the registers that will receive the data delivered
> within TPC. This subset is remembered until power off or until changed.

I know everything you have just said.
I just want to point out that code in many places assumes that register
window is the same as set on device initialization.

However some code changes the register window, and therefore has to
change it back at the end of execution.
If error happens, window can be left changed, while rest of code thinks
it isn't changed.
Thats is why a tracking of it would eliminate the problem safely.

> 
> 
> > 
> > However, I am thinking, that maybe I should always write
> > both param and
> > extra register? I just write 0xFF to extra register and
> > thats all.
> 
> You should write into a param register when you want to alter the command
> parameters. You cannot do so during auto incrementing block access, for
> example.
> 
> But, if you're using the auto incrementing write, you will have to write
> extra register for every page transferred.
But what if I fill extra register with 0xFF?
And besides on reads, the fact that I *write* the extra register before
I execute read command shouldn't matter at all regardless of what I
write there.
On writes however I *do* need to write extra register anyway with proper
values.

Therefore I see no reason why I can't set write window to cover both
param and extra register, and leave it always like that.

> 
> That's where changing RW_REG_ADDR comes handy.
> 
> > Windows driver does that partially. It writes 0xFF to
> > managmemt and
> > 0xF8 to overwrite flag (why???)
> 
> It's a factory default.
> Try to read it from some empty block. :-)
> (My theory is that missing bits contain invisible ECC data).
> 
> 
> > I don't
> > think that matters.
> > It also always sends the MS_TPC_SET_RW_REG_ADRS, which I
> > don't like.
> > 
> 
> This only reduces the performance slightly. SET_RW_REG_ADDR does not
> influence the media's state machine as far as I can tell, unless you try
> to push it during the data transfer cycle (whereupon you will end up
> having a literal value of the tpc in the media data buffer).
Indeed. Maybe I too should just send the MS_TPC_SET_RW_REG_ADRS at start
of command, and know that nothing will go south....

Even if that reduces performance by 0.2%, it isn't big deal. Data
corruptions is very big deal.


Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> I know everything you have just said.
> I just want to point out that code in many places assumes
> that register
> window is the same as set on device initialization.

It's not.
Can you please point at a particular place?

> > But, if you're using the auto incrementing write, you
> will have to write
> > extra register for every page transferred.
> But what if I fill extra register with 0xFF?
> And besides on reads, the fact that I *write* the extra
> register before
> I execute read command shouldn't matter at all regardless
> of what I
> write there.
> On writes however I *do* need to write extra register
> anyway with proper
> values.
> 
> Therefore I see no reason why I can't set write window to
> cover both
> param and extra register, and leave it always like that.

Because when you do autoincrementing write you _can not_ write into param register. You will break the command execution.

On the other thought, it may be unnecessary to write unique extra data to every page, so one full register write at the beginning of the command may do. Considering, that legacy memstick is not going to evolve, this may be reasonable assumption.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Wed, 2010-08-11 at 01:08 -0700, Alex Dubov wrote: 
> > I know everything you have just said.
> > I just want to point out that code in many places assumes
> > that register
> > window is the same as set on device initialization.
> 
> It's not.
> Can you please point at a particular place?
> 
> > > But, if you're using the auto incrementing write, you
> > will have to write
> > > extra register for every page transferred.
> > But what if I fill extra register with 0xFF?
> > And besides on reads, the fact that I *write* the extra
> > register before
> > I execute read command shouldn't matter at all regardless
> > of what I
> > write there.
> > On writes however I *do* need to write extra register
> > anyway with proper
> > values.
> > 
> > Therefore I see no reason why I can't set write window to
> > cover both
> > param and extra register, and leave it always like that.
> 
> Because when you do autoincrementing write you _can not_ write into param register. You will break the command execution.
Thanks, I finally understand you, so you are objection to write of
_param_ register, not the _extra_.
I agree with you.


> 
> On the other thought, it may be unnecessary to write unique extra data to every page, so one full register write at the beginning of the command may do. Considering, that legacy memstick is not going to evolve, this may be reasonable assumption.
Indeed that what I do now.
Maximum I might need to clear page status bits, but I can do that later
after I write the block.
This won't be any performance impact because amount of bad pages
shouldn't be normally greater that zero.
(Otherwise there will be data loss...)

One interesting thing that I just want your opinion on is what to do
with correctable errors.
Common sense suggests to relocate the sector + and mark it bad.
But I don't know how common such sectors are, and thus I could do more
harm that good by marking too many sectors as bad.

Of course all such problems are reason why today flash devices contain
the FTL inside, and can improve/define it in the way they want.

No more questions for now, thank you very much for help.

I hope I create ms_block.c soon, and put that old problem to the rest.

As time permits I will also port your driver for xD portion of jMicron
device (which I have).

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> Maximum I might need to clear page status bits, but I can
> do that later
> after I write the block.
> This won't be any performance impact because amount of bad
> pages
> shouldn't be normally greater that zero.
> (Otherwise there will be data loss...)


These things do degrade. I think, memsticks do write-verify, so bad blocks
will appear during write and can be marked as such without any data loss.

> 
> One interesting thing that I just want your opinion on is
> what to do
> with correctable errors.
> Common sense suggests to relocate the sector + and mark it
> bad.
> But I don't know how common such sectors are, and thus I
> could do more
> harm that good by marking too many sectors as bad.

I agree that number of writes to the media should be kept minimal.
So bad (in either way) blocks encountered should be logged, but not
touched, unless the error appears during an actual write/modify operation.

> 
> I hope I create ms_block.c soon, and put that old problem
> to the rest.

If you have time and desire, try to put a low level format option in - 
some function to erase all blocks, except the system ones.

> 
> As time permits I will also port your driver for xD portion
> of jMicron
> device (which I have).

By the way, I've got some errata for the newer JMicron chipsets. If they
have not contacted you yet, I'll forward it to you.



      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Thu, 2010-08-12 at 00:22 -0700, Alex Dubov wrote: 
> > Maximum I might need to clear page status bits, but I can
> > do that later
> > after I write the block.
> > This won't be any performance impact because amount of bad
> > pages
> > shouldn't be normally greater that zero.
> > (Otherwise there will be data loss...)
> 
> 
> These things do degrade. I think, memsticks do write-verify, so bad blocks
> will appear during write and can be marked as such without any data loss.
> 
> > 
> > One interesting thing that I just want your opinion on is
> > what to do
> > with correctable errors.
> > Common sense suggests to relocate the sector + and mark it
> > bad.
> > But I don't know how common such sectors are, and thus I
> > could do more
> > harm that good by marking too many sectors as bad.
> 
> I agree that number of writes to the media should be kept minimal.
> So bad (in either way) blocks encountered should be logged, but not
> touched, unless the error appears during an actual write/modify operation.
> 
> > 
> > I hope I create ms_block.c soon, and put that old problem
> > to the rest.
> 
> If you have time and desire, try to put a low level format option in - 
> some function to erase all blocks, except the system ones.
> 
> > 
> > As time permits I will also port your driver for xD portion
> > of jMicron
> > device (which I have).
> 
> By the way, I've got some errata for the newer JMicron chipsets. If they
> have not contacted you yet, I'll forward it to you.

Yeah, I didn't done anything at that direction, but eventually I get to
it.


Best regards,
Maxim Levitsky

JMicron chipset update

[Alex Dubov]

Apparently, the values I was using to configure a jmicron memstick
interface only work with some chipset revisions. So they've sent me the
following message.

If you've got a working jmicron adapter handy, you may want to try these
changes.

-----------------
We found that there is a definition problem in your code about
JMicron MS Controller.
    Our new product would get problem with this part.
    Following is the bit definition in our code.

    // --- Clock Control Register - Offset 0x48 --- //
    // D[31:4] Reserved
    // D[3]    Force MMIO Control.  0: Control by PCI CNFG, 1: Control
by MMIO.
    // D[2:0]  Clock MUX Select
    #define MSHC_CLKMUX_CONTROL_BY_MMIO   0x00000008
    #define MSHC_CLKMUX_CLK_40MHZ                  0x00000001
    #define MSHC_CLKMUX_CLK_50MHZ                  0x00000002
    #define MSHC_CLKMUX_CLK_62_5MHZ              0x00000004
    #define MSHC_CLKMUX_CLK_60MHZ                  0x00000010  // Must
set PCICnfg Offset BCh D[0] to 1
    #define MSHC_CLKMUX_CLK_OFF                      0x00000000
    #define MSHC_CLKMUX_CLK_MASK                   0x00000017

    Driver have to set this register to 0x08 to clear default clock
setting.
    And then set its value with specfic clock setting (EX:  40MHz ->
0x09, 50MHz -> 0x0A) to change clock.
    (For MS Pro-HG, we suggest to use 50MHz with 8-bit parallel mode.)
   
    Besides, driver have to set the register to 40MHz setting before
identify MS card for safe.





      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

About INT bits, I still don't understand them exactly.

First of all we have 4 meaningful bits.
In parallel mode these are exposed on data lines, in serial mode only 
MEMSTICK_INT_CED is.
And I can always send MS_TPC_GET_INT or just read the registers.

#define MEMSTICK_INT_CMDNAK 0x01
#define MEMSTICK_INT_BREQ   0x20
#define MEMSTICK_INT_ERR    0x40
#define MEMSTICK_INT_CED    0x80


Now, I send a command to device, say MS_CMD_BLOCK_READ.
What bits I need to poll until I can be sure that command is completed?

Also the MEMSTICK_INT_BREQ tells that input is available in firmware
buffer (to read using TPC_READ_LONG_DATA)?



Is that true that MEMSTICK_INT_BREQ is a summary of fifo full/empty bits
in status0?

And same about MEMSTICK_INT_ERR and status1.

I try my best to create a driver that actually works, simple, and error
free, even in unusual conditions.
Thats why I am asking all these questions.

Thanks for help,
Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> About INT bits, I still don't
> understand them exactly.
> 
> First of all we have 4 meaningful bits.
> In parallel mode these are exposed on data lines, in serial
> mode only 
> MEMSTICK_INT_CED is.
> And I can always send MS_TPC_GET_INT or just read the
> registers.
> 
> #define MEMSTICK_INT_CMDNAK 0x01

This bit means command was not acknowledged by the media (media not ready).

> #define MEMSTICK_INT_BREQ   0x20

This bit means media is ready for long data transfer (either in or out).

> #define MEMSTICK_INT_ERR    0x40

This bit means that some error had occurred during command execution. 

> #define MEMSTICK_INT_CED    0x80

This bit marks a completion of the command, but it may switch value in
between, so it is not that reliable by itself.

> 
> 
> Now, I send a command to device, say MS_CMD_BLOCK_READ.
> What bits I need to poll until I can be sure that command
> is completed?

All of them.

> 
> Also the MEMSTICK_INT_BREQ tells that input is available in
> firmware
> buffer (to read using TPC_READ_LONG_DATA)?
> 

Not exactly. BREQ signal indicated that media's state machine is in the
mode to pump data in or out. You wait for it, than you do the data
transfer (it works the same with READ and WRITE).

> 
> 
> Is that true that MEMSTICK_INT_BREQ is a summary of fifo
> full/empty bits
> in status0?

This can not be relied upon. Sony states, that only BREQ bit must be used
in block transfer operations. BE/BF bits are probably of more use in
memstick IO cards (there exists such a beast).

> 
> And same about MEMSTICK_INT_ERR and status1.

status1 errors apply only to data errors (bit flips).
There are errors in command execution that do not result in bit flips,
rather data can not be delivered at all or command/parameters are invalid.


> 
> I try my best to create a driver that actually works,
> simple, and error
> free, even in unusual conditions.
> Thats why I am asking all these questions.
> 
> Thanks for help,
> Best regards,
> Maxim Levitsky
> 
> 


      

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Maxim Levitsky]

On Tue, 2010-08-10 at 00:53 -0700, Alex Dubov wrote: 
> > About INT bits, I still don't
> > understand them exactly.
> > 
> > First of all we have 4 meaningful bits.
> > In parallel mode these are exposed on data lines, in serial
> > mode only 
> > MEMSTICK_INT_CED is.
> > And I can always send MS_TPC_GET_INT or just read the
> > registers.
> > 
> > #define MEMSTICK_INT_CMDNAK 0x01
> 
> This bit means command was not acknowledged by the media (media not ready).
> 
> > #define MEMSTICK_INT_BREQ   0x20
> 
> This bit means media is ready for long data transfer (either in or out).
> 
> > #define MEMSTICK_INT_ERR    0x40
> 
> This bit means that some error had occurred during command execution. 
> 
> > #define MEMSTICK_INT_CED    0x80
> 
> This bit marks a completion of the command, but it may switch value in
> between, so it is not that reliable by itself.
> 
> > 
> > 
> > Now, I send a command to device, say MS_CMD_BLOCK_READ.
> > What bits I need to poll until I can be sure that command
> > is completed?
> 
> All of them.
> 
> > 
> > Also the MEMSTICK_INT_BREQ tells that input is available in
> > firmware
> > buffer (to read using TPC_READ_LONG_DATA)?
> > 
> 
> Not exactly. BREQ signal indicated that media's state machine is in the
> mode to pump data in or out. You wait for it, than you do the data
> transfer (it works the same with READ and WRITE).
> 
> > 
> > 
> > Is that true that MEMSTICK_INT_BREQ is a summary of fifo
> > full/empty bits
> > in status0?
> 
> This can not be relied upon. Sony states, that only BREQ bit must be used
> in block transfer operations. BE/BF bits are probably of more use in
> memstick IO cards (there exists such a beast).
Even better. 
> 
> > 
> > And same about MEMSTICK_INT_ERR and status1.
> 
> status1 errors apply only to data errors (bit flips).
> There are errors in command execution that do not result in bit flips,
> rather data can not be delivered at all or command/parameters are invalid.
Understood.
However if I get MEMSTICK_INT_ERR, I should also look at status1,
because there might be correctable error.


Thank you very much.

Best regards,
Maxim Levitsky

Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.

[Alex Dubov]

> result in bit flips,
> > rather data can not be delivered at all or
> command/parameters are invalid.
> Understood.
> However if I get MEMSTICK_INT_ERR, I should also look at
> status1,
> because there might be correctable error.
> 
> 

I don't remember the details, but it may be, that status1 errors do not
raise INT_ERR (or do not always raise?). The idea is, first you check
INT_ERR. If command completed successfully and you received the data, you
may still need to check status1 for possible bit flips.

Some experimentation may be required, especially for the case of
correctable errors. After all, they should not prevent the command from
successful completion, yet action should be taken on them (at the very
least - log warning, just like with normal disks).