[PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Hauke Mehrtens]

From: John Crispin <john@phrozen.org>

This uses the same device tree attribute as physmap_of.c

Signed-off-by: John Crispin <john@phrozen.org>
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/mtd/maps/lantiq-flash.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/maps/lantiq-flash.c b/drivers/mtd/maps/lantiq-flash.c
index c8febb3..88fc6d9 100644
--- a/drivers/mtd/maps/lantiq-flash.c
+++ b/drivers/mtd/maps/lantiq-flash.c
@@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
 	if (!ltq_mtd->map)
 		return -ENOMEM;
 
-	ltq_mtd->map->phys = ltq_mtd->res->start;
+	if (of_find_property(pdev->dev.of_node, "no-unaligned-direct-access",
+			     NULL))
+		ltq_mtd->map->phys = NO_XIP;
+	else
+		ltq_mtd->map->phys = ltq_mtd->res->start;
 	ltq_mtd->map->size = resource_size(ltq_mtd->res);
 	ltq_mtd->map->virt = devm_ioremap_resource(&pdev->dev, ltq_mtd->res);
 	if (IS_ERR(ltq_mtd->map->virt))
-- 
2.8.1

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Jonas Gorski]

Hi,

On 6 June 2016 at 22:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> From: John Crispin <john@phrozen.org>
>
> This uses the same device tree attribute as physmap_of.c
>
> Signed-off-by: John Crispin <john@phrozen.org>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/mtd/maps/lantiq-flash.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/maps/lantiq-flash.c b/drivers/mtd/maps/lantiq-flash.c
> index c8febb3..88fc6d9 100644
> --- a/drivers/mtd/maps/lantiq-flash.c
> +++ b/drivers/mtd/maps/lantiq-flash.c
> @@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
>         if (!ltq_mtd->map)
>                 return -ENOMEM;
>
> -       ltq_mtd->map->phys = ltq_mtd->res->start;
> +       if (of_find_property(pdev->dev.of_node, "no-unaligned-direct-access",
> +                            NULL))
> +               ltq_mtd->map->phys = NO_XIP;

This does not what you think it does; i.e. preventing unaligned io
memory accesses. All it's used for is in two drivers
(cfi_cmdset_0001.c, lpddr_cmds.c) as a check whether to populate
_point/_unpoint. But it does not prevent the map_read/map_write
accessors from doing memcpy_{from,to}_io() with unaligned addresses,
which at least on MIPS results in unaligned accesses with LWL/LWR.
Which is what I assume you want to prevent.


Regards
Jonas

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Hauke Mehrtens]

On 06/06/2016 11:10 PM, Jonas Gorski wrote:
> Hi,
> 
> On 6 June 2016 at 22:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> From: John Crispin <john@phrozen.org>
>>
>> This uses the same device tree attribute as physmap_of.c
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/mtd/maps/lantiq-flash.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/maps/lantiq-flash.c b/drivers/mtd/maps/lantiq-flash.c
>> index c8febb3..88fc6d9 100644
>> --- a/drivers/mtd/maps/lantiq-flash.c
>> +++ b/drivers/mtd/maps/lantiq-flash.c
>> @@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
>>         if (!ltq_mtd->map)
>>                 return -ENOMEM;
>>
>> -       ltq_mtd->map->phys = ltq_mtd->res->start;
>> +       if (of_find_property(pdev->dev.of_node, "no-unaligned-direct-access",
>> +                            NULL))
>> +               ltq_mtd->map->phys = NO_XIP;
> 
> This does not what you think it does; i.e. preventing unaligned io
> memory accesses. All it's used for is in two drivers
> (cfi_cmdset_0001.c, lpddr_cmds.c) as a check whether to populate
> _point/_unpoint. But it does not prevent the map_read/map_write
> accessors from doing memcpy_{from,to}_io() with unaligned addresses,
> which at least on MIPS results in unaligned accesses with LWL/LWR.
> Which is what I assume you want to prevent.

Hi Jonas,

I want to upstream the patch from OpenWrt:
https://dev.openwrt.org/changeset/35992

I want to deactivate the execute-in-place (XIP) which this property does.

Hauke

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Jonas Gorski]

