Re: [alsa-devel] [PATCH 4/4] ASoC: fsl: make fsl_ssi driver compilable on ARM/IMX
Sergei Shtylyov wrote:
These functions are not equivalent concerning endianness. You shouldprobably have used readl_be() instead, else it won't work on PPC anymore.
readl_be() is not good enough, either. The in/out_be functions have specific syncronization instructions in them, so that the operations conform to the PowerPC instruction ordering architecture.
You probably need to do something like this:
#ifdef PPC #define read_ssi(x) in_be32(x) #else #if defined ARM #define read_ssi(x) readl(x) #endif
On Thu, Feb 23, 2012 at 09:44:37AM -0600, Timur Tabi wrote:
Sergei Shtylyov wrote:
These functions are not equivalent concerning endianness. You shouldprobably have used readl_be() instead, else it won't work on PPC anymore.
readl_be() is not good enough, either. The in/out_be functions have specific syncronization instructions in them, so that the operations conform to the PowerPC instruction ordering architecture.
Is your readl() or readl_be() unordered then? Wouldn't that be buggy between coherent DMA accesses and accessing, eg, a PCI peripheral to enable or read DMA status?
Russell King - ARM Linux wrote:
Is your readl() or readl_be() unordered then?
Yes. At least, it sure looks like it is.
Wouldn't that be buggy between coherent DMA accesses and accessing, eg, a PCI peripheral to enable or read DMA status?
I think so.
We almost never use readl. I see that it is used in some places, but honestly I can't see how it can be valid. For instance, the mpic driver uses it in mpic_startup_ht_interrupt(). I don't understand how that's valid, since there's nothing preventing the readl() from happening *before* the writeb.
I'll have to ask around, because I'm sure I'm missing something.
On Thu, Feb 23, 2012 at 11:04:59AM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote:
Is your readl() or readl_be() unordered then?
Yes. At least, it sure looks like it is.
Wouldn't that be buggy between coherent DMA accesses and accessing, eg, a PCI peripheral to enable or read DMA status?
I think so.
We almost never use readl. I see that it is used in some places, but honestly I can't see how it can be valid. For instance, the mpic driver uses it in mpic_startup_ht_interrupt(). I don't understand how that's valid, since there's nothing preventing the readl() from happening *before* the writeb.
That just sounds broken. If readl() can happen before a writeb() issued in program order, how do most of the PCI drivers work?
I'll have to ask around, because I'm sure I'm missing something.
If what you say is true, it means we have different semantics for the same accessors on different architectures. No wonder it's needing drivers to gain ifdefs to select appropriate accessors.
This needs sorting out properly. It needs the semantics of readl() etc to be fully defined, and all architectures to implement those semantics rather than what they think would be appropriate for them.
On Thu, Feb 23, 2012 at 12:14 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Thu, Feb 23, 2012 at 11:04:59AM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote: We almost never use readl. I see that it is used in some places, but honestly I can't see how it can be valid. For instance, the mpic driver uses it in mpic_startup_ht_interrupt(). I don't understand how that's valid, since there's nothing preventing the readl() from happening *before* the writeb.
That just sounds broken. If readl() can happen before a writeb() issued in program order, how do most of the PCI drivers work?
I was under the impression that on powerpc io memory was mapped cache-inhibited and guarded, which provides most ordered, but not store to load ordering without an additional synchronization instruction. See http://cache.freescale.com/files/32bit/doc/app_note/AN3441.pdf on page 8.
PCI adds some additional complexity, since the PCI device is not connected directly to the CPU. One must consider the actions of the PCI host controller.
Russell King - ARM Linux wrote:
I'll have to ask around, because I'm sure I'm missing something.
If what you say is true, it means we have different semantics for the same accessors on different architectures. No wonder it's needing drivers to gain ifdefs to select appropriate accessors.
This needs sorting out properly. It needs the semantics of readl() etc to be fully defined, and all architectures to implement those semantics rather than what they think would be appropriate for them.
So I asked around, and sure enough, my macro-fu was weak. On PowerPC, readl() maps to in_le32(). Even though PowerPC is big-endian, some devices are little endian (e.g. PCI and some PICs).
in_le32() is the byte-swapping version of in_be32(). Both functions have instruction order synchronization instructions in them.
So replacing in_be32() with readl() will break on PowerPC.
On Fri, Feb 24, 2012 at 03:29:04PM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote:
I'll have to ask around, because I'm sure I'm missing something.
If what you say is true, it means we have different semantics for the same accessors on different architectures. No wonder it's needing drivers to gain ifdefs to select appropriate accessors.
This needs sorting out properly. It needs the semantics of readl() etc to be fully defined, and all architectures to implement those semantics rather than what they think would be appropriate for them.
So I asked around, and sure enough, my macro-fu was weak. On PowerPC, readl() maps to in_le32(). Even though PowerPC is big-endian, some devices are little endian (e.g. PCI and some PICs).
in_le32() is the byte-swapping version of in_be32(). Both functions have instruction order synchronization instructions in them.
So replacing in_be32() with readl() will break on PowerPC.
But not a BE variant. Like ioread32be().
BenH tells me that he'll accept patches which converts in_32be() to ioread32be(), as in_32be() is ancient PPC cruft that needs to die.
Russell King - ARM Linux wrote:
BenH tells me that he'll accept patches which converts in_32be() to ioread32be(), as in_32be() is ancient PPC cruft that needs to die.
Where did he say this? in_be32() (and its variants) are used EVERYWHERE in PowerPC code.
Also, keep in mind that on ARM, ioread32be() is probably wrong, because we want to read the register as little-endian. The SSI audio controller is little-endian on i.MX chips and big-endian on PowerPC chip. So we really need a native-endian variant of ioread32be(). ioread32() is little-endian on PowerPC, so we can't use that.
On Fri, Feb 24, 2012 at 04:00:17PM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote:
BenH tells me that he'll accept patches which converts in_32be() to ioread32be(), as in_32be() is ancient PPC cruft that needs to die.
Where did he say this? in_be32() (and its variants) are used EVERYWHERE in PowerPC code.
It was actually Grant Likely who said it after Ben H said that drivers using in_be* is historical PPC stuff that needs fixing.
Also, keep in mind that on ARM, ioread32be() is probably wrong, because we want to read the register as little-endian. The SSI audio controller is little-endian on i.MX chips and big-endian on PowerPC chip. So we really need a native-endian variant of ioread32be(). ioread32() is little-endian on PowerPC, so we can't use that.
Ho hum. So, what that's saying to me is that even implementing in_32be() on ARM results in rubbish because it wouldn't be big endian there.
So are we heading towards driver specific sets of IO accessors? :-( It sounds like that's what it needs because of these different endian-ness issues.
And lets not forget that we're talking just about this - there's also the issue of on-SoC devices on ARM now appearing behind PCI buses on x86, which would make those appear as LE. I bet if any of those appearing on PPC would appear in BE mode. So, 'native endian' doesn't work either.
So, I'm getting the feeling that something as simple as IO accessors are just a huge mountain of crap waiting to bite.
Russell King - ARM Linux wrote:
Ho hum. So, what that's saying to me is that even implementing in_32be() on ARM results in rubbish because it wouldn't be big endian there.
Well, the "be" in in_be32() means to assume that 32-bit integer is stored in big-endian format, and to convert it to native format during the read. An ARM implementation of in_be32() would include byte-swapping.
So are we heading towards driver specific sets of IO accessors? :-( It sounds like that's what it needs because of these different endian-ness issues.
I'm surprised that ioread32() on PowerPC always does a little-endian read, but I'm guessing that was done for compatibility. I don't know what to do about that.
And lets not forget that we're talking just about this - there's also the issue of on-SoC devices on ARM now appearing behind PCI buses on x86, which would make those appear as LE. I bet if any of those appearing on PPC would appear in BE mode. So, 'native endian' doesn't work either.
I think PCI is a special-case on most PowerPC parts, in that the registers are still little-endian. But I don't know how pervasive that is. In fact, I think on some Freescale PowerPC SOCs, the PCI registers are little-endian, and other others they're big-endian. You can see that in fsl_pci.c.
So, I'm getting the feeling that something as simple as IO accessors are just a huge mountain of crap waiting to bite.
Very few devices are shared between Freescale's PowerPC and ARM SOCs, so the impact is only a few drivers. That's why it might be okay (for now) to have "dual" I/O accessors in specific drivers.
On Fri, Feb 24, 2012 at 05:05:16PM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote:
Ho hum. So, what that's saying to me is that even implementing in_32be() on ARM results in rubbish because it wouldn't be big endian there.
Well, the "be" in in_be32() means to assume that 32-bit integer is stored in big-endian format, and to convert it to native format during the read. An ARM implementation of in_be32() would include byte-swapping.
So, let's go around this loop again.
What would be wrong with converting in_be32() in these drivers to instead ioread32be()?
Russell King - ARM Linux wrote:
What would be wrong with converting in_be32() in these drivers to instead ioread32be()?
Well, it wouldn't actually fix any problems. It's just a stylistic change for PowerPC. On ARM, however, ioread32be() is wrong because the SSI registers are little-endian.
In case you're confused: the SSI is an I2S audio controller developed by the i.MX (ARM) division on Freescale. When the PowerPC division wanted to add an I2S controller to its SOCs, they just took the same SSI device and dropped in the PowerPC SOC. But since PowerPC is big-endian, the registers are byte-swapped in hardware. So even though the i.MX SSI and the PowerPC SSI have the same registers and do the same thing, those actual registers are little-endian on i.MX and big-endian on PowerPC.
As far as I know, there is no single function in the kernel that will perform an ordered 32-bit big-endian read on PowerPC and an ordered 32-bit little-endian read on ARM.
On Fri, Feb 24, 2012 at 05:22:14PM -0600, Timur Tabi wrote:
Russell King - ARM Linux wrote:
What would be wrong with converting in_be32() in these drivers to instead ioread32be()?
Well, it wouldn't actually fix any problems. It's just a stylistic change for PowerPC. On ARM, however, ioread32be() is wrong because the SSI registers are little-endian.
So, to go around the loop a little further: to make this driver work _unaltered_ on ARM, we'd need an in_32be() that read the register _without_ doing the byte swapping.
But then, such an implementation of in_32be() would be absurd because it's not doing what it says.
So, the driver needs to change its accessor method. There is no accessor in existance at present which does the right thing.
And then I went on to explain that there's more complicated cases too, where it seems that in theory you can end up with the same IP connected in BE mode and sitting behind a PCI bus, which presents the need for accessors which know the bus type by some means.
Russell King - ARM Linux wrote:
So, to go around the loop a little further: to make this driver work _unaltered_ on ARM, we'd need an in_32be() that read the register _without_ doing the byte swapping.
But then, such an implementation of in_32be() would be absurd because it's not doing what it says.
Exactly.
So, the driver needs to change its accessor method. There is no accessor in existance at present which does the right thing.
Yes, that is my assessment.
And then I went on to explain that there's more complicated cases too, where it seems that in theory you can end up with the same IP connected in BE mode and sitting behind a PCI bus, which presents the need for accessors which know the bus type by some means.
And since most PCI devices are little-endian, I think that's why readl() is always little-endian.
We might be able to modify the PowerPC version of ioread32() to be native-endian without too much difficulty, but that's something that Ben Herrenschmidt would need to approve.
participants (3)
-
Russell King - ARM Linux -
Timur Tabi -
Trent Piepho