SIL24: pcie_set_readrq 4096

SIL24: pcie_set_readrq 4096

[Jassi Brar]

Hi,
  After implementing driver for PCIe RC controller of a SOC, I came
across peculiarity
while testing SIL3132 directly connected to the root port.

My RC IP is configured for maximum of 256bytes of payload size.
The sata_sil24 driver unconditionally sets pcie_set_readrq(pdev, 4096)
for the endpoint
which apparently works for 'every' other host irrespective of their capacity.

In my case it works only if I change to
  pcie_set_readrq(pdev, 256)

Shouldn't the Linux PCI have provision to allow max_read/payload_size
not higher than
the least of max capacities of all agents in the chain ?

I see similar use of pcie_set_readrq(pdev, 4096), so could be I am
missing something ?

Thanks.

Re: SIL24: pcie_set_readrq 4096

[Tejun Heo]

Hello,

On 07/22/2010 04:47 AM, Jassi Brar wrote:
>   After implementing driver for PCIe RC controller of a SOC, I came
> across peculiarity
> while testing SIL3132 directly connected to the root port.
> 
> My RC IP is configured for maximum of 256bytes of payload size.
> The sata_sil24 driver unconditionally sets pcie_set_readrq(pdev, 4096)
> for the endpoint
> which apparently works for 'every' other host irrespective of their capacity.
> 
> In my case it works only if I change to
>   pcie_set_readrq(pdev, 256)
> 
> Shouldn't the Linux PCI have provision to allow max_read/payload_size
> not higher than
> the least of max capacities of all agents in the chain ?
> 
> I see similar use of pcie_set_readrq(pdev, 4096), so could be I am
> missing something ?

It's been a while since I read pci-e spec but IIUC readrq size and
payload size are independent and if readrq is larger than payload size
it's supposed to complete in multiple steps.  I could be wrong tho.

Thanks.

-- 
tejun

Re: SIL24: pcie_set_readrq 4096

[Jassi Brar]

On Thu, Jul 22, 2010 at 10:18 PM, Tejun Heo <tj@kernel.org> wrote:
> It's been a while since I read pci-e spec but IIUC readrq size and
> payload size are independent and if readrq is larger than payload size
> it's supposed to complete in multiple steps.  I could be wrong tho.
AFAIUI readrq size is independent and not greater than the max_payload size.
Payload size is what max a device can handle and readrq is what a device can
ask.
SIL24 does support 4096 and hence can set readrq to that limit, but if the
payload capacity of the RC is less than 4096, I am not sure if it is supposed to
work.

Re: SIL24: pcie_set_readrq 4096

[Tejun Heo]

On 07/22/2010 04:34 PM, Jassi Brar wrote:
> On Thu, Jul 22, 2010 at 10:18 PM, Tejun Heo <tj@kernel.org> wrote:
>> It's been a while since I read pci-e spec but IIUC readrq size and
>> payload size are independent and if readrq is larger than payload size
>> it's supposed to complete in multiple steps.  I could be wrong tho.
> AFAIUI readrq size is independent and not greater than the max_payload size.
> Payload size is what max a device can handle and readrq is what a device can
> ask.
> SIL24 does support 4096 and hence can set readrq to that limit, but if the
> payload capacity of the RC is less than 4096, I am not sure if it is supposed to
> work.

Hmm... my impression is that a single read request may be served by
multiple TLPs, so readrq size can go over payload size which limits
the size of payload in a single TLP, but let's wait for someone more
knowledgeable to chime in.

Thanks.

-- 
tejun

Re: SIL24: pcie_set_readrq 4096

[Rolf Eike Beer]

[-- Attachment #1: Type: Text/Plain, Size: 969 bytes --]

Jassi Brar wrote:
> On Thu, Jul 22, 2010 at 10:18 PM, Tejun Heo <tj@kernel.org> wrote:
> > It's been a while since I read pci-e spec but IIUC readrq size and
> > payload size are independent and if readrq is larger than payload size
> > it's supposed to complete in multiple steps.  I could be wrong tho.
> 
> AFAIUI readrq size is independent and not greater than the max_payload
> size. Payload size is what max a device can handle and readrq is what a
> device can ask.

The first and third statements are correct, the second is not. Read request may 
be larger than the usual payload size.

However that is something the complete PCIe fabric up to that device has to 
support. So if the root port does only support read request of something 
smaller the device must be configured with that maximum size so it will not 
issue any bigger requests. Blindly configuring a device to the maximum allowed 
value looks dangerous and broken to me.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]