Subject: Re: [PATCH] PCI: apple: Configure link speeds properly
To:     Rob Herring <robh@kernel.org>
Cc:     Marc Zyngier <maz@kernel.org>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        PCI <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20211122111332.72264-1-marcan@marcan.st>
 <CAL_Jsq+vFbFN+WQhi3dRicW+kaP1Oi9JPSmnAFL7XAc0eCvgrA@mail.gmail.com>
From:   Hector Martin <marcan@marcan.st>
Message-ID: <8e881b80-614c-dccf-ddaf-895d1acf26c7@marcan.st>
Date:   Wed, 24 Nov 2021 16:40:17 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CAL_Jsq+vFbFN+WQhi3dRicW+kaP1Oi9JPSmnAFL7XAc0eCvgrA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: es-ES
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 24/11/2021 11.23, Rob Herring wrote:
>> +#include "../pci.h"
>> +/* Apple PCIe is based on DesignWare IP and shares some registers */
>> +#include "dwc/pcie-designware.h"
> 
> I'm starting to regret this not being part of the DWC driver...

Main issue is the DWC driver seems to have a pretty hard-coded 
assumption of one port per controller, plus does a bunch of stuff 
differently for the higher layers. It seems Apple used the DWC PHY/LTSSM 
bits, then rolled their own upper layer.

>> +/* The offset of the PCIe capabilities structure in bridge config space */
>> +#define PCIE_CAP_BASE          0x70
> 
> This offset is discoverable, so don't hardcode it.

Sure, it just means I have to reinvent the PCI capability lookup wheel 
again. I'd love to use the regular accessors, but the infrastructure 
isn't up to the point where we can do that yet yere. DWC also reinvents 
this wheel, but we can't reuse that code because it pokes these 
registers through a separate reg range, not config space (even though it 
seems like they should be the same thing? I'm not sure what's going on 
in the DWC devices... for the Apple controller it's just the ECAM).

>> +       max_gen = of_pci_get_max_link_speed(port->np);
>> +       if (max_gen < 0) {
>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
> 
> Better to fail than limp along in gen1? Though you don't check the
> return value...
> 
> Usually, the DT property is there to limit the speed when there's a
> board limitation.

The default *setting* is actually Gen4, but without 
PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make 
more sense to only set the LNKCTL field if max-link-speed is specified, 
and unconditionally poke that bit. That'll get us Gen4 by default (or 
even presumably Gen5 in future controllers, if everything else stays 
compatible).

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub