On 05/31/2013 07:55 AM, Michal Simek wrote:
> - p = (u32 *)of_get_property(op->dev.of_node, "xlnx,dcr-splb-slave-if", NULL);
> - tft_access = p ? *p : 0;
> + of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
> + &tft_access);
This is okay, but just FYI, you could instead have just used be32_to_cpup().
--
Timur Tabi
On 05/31/2013 03:05 PM, Timur Tabi wrote:
> On 05/31/2013 07:55 AM, Michal Simek wrote:
>> - p = (u32 *)of_get_property(op->dev.of_node, "xlnx,dcr-splb-slave-if", NULL);
>> - tft_access = p ? *p : 0;
>> + of_property_read_u32(op->dev.of_node, "xlnx,dcr-splb-slave-if",
>> + &tft_access);
>
> This is okay, but just FYI, you could instead have just used be32_to_cpup().
yep. I was there in v1 but then Soren suggested to use of_property_read_u32
http://lkml.org/lkml/2013/5/29/365
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 05/31/2013 07:55 AM, Michal Simek wrote:
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index f3d4a69..e27a4f6 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -131,7 +131,7 @@ struct xilinxfb_drvdata {
> dcr_host_t dcr_host;
> unsigned int dcr_len;
> #endif
> - void *fb_virt; /* virt. address of the frame buffer */
> + void __iomem *fb_virt; /* virt. address of the frame buffer */
> dma_addr_t fb_phys; /* phys. address of the frame buffer */
> int fb_alloced; /* Flag, was the fb memory alloced? */
>
> @@ -273,8 +273,10 @@ static int xilinxfb_assign(struct platform_device *pdev,
> drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize);
> } else {
> drvdata->fb_alloced = 1;
> - drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
> - &drvdata->fb_phys, GFP_KERNEL);
> + drvdata->fb_virt = (__force void __iomem *)
> + dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
> + &drvdata->fb_phys,
> + GFP_KERNEL);
I think this is wrong. At least, it would be on PowerPC.
dma_alloc_coherent() allocates regular RAM, not I/O memory. So it
should not be __iomem.
--
Timur Tabi
On 05/31/2013 03:26 PM, Timur Tabi wrote:
> On 05/31/2013 07:55 AM, Michal Simek wrote:
>
>> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
>> index f3d4a69..e27a4f6 100644
>> --- a/drivers/video/xilinxfb.c
>> +++ b/drivers/video/xilinxfb.c
>> @@ -131,7 +131,7 @@ struct xilinxfb_drvdata {
>> dcr_host_t dcr_host;
>> unsigned int dcr_len;
>> #endif
>> - void *fb_virt; /* virt. address of the frame buffer */
>> + void __iomem *fb_virt; /* virt. address of the frame buffer */
>> dma_addr_t fb_phys; /* phys. address of the frame buffer */
>> int fb_alloced; /* Flag, was the fb memory alloced? */
>>
>> @@ -273,8 +273,10 @@ static int xilinxfb_assign(struct platform_device *pdev,
>> drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize);
>> } else {
>> drvdata->fb_alloced = 1;
>> - drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
>> - &drvdata->fb_phys, GFP_KERNEL);
>> + drvdata->fb_virt = (__force void __iomem *)
>> + dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
>> + &drvdata->fb_phys,
>> + GFP_KERNEL);
>
> I think this is wrong. At least, it would be on PowerPC.
> dma_alloc_coherent() allocates regular RAM, not I/O memory. So it
> should not be __iomem.
The same is for Microblaze. Driver shares fb_virt for IO memory
and for allocated memory. The purpose of this driver wasn't
to change the driver logic just resolved sparse warnings.
The other way is also wrong.
I have compiled this driver with ppc toolchain and it should
remove sparse warnings for PPC.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Friday 31 May 2013 14:55:36 Michal Simek wrote:
> Dynamically detect endianess on IP and use
> ioread/iowrite functions instead of powerpc and microblaze
> specific out_be32.
>
> Signed-off-by: Michal Simek <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
On 05/31/2013 08:37 AM, Michal Simek wrote:
> The same is for Microblaze. Driver shares fb_virt for IO memory
> and for allocated memory. The purpose of this driver wasn't
> to change the driver logic just resolved sparse warnings.
> The other way is also wrong.
> I have compiled this driver with ppc toolchain and it should
> remove sparse warnings for PPC.
But it's not I/O memory. It's regular memory. __iomem is for
memory-mapped I/O, which is limited to a specific range of memory locations.
If sometimes you use regular memory for the framebuffer, and other times
you use real I/O memory for the framebuffer, then you should have two
different pointers.
--
Timur Tabi
On 05/31/2013 03:41 PM, Timur Tabi wrote:
> On 05/31/2013 08:37 AM, Michal Simek wrote:
>> The same is for Microblaze. Driver shares fb_virt for IO memory
>> and for allocated memory. The purpose of this driver wasn't
>> to change the driver logic just resolved sparse warnings.
>> The other way is also wrong.
>> I have compiled this driver with ppc toolchain and it should
>> remove sparse warnings for PPC.
>
> But it's not I/O memory. It's regular memory. __iomem is for
> memory-mapped I/O, which is limited to a specific range of memory locations.
>
> If sometimes you use regular memory for the framebuffer, and other times
> you use real I/O memory for the framebuffer, then you should have two
> different pointers.
I agree with you and if you like I can change it.
But there will be at least one retype because dma_alloc_coherent returns void *
but struct fb_info expects that pointer is __iomem (char __iomem *screen_base).
Patch is below.
Thanks,
Michal
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index f3d4a69..885f294 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -132,8 +132,8 @@ struct xilinxfb_drvdata {
unsigned int dcr_len;
#endif
void *fb_virt; /* virt. address of the frame buffer */
+ void __iomem *fb_virt_io; /* virt. address of the frame buffer */
dma_addr_t fb_phys; /* phys. address of the frame buffer */
- int fb_alloced; /* Flag, was the fb memory alloced? */
u8 flags; /* features of the driver */
@@ -270,24 +270,36 @@ static int xilinxfb_assign(struct platform_device *pdev,
/* Allocate the framebuffer memory */
if (pdata->fb_phys) {
drvdata->fb_phys = pdata->fb_phys;
- drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize);
+ drvdata->fb_virt_io = ioremap(pdata->fb_phys, fbsize);
+
+ if (!drvdata->fb_virt_io) {
+ dev_err(dev, "Could not allocate frame buffer memory\n");
+ rc = -ENOMEM;
+ if (drvdata->flags & BUS_ACCESS_FLAG)
+ goto err_fbmem;
+ else
+ goto err_region;
+ }
+
+ /* Clear (turn to black) the framebuffer */
+ memset_io(drvdata->fb_virt_io, 0, fbsize);
} else {
- drvdata->fb_alloced = 1;
drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
&drvdata->fb_phys, GFP_KERNEL);
- }
- if (!drvdata->fb_virt) {
- dev_err(dev, "Could not allocate frame buffer memory\n");
- rc = -ENOMEM;
- if (drvdata->flags & BUS_ACCESS_FLAG)
- goto err_fbmem;
- else
- goto err_region;
+ if (!drvdata->fb_virt_io) {
+ dev_err(dev, "Could not allocate frame buffer memory\n");
+ rc = -ENOMEM;
+ if (drvdata->flags & BUS_ACCESS_FLAG)
+ goto err_fbmem;
+ else
+ goto err_region;
+ memset(drvdata->fb_virt, 0, fbsize);
}
- /* Clear (turn to black) the framebuffer */
- memset_io((void __iomem *)drvdata->fb_virt, 0, fbsize);
+
/* Tell the hardware where the frame buffer is */
xilinx_fb_out32(drvdata, REG_FB_ADDR, drvdata->fb_phys);
@@ -307,7 +319,11 @@ static int xilinxfb_assign(struct platform_device *pdev,
/* Fill struct fb_info */
drvdata->info.device = dev;
- drvdata->info.screen_base = (void __iomem *)drvdata->fb_virt;
+ if (drvdata->fb_virt)
+ drvdata->info.screen_base = (__force void __iomem *)
+ drvdata->fb_virt;
+ else
+ drvdata->info.screen_base = drvdata->fb_virt_io;
drvdata->info.fbops = &xilinxfb_ops;
drvdata->info.fix = xilinx_fb_fix;
drvdata->info.fix.smem_start = drvdata->fb_phys;
@@ -341,8 +357,8 @@ static int xilinxfb_assign(struct platform_device *pdev,
if (drvdata->flags & BUS_ACCESS_FLAG) {
/* Put a banner in the log (for DEBUG) */
- dev_dbg(dev, "regs: phys=%x, virt=%p\n", drvdata->regs_phys,
- drvdata->regs);
+ dev_dbg(dev, "regs: phys=%x, virt=%p\n",
+ (u32)drvdata->regs_phys, drvdata->regs);
}
/* Put a banner in the log (for DEBUG) */
dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
@@ -354,11 +370,11 @@ err_regfb:
fb_dealloc_cmap(&drvdata->info.cmap);
err_cmap:
- if (drvdata->fb_alloced)
+ if (drvdata->fb_virt)
dma_free_coherent(dev, PAGE_ALIGN(fbsize), drvdata->fb_virt,
drvdata->fb_phys);
else
- iounmap(drvdata->fb_virt);
+ iounmap(drvdata->fb_virt_io);
/* Turn off the display */
xilinx_fb_out32(drvdata, REG_CTRL, 0);
@@ -386,11 +402,11 @@ static int xilinxfb_release(struct device *dev)
fb_dealloc_cmap(&drvdata->info.cmap);
- if (drvdata->fb_alloced)
+ if (drvdata->fb_virt)
dma_free_coherent(dev, PAGE_ALIGN(drvdata->info.fix.smem_len),
drvdata->fb_virt, drvdata->fb_phys);
else
- iounmap(drvdata->fb_virt);
+ iounmap(drvdata->fb_virt_io);
/* Turn off the display */
xilinx_fb_out32(drvdata, REG_CTRL, 0);
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Friday 31 May 2013 16:22:24 Michal Simek wrote:
> @@ -307,7 +319,11 @@ static int xilinxfb_assign(struct platform_device *pdev,
>
> /* Fill struct fb_info */
> drvdata->info.device = dev;
> - drvdata->info.screen_base = (void __iomem *)drvdata->fb_virt;
> + if (drvdata->fb_virt)
> + drvdata->info.screen_base = (__force void __iomem *)
> + drvdata->fb_virt;
> + else
> + drvdata->info.screen_base = drvdata->fb_virt_io;
Yes, unfortunately, this is what all other frame buffer drivers do
at the moment. It is technically not correct, but most architectures
are able to call readl/writel on regular memory, or dereference
__iomem tokens, so we often get away with it. It's probably not
worth fixing it in the fbdev code base as that would be a huge
change, and people are migrating to DRM/KMS.
Arnd
On 05/31/2013 09:56 AM, Arnd Bergmann wrote:
> Yes, unfortunately, this is what all other frame buffer drivers do
> at the moment. It is technically not correct, but most architectures
> are able to call readl/writel on regular memory, or dereference
> __iomem tokens, so we often get away with it. It's probably not
> worth fixing it in the fbdev code base as that would be a huge
> change, and people are migrating to DRM/KMS.
But why bother fixing this bug if it just makes things worse? Sparse is
supposed to warn us about bad code. This patch doesn't fix the bug, it
just masks the warnings!
--
Timur Tabi
On Friday 31 May 2013 10:06:43 Timur Tabi wrote:
> On 05/31/2013 09:56 AM, Arnd Bergmann wrote:
> > Yes, unfortunately, this is what all other frame buffer drivers do
> > at the moment. It is technically not correct, but most architectures
> > are able to call readl/writel on regular memory, or dereference
> > __iomem tokens, so we often get away with it. It's probably not
> > worth fixing it in the fbdev code base as that would be a huge
> > change, and people are migrating to DRM/KMS.
>
> But why bother fixing this bug if it just makes things worse? Sparse is
> supposed to warn us about bad code. This patch doesn't fix the bug, it
> just masks the warnings!
Yes, good point. It's probably best cast the ioremap() output to
a regular pointer here, as that is actually just uncached RAM,
not an MMIO register.
Arnd
On Friday 31 May 2013 16:22:24 Michal Simek wrote:
> if (pdata->fb_phys) {
> drvdata->fb_phys = pdata->fb_phys;
> - drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize);
> + drvdata->fb_virt_io = ioremap(pdata->fb_phys, fbsize);
> +
> + if (!drvdata->fb_virt_io) {
> + dev_err(dev, "Could not allocate frame buffer memory\n");
> + rc = -ENOMEM;
> + if (drvdata->flags & BUS_ACCESS_FLAG)
> + goto err_fbmem;
> + else
> + goto err_region;
> + }
> +
> + /* Clear (turn to black) the framebuffer */
> + memset_io(drvdata->fb_virt_io, 0, fbsize);
> } else {
> - drvdata->fb_alloced = 1;
> drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize),
> &drvdata->fb_phys, GFP_KERNEL);
> - }
>
I think you also want to use ioremap_wc or dma_alloc_writecombine
here, to get a write-combining mapping, rather than a device mapping
that you would use for MMIO register access.
There is also a builtin assumption in the code above that the DMA
address space pointer (which you pass into REG_FB_ADDR) is the
same as what you pass into drvdata->info.fix.smem_start. That is
not the case in general, but I don't see a good way around it
when pdata->fb_phys is set by the platform to something outside
of system memory. It should probably have a comment next to it.
Arnd
On 05/31/2013 05:29 PM, Arnd Bergmann wrote:
> On Friday 31 May 2013 10:06:43 Timur Tabi wrote:
>> On 05/31/2013 09:56 AM, Arnd Bergmann wrote:
>>> Yes, unfortunately, this is what all other frame buffer drivers do
>>> at the moment. It is technically not correct, but most architectures
>>> are able to call readl/writel on regular memory, or dereference
>>> __iomem tokens, so we often get away with it. It's probably not
>>> worth fixing it in the fbdev code base as that would be a huge
>>> change, and people are migrating to DRM/KMS.
>>
>> But why bother fixing this bug if it just makes things worse? Sparse is
>> supposed to warn us about bad code. This patch doesn't fix the bug, it
>> just masks the warnings!
>
> Yes, good point. It's probably best cast the ioremap() output to
> a regular pointer here, as that is actually just uncached RAM,
> not an MMIO register.
ok. It means I will just remove this patch from this patchset.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform