Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181117181225.10737-1-andrew.smirnov@gmail.com>
 <20181117181225.10737-4-andrew.smirnov@gmail.com> <ce2ac1a880ce2a7a39f0e42b5059676daa748e52.camel@nxp.com>
In-Reply-To: <ce2ac1a880ce2a7a39f0e42b5059676daa748e52.camel@nxp.com>
From:   Andrey Smirnov <andrew.smirnov@gmail.com>
Date:   Mon, 26 Nov 2018 10:24:31 -0800
Message-ID: <CAHQ1cqH6LT4GsxUchrmVG1m3fqawgyUjxdHvh=SzxhBiQOd8eg@mail.gmail.com>
Subject: Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
To:     Leonard Crestez <leonard.crestez@nxp.com>,
        Lucas Stach <l.stach@pengutronix.de>
Cc:     Richard Zhu <hongxing.zhu@nxp.com>, linux-imx@nxp.com,
        Chris Healy <cphealy@gmail.com>,
        Dong Aisheng <aisheng.dong@nxp.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Fabio Estevam <fabio.estevam@nxp.com>,
        Mark Rutland <mark.rutland@arm.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > -     case IMX7D:
> > +     case IMX8MQ:
> > +             if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > +                                      &imx6_pcie->gpr1x)) {
> > +                     dev_err(dev, "Failed to get GPR1x address\n");
> > +                     return -EINVAL;
> > +             }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
>

I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?

> > +
> > +             if (of_property_read_u32_array(
> > +                         node, "fsl,gpr12-device-type",
> > +                         imx6_pcie->device_type,
> > +                         ARRAY_SIZE(imx6_pcie->device_type))) {
> > +                     dev_err(dev, "Failed to get device type
> > mask/value\n");
> > +                     return -EINVAL;
> > +             }
>
> The device type can be set on multiple SOCs, why are you adding a
> mandatory property only for 8m?

My thinking was that other SoCs don't really have two controllers, so
they don't really need to have that property, but, more importantly,
not forcing them to have this property should preserve backwards
compatibility with old DTBs.

>
> There should probably be a separate patch with documented DT bindings.
>

Yes, definitely, I just wanted to come up with a set of bindings
agreed on by the driver maintainers first.

Thanks,
Andrey Smirnov