Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] PCI: dwc: meson add quirk
To:     Rob Herring <robh@kernel.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     Artem Lapkin <email2tema@gmail.com>,
        Yue Wang <yue.wang@amlogic.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Krzysztof Wilczynski <kw@linux.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Christian Hewitt <christianshewitt@gmail.com>,
        PCI <linux-pci@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        art@khadas.com, Nick Xie <nick@khadas.com>, gouwa@khadas.com
References: <20210618063821.1383357-1-art@khadas.com>
 <CAFBinCB6bHy6Han0+oUcuGfccv1Rh_P0Gows1ezWdV4eA267tg@mail.gmail.com>
 <CAL_JsqK+zjf2r_Q9gE8JwJw+Emn+JB4wOyH7eQct=kBvpUKstw@mail.gmail.com>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
Message-ID: <9b27444c-ea13-0dd2-a671-cef27e03b35c@baylibre.com>
Date:   Fri, 18 Jun 2021 17:08:42 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <CAL_JsqK+zjf2r_Q9gE8JwJw+Emn+JB4wOyH7eQct=kBvpUKstw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/06/2021 16:30, Rob Herring wrote:
> On Fri, Jun 18, 2021 at 6:12 AM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>
>> Hi Artem,
>>
>> On Fri, Jun 18, 2021 at 8:38 AM Artem Lapkin <email2tema@gmail.com> 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
>