On Mon, Nov 15, 2010 at 2:09 AM, Gabor Juhos <juhosg@xxxxxxxxxxx> wrote:
> 2010.11.15. 5:04 keltezéssel, Grant Likely írta:
>> On Sun, Nov 14, 2010 at 10:03:56PM +0100, Gabor Juhos wrote:
>>>>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>>>>> +{
>>>>> + return __raw_readl(sp->base + reg);
>>>>> +}
>>>>> +
>>>>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>>>>> +{
>>>>> + __raw_writel(val, sp->base + reg);
>>>>> +}
>>>>
>>>> This is suspect. Why is __raw_{readl,writel} being used instead of
>>>> ioread32/iowrite32? The __raw versions don't provide any kind of
>>>> ordering barriers.
>>>
>>> Mainly because the resulting code is smaller, and the performance is a bit
>>> better with the use of the __raw versions. The controller is embedded into the
>>> SoC and the registers are memory mapped, so i think it is safe to access them
>>> with __raw_{readl,writel}. However I can change it if that is the preferred method.
>>>
>>
>> Smaller, yes, because it doesn't have any io barriers; but is it safe?
>> Do you know whether or not the CPU will reorder the instructions on
>> you? Being embedded into the SoC doesn't really mean anything in this
>> regard. Unless you really understand all the behaviour of the CPU and
>> bus, then the safe versions must be used.
>>
>> If you *do* really understand all the behaviour and decide it is safe
>> to use the __raw versions, then the driver needs to be well documented
>> as to the reasons why the __raw versions are safe to use.
>
> These SoCs are using the MIPS 24K core. This core is based on an in-order
> architecture, so it is safe to use the __raw versions from the CPU's side.
>
> To be honest, I have no informations about that the completion of the request is
> always in order that the request are received on the AHB bus between the CPU and
> the SPI controller. However the Atheros' reference code uses the __raw versions
> everywhere to access the registers of the built-in devices, so I assume that no
> out-of-order completion is allowed on that bus.
Ralf, what say you? I personally don't like this, and it makes for a
bad example of driver code, but I'll accept it if you say that it is
the right thing to do for MIPS device drivers. (Although I retain my
requirement that the use of __raw accessors needs to be well
documented).
g.
