RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: 2022年8月18日 22:04
> To: Shawn Guo <shawnguo@xxxxxxxxxx>
> Cc: Wei Fang <wei.fang@xxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> dl-linux-imx <linux-imx@xxxxxxx>; Peng Fan <peng.fan@xxxxxxx>; Jacky Bai
> <ping.bai@xxxxxxx>; sudeep.holla@xxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Aisheng Dong <aisheng.dong@xxxxxxx>
> Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item
> 
> On 18/08/2022 16:57, Shawn Guo wrote:
> > On Thu, Aug 18, 2022 at 12:46:33PM +0300, Krzysztof Kozlowski wrote:
> >> On 18/08/2022 12:22, Shawn Guo wrote:
> >>> On Thu, Aug 18, 2022 at 10:51:02AM +0300, Krzysztof Kozlowski wrote:
> >>>> On 18/08/2022 04:33, Shawn Guo wrote:
> >>>>> On Mon, Jul 04, 2022 at 11:12:09AM +0200, Krzysztof Kozlowski wrote:
> >>>>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> index daa2f79a294f..6642c246951b 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/net/fsl,fec.yaml
> >>>>>>> @@ -40,6 +40,10 @@ properties:
> >>>>>>>            - enum:
> >>>>>>>                - fsl,imx7d-fec
> >>>>>>>            - const: fsl,imx6sx-fec
> >>>>>>> +      - items:
> >>>>>>> +          - enum:
> >>>>>>> +              - fsl,imx8ulp-fec
> >>>>>>> +          - const: fsl,imx6ul-fec
> >>>>>>
> >>>>>> This is wrong.  fsl,imx6ul-fec has to be followed by
> >>>>>> fsl,imx6q-fec. I think someone made similar mistakes earlier so this is a
> mess.
> >>>>>
> >>>>> Hmm, not sure I follow this.  Supposing we want to have the
> >>>>> following compatible for i.MX8ULP FEC, why do we have to have
> "fsl,imx6q-fec"
> >>>>> here?
> >>>>>
> >>>>> 	fec: ethernet@29950000 {
> >>>>> 		compatible = "fsl,imx8ulp-fec", "fsl,imx6ul-fec";
> >>>>> 		...
> >>>>> 	};
> >>>>
> >>>> Because a bit earlier this bindings is saying that fsl,imx6ul-fec
> >>>> must be followed by fsl,imx6q-fec.
> >>>
> >>> The FEC driver OF match table suggests that fsl,imx6ul-fec and
> >>> fsl,imx6q-fec are not really compatible.
> >>>
> >>> static const struct of_device_id fec_dt_ids[] = {
> >>>         { .compatible = "fsl,imx25-fec", .data =
> &fec_devtype[IMX25_FEC], },
> >>>         { .compatible = "fsl,imx27-fec", .data =
> &fec_devtype[IMX27_FEC], },
> >>>         { .compatible = "fsl,imx28-fec", .data =
> &fec_devtype[IMX28_FEC], },
> >>>         { .compatible = "fsl,imx6q-fec", .data =
> &fec_devtype[IMX6Q_FEC], },
> >>>         { .compatible = "fsl,mvf600-fec", .data =
> &fec_devtype[MVF600_FEC], },
> >>>         { .compatible = "fsl,imx6sx-fec", .data =
> &fec_devtype[IMX6SX_FEC], },
> >>>         { .compatible = "fsl,imx6ul-fec", .data =
> >>> &fec_devtype[IMX6UL_FEC], },
> >>
> >> I don't see here any incompatibility. Binding driver with different
> >> driver data is not a proof of incompatible devices.
> >
> > To me, different driver data is a good sign of incompatibility.  It
> > mostly means that software needs to program the hardware block
> > differently.
> 
> Any device being 100% compatible with old one and having additional features
> will have different driver data... so no, it's not a proof.
> There are many of such examples and we call them compatible, because the
> device could operate when bound by the fallback compatible.
> 
> If this is the case here - how do I know? I raised and the answer was
> affirmative...
> 
> >
> >
> >> Additionally, the
> >> binding describes the hardware, not the driver.
> >>
> >>>         { .compatible = "fsl,imx8mq-fec", .data =
> &fec_devtype[IMX8MQ_FEC], },
> >>>         { .compatible = "fsl,imx8qm-fec", .data =
> &fec_devtype[IMX8QM_FEC], },
> >>>         { /* sentinel */ }
> >>> };
> >>> MODULE_DEVICE_TABLE(of, fec_dt_ids);
> >>>
> >>> Should we fix the binding doc?
> >>
> >> Maybe, I don't know. The binding describes the hardware, so based on
> >> it the devices are compatible. Changing this, except ABI impact,
> >> would be possible with proper reason, but not based on Linux driver code.
> >
> > Well, if Linux driver code is written in the way that hardware
> > requires, I guess that's just based on hardware characteristics.
> >
> > To me, having a device compatible to two devices that require
> > different programming model is unnecessary and confusing.
> 
> It's the first time anyone mentions here the programming model is different... If
> it is different, the devices are likely not compatible.
> 
> However when I raised this issue last time, there were no concerns with calling
> them all compatible. Therefore I wonder if the folks working on this driver
> actually know what's there... I don't know, I gave recommendations based on
> what is described in the binding and expect the engineer to come with that
> knowledge.
> 
> 
Sorry, I did not explain clearly last time, I just mentioned that imx8ulp fec was
totally reused from imx6ul and was a little different from imx6q.
So what should I do next? Should I fix the binding doc ?