Re: [PATCH v5 1/4] dt-bindings: imx6q-pcie: Add reg-name "dbi2" and "atu" for i.MX8M PCIe Endpoint

[Niklas Cassel]

On Thu, Oct 10, 2024 at 02:17:25AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Niklas Cassel <cassel@xxxxxxxxxx>
> > Sent: 2024年10月9日 23:00
> > To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> > Cc: robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> > shawnguo@xxxxxxxxxx; l.stach@xxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v5 1/4] dt-bindings: imx6q-pcie: Add reg-name "dbi2" and
> > "atu" for i.MX8M PCIe Endpoint
> > 
> > On Tue, Aug 13, 2024 at 03:42:20PM +0800, Richard Zhu wrote:
> > > Add reg-name: "dbi2", "atu" for i.MX8M PCIe Endpoint.
> > >
> > > For i.MX8M PCIe EP, the dbi2 and atu addresses are pre-defined in the
> > > driver. This method is not good.
> > >
> > > In commit b7d67c6130ee ("PCI: imx6: Add iMX95 Endpoint (EP) support"),
> > > Frank suggests to fetch the dbi2 and atu from DT directly. This commit
> > > is preparation to do that for i.MX8M PCIe EP.
> > >
> > > These changes wouldn't break driver function. When "dbi2" and "atu"
> > > properties are present, i.MX PCIe driver would fetch the according
> > > base addresses from DT directly. If only two reg properties are
> > > provided, i.MX PCIe driver would fall back to the old method.
> > >
> > > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > > ---
> > >  .../devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml  | 13
> > > +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> > > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> > > index a06f75df8458..84ca12e8b25b 100644
> > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> > > @@ -65,12 +65,14 @@ allOf:
> > >      then:
> > >        properties:
> > >          reg:
> > > -          minItems: 2
> > > -          maxItems: 2
> > > +          minItems: 4
> > > +          maxItems: 4
> > 
> > Now it seems like this patch has already been picked up, but how is this not
> > breaking DT backwards compatibility?
> > 
> > You are here increasing minItems, which means that an older DT should now fail
> > to validate using the new schema?
> > 
> > I thought that it was only acceptable to add new optional properties after the
> > DT binding has been accepted.
> > 
> > What am I missing?
> > 
> > 
> > If the specific compatible isn't used by any DTS in a released kernel, then I think
> > that the commit log should have clearly stated so, and explained that that is the
> > reason why it is okay to break DT backwards compatibility.
> > 
> Hi Niklas:
> Thanks for your comments and concerns.
> Up to now, the pcie_ep of i.MX8MP is only present in i.mx8mp.dtsi file.
> And it isn't used by any DTS in the release kernels.
> So, this series wouldn't break DT backwards compatibility.

Ok, this information should have been in the commit message IMO.
(Too late now, but for the next patch affecting i.mx8mp.dtsi)

In the normal case, someone reviewing a DT binding patch will of course
assume there thre is actually a DTS using the binding, thus in the
(non-normal) case where there is no DTS using the binding, I think that
you should explicitly mention that in the commit message.


Kind regards,
Niklas