[RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Alexey Korolev]

Physmap is generic map driver for different platforms and flash types.
We added support of LPDDR to physmap.
All changes here are related to introduction of new pfow_base parameter.
This parameter is valid in case of LPDDR chips only.

Signed-off-by: Alexey Korolev <akorolev@infradead.org>
---

diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
index 5ea1693..84522fb 100644
--- a/drivers/mtd/maps/Kconfig
+++ b/drivers/mtd/maps/Kconfig
@@ -10,8 +10,8 @@ config MTD_COMPLEX_MAPPINGS
 	  paged mappings of flash chips.
 
 config MTD_PHYSMAP
-	tristate "CFI Flash device in physical memory map"
-	depends on MTD_CFI || MTD_JEDECPROBE || MTD_ROM
+	tristate "Flash device in physical memory map"
+	depends on MTD_CFI || MTD_JEDECPROBE || MTD_ROM || MTD_LPDDR
 	help
 	  This provides a 'mapping' driver which allows the NOR Flash and
 	  ROM driver code to communicate with chips which are mapped
@@ -61,6 +61,16 @@ config MTD_PHYSMAP_BANKWIDTH
 	  Ignore this option if you use run-time physmap configuration
 	  (i.e., run-time calling physmap_configure()).
 
+config MTD_PHYSMAP_PFOWBASE
+	hex "PFOW base address for LPDDR chips"
+	depends on MTD_PHYSMAP && MTD_LPDDR
+	default "0x00000000"
+	help
+	  This is the offset of PFOW window within address space of LPDDR flash 
+	  device. Refer to BSP documentation to get PFOW window base offset. 
+	  Ignore this option if you use run-time physmap configuration
+	  (i.e., run-time calling physmap_configure()).
+
 config MTD_PHYSMAP_OF
 	tristate "Flash device in physical memory map based on OF description"
 	depends on PPC_OF && (MTD_CFI || MTD_JEDECPROBE || MTD_ROM)
diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 42d844f..c2e1cb8 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -83,7 +83,12 @@ static int physmap_flash_remove(struct platform_device *dev)
 	return 0;
 }
 
-static const char *rom_probe_types[] = { "cfi_probe", "jedec_probe", "map_rom", NULL };
+static const char *rom_probe_types[] = {
+					"cfi_probe",
+					"jedec_probe",
+					"qinfo_probe",
+					"map_rom",
+					NULL };
 #ifdef CONFIG_MTD_PARTITIONS
 static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
 #endif
@@ -128,6 +133,7 @@ static int physmap_flash_probe(struct platform_device *dev)
 		info->map[i].size = dev->resource[i].end - dev->resource[i].start + 1;
 		info->map[i].bankwidth = physmap_data->width;
 		info->map[i].set_vpp = physmap_data->set_vpp;
