Hello,
This is for PCMCIA (cardbus) version of Silicon Image 3124 SerialATA
controller. When cacheline size is configured, the controller uses Read
Multiple commands.
? Bit [07:00]: Cache Line Size (R/W). This bit field is used to specify
the system cacheline size in terms of 32-bit words. The SiI3124, when
initiating a read transaction, will issue the Read Multiple PCI command
if empty space in its FIFO is greater than the value programmed in this
register.
As the BIOS doesn't run after hotplugging cardbus card, the cache line
isn't configured and the controller ends up having 0 cache line size and
always using Read command. When that happens, write performance drops
hard - the throughput is < 2Mbytes/s.
http://thread.gmane.org/gmane.linux.ide/12908/focus=12908
So, sata_sil24 driver has to program CLS if it's not already set, but
I'm not sure which number to punch in. FWIW, sil3124 doesn't seem to
put restrictions on the values which can be used for CLS. There are
several candidates...
* L1_CACHE_BYTES / 4 : this is used by init routine in yenta_socket.c.
It seems to be a sane default but I'm not sure whether L1 cache line
size always coincides with the size as seen from PCI bus.
* pci_cache_line_size in drivers/pci/pci.c : this is used for
pci_generic_prep_mwi() and can be overridden by arch specific code.
this seems more appropriate but is not exported.
For all involved commands - memory read line, memory read multiple and
memory write and invalidate - a value larger than actual cacheline size
doesn't hurt but a smaller value may.
I'm thinking of implementing a query function for pci_cache_line_size,
say, int pci_cacheline_size(struct pci_dev *pdev), and use it in the
device init routine. Does this sound sane?
Thanks.
--
tejun
On Thu, Sep 07, 2006 at 10:31:02AM +0200, Tejun Heo wrote:
> As the BIOS doesn't run after hotplugging cardbus card, the cache line
> isn't configured and the controller ends up having 0 cache line size and
> always using Read command. When that happens, write performance drops
> hard - the throughput is < 2Mbytes/s.
>
> http://thread.gmane.org/gmane.linux.ide/12908/focus=12908
>
> So, sata_sil24 driver has to program CLS if it's not already set, but
> I'm not sure which number to punch in. FWIW, sil3124 doesn't seem to
> put restrictions on the values which can be used for CLS. There are
> several candidates...
>
> * L1_CACHE_BYTES / 4 : this is used by init routine in yenta_socket.c.
> It seems to be a sane default but I'm not sure whether L1 cache line
> size always coincides with the size as seen from PCI bus.
>
> * pci_cache_line_size in drivers/pci/pci.c : this is used for
> pci_generic_prep_mwi() and can be overridden by arch specific code.
> this seems more appropriate but is not exported.
>
> For all involved commands - memory read line, memory read multiple and
> memory write and invalidate - a value larger than actual cacheline size
> doesn't hurt but a smaller value may.
Just call pci_set_mwi(), that'll make sure the cache line size is set
correctly.
Matthew Wilcox wrote:
> Just call pci_set_mwi(), that'll make sure the cache line size is set
> correctly.
Sounds simple enough. Just two small worries though.
* It has an apparent side effect of setting PCI_COMMAND_INVALIDATE,
which should be okay in sil3124's case.
* The controller might have some restrictions on configurable cache line
size. This is the same for MWI, so I guess this problem is just imaginary.
For the time being, I'll go with pci_set_mwi() but IMHO it would be
better to have a pci helper for this purpose -
pci_config_cacheline_size() or something.
Thanks.
--
tejun
On Thu, Sep 07, 2006 at 01:20:22PM +0200, Tejun Heo wrote:
> Matthew Wilcox wrote:
> >Just call pci_set_mwi(), that'll make sure the cache line size is set
> >correctly.
>
> Sounds simple enough. Just two small worries though.
>
> * It has an apparent side effect of setting PCI_COMMAND_INVALIDATE,
> which should be okay in sil3124's case.
>
> * The controller might have some restrictions on configurable cache line
> size. This is the same for MWI, so I guess this problem is just imaginary.
>
> For the time being, I'll go with pci_set_mwi() but IMHO it would be
> better to have a pci helper for this purpose -
> pci_config_cacheline_size() or something.
I've often wondered why we don't set the cache line size when we set the
bus master bit - ISTR when I read the PCI spec (2.1 or 2.2) it implied
that this should be set for bus master operations.
I've certainly seen PCI devices which can bus master and do have the
cache line size register but have the invalidate bit set forced to zero
in the command register.
Makes sense when you consider there are cache line considerations for
"memory read multiple" and "memory read line" bus commands in addition
to the "memory write and invalidate" bus command.
(Consider - if you use memory read line and haven't programmed the
cache line size, how does the master know how long a cache line is and
hence knows when to use this command?)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Thu, 7 Sep 2006, Tejun Heo wrote:
> Hello,
>
> This is for PCMCIA (cardbus) version of Silicon Image 3124 SerialATA
> controller. When cacheline size is configured, the controller uses Read
> Multiple commands.
>
> ? Bit [07:00]: Cache Line Size (R/W). This bit field is used to specify
> the system cacheline size in terms of 32-bit words. The SiI3124, when
> initiating a read transaction, will issue the Read Multiple PCI command
> if empty space in its FIFO is greater than the value programmed in this
> register.
>
> As the BIOS doesn't run after hotplugging cardbus card, the cache line
> isn't configured and the controller ends up having 0 cache line size and
> always using Read command. When that happens, write performance drops
> hard - the throughput is < 2Mbytes/s.
>
> http://thread.gmane.org/gmane.linux.ide/12908/focus=12908
>
> So, sata_sil24 driver has to program CLS if it's not already set, but
> I'm not sure which number to punch in. FWIW, sil3124 doesn't seem to
> put restrictions on the values which can be used for CLS. There are
> several candidates...
>
> * L1_CACHE_BYTES / 4 : this is used by init routine in yenta_socket.c.
> It seems to be a sane default but I'm not sure whether L1 cache line
> size always coincides with the size as seen from PCI bus.
>
> * pci_cache_line_size in drivers/pci/pci.c : this is used for
> pci_generic_prep_mwi() and can be overridden by arch specific code.
> this seems more appropriate but is not exported.
>
> For all involved commands - memory read line, memory read multiple and
> memory write and invalidate - a value larger than actual cacheline size
> doesn't hurt but a smaller value may.
>
> I'm thinking of implementing a query function for pci_cache_line_size,
> say, int pci_cacheline_size(struct pci_dev *pdev), and use it in the
> device init routine. Does this sound sane?
>
> Thanks.
>
> --
> tejun
The cache line size specifies the system cache-line size in dword
increments. For most, (ix86) this would be 8, i.e., eight 32-bit
words or 32 bytes. This is from page 376, PCI System Architecture,
ISBN 0-201-30974-2. It also says that a device may limit the number
of cache cycles if an unsupported value is written there. In that
case, the device will act as if the value 0 was written (no write-and-
invalidate transactions), basically poor performance.
The L1 cache size shouldn't have anything to do with this, BTW.
Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.24 on an i686 machine (5592.66 BogoMips).
New book: http://www.AbominableFirebug.com/
_
****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.
Thank you.
On Thu, Sep 07, 2006 at 01:07:56PM +0100, Russell King wrote:
> I've often wondered why we don't set the cache line size when we set the
> bus master bit - ISTR when I read the PCI spec (2.1 or 2.2) it implied
> that this should be set for bus master operations.
It's not required ... 3.2.1 of pci 2.3 says:
While Memory Write and Invalidate is the only command that requires
implementation of the Cacheline Size register, it is strongly suggested
the memory read commands use it as well. A bridge that prefetches is
responsible for any latent data not consumed by the master.
(obviously this is talking about requirements placed on the device, not
on the OS, but it'd behoove us to help the device out here).
It's also useful to implement it for slave devices. PCI 2.3 has the
concept of cacheline wrap transactions -- eg with a cacheline size of
0x10, it can transfer data to 0x108, then 0x10C, 0x100, 0x104, then
0x118, etc
So I think we should redo the PCI subsystem to set cacheline size during
the buswalk rather than waiting for drivers to ask for it to be set.
>
> So I think we should redo the PCI subsystem to set cacheline size during
> the buswalk rather than waiting for drivers to ask for it to be set.
... while allowing for quirks for devices that go puke when this
register gets written ;)
(afaik there are a few)
On Thu, Sep 07, 2006 at 02:33:25PM +0200, Arjan van de Ven wrote:
>
> >
> > So I think we should redo the PCI subsystem to set cacheline size during
> > the buswalk rather than waiting for drivers to ask for it to be set.
>
> ... while allowing for quirks for devices that go puke when this
> register gets written ;)
>
> (afaik there are a few)
So you want:
unsigned int no_cls:1; /* Device pukes on write to Cacheline Size */
in struct pci_dev?
Matthew Wilcox wrote:
> On Thu, Sep 07, 2006 at 02:33:25PM +0200, Arjan van de Ven wrote:
>>> So I think we should redo the PCI subsystem to set cacheline size during
>>> the buswalk rather than waiting for drivers to ask for it to be set.
>> ... while allowing for quirks for devices that go puke when this
>> register gets written ;)
>>
>> (afaik there are a few)
>
> So you want:
>
> unsigned int no_cls:1; /* Device pukes on write to Cacheline Size */
>
> in struct pci_dev?
The spec says that devices can put additional restriction on supported
cacheline size (IIRC, the example was something like power of two >= or
<= certain size) and should ignore (treat as zero) if unsupported value
is written. So, there might be need for more low level driver
involvement which knows device restrictions, but I don't know whether
such devices exist.
--
tejun
On Thu, 2006-09-07 at 06:40 -0600, Matthew Wilcox wrote:
> On Thu, Sep 07, 2006 at 02:33:25PM +0200, Arjan van de Ven wrote:
> >
> > >
> > > So I think we should redo the PCI subsystem to set cacheline size during
> > > the buswalk rather than waiting for drivers to ask for it to be set.
> >
> > ... while allowing for quirks for devices that go puke when this
> > register gets written ;)
> >
> > (afaik there are a few)
>
> So you want:
>
> unsigned int no_cls:1; /* Device pukes on write to Cacheline Size */
>
> in struct pci_dev?
something like that yes...
On Thu, Sep 07, 2006 at 06:23:11AM -0600, Matthew Wilcox wrote:
> On Thu, Sep 07, 2006 at 01:07:56PM +0100, Russell King wrote:
> > I've often wondered why we don't set the cache line size when we set the
> > bus master bit - ISTR when I read the PCI spec (2.1 or 2.2) it implied
> > that this should be set for bus master operations.
>
> It's not required ... 3.2.1 of pci 2.3 says:
>
> While Memory Write and Invalidate is the only command that requires
> implementation of the Cacheline Size register, it is strongly suggested
> the memory read commands use it as well. A bridge that prefetches is
> responsible for any latent data not consumed by the master.
>
> (obviously this is talking about requirements placed on the device, not
> on the OS, but it'd behoove us to help the device out here).
>
> It's also useful to implement it for slave devices. PCI 2.3 has the
> concept of cacheline wrap transactions -- eg with a cacheline size of
> 0x10, it can transfer data to 0x108, then 0x10C, 0x100, 0x104, then
> 0x118, etc
As does v2.2 and v2.1.
> So I think we should redo the PCI subsystem to set cacheline size during
> the buswalk rather than waiting for drivers to ask for it to be set.
Agreed, and this is something I'm already doing on ARM in my
pcibios_fixup_bus().
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Thu, Sep 07, 2006 at 02:53:57PM +0200, Tejun Heo wrote:
> The spec says that devices can put additional restriction on supported
> cacheline size (IIRC, the example was something like power of two >= or
> <= certain size) and should ignore (treat as zero) if unsupported value
> is written. So, there might be need for more low level driver
> involvement which knows device restrictions, but I don't know whether
> such devices exist.
That's nothing we can do anything about. The system cacheline size is
what it is. If the device doesn't support it, we can't fall back to a
different size, it'll cause data corruption. So we'll just continue on,
and devices which live up to the spec will act as if we hadn't
programmed a cache size. For devices that don't, we'll have the quirk.
Arguably devices which don't support the real system cacheline size
would only get data corruption if they used MWI, so we only have to
prevent them from using MWI; they could use a different cacheline size
for MRM and MRL without causing data corruption. But I don't think we
want to go down that route; do you?
On Thu, Sep 07, 2006 at 02:53:57PM +0200, Tejun Heo wrote:
> Matthew Wilcox wrote:
> >On Thu, Sep 07, 2006 at 02:33:25PM +0200, Arjan van de Ven wrote:
> >>>So I think we should redo the PCI subsystem to set cacheline size during
> >>>the buswalk rather than waiting for drivers to ask for it to be set.
> >>... while allowing for quirks for devices that go puke when this
> >>register gets written ;)
> >>
> >>(afaik there are a few)
> >
> >So you want:
> >
> > unsigned int no_cls:1; /* Device pukes on write to Cacheline Size */
> >
> >in struct pci_dev?
>
> The spec says that devices can put additional restriction on supported
> cacheline size (IIRC, the example was something like power of two >= or
> <= certain size) and should ignore (treat as zero) if unsupported value
> is written. So, there might be need for more low level driver
> involvement which knows device restrictions, but I don't know whether
> such devices exist.
The problem is that both ends of the bus need to know the cache line
size for some of these commands to work correctly.
Consider, as in Matthew's case, a read command which wraps at the cache
line boundary. Unless both the master and slave are programmed with the
same cache line size, it's entirely possible for the wrong memory
locations to be read.
Eg, the device might expect 0x118, 0x11c, 0x100, 0x104, but it actually
gets 0x118, 0x11c, 0x120, 0x124 because the target got programmed with
64 while the master was set to 32.
This is, of course, supposing that the memory read command is used in
the cache line wrap mode.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
Matthew Wilcox wrote:
> On Thu, Sep 07, 2006 at 02:53:57PM +0200, Tejun Heo wrote:
>> The spec says that devices can put additional restriction on supported
>> cacheline size (IIRC, the example was something like power of two >= or
>> <= certain size) and should ignore (treat as zero) if unsupported value
>> is written. So, there might be need for more low level driver
>> involvement which knows device restrictions, but I don't know whether
>> such devices exist.
>
> That's nothing we can do anything about. The system cacheline size is
> what it is. If the device doesn't support it, we can't fall back to a
> different size, it'll cause data corruption. So we'll just continue on,
> and devices which live up to the spec will act as if we hadn't
> programmed a cache size. For devices that don't, we'll have the quirk.
For MWI, it will cause data corruption. For READ LINE and MULTIPLE, I
think it would be okay. The memory is prefetchable after all. Anyways,
this shouldn't be of too much problem and probably can be handled by
quirks if ever needed.
> Arguably devices which don't support the real system cacheline size
> would only get data corruption if they used MWI, so we only have to
> prevent them from using MWI; they could use a different cacheline size
> for MRM and MRL without causing data corruption. But I don't think we
> want to go down that route; do you?
Oh yeah, that's what I was trying to say, and I don't want to go down
that route. So, I guess this one is settled.
Thanks.
--
tejun
Matthew Wilcox wrote:
> On Thu, Sep 07, 2006 at 02:53:57PM +0200, Tejun Heo wrote:
>> The spec says that devices can put additional restriction on supported
>> cacheline size (IIRC, the example was something like power of two >= or
>> <= certain size) and should ignore (treat as zero) if unsupported value
>> is written. So, there might be need for more low level driver
>> involvement which knows device restrictions, but I don't know whether
>> such devices exist.
>
> That's nothing we can do anything about. The system cacheline size is
> what it is. If the device doesn't support it, we can't fall back to a
> different size, it'll cause data corruption. So we'll just continue on,
> and devices which live up to the spec will act as if we hadn't
> programmed a cache size. For devices that don't, we'll have the quirk.
>
> Arguably devices which don't support the real system cacheline size
> would only get data corruption if they used MWI, so we only have to
> prevent them from using MWI; they could use a different cacheline size
> for MRM and MRL without causing data corruption. But I don't think we
> want to go down that route; do you?
FWIW, there are definitely both ethernet and SATA PCI devices which only
allow a limited set of values in the cacheline size register... and that
limited set does not include some of the modern machines.
Jeff
On Thu, Sep 07, 2006 at 03:19:04PM +0200, Tejun Heo wrote:
...
> For MWI, it will cause data corruption. For READ LINE and MULTIPLE, I
> think it would be okay. The memory is prefetchable after all.
Within the context of DMA API, memory is prefetchable by the device
for "streaming" transactions but not for "coherent" memory.
PCI subsystem has no way of knowing which transaction a device
will use for any particular type of memory access. Only the
driver can embed that knowledge.
> Anyways, this shouldn't be of too much problem and probably
> can be handled by quirks if ever needed.
>
> >Arguably devices which don't support the real system cacheline size
> >would only get data corruption if they used MWI, so we only have to
> >prevent them from using MWI; they could use a different cacheline size
> >for MRM and MRL without causing data corruption. But I don't think we
> >want to go down that route; do you?
>
> Oh yeah, that's what I was trying to say, and I don't want to go down
> that route. So, I guess this one is settled.
hrm...if the driver can put a safe value in cachelinesize register
and NOT enable MWI, I can imagine a significant performance boost
if the device can use MRM or MRL. But IMHO it's up to the driver
writers (or other contributors) to figure that out.
Current API (pci_set_mwi()) ties enabling MRM/MRL with enabling MWI
and I don't see a really good reason for that. Only the converse
is true - enabling MWI requires setting cachelinesize.
hth,
grant
Grant Grundler wrote:
> On Thu, Sep 07, 2006 at 03:19:04PM +0200, Tejun Heo wrote:
> ...
>> For MWI, it will cause data corruption. For READ LINE and MULTIPLE, I
>> think it would be okay. The memory is prefetchable after all.
>
> Within the context of DMA API, memory is prefetchable by the device
> for "streaming" transactions but not for "coherent" memory.
> PCI subsystem has no way of knowing which transaction a device
> will use for any particular type of memory access. Only the
> driver can embed that knowledge.
I think using larger cacheline value should be okay for both
prefetchable and non-prefetchable memory. Using larger value tells the
device to be more conservative in issuing MRL, MRW or WMI. As Russell
has pointed out, cacheline-wrapping access wouldn't work but I think
it's reasonable to expect for such device to be flexible about cacheline
config.
>> Oh yeah, that's what I was trying to say, and I don't want to go down
>> that route. So, I guess this one is settled.
>
> hrm...if the driver can put a safe value in cachelinesize register
> and NOT enable MWI, I can imagine a significant performance boost
> if the device can use MRM or MRL. But IMHO it's up to the driver
> writers (or other contributors) to figure that out.
>
> Current API (pci_set_mwi()) ties enabling MRM/MRL with enabling MWI
> and I don't see a really good reason for that. Only the converse
> is true - enabling MWI requires setting cachelinesize.
arch/i386/pci/common.c overrides cacheline size to min 32 regardless of
actual size. So, we seem to be using larger cacheline size for MWI already.
Jeff pointed out that there actually are devices which limit CLS config.
IMHO, making PCI configure CLS automatically and provide helpers to
LLD to override it if necessary should cut it.
Thanks.
--
tejun
Tejun Heo wrote:
> arch/i386/pci/common.c overrides cacheline size to min 32 regardless of
> actual size. So, we seem to be using larger cacheline size for MWI
> already.
It clamps the minimum size to 32, yes, but on modern machines common.c
configures it to a larger size.
> Jeff pointed out that there actually are devices which limit CLS config.
> IMHO, making PCI configure CLS automatically and provide helpers to LLD
> to override it if necessary should cut it.
We still have to add a raft of quirks, if we start automatically
configurating CLS... Also, many PCI devices hardcode it to zero.
If we start configuring CLS automatically, I forsee a period of breakage...
Jeff
Grant Grundler wrote:
> hrm...if the driver can put a safe value in cachelinesize register
> and NOT enable MWI, I can imagine a significant performance boost
> if the device can use MRM or MRL. But IMHO it's up to the driver
> writers (or other contributors) to figure that out.
Yes.
> Current API (pci_set_mwi()) ties enabling MRM/MRL with enabling MWI
> and I don't see a really good reason for that. Only the converse
> is true - enabling MWI requires setting cachelinesize.
Correct, that's why it was done that way, when I wrote the API.
Enabling MWI required making sure the BIOS configured our CLS for us,
which was often not the case. No reason why we can't do a
pdev->set_cls = 1;
rc = pci_enable_device(pdev);
or
rc = pci_set_cacheline_size(pdev);
Regards,
Jeff
On Thu, Sep 07, 2006 at 12:00:39PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> >arch/i386/pci/common.c overrides cacheline size to min 32 regardless of
> >actual size. So, we seem to be using larger cacheline size for MWI
> >already.
>
> It clamps the minimum size to 32, yes, but on modern machines common.c
> configures it to a larger size.
>
>
> >Jeff pointed out that there actually are devices which limit CLS config.
> > IMHO, making PCI configure CLS automatically and provide helpers to LLD
> >to override it if necessary should cut it.
>
> We still have to add a raft of quirks, if we start automatically
> configurating CLS... Also, many PCI devices hardcode it to zero.
That's not a problem. As long as they silently discard the writes to
the CLS register, we'll cope. If they decide to flip out, then we have
the no_cls flag.
> If we start configuring CLS automatically, I forsee a period of breakage...
Probably. But it's something we should have done a long time ago.
On Thu, Sep 07, 2006 at 12:04:07PM -0400, Jeff Garzik wrote:
...
> >Current API (pci_set_mwi()) ties enabling MRM/MRL with enabling MWI
> >and I don't see a really good reason for that. Only the converse
> >is true - enabling MWI requires setting cachelinesize.
>
> Correct, that's why it was done that way, when I wrote the API.
> Enabling MWI required making sure the BIOS configured our CLS for us,
> which was often not the case. No reason why we can't do a
>
> pdev->set_cls = 1;
> rc = pci_enable_device(pdev);
I was thinking the pci_enable_device could safely set CLS if MWI
were NOT enabled. I'm sure somethings would break that worked before
but that's what quirks are for, right?
Anyway, I'm convinced the arch specific PCI code should be setting CLS
either at bus_fixup or via some quirks to compensate for "broken" firmware
(which didn't set CLS). Maybe there is more brokn HW out there than
I think...I'm easy convince that's the case.
If a driver wanted to enable MWI, then pci_set_mwi() should
verify (or force) CLS setttings or return an error.
That part seems pretty straight forward and don't need a
change in API here.
thanks,
grant