Re: [flashrom] [PATCH 7/8] ichspi:c add support for Intel Hardware Sequencing

[Carl-Daniel Hailfinger]

Am 28.06.2011 05:33 schrieb Stefan Tauner:
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -8507,7 +8507,27 @@ const struct flashchip flashchips[] = {
>       .write		= write_jedec_1,
>       .read		= read_memmapped,
>   },
> -
> +#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> +	{
> +		.vendor		= "Intel",
> +		.name		= "Hardware Sequencing",
> +		.bustype	= CHIP_BUSTYPE_SPI,
> +		.manufacture_id	= INTEL_ID,
> +		.model_id	= INTEL_HWSEQ,
> +		.total_size	= 0,
> +		.page_size	= 256,
> +		.tested		= TEST_OK_PREW,
> +		.probe		= ich_hwseq_probe,
> +		.block_erasers	=
> +		{
> +			{ /* erase blocks will be set by the probing function */
> +				.block_erase = ich_hwseq_block_erase,
> +			}
> +		},
> +		.write		= ich_hwseq_write_256,
> +		.read		= ich_hwseq_read,
> +	},
> +#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
>   {
>       .vendor		= "AMIC",
>       .name		= "unknown AMIC SPI chip",

I consider a chip called "Hardware Sequencing" to be a really evil
thing. Hooking up a programmer with the help of a programmer-specific
flash chip seems to be quite popular, and it was rejected each time and
an alternative solution was found. That said, if probing for the real
chip ID is impossible here and you only have read/write/erase primitives
without any SPI access, this is essentially not a SPI bus chip, but a
programmer-specific bus chip. The struct flashchip should probably look
more like this:

+#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
+	{
+		.vendor		= "Unknown",
+		.name		= "Abstract opaque chip",
+		.bustype	= CHIP_BUSTYPE_ABSTRACT,
+		.manufacture_id	= ABSTRACT_MANUF_ID,
+		.model_id	= ABSTRACT_MODEL_ID,
+		.total_size	= 0,
+		.page_size	= 0,
+		.tested		= TEST_OK_PREW,
+		.probe		= abstract_probe, //abstract_probe points to programmer->abstract_probe which points to ich_hwseq_probe
+		.block_erasers	=
+		{
+			{ /* erase blocks will be set by the probing function */
+				.block_erase = abstract_erase, // points to programmer->abstract_erase which points to ich_hwseq_block_erase
+			}
+		},
+		.write		= ich_hwseq_write_256,

Same abstract_ game as above.


+		.read		= ich_hwseq_read,

Dito.


+	},
+#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
 	{
 		.vendor		= "AMIC",
 		.name		= "unknown AMIC SPI chip",


Yes, the programmer struct would have to be extended for this, but at
least that allows us to drive anything without having to care about the
bus if we can't access the bus anyway.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH 7/8] ichspi:c add support for Intel Hardware Sequencing

[Stefan Tauner]

On Thu, 30 Jun 2011 23:49:39 +0200
Carl-Daniel Hailfinger <[e-mail]> wrote:
> Am 28.06.2011 05:33 schrieb Stefan Tauner:
> > --- a/flashchips.c
> > +++ b/flashchips.c
> > @@ -8507,7 +8507,27 @@ const struct flashchip flashchips[] = {
> >       .write		= write_jedec_1,
> >       .read		= read_memmapped,
> >   },
> > -
> > +#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> > +	{
> > +		.vendor		= "Intel",
> > +		.name		= "Hardware Sequencing",
> > +		.bustype	= CHIP_BUSTYPE_SPI,
> > +		.manufacture_id	= INTEL_ID,
> > +		.model_id	= INTEL_HWSEQ,
> > +		.total_size	= 0,
> > +		.page_size	= 256,
> > +		.tested		= TEST_OK_PREW,
> > +		.probe		= ich_hwseq_probe,
> > +		.block_erasers	=
> > +		{
> > +			{ /* erase blocks will be set by the probing function */
> > +				.block_erase = ich_hwseq_block_erase,
> > +			}
> > +		},
> > +		.write		= ich_hwseq_write_256,
> > +		.read		= ich_hwseq_read,
> > +	},
> > +#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> >   {
> >       .vendor		= "AMIC",
> >       .name		= "unknown AMIC SPI chip",
>
> I consider a chip called "Hardware Sequencing" to be a really evil
> thing. Hooking up a programmer with the help of a programmer-specific
> flash chip seems to be quite popular, and it was rejected each time and
> an alternative solution was found. That said, if probing for the real
> chip ID is impossible here and you only have read/write/erase primitives
> without any SPI access, this is essentially not a SPI bus chip, but a
> programmer-specific bus chip.

one could argue that software sequencing does not allow real SPI access
either, only flashrom's architecture is working around it because it is
versatile enough (and ichspi.c quite complex) and swseq allows the
minimum set of operations (if not totally locked down). that's true for
other programmers too (you named it87 can't handle what i naturally
expected to be supported in the SFDP patch thread). so there is no
clear line between "real" programmers and opaque flashing interfaces
with a hidden SPI backend.

i do agree though that the line should be drawn here, or at least no
further.
> The struct flashchip should probably look
> more like this:
>
> +#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> +	{
> +		.vendor		= "Unknown",
> +		.name		= "Abstract opaque chip",
> +		.bustype	= CHIP_BUSTYPE_ABSTRACT,
> +		.manufacture_id	= ABSTRACT_MANUF_ID,
> +		.model_id	= ABSTRACT_MODEL_ID,
> +		.total_size	= 0,
> +		.page_size	= 0,
> +		.tested		= TEST_OK_PREW,
> +		.probe		= abstract_probe, //abstract_probe points to programmer->abstract_probe which points to ich_hwseq_probe
> +		.block_erasers	=
> +		{
> +			{ /* erase blocks will be set by the probing function */
> +				.block_erase = abstract_erase, // points to programmer->abstract_erase which points to ich_hwseq_block_erase
> +			}
> +		},
> +		.write		= ich_hwseq_write_256,
>
> Same abstract_ game as above.
>
> +		.read		= ich_hwseq_read,
>
> Dito.
>
> +	},
> +#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
>   {
>       .vendor		= "AMIC",
>       .name		= "unknown AMIC SPI chip",
>
> Yes, the programmer struct would have to be extended for this, but at
> least that allows us to drive anything without having to care about the
> bus if we can't access the bus anyway.

hm. your register parallel programmer patch does split the parallel
part from the generic programmer core (good stuff!). that opaque thingy
should probably reside in the core? or its own struct? anyway, i need a
way to register or mark it active/supported. suggestions?

is it possible that there are multiple opaque interfaces?
this is also a problem with my approach of course.

besides that i dont see any problems with the approach you have
outlined, but the above decisions must be finalized and i guess your
parallel programmer register patch should be in, before i start that.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner