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
>
