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;
Date:   Thu, 20 Jan 2022 18:19:15 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Rob Herring <robh@kernel.org>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Marek =?utf-8?B?QmVow7pu?= <kabel@kernel.org>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 05/11] PCI: mvebu: Correctly configure x1/x4 mode
Message-ID: <20220120171915.rynfs3hucnfj4kyb@pali>
References: <20220105150239.9628-1-pali@kernel.org>
 <20220112151814.24361-1-pali@kernel.org>
 <20220112151814.24361-6-pali@kernel.org>
 <YemXPQx4F1eRtLxO@robh.at.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YemXPQx4F1eRtLxO@robh.at.kernel.org>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Thursday 20 January 2022 11:09:17 Rob Herring wrote:
> On Wed, Jan 12, 2022 at 04:18:08PM +0100, Pali Rohár wrote:
> > If x1/x4 mode is not set correctly then link with endpoint card is not
> > established.
> > 
> > Use DTS property 'num-lanes' to deteriminate x1/x4 mode.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/pci/controller/pci-mvebu.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index a075ba26cff1..0f2ec0a17874 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -93,6 +93,7 @@ struct mvebu_pcie_port {
> >  	void __iomem *base;
> >  	u32 port;
> >  	u32 lane;
> > +	bool is_x4;
> 
> I would just store the number of lanes.
> 
> >  	int devfn;
> >  	unsigned int mem_target;
> >  	unsigned int mem_attr;
> > @@ -233,13 +234,25 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
> >  
> >  static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
> >  {
> > -	u32 ctrl, cmd, dev_rev, mask;
> > +	u32 ctrl, lnkcap, cmd, dev_rev, mask;
> >  
> >  	/* Setup PCIe controller to Root Complex mode. */
> >  	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);
> >  	ctrl |= PCIE_CTRL_RC_MODE;
> >  	mvebu_writel(port, ctrl, PCIE_CTRL_OFF);
> >  
> > +	/*
> > +	 * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> > +	 * Capability register. This register is defined by PCIe specification
> > +	 * as read-only but this mvebu controller has it as read-write and must
> > +	 * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> > +	 * not set correctly then link with endpoint card is not established.
> > +	 */
> > +	lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
> > +	lnkcap &= ~PCI_EXP_LNKCAP_MLW;
> > +	lnkcap |= (port->is_x4 ? 4 : 1) << 4;
> 
> then this is just: lanes << 4

As only 1 and 4 are valid valid values, I chose this style (is_x4) to
ensure that no other (invalid) value is written into mvebu register.
Setting width to smaller number (if incorrect value is provided) still
allows controller to work and link should be negotiated. So setting 1 is
a sane default when controller does not have configured 4 SerDes lines
to PCIe.

> > +	mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
> > +
> >  	/* Disable Root Bridge I/O space, memory space and bus mastering. */
> >  	cmd = mvebu_readl(port, PCIE_CMD_OFF);
> >  	cmd &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> > @@ -986,6 +999,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> >  	struct device *dev = &pcie->pdev->dev;
> >  	enum of_gpio_flags flags;
> >  	int reset_gpio, ret;
> > +	u32 num_lanes;
> >  
> >  	port->pcie = pcie;
> >  
> > @@ -998,6 +1012,9 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> >  	if (of_property_read_u32(child, "marvell,pcie-lane", &port->lane))
> >  		port->lane = 0;
> >  
> > +	if (!of_property_read_u32(child, "num-lanes", &num_lanes) && num_lanes == 4)
> > +		port->is_x4 = true;
> 
> And this can be:
> 
> num_lanes = 1;
> of_property_read_u32(child, "num-lanes", &num_lanes);
> 
> If you want to validate the DT is only 1 or 4, make the DT schema do 
> that.

The problem is that there is no schema for this platform and PCIe yet.
So adding it would mean to first convert everything to schema and then
this constrain can be expressed in schema.

I'm planning to look at possibility to write schema for this platform
but I do not want to do it before open issues with representation of
other pcie properties in dts schema are resolved.

> 
> > +
> >  	port->name = devm_kasprintf(dev, GFP_KERNEL, "pcie%d.%d", port->port,
> >  				    port->lane);
> >  	if (!port->name) {
> > -- 
> > 2.20.1
> > 
> >