Re: [PATCH v5 01/12] misc: add driver for sequencer serial port

[Cyril Chemparathy <cyril-l0cyMroinI0@public.gmane.org>]

On 11/16/2010 03:35 PM, Grant Likely wrote:
> On Tue, Nov 16, 2010 at 9:15 AM, Cyril Chemparathy <cyril-l0cyMroinI0@public.gmane.org> wrote:
>> On 11/16/2010 02:10 AM, Grant Likely wrote:
>>> On Mon, Nov 15, 2010 at 02:12:03PM -0500, Cyril Chemparathy wrote:
>>>> TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port
>>>> device.  It has a built-in programmable execution engine that can be programmed
>>>> to operate as almost any serial bus (I2C, SPI, EasyScale, and others).
>>>>
>>>> This patch adds a driver for this controller device.  The driver does not
>>>> expose a user-land interface.  Protocol drivers built on top of this layer are
>>>> expected to remain in-kernel.
>>>>
>> [...]
>>>> +static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
>>>> +{
>>>> +     __raw_writel(val, ssp->regs + reg);
>>>> +}
>>>
>>> I'm pretty sure it was resolved that __raw_ versions should not be
>>> used here.
>>
>> The endian-swap done by writel/readl are incorrect since these devices
>> are meant to be accessed native-endian at all times.
>>
>> See [1] below for Russell King's earlier response on this.  In this
>> case, I don't think memory-device ordering matters, and therefore the
>> __raw_ variants should be ok.  Should I just insert barriers into the
>> read/write wrappers here?
> 
> I'll let Russel make the decision here; but I must admit I'm puzzled.
> Are you running an ARMEB machine?  the le32_to_cpu macros should be
> no-ops on little endian.  If you do still use the __raw variants, then
> at the very least the reason for doing so must be well documented.
> 
> Personally, I'd rather see the appropriate macros added to io.h
> ioread32be()/iowrite32be() perhaps?  Or am I missing something subtle
> about the hardware behaviour?
> 

As with most of the davinci series devices, the tnetv107x SOC can
"theoretically" run both big and little endian.  Since the CPU and SOC
endian-ness are tied, on-chip peripherals are to be accessed
native-endian (i.e. without swap) at all times.

However, many of these parts do not "officially" support big-endian, and
this is the case with tnetv107x as well.  Even so, it would be best not
to break this, just in case these h/w blocks get reused on some future
big-endian capable derivative.

Further, I found this to be common practice across many davinci drivers:

$ find . -name '*davinci*' | xargs grep __raw_ | cut -d: -f1 | uniq
./drivers/input/keyboard/davinci_keyscan.c
./drivers/mtd/nand/davinci_nand.c
./drivers/mfd/davinci_voicecodec.c
./drivers/i2c/busses/i2c-davinci.c
./drivers/usb/musb/davinci.c
./drivers/net/davinci_mdio.c
./drivers/net/davinci_cpdma.c
./sound/soc/davinci/davinci-mcasp.c
./sound/soc/davinci/davinci-i2s.c

Regards
Cyril.