+		info->map[i].pfow_base = physmap_data->pfow_base;
 
 		info->map[i].virt = ioremap(info->map[i].phys, info->map[i].size);
 		if (info->map[i].virt == NULL) {
@@ -269,6 +275,9 @@ static struct platform_driver physmap_flash_driver = {
 #ifdef PHYSMAP_COMPAT
 static struct physmap_flash_data physmap_flash_data = {
 	.width		= CONFIG_MTD_PHYSMAP_BANKWIDTH,
+#ifdef CONFIG_MTD_PHYSMAP_PFOWBASE
+	.pfow_base	= CONFIG_MTD_PHYSMAP_PFOWBASE,
+#endif
 };
 
 static struct resource physmap_flash_resource = {
diff --git a/include/linux/mtd/physmap.h b/include/linux/mtd/physmap.h
index c8e63a5..76f7cab 100644
--- a/include/linux/mtd/physmap.h
+++ b/include/linux/mtd/physmap.h
@@ -24,6 +24,7 @@ struct physmap_flash_data {
 	unsigned int		width;
 	void			(*set_vpp)(struct map_info *, int);
 	unsigned int		nr_parts;
+	unsigned int		pfow_base;
 	struct mtd_partition	*parts;
 };
 

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[David Woodhouse]

On Wed, 2008-10-08 at 22:39 +0100, Alexey Korolev wrote:
> +config MTD_PHYSMAP_PFOWBASE
> +       hex "PFOW base address for LPDDR chips"
> +       depends on MTD_PHYSMAP && MTD_LPDDR
> +       default "0x00000000"

I'd rather not extend the MTD_PHYSMAP_xxx options; in fact I'd prefer to
phase them out completely. The platform device and the device tree
should be perfectly sufficient. 

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Alexey Korolev]

Hi,

> > +config MTD_PHYSMAP_PFOWBASE
> > +       hex "PFOW base address for LPDDR chips"
> > +       depends on MTD_PHYSMAP && MTD_LPDDR
> > +       default "0x00000000"
> 
> I'd rather not extend the MTD_PHYSMAP_xxx options; in fact I'd prefer to
> phase them out completely. The platform device and the device tree
> should be perfectly sufficient. 

I did this to support physmap "compat mode" option as well as mode using platform
device. For now both modes works well.
If "compat mode" options are becoming obsolete it is not a problem to kick off new features from it. 

Thanks,
Alexey

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

> I'd rather not extend the MTD_PHYSMAP_xxx options; in fact I'd prefer to
> phase them out completely. The platform device and the device tree
> should be perfectly sufficient.

Can you explain your plan here?  I like physmap.  The more powerful it
is the fewer devices need to be described.

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Lennert Buytenhek]

On Thu, Oct 16, 2008 at 05:30:27PM -0700, Jared Hulbert wrote:

> > I'd rather not extend the MTD_PHYSMAP_xxx options; in fact I'd prefer to
> > phase them out completely. The platform device and the device tree
> > should be perfectly sufficient.
> 
> Can you explain your plan here?  I like physmap.  The more powerful it
> is the fewer devices need to be described.

IMHO it's fine if physmap gets additional options, but those extra
options should be configured via platform data, and not via the legacy
CONFIG_MTD_PHYSMAP_xxx kernel config options.

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

> IMHO it's fine if physmap gets additional options, but those extra
> options should be configured via platform data, and not via the legacy
> CONFIG_MTD_PHYSMAP_xxx kernel config options.

How?  Maybe I don't understand what you mean by platform data...

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Alexey Korolev]

Hi,
> > IMHO it's fine if physmap gets additional options, but those extra
> > options should be configured via platform data, and not via the legacy
> > CONFIG_MTD_PHYSMAP_xxx kernel config options.
> 
> How?  Maybe I don't understand what you mean by platform data...
>
It is possible to setup flash physmap parameters in platform driver. In
the case of MainstoneII you just need to specify physmap flash data in mainstone.c 
like this.

static struct physmap_flash_data mst_flash_data = {
	.width = 2,
	.pfow_base = 0x0,
};

static struct resource flash_resources = {
	.start	= PXA_CS0_PHYS,
	.end	= PXA_CS0_PHYS + SZ_128M - 1,
	.flags	= IORESOURCE_MEM,
};

static struct platform_device mst_flash_device = {
	.name		= "physmap-flash",
	.id		= 0,
	.dev = {
		.platform_data = &mst_flash_data,
	},
	.resource = &flash_resources,
	.num_resources = 1,
};

Thanks,
Alexey

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

> It is possible to setup flash physmap parameters in platform driver. In
> the case of MainstoneII you just need to specify physmap flash data in mainstone.c
> like this.

Right.  Which means you have to tweak the kernel sources when you
remap things.  I much prefer to let these kinds of options and
parameters not be hard coded into the kernel sources.  How about a
.config options or commandline input instead?

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[David Woodhouse]

On Fri, 2008-10-17 at 10:58 -0700, Jared Hulbert wrote:
> Right.  Which means you have to tweak the kernel sources when you
> remap things.  I much prefer to let these kinds of options and
> parameters not be hard coded into the kernel sources.  How about a
> .config options or commandline input instead?

On sensible platforms it just appears in the device-tree...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

>> Right.  Which means you have to tweak the kernel sources when you
>> remap things.  I much prefer to let these kinds of options and
>> parameters not be hard coded into the kernel sources.  How about a
>> .config options or commandline input instead?
>
> On sensible platforms it just appears in the device-tree...

What's the device tree?

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Lennert Buytenhek]

On Fri, Oct 17, 2008 at 10:58:10AM -0700, Jared Hulbert wrote:

> > It is possible to setup flash physmap parameters in platform driver.
> > In the case of MainstoneII you just need to specify physmap flash
> > data in mainstone.c like this.
> 
> Right.  Which means you have to tweak the kernel sources when you
> remap things.

How do you remap your flash chip?

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Nicolas Pitre]

On Fri, 17 Oct 2008, Jared Hulbert wrote:

> > It is possible to setup flash physmap parameters in platform driver. In
> > the case of MainstoneII you just need to specify physmap flash data in mainstone.c
> > like this.
> 
> Right.  Which means you have to tweak the kernel sources when you
> remap things.  I much prefer to let these kinds of options and
> parameters not be hard coded into the kernel sources.

Do you have remapable hardware?  ;-)


Nicolas

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

>> Right.  Which means you have to tweak the kernel sources when you
>> remap things.  I much prefer to let these kinds of options and
>> parameters not be hard coded into the kernel sources.
>
> Do you have remapable hardware?  ;-)

Yes.  Yes, I do.  I have a designed that is basically a Mainstone 2
with a highly configurable Flash bus.

What more there are dozens of machines out there for each major
reference design that differ only in how the various memory types are
configured and which peripherals are included.  The different
peripherals can be turned on or off with drivers in the .config, but
if I add Flash on a different chipselect I have to hack .c files.

On a x86 system you don't have to hack .c files for every "machine"
out there.  We don't have to tweak a device-map .c file to use a SATA
vs a SCSI vs an IDE drive.  Why do we put up with that for Flash?  You
can run the same zImage on PXA250/270/320 but if I dare move Flash
around I have to recompile?  We are so close.  CFI configures most NOR
parts, we just need a rational physmap mechanism to make this entire
Flash config thing "machine" agnostic.  Is nobody with me on this one?

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Lennert Buytenhek]

On Fri, Oct 17, 2008 at 07:54:41PM -0700, Jared Hulbert wrote:

> On a x86 system you don't have to hack .c files for every "machine"
> out there.  We don't have to tweak a device-map .c file to use a SATA
> vs a SCSI vs an IDE drive.  Why do we put up with that for Flash?

You are confusing a bunch of orthogonal issues here.


There's a couple of mechanisms to describe/list peripherals in a
system:
1. The peripheral sits on the PCI or USB bus or another type of bus
   that supports autodetection or autoconfiguration.
2. The peripheral isn't autodetectable and is described in an ACPI
   table or in OF or a similar type of mechanism where the bootloader/
   firmware/BIOS/whatever-you-want-to-call-it provides a list of
   peripherals to the kernel so that the kernel can instantiate the
   right drivers.  (x86/ppc?)
3. The kernel has some kind of way of detecting which platform it is
   running on, and contains built-in tables that describe which
   non-autodetectable peripherals are present on a given platform.  (ARM)

ARM is not a platform that supports (2), and I guess the part of the
discussion is whether a flash bus is (1) or not.

I don't think there is enough info to do full reliable autodetection
of flash chips on any random platform, and then there are things like
platforms using partition tables that aren't stored on the flash chip
itself, so I don't think (1) holds and we're stuck with (3) in the
general case.  IOW, I think NOR chips fall in the same category as
AC97 codecs or I2C devices or SPI ROMs.


But (orthogonal to this), the discussion was about instantiating
platform devices in per-platform code versus using .config to describe
peripherals to the kernel.

Both of these methods to describe peripherals to the kernel fall into
category (3) above, and conceptually, using platform data or setting
.config are entirely the same thing.  But in practice, the platform
data approach has a bunch of advantages over the .config approach,
such as that with the platform data approach it's still possible to
build a single kernel image that supports booting on multiple different
boards (which is important e.g. for distributors), whereas with the
.config approach of configuring physmap you're forcing every board
type that you want to support with your kernel image to have a flash
chip at that address.

(In fact, there are no advantages of the .config method over the
platform data method, only disadvantages, which is why I still want
to kill the CONFIG_MTD_PHYSMAP_* options one day.)


What your argument _appears_ to be is that it's okay to use an
inferior method of configuring peripherals (.config) over a superior
method (instantiating platform devices from platform code) because we
should really be using an entirely different third method (autodetect
the device or pass the info in from the firmware) in the first place.
I don't really see how that conclusion follows.


> You can run the same zImage on PXA250/270/320 but if I dare move Flash
> around I have to recompile?

"You can run the same zImage on a Core 2 and an Opteron machine, but
if I take out my soldering iron and re-route my onboard network card's
PCI IRQ line to a different interrupt pin on my southbridge I have to
rebuild my ACPI table??" :-)

Anyway, doesn't changing your .config involve recompiling as well?

If what you are trying to say is that ARM should support a method
such as (2) above, I wouldn't immediately disagree.  But I don't see
what that has to do with platform data versus .config -- they are
conceptually the same thing.

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

> ARM is not a platform that supports (2), and I guess the part of the
> discussion is whether a flash bus is (1) or not.

CFI Flash chips fall in the (1) category as a rule.

> I don't think there is enough info to do full reliable autodetection
> of flash chips on any random platform

I think (1) applies if you consider PHYSMAP as an analog of the USB
Host Controller driver, no?

Once you configure a physaddr as a "CFI Flash bus" using PHYSMAP, the
"full reliable autodetection" occurs.

> and then there are things like
> platforms using partition tables that aren't stored on the flash chip
> itself

I haven't used a system that stored the partition tables on Flash for
at least 6 years.  The partition tables can be configured on the
kernel command line.

> conceptually, using platform data or setting
> .config are entirely the same thing.

I can see why you would say that and how from a certion POV it's quite
true.  However, I think there is a big distinction.  The .config
method requires a different defconfig entry to support a platform.
The platform data requires adding a .c file to arch/arm/mach-pxa/ and
tweaks to arch/arm/mach-pxa/Kconfig and arch/arm/mach-pxa/Makefile,
no?

>  But in practice, the platform
> data approach has a bunch of advantages over the .config approach,
> such as that with the platform data approach it's still possible to
> build a single kernel image that supports booting on multiple different
> boards (which is important e.g. for distributors),'

Yes there are places where platform data approach is very useful, I
totally agree.  Yet there are also places where it is annoying.

> whereas with the
> .config approach of configuring physmap you're forcing every board
> type that you want to support with your kernel image to have a flash
> chip at that address.

Also I'm not sure that totally accurate.  Here you can use the kernel
command line to alter the physmap.

And besides even if I did hardcode the physmap address.  I'm not
forcing boards to have Flash there, I'm requiring that it be safe to
probe for a Flash chip there.  Okay that's splitting hairs but we are
autodetecting....

I'm not arguing that having dozens of CONFIG_PHYSMAP_X options is the
way to do this.  Perhaps changing CONFIG_PHYSMAP to be a commandline
string that can be extended as needed.  Maybe a PPC style device
tree....  What I don't like is adding .c files and Kconfig options
because I took an existing platform moved the Flash and called it a
new platform.

> (In fact, there are no advantages of the .config method over the
> platform data method, only disadvantages,

That's just not true.  1 file change to support a "platform" versus 3....

> which is why I still want
> to kill the CONFIG_MTD_PHYSMAP_* options one day.)

You misunderstand me.  I'm okay with killing CONFIG_MTD_PHYSMAP_*.  I
just want to make sure in the process we don't kill physmap or cripple
it.

I just want to make sure we keep the functionality those options hold.
 For example we could simply move them to the kernel commandline.  We
can't just pull the functionality, and expect everyone will be happy
to create a new board file because we tweak the Flash config a bit.

> What your argument _appears_ to be is that it's okay to use an
> inferior method of configuring peripherals (.config) over a superior
> method (instantiating platform devices from platform code)

Thats it, you got me.  I just _love_ inferior methods, can't get
enough of them.  :)

For what I do with the kernel instantiating platform devices from
platform code is not the superior method.  For what I do this is too
much overhead, and complicated.

> because we
> should really be using an entirely different third method (autodetect
> the device or pass the info in from the firmware) in the first place.
> I don't really see how that conclusion follows.

I argue we already DO the autodetect (CFI probing) and pass info in
from the firmware (kernel commandline).

> "You can run the same zImage on a Core 2 and an Opteron machine, but
> if I take out my soldering iron and re-route my onboard network card's
> PCI IRQ line to a different interrupt pin on my southbridge I have to
> rebuild my ACPI table??" :-)

Ummm.  I think what I am arguing is more like moving the location of
the NIC from one PCI slot to another.  Most chips have several
chipselects where CFI Flash can be placed, does using one chipselect
or the other really constitute a different platform?

> Anyway, doesn't changing your .config involve recompiling as well?

right.  But not if you use the kernel commandline.

> If what you are trying to say is that ARM should support a method
> such as (2) above, I wouldn't immediately disagree.

Well I think we have a little of both.  (1) applies as CFI chips can
be autodetected.  (2) could apply to what we hard code if we had a
more full featured physmap commandline or device tree or something.

>  But I don't see
> what that has to do with platform data versus .config -- they are
> conceptually the same thing.

Like I said above I don't quite agree here.  But I can't argue that
the .config route is ideal, but has served a purpose that the platform
data can not.

Nevertheless, I over used .config to include the kernel commandline at
times which is confusing.

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Nicolas Pitre]

On Sat, 18 Oct 2008, Jared Hulbert wrote:

> > ARM is not a platform that supports (2), and I guess the part of the
> > discussion is whether a flash bus is (1) or not.
> 
> CFI Flash chips fall in the (1) category as a rule.

No.  They clearly are (3), unless you have a special PCI controller or 
the like on which flash memory is hooked to.  CFI flash _type_ is 
autodetectable of course, but there is usually nothing at a higher level 
telling the system where the flash is on the memory bus in any automated 
way.  OTOH' the PCI and USB buses are self describing with device IDs, 
etc.

> I think (1) applies if you consider PHYSMAP as an analog of the USB
> Host Controller driver, no?

No.  You may have a USB controller on a PCI bus, in which case (1) 
applies, but on those platforms where the USB host controller is not 
found on a PCI bus, there is always platform data telling the driver 
_where_ the controller is to be found.  In that case the controller 
itself falls into category (3).  That's also the case for physmap.

> Once you configure a physaddr as a "CFI Flash bus" using PHYSMAP, the
> "full reliable autodetection" occurs.

For the flash part themselves, just like USB _devices_.  But that's not 
the issue here.

> I haven't used a system that stored the partition tables on Flash for
> at least 6 years.  The partition tables can be configured on the
> kernel command line.

Indeed, and they can be provided by platform data too.  But flash 
partition information is completely orthogonal to the actual physical 
flash location.  You may have, say, 2 Mainstone boards and they will 
both have the same set of physical chip select lines for possible flash, 
but their partition definitions might be completely different.

> > conceptually, using platform data or setting
> > .config are entirely the same thing.
> 
> I can see why you would say that and how from a certion POV it's quite
> true.  However, I think there is a big distinction.  The .config
> method requires a different defconfig entry to support a platform.

No, that's not true.  In fact we're actively working towards the 
possibility for a single defconfig, hence a single kernel binary, to 
support many different platforms at once.

> The platform data requires adding a .c file to arch/arm/mach-pxa/ and
> tweaks to arch/arm/mach-pxa/Kconfig and arch/arm/mach-pxa/Makefile,
> no?

Of course.  That's the case for every different hardware platforms.

> >  But in practice, the platform
> > data approach has a bunch of advantages over the .config approach,
> > such as that with the platform data approach it's still possible to
> > build a single kernel image that supports booting on multiple different
> > boards (which is important e.g. for distributors),'
> 
> Yes there are places where platform data approach is very useful, I
> totally agree.  Yet there are also places where it is annoying.

Could you elaborate?

> > whereas with the
> > .config approach of configuring physmap you're forcing every board
> > type that you want to support with your kernel image to have a flash
> > chip at that address.
> 
> Also I'm not sure that totally accurate.  Here you can use the kernel
> command line to alter the physmap.

Why would you ever want to do that?

> And besides even if I did hardcode the physmap address.  I'm not
> forcing boards to have Flash there, I'm requiring that it be safe to
> probe for a Flash chip there.

Sure.  But that's not the issue, is it?

> I'm not arguing that having dozens of CONFIG_PHYSMAP_X options is the
> way to do this.  Perhaps changing CONFIG_PHYSMAP to be a commandline
> string that can be extended as needed.  Maybe a PPC style device
> tree....  What I don't like is adding .c files and Kconfig options
> because I took an existing platform moved the Flash and called it a
> new platform.

If you moved the flash then either you 1) modified the hardware in which 
case it strictly speaking can't be called under the existing name, or 2) 
platform data for that platform didn't acurately reflect the physical 
capabilities of the hardware.



Nicolas

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Jared Hulbert]

> there is always platform data telling the driver
> _where_ the controller is to be found.  In that case the controller
> itself falls into category (3).  That's also the case for physmap.

Right, we need to tell the system where potential CFI Flash buses are
with some kind of data.  Physmap decouples that data from the platform
data.

> No, that's not true.  In fact we're actively working towards the
> possibility for a single defconfig, hence a single kernel binary, to
> support many different platforms at once.

Okay, that's good.

>> The platform data requires adding a .c file to arch/arm/mach-pxa/ and
>> tweaks to arch/arm/mach-pxa/Kconfig and arch/arm/mach-pxa/Makefile,
>> no?
>
> Of course.  That's the case for every different hardware platforms.

I think that is pushing it.  There would be tens of thousands of x86
"platforms" if you took that argument to it's extreme.

>> >  But in practice, the platform
>> > data approach has a bunch of advantages over the .config approach,
>> > such as that with the platform data approach it's still possible to
>> > build a single kernel image that supports booting on multiple different
>> > boards (which is important e.g. for distributors),'
>>
>> Yes there are places where platform data approach is very useful, I
>> totally agree.  Yet there are also places where it is annoying.
>
> Could you elaborate?

A lot of PXA270 hardware out there is pretty similar, there's only a
couple of registered platforms but there are thousands of different
boards.  Do we want thousands of platforms defined for the trivial
differences?

>> Also I'm not sure that totally accurate.  Here you can use the kernel
>> command line to alter the physmap.
>
> Why would you ever want to do that?

We have basically a respin of Mainstone with socket cards for Flash.
So we  alter the physmap to look at one chipselect or the other,
depending on what we are doing.  It's great for testing stuff.

That's why physmap is so important to me, I can test a stock kernel
for Mainstone with all kinds of unholy Flash configs without ever
touching the kernel.  This is very cool for us.

So you guys think of soldering irons when I talk about altering the
Flash layout, I think of swapping a card out.

> If you moved the flash then either you 1) modified the hardware in which
> case it strictly speaking can't be called under the existing name, or 2)
> platform data for that platform didn't acurately reflect the physical
> capabilities of the hardware.

So if I do a mild hack or revision of a board that should trigger a
new machine registration?  Really?  Even though today I can just tweak
the command line's physmap entry?

Okay.... 2) is a pretty interesting argument.  So how do we do
platform data for my board that has socketable Flash?

I don't know.  We have this cool ability to autodetect the Flash but
it seems like the direction we're going doesn't take advantage of
that.  I probably just don't see something.

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Nicolas Pitre]

On Sat, 18 Oct 2008, Jared Hulbert wrote:

> >> The platform data requires adding a .c file to arch/arm/mach-pxa/ and
> >> tweaks to arch/arm/mach-pxa/Kconfig and arch/arm/mach-pxa/Makefile,
> >> no?
> >
> > Of course.  That's the case for every different hardware platforms.
> 
> I think that is pushing it.  There would be tens of thousands of x86
> "platforms" if you took that argument to it's extreme.

Not really, because all x86 platforms are rather "all the same" 
otherwise they wouldn't be compatible.  Where differences are to be 
found, usually they can be discovered at run time.

On ARM, many of those differences cannot be found at run time, so 
they're statically defined and bundled on a "platform" basis.

> A lot of PXA270 hardware out there is pretty similar, there's only a
> couple of registered platforms but there are thousands of different
> boards.  Do we want thousands of platforms defined for the trivial
> differences?

Depends how trivial the differences are.  For those differences that 
cannot be probed at run time then yes we want a different platform 
defined.  You have to describe those differences somehow _anyway_, and 
for hardware it is often better to encode them in the source directly 
instead of relying on the user properly configuring the kernel cmdline 
argument in the bootloader each time, especially with hardware which is 
not expected to change.

> We have basically a respin of Mainstone with socket cards for Flash.
> So we  alter the physmap to look at one chipselect or the other,
> depending on what we are doing.  It's great for testing stuff.
> 
> That's why physmap is so important to me, I can test a stock kernel
> for Mainstone with all kinds of unholy Flash configs without ever
> touching the kernel.  This is very cool for us.
> 
> So you guys think of soldering irons when I talk about altering the
> Flash layout, I think of swapping a card out.

Still, those chip select lines where flash can be found certainly aren't 
that many?  So why not always let physmap know about them all and probe 
whatever flash that might (or might not) be connected there?

> > If you moved the flash then either you 1) modified the hardware in which
> > case it strictly speaking can't be called under the existing name, or 2)
> > platform data for that platform didn't acurately reflect the physical
> > capabilities of the hardware.
> 
> So if I do a mild hack or revision of a board that should trigger a
> new machine registration?  Really?  Even though today I can just tweak
> the command line's physmap entry?

Probably in your case the ability to override a default setting with the 
kernel cmdline is perfectly appropriate.  But most people simply want to 
configure their kernel for machine X and Y, and expect the kernel to 
know the difference between those already.

> Okay.... 2) is a pretty interesting argument.  So how do we do
> platform data for my board that has socketable Flash?

Have physmap always probe CS0 and CS1.  And if you have a way to know 
what flash bank is first then you can even have proper names for them 
(see lubbock_init() for an example of bus width and flash bank ordering 
determined at run time).

> I don't know.  We have this cool ability to autodetect the Flash but
> it seems like the direction we're going doesn't take advantage of
> that.  I probably just don't see something.

Actual flash autodetection is one thing.  Possible flash location is 
another.


Nicolas

Re: [RFC/PATCH 8/8] LPDDR extended physmap driver to support LPDDR flash.

[Lennert Buytenhek]

On Fri, Oct 17, 2008 at 10:27:47AM -0700, Jared Hulbert wrote:

> > IMHO it's fine if physmap gets additional options, but those extra
> > options should be configured via platform data, and not via the legacy
> > CONFIG_MTD_PHYSMAP_xxx kernel config options.
> 
> How?  Maybe I don't understand what you mean by platform data...

There are two ways to instantiate physmap-flash:

1. By setting the legacy CONFIG_MTD_PHYSMAP_xxx kernel config options, eg:

	arch/blackfin/configs/BF537-STAMP_defconfig:CONFIG_MTD_PHYSMAP=m
	arch/blackfin/configs/BF537-STAMP_defconfig:CONFIG_MTD_PHYSMAP_START=0x20000000
	arch/blackfin/configs/BF537-STAMP_defconfig:CONFIG_MTD_PHYSMAP_LEN=0x0
	arch/blackfin/configs/BF537-STAMP_defconfig:CONFIG_MTD_PHYSMAP_BANKWIDTH=2


2. By instantiating a physmap plaform device in your platform code, eg
   arch/arm/mach-iop32x/glantank.c:

	static struct physmap_flash_data glantank_flash_data = {
		.width		= 2,
	};

	static struct resource glantank_flash_resource = {
		.start		= 0xf0000000,
		.end		= 0xf007ffff,
		.flags		= IORESOURCE_MEM,
	};

	static struct platform_device glantank_flash_device = {
		.name		= "physmap-flash",
		.id		= 0,
		.dev		= {
			.platform_data	= &glantank_flash_data,
		},
		.num_resources	= 1,
		.resource	= &glantank_flash_resource,
	};

	platform_device_register(&glantank_flash_device);


(2) is the preferred way, and (1) only still exists because of backward
compatibility.  If we want to add new options to physmap, we shouldn't
introduce new kernel config options but just convert prospective users
over to (2) while we're at it.