LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Memory Mapping a char array in User Space
From: Ravi Gupta @ 2010-07-26 17:28 UTC (permalink / raw)
  To: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

Hi,

I am new to linux device driver development and I'm trying to learn the
memory mapping. Currently I have written a simple device driver(major number
251 and minor number 0) and in its mmap(struct file *file, struct
vm_area_struct *vma) function, I am trying to memory map a global character
array (defined in driver) to user space memory.This is my current
implementation

char map[25];

static int test_mmap(struct file *filp, struct vm_area_struct *vma)
{
            strcpy(map, "Hello World!!");
	if (remap_pfn_range(vma, vma->vm_start, page_to_pfn(virt_to_page(map)),
	                    vma->vm_end - vma->vm_start, vma->vm_page_prot)) {
                return -EAGAIN;
	}
	return 0;
}*
*

Now after compiling the driver successfully, I created a character device
file /dev/test0 using mknod command (mknod /dev/test c 251 0). And in my C
program I tried to memory map the /dev/test file.

Now what I want is that whenever I map /dev/test, internally that global
char array gets memory mapped to the user space? Also what should I pass as
the length parameter in the mmap() function? Currently I am passing 25(size
of the array). My device gets memory map successfully but when I tried to
read from it I get garbage value. Is there something that I am missing?

Thanks in advance
Ravi Gupta

[-- Attachment #2: Type: text/html, Size: 2531 bytes --]

^ permalink raw reply

* Please pull 'next' branch of 4xx tree
From: Josh Boyer @ 2010-07-26 16:58 UTC (permalink / raw)
  To: benh; +Cc: linuxppc-dev

Hi Ben,

A few small fixes for 4xx.  Please pull.

josh

The following changes since commit cccd23428347251713b643d4bc5edb610308fd49:

  powerpc: Removing dead CONFIG_SMP_750 (2010-07-09 11:28:38 +1000)

are available in the git repository at:
  ssh://master.kernel.org/pub/scm/linux/kernel/git/jwboyer/powerpc-4xx.git next

Christian Dietrich (1):
      Remove REDWOOD_[456] config options and conditional code

Lee Nipper (1):
      powerpc/40x: Distinguish AMCC PowerPC 405EX and 405EXr correctly

Stefan Roese (1):
      powerpc/44x: Fix UART2/3 interrupt assignment in PPC460EX/GT dts files

 arch/powerpc/boot/dts/canyonlands.dts |    4 +-
 arch/powerpc/boot/dts/glacier.dts     |    4 +-
 arch/powerpc/kernel/cputable.c        |  118 +++++++++++++++++++++++++++++++--
 arch/powerpc/platforms/40x/Kconfig    |   16 -----
 drivers/mtd/maps/Kconfig              |    2 +-
 drivers/mtd/maps/redwood.c            |   43 ------------
 drivers/net/Kconfig                   |    2 +-
 drivers/net/smc91x.h                  |   37 ----------
 8 files changed, 117 insertions(+), 109 deletions(-)

^ permalink raw reply

* Re: [PATCH] powerpc: Add vmcoreinfo symbols to allow makdumpfile to filter core files properly
From: Neil Horman @ 2010-07-26 15:23 UTC (permalink / raw)
  To: linux-kernel, kexec, vgoyal, hbabu, benh, paulus, linuxppc-dev
In-Reply-To: <20100713134609.GA14514@hmsreliant.think-freely.org>

On Tue, Jul 13, 2010 at 09:46:09AM -0400, Neil Horman wrote:
> Hey all-
> 	About 2 years ago now, I sent this patch upstream to allow makedumpfile
> to properly filter cores on ppc64:
> http://www.mail-archive.com/kexec@lists.infradead.org/msg02426.html
> It got acks from the kexec folks so I pulled it into RHEL, but I never checked
> back here to make sure it ever made it in, which apparently it didn't.  It still
> needs to be included, so I'm reposting it here, making sure to copy all the ppc
> folks this time.  I've retested it on the latest linus kernel and it works fine,
> allowing makedumpfile to find all the symbols it needs to properly strip a
> vmcore on ppc64.
> 
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
Ping, anyone want to chime in on this, its needed for dump filtering to work
properly on ppc64
Neil

> 
>  machine_kexec.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> 
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index bb3d893..0df7031 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -45,6 +45,18 @@ void machine_kexec_cleanup(struct kimage *image)
>  		ppc_md.machine_kexec_cleanup(image);
>  }
>  
> +void arch_crash_save_vmcoreinfo(void)
> +{
> +
> +#ifdef CONFIG_NEED_MULTIPLE_NODES
> +	VMCOREINFO_SYMBOL(node_data);
> +	VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
> +#endif
> +#ifndef CONFIG_NEED_MULTIPLE_NODES
> +	VMCOREINFO_SYMBOL(contig_page_data);
> +#endif
> +}
> +
>  /*
>   * Do not allocate memory (or fail in any way) in machine_kexec().
>   * We are past the point of no return, committed to rebooting now.
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

^ permalink raw reply

* Re: [PATCH] PCI: MSI: Restore read_msi_msg_desc(); add get_cached_msi_msg_desc()
From: Michael Ellerman @ 2010-07-26 11:20 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Stephen Rothwell, ppc-dev, LKML, Jesse Barnes, linux-pci
In-Reply-To: <1279893388.2089.9.camel@achroite.uk.solarflarecom.com>

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

On Fri, 2010-07-23 at 14:56 +0100, Ben Hutchings wrote:
> commit 2ca1af9aa3285c6a5f103ed31ad09f7399fc65d7 "PCI: MSI: Remove
> unsafe and unnecessary hardware access" changed read_msi_msg_desc() to
> return the last MSI message written instead of reading it from the
> device, since it may be called while the device is in a reduced
> power state.
> 
> However, the pSeries platform code really does need to read messages
> from the device, since they are initially written by firmware.
> Therefore:
> - Restore the previous behaviour of read_msi_msg_desc()
> - Add new functions get_cached_msi_msg{,_desc}() which return the
>   last MSI message written
> - Use the new functions where appropriate
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Looks good to me.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply

* RE: Timestamp in PowerPC 440
From: Jenkins, Clive @ 2010-07-26 11:02 UTC (permalink / raw)
  To: kostas padarnitsas, linuxppc-dev
In-Reply-To: <BAY152-w35F374705FF06792DCEED3BCA60@phx.gbl>

> I have ported Linux 2.6 to PowerPC 440. Could you tell me
> how it is possible to access the timebase register (TBU, TBL)
> from user space (in Linux) because I want to measure the time
> of an application that runs in Linux?

I just grabbed this code from the kernel (you can put the macro
expansions into the function if you like):

typedef unsigned long long u64;

#define mftbl() ({unsigned long rval; \
 asm volatile("mftbl %0" : "=3Dr" (rval)); rval;})
#define mftbu() ({unsigned long rval; \
 asm volatile("mftbu %0" : "=3Dr" (rval)); rval;})

static inline u64 get_tb(void)
{
	unsigned int tbhi, tblo, tbhi2;
	do {
		tbhi =3D get_tbu();
		tblo =3D get_tbl();
		tbhi2 =3D get_tbu();
	} while (tbhi !=3D tbhi2);
	return ((u64)tbhi << 32) | tblo;
}

The timebase frequency is available in text form from
/proc/cpuinfo and in binary (as a 4-byte bigendian
integer)from /proc/device-tree/...

grep timebase /proc/cpuinfo

find /proc/device-tree -name timebase-frequency -print \
 -exec od -td -An '{}' \;

Clive

^ permalink raw reply

* RE: [PATCH 3/6] of/spi: add support to parse the SPI flash's partitions
From: Hu Mingkai-B21284 @ 2010-07-26  8:20 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <AANLkTimQNWOvk_Tn3yedA-TSEc3x=+QR=rynvB8PKpTp@mail.gmail.com>

=20

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On=20
> Behalf Of Grant Likely
> Sent: Monday, July 26, 2010 3:53 PM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 3/6] of/spi: add support to parse the SPI=20
> flash's partitions
>=20
> On Mon, Jul 26, 2010 at 1:25 AM, Hu Mingkai-B21284=20
> <B21284@freescale.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf=20
> Of Grant=20
> >> Likely
> >> Sent: Monday, July 26, 2010 8:28 AM
> >> To: Hu Mingkai-B21284
> >> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
> >> Roy-R61911
> >> Subject: Re: [PATCH 3/6] of/spi: add support to parse the=20
> SPI flash's=20
> >> partitions
> >>
> >> On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
> >> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> >> > ---
> >> > =A0drivers/of/of_spi.c =A0 =A0 =A0 | =A0 11 +++++++++++
> >> > =A0drivers/spi/spi_mpc8xxx.c | =A0 =A01 +
> >> > =A02 files changed, 12 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c index=20
> >> > 5fed7e3..284ca0e 100644
> >> > --- a/drivers/of/of_spi.c
> >> > +++ b/drivers/of/of_spi.c
> >> > @@ -10,6 +10,8 @@
> >> > =A0#include <linux/device.h>
> >> > =A0#include <linux/spi/spi.h>
> >> > =A0#include <linux/of_spi.h>
> >> > +#include <linux/spi/flash.h>
> >> > +#include <linux/mtd/partitions.h>
> >> >
> >> > =A0/**
> >> > =A0 * of_register_spi_devices - Register child devices onto
> >> the SPI bus
> >> > @@ -26,6 +28,7 @@ void of_register_spi_devices(struct
> >> spi_master *master, struct device_node *np)
> >> > =A0 =A0 const __be32 *prop;
> >> > =A0 =A0 int rc;
> >> > =A0 =A0 int len;
> >> > + =A0 struct flash_platform_data *pdata;
> >> >
> >> > =A0 =A0 for_each_child_of_node(np, nc) {
> >> > =A0 =A0 =A0 =A0 =A0 =A0 /* Alloc an spi_device */ @@ -81,6 +84,14 =
@@ void=20
> >> > of_register_spi_devices(struct
> >> spi_master *master, struct device_node *np)
> >> > =A0 =A0 =A0 =A0 =A0 =A0 of_node_get(nc);
> >> > =A0 =A0 =A0 =A0 =A0 =A0 spi->dev.of_node =3D nc;
> >> >
> >> > + =A0 =A0 =A0 =A0 =A0 /* Parse the mtd partitions */
> >> > + =A0 =A0 =A0 =A0 =A0 pdata =3D kzalloc(sizeof(*pdata), =
GFP_KERNEL);
> >> > + =A0 =A0 =A0 =A0 =A0 if (!pdata)
> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> >> > + =A0 =A0 =A0 =A0 =A0 pdata->nr_parts =3D=20
> of_mtd_parse_partitions(&master->dev,
> >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 nc, =
&pdata->parts);
> >> > + =A0 =A0 =A0 =A0 =A0 spi->dev.platform_data =3D pdata;
> >> > +
> >>
> >> Nack. =A0Not all spi devices are mtd devices. =A0In fact, most are =
not.
> >>
> >> The spi driver itself should call the=20
> of_mtd_parse_partitions code to=20
> >> get the partition map. =A0Do not use pdata in this case.
> >>
> >
> > Yes, we can call of_mtd_parse_partitions to get the=20
> partiton map, but=20
> > how can we pass this map to spi device to register to MTD layer?
> >
> > The spi device is created and registered when call=20
> > of_register_spi_device in the spi controller probe=20
> function, then the=20
> > spi device will traverse the spi device driver list to find=20
> the proper=20
> > driver, if matched, then call the spi device driver probe=20
> code where=20
> > the mtd partition info is registered to the mtd layer.
> >
> > so where is the proper place to put the=20
> of_mtd_parse_partitions code?
>=20
> In the device driver probe code.
>=20

This is the way that I did at first, thus we need to add the same code
in all the spi flash driver to get partition map info.

or we add the get partition map code to of_spi.c?

+/*
+ * of_parse_flash_partition - Parse the flash partition on the SPI bus
+ * @spi:       Pointer to spi_device device
+ */
+void of_parse_flash_partition(struct spi_device *spi)
+{
+       struct mtd_partition *parts;
+       struct flash_platform_data *spi_pdata;
+       int nr_parts =3D 0;
+       static int num_flash;
+       struct device_node *np =3D spi->dev.archdata.of_node;
+
+       nr_parts =3D of_mtd_parse_partitions(&spi->dev, np, &parts);
+       if (!nr_parts)
+               goto end;
+
+       spi_pdata =3D kzalloc(sizeof(*spi_pdata), GFP_KERNEL);
+       if (!spi_pdata)
+               goto end;
+       spi_pdata->name =3D kzalloc(10, GFP_KERNEL);
+       if (!spi_pdata->name)
+               goto free_flash;
+       snprintf(spi_pdata->name, 10, "SPIFLASH%d", num_flash++);
+
+       spi_pdata->parts =3D parts;
+       spi_pdata->nr_parts =3D nr_parts;
+
+       spi->dev.platform_data =3D spi_pdata;
+
+       return;
+
+free_flash:
+       kfree(spi_pdata);
+end:
+       return;
+}


> >> > diff --git a/drivers/spi/spi_mpc8xxx.c=20
> b/drivers/spi/spi_mpc8xxx.c=20
> >> > index efed70e..0fadaeb 100644
> >> > --- a/drivers/spi/spi_mpc8xxx.c
> >> > +++ b/drivers/spi/spi_mpc8xxx.c
> >> > @@ -137,6 +137,7 @@ int mpc8xxx_spi_transfer(struct spi_device=20
> >> > *spi,
> >> >
> >> > =A0void mpc8xxx_spi_cleanup(struct spi_device *spi) =A0{
> >> > + =A0 kfree(spi->dev.platform_data);
> >>
> >> Irrelevant given the above comment, but how does this even work? =
=A0
> >> What if a driver was detached and reattached to an=20
> spi_device? =A0The=20
> >> platform_data would be freed. =A0Not to mention that the=20
> pointer isn't=20
> >> cleared, so the driver would have no idea that it has a freed=20
> >> pointer.
> >>
> >
> > It works. If the driver detached, the spi device=20
> platform_data will be=20
> > released, when reattached, the of_register_spi_devices will=20
> be called=20
> > again, the platform_data will be allocated.
>=20
> Ah right, I wasn't looking in the right place.  On that note=20
> however, the cleanup of spi_device allocated by the OF code=20
> should also have a reciprocal helper that frees them too.  It=20
> shouldn't be done in the individual drivers.
>=20

We can define a helper to free the platform_data, but where it should be =
called?
Maybe spidev_release is a better place to call, after all the =
platfrom_data is allocated
when the spi device is created, so it should be freed when released.

Thanks,
Mingkai

^ permalink raw reply

* Re: [PATCH 6/6] DTS: add SPI flash(s25fl128p01) support on p4080ds and mpc8536ds board
From: Grant Likely @ 2010-07-26  7:59 UTC (permalink / raw)
  To: Hu Mingkai-B21284; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <73839B4A0818E747864426270AC332C30540064C@zmy16exm20.fsl.freescale.net>

On Mon, Jul 26, 2010 at 1:39 AM, Hu Mingkai-B21284 <B21284@freescale.com> w=
rote:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of
>> Grant Likely
>> Sent: Monday, July 26, 2010 8:35 AM
>> To: Hu Mingkai-B21284
>> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
>> Roy-R61911
>> Subject: Re: [PATCH 6/6] DTS: add SPI flash(s25fl128p01)
>> support on p4080ds and mpc8536ds board
>>
>> On Tue, Jul 20, 2010 at 10:08:25AM +0800, Mingkai Hu wrote:
>> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
>> > ---
>> > =A0arch/powerpc/boot/dts/mpc8536ds.dts | =A0 52
>> +++++++++++++++++++++++++++++++++++
>> > =A0arch/powerpc/boot/dts/p4080ds.dts =A0 | =A0 =A09 ++----
>> > =A02 files changed, 55 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/arch/powerpc/boot/dts/mpc8536ds.dts
>> > b/arch/powerpc/boot/dts/mpc8536ds.dts
>> > index 815cebb..e5d07ec 100644
>> > --- a/arch/powerpc/boot/dts/mpc8536ds.dts
>> > +++ b/arch/powerpc/boot/dts/mpc8536ds.dts
>> > @@ -108,6 +108,58 @@
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > =A0 =A0 =A0 =A0 =A0 =A0 };
>> >
>> > + =A0 =A0 =A0 =A0 =A0 spi@7000 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <0>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,espi";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x7000 0x1000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupts =3D <59 0x2>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupt-parent =3D <&mpic>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,espi-num-chipselects =3D <4>=
;
>> > +
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flash@0 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =
=3D <1>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D =
<1>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
spansion,s25sl12801";
>>
>> whitespace inconsitencies
>>
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spi-max-frequenc=
y =3D <40000000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 partition@u-boot=
 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
label =3D "u-boot";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
reg =3D <0x00000000 0x00100000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
read-only;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 partition@kernel=
 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
label =3D "kernel";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
reg =3D <0x00100000 0x00500000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
read-only;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 partition@dtb {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
label =3D "dtb";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
reg =3D <0x00600000 0x00100000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
read-only;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 partition@fs {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
label =3D "file system";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
reg =3D <0x00700000 0x00900000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flash@1 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
spansion,s25sl12801";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <1>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spi-max-frequenc=
y =3D <40000000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flash@2 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
spansion,s25sl12801";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <2>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spi-max-frequenc=
y =3D <40000000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flash@3 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
spansion,s25sl12801";
>>
>> whitespace inconsitencies
>>
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <3>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 spi-max-frequenc=
y =3D <40000000>;
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
>> > + =A0 =A0 =A0 =A0 =A0 };
>> > +
>> > =A0 =A0 =A0 =A0 =A0 =A0 dma@21300 {
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
>> > diff --git a/arch/powerpc/boot/dts/p4080ds.dts
>> > b/arch/powerpc/boot/dts/p4080ds.dts
>> > index 6b29eab..ac7dd23 100644
>> > --- a/arch/powerpc/boot/dts/p4080ds.dts
>> > +++ b/arch/powerpc/boot/dts/p4080ds.dts
>> > @@ -236,22 +236,19 @@
>> > =A0 =A0 =A0 =A0 =A0 =A0 };
>> >
>> > =A0 =A0 =A0 =A0 =A0 =A0 spi@110000 {
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cell-index =3D <0>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <0>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl,espi";
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x110000 0x1000>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupts =3D <53 0x2>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 interrupt-parent =3D <&mpic>;
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 espi,num-ss-bits =3D <4>;
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mode =3D "cpu";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl,espi-num-chipselects =3D <4>=
;
>> >
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fsl_m25p80@0 {
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flash@0 {
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells=
 =3D <1>;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =
=3D <1>;
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
fsl,espi-flash";
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "=
spansion,s25sl12801";
>>
>> whitespace inconsistencies
>>
>
> I use checkpatch to check the patch, but don't find any warnings and
> errors.
> so what's the whitespace inconsistencies means?
>
> hmk@rock:~/public/linux-2.6$ ./scripts/checkpatch.pl
> patch/public/v1/0006-DTS-add-SPI-flash-s25fl128p01-support-on-p4080ds-an
> d.patch
> total: 0 errors, 0 warnings, 83 lines checked
>
> patch/public/v1/0006-DTS-add-SPI-flash-s25fl128p01-support-on-p4080ds-an
> d.patch has no obvious style problems and is ready for submission.

Configure your editor to show tab characters differently from spaces.
About a dozen lines have a single space at the beginning of the line
before the tab indentation.

g.

^ permalink raw reply

* Re: [PATCH 4/6] mtd: m25p80: change the read function to read page by page
From: Grant Likely @ 2010-07-26  7:55 UTC (permalink / raw)
  To: Hu Mingkai-B21284; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <73839B4A0818E747864426270AC332C305400642@zmy16exm20.fsl.freescale.net>

On Mon, Jul 26, 2010 at 1:33 AM, Hu Mingkai-B21284 <B21284@freescale.com> w=
rote:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of
>> Grant Likely
>> Sent: Monday, July 26, 2010 8:30 AM
>> To: Hu Mingkai-B21284
>> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
>> Roy-R61911
>> Subject: Re: [PATCH 4/6] mtd: m25p80: change the read
>> function to read page by page
>>
>> On Tue, Jul 20, 2010 at 10:08:23AM +0800, Mingkai Hu wrote:
>> > For Freescale's eSPI controller, the max transaction length
>> one time
>> > is limitted by the SPCOM[TRANSLEN] field which is 0x10000. When used
>> > mkfs.ext2 command to create ext2 filesystem on the flash, the read
>> > length will exceed the max value of the SPCOM[TRANSLEN] field, so
>> > change the read function to read page by page.
>> >
>> > For other SPI flash driver, also needed to change the read
>> function if
>> > used the eSPI controller.
>> >
>> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
>> > ---
>> > =A0drivers/mtd/devices/m25p80.c | =A0 18 ++++++++++++++----
>> > =A01 files changed, 14 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/mtd/devices/m25p80.c
>> > b/drivers/mtd/devices/m25p80.c index 81e49a9..6cbe6b1 100644
>> > --- a/drivers/mtd/devices/m25p80.c
>> > +++ b/drivers/mtd/devices/m25p80.c
>> > @@ -317,6 +317,7 @@ static int m25p80_read(struct mtd_info
>> *mtd, loff_t from, size_t len,
>> > =A0 =A0 struct m25p *flash =3D mtd_to_m25p(mtd);
>> > =A0 =A0 struct spi_transfer t[2];
>> > =A0 =A0 struct spi_message m;
>> > + =A0 u32 i, page_size =3D 0;
>> >
>> > =A0 =A0 DEBUG(MTD_DEBUG_LEVEL2, "%s: %s %s 0x%08x, len %zd\n",
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_name(&flash->spi->dev), __=
func__,
>> "from", @@ -341,7 +342,6 @@
>> > static int m25p80_read(struct mtd_info *mtd, loff_t from,
>> size_t len,
>> > =A0 =A0 spi_message_add_tail(&t[0], &m);
>> >
>> > =A0 =A0 t[1].rx_buf =3D buf;
>> > - =A0 t[1].len =3D len;
>> > =A0 =A0 spi_message_add_tail(&t[1], &m);
>> >
>> > =A0 =A0 /* Byte count starts at zero. */
>> > @@ -364,11 +364,21 @@ static int m25p80_read(struct mtd_info *mtd,
>> > loff_t from, size_t len,
>> >
>> > =A0 =A0 /* Set up the write data buffer. */
>> > =A0 =A0 flash->command[0] =3D OPCODE_READ;
>> > - =A0 m25p_addr2cmd(flash, from, flash->command);
>> >
>> > - =A0 spi_sync(flash->spi, &m);
>> > + =A0 for (i =3D page_size; i < len; i +=3D page_size) {
>> > + =A0 =A0 =A0 =A0 =A0 page_size =3D len - i;
>> > + =A0 =A0 =A0 =A0 =A0 if (page_size > flash->page_size)
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 page_size =3D flash->page_size;
>> >
>> > - =A0 *retlen =3D m.actual_length - m25p_cmdsz(flash) -
>> FAST_READ_DUMMY_BYTE;
>> > + =A0 =A0 =A0 =A0 =A0 m25p_addr2cmd(flash, from + i, flash->command);
>> > + =A0 =A0 =A0 =A0 =A0 t[1].len =3D page_size;
>> > + =A0 =A0 =A0 =A0 =A0 t[1].rx_buf =3D buf + i;
>> > +
>> > + =A0 =A0 =A0 =A0 =A0 spi_sync(flash->spi, &m);
>> > +
>> > + =A0 =A0 =A0 =A0 =A0 *retlen +=3D m.actual_length - m25p_cmdsz(flash)
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 - FAST_READ_DUMMY_BYTE;
>> > + =A0 }
>>
>> This patch seems to cripple all users just because eSPI
>> cannot handle it. =A0Am I reading it wrong?
>>
>
> You're right, this will cripple all users.
>
> At first, I want to contain this specific code with CONFIG_SPI_FSL_ESPI
> option,
> or register a specific read function like:
>
> #ifdef CONFIG_SPI_FSL_ESPI
> =A0 =A0 =A0 =A0flash->mtd.read =3D m25p80_read_espi;
> #endif
>
> but this will make the code looks arguly,

And it is multiplatform-unfriendly.

> are there other ways?

You need to select the correct transfer behaviour at driver probe
time.  You'll need a way to indicate that the spi_master isn't able to
handle your transfer request.  I'd need to think about it more to come
up with a concrete suggestion.

g.

^ permalink raw reply

* Re: [PATCH 3/6] of/spi: add support to parse the SPI flash's partitions
From: Grant Likely @ 2010-07-26  7:52 UTC (permalink / raw)
  To: Hu Mingkai-B21284; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <73839B4A0818E747864426270AC332C305400637@zmy16exm20.fsl.freescale.net>

On Mon, Jul 26, 2010 at 1:25 AM, Hu Mingkai-B21284 <B21284@freescale.com> w=
rote:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of
>> Grant Likely
>> Sent: Monday, July 26, 2010 8:28 AM
>> To: Hu Mingkai-B21284
>> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
>> Roy-R61911
>> Subject: Re: [PATCH 3/6] of/spi: add support to parse the SPI
>> flash's partitions
>>
>> On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
>> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
>> > ---
>> > =A0drivers/of/of_spi.c =A0 =A0 =A0 | =A0 11 +++++++++++
>> > =A0drivers/spi/spi_mpc8xxx.c | =A0 =A01 +
>> > =A02 files changed, 12 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c index
>> > 5fed7e3..284ca0e 100644
>> > --- a/drivers/of/of_spi.c
>> > +++ b/drivers/of/of_spi.c
>> > @@ -10,6 +10,8 @@
>> > =A0#include <linux/device.h>
>> > =A0#include <linux/spi/spi.h>
>> > =A0#include <linux/of_spi.h>
>> > +#include <linux/spi/flash.h>
>> > +#include <linux/mtd/partitions.h>
>> >
>> > =A0/**
>> > =A0 * of_register_spi_devices - Register child devices onto
>> the SPI bus
>> > @@ -26,6 +28,7 @@ void of_register_spi_devices(struct
>> spi_master *master, struct device_node *np)
>> > =A0 =A0 const __be32 *prop;
>> > =A0 =A0 int rc;
>> > =A0 =A0 int len;
>> > + =A0 struct flash_platform_data *pdata;
>> >
>> > =A0 =A0 for_each_child_of_node(np, nc) {
>> > =A0 =A0 =A0 =A0 =A0 =A0 /* Alloc an spi_device */
>> > @@ -81,6 +84,14 @@ void of_register_spi_devices(struct
>> spi_master *master, struct device_node *np)
>> > =A0 =A0 =A0 =A0 =A0 =A0 of_node_get(nc);
>> > =A0 =A0 =A0 =A0 =A0 =A0 spi->dev.of_node =3D nc;
>> >
>> > + =A0 =A0 =A0 =A0 =A0 /* Parse the mtd partitions */
>> > + =A0 =A0 =A0 =A0 =A0 pdata =3D kzalloc(sizeof(*pdata), GFP_KERNEL);
>> > + =A0 =A0 =A0 =A0 =A0 if (!pdata)
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
>> > + =A0 =A0 =A0 =A0 =A0 pdata->nr_parts =3D of_mtd_parse_partitions(&mas=
ter->dev,
>> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 nc, &pdata->part=
s);
>> > + =A0 =A0 =A0 =A0 =A0 spi->dev.platform_data =3D pdata;
>> > +
>>
>> Nack. =A0Not all spi devices are mtd devices. =A0In fact, most are not.
>>
>> The spi driver itself should call the of_mtd_parse_partitions
>> code to get the partition map. =A0Do not use pdata in this case.
>>
>
> Yes, we can call of_mtd_parse_partitions to get the partiton map, but
> how can we
> pass this map to spi device to register to MTD layer?
>
> The spi device is created and registered when call
> of_register_spi_device in the
> spi controller probe function, then the spi device will traverse the spi
> device driver
> list to find the proper driver, if matched, then call the spi device
> driver probe code
> where the mtd partition info is registered to the mtd layer.
>
> so where is the proper place to put the of_mtd_parse_partitions code?

In the device driver probe code.

>> > diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
>> > index efed70e..0fadaeb 100644
>> > --- a/drivers/spi/spi_mpc8xxx.c
>> > +++ b/drivers/spi/spi_mpc8xxx.c
>> > @@ -137,6 +137,7 @@ int mpc8xxx_spi_transfer(struct spi_device *spi,
>> >
>> > =A0void mpc8xxx_spi_cleanup(struct spi_device *spi) =A0{
>> > + =A0 kfree(spi->dev.platform_data);
>>
>> Irrelevant given the above comment, but how does this even
>> work? =A0What if a driver was detached and reattached to an
>> spi_device? =A0The platform_data would be freed. =A0Not to
>> mention that the pointer isn't cleared, so the driver would
>> have no idea that it has a freed pointer.
>>
>
> It works. If the driver detached, the spi device platform_data will be
> released,
> when reattached, the of_register_spi_devices will be called again, the
> platform_data
> will be allocated.

Ah right, I wasn't looking in the right place.  On that note however,
the cleanup of spi_device allocated by the OF code should also have a
reciprocal helper that frees them too.  It shouldn't be done in the
individual drivers.

g.

^ permalink raw reply

* Re: [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
From: Grant Likely @ 2010-07-26  7:45 UTC (permalink / raw)
  To: Zang Roy-R61911; +Cc: linuxppc-dev, Hu Mingkai-B21284
In-Reply-To: <83024545B35D6445A690EFD603E7869A050ECB@zch01exm23.fsl.freescale.net>

On Mon, Jul 26, 2010 at 1:07 AM, Zang Roy-R61911 <r61911@freescale.com> wro=
te:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of
>> Grant Likely
>> Sent: Monday, July 26, 2010 8:14 AM
>> To: Hu Mingkai-B21284; "@angua.secretlab.ca"@angua.secretlab.ca
>> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
>> Roy-R61911
>> Subject: Re: [PATCH 1/6] spi/mpc8xxx: refactor the common
>> code for SPI/eSPI controller
>>
>> On Tue, Jul 20, 2010 at 10:08:20AM +0800, Mingkai Hu wrote:
>> > Refactor the common code to file spi_mpc8xxx.c used by SPI/eSPI
>> > controller driver, move the SPI controller driver to a new file
>> > fsl_spi.c, and leave the QE/CPM SPI controller code in this file.
>> >
>> > Because the register map of the SPI controller and eSPI controller
>> > is so different, also leave the code operated the register to the
>> > driver code, not the common code.
>> >
>> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
>> > ---
>> > =A0drivers/spi/Kconfig =A0 =A0 =A0 | =A0 13 +-
>> > =A0drivers/spi/Makefile =A0 =A0 =A0| =A0 =A01 +
>> > =A0drivers/spi/fsl_spi.c =A0 =A0 | 1118
>> ++++++++++++++++++++++++++++++++++++++++++
>> > =A0drivers/spi/spi_mpc8xxx.c | 1198
>> ++-------------------------------------------
>> > =A0drivers/spi/spi_mpc8xxx.h | =A0135 +++++
>>
>> Please name files spi-*.[ch]. =A0I'm going to start enforcing
>> this naming convention in the drivers/spi directory.
>>
>> > =A05 files changed, 1299 insertions(+), 1166 deletions(-)
>> > =A0create mode 100644 drivers/spi/fsl_spi.c
>> > =A0create mode 100644 drivers/spi/spi_mpc8xxx.h
>> >
>> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> > index 91c2f4f..cd564e2 100644
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -183,11 +183,18 @@ config SPI_MPC512x_PSC
>> > =A0 =A0 =A0 Controller in SPI master mode.
>> >
>> > =A0config SPI_MPC8xxx
>> > - =A0 tristate "Freescale MPC8xxx SPI controller"
>> > + =A0 bool
>>
>> This should be tristate so it can be loaded as a module.
>>
>> > =A0 =A0 depends on FSL_SOC
>> > =A0 =A0 help
>> > - =A0 =A0 This enables using the Freescale MPC8xxx SPI
>> controllers in master
>> > - =A0 =A0 mode.
>> > + =A0 =A0 This enables using the Freescale MPC8xxx SPI/eSPI controller=
s
>> > + =A0 =A0 driver library.
>>
>> Drop the help text entirely. =A0It isn't needed on
>> non-user-visible options.
>>
>> > +
>> > +config SPI_FSL_SPI
>> > + =A0 tristate "Freescale SPI controller"
>>
>> "Freescale SPI controller is rather generic and doesn't give any clues
>> as to which devices actually have this controller. =A0At the very least
>> the help text should state what parts contain this controller.
>>
>> On that note, the naming convention seems a little loose.
>> Since both the eSPI and SPI are using SPI_MPC8xxx, then
>> really the names should be:
>>
>> config SPI_MPC8xxx_SPI
>> and
>> config SPI_MPC8xxx_ESPI
>
> SPI_FSL_SPI and SPI_FSL_ESDP are better at this point.
> 83xx platforms and some of 85xx platforms use SPI controller.
> From mpc8536, 85xx use ESP controller, BUT currently all P10xx, P20xx,
> P40xx use ESPI controller.
> 8xxx will not cover pxxx serial processor.

Then the config item for the shared code should be renamed to reflect this.

g.

^ permalink raw reply

* RE: [PATCH 3/6] of/spi: add support to parse the SPI flash's partitions
From: Hu Mingkai-B21284 @ 2010-07-26  7:25 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726002813.GB25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:28 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 3/6] of/spi: add support to parse the SPI=20
> flash's partitions
>=20
> On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  drivers/of/of_spi.c       |   11 +++++++++++
> >  drivers/spi/spi_mpc8xxx.c |    1 +
> >  2 files changed, 12 insertions(+), 0 deletions(-)
> >=20
> > diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c index=20
> > 5fed7e3..284ca0e 100644
> > --- a/drivers/of/of_spi.c
> > +++ b/drivers/of/of_spi.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/device.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/of_spi.h>
> > +#include <linux/spi/flash.h>
> > +#include <linux/mtd/partitions.h>
> > =20
> >  /**
> >   * of_register_spi_devices - Register child devices onto=20
> the SPI bus=20
> > @@ -26,6 +28,7 @@ void of_register_spi_devices(struct=20
> spi_master *master, struct device_node *np)
> >  	const __be32 *prop;
> >  	int rc;
> >  	int len;
> > +	struct flash_platform_data *pdata;
> > =20
> >  	for_each_child_of_node(np, nc) {
> >  		/* Alloc an spi_device */
> > @@ -81,6 +84,14 @@ void of_register_spi_devices(struct=20
> spi_master *master, struct device_node *np)
> >  		of_node_get(nc);
> >  		spi->dev.of_node =3D nc;
> > =20
> > +		/* Parse the mtd partitions */
> > +		pdata =3D kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +		if (!pdata)
> > +			return;
> > +		pdata->nr_parts =3D of_mtd_parse_partitions(&master->dev,
> > +				nc, &pdata->parts);
> > +		spi->dev.platform_data =3D pdata;
> > +
>=20
> Nack.  Not all spi devices are mtd devices.  In fact, most are not.
>=20
> The spi driver itself should call the of_mtd_parse_partitions=20
> code to get the partition map.  Do not use pdata in this case.
>=20

Yes, we can call of_mtd_parse_partitions to get the partiton map, but
how can we
pass this map to spi device to register to MTD layer?

The spi device is created and registered when call
of_register_spi_device in the
spi controller probe function, then the spi device will traverse the spi
device driver
list to find the proper driver, if matched, then call the spi device
driver probe code
where the mtd partition info is registered to the mtd layer.

so where is the proper place to put the of_mtd_parse_partitions code?


> >  		/* Register the new device */
> >  		request_module(spi->modalias);
> >  		rc =3D spi_add_device(spi);
> > diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c=20
> > index efed70e..0fadaeb 100644
> > --- a/drivers/spi/spi_mpc8xxx.c
> > +++ b/drivers/spi/spi_mpc8xxx.c
> > @@ -137,6 +137,7 @@ int mpc8xxx_spi_transfer(struct spi_device *spi,
> > =20
> >  void mpc8xxx_spi_cleanup(struct spi_device *spi)  {
> > +	kfree(spi->dev.platform_data);
>=20
> Irrelevant given the above comment, but how does this even=20
> work?  What if a driver was detached and reattached to an=20
> spi_device?  The platform_data would be freed.  Not to=20
> mention that the pointer isn't cleared, so the driver would=20
> have no idea that it has a freed pointer.
>=20

It works. If the driver detached, the spi device platform_data will be
released,
when reattached, the of_register_spi_devices will be called again, the
platform_data
will be allocated.

Thanks,
Mingkai

^ permalink raw reply

* RE: [PATCH 6/6] DTS: add SPI flash(s25fl128p01) support on p4080ds and mpc8536ds board
From: Hu Mingkai-B21284 @ 2010-07-26  7:39 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726003501.GE25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:35 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 6/6] DTS: add SPI flash(s25fl128p01)=20
> support on p4080ds and mpc8536ds board
>=20
> On Tue, Jul 20, 2010 at 10:08:25AM +0800, Mingkai Hu wrote:
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  arch/powerpc/boot/dts/mpc8536ds.dts |   52=20
> +++++++++++++++++++++++++++++++++++
> >  arch/powerpc/boot/dts/p4080ds.dts   |    9 ++----
> >  2 files changed, 55 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/arch/powerpc/boot/dts/mpc8536ds.dts=20
> > b/arch/powerpc/boot/dts/mpc8536ds.dts
> > index 815cebb..e5d07ec 100644
> > --- a/arch/powerpc/boot/dts/mpc8536ds.dts
> > +++ b/arch/powerpc/boot/dts/mpc8536ds.dts
> > @@ -108,6 +108,58 @@
> >  			};
> >  		};
> > =20
> > +		spi@7000 {
> > +			#address-cells =3D <1>;
> > +			#size-cells =3D <0>;
> > +			compatible =3D "fsl,espi";
> > +			reg =3D <0x7000 0x1000>;
> > +			interrupts =3D <59 0x2>;
> > +			interrupt-parent =3D <&mpic>;
> > + 			fsl,espi-num-chipselects =3D <4>;
> > +
> > + 			flash@0 {
> > +				#address-cells =3D <1>;
> > +				#size-cells =3D <1>;
> > + 				compatible =3D "spansion,s25sl12801";
>=20
> whitespace inconsitencies
>=20
> > +				reg =3D <0>;
> > +				spi-max-frequency =3D <40000000>;
> > +				partition@u-boot {
> > +					label =3D "u-boot";
> > +					reg =3D <0x00000000 0x00100000>;
> > +					read-only;
> > +				};
> > +				partition@kernel {
> > +					label =3D "kernel";
> > +					reg =3D <0x00100000 0x00500000>;
> > +					read-only;
> > +				};
> > +				partition@dtb {
> > +					label =3D "dtb";
> > +					reg =3D <0x00600000 0x00100000>;
> > +					read-only;
> > +				};
> > +				partition@fs {
> > +					label =3D "file system";
> > +					reg =3D <0x00700000 0x00900000>;
> > +				};
> > +			};
> > + 			flash@1 {
> > + 				compatible =3D "spansion,s25sl12801";
> > +				reg =3D <1>;
> > +				spi-max-frequency =3D <40000000>;
> > +			};
> > + 			flash@2 {
> > + 				compatible =3D "spansion,s25sl12801";
> > +				reg =3D <2>;
> > +				spi-max-frequency =3D <40000000>;
> > +			};
> > + 			flash@3 {
> > + 				compatible =3D "spansion,s25sl12801";
>=20
> whitespace inconsitencies
>=20
> > +				reg =3D <3>;
> > +				spi-max-frequency =3D <40000000>;
> > +			};
> > +		};
> > +
> >  		dma@21300 {
> >  			#address-cells =3D <1>;
> >  			#size-cells =3D <1>;
> > diff --git a/arch/powerpc/boot/dts/p4080ds.dts=20
> > b/arch/powerpc/boot/dts/p4080ds.dts
> > index 6b29eab..ac7dd23 100644
> > --- a/arch/powerpc/boot/dts/p4080ds.dts
> > +++ b/arch/powerpc/boot/dts/p4080ds.dts
> > @@ -236,22 +236,19 @@
> >  		};
> > =20
> >  		spi@110000 {
> > -			cell-index =3D <0>;
> >  			#address-cells =3D <1>;
> >  			#size-cells =3D <0>;
> >  			compatible =3D "fsl,espi";
> >  			reg =3D <0x110000 0x1000>;
> >  			interrupts =3D <53 0x2>;
> >  			interrupt-parent =3D <&mpic>;
> > -			espi,num-ss-bits =3D <4>;
> > -			mode =3D "cpu";
> > + 			fsl,espi-num-chipselects =3D <4>;
> > =20
> > -			fsl_m25p80@0 {
> > + 			flash@0 {
> >  				#address-cells =3D <1>;
> >  				#size-cells =3D <1>;
> > -				compatible =3D "fsl,espi-flash";
> > + 				compatible =3D "spansion,s25sl12801";
>=20
> whitespace inconsistencies
>=20

I use checkpatch to check the patch, but don't find any warnings and
errors.
so what's the whitespace inconsistencies means?

hmk@rock:~/public/linux-2.6$ ./scripts/checkpatch.pl
patch/public/v1/0006-DTS-add-SPI-flash-s25fl128p01-support-on-p4080ds-an
d.patch
total: 0 errors, 0 warnings, 83 lines checked

patch/public/v1/0006-DTS-add-SPI-flash-s25fl128p01-support-on-p4080ds-an
d.patch has no obvious style problems and is ready for submission.

Thanks,
Mingkai

^ permalink raw reply

* RE: [PATCH 5/6] powerpc/of: add eSPI controller dts bindings
From: Hu Mingkai-B21284 @ 2010-07-26  7:35 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726003343.GD25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:34 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 5/6] powerpc/of: add eSPI controller dts bindings
>=20
> On Tue, Jul 20, 2010 at 10:08:24AM +0800, Mingkai Hu wrote:
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  Documentation/powerpc/dts-bindings/fsl/spi.txt |   20=20
> ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> >=20
> > diff --git a/Documentation/powerpc/dts-bindings/fsl/spi.txt=20
> > b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> > index 80510c0..b360bf9 100644
> > --- a/Documentation/powerpc/dts-bindings/fsl/spi.txt
> > +++ b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> > @@ -29,3 +29,23 @@ Example:
> >  		gpios =3D <&gpio 18 1	// device reg=3D<0>
> >  			 &gpio 19 1>;	// device reg=3D<1>
> >  	};
> > +
> > +
> > +* eSPI (Enhanced Serial Peripheral Interface)
> > +
> > +Required properties:
> > +- compatible : should be "fsl,espi".
>=20
> Good practice is to always fully identify the SoC in the=20
> compatible values, followed by an optional list of other=20
> specific chips it is compatible with.  Generic compatibles=20
> like "fsl,espi" are not a good idea.
>=20
> +- compatible: should be "fsl,<chip>-espi".
>=20

Ok, the mpc8536 is the first chip to develop this driver, so will use
"fsl,mpc8536-espi".

Thanks,
Mingkai

^ permalink raw reply

* RE: [PATCH 4/6] mtd: m25p80: change the read function to read page by page
From: Hu Mingkai-B21284 @ 2010-07-26  7:33 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726003028.GC25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:30 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 4/6] mtd: m25p80: change the read=20
> function to read page by page
>=20
> On Tue, Jul 20, 2010 at 10:08:23AM +0800, Mingkai Hu wrote:
> > For Freescale's eSPI controller, the max transaction length=20
> one time=20
> > is limitted by the SPCOM[TRANSLEN] field which is 0x10000. When used
> > mkfs.ext2 command to create ext2 filesystem on the flash, the read=20
> > length will exceed the max value of the SPCOM[TRANSLEN] field, so=20
> > change the read function to read page by page.
> >=20
> > For other SPI flash driver, also needed to change the read=20
> function if=20
> > used the eSPI controller.
> >=20
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  drivers/mtd/devices/m25p80.c |   18 ++++++++++++++----
> >  1 files changed, 14 insertions(+), 4 deletions(-)
> >=20
> > diff --git a/drivers/mtd/devices/m25p80.c=20
> > b/drivers/mtd/devices/m25p80.c index 81e49a9..6cbe6b1 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -317,6 +317,7 @@ static int m25p80_read(struct mtd_info=20
> *mtd, loff_t from, size_t len,
> >  	struct m25p *flash =3D mtd_to_m25p(mtd);
> >  	struct spi_transfer t[2];
> >  	struct spi_message m;
> > +	u32 i, page_size =3D 0;
> > =20
> >  	DEBUG(MTD_DEBUG_LEVEL2, "%s: %s %s 0x%08x, len %zd\n",
> >  			dev_name(&flash->spi->dev), __func__,=20
> "from", @@ -341,7 +342,6 @@=20
> > static int m25p80_read(struct mtd_info *mtd, loff_t from,=20
> size_t len,
> >  	spi_message_add_tail(&t[0], &m);
> > =20
> >  	t[1].rx_buf =3D buf;
> > -	t[1].len =3D len;
> >  	spi_message_add_tail(&t[1], &m);
> > =20
> >  	/* Byte count starts at zero. */
> > @@ -364,11 +364,21 @@ static int m25p80_read(struct mtd_info *mtd,=20
> > loff_t from, size_t len,
> > =20
> >  	/* Set up the write data buffer. */
> >  	flash->command[0] =3D OPCODE_READ;
> > -	m25p_addr2cmd(flash, from, flash->command);
> > =20
> > -	spi_sync(flash->spi, &m);
> > +	for (i =3D page_size; i < len; i +=3D page_size) {
> > +		page_size =3D len - i;
> > +		if (page_size > flash->page_size)
> > +			page_size =3D flash->page_size;
> > =20
> > -	*retlen =3D m.actual_length - m25p_cmdsz(flash) -=20
> FAST_READ_DUMMY_BYTE;
> > +		m25p_addr2cmd(flash, from + i, flash->command);
> > +		t[1].len =3D page_size;
> > +		t[1].rx_buf =3D buf + i;
> > +
> > +		spi_sync(flash->spi, &m);
> > +
> > +		*retlen +=3D m.actual_length - m25p_cmdsz(flash)
> > +			- FAST_READ_DUMMY_BYTE;
> > +	}
>=20
> This patch seems to cripple all users just because eSPI=20
> cannot handle it.  Am I reading it wrong?
>=20

You're right, this will cripple all users.

At first, I want to contain this specific code with CONFIG_SPI_FSL_ESPI
option,
or register a specific read function like:

#ifdef CONFIG_SPI_FSL_ESPI
        flash->mtd.read =3D m25p80_read_espi;
#endif

but this will make the code looks arguly, are there other ways?

Thanks,
Mingkai

^ permalink raw reply

* RE: [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
From: Zang Roy-R61911 @ 2010-07-26  7:07 UTC (permalink / raw)
  To: Grant Likely, Hu Mingkai-B21284, galak; +Cc: linuxppc-dev
In-Reply-To: <20100726001415.GA24562@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:14 AM
> To: Hu Mingkai-B21284; "@angua.secretlab.ca"@angua.secretlab.ca
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 1/6] spi/mpc8xxx: refactor the common=20
> code for SPI/eSPI controller
>=20
> On Tue, Jul 20, 2010 at 10:08:20AM +0800, Mingkai Hu wrote:
> > Refactor the common code to file spi_mpc8xxx.c used by SPI/eSPI
> > controller driver, move the SPI controller driver to a new file
> > fsl_spi.c, and leave the QE/CPM SPI controller code in this file.
> >=20
> > Because the register map of the SPI controller and eSPI controller
> > is so different, also leave the code operated the register to the
> > driver code, not the common code.
> >=20
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  drivers/spi/Kconfig       |   13 +-
> >  drivers/spi/Makefile      |    1 +
> >  drivers/spi/fsl_spi.c     | 1118=20
> ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/spi/spi_mpc8xxx.c | 1198=20
> ++-------------------------------------------
> >  drivers/spi/spi_mpc8xxx.h |  135 +++++
>=20
> Please name files spi-*.[ch].  I'm going to start enforcing=20
> this naming convention in the drivers/spi directory.
>=20
> >  5 files changed, 1299 insertions(+), 1166 deletions(-)
> >  create mode 100644 drivers/spi/fsl_spi.c
> >  create mode 100644 drivers/spi/spi_mpc8xxx.h
> >=20
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 91c2f4f..cd564e2 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -183,11 +183,18 @@ config SPI_MPC512x_PSC
> >  	  Controller in SPI master mode.
> > =20
> >  config SPI_MPC8xxx
> > -	tristate "Freescale MPC8xxx SPI controller"
> > +	bool
>=20
> This should be tristate so it can be loaded as a module.
>=20
> >  	depends on FSL_SOC
> >  	help
> > -	  This enables using the Freescale MPC8xxx SPI=20
> controllers in master
> > -	  mode.
> > +	  This enables using the Freescale MPC8xxx SPI/eSPI controllers
> > +	  driver library.
>=20
> Drop the help text entirely.  It isn't needed on=20
> non-user-visible options.
>=20
> > +
> > +config SPI_FSL_SPI
> > +	tristate "Freescale SPI controller"
>=20
> "Freescale SPI controller is rather generic and doesn't give any clues
> as to which devices actually have this controller.  At the very least
> the help text should state what parts contain this controller.
>=20
> On that note, the naming convention seems a little loose. =20
> Since both the eSPI and SPI are using SPI_MPC8xxx, then=20
> really the names should be:
>=20
> config SPI_MPC8xxx_SPI
> and
> config SPI_MPC8xxx_ESPI

SPI_FSL_SPI and SPI_FSL_ESDP are better at this point.
83xx platforms and some of 85xx platforms use SPI controller.=20
>From mpc8536, 85xx use ESP controller, BUT currently all P10xx, P20xx,
P40xx use ESPI controller.=20
8xxx will not cover pxxx serial processor.
Thanks.
Roy

^ permalink raw reply

* RE: [PATCH 2/6] eSPI: add eSPI controller support
From: Hu Mingkai-B21284 @ 2010-07-26  7:02 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726002525.GA25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:25 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 2/6] eSPI: add eSPI controller support
>=20
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index=20
> > cd564e2..c647a00 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -196,6 +196,13 @@ config SPI_FSL_SPI
> >  	help
> >  	  This enables using the Freescale SPI controllers in=20
> master mode.
> > =20
> > +config SPI_FSL_ESPI
> > +	tristate "Freescale eSPI controller"
> > +	depends on FSL_SOC
> > +	select SPI_MPC8xxx
> > +	help
> > +	  This enables using the Freescale eSPI controllers in=20
> master mode.
> > +
>=20
> Ditto from last patch.  config SPI_MPC8xxx_SPI
>=20

Ok.

> >  config SPI_OMAP_UWIRE
> >  	tristate "OMAP1 MicroWire"
> >  	depends on ARCH_OMAP1
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index=20
> > dca9fea..6af459b 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)	=09
> +=3D mpc52xx_psc_spi.o
> >  obj-$(CONFIG_SPI_MPC52xx)		+=3D mpc52xx_spi.o
> >  obj-$(CONFIG_SPI_MPC8xxx)		+=3D spi_mpc8xxx.o
> >  obj-$(CONFIG_SPI_FSL_SPI)		+=3D fsl_spi.o
> > +obj-$(CONFIG_SPI_FSL_ESPI)		+=3D fsl_espi.o
>=20
> spi_mpc8xxx_espi.o
>=20

Ok.

> >  obj-$(CONFIG_SPI_PPC4xx)		+=3D spi_ppc4xx.o
> >  obj-$(CONFIG_SPI_S3C24XX_GPIO)		+=3D spi_s3c24xx_gpio.o
> >  obj-$(CONFIG_SPI_S3C24XX)		+=3D spi_s3c24xx_hw.o
> > diff --git a/drivers/spi/fsl_espi.c=20
> b/drivers/spi/fsl_espi.c new file=20
> > mode 100644 index 0000000..ac70c8c
> > --- /dev/null
> > +++ b/drivers/spi/fsl_espi.c
> > @@ -0,0 +1,562 @@
> > +/*
> > + * MPC8xxx eSPI controller driver.
> > + *
> > + * Copyright 2010 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute  it and/or=20
> > +modify it
> > + * under  the terms of  the GNU General  Public License as=20
> published=20
> > +by the
> > + * Free Software Foundation;  either version 2 of the  License, or=20
> > +(at your
> > + * option) any later version.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/irq.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/fsl_devices.h>
> > +#include <linux/mm.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_spi.h>
> > +#include <sysdev/fsl_soc.h>
> > +
> > +#include "spi_mpc8xxx.h"
> > +
> > +/* eSPI Controller mode register definitions */
> > +#define SPMODE_ENABLE		(1 << 31)
> > +#define SPMODE_LOOP		(1 << 30)
> > +#define SPMODE_TXTHR(x)		((x) << 8)
> > +#define SPMODE_RXTHR(x)		((x) << 0)
> > +
> > +/* eSPI Controller CS mode register definitions */
> > +#define CSMODE_CI_INACTIVEHIGH	(1 << 31)
> > +#define CSMODE_CP_BEGIN_EDGECLK	(1 << 30)
> > +#define CSMODE_REV		(1 << 29)
> > +#define CSMODE_DIV16		(1 << 28)
> > +#define CSMODE_PM(x)		((x) << 24)
> > +#define CSMODE_POL_1		(1 << 20)
> > +#define CSMODE_LEN(x)		((x) << 16)
> > +#define CSMODE_BEF(x)		((x) << 12)
> > +#define CSMODE_AFT(x)		((x) << 8)
> > +#define CSMODE_CG(x)		((x) << 3)
> > +
> > +/* Default mode/csmode for eSPI controller */ #define=20
> SPMODE_INIT_VAL=20
> > +(SPMODE_TXTHR(4) | SPMODE_RXTHR(3)) #define CSMODE_INIT_VAL=20
> > +(CSMODE_POL_1 | CSMODE_BEF(0) \
> > +			| CSMODE_AFT(0) | CSMODE_CG(1))
> > +
> > +/* SPIE register values */
> > +#define	SPIE_NE		0x00000200	/* Not empty */
> > +#define	SPIE_NF		0x00000100	/* Not full */
> > +
> > +/* SPIM register values */
> > +#define	SPIM_NE		0x00000200	/* Not empty */
> > +#define	SPIM_NF		0x00000100	/* Not full */
> > +#define SPIE_RXCNT(reg)     ((reg >> 24) & 0x3F)
> > +#define SPIE_TXCNT(reg)     ((reg >> 16) & 0x3F)
> > +
> > +/* SPCOM register values */
> > +#define SPCOM_CS(x)		((x) << 30)
> > +#define SPCOM_TRANLEN(x)	((x) << 0)
> > +#define	SPCOM_TRANLEN_MAX	0xFFFF	/* Max=20
> transaction length */
>=20
> Inconsistent whitespacing.  Some lines use tabs; others=20
> spaces.  Should be consistent on all the lines.
>=20

Ok.

> > +
> > +static
> > +int fsl_espi_setup_transfer(struct spi_device *spi, struct=20
> > +spi_transfer *t)
>=20
> Break the line in the arguments instead of the declaration. =20
> When grepping, the stuff at the front of the line is more=20
> important to see.  So:
>=20
> +static int fsl_espi_setup_transfer(struct spi_device *spi,
> +				   struct spi_transfer *t)
>=20

Ok.

> > +{
> > +	struct mpc8xxx_spi *mpc8xxx_spi;
> > +	u8 bits_per_word, pm;
> > +	u32 hz;
> > +	struct spi_mpc8xxx_cs	*cs =3D spi->controller_state;
> > +
> > +	mpc8xxx_spi =3D spi_master_get_devdata(spi->master);
> > +
> > +	if (t) {
> > +		bits_per_word =3D t->bits_per_word;
> > +		hz =3D t->speed_hz;
> > +	} else {
> > +		bits_per_word =3D 0;
> > +		hz =3D 0;
> > +	}
>=20
> Just initialize bits_per_word and hz to 0 when they are=20
> declared to eliminate the else clause.
>=20

Good suggestion.

> > +
> > +	/* spi_transfer level calls that work per-word */
> > +	if (!bits_per_word)
> > +		bits_per_word =3D spi->bits_per_word;
> > +
> > +	/* Make sure its a bit width we support [4..16] */
> > +	if ((bits_per_word < 4) || (bits_per_word > 16))
> > +		return -EINVAL;
> > +
> > +	if (!hz)
> > +		hz =3D spi->max_speed_hz;
> > +
> > +	cs->rx_shift =3D 0;
> > +	cs->tx_shift =3D 0;
> > +	cs->get_rx =3D mpc8xxx_spi_rx_buf_u32;
> > +	cs->get_tx =3D mpc8xxx_spi_tx_buf_u32;
> > +	if (bits_per_word <=3D 8) {
> > +		cs->rx_shift =3D 8 - bits_per_word;
> > +	} else if (bits_per_word <=3D 16) {
> > +		cs->rx_shift =3D 16 - bits_per_word;
> > +		if (spi->mode & SPI_LSB_FIRST)
> > +			cs->get_tx =3D fsl_espi_tx_buf_lsb;
> > +	} else
> > +		return -EINVAL;
> > +
> > +	mpc8xxx_spi->rx_shift =3D cs->rx_shift;
> > +	mpc8xxx_spi->tx_shift =3D cs->tx_shift;
> > +	mpc8xxx_spi->get_rx =3D cs->get_rx;
> > +	mpc8xxx_spi->get_tx =3D cs->get_tx;
> > +
> > +	bits_per_word =3D bits_per_word - 1;
> > +
> > +	/* mask out bits we are going to set */
> > +	cs->hw_mode &=3D ~(CSMODE_LEN(0xF) | CSMODE_DIV16
> > +				  | CSMODE_PM(0xF));
> > +
> > +	cs->hw_mode |=3D CSMODE_LEN(bits_per_word);
> > +
> > +	if ((mpc8xxx_spi->spibrg / hz) > 64) {
> > +		cs->hw_mode |=3D CSMODE_DIV16;
> > +		pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
> > +
> > +		WARN_ONCE(pm > 16, "%s: Requested speed is too=20
> low: %d Hz. "
> > +			  "Will use %d Hz instead.\n",=20
> dev_name(&spi->dev),
> > +			  hz, mpc8xxx_spi->spibrg / 1024);
> > +		if (pm > 16)
> > +			pm =3D 16;
> > +	} else
> > +		pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
>=20
> When the if clause has { }, please use braces in the else clause also.
>=20

Ok.

> > +static const struct of_device_id of_fsl_espi_match[] =3D {
> > +	{ .compatible =3D "fsl,espi" },
>=20
> Not good practice.  Use the real chip name in the compatible=20
> value.  "fsl,<chip>-espi".
>=20

Ok.

> > +	{},
>=20
> NIT: Drop the comma here to hint that no more elements should=20
> follow after the null entry.
>=20

Ok.

> > +};
> > +MODULE_DEVICE_TABLE(of, of_fsl_espi_match);
> > +
> > +static struct of_platform_driver of_fsl_espi_driver =3D {
> > +	.driver =3D {
> > +		.name =3D "fsl_espi",
> > +		.owner =3D THIS_MODULE,
> > +		.of_match_table =3D of_fsl_espi_match,
> > +	},
> > +	.probe		=3D of_fsl_espi_probe,
> > +	.remove		=3D __devexit_p(of_fsl_espi_remove),
> > +};
> > +
> > +static int __init fsl_espi_init(void) {
> > +	return of_register_platform_driver(&of_fsl_espi_driver);
> > +}
> > +
> > +static void __exit fsl_espi_exit(void) {
> > +	of_unregister_platform_driver(&of_fsl_espi_driver);
> > +}
> > +
> > +module_init(fsl_espi_init);
>=20
> Move module_init() to right below fsl_espi_init.
>=20
Ok.

> > +module_exit(fsl_espi_exit);
> > +
> > +MODULE_AUTHOR("Mingkai Hu");
> > +MODULE_DESCRIPTION("Enhanced MPC8xxx SPI Driver");=20
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/spi/spi_mpc8xxx.h b/drivers/spi/spi_mpc8xxx.h=20
> > index dcc6443..a8e8270 100644
> > --- a/drivers/spi/spi_mpc8xxx.h
> > +++ b/drivers/spi/spi_mpc8xxx.h
> > @@ -20,6 +20,7 @@
> > =20
> >  /* SPI Controller registers */
> >  struct mpc8xxx_spi_reg {
> > +#ifndef CONFIG_SPI_FSL_ESPI
> >  	u8 res1[0x20];
> >  	__be32 mode;
> >  	__be32 event;
> > @@ -27,6 +28,16 @@ struct mpc8xxx_spi_reg {
> >  	__be32 command;
> >  	__be32 transmit;
> >  	__be32 receive;
> > +#else
> > +	__be32 mode;		/* 0x000 - eSPI mode register */
> > +	__be32 event;		/* 0x004 - eSPI event register */
> > +	__be32 mask;		/* 0x008 - eSPI mask register */
> > +	__be32 command;		/* 0x00c - eSPI command register */
> > +	__be32 transmit;	/* 0x010 - eSPI transmit FIFO=20
> access register*/
> > +	__be32 receive;		/* 0x014 - eSPI receive FIFO=20
> access register*/
> > +	u8 res1[8];		/* 0x018 - 0x01c reserved */
> > +	__be32 csmode[4];	/* 0x020 - 0x02c eSPI cs mode=20
> register */
> > +#endif
>=20
> Not multiplatform friendly.  If the two devices use different=20
> register maps, then the register map needs to be defined in=20
> the .c file.  You need to code for the case where a single=20
> kernel may contain both drivers.
>=20

You're right, this will disable the kernel supporting both drivers.
I'll fix it.

Thanks,
Mingkai

^ permalink raw reply

* Re: [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
From: Grant Likely @ 2010-07-26  6:48 UTC (permalink / raw)
  To: Hu Mingkai-B21284; +Cc: ", linuxppc-dev, Zang Roy-R61911
In-Reply-To: <73839B4A0818E747864426270AC332C3054005D5@zmy16exm20.fsl.freescale.net>

On Mon, Jul 26, 2010 at 12:18 AM, Hu Mingkai-B21284
<B21284@freescale.com> wrote:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of
>> Grant Likely
>> Sent: Monday, July 26, 2010 8:14 AM
>> To: Hu Mingkai-B21284; "@angua.secretlab.ca"@angua.secretlab.ca
>> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang
>> Roy-R61911
>> Subject: Re: [PATCH 1/6] spi/mpc8xxx: refactor the common
>> code for SPI/eSPI controller
>>
>> On Tue, Jul 20, 2010 at 10:08:20AM +0800, Mingkai Hu wrote:
>> > Refactor the common code to file spi_mpc8xxx.c used by SPI/eSPI
>> > controller driver, move the SPI controller driver to a new file
>> > fsl_spi.c, and leave the QE/CPM SPI controller code in this file.
>> >
>> > Because the register map of the SPI controller and eSPI controller
>> > is so different, also leave the code operated the register to the
>> > driver code, not the common code.
>> >
>> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
>> > ---
>> > =A0drivers/spi/Kconfig =A0 =A0 =A0 | =A0 13 +-
>> > =A0drivers/spi/Makefile =A0 =A0 =A0| =A0 =A01 +
>> > =A0drivers/spi/fsl_spi.c =A0 =A0 | 1118
>> ++++++++++++++++++++++++++++++++++++++++++
>> > =A0drivers/spi/spi_mpc8xxx.c | 1198
>> ++-------------------------------------------
>> > =A0drivers/spi/spi_mpc8xxx.h | =A0135 +++++
>>
>> Please name files spi-*.[ch]. =A0I'm going to start enforcing
>> this naming convention in the drivers/spi directory.
>>
>
> Ok.
>
>> > =A05 files changed, 1299 insertions(+), 1166 deletions(-)
>> > =A0create mode 100644 drivers/spi/fsl_spi.c
>> > =A0create mode 100644 drivers/spi/spi_mpc8xxx.h
>> >
>> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> > index 91c2f4f..cd564e2 100644
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -183,11 +183,18 @@ config SPI_MPC512x_PSC
>> > =A0 =A0 =A0 Controller in SPI master mode.
>> >
>> > =A0config SPI_MPC8xxx
>> > - =A0 tristate "Freescale MPC8xxx SPI controller"
>> > + =A0 bool
>>
>> This should be tristate so it can be loaded as a module.
>>
>
> This option is selected by FSL_SPI and FSL_ESPI option, can we load the
> driver as a module?

Yes.  You may need to export some symbols, but the common library can
be loaded as a module.

>> > +static void __exit fsl_spi_exit(void)
>> > +{
>> > + =A0 of_unregister_platform_driver(&of_fsl_spi_driver);
>> > + =A0 legacy_driver_unregister();
>> > +}
>>
>> It would appear that the legacy driver should *also* be
>> separated out into its own module. =A0I realize you're just cut
>> & pasting code here, but it should be considered for a followup patch.
>>
>
> Can we remove the legacy driver thoroughly?
> If we separated out the code, what's the proper name for it?
> spi_mpc8xxx_legacy.c?

Sure.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* RE: [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
From: Hu Mingkai-B21284 @ 2010-07-26  6:18 UTC (permalink / raw)
  To: Grant Likely, "; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726001415.GA24562@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:14 AM
> To: Hu Mingkai-B21284; "@angua.secretlab.ca"@angua.secretlab.ca
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 1/6] spi/mpc8xxx: refactor the common=20
> code for SPI/eSPI controller
>=20
> On Tue, Jul 20, 2010 at 10:08:20AM +0800, Mingkai Hu wrote:
> > Refactor the common code to file spi_mpc8xxx.c used by SPI/eSPI
> > controller driver, move the SPI controller driver to a new file
> > fsl_spi.c, and leave the QE/CPM SPI controller code in this file.
> >=20
> > Because the register map of the SPI controller and eSPI controller
> > is so different, also leave the code operated the register to the
> > driver code, not the common code.
> >=20
> > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> > ---
> >  drivers/spi/Kconfig       |   13 +-
> >  drivers/spi/Makefile      |    1 +
> >  drivers/spi/fsl_spi.c     | 1118=20
> ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/spi/spi_mpc8xxx.c | 1198=20
> ++-------------------------------------------
> >  drivers/spi/spi_mpc8xxx.h |  135 +++++
>=20
> Please name files spi-*.[ch].  I'm going to start enforcing=20
> this naming convention in the drivers/spi directory.
>=20

Ok.

> >  5 files changed, 1299 insertions(+), 1166 deletions(-)
> >  create mode 100644 drivers/spi/fsl_spi.c
> >  create mode 100644 drivers/spi/spi_mpc8xxx.h
> >=20
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 91c2f4f..cd564e2 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -183,11 +183,18 @@ config SPI_MPC512x_PSC
> >  	  Controller in SPI master mode.
> > =20
> >  config SPI_MPC8xxx
> > -	tristate "Freescale MPC8xxx SPI controller"
> > +	bool
>=20
> This should be tristate so it can be loaded as a module.
>=20

This option is selected by FSL_SPI and FSL_ESPI option, can we load the
driver as a module?

> >  	depends on FSL_SOC
> >  	help
> > -	  This enables using the Freescale MPC8xxx SPI=20
> controllers in master
> > -	  mode.
> > +	  This enables using the Freescale MPC8xxx SPI/eSPI controllers
> > +	  driver library.
>=20
> Drop the help text entirely.  It isn't needed on=20
> non-user-visible options.
>=20

Ok.

> > +
> > +config SPI_FSL_SPI
> > +	tristate "Freescale SPI controller"
>=20
> "Freescale SPI controller is rather generic and doesn't give any clues
> as to which devices actually have this controller.  At the very least
> the help text should state what parts contain this controller.
>=20

Ok.

> On that note, the naming convention seems a little loose. =20
> Since both the eSPI and SPI are using SPI_MPC8xxx, then=20
> really the names should be:
>=20
> config SPI_MPC8xxx_SPI
> and
> config SPI_MPC8xxx_ESPI
>

I'll chage it, SPI_MPC8xxx_SPI/ESPI is more clearer.
=20
> > +static void __exit fsl_spi_exit(void)
> > +{
> > +	of_unregister_platform_driver(&of_fsl_spi_driver);
> > +	legacy_driver_unregister();
> > +}
>=20
> It would appear that the legacy driver should *also* be=20
> separated out into its own module.  I realize you're just cut=20
> & pasting code here, but it should be considered for a followup patch.
>=20

Can we remove the legacy driver thoroughly?
If we separated out the code, what's the proper name for it?
spi_mpc8xxx_legacy.c?

> > +
> > +module_init(fsl_spi_init);
> > +module_exit(fsl_spi_exit);
>=20
> module_init() should appear immediately after the init=20
> fsl_spi_init() function.
>=20

Ok.

Thanks,
Mingkai=20

^ permalink raw reply

* RE: [PATCH 0/6] refactor spi_mpc8xxx.c and add eSPI controller support
From: Hu Mingkai-B21284 @ 2010-07-26  5:52 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, Zang Roy-R61911
In-Reply-To: <20100726003618.GF25419@angua.secretlab.ca>

=20

> -----Original Message-----
> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of=20
> Grant Likely
> Sent: Monday, July 26, 2010 8:36 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; galak@kernel.crashing.org; Zang=20
> Roy-R61911
> Subject: Re: [PATCH 0/6] refactor spi_mpc8xxx.c and add eSPI=20
> controller support
>=20
> On Tue, Jul 20, 2010 at 10:08:19AM +0800, Mingkai Hu wrote:
> > This patchset refactor the file spi_mpc8xxx.c to abstract=20
> some common=20
> > code as a lib used by the SPI/eSPI controller driver, move the SPI=20
> > controller driver code to fsl_spi.c, and add the eSPI=20
> controller support with fsl_espi.c.
> >=20
> > Tested on P4080DS and MPC8536DS board based on latest Linux tree.
> >=20
> > [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI=20
> > controller [PATCH 2/6] eSPI: add eSPI controller support=20
> [PATCH 3/6]=20
> > of/spi: add support to parse the SPI flash's partitions [PATCH 4/6]=20
> > mtd: m25p80: change the read function to read page by page=20
> [PATCH 5/6]=20
> > powerpc/of: add eSPI controller dts bindings [PATCH 6/6]=20
> DTS: add SPI=20
> > flash(s25fl128p01) support on p4080ds and mpc8536ds board
>=20
> Hi Mingkai,
>=20
> Thanks for the patches, I've just sent my review.  Make sure=20
> you cc: spi-devel-general@lists.sourceforge.net when you post v2
>=20

Many thanks for you  review and comments, I'll submit the v2 version
with cc to spi-devel-general maillist.

Thanks,
Mingkai

^ permalink raw reply

* Timestamp in PowerPC 440
From: kostas padarnitsas @ 2010-07-26  2:14 UTC (permalink / raw)
  To: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]


Hello,
I have ported Linux 2.6 to PowerPC 440. Could you tell me how it is possible to access the timebase register (TBU, TBL) from user space (in Linux) because I want to measure the time of an application that runs in Linux?  Thank you in advance,
Kostas 		 	   		  
_________________________________________________________________
Your E-mail and More On-the-Go. Get Windows Live Hotmail Free.
https://signup.live.com/signup.aspx?id=60969

[-- Attachment #2: Type: text/html, Size: 710 bytes --]

^ permalink raw reply

* Re: [PATCH 0/6] refactor spi_mpc8xxx.c and add eSPI controller support
From: Grant Likely @ 2010-07-26  0:36 UTC (permalink / raw)
  To: Mingkai Hu; +Cc: linuxppc-dev
In-Reply-To: <1279591705-7574-1-git-send-email-Mingkai.hu@freescale.com>

On Tue, Jul 20, 2010 at 10:08:19AM +0800, Mingkai Hu wrote:
> This patchset refactor the file spi_mpc8xxx.c to abstract some common code
> as a lib used by the SPI/eSPI controller driver, move the SPI controller
> driver code to fsl_spi.c, and add the eSPI controller support with fsl_espi.c.
> 
> Tested on P4080DS and MPC8536DS board based on latest Linux tree.
> 
> [PATCH 1/6] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
> [PATCH 2/6] eSPI: add eSPI controller support
> [PATCH 3/6] of/spi: add support to parse the SPI flash's partitions
> [PATCH 4/6] mtd: m25p80: change the read function to read page by page
> [PATCH 5/6] powerpc/of: add eSPI controller dts bindings
> [PATCH 6/6] DTS: add SPI flash(s25fl128p01) support on p4080ds and mpc8536ds board

Hi Mingkai,

Thanks for the patches, I've just sent my review.  Make sure you cc: spi-devel-general@lists.sourceforge.net when you post v2

g.

^ permalink raw reply

* Re: [PATCH 6/6] DTS: add SPI flash(s25fl128p01) support on p4080ds and mpc8536ds board
From: Grant Likely @ 2010-07-26  0:35 UTC (permalink / raw)
  To: Mingkai Hu; +Cc: linuxppc-dev
In-Reply-To: <1279591705-7574-7-git-send-email-Mingkai.hu@freescale.com>

On Tue, Jul 20, 2010 at 10:08:25AM +0800, Mingkai Hu wrote:
> Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> ---
>  arch/powerpc/boot/dts/mpc8536ds.dts |   52 +++++++++++++++++++++++++++++++++++
>  arch/powerpc/boot/dts/p4080ds.dts   |    9 ++----
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/mpc8536ds.dts b/arch/powerpc/boot/dts/mpc8536ds.dts
> index 815cebb..e5d07ec 100644
> --- a/arch/powerpc/boot/dts/mpc8536ds.dts
> +++ b/arch/powerpc/boot/dts/mpc8536ds.dts
> @@ -108,6 +108,58 @@
>  			};
>  		};
>  
> +		spi@7000 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "fsl,espi";
> +			reg = <0x7000 0x1000>;
> +			interrupts = <59 0x2>;
> +			interrupt-parent = <&mpic>;
> + 			fsl,espi-num-chipselects = <4>;
> +
> + 			flash@0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> + 				compatible = "spansion,s25sl12801";

whitespace inconsitencies

> +				reg = <0>;
> +				spi-max-frequency = <40000000>;
> +				partition@u-boot {
> +					label = "u-boot";
> +					reg = <0x00000000 0x00100000>;
> +					read-only;
> +				};
> +				partition@kernel {
> +					label = "kernel";
> +					reg = <0x00100000 0x00500000>;
> +					read-only;
> +				};
> +				partition@dtb {
> +					label = "dtb";
> +					reg = <0x00600000 0x00100000>;
> +					read-only;
> +				};
> +				partition@fs {
> +					label = "file system";
> +					reg = <0x00700000 0x00900000>;
> +				};
> +			};
> + 			flash@1 {
> + 				compatible = "spansion,s25sl12801";
> +				reg = <1>;
> +				spi-max-frequency = <40000000>;
> +			};
> + 			flash@2 {
> + 				compatible = "spansion,s25sl12801";
> +				reg = <2>;
> +				spi-max-frequency = <40000000>;
> +			};
> + 			flash@3 {
> + 				compatible = "spansion,s25sl12801";

whitespace inconsitencies

> +				reg = <3>;
> +				spi-max-frequency = <40000000>;
> +			};
> +		};
> +
>  		dma@21300 {
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> diff --git a/arch/powerpc/boot/dts/p4080ds.dts b/arch/powerpc/boot/dts/p4080ds.dts
> index 6b29eab..ac7dd23 100644
> --- a/arch/powerpc/boot/dts/p4080ds.dts
> +++ b/arch/powerpc/boot/dts/p4080ds.dts
> @@ -236,22 +236,19 @@
>  		};
>  
>  		spi@110000 {
> -			cell-index = <0>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			compatible = "fsl,espi";
>  			reg = <0x110000 0x1000>;
>  			interrupts = <53 0x2>;
>  			interrupt-parent = <&mpic>;
> -			espi,num-ss-bits = <4>;
> -			mode = "cpu";
> + 			fsl,espi-num-chipselects = <4>;
>  
> -			fsl_m25p80@0 {
> + 			flash@0 {
>  				#address-cells = <1>;
>  				#size-cells = <1>;
> -				compatible = "fsl,espi-flash";
> + 				compatible = "spansion,s25sl12801";

whitespace inconsistencies

>  				reg = <0>;
> -				linux,modalias = "fsl_m25p80";
>  				spi-max-frequency = <40000000>; /* input clock */
>  				partition@u-boot {
>  					label = "u-boot";
> -- 
> 1.6.4
> 
> 

^ permalink raw reply

* Re: [PATCH 5/6] powerpc/of: add eSPI controller dts bindings
From: Grant Likely @ 2010-07-26  0:33 UTC (permalink / raw)
  To: Mingkai Hu; +Cc: linuxppc-dev
In-Reply-To: <1279591705-7574-6-git-send-email-Mingkai.hu@freescale.com>

On Tue, Jul 20, 2010 at 10:08:24AM +0800, Mingkai Hu wrote:
> Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> ---
>  Documentation/powerpc/dts-bindings/fsl/spi.txt |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/powerpc/dts-bindings/fsl/spi.txt b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> index 80510c0..b360bf9 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/spi.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/spi.txt
> @@ -29,3 +29,23 @@ Example:
>  		gpios = <&gpio 18 1	// device reg=<0>
>  			 &gpio 19 1>;	// device reg=<1>
>  	};
> +
> +
> +* eSPI (Enhanced Serial Peripheral Interface)
> +
> +Required properties:
> +- compatible : should be "fsl,espi".

Good practice is to always fully identify the SoC in the compatible values, followed by an optional list of other specific chips it is compatible with.  Generic compatibles like "fsl,espi" are not a good idea.

+- compatible: should be "fsl,<chip>-espi".

> +- reg : Offset and length of the register set for the device.
> +- interrupts : should contain eSPI interrupt, the device has one interrupt.
> +- fsl,espi-num-chipselects : the number of the chipselect signals.
> +
> +Example:
> +	spi@110000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "fsl,espi";
> +		reg = <0x110000 0x1000>;
> +		interrupts = <53 0x2>;
> +		interrupt-parent = <&mpic>;
> +		fsl,espi-num-chipselects = <4>;
> +	};
> -- 
> 1.6.4
> 
> 

^ permalink raw reply

* Re: [PATCH 4/6] mtd: m25p80: change the read function to read page by page
From: Grant Likely @ 2010-07-26  0:30 UTC (permalink / raw)
  To: Mingkai Hu; +Cc: linuxppc-dev
In-Reply-To: <1279591705-7574-5-git-send-email-Mingkai.hu@freescale.com>

On Tue, Jul 20, 2010 at 10:08:23AM +0800, Mingkai Hu wrote:
> For Freescale's eSPI controller, the max transaction length one time
> is limitted by the SPCOM[TRANSLEN] field which is 0x10000. When used
> mkfs.ext2 command to create ext2 filesystem on the flash, the read
> length will exceed the max value of the SPCOM[TRANSLEN] field, so
> change the read function to read page by page.
> 
> For other SPI flash driver, also needed to change the read function
> if used the eSPI controller.
> 
> Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 81e49a9..6cbe6b1 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -317,6 +317,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	struct m25p *flash = mtd_to_m25p(mtd);
>  	struct spi_transfer t[2];
>  	struct spi_message m;
> +	u32 i, page_size = 0;
>  
>  	DEBUG(MTD_DEBUG_LEVEL2, "%s: %s %s 0x%08x, len %zd\n",
>  			dev_name(&flash->spi->dev), __func__, "from",
> @@ -341,7 +342,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	spi_message_add_tail(&t[0], &m);
>  
>  	t[1].rx_buf = buf;
> -	t[1].len = len;
>  	spi_message_add_tail(&t[1], &m);
>  
>  	/* Byte count starts at zero. */
> @@ -364,11 +364,21 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>  
>  	/* Set up the write data buffer. */
>  	flash->command[0] = OPCODE_READ;
> -	m25p_addr2cmd(flash, from, flash->command);
>  
> -	spi_sync(flash->spi, &m);
> +	for (i = page_size; i < len; i += page_size) {
> +		page_size = len - i;
> +		if (page_size > flash->page_size)
> +			page_size = flash->page_size;
>  
> -	*retlen = m.actual_length - m25p_cmdsz(flash) - FAST_READ_DUMMY_BYTE;
> +		m25p_addr2cmd(flash, from + i, flash->command);
> +		t[1].len = page_size;
> +		t[1].rx_buf = buf + i;
> +
> +		spi_sync(flash->spi, &m);
> +
> +		*retlen += m.actual_length - m25p_cmdsz(flash)
> +			- FAST_READ_DUMMY_BYTE;
> +	}

This patch seems to cripple all users just because eSPI cannot handle it.  Am I reading it wrong?

>  
>  	mutex_unlock(&flash->lock);
>  
> -- 
> 1.6.4
> 
> 

^ permalink raw reply

* Re: [PATCH 3/6] of/spi: add support to parse the SPI flash's partitions
From: Grant Likely @ 2010-07-26  0:28 UTC (permalink / raw)
  To: Mingkai Hu; +Cc: linuxppc-dev
In-Reply-To: <1279591705-7574-4-git-send-email-Mingkai.hu@freescale.com>

On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
> Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com>
> ---
>  drivers/of/of_spi.c       |   11 +++++++++++
>  drivers/spi/spi_mpc8xxx.c |    1 +
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c
> index 5fed7e3..284ca0e 100644
> --- a/drivers/of/of_spi.c
> +++ b/drivers/of/of_spi.c
> @@ -10,6 +10,8 @@
>  #include <linux/device.h>
>  #include <linux/spi/spi.h>
>  #include <linux/of_spi.h>
> +#include <linux/spi/flash.h>
> +#include <linux/mtd/partitions.h>
>  
>  /**
>   * of_register_spi_devices - Register child devices onto the SPI bus
> @@ -26,6 +28,7 @@ void of_register_spi_devices(struct spi_master *master, struct device_node *np)
>  	const __be32 *prop;
>  	int rc;
>  	int len;
> +	struct flash_platform_data *pdata;
>  
>  	for_each_child_of_node(np, nc) {
>  		/* Alloc an spi_device */
> @@ -81,6 +84,14 @@ void of_register_spi_devices(struct spi_master *master, struct device_node *np)
>  		of_node_get(nc);
>  		spi->dev.of_node = nc;
>  
> +		/* Parse the mtd partitions */
> +		pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return;
> +		pdata->nr_parts = of_mtd_parse_partitions(&master->dev,
> +				nc, &pdata->parts);
> +		spi->dev.platform_data = pdata;
> +

Nack.  Not all spi devices are mtd devices.  In fact, most are not.

The spi driver itself should call the of_mtd_parse_partitions code to get the partition map.  Do not use pdata in this case.

>  		/* Register the new device */
>  		request_module(spi->modalias);
>  		rc = spi_add_device(spi);
> diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> index efed70e..0fadaeb 100644
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -137,6 +137,7 @@ int mpc8xxx_spi_transfer(struct spi_device *spi,
>  
>  void mpc8xxx_spi_cleanup(struct spi_device *spi)
>  {
> +	kfree(spi->dev.platform_data);

Irrelevant given the above comment, but how does this even work?  What if a driver was detached and reattached to an spi_device?  The platform_data would be freed.  Not to mention that the pointer isn't cleared, so the driver would have no idea that it has a freed pointer.

>  	kfree(spi->controller_state);
>  }
>  
> -- 
> 1.6.4
> 
> 

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox