Re: [PATCH] PCI: dwc: meson add quirk

[Neil Armstrong]

On 18/06/2021 16:30, Rob Herring wrote:
> On Fri, Jun 18, 2021 at 6:12 AM Martin Blumenstingl
> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
>>
>> Hi Artem,
>>
>> On Fri, Jun 18, 2021 at 8:38 AM Artem Lapkin <email2tema@xxxxxxxxx> wrote:
>>>
>>> Device set same 256 bytes maximum read request size equal MAX_READ_REQ_SIZE
>>> was find some issue with HDMI scrambled picture and nvme devices
>>> at intensive writing...
>>>
>>> [    4.798971] nvme 0000:01:00.0: fix MRRS from 512 to 256
>>>
>>> This quirk setup same MRRS if we try solve this problem with
>>> pci=pcie_bus_perf kernel command line param
>> thank you for investigating this issue and for providing a fix!
>>
>> [...]
>>> +static void meson_pcie_quirk(struct pci_dev *dev)
>>> +{
>>> +       int mrrs;
>>> +
>>> +       /* no need quirk */
>>> +       if (pcie_bus_config != PCIE_BUS_DEFAULT)
>>> +               return;
>>> +
>>> +       /* no need for root bus */
>>> +       if (pci_is_root_bus(dev->bus))
>>> +               return;
>>> +
>>> +       mrrs = pcie_get_readrq(dev);
>>> +
>>> +       /*
>>> +        * set same 256 bytes maximum read request size equal MAX_READ_REQ_SIZE
>>> +        * was find some issue with HDMI scrambled picture and nvme devices
>>> +        * at intensive writing...
>>> +        */
>>> +
>>> +       if (mrrs != MAX_READ_REQ_SIZE) {
>>> +               dev_info(&dev->dev, "fix MRRS from %d to %d\n", mrrs, MAX_READ_REQ_SIZE);
>>> +               pcie_set_readrq(dev, MAX_READ_REQ_SIZE);
>>> +       }
>>> +}
>>> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_pcie_quirk);
> 
> Isn't this going to run for everyone if meson driver happens to be enabled?

It should be enabled only when the Amlogic bridge is present, thus similar filtering as keystone & loongon
is needed, but with such filtering we could reuse ks_pcie_quirk() and loongson_mrrs_quirk() as is.

> 
>> it seems that other PCIe controllers need something similar. in
>> particular I found pci-keystone [0] and pci-loongson [1]
>> while comparing your code with the two existing implementations two
>> things came to my mind:
>> 1. your implementation slightly differs from the two existing ones as
>> it's not walking through the parent PCI busses (I think this would be
>> relevant if there's another bridge between the host bridge and the
>> actual device)
>> 2. (this is a question towards the PCI maintainers) does it make sense
>> to have this MRRS quirk re-usable somewhere?
> 
> Yes. Ideally, the max size could just be data in the bus or bridge
> struct and perhaps some flags too, then the core can handle
> everything.

AFAIL Simply moving ks_pcie_quirk() and loongson_mrrs_quirk() to core with the amlogic pci IDS added would be sufficient here.

Neil

> 
> Rob
>