| Message ID | 1465245341-16026-1-git-send-email-hauke@hauke-m.de |
|---|---|
| State | Rejected |
| Delegated to: | Brian Norris |
| Headers | show |
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
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
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
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
> > 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
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
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))