| Message ID | 20240222-net-v4-0-eea68f93f090@outlook.com |
|---|---|
| Headers | show |
| Series | net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles | expand |
On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > Register the sub MDIO bus if it is found. Also implement the internal > PHY reset procedure as needed. > > + ret = clk_bulk_prepare_enable(CLK_NUM, priv->clks); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable clk bulk: %d\n", ret); > + return ret; > + } > > - clk_prepare_enable(priv->clk); > if (priv->phy_rst) > hisi_femac_phy_reset(priv); > > @@ -948,6 +993,7 @@ static const struct of_device_id hisi_femac_match[] = { > {.compatible = "hisilicon,hisi-femac-v1",}, > {.compatible = "hisilicon,hisi-femac-v2",}, > {.compatible = "hisilicon,hi3516cv300-femac",}, > + {.compatible = "hisilicon,hi3798mv200-femac",}, What's the point of growing the list? All are fully compatible according to your match table. Stop growing it and either express compatibility or use proper match data, if they are not compatible. Best regards, Krzysztof
On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > The only documented SoC Hi3516DV300 does not receive any updates from 8 > years ago. With the recent driver changes, it unlikely works for this > SoC anymore. Remove the binding for this SoC. > > Also it's hard to get the version number and it's unknown how the > version can be used. Remove them until it's really needed. > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c > index eab91e011d11..9466ca9da2bb 100644 > --- a/drivers/net/ethernet/hisilicon/hisi_femac.c > +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c > @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev) > #endif > > static const struct of_device_id hisi_femac_match[] = { > - {.compatible = "hisilicon,hisi-femac-v1",}, > - {.compatible = "hisilicon,hisi-femac-v2",}, > - {.compatible = "hisilicon,hi3516cv300-femac",}, > + {.compatible = "hisilicon,hisi-femac",}, What is happening here? Removal could be justified, but then order of your patches is totally wrong. But that hisi-femac is a no-go or provide proper rationale. Best regards, Krzysztof
On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote: > On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> The only documented SoC Hi3516DV300 does not receive any updates from 8 >> years ago. With the recent driver changes, it unlikely works for this >> SoC anymore. Remove the binding for this SoC. >> >> Also it's hard to get the version number and it's unknown how the >> version can be used. Remove them until it's really needed. >> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c >> index eab91e011d11..9466ca9da2bb 100644 >> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c >> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c >> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev) >> #endif >> >> static const struct of_device_id hisi_femac_match[] = { >> - {.compatible = "hisilicon,hisi-femac-v1",}, >> - {.compatible = "hisilicon,hisi-femac-v2",}, >> - {.compatible = "hisilicon,hi3516cv300-femac",}, >> + {.compatible = "hisilicon,hisi-femac",}, > > What is happening here? Removal could be justified, but then order of > your patches is totally wrong. But that hisi-femac is a no-go or provide > proper rationale. I don't understand exactly... In dts, we commonly have a SoC specific compatible string first, generic compatible string the second. e.g. compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd". So i think this is recommended. Or does it mean we only need them in dts, not in the driver? The generic driver only needs a generic compatible "hisilicon,hisi-femac" in all? > > Best regards, > Krzysztof >
On 27/02/2024 02:51, Yang Xiwen wrote: > On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote: >> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> The only documented SoC Hi3516DV300 does not receive any updates from 8 >>> years ago. With the recent driver changes, it unlikely works for this >>> SoC anymore. Remove the binding for this SoC. >>> >>> Also it's hard to get the version number and it's unknown how the >>> version can be used. Remove them until it's really needed. >>> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c >>> index eab91e011d11..9466ca9da2bb 100644 >>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c >>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c >>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev) >>> #endif >>> >>> static const struct of_device_id hisi_femac_match[] = { >>> - {.compatible = "hisilicon,hisi-femac-v1",}, >>> - {.compatible = "hisilicon,hisi-femac-v2",}, >>> - {.compatible = "hisilicon,hi3516cv300-femac",}, >>> + {.compatible = "hisilicon,hisi-femac",}, >> >> What is happening here? Removal could be justified, but then order of >> your patches is totally wrong. But that hisi-femac is a no-go or provide >> proper rationale. > > I don't understand exactly... In dts, we commonly have a SoC specific > compatible string first, generic compatible string the second. e.g. > > compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd". There is no generic compatible here. hi3798mv200 is soc. > > So i think this is recommended. Or does it mean we only need them in It is allowed for certain cases and recommended for even fewer ones. Do you want to say you have fully discoverable features here and you do not need any properties? Or you want to say that all possible hardware will have exactly the same programming interface (registers etc)? Best regards, Krzysztof
On 2/27/2024 2:53 PM, Krzysztof Kozlowski wrote: > On 27/02/2024 02:51, Yang Xiwen wrote: >> On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote: >>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: >>>> From: Yang Xiwen <forbidden405@outlook.com> >>>> >>>> The only documented SoC Hi3516DV300 does not receive any updates from 8 >>>> years ago. With the recent driver changes, it unlikely works for this >>>> SoC anymore. Remove the binding for this SoC. >>>> >>>> Also it's hard to get the version number and it's unknown how the >>>> version can be used. Remove them until it's really needed. >>>> >>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>> --- >>>> drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c >>>> index eab91e011d11..9466ca9da2bb 100644 >>>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c >>>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c >>>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev) >>>> #endif >>>> >>>> static const struct of_device_id hisi_femac_match[] = { >>>> - {.compatible = "hisilicon,hisi-femac-v1",}, >>>> - {.compatible = "hisilicon,hisi-femac-v2",}, >>>> - {.compatible = "hisilicon,hi3516cv300-femac",}, >>>> + {.compatible = "hisilicon,hisi-femac",}, >>> >>> What is happening here? Removal could be justified, but then order of >>> your patches is totally wrong. But that hisi-femac is a no-go or provide >>> proper rationale. >> >> I don't understand exactly... In dts, we commonly have a SoC specific >> compatible string first, generic compatible string the second. e.g. >> >> compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd". > > There is no generic compatible here. hi3798mv200 is soc. > >> >> So i think this is recommended. Or does it mean we only need them in > > It is allowed for certain cases and recommended for even fewer ones. Do > you want to say you have fully discoverable features here and you do not > need any properties? Or you want to say that all possible hardware will > have exactly the same programming interface (registers etc)? Of course not. Take FEMAC for example. The FEMAC core on Hi3516 and Hi3798 can programmed in the same way. They use the same resources(resets and clocks). Though still a bit different in hardware, basically the fifo depth etc.. Hi3516 FEMAC is not special enough to have a new compatible string, nor do hi3798 FEMAC. Nor do i think we need those undocumented version numbers, i.e. "hisilicon,hisi-femac-v1/2". As i observed, this driver is generic enough to handle all the FEMAC cores i know, no matter which version nor which SoC. This can also be concluded from the driver, the driver defined 3 compatibles but they are all treated the same. Should I remove all of them, and only leave a generic "hisilicon,hisi-femac" instead? > > Best regards, > Krzysztof >
On 27/02/2024 08:36, Yang Xiwen wrote: > On 2/27/2024 2:53 PM, Krzysztof Kozlowski wrote: >> On 27/02/2024 02:51, Yang Xiwen wrote: >>> On 2/26/2024 3:55 PM, Krzysztof Kozlowski wrote: >>>> On 22/02/2024 13:43, Yang Xiwen via B4 Relay wrote: >>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>> >>>>> The only documented SoC Hi3516DV300 does not receive any updates from 8 >>>>> years ago. With the recent driver changes, it unlikely works for this >>>>> SoC anymore. Remove the binding for this SoC. >>>>> >>>>> Also it's hard to get the version number and it's unknown how the >>>>> version can be used. Remove them until it's really needed. >>>>> >>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>> --- >>>>> drivers/net/ethernet/hisilicon/hisi_femac.c | 4 +--- >>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c >>>>> index eab91e011d11..9466ca9da2bb 100644 >>>>> --- a/drivers/net/ethernet/hisilicon/hisi_femac.c >>>>> +++ b/drivers/net/ethernet/hisilicon/hisi_femac.c >>>>> @@ -990,9 +990,7 @@ static int hisi_femac_drv_resume(struct platform_device *pdev) >>>>> #endif >>>>> >>>>> static const struct of_device_id hisi_femac_match[] = { >>>>> - {.compatible = "hisilicon,hisi-femac-v1",}, >>>>> - {.compatible = "hisilicon,hisi-femac-v2",}, >>>>> - {.compatible = "hisilicon,hi3516cv300-femac",}, >>>>> + {.compatible = "hisilicon,hisi-femac",}, >>>> >>>> What is happening here? Removal could be justified, but then order of >>>> your patches is totally wrong. But that hisi-femac is a no-go or provide >>>> proper rationale. >>> >>> I don't understand exactly... In dts, we commonly have a SoC specific >>> compatible string first, generic compatible string the second. e.g. >>> >>> compatible = "hisilicon,hi3798mv200-perictrl", "syscon", "simple-mfd". >> >> There is no generic compatible here. hi3798mv200 is soc. >> >>> >>> So i think this is recommended. Or does it mean we only need them in >> >> It is allowed for certain cases and recommended for even fewer ones. Do >> you want to say you have fully discoverable features here and you do not >> need any properties? Or you want to say that all possible hardware will >> have exactly the same programming interface (registers etc)? > > Of course not. Take FEMAC for example. The FEMAC core on Hi3516 and If they have different programming interface then you cannot use generic compatible as fallback. > Hi3798 can programmed in the same way. They use the same > resources(resets and clocks). Though still a bit different in hardware, > basically the fifo depth etc.. > > Hi3516 FEMAC is not special enough to have a new compatible string, nor > do hi3798 FEMAC. Nor do i think we need those undocumented version > numbers, i.e. "hisilicon,hisi-femac-v1/2". As i observed, this driver is > generic enough to handle all the FEMAC cores i know, no matter which > version nor which SoC. This can also be concluded from the driver, the > driver defined 3 compatibles but they are all treated the same. > > Should I remove all of them, and only leave a generic > "hisilicon,hisi-femac" instead? No. Use one SoC specific compatible as fallback. That's what we ask almost every time... Best regards, Krzysztof
Signed-off-by: Yang Xiwen <forbidden405@outlook.com> --- Changes in v4: - edit commit log to show why mdio bus clk is optional (Krzysztof Kozlowski) - add clk_bulk_disable_unprepare() during error path (Maxime Chevallier) - remove of_node_put() (Simon Horman) - remove histb-clock.h header in binding example as it's goign to be deprecated. - rearrange patches so binding comes before driver - Link to v3: https://lore.kernel.org/r/20240220-net-v3-0-b68e5b75e765@outlook.com Changes in v3: - rearrange patches to fix bot error. (Rob Herring) - rewrite commit logs (Andrew Lunn) - use clk_bulk_ APIs (Andrew Lunn) - fix uninitialization use of ret (assign to -ENODEV before goto) (Simon Horman) - Link to v2: https://lore.kernel.org/r/20240216-net-v2-0-89bd4b7065c2@outlook.com Changes in v2: - replace email. - hisi-femac: s/BUS/MACIF (Andrew Lunn) - hisi-femac: add "hisilicon,hisi-femac" compatible since the driver seems generic enough for various SoCs - hisi-femac-mdio: convert binding to YAML (Krzysztof Kozlowski) - rewrite commit logs (Krzysztof Kozlowski) - Link to v1: https://lore.kernel.org/r/20240216-net-v1-0-e0ad972cda99@outlook.com --- Yang Xiwen (6): dt-bindings: net: hisilicon-femac-mdio: convert to YAML net: mdio: hisi-femac: make clock optional dt-bindings: net: remove outdated hisilicon-femac dt-bindings: net: add hisilicon,hisi-femac net: hisilicon: add support for hisi_femac core on Hi3798MV200 net: hisi_femac: remove unused compatible strings .../bindings/net/hisilicon,hisi-femac-mdio.yaml | 43 ++++++++ .../bindings/net/hisilicon,hisi-femac.yaml | 116 +++++++++++++++++++++ .../bindings/net/hisilicon-femac-mdio.txt | 22 ---- .../devicetree/bindings/net/hisilicon-femac.txt | 41 -------- drivers/net/ethernet/hisilicon/hisi_femac.c | 78 +++++++++++--- drivers/net/mdio/mdio-hisi-femac.c | 2 +- 6 files changed, 221 insertions(+), 81 deletions(-) --- base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d change-id: 20240216-net-9a208e17c40f Best regards,