t1040 IFC flash driver Extended Chip Select

Hi,

We are using the t1040 platform, and we have found that we need to 
populate this register. In the Technical Reference Manual it's 
description is section 24.3.2. This option appears in the driver, but it 
doesn't appears to be used anyplace.

We we're considered adding something to the device tree to allow 
populating this value, but I'm wondering if any of you have specific 
considerations on how this is done. Or maybe it's not needed at all, and 
we're just missing something.

(not a good patch, just an example.)

diff --git 

a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt 
b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
index 89427b0..b506001 100644

--- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt

@@ -24,6 +24,8 @@ Properties:
  - ranges : Each range corresponds to a single chipselect, and covers
             the entire access window as configured.

+- cspr_ext : This value sets the extended chip select for all banks.
+
  Child device nodes describe the devices connected to IFC such as NOR (e.g.
  cfi-flash) and NAND (fsl,ifc-nand). There might be board specific devices
  like FPGAs, CPLDs, etc.

Daniel

On 07/06/2016 03:23 PM, Daniel Walker wrote:=0A=

Hi,=0A=
=0A=
We are using the t1040 platform, and we have found that we need to =0A=
populate this register. In the Technical Reference Manual it's =0A=
description is section 24.3.2. This option appears in the driver, but it =

=0A=

doesn't appears to be used anyplace.=0A=

=0A=
I'm not sure what you mean by "in the driver".  U-boot sets these registers=
.=0A=
=0A=

We we're considered adding something to the device tree to allow =0A=
populating this value, but I'm wondering if any of you have specific =0A=
considerations on how this is done. Or maybe it's not needed at all, and =

=0A=

=0A=
I see no reason to put this here.  Boot software should be setting the=0A=
chipselect registers, and if for some reason you don't want to do that,=0A=
you could just translate the address through ranges to find the value to=0A=
write.  Why would you have the binding assume it's the same for all banks?=
=0A=
=0A=
The information that is missing from the device tree, that currently=0A=
must come from boot software programming the registers, is the various=0A=
attributes that get programmed in CSPR/CSOR.=0A=
=0A=
-Scott=0A=
=0A=

On 07/06/2016 05:57 PM, Scott Wood wrote:

On 07/06/2016 03:23 PM, Daniel Walker wrote:

quoted

Hi,

We are using the t1040 platform, and we have found that we need to
populate this register. In the Technical Reference Manual it's
description is section 24.3.2. This option appears in the driver, but it
doesn't appears to be used anyplace.

I'm not sure what you mean by "in the driver".  U-boot sets these registers.

My hardware doesn't use u-boot , and it doesn't set these values. 
However, there are other reasons to have this. For example, you use 
u-boot but it's got a defect which sets the values incorrectly. You may 
not be able to update your bootloader on a shipped product.

quoted

We we're considered adding something to the device tree to allow
populating this value, but I'm wondering if any of you have specific
considerations on how this is done. Or maybe it's not needed at all, and
we're just missing something.

(not a good patch, just an example.)

diff --git
a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
index 89427b0..b506001 100644

--- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt

@@ -24,6 +24,8 @@ Properties:
   - ranges : Each range corresponds to a single chipselect, and covers
              the entire access window as configured.

+- cspr_ext : This value sets the extended chip select for all banks.

I see no reason to put this here.  Boot software should be setting the
chipselect registers, and if for some reason you don't want to do that,
you could just translate the address through ranges to find the value to
write.  Why would you have the binding assume it's the same for all banks?

It was only an example, to start the conversation. I don't understand 
what you mean by translating the address thru ranges?

The information that is missing from the device tree, that currently
must come from boot software programming the registers, is the various
attributes that get programmed in CSPR/CSOR.

Like I said mine doesn't do this, so it's required that it be set in an 
alternative way. The only alternative we have currently is adding some 
code to manually set the values but it's not ideal (and not upstreamable).

Daniel

On 07/07/2016 12:26 PM, Scott Wood wrote:

On 07/07/2016 10:48 AM, Daniel Walker wrote:

quoted

On 07/06/2016 05:57 PM, Scott Wood wrote:

quoted

On 07/06/2016 03:23 PM, Daniel Walker wrote:

quoted

Hi,

We are using the t1040 platform, and we have found that we need to
populate this register. In the Technical Reference Manual it's
description is section 24.3.2. This option appears in the driver, but it
doesn't appears to be used anyplace.

I'm not sure what you mean by "in the driver".  U-boot sets these registers.

My hardware doesn't use u-boot , and it doesn't set these values.
However, there are other reasons to have this. For example, you use
u-boot but it's got a defect which sets the values incorrectly. You may
not be able to update your bootloader on a shipped product.

How did you get to the point of shipping a product with this wrong?  Did
previous software versions not use IFC?

I don't know the details .. We have a hack in place to set the values in 
the kernel, but I don't see that as acceptable.

We could have the Linux driver initialize things, but as I said, the
missing piece from the device tree isn't this register, it's the attributes.

What else does/doesn't your loader do?  Does it set up a LAW that covers
IFC?  You only put this one register here -- does your loader set up
CSPR/CSOR?  If it's just this one register that was overlooked, and you
can't update the bootloader, simplest may be to just have your board's
platform code write it.

