| Message ID | 20220704101056.24821-2-wei.fang@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add the fec node on i.MX8ULP platform | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 04/07/2022 12:10, Wei Fang wrote: > Add compatible item for i.MX8ULP platform. Wrong subject prefix (dt-bindings). Wrong subject contents - do not use some generic sentences like "update X", just write what you are doing or what you want to achieve. For example: dt-bindings: net: fsl,fec: add i.MX8 ULP FEC > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > 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. > - items: > - const: fsl,imx8mq-fec > - const: fsl,imx6sx-fec Best regards, Krzysztof
Hi Krzysztof, Sorry, I'm still a little confused. Do you mean to modify as follows? > + - items: > + - enum: > + - fsl,imx8ulp-fec > + - const: fsl,imx6ul-fec > + - const: fsl,imx6q-fec And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different. -----Original Message----- From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Sent: 2022年7月4日 17:12 To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; s.hauer@pengutronix.de Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com> Subject: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item Caution: EXT Email On 04/07/2022 12:10, Wei Fang wrote: > Add compatible item for i.MX8ULP platform. Wrong subject prefix (dt-bindings). Wrong subject contents - do not use some generic sentences like "update X", just write what you are doing or what you want to achieve. For example: dt-bindings: net: fsl,fec: add i.MX8 ULP FEC > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > 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. > - items: > - const: fsl,imx8mq-fec > - const: fsl,imx6sx-fec Best regards, Krzysztof
On 05/07/2022 04:47, Wei Fang wrote: > Hi Krzysztof, > > Sorry, I'm still a little confused. Do you mean to modify as follows? >> + - items: >> + - enum: >> + - fsl,imx8ulp-fec >> + - const: fsl,imx6ul-fec >> + - const: fsl,imx6q-fec Yes > > And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different. I understand. But if imx8ulp is the same as imx6ul and imx6ul is compatible with imx6q, then I expect imx8ulp to be compatible with imx6ul and with imx6q. Best regards, Krzysztof
Hi Krzysztof, Thanks for your suggestion, I will resubmit the patch. -----Original Message----- From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Sent: 2022年7月5日 15:27 To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org; s.hauer@pengutronix.de Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai <ping.bai@nxp.com>; sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com> Subject: Re: [EXT] Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item Caution: EXT Email On 05/07/2022 04:47, Wei Fang wrote: > Hi Krzysztof, > > Sorry, I'm still a little confused. Do you mean to modify as follows? >> + - items: >> + - enum: >> + - fsl,imx8ulp-fec >> + - const: fsl,imx6ul-fec >> + - const: fsl,imx6q-fec Yes > > And as far as I know, the imx8ulp's fec is reused from imx6ul, they both have the same features. However, the fec of imx8ulp(and imx6ul) is a little different from imx6q, therefore, the functions supported by the driver are also somewhat different. I understand. But if imx8ulp is the same as imx6ul and imx6ul is compatible with imx6q, then I expect imx8ulp to be compatible with imx6ul and with imx6q. Best regards, Krzysztof
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"; ... }; Shawn > > > - items: > > - const: fsl,imx8mq-fec > > - const: fsl,imx6sx-fec
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. Best regards, Krzysztof
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], }, { .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? Shawn
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. 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. Best regards, Krzysztof
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. > 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. Shawn
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. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: 2022年8月18日 22:04 > To: Shawn Guo <shawnguo@kernel.org> > Cc: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > s.hauer@pengutronix.de; netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai > <ping.bai@nxp.com>; sudeep.holla@arm.com; > linux-arm-kernel@lists.infradead.org; Aisheng Dong <aisheng.dong@nxp.com> > 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 ?
> Subject: RE: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item > > > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: 2022年8月18日 22:04 > > To: Shawn Guo <shawnguo@kernel.org> > > Cc: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > s.hauer@pengutronix.de; netdev@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; Jacky Bai > > <ping.bai@nxp.com>; sudeep.holla@arm.com; > > linux-arm-kernel@lists.infradead.org; Aisheng Dong > > <aisheng.dong@nxp.com> > > 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 ? Just my understanding, saying i.MX6Q supports feature A, i.MX6UL support feature A + B, Then i.MX6UL is compatible with i.MX6Q. If upper is true from hardware level, then i.MX8ULP FEC node should contain 8ulp, 6ul, 6q. But the list may expand too long if more and more devices are supported and hardware backward compatible Regards, Peng.
On 19/08/2022 06:13, Peng Fan wrote: >>> >> 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 ? > > Just my understanding, saying i.MX6Q supports feature A, > i.MX6UL support feature A + B, Then i.MX6UL is compatible with i.MX6Q. Or if i.MX8ULP can bind with any previous compatible and still work (with limited subset of features). > > If upper is true from hardware level, then i.MX8ULP FEC node > should contain 8ulp, 6ul, 6q. > > But the list may expand too long if more and more devices are supported > and hardware backward compatible True. I guess three items is the limit and anything newer should restart the sequence. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: 2022年8月19日 14:32 > To: Peng Fan <peng.fan@nxp.com>; Wei Fang <wei.fang@nxp.com>; Shawn > Guo <shawnguo@kernel.org> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > s.hauer@pengutronix.de; netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de; festevam@gmail.com; > dl-linux-imx <linux-imx@nxp.com>; Jacky Bai <ping.bai@nxp.com>; > sudeep.holla@arm.com; linux-arm-kernel@lists.infradead.org; Aisheng Dong > <aisheng.dong@nxp.com> > Subject: Re: [PATCH 1/3] dt-bings: net: fsl,fec: update compatible item > > On 19/08/2022 06:13, Peng Fan wrote: > >>> > >> 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 ? > > > > Just my understanding, saying i.MX6Q supports feature A, i.MX6UL > > support feature A + B, Then i.MX6UL is compatible with i.MX6Q. > > Or if i.MX8ULP can bind with any previous compatible and still work (with > limited subset of features). > > > > > If upper is true from hardware level, then i.MX8ULP FEC node should > > contain 8ulp, 6ul, 6q. > > > > But the list may expand too long if more and more devices are > > supported and hardware backward compatible > > True. I guess three items is the limit and anything newer should restart the > sequence. > So, the binding doc doesn't need to be fixed ?
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 - items: - const: fsl,imx8mq-fec - const: fsl,imx6sx-fec
Add compatible item for i.MX8ULP platform. Signed-off-by: Wei Fang <wei.fang@nxp.com> --- Documentation/devicetree/bindings/net/fsl,fec.yaml | 4 ++++ 1 file changed, 4 insertions(+)