t1040 IFC flash driver Extended Chip Select

t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

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=
> 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=
> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=0A=
> @@ -24,6 +24,8 @@ Properties:=0A=
>   - ranges : Each range corresponds to a single chipselect, and covers=0A=
>              the entire access window as configured.=0A=
> =0A=
> +- cspr_ext : This value sets the extended chip select for all banks.=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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/06/2016 05:57 PM, Scott Wood wrote:
> On 07/06/2016 03:23 PM, Daniel Walker wrote:
>> 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.

>
>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/07/2016 10:48 AM, Daniel Walker wrote:=0A=
> On 07/06/2016 05:57 PM, Scott Wood wrote:=0A=
>> 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 i=
t=0A=
>>> 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=
>>> 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=
>>> 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=
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt=
=0A=
>>> @@ -24,6 +24,8 @@ Properties:=0A=
>>>    - ranges : Each range corresponds to a single chipselect, and covers=
=0A=
>>>               the entire access window as configured.=0A=
>>>=0A=
>>> +- cspr_ext : This value sets the extended chip select for all banks.=
=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 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=
>> 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=
> =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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/07/2016 12:26 PM, Scott Wood wrote:
> On 07/07/2016 10:48 AM, Daniel Walker wrote:
>> On 07/06/2016 05:57 PM, Scott Wood wrote:
>>> On 07/06/2016 03:23 PM, Daniel Walker wrote:
>>>> 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 ?


>>>> 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()
>
>>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

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=
>>>> 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=
>>> 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 upstreamabl=
e).=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 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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/07/2016 01:34 PM, Scott Wood wrote:
> On 07/07/2016 02:44 PM, Daniel Walker wrote:
>> 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 ..

>
>>>>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/07/2016 03:52 PM, Daniel Walker wrote:=0A=
> On 07/07/2016 01:34 PM, Scott Wood wrote:=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=
>> 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=
> Ok ..=0A=
> =0A=
>>=0A=
>>>>>> The information that is missing from the device tree, that currently=
=0A=
>>>>>> must come from boot software programming the registers, is the vario=
us=0A=
>>>>>> 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=
>>>>> alternative way. The only alternative we have currently is adding som=
e=0A=
>>>>> code to manually set the values but it's not ideal (and not upstreama=
ble).=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 wr=
ite.=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=
>> 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=
> 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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

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;
+                       };
+               };
+       };
+

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=
> On 07/07/2016 02:23 PM, Scott Wood wrote:=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=
>> 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=
> =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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/07/2016 02:59 PM, Scott Wood wrote:
> On 07/07/2016 04:49 PM, Daniel Walker wrote:
>> 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,
> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=
> On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=
>> On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=
>>> On 07/07/2016 02:23 PM, Scott Wood wrote:=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=
>>>> What specifically is the problem you're having?  Is it that CSPR_EXT i=
s=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=
>>> 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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/07/2016 03:37 PM, Scott Wood wrote:
> On 07/07/2016 05:01 PM, Daniel Walker wrote:
>> On 07/07/2016 02:59 PM, Scott Wood wrote:
>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:
>>>> 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,
>>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=
> On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=
>> On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=
>>> On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=
>>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=
>>>>> On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=
>>>>>> I suspect that add the usage of cspr_ext into the driver would fix t=
he=0A=
>>>>>> 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=
>>>>>> 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 la=
ck=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 tha=
t's=0A=
>>>>>> the case, so I doubt adding CSPR_EXT matching to the driver will sol=
ve=0A=
>>>>>> 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=
>>> 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=
>> 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=
> =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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/08/2016 06:12 PM, Scott Wood wrote:
> On 07/07/2016 06:48 PM, Daniel Walker wrote:
>> On 07/07/2016 03:37 PM, Scott Wood wrote:
>>> On 07/07/2016 05:01 PM, Daniel Walker wrote:
>>>> On 07/07/2016 02:59 PM, Scott Wood wrote:
>>>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:
>>>>>> 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,
>>>>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/11/2016 11:36 AM, Daniel Walker wrote:=0A=
> On 07/08/2016 06:12 PM, Scott Wood wrote:=0A=
>> On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=
>>> On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=
>>>> On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=
>>>>> On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=
>>>>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=
>>>>>>> On 07/07/2016 02:23 PM, Scott Wood wrote:=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=
>>>>>>>> What specifically is the problem you're having?  Is it that CSPR_E=
XT 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, a=
nd=0A=
>>>>>>>> only CSPR_EXT distinguishing them.  Since you proposed a device tr=
ee=0A=
>>>>>>>> binding that assumes all devices have the same CSPR_EXT, I doubt t=
hat's=0A=
>>>>>>>> the case, so I doubt adding CSPR_EXT matching to the driver will s=
olve=0A=
>>>>>>>> your problem.=0A=
>>>>>>>>=0A=
>>>>>>>> -Scott=0A=
>>>>>>>>=0A=
>>>>>>> I didn't do the debug on this. From my perspective it's either flas=
h=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=
>>>>> So from u-boot perspective, the values in the device tree under "rang=
es"=0A=
>>>>> or parts of it, are place into the cspr and cspr_ext ? Is that how it=
's=0A=
>>>>> 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=
>>>> the address in the device tree, but U-Boot doesn't get them from the=
=0A=
>>>> device tree.=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=
>> 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=
> =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=

Re: t1040 IFC flash driver Extended Chip Select

[Daniel Walker]

On 07/11/2016 09:55 AM, Scott Wood wrote:
> On 07/11/2016 11:36 AM, Daniel Walker wrote:
>> On 07/08/2016 06:12 PM, Scott Wood wrote:
>>> On 07/07/2016 06:48 PM, Daniel Walker wrote:
>>>> On 07/07/2016 03:37 PM, Scott Wood wrote:
>>>>> On 07/07/2016 05:01 PM, Daniel Walker wrote:
>>>>>> On 07/07/2016 02:59 PM, Scott Wood wrote:
>>>>>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:
>>>>>>>> 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,
>>>>>>> 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

Re: t1040 IFC flash driver Extended Chip Select

[Scott Wood]

On 07/11/2016 12:10 PM, Daniel Walker wrote:=0A=
> On 07/11/2016 09:55 AM, Scott Wood wrote:=0A=
>> On 07/11/2016 11:36 AM, Daniel Walker wrote:=0A=
>>> On 07/08/2016 06:12 PM, Scott Wood wrote:=0A=
>>>> On 07/07/2016 06:48 PM, Daniel Walker wrote:=0A=
>>>>> On 07/07/2016 03:37 PM, Scott Wood wrote:=0A=
>>>>>> On 07/07/2016 05:01 PM, Daniel Walker wrote:=0A=
>>>>>>> On 07/07/2016 02:59 PM, Scott Wood wrote:=0A=
>>>>>>>> On 07/07/2016 04:49 PM, Daniel Walker wrote:=0A=
>>>>>>>>> On 07/07/2016 02:23 PM, Scott Wood wrote:=0A=
>>>>>>>>>> I suspect that add the usage of cspr_ext into the driver would f=
ix the=0A=
>>>>>>>>>> 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=
>>>>>>>>>> not getting written to, and thus the device does not appear at t=
he=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 ha=
ve=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=
>>>>>>>>> I didn't do the debug on this. From my perspective it's either fl=
ash=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 sam=
e=0A=
>>>>>>>> thing as that code.=0A=
>>>>>>>>=0A=
>>>>>>> So from u-boot perspective, the values in the device tree under "ra=
nges"=0A=
>>>>>>> or parts of it, are place into the cspr and cspr_ext ? Is that how =
it's=0A=
>>>>>>> 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 ma=
tch=0A=
>>>>>> the address in the device tree, but U-Boot doesn't get them from the=
=0A=
>>>>>> device tree.=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=
>>>> 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, o=
r=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'r=
e=0A=
>>>> 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=
>>> 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=
>> 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=
> =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=