I don't know those details .. I can say that cspr_ext is the only thing 
we need to get IFC working, the only thing we have found.

It seems natual that if cspr is in the device tree, you would also want 
cspr_ext because both are used to identify the device. The fact that 
it's missing to me is strange. As I said in my prior email, even if 
uboot sets those, you could have cases when it's wrong. Why would I not 
be able to simply change the device tree to correct it ?

quoted

quoted

quoted

We we're considered adding something to the device tree to allow
populating this value, but I'm wondering if any of you have specific
considerations on how this is done. Or maybe it's not needed at all, and
we're just missing something.

(not a good patch, just an example.)

diff --git
a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
index 89427b0..b506001 100644

--- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt

@@ -24,6 +24,8 @@ Properties:
    - ranges : Each range corresponds to a single chipselect, and covers
               the entire access window as configured.

+- cspr_ext : This value sets the extended chip select for all banks.

I see no reason to put this here.  Boot software should be setting the
chipselect registers, and if for some reason you don't want to do that,
you could just translate the address through ranges to find the value to
write.  Why would you have the binding assume it's the same for all banks?

It was only an example, to start the conversation. I don't understand
what you mean by translating the address thru ranges?

of_address_to_resource()

quoted

quoted

The information that is missing from the device tree, that currently
must come from boot software programming the registers, is the various
attributes that get programmed in CSPR/CSOR.

Like I said mine doesn't do this, so it's required that it be set in an
alternative way. The only alternative we have currently is adding some
code to manually set the values but it's not ideal (and not upstreamable).

I wouldn't have a problem merging code in a platform board file that
writes a single register that a hard-to-update bootloader forgot to write.

I can submit it to you, but I would much prefer a general solution that 
others can use without having to create board files. Our goal has been 
to reduce board files as much as possible, do you not agree with that?

Daniel

On 07/07/2016 01:34 PM, Scott Wood wrote:

On 07/07/2016 02:44 PM, Daniel Walker wrote:

quoted

It seems natual that if cspr is in the device tree, you would also want
cspr_ext because both are used to identify the device. The fact that
it's missing to me is strange. As I said in my prior email, even if
uboot sets those, you could have cases when it's wrong. Why would I not
be able to simply change the device tree to correct it ?

CSPR is not in the device tree.  The physical address of each chipselect
is in the device tree (via the ranges property on the IFC node) and that
covers both the address portion of CSPR, and CSPR_EXT.

What I do see missing from the driver is using CSPR_EXT to match the
device, most likely because the initial IFC version didn't have
CSPR_EXT.  Fixing that doesn't require a device tree change.

Ok ..

quoted

quoted

quoted

quoted

The information that is missing from the device tree, that currently
must come from boot software programming the registers, is the various
attributes that get programmed in CSPR/CSOR.

Like I said mine doesn't do this, so it's required that it be set in an
alternative way. The only alternative we have currently is adding some
code to manually set the values but it's not ideal (and not upstreamable).

I wouldn't have a problem merging code in a platform board file that
writes a single register that a hard-to-update bootloader forgot to write.

I can submit it to you, but I would much prefer a general solution that
others can use without having to create board files. Our goal has been
to reduce board files as much as possible, do you not agree with that?

I do agree that board files are not ideal, but they're still a
reasonable place to put board-specific quirks.

I don't want to put a half-measure into the main driver and pretend it's
a general solution.  If the driver is to set the address, it should also
set the rest of CSPR/CSOR, which requires that information to be added
to the device tree.  If you want to propose the latter I have no problem
with that, as long as compatibility is maintained.

I suspect that add the usage of cspr_ext into the driver would fix the 
issue we have. It reads like you would find that acceptable ?

I'm not really stuck on a particular device tree solution, but it was 
what we initial though of.

So you would support adding usage of of_address_to_resource to set the 
cspr and cspr_ext in the driver ?

Daniel

On 07/07/2016 10:48 AM, Daniel Walker wrote:=0A=

On 07/06/2016 05:57 PM, Scott Wood wrote:=0A=

quoted

On 07/06/2016 03:23 PM, Daniel Walker wrote:=0A=

quoted

Hi,=0A=
=0A=
We are using the t1040 platform, and we have found that we need to=0A=
populate this register. In the Technical Reference Manual it's=0A=
description is section 24.3.2. This option appears in the driver, but i=

t=0A=

quoted

quoted

doesn't appears to be used anyplace.=0A=

I'm not sure what you mean by "in the driver".  U-boot sets these regist=

ers.=0A=

=0A=
My hardware doesn't use u-boot , and it doesn't set these values. =0A=
However, there are other reasons to have this. For example, you use =0A=
u-boot but it's got a defect which sets the values incorrectly. You may =

=0A=

not be able to update your bootloader on a shipped product.=0A=

=0A=
How did you get to the point of shipping a product with this wrong?  Did=0A=
previous software versions not use IFC?=0A=
=0A=
We could have the Linux driver initialize things, but as I said, the=0A=
missing piece from the device tree isn't this register, it's the attributes=
.=0A=
=0A=
What else does/doesn't your loader do?  Does it set up a LAW that covers=0A=
IFC?  You only put this one register here -- does your loader set up=0A=
CSPR/CSOR?  If it's just this one register that was overlooked, and you=0A=
can't update the bootloader, simplest may be to just have your board's=0A=
platform code write it.=0A=
=0A=

quoted

quoted

We we're considered adding something to the device tree to allow=0A=
populating this value, but I'm wondering if any of you have specific=0A=
considerations on how this is done. Or maybe it's not needed at all, an=

d=0A=

quoted

quoted

we're just missing something.=0A=
=0A=
(not a good patch, just an example.)=0A=
=0A=
diff --git=0A=
a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=0A=
b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=0A=
index 89427b0..b506001 100644=0A=

--- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=

=0A=

quoted

quoted

+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=

=0A=

quoted

quoted

@@ -24,6 +24,8 @@ Properties:=0A=
   - ranges : Each range corresponds to a single chipselect, and covers=

=0A=

quoted

quoted

              the entire access window as configured.=0A=
=0A=
+- cspr_ext : This value sets the extended chip select for all banks.=

=0A=

quoted

I see no reason to put this here.  Boot software should be setting the=

=0A=

quoted

chipselect registers, and if for some reason you don't want to do that,=

=0A=

quoted

you could just translate the address through ranges to find the value to=

=0A=

quoted

write.  Why would you have the binding assume it's the same for all bank=

s?=0A=

=0A=
It was only an example, to start the conversation. I don't understand =0A=
what you mean by translating the address thru ranges?=0A=

=0A=
of_address_to_resource()=0A=
=0A=

quoted

The information that is missing from the device tree, that currently=0A=
must come from boot software programming the registers, is the various=

=0A=

quoted

attributes that get programmed in CSPR/CSOR.=0A=
=0A=

=0A=
Like I said mine doesn't do this, so it's required that it be set in an =

=0A=

alternative way. The only alternative we have currently is adding some =

=0A=

code to manually set the values but it's not ideal (and not upstreamable)=

.=0A=
=0A=
I wouldn't have a problem merging code in a platform board file that=0A=
writes a single register that a hard-to-update bootloader forgot to write.=
=0A=
=0A=
-Scott=0A=
=0A=

On 07/07/2016 02:23 PM, Scott Wood wrote:

I suspect that add the usage of cspr_ext into the driver would fix the
issue we have. It reads like you would find that acceptable ?
What specifically is the problem you're having?  Is it that CSPR_EXT is
not getting written to, and thus the device does not appear at the
address that it should?

Or is the driver matching incorrectly?  The only way the driver's lack
of using CSPR_EXT to match would be a problem would be if you have
multiple chipselects with the same address in the lower 32 bits, and
only CSPR_EXT distinguishing them.  Since you proposed a device tree
binding that assumes all devices have the same CSPR_EXT, I doubt that's
the case, so I doubt adding CSPR_EXT matching to the driver will solve
your problem.

-Scott

I didn't do the debug on this. From my perspective it's either flash 
works, or it doesn't work. We need the code below for it to work,


+#define IFC_REG_BASEADDR       0x124000
+
+static void rsp3_setup_ifc(void)
+{
+       /* set Extended Base Address for external flash chips */
+       void __iomem *ccsr_ifc;
+       ccsr_ifc = g_ccsrbp + IFC_REG_BASEADDR;
+       iowrite32be(0xF, (ccsr_ifc + 0x0C)); /* Extended Address */
+}
+

And this is our device tree blob,

+       ifc: localbus@ffb124000 {
+               #address-cells = <2>;
+               #size-cells = <1>;
+               compatible = "fsl,ifc", "simple-bus";
+               interrupts = <0x19 0x2 0x0 0x0>;
+               reg = <0xf 0xfb124000 0 0x2000>;
+               ranges = <0 0 0xf 0xfc000000 0x4000000>;
+
+               flash@0,0 {
+                       #address-cells = <1>;
+                       #size-cells = <1>;
+                       compatible = "cfi-flash";
+                       reg = <0x0 0x0 0x4000000>;
+
+                       bank-width = <2>;
+                       device-width = <1>;
+
+                       csl_s@200000 {
+                               label = "csl_s";
+                               reg = <0x20000 0x200000>;
+                       };
+
+                       space_ava@400000 {
+                               label ="Space Available";
+                               reg = <0x400000 0x2BA0000>;
+                       };
+
+                       nvram_backup@35e0000 {
+                               label ="NVRAM Backup";
+                               reg = <0x35e0000 0x20000>;
+                       };
+
+                       upgrade@3600000 {
+                               label ="Upgrade Boot Rom";
+                               reg = <0x3600000 0x200000>;
+                       };
+
+                       nvram@3de0000 {
+                               label ="NVRAM";
+                               reg = <0x3de0000 0x20000>;
+                       };
+
+                       bootrom@3e00000 {
+                               label ="Running Boot Rom";
+                               reg = <0x3e00000 0x200000>;
+                               read-only;
+                       };
+               };
+       };
+