On 6 June 2016 at 23:38, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 06/06/2016 11:10 PM, Jonas Gorski wrote:
>> Hi,
>>
>> On 6 June 2016 at 22:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>> From: John Crispin <john@phrozen.org>
>>>
>>> This uses the same device tree attribute as physmap_of.c
>>>
>>> Signed-off-by: John Crispin <john@phrozen.org>
>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>> ---
>>>  drivers/mtd/maps/lantiq-flash.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/maps/lantiq-flash.c b/drivers/mtd/maps/lantiq-flash.c
>>> index c8febb3..88fc6d9 100644
>>> --- a/drivers/mtd/maps/lantiq-flash.c
>>> +++ b/drivers/mtd/maps/lantiq-flash.c
>>> @@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
>>>         if (!ltq_mtd->map)
>>>                 return -ENOMEM;
>>>
>>> -       ltq_mtd->map->phys = ltq_mtd->res->start;
>>> +       if (of_find_property(pdev->dev.of_node, "no-unaligned-direct-access",
>>> +                            NULL))
>>> +               ltq_mtd->map->phys = NO_XIP;
>>
>> This does not what you think it does; i.e. preventing unaligned io
>> memory accesses. All it's used for is in two drivers
>> (cfi_cmdset_0001.c, lpddr_cmds.c) as a check whether to populate
>> _point/_unpoint. But it does not prevent the map_read/map_write
>> accessors from doing memcpy_{from,to}_io() with unaligned addresses,
>> which at least on MIPS results in unaligned accesses with LWL/LWR.
>> Which is what I assume you want to prevent.
>
> Hi Jonas,
>
> I want to upstream the patch from OpenWrt:
> https://dev.openwrt.org/changeset/35992
>
> I want to deactivate the execute-in-place (XIP) which this property does.

I understand that, but I wanted to point out that setting NO_XIP does
neither prevent XIP*, nor does it prevent unaligned direct accesses.
Which are also IMHO two different concepts.

NO_XIP is checked at exactly one place, by mtd_is_linear()[1] (and
only if CONFIG_MTD_COMPLEX_MAPPINGS is enabled). And mtd_is_linear()
is only used by the two mentioned drivers, nothing more. Unless I'm
overlooking something, this patch has no obvservebal effect, as the
mtd code doesn't actually do anything meaningful with NO_XIP.


Regards
Jonas

* actually AFAICT XIP isn't supported at all, at least not on MIPS.

[1] http://lxr.free-electrons.com/source/include/linux/mtd/map.h#L477

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[John Crispin]

On 06/06/2016 23:54, Jonas Gorski wrote:
> On 6 June 2016 at 23:38, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> On 06/06/2016 11:10 PM, Jonas Gorski wrote:
>>> Hi,
>>>
>>> On 6 June 2016 at 22:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>>> From: John Crispin <john@phrozen.org>
>>>>
>>>> This uses the same device tree attribute as physmap_of.c
>>>>
>>>> Signed-off-by: John Crispin <john@phrozen.org>
>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>> ---
>>>>  drivers/mtd/maps/lantiq-flash.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/maps/lantiq-flash.c b/drivers/mtd/maps/lantiq-flash.c
>>>> index c8febb3..88fc6d9 100644
>>>> --- a/drivers/mtd/maps/lantiq-flash.c
>>>> +++ b/drivers/mtd/maps/lantiq-flash.c
>>>> @@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
>>>>         if (!ltq_mtd->map)
>>>>                 return -ENOMEM;
>>>>
>>>> -       ltq_mtd->map->phys = ltq_mtd->res->start;
>>>> +       if (of_find_property(pdev->dev.of_node, "no-unaligned-direct-access",
>>>> +                            NULL))
>>>> +               ltq_mtd->map->phys = NO_XIP;
>>>
>>> This does not what you think it does; i.e. preventing unaligned io
>>> memory accesses. All it's used for is in two drivers
>>> (cfi_cmdset_0001.c, lpddr_cmds.c) as a check whether to populate
>>> _point/_unpoint. But it does not prevent the map_read/map_write
>>> accessors from doing memcpy_{from,to}_io() with unaligned addresses,
>>> which at least on MIPS results in unaligned accesses with LWL/LWR.
>>> Which is what I assume you want to prevent.
>>
>> Hi Jonas,
>>
>> I want to upstream the patch from OpenWrt:
>> https://dev.openwrt.org/changeset/35992
>>
>> I want to deactivate the execute-in-place (XIP) which this property does.
> 
> I understand that, but I wanted to point out that setting NO_XIP does
> neither prevent XIP*, nor does it prevent unaligned direct accesses.
> Which are also IMHO two different concepts.
> 
> NO_XIP is checked at exactly one place, by mtd_is_linear()[1] (and
> only if CONFIG_MTD_COMPLEX_MAPPINGS is enabled). And mtd_is_linear()
> is only used by the two mentioned drivers, nothing more. Unless I'm
> overlooking something, this patch has no obvservebal effect, as the
> mtd code doesn't actually do anything meaningful with NO_XIP.
> 

this patch makes the chip use the fixup_use_point() code. adding matti
to the loop, this patch was originally written by him if i am not mistaken

	John

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Matti Laakso]

> > Subject: Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash
> > To: jogo@openwrt.org; hauke@hauke-m.de
> > CC: linux-mtd@lists.infradead.org; dwmw2@infradead.org; 
> malaakso@elisanet.fi
> > From: john@phrozen.org
> > Date: Tue, 7 Jun 2016 07:15:42 +0200
> >
> >
> >
> > On 06/06/2016 23:54, Jonas Gorski wrote:
> > > On 6 June 2016 at 23:38, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> > >> On 06/06/2016 11:10 PM, Jonas Gorski wrote:
> > >>> Hi,
> > >>>
> > >>> On 6 June 2016 at 22:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> > >>>> From: John Crispin <john@phrozen.org>
> > >>>>
> > >>>> This uses the same device tree attribute as physmap_of.c
> > >>>>
> > >>>> Signed-off-by: John Crispin <john@phrozen.org>
> > >>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> > >>>> ---
> > >>>> drivers/mtd/maps/lantiq-flash.c | 6 +++++-
> > >>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/mtd/maps/lantiq-flash.c 
> b/drivers/mtd/maps/lantiq-flash.c
> > >>>> index c8febb3..88fc6d9 100644
> > >>>> --- a/drivers/mtd/maps/lantiq-flash.c
> > >>>> +++ b/drivers/mtd/maps/lantiq-flash.c
> > >>>> @@ -137,7 +137,11 @@ ltq_mtd_probe(struct platform_device *pdev)
> > >>>> if (!ltq_mtd->map)
> > >>>> return -ENOMEM;
> > >>>>
> > >>>> - ltq_mtd->map->phys = ltq_mtd->res->start;
> > >>>> + if (of_find_property(pdev->dev.of_node, 
> "no-unaligned-direct-access",
> > >>>> + NULL))
> > >>>> + ltq_mtd->map->phys = NO_XIP;
> > >>>
> > >>> This does not what you think it does; i.e. preventing unaligned io
> > >>> memory accesses. All it's used for is in two drivers
> > >>> (cfi_cmdset_0001.c, lpddr_cmds.c) as a check whether to populate
> > >>> _point/_unpoint. But it does not prevent the map_read/map_write
> > >>> accessors from doing memcpy_{from,to}_io() with unaligned addresses,
> > >>> which at least on MIPS results in unaligned accesses with LWL/LWR.
> > >>> Which is what I assume you want to prevent.
> > >>
> > >> Hi Jonas,
> > >>
> > >> I want to upstream the patch from OpenWrt:
> > >> https://dev.openwrt.org/changeset/35992
> > >>
> > >> I want to deactivate the execute-in-place (XIP) which this 
> property does.
> > >
> > > I understand that, but I wanted to point out that setting NO_XIP does
> > > neither prevent XIP*, nor does it prevent unaligned direct accesses.
> > > Which are also IMHO two different concepts.
> > >
> > > NO_XIP is checked at exactly one place, by mtd_is_linear()[1] (and
> > > only if CONFIG_MTD_COMPLEX_MAPPINGS is enabled). And mtd_is_linear()
> > > is only used by the two mentioned drivers, nothing more. Unless I'm
> > > overlooking something, this patch has no obvservebal effect, as the
> > > mtd code doesn't actually do anything meaningful with NO_XIP.
> > >
> >
> > this patch makes the chip use the fixup_use_point() code. adding matti
> > to the loop, this patch was originally written by him if i am not 
> mistaken
> >
> > John

Hi,

I did indeed write this patch. To give some background, this is needed 
only in one device that I know of, a Lantiq Danube -based router with an 
Intel command set flash. All other devices based on this SoC use AMD 
command set flash, and don't need this workaround, unless 
cfi_cmdset_0002.c some day starts populating _point/_unpoint. JFFS2 code 
uses _point and then accesses flash directly during scan, and this 
triggers a data bus error due to some additional alignment requirements 
in the EBU (bus which the flash is connected to).

To be honest I'm not at all familiar with the exact alignment 
requirements of Xway EBU, and I had a hunch that this probably isn't the 
correct way to fix things when I wrote this patch.

Best regards,
Matti Laakso

Re: [PATCH v2] MTD: lantiq: handle NO_XIP on cfi0001 flash

[Brian Norris]

On Mon, Jun 06, 2016 at 11:54:52PM +0200, Jonas Gorski wrote:
> * actually AFAICT XIP isn't supported at all, at least not on MIPS.

I believe we recently determined that all mainline uses of MTD XIP are
defunct and/or broken. And I know very little about them, and care about
them even less.

Here's a piece of the last conversation I remember about it:

http://lists.infradead.org/pipermail/linux-mtd/2016-March/065997.html

Brian