Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ

[Lucas Stach]

Hi Andrey,

Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@xxxxxxx> 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?

Yes, after rereading the patch with this in mind I agree that having
the GPR offset on DT directly is IMO the better approach than an
abstract ID.

One other thing I noticed is that we probably need some property to
encode if the clock is supplied by an external clkgen, or if the clock
is provided by the i.MX. Hardcoding this in the driver will lead to DT
backward compatibility headaches later on if someone decides to build a
board with the clock provided from the i.MX.

Regards,
Lucas