On 07/07/2016 03:52 PM, Daniel Walker wrote:=0A=

On 07/07/2016 01:34 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 02:44 PM, Daniel Walker wrote:=0A=

quoted

It seems natual that if cspr is in the device tree, you would also want=

=0A=

quoted

quoted

cspr_ext because both are used to identify the device. The fact that=0A=
it's missing to me is strange. As I said in my prior email, even if=0A=
uboot sets those, you could have cases when it's wrong. Why would I not=

=0A=

quoted

quoted

be able to simply change the device tree to correct it ?=0A=

CSPR is not in the device tree.  The physical address of each chipselect=

=0A=

quoted

is in the device tree (via the ranges property on the IFC node) and that=

=0A=

quoted

covers both the address portion of CSPR, and CSPR_EXT.=0A=
=0A=
What I do see missing from the driver is using CSPR_EXT to match the=0A=
device, most likely because the initial IFC version didn't have=0A=
CSPR_EXT.  Fixing that doesn't require a device tree change.=0A=

=0A=
Ok ..=0A=
=0A=

quoted

=0A=

quoted

quoted

quoted

quoted

The information that is missing from the device tree, that currently=

=0A=

quoted

quoted

quoted

quoted

quoted

must come from boot software programming the registers, is the vario=

us=0A=

quoted

quoted

quoted

quoted

quoted

attributes that get programmed in CSPR/CSOR.=0A=
=0A=

Like I said mine doesn't do this, so it's required that it be set in =

an=0A=

quoted

quoted

quoted

quoted

alternative way. The only alternative we have currently is adding som=

e=0A=

quoted

quoted

quoted

quoted

code to manually set the values but it's not ideal (and not upstreama=

ble).=0A=

quoted

quoted

quoted

I wouldn't have a problem merging code in a platform board file that=

=0A=

quoted

quoted

quoted

writes a single register that a hard-to-update bootloader forgot to wr=

ite.=0A=

quoted

quoted

I can submit it to you, but I would much prefer a general solution that=

=0A=

quoted

quoted

others can use without having to create board files. Our goal has been=

=0A=

quoted

quoted

to reduce board files as much as possible, do you not agree with that?=

=0A=

quoted

I do agree that board files are not ideal, but they're still a=0A=
reasonable place to put board-specific quirks.=0A=
=0A=
I don't want to put a half-measure into the main driver and pretend it's=

=0A=

quoted

a general solution.  If the driver is to set the address, it should also=

=0A=

quoted

set the rest of CSPR/CSOR, which requires that information to be added=

=0A=

quoted

to the device tree.  If you want to propose the latter I have no problem=

=0A=

quoted

with that, as long as compatibility is maintained.=0A=

=0A=
I suspect that add the usage of cspr_ext into the driver would fix the =

=0A=

issue we have. It reads like you would find that acceptable ?=0A=

=0A=
What specifically is the problem you're having?  Is it that CSPR_EXT is=0A=
not getting written to, and thus the device does not appear at the=0A=
address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver's lack=0A=
of using CSPR_EXT to match would be a problem would be if you have=0A=
multiple chipselects with the same address in the lower 32 bits, and=0A=
only CSPR_EXT distinguishing them.  Since you proposed a device tree=0A=
binding that assumes all devices have the same CSPR_EXT, I doubt that's=0A=
the case, so I doubt adding CSPR_EXT matching to the driver will solve=0A=
your problem.=0A=
=0A=
-Scott=0A=
=0A=

On 07/07/2016 02:59 PM, Scott Wood wrote:

On 07/07/2016 04:49 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:

quoted

I suspect that add the usage of cspr_ext into the driver would fix the
issue we have. It reads like you would find that acceptable ?
What specifically is the problem you're having?  Is it that CSPR_EXT is
not getting written to, and thus the device does not appear at the
address that it should?

Or is the driver matching incorrectly?  The only way the driver's lack
of using CSPR_EXT to match would be a problem would be if you have
multiple chipselects with the same address in the lower 32 bits, and
only CSPR_EXT distinguishing them.  Since you proposed a device tree
binding that assumes all devices have the same CSPR_EXT, I doubt that's
the case, so I doubt adding CSPR_EXT matching to the driver will solve
your problem.

-Scott

I didn't do the debug on this. From my perspective it's either flash
works, or it doesn't work. We need the code below for it to work,

Adding CSPR_EXT matching to the driver will not accomplish the same
thing as that code.

So from u-boot perspective, the values in the device tree under "ranges" 
or parts of it, are place into the cspr and cspr_ext ? Is that how it's 
suppose to work ?

Daniel

On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=

On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=

quoted

I suspect that add the usage of cspr_ext into the driver would fix the=

=0A=

quoted

quoted

quoted

issue we have. It reads like you would find that acceptable ?=0A=
What specifically is the problem you're having?  Is it that CSPR_EXT i=

s=0A=

quoted

quoted

quoted

not getting written to, and thus the device does not appear at the=0A=
address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver's lack=

=0A=

quoted

quoted

quoted

of using CSPR_EXT to match would be a problem would be if you have=0A=
multiple chipselects with the same address in the lower 32 bits, and=

=0A=

quoted

quoted

quoted

only CSPR_EXT distinguishing them.  Since you proposed a device tree=

=0A=

quoted

quoted

quoted

binding that assumes all devices have the same CSPR_EXT, I doubt that'=

s=0A=

quoted

quoted

quoted

the case, so I doubt adding CSPR_EXT matching to the driver will solve=

=0A=

quoted

quoted

quoted

your problem.=0A=
=0A=
-Scott=0A=
=0A=

I didn't do the debug on this. From my perspective it's either flash=0A=
works, or it doesn't work. We need the code below for it to work,=0A=

Adding CSPR_EXT matching to the driver will not accomplish the same=0A=
thing as that code.=0A=
=0A=

=0A=
So from u-boot perspective, the values in the device tree under "ranges" =

=0A=

or parts of it, are place into the cspr and cspr_ext ? Is that how it's =

=0A=

suppose to work ?=0A=

=0A=
U-Boot writes values that are hardcoded in the board config header.=0A=
These values (as well as the area covered by the IFC LAW) need to match=0A=
the address in the device tree, but U-Boot doesn't get them from the=0A=
device tree.=0A=
=0A=
-Scott=0A=
=0A=

On 07/07/2016 02:44 PM, Daniel Walker wrote:=0A=

It seems natual that if cspr is in the device tree, you would also want =

=0A=

cspr_ext because both are used to identify the device. The fact that =0A=
it's missing to me is strange. As I said in my prior email, even if =0A=
uboot sets those, you could have cases when it's wrong. Why would I not =

=0A=

be able to simply change the device tree to correct it ?=0A=

=0A=
CSPR is not in the device tree.  The physical address of each chipselect=0A=
is in the device tree (via the ranges property on the IFC node) and that=0A=
covers both the address portion of CSPR, and CSPR_EXT.=0A=
=0A=
What I do see missing from the driver is using CSPR_EXT to match the=0A=
device, most likely because the initial IFC version didn't have=0A=
CSPR_EXT.  Fixing that doesn't require a device tree change.=0A=
=0A=

quoted

quoted

quoted

The information that is missing from the device tree, that currently=

=0A=

quoted

quoted

quoted

must come from boot software programming the registers, is the various=

=0A=

quoted

quoted

quoted

attributes that get programmed in CSPR/CSOR.=0A=
=0A=

Like I said mine doesn't do this, so it's required that it be set in an=

=0A=

quoted

quoted

alternative way. The only alternative we have currently is adding some=

=0A=

quoted

quoted

code to manually set the values but it's not ideal (and not upstreamabl=

e).=0A=

quoted

I wouldn't have a problem merging code in a platform board file that=0A=
writes a single register that a hard-to-update bootloader forgot to writ=

e.=0A=

=0A=
I can submit it to you, but I would much prefer a general solution that =

=0A=

others can use without having to create board files. Our goal has been =

=0A=

to reduce board files as much as possible, do you not agree with that?=0A=

=0A=
I do agree that board files are not ideal, but they're still a=0A=
reasonable place to put board-specific quirks.=0A=
=0A=
I don't want to put a half-measure into the main driver and pretend it's=0A=
a general solution.  If the driver is to set the address, it should also=0A=
set the rest of CSPR/CSOR, which requires that information to be added=0A=
to the device tree.  If you want to propose the latter I have no problem=0A=
with that, as long as compatibility is maintained.=0A=
=0A=
-Scott=0A=
=0A=

On 07/07/2016 03:37 PM, Scott Wood wrote:

On 07/07/2016 05:01 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:

quoted

I suspect that add the usage of cspr_ext into the driver would fix the
issue we have. It reads like you would find that acceptable ?
What specifically is the problem you're having?  Is it that CSPR_EXT is
not getting written to, and thus the device does not appear at the
address that it should?

Or is the driver matching incorrectly?  The only way the driver's lack
of using CSPR_EXT to match would be a problem would be if you have
multiple chipselects with the same address in the lower 32 bits, and
only CSPR_EXT distinguishing them.  Since you proposed a device tree
binding that assumes all devices have the same CSPR_EXT, I doubt that's
the case, so I doubt adding CSPR_EXT matching to the driver will solve
your problem.

-Scott

I didn't do the debug on this. From my perspective it's either flash
works, or it doesn't work. We need the code below for it to work,

Adding CSPR_EXT matching to the driver will not accomplish the same
thing as that code.

So from u-boot perspective, the values in the device tree under "ranges"
or parts of it, are place into the cspr and cspr_ext ? Is that how it's
suppose to work ?

U-Boot writes values that are hardcoded in the board config header.
These values (as well as the area covered by the IFC LAW) need to match
the address in the device tree, but U-Boot doesn't get them from the
device tree.

I was suggesting the values it writes are the same as the ones inside 
the device tree. So we could have both csrp and csrp_ext written from 
the driver and the values would
come from the ranges property.

Daniel

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=

On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=

quoted

=0A=
I suspect that add the usage of cspr_ext into the driver would fix the=

=0A=

quoted

issue we have. It reads like you would find that acceptable ?=0A=
What specifically is the problem you're having?  Is it that CSPR_EXT is=

=0A=

quoted

not getting written to, and thus the device does not appear at the=0A=
address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver's lack=

=0A=

quoted

of using CSPR_EXT to match would be a problem would be if you have=0A=
multiple chipselects with the same address in the lower 32 bits, and=0A=
only CSPR_EXT distinguishing them.  Since you proposed a device tree=0A=
binding that assumes all devices have the same CSPR_EXT, I doubt that's=

=0A=

quoted

the case, so I doubt adding CSPR_EXT matching to the driver will solve=

=0A=

quoted

your problem.=0A=
=0A=
-Scott=0A=
=0A=

=0A=
I didn't do the debug on this. From my perspective it's either flash =0A=
works, or it doesn't work. We need the code below for it to work,=0A=

=0A=
Adding CSPR_EXT matching to the driver will not accomplish the same=0A=
thing as that code.=0A=
=0A=
-Scott=0A=
=0A=

On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=

On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=

quoted

I suspect that add the usage of cspr_ext into the driver would fix t=

he=0A=

quoted

quoted

quoted

quoted

quoted

issue we have. It reads like you would find that acceptable ?=0A=
What specifically is the problem you're having?  Is it that CSPR_EXT=

 is=0A=

quoted

quoted

quoted

quoted

quoted

not getting written to, and thus the device does not appear at the=

=0A=

quoted

quoted

quoted

quoted

quoted

address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver's la=

ck=0A=

quoted

quoted

quoted

quoted

quoted

of using CSPR_EXT to match would be a problem would be if you have=

=0A=

quoted

quoted

quoted

quoted

quoted

multiple chipselects with the same address in the lower 32 bits, and=

=0A=

quoted

quoted

quoted

quoted

quoted

only CSPR_EXT distinguishing them.  Since you proposed a device tree=

=0A=

quoted

quoted

quoted

quoted

quoted

binding that assumes all devices have the same CSPR_EXT, I doubt tha=

t's=0A=

quoted

quoted

quoted

quoted

quoted

the case, so I doubt adding CSPR_EXT matching to the driver will sol=

ve=0A=

quoted

quoted

quoted

quoted

quoted

your problem.=0A=
=0A=
-Scott=0A=
=0A=

I didn't do the debug on this. From my perspective it's either flash=

=0A=

quoted

quoted

quoted

quoted

works, or it doesn't work. We need the code below for it to work,=0A=

Adding CSPR_EXT matching to the driver will not accomplish the same=0A=
thing as that code.=0A=
=0A=

So from u-boot perspective, the values in the device tree under "ranges=

"=0A=

quoted

quoted

or parts of it, are place into the cspr and cspr_ext ? Is that how it's=

=0A=

quoted

quoted

suppose to work ?=0A=

U-Boot writes values that are hardcoded in the board config header.=0A=
These values (as well as the area covered by the IFC LAW) need to match=

=0A=

quoted

the address in the device tree, but U-Boot doesn't get them from the=0A=
device tree.=0A=
=0A=

=0A=
I was suggesting the values it writes are the same as the ones inside =0A=
the device tree. So we could have both csrp and csrp_ext written from =0A=
the driver and the values would=0A=
come from the ranges property.=0A=

=0A=
There's more to CSPR than just the address.  The driver should either be=0A=
able to assume that all of CSPR/CSOR has been correctly initialized, or=0A=
it should assume none of that has been initialized -- which again,=0A=
requires the attribute information to be in the device tree.  If you're=0A=
doing something in between, then that's a board quirk rather than a=0A=
general solution.=0A=
=0A=
-Scott=0A=
=0A=

On 07/08/2016 06:12 PM, Scott Wood wrote:

On 07/07/2016 06:48 PM, Daniel Walker wrote:

quoted

On 07/07/2016 03:37 PM, Scott Wood wrote:

quoted

On 07/07/2016 05:01 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:

quoted

I suspect that add the usage of cspr_ext into the driver would fix the
issue we have. It reads like you would find that acceptable ?
What specifically is the problem you're having?  Is it that CSPR_EXT is
not getting written to, and thus the device does not appear at the
address that it should?

Or is the driver matching incorrectly?  The only way the driver's lack
of using CSPR_EXT to match would be a problem would be if you have
multiple chipselects with the same address in the lower 32 bits, and
only CSPR_EXT distinguishing them.  Since you proposed a device tree
binding that assumes all devices have the same CSPR_EXT, I doubt that's
the case, so I doubt adding CSPR_EXT matching to the driver will solve
your problem.

-Scott

I didn't do the debug on this. From my perspective it's either flash
works, or it doesn't work. We need the code below for it to work,

Adding CSPR_EXT matching to the driver will not accomplish the same
thing as that code.

So from u-boot perspective, the values in the device tree under "ranges"
or parts of it, are place into the cspr and cspr_ext ? Is that how it's
suppose to work ?

U-Boot writes values that are hardcoded in the board config header.
These values (as well as the area covered by the IFC LAW) need to match
the address in the device tree, but U-Boot doesn't get them from the
device tree.

