Supporting boards with NAND flash on other chip selects than CS0

[Mart Raudsepp]

Hello,

Currently southbridge/amd/cs5536/cs5536.c:chipset_flash_setup() sets up
NAND flash if enable_ide_nand_flash is set to "1" in the device tree
based on a static const structure in the same file - FlashInitTable.

The code itself supports having different NAND interfaces, specify extra
NOR for the setup and so on, based on the struct values. However in
practice this can't be used, because the struct is defined in the
southbridge code, and can't be overriden by board specific dts files
through any means - effectively telling things like "This board has NAND
on CS1" is not possible.

Due to that the ArtecGroup ThinCan DBE61 and DBE62 do not set up NAND
flash properly in coreboot-v3. They could have (and DBE62 does)
enable_ide_nand_flash enabled in dts, but that enables it on CS0, and
not on CS1.

I don't know how it's best to solve this.
I think the most generic way would be to make the whole 4 member array
struct definable or overridable in dts, but I don't think our dtc code
supports anything like that - at best we could have a variable length
array (cells) that is used for unwanted_vpci as well, telling that it
has to come in three member chunks, where first tells the type, second
the interface and third the (page size?) mask. But that is very
suboptimal too - a zero would terminate the array immediately, the type
is really an enum, not a 4 byte cell, etc.

Another option is to just assume there is only one NAND to enable, and
give the chip select location in dts - this doesn't regress what is in
practice currently supported, while being the minimum necessary to setup
up NAND for ThinCan boards properly. An initial patch for that is
attached.

Or maybe the call to chipset_flash_setup() should be pushed down to be
the responsibility of board specific C code, so it could pass in its own
FlashInitTable, after chipset_flash_setup is modified to take such a
struct instead of using the file global const struct defined in the
southbridge file?

Thoughts?

Mart Raudsepp
Artec Design LLC

Supporting boards with NAND flash on other chip selects than CS0

[Carl-Daniel Hailfinger]

On 06.11.2008 13:13, Mart Raudsepp wrote:
> Hello,
>
> Currently southbridge/amd/cs5536/cs5536.c:chipset_flash_setup() sets up
> NAND flash if enable_ide_nand_flash is set to "1" in the device tree
> based on a static const structure in the same file - FlashInitTable.
>
> The code itself supports having different NAND interfaces, specify extra
> NOR for the setup and so on, based on the struct values. However in
> practice this can't be used, because the struct is defined in the
> southbridge code, and can't be overriden by board specific dts files
> through any means - effectively telling things like "This board has NAND
> on CS1" is not possible.
>
> Due to that the ArtecGroup ThinCan DBE61 and DBE62 do not set up NAND
> flash properly in coreboot-v3. They could have (and DBE62 does)
> enable_ide_nand_flash enabled in dts, but that enables it on CS0, and
> not on CS1.
>
> I don't know how it's best to solve this.
> I think the most generic way would be to make the whole 4 member array
> struct definable or overridable in dts, but I don't think our dtc code
> supports anything like that - at best we could have a variable length
> array (cells) that is used for unwanted_vpci as well, telling that it
> has to come in three member chunks, where first tells the type, second
> the interface and third the (page size?) mask. But that is very
> suboptimal too - a zero would terminate the array immediately, the type
> is really an enum, not a 4 byte cell, etc.
>
> Another option is to just assume there is only one NAND to enable, and
> give the chip select location in dts - this doesn't regress what is in
> practice currently supported, while being the minimum necessary to setup
> up NAND for ThinCan boards properly. An initial patch for that is
> attached.
>
> Or maybe the call to chipset_flash_setup() should be pushed down to be
> the responsibility of board specific C code, so it could pass in its own
> FlashInitTable, after chipset_flash_setup is modified to take such a
> struct instead of using the file global const struct defined in the
> southbridge file?
>
> Thoughts?
>
> Mart Raudsepp
> Artec Design LLC
>
> Subject:
> [PATCH] cs5536: Support NAND flash on other locations than CS0
> From:
> Mart Raudsepp 
> Date:
> Thu, 6 Nov 2008 13:36:20 +0200
>
> Modify chipset_flash_setup to support enabling NAND flash on other locations
> than CS0, by making enable_ide_nand_flash have a non-boolean meaning where zero
> means no NAND (IDE), and 1 through 4 gives the one-based chip select array
> location (so 1 means CS0, 2 means CS1, 3 means CS2 and 4 means CS3, as chip
> select notation is zero-based).
>
> This loses the code for supporting more than one NAND chip select or different
> ones than FLASH_MEM_4K, but these couldn't be supported before anyway, because
> that is board specific, but the supporting structure was a static const struct
> in generic southbridge specific code.
> This support should be instead implemented via the device tree dts files.
>
> Enables NAND on ArtecGroup DBE61 and DBE62 on CS1, as that's where  it is.
> The end result is that these mainboards can now boot off of NAND with FILO
> without local modifications to the previously existing southbridge specific
> static const struct that had no chance of being upstreamed as it would break
> all other CS5536 NAND boards that have it on CS0.
>
> Signed-off-by: Mart Raudsepp
>
> --- a/mainboard/artecgroup/dbe61/dts
> +++ b/mainboard/artecgroup/dbe61/dts
> @@ -22,7 +22,7 @@
>
> /*
>    	chip southbridge/amd/cs5536_lx
> 	register "enable_ide_nand_flash" = "0"
> 	register "enable_ide_nand_flash" = "2"
>
> register "isa_irq" = "0"
> #register "flash_irq" = "14"

You patched the config.lb leftovers in that dts. Please fix the real dts
definitions instead.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

Supporting boards with NAND flash on other chip selects than CS0

[ron minnich]

I like your idea of making the variable non-boolean. It makes sense,
and we did not intend the dts to just be booleans. People are starting
to really use the dts as we intended now.

Overall it's a good patch, modulo the fixes carl-daniel pointed out.
Can you revise and resubmit?

ron

Supporting boards with NAND flash on other chip selects than CS0

[Mart Raudsepp]

Ühel kenal päeval, N, 2008-11-06 kell 08:06, kirjutas ron minnich:
> I like your idea of making the variable non-boolean. It makes sense,
> and we did not intend the dts to just be booleans. People are starting
> to really use the dts as we intended now.
>
> Overall it's a good patch, modulo the fixes carl-daniel pointed out.
> Can you revise and resubmit?

r985. Was waiting for work day end before simply git svn dcommit'ing to
get any further comments.
Mainly because I'm not all that happy with it myself, as it does lose
code - even though code whose functionality can't be used without
board-specific configuration support which doesn't exist.

So I have two concerns here (or lets say things we might want to work on
on top of this):

* There is no way to specify more than one NAND, or extra NOR setup, or
different NAND mapping (page size?), etc. Basically all the arguments in
the previous FlashInitTable structure - you could tell type, interface
and mask separately for each chip select. If that possibility might be
necessary for some boards in the future, then some more extensive
support in dts is needed. The main problem here is that the code
handling such an extensive struct configuration is now deleted (though
of course visible in SVN history if you know to look).
That said, I'm not even sure any of the other possibilities many any
sense in context of CS5536 - more than one NAND or different types, or
different mapping than 4K.

* CS1 counter-intuitively wanting enable_ide_nand_flash="2", etc. At
least that's commented in the southbridge dts' above the default
setting.

Regards,
Mart Raudespp

Supporting boards with NAND flash on other chip selects than CS0

[ron minnich]

On Thu, Nov 6, 2008 at 10:01 AM, Mart Raudsepp
 wrote:
> There is no way to specify more than one NAND, or extra NOR setup, or
>
> different NAND mapping (page size?), etc. Basically all the arguments in
> the previous FlashInitTable structure - you could tell type, interface
> and mask separately for each chip select. If that possibility might be
> necessary for some boards in the future, then some more extensive
> support in dts is needed. The main problem here is that the code
> handling such an extensive struct configuration is now deleted (though
> of course visible in SVN history if you know to look).
> That said, I'm not even sure any of the other possibilities many any
> sense in context of CS5536 - more than one NAND or different types, or
> different mapping than 4K.

I believe geode is end of life; I don't expect to see lots of future
designs. We may want this back in future but it is not likely.

It's a good solution for the short term, and there is no long term.

thanks

ron