On Thursday 30 May 2013 11:41:01 Michal Simek wrote:
> * To perform the read/write on the registers we need to check on
> * which bus its connected and call the appropriate write API.
> */
> -static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
> +static void xilinx_fb_out32(struct xilinxfb_drvdata *drvdata, u32 offset,
> u32 val)
> {
> if (drvdata->flags & PLB_ACCESS_FLAG)
> - out_be32(drvdata->regs + (offset << 2), val);
> + __raw_writel(val, drvdata->regs + (offset << 2));
> #ifdef CONFIG_PPC_DCR
> else
> dcr_write(drvdata->dcr_host, offset, val);
>
This is probably missing barriers, and is wrong on systems on which
the endianess of the device is different from the CPU.
You already have an indirection in there, so I guess it won't hurt
to create a third case for little-endian registers and add
another bit in drvdata->flags, or make it depend on the architecture,
if the endianess of the device registers is known at compile time.
Arnd
On Thu, May 30, 2013 at 5:04 PM, Arnd Bergmann <[email protected]> wrote:
>
> This is probably missing barriers, and is wrong on systems on which
> the endianess of the device is different from the CPU.
I suggest what was done in fsl_ssi.c:
#ifdef PPC
#define read_ssi(addr) in_be32(addr)
#define write_ssi(val, addr) out_be32(addr, val)
#define write_ssi_mask(addr, clear, set) clrsetbits_be32(addr, clear, set)
#elif defined ARM
#define read_ssi(addr) readl(addr)
#define write_ssi(val, addr) writel(val, addr)
/*
* FIXME: Proper locking should be added at write_ssi_mask caller level
* to ensure this register read/modify/write sequence is race free.
*/
static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set)
{
u32 val = readl(addr);
val = (val & ~clear) | set;
writel(val, addr);
}
#endif
On 05/31/2013 12:04 AM, Arnd Bergmann wrote:
> On Thursday 30 May 2013 11:41:01 Michal Simek wrote:
>> * To perform the read/write on the registers we need to check on
>> * which bus its connected and call the appropriate write API.
>> */
>> -static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
>> +static void xilinx_fb_out32(struct xilinxfb_drvdata *drvdata, u32 offset,
>> u32 val)
>> {
>> if (drvdata->flags & PLB_ACCESS_FLAG)
>> - out_be32(drvdata->regs + (offset << 2), val);
>> + __raw_writel(val, drvdata->regs + (offset << 2));
>> #ifdef CONFIG_PPC_DCR
>> else
>> dcr_write(drvdata->dcr_host, offset, val);
>>
>
> This is probably missing barriers, and is wrong on systems on which
> the endianess of the device is different from the CPU.
>
> You already have an indirection in there, so I guess it won't hurt
> to create a third case for little-endian registers and add
> another bit in drvdata->flags, or make it depend on the architecture,
> if the endianess of the device registers is known at compile time.
The PLB_ACCESS_FLAGS is incorrectly named. It means BUS_ACCESS.
But I will find a way how to autodetect endianess directly on IP
as I have done it for uartlite and will send v3.
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