I was suggesting the values it writes are the same as the ones inside
the device tree. So we could have both csrp and csrp_ext written from
the driver and the values would
come from the ranges property.

There's more to CSPR than just the address.  The driver should either be
able to assume that all of CSPR/CSOR has been correctly initialized, or
it should assume none of that has been initialized -- which again,
requires the attribute information to be in the device tree.  If you're
doing something in between, then that's a board quirk rather than a
general solution.

-Scott

It would seems like a good idea to add it then. I think it can be piece 
mail, rather than all or nothing tho. How difficult is adding the other 
part to the driver , v.s. just the cspr_ext ?

Daniel

On 07/11/2016 09:55 AM, Scott Wood wrote:

On 07/11/2016 11:36 AM, Daniel Walker wrote:

quoted

On 07/08/2016 06:12 PM, Scott Wood wrote:

quoted

On 07/07/2016 06:48 PM, Daniel Walker wrote:

quoted

On 07/07/2016 03:37 PM, Scott Wood wrote:

quoted

On 07/07/2016 05:01 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:

quoted

I suspect that add the usage of cspr_ext into the driver would fix the
issue we have. It reads like you would find that acceptable ?
What specifically is the problem you're having?  Is it that CSPR_EXT is
not getting written to, and thus the device does not appear at the
address that it should?

Or is the driver matching incorrectly?  The only way the driver's lack
of using CSPR_EXT to match would be a problem would be if you have
multiple chipselects with the same address in the lower 32 bits, and
only CSPR_EXT distinguishing them.  Since you proposed a device tree
binding that assumes all devices have the same CSPR_EXT, I doubt that's
the case, so I doubt adding CSPR_EXT matching to the driver will solve
your problem.

-Scott

I didn't do the debug on this. From my perspective it's either flash
works, or it doesn't work. We need the code below for it to work,

Adding CSPR_EXT matching to the driver will not accomplish the same
thing as that code.

So from u-boot perspective, the values in the device tree under "ranges"
or parts of it, are place into the cspr and cspr_ext ? Is that how it's
suppose to work ?

U-Boot writes values that are hardcoded in the board config header.
These values (as well as the area covered by the IFC LAW) need to match
the address in the device tree, but U-Boot doesn't get them from the
device tree.

I was suggesting the values it writes are the same as the ones inside
the device tree. So we could have both csrp and csrp_ext written from
the driver and the values would
come from the ranges property.

There's more to CSPR than just the address.  The driver should either be
able to assume that all of CSPR/CSOR has been correctly initialized, or
it should assume none of that has been initialized -- which again,
requires the attribute information to be in the device tree.  If you're
doing something in between, then that's a board quirk rather than a
general solution.

-Scott

It would seems like a good idea to add it then. I think it can be piece
mail, rather than all or nothing tho. How difficult is adding the other
part to the driver , v.s. just the cspr_ext ?

Writing only cspr_ext is a hack to work around a bug and should not be
disguised as a "piecemeal" implementation of something different.

-Scott

Ok .. How hard is it to do all the stuff your asking for ?

Daniel

On 07/11/2016 11:36 AM, Daniel Walker wrote:=0A=

On 07/08/2016 06:12 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=

quoted

I suspect that add the usage of cspr_ext into the driver would fix=

 the=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

issue we have. It reads like you would find that acceptable ?=0A=
What specifically is the problem you're having?  Is it that CSPR_E=

XT is=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

not getting written to, and thus the device does not appear at the=

=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver's =

lack=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

of using CSPR_EXT to match would be a problem would be if you have=

=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

multiple chipselects with the same address in the lower 32 bits, a=

nd=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

only CSPR_EXT distinguishing them.  Since you proposed a device tr=

ee=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

binding that assumes all devices have the same CSPR_EXT, I doubt t=

hat's=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

the case, so I doubt adding CSPR_EXT matching to the driver will s=

olve=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

your problem.=0A=
=0A=
-Scott=0A=
=0A=

I didn't do the debug on this. From my perspective it's either flas=

h=0A=

quoted

quoted

quoted

quoted

quoted

quoted

works, or it doesn't work. We need the code below for it to work,=

=0A=

quoted

quoted

quoted

quoted

quoted

Adding CSPR_EXT matching to the driver will not accomplish the same=

=0A=

quoted

quoted

quoted

quoted

quoted

thing as that code.=0A=
=0A=

So from u-boot perspective, the values in the device tree under "rang=

es"=0A=

quoted

quoted

quoted

quoted

or parts of it, are place into the cspr and cspr_ext ? Is that how it=

's=0A=

quoted

quoted

quoted

quoted

suppose to work ?=0A=

U-Boot writes values that are hardcoded in the board config header.=0A=
These values (as well as the area covered by the IFC LAW) need to matc=

h=0A=

quoted

quoted

quoted

the address in the device tree, but U-Boot doesn't get them from the=

=0A=

quoted

quoted

quoted

device tree.=0A=
=0A=

I was suggesting the values it writes are the same as the ones inside=

=0A=

quoted

quoted

the device tree. So we could have both csrp and csrp_ext written from=

=0A=

quoted

quoted

the driver and the values would=0A=
come from the ranges property.=0A=

There's more to CSPR than just the address.  The driver should either be=

=0A=

quoted

able to assume that all of CSPR/CSOR has been correctly initialized, or=

=0A=

quoted

it should assume none of that has been initialized -- which again,=0A=
requires the attribute information to be in the device tree.  If you're=

=0A=

quoted

doing something in between, then that's a board quirk rather than a=0A=
general solution.=0A=
=0A=
-Scott=0A=
=0A=

=0A=
It would seems like a good idea to add it then. I think it can be piece =

=0A=

mail, rather than all or nothing tho. How difficult is adding the other =

=0A=

part to the driver , v.s. just the cspr_ext ?=0A=

=0A=
Writing only cspr_ext is a hack to work around a bug and should not be=0A=
disguised as a "piecemeal" implementation of something different.=0A=
=0A=
-Scott=0A=

On 07/11/2016 12:10 PM, Daniel Walker wrote:=0A=

On 07/11/2016 09:55 AM, Scott Wood wrote:=0A=

quoted

On 07/11/2016 11:36 AM, Daniel Walker wrote:=0A=

quoted

On 07/08/2016 06:12 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=

quoted

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=

quoted

On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=

quoted

I suspect that add the usage of cspr_ext into the driver would f=

ix the=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

issue we have. It reads like you would find that acceptable ?=0A=
What specifically is the problem you're having?  Is it that CSPR=

_EXT is=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

not getting written to, and thus the device does not appear at t=

he=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

address that it should?=0A=
=0A=
Or is the driver matching incorrectly?  The only way the driver'=

s lack=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

of using CSPR_EXT to match would be a problem would be if you ha=

ve=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

multiple chipselects with the same address in the lower 32 bits,=

 and=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

only CSPR_EXT distinguishing them.  Since you proposed a device =

tree=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

binding that assumes all devices have the same CSPR_EXT, I doubt=

 that's=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

the case, so I doubt adding CSPR_EXT matching to the driver will=

 solve=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

your problem.=0A=
=0A=
-Scott=0A=
=0A=

I didn't do the debug on this. From my perspective it's either fl=

ash=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

works, or it doesn't work. We need the code below for it to work,=

=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Adding CSPR_EXT matching to the driver will not accomplish the sam=

e=0A=

quoted

quoted

quoted

quoted

quoted

quoted

quoted

thing as that code.=0A=
=0A=

So from u-boot perspective, the values in the device tree under "ra=

nges"=0A=

quoted

quoted

quoted

quoted

quoted

quoted

or parts of it, are place into the cspr and cspr_ext ? Is that how =

it's=0A=

quoted

quoted

quoted

quoted

quoted

quoted

suppose to work ?=0A=

U-Boot writes values that are hardcoded in the board config header.=

=0A=

quoted

quoted

quoted

quoted

quoted

These values (as well as the area covered by the IFC LAW) need to ma=

tch=0A=

quoted

quoted

quoted

quoted

quoted

the address in the device tree, but U-Boot doesn't get them from the=

=0A=

quoted

quoted

quoted

quoted

quoted

device tree.=0A=
=0A=

I was suggesting the values it writes are the same as the ones inside=

=0A=

quoted

quoted

quoted

quoted

the device tree. So we could have both csrp and csrp_ext written from=

=0A=

quoted

quoted

quoted

quoted

the driver and the values would=0A=
come from the ranges property.=0A=

There's more to CSPR than just the address.  The driver should either =

be=0A=

quoted

quoted

quoted

able to assume that all of CSPR/CSOR has been correctly initialized, o=

r=0A=

quoted

quoted

quoted

it should assume none of that has been initialized -- which again,=0A=
requires the attribute information to be in the device tree.  If you'r=

e=0A=

quoted

quoted

quoted

doing something in between, then that's a board quirk rather than a=0A=
general solution.=0A=
=0A=
-Scott=0A=
=0A=

It would seems like a good idea to add it then. I think it can be piece=

=0A=

quoted

quoted

mail, rather than all or nothing tho. How difficult is adding the other=

=0A=

quoted

quoted

part to the driver , v.s. just the cspr_ext ?=0A=

Writing only cspr_ext is a hack to work around a bug and should not be=

=0A=

quoted

disguised as a "piecemeal" implementation of something different.=0A=
=0A=
-Scott=0A=

=0A=
Ok .. How hard is it to do all the stuff your asking for ?=0A=

=0A=
It shouldn't be hard.  Add properties (at the chipselect level -- not=0A=
IFC controller) that hold the non-address portions of CSPR, CSOR, and=0A=
(if applicable to the IFC version) CSOR_EXT, as well as the chipselect=0A=
number.  If the chipselect property is present then the others must be=0A=
as well.  Code to handle this should go in drivers/memory/fsl_ifc.c=0A=
rather than the NAND driver.=0A=
=0A=
If the chipselect property is absent, then the driver will assume that=0A=
the loader has set up all chipselect registers correctly.=0A=
=0A=
-Scott=0A=
=0A=