Sorry, for late response.
On Saturday, October 30, 2010 1:46 AM, Oliver Hartkopp wrote:
>
> It's just a question if the driver for an Intel Chipset should/could ignore
> the endian problematic.
>
> If this is intended the driver should only appear in Kconfig depending on X86
> or little endian systems.
This driver is for only x86 processor.
I have no intention to support processor except x86.
>
> Besides this remark, the struct pch_can_regs could also be defined in a way
> that every single CAN payload data byte could be accessed directly to allow
> e.g. this code in pch_can_rx_normal():
>
> cf->data[0] = ioread8(&priv->regs->if1_data0);
> cf->data[1] = ioread8(&priv->regs->if1_data1);
> cf->data[2] = ioread8(&priv->regs->if1_data2);
> (..)
> cf->data[6] = ioread8(&priv->regs->if1_data6);
> cf->data[7] = ioread8(&priv->regs->if1_data7);
>
> This is easy to understand and additionally endian aware.
I agree. Thease codes are very simple.
I will modify like above.
>
> In opposite to this:
>
> + if (cf->can_dlc > 2) {
> + u32 data1 = *((u16 *)&cf->data[2]);
> + iowrite32(data1, &priv->regs->if2_dataa2);
> + }
>
> Which puts a little? endian u16 into the big endian register layout.
> Sorry i'm just getting curious on this register access implementation.
I think cf->data is like below
MSB----------LSB
D3-D2-D1-D0
D7-D6-D5-D4
*((u16 *)&cf->data[2]) means "D3-D2".
This order coprresponds to register order.
data register is like below
MSB----------LSB
**-**-D3-D2
** means reserved area.
Thanks,
Tomoya MORINAGA
OKI SEMICONDUCTOR CO., LTD.
On 11/09/2010 01:26 PM, Tomoya MORINAGA wrote:
>> It's just a question if the driver for an Intel Chipset should/could ignore
>> the endian problematic.
>>
>> If this is intended the driver should only appear in Kconfig depending on X86
>> or little endian systems.
>
> This driver is for only x86 processor.
> I have no intention to support processor except x86.
Fair enough, if you do the endianess conversion correct, either with my
proposed cpu_to_be16 or with the simpler and probably unnoticeable
slower version from Oliver. With Olivers version it's a bit harder to
build a loop that just copies the number of bytes specified in dlc.
>> Besides this remark, the struct pch_can_regs could also be defined in a way
>> that every single CAN payload data byte could be accessed directly to allow
>> e.g. this code in pch_can_rx_normal():
>>
>> cf->data[0] = ioread8(&priv->regs->if1_data0);
>> cf->data[1] = ioread8(&priv->regs->if1_data1);
>> cf->data[2] = ioread8(&priv->regs->if1_data2);
>> (..)
>> cf->data[6] = ioread8(&priv->regs->if1_data6);
>> cf->data[7] = ioread8(&priv->regs->if1_data7);
>>
>> This is easy to understand and additionally endian aware.
>
> I agree. Thease codes are very simple.
> I will modify like above.
>
>>
>> In opposite to this:
>>
>> + if (cf->can_dlc > 2) {
>> + u32 data1 = *((u16 *)&cf->data[2]);
>> + iowrite32(data1, &priv->regs->if2_dataa2);
>> + }
>>
>> Which puts a little? endian u16 into the big endian register layout.
>> Sorry i'm just getting curious on this register access implementation.
>
> I think cf->data is like below
> MSB----------LSB
> D3-D2-D1-D0
> D7-D6-D5-D4
No, cf->data is an array of 8 u8, so it looks this way:
D0 D1 D2 D3 D4 D5 D6 D7
> *((u16 *)&cf->data[2]) means "D3-D2".
No, it's D2 D3. This is why you need the endianess conversion.
> This order coprresponds to register order.
> data register is like below
> MSB----------LSB
> **-**-D3-D2
>
> ** means reserved area.
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |