| Message ID | 20240216-net-v1-0-e0ad972cda99@outlook.com |
|---|---|
| Headers | show |
| Series | net: hisi-femac: add support for Hi3798MV200, remove unmaintained compatibles | expand |
> + for (i = 0; i < CLK_NUM; i++) { > + priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]); > + if (IS_ERR(priv->clks[i])) { > + dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i], > + PTR_ERR(priv->clks[i])); > + ret = -ENODEV; > + goto out_free_netdev; > + } The clk API has devm_clk_bulk_ versions. Please take a look at them, and see if it will simplify the code. Andrew
On 2/16/2024 7:57 AM, Andrew Lunn wrote: >> + for (i = 0; i < CLK_NUM; i++) { >> + priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]); >> + if (IS_ERR(priv->clks[i])) { >> + dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i], >> + PTR_ERR(priv->clks[i])); >> + ret = -ENODEV; >> + goto out_free_netdev; >> + } > The clk API has devm_clk_bulk_ versions. Please take a look at them, and see > if it will simplify the code. I know this API, but it can't be used. We need to control clocks individually in reset procedure. > Andrew
On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > These compatible strings are not found in any mainline dts, remove them. That's not a real reason. What about all other users? Best regards, Krzysztof
On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote: > On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> These compatible strings are not found in any mainline dts, remove them. > That's not a real reason. What about all other users? The people who want their devices being supported should post a working dts first. Having found the dts missing is strongly telling me that this SoC(Hi3516) is orphan and EOL already. I can't even find it in git commit logs. I'll argue that the old binding is simply wrong, and does not describe the hardware properly. Who knows? Could anyone tell me if the driver is still working for Hi3516 or not? I'm very willing to keep the backward compatibility if someone can tell me the effort i paid to maintain the old binding really makes sense. But the only things i found in mainline kernel about Hi3516 is an CRG(clock) driver and this femac driver. And it's been 8 years since last update for this SoC. > > Best regards, > Krzysztof >
On 16/02/2024 09:21, Yang Xiwen wrote: > On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote: >> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> These compatible strings are not found in any mainline dts, remove them. >> That's not a real reason. What about all other users? > The people who want their devices being supported should post a working > dts first. Having found the dts missing is strongly telling me that this Considering how poor HiSilicon contributions were - in numbers and quality - that's kind of expected. :( > SoC(Hi3516) is orphan and EOL already. I can't even find it in git > commit logs. I'll argue that the old binding is simply wrong, and does > not describe the hardware properly. Who knows? Could anyone tell me if > the driver is still working for Hi3516 or not? I'm very willing to keep > the backward compatibility if someone can tell me the effort i paid to > maintain the old binding really makes sense. But the only things i found > in mainline kernel about Hi3516 is an CRG(clock) driver and this femac > driver. And it's been 8 years since last update for this SoC. OK, that's fine with me, but please add parts of this explanation to the commit msg (SoC is EOL, driver looks buggy and might not even work, platform was upstreamed 8 years ago and no maintenance work happened on it, thus it looks abandoned etc.). Best regards, Krzysztof
On 2/16/2024 4:26 PM, Krzysztof Kozlowski wrote: > On 16/02/2024 09:21, Yang Xiwen wrote: >> On 2/16/2024 3:20 PM, Krzysztof Kozlowski wrote: >>> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote: >>>> From: Yang Xiwen <forbidden405@outlook.com> >>>> >>>> These compatible strings are not found in any mainline dts, remove them. >>> That's not a real reason. What about all other users? >> The people who want their devices being supported should post a working >> dts first. Having found the dts missing is strongly telling me that this > Considering how poor HiSilicon contributions were - in numbers and > quality - that's kind of expected. :( > > >> SoC(Hi3516) is orphan and EOL already. I can't even find it in git >> commit logs. I'll argue that the old binding is simply wrong, and does >> not describe the hardware properly. Who knows? Could anyone tell me if >> the driver is still working for Hi3516 or not? I'm very willing to keep >> the backward compatibility if someone can tell me the effort i paid to >> maintain the old binding really makes sense. But the only things i found >> in mainline kernel about Hi3516 is an CRG(clock) driver and this femac >> driver. And it's been 8 years since last update for this SoC. > OK, that's fine with me, but please add parts of this explanation to the > commit msg (SoC is EOL, driver looks buggy and might not even work, > platform was upstreamed 8 years ago and no maintenance work happened on > it, thus it looks abandoned etc.). For me, it's a bit lucky to find a (partially) working driver in mainline. It'll take me even more time if no mainline driver is available. In fact, i wrote the driver for mainline u-boot from scratch and it has been merged. So it's good to have this binding accepted unmodified, or i'll have to modify u-boot side driver code to keep them sync. > > Best regards, > Krzysztof >
> For me, it's a bit lucky to find a (partially) working driver in mainline. > It'll take me even more time if no mainline driver is available. In fact, i > wrote the driver for mainline u-boot from scratch and it has been merged. So > it's good to have this binding accepted unmodified, or i'll have to modify > u-boot side driver code to keep them sync. Sorry, but that is not how it works. If during review we decided it needs to be modified, you will need to modify it. I would suggest you first mainstream bindings to the kernel, because it has active DT maintainers how really care about bindings. Then get is merged to u-boot. Andrew
> + // Register the optional MDIO bus > + for_each_available_child_of_node(node, mdio_np) { > + if (of_node_name_prefix(mdio_np, "mdio")) { > + priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev); > + of_node_put(mdio_np); > + if (!priv->mdio_pdev) { > + dev_err(dev, "failed to register MDIO bus device\n"); > + goto out_free_netdev; > + } > + mdio_registered = true; > + break; > + } > + } > + > + if (!mdio_registered) > + dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n"); I don't understand the architecture of this device yet... It seems like you have an integrated PHY? In the example, you used a phy-handle to bind the MAC to the PHY. So why is the MDIO bus optional? Do the MII signals from the MAC also go to SoC pins, so you could use an external PHY? Is there a SERDES so you could connect to an SFP cage? Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used to control external PHYs? If everything is internal, fixed in silicon, no variation possible, you don't need to describe the MDIO bus in DT. The MAC driver can register it, and then get the PHY at the hard coded address it uses. Andrew
On 2/16/2024 9:23 PM, Andrew Lunn wrote: >> + // Register the optional MDIO bus >> + for_each_available_child_of_node(node, mdio_np) { >> + if (of_node_name_prefix(mdio_np, "mdio")) { >> + priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev); >> + of_node_put(mdio_np); >> + if (!priv->mdio_pdev) { >> + dev_err(dev, "failed to register MDIO bus device\n"); >> + goto out_free_netdev; >> + } >> + mdio_registered = true; >> + break; >> + } >> + } >> + >> + if (!mdio_registered) >> + dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n"); > I don't understand the architecture of this device yet... > > It seems like you have an integrated PHY? In the example, you used a > phy-handle to bind the MAC to the PHY. So why is the MDIO bus > optional? Because the MAC can also support external PHY according to the datasheet. Maybe some other SoCs didn't implement this internal PHY and used an external PHY instead. > > Do the MII signals from the MAC also go to SoC pins, so you could use > an external PHY? Is there a SERDES so you could connect to an SFP > cage? No. MII signals is not accessible outside of the SoC. The SoC only exports FEPHY pins (i.e. RXN(P) and TXN(P)). > > Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used > to control external PHYs? It can, but not for Hi3798MV200. The datasheet said it can use both internal phy or external phy. But for Hi3798MV200, seems impossible. > > If everything is internal, fixed in silicon, no variation possible, > you don't need to describe the MDIO bus in DT. The MAC driver can > register it, and then get the PHY at the hard coded address it uses. > > Andrew
On Fri, Feb 16, 2024 at 07:59:19AM +0800, Yang Xiwen wrote: > On 2/16/2024 7:57 AM, Andrew Lunn wrote: > > > + for (i = 0; i < CLK_NUM; i++) { > > > + priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]); > > > + if (IS_ERR(priv->clks[i])) { > > > + dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i], > > > + PTR_ERR(priv->clks[i])); > > > + ret = -ENODEV; > > > + goto out_free_netdev; > > > + } > > The clk API has devm_clk_bulk_ versions. Please take a look at them, and see > > if it will simplify the code. > I know this API, but it can't be used. We need to control clocks > individually in reset procedure. /** * struct clk_bulk_data - Data used for bulk clk operations. * * @id: clock consumer ID * @clk: struct clk * to store the associated clock * * The CLK APIs provide a series of clk_bulk_() API calls as * a convenience to consumers which require multiple clks. This * structure is used to manage data for these calls. */ struct clk_bulk_data { const char *id; struct clk *clk; }; You pass the bulk API calls an array of this structure. After the get has completed, you can access the individual clocks via the clk pointer. So you can use the bulk API on probe and remove when you need to operate on all three, and the single clk API for your reset handler etc. Andrew
On 2/16/2024 9:49 PM, Andrew Lunn wrote: > On Fri, Feb 16, 2024 at 07:59:19AM +0800, Yang Xiwen wrote: >> On 2/16/2024 7:57 AM, Andrew Lunn wrote: >>>> + for (i = 0; i < CLK_NUM; i++) { >>>> + priv->clks[i] = devm_clk_get_enabled(&pdev->dev, clk_strs[i]); >>>> + if (IS_ERR(priv->clks[i])) { >>>> + dev_err(dev, "failed to get enabled clk %s: %ld\n", clk_strs[i], >>>> + PTR_ERR(priv->clks[i])); >>>> + ret = -ENODEV; >>>> + goto out_free_netdev; >>>> + } >>> The clk API has devm_clk_bulk_ versions. Please take a look at them, and see >>> if it will simplify the code. >> I know this API, but it can't be used. We need to control clocks >> individually in reset procedure. > /** > * struct clk_bulk_data - Data used for bulk clk operations. > * > * @id: clock consumer ID > * @clk: struct clk * to store the associated clock > * > * The CLK APIs provide a series of clk_bulk_() API calls as > * a convenience to consumers which require multiple clks. This > * structure is used to manage data for these calls. > */ > struct clk_bulk_data { > const char *id; > struct clk *clk; > }; > > You pass the bulk API calls an array of this structure. After the get > has completed, you can access the individual clocks via the clk > pointer. So you can use the bulk API on probe and remove when you need > to operate on all three, and the single clk API for your reset handler > etc. seems okay. I'll implement this in next version. > > Andrew
On Fri, Feb 16, 2024 at 09:41:29PM +0800, Yang Xiwen wrote: > On 2/16/2024 9:23 PM, Andrew Lunn wrote: > > > + // Register the optional MDIO bus > > > + for_each_available_child_of_node(node, mdio_np) { > > > + if (of_node_name_prefix(mdio_np, "mdio")) { > > > + priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev); > > > + of_node_put(mdio_np); > > > + if (!priv->mdio_pdev) { > > > + dev_err(dev, "failed to register MDIO bus device\n"); > > > + goto out_free_netdev; > > > + } > > > + mdio_registered = true; > > > + break; > > > + } > > > + } > > > + > > > + if (!mdio_registered) > > > + dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n"); > > I don't understand the architecture of this device yet... > > > > It seems like you have an integrated PHY? In the example, you used a > > phy-handle to bind the MAC to the PHY. So why is the MDIO bus > > optional? > Because the MAC can also support external PHY according to the datasheet. > Maybe some other SoCs didn't implement this internal PHY and used an > external PHY instead. > > > > Do the MII signals from the MAC also go to SoC pins, so you could use > > an external PHY? Is there a SERDES so you could connect to an SFP > > cage? > No. MII signals is not accessible outside of the SoC. The SoC only exports > FEPHY pins (i.e. RXN(P) and TXN(P)). > > > > Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used > > to control external PHYs? > It can, but not for Hi3798MV200. The datasheet said it can use both internal > phy or external phy. But for Hi3798MV200, seems impossible. So for the Hi3798MV200 this is not optional, the MDIO bus is mandatory. Also, it sounds like it exists in the silicon. So it is better to always describe it in the .dtsi file. And i took a quick look at mdio-hisi-femac.c. It has a probe function which does: data->membase = devm_platform_ioremap_resource(pdev, 0); meaning it expects to have its own address range. It is a device of its own. That also explains the compatible. So please move the MDIO bus to a node of its own, rather than embedding it within the MAC node. Andrew
On 2/16/2024 9:58 PM, Andrew Lunn wrote: > On Fri, Feb 16, 2024 at 09:41:29PM +0800, Yang Xiwen wrote: >> On 2/16/2024 9:23 PM, Andrew Lunn wrote: >>>> + // Register the optional MDIO bus >>>> + for_each_available_child_of_node(node, mdio_np) { >>>> + if (of_node_name_prefix(mdio_np, "mdio")) { >>>> + priv->mdio_pdev = of_platform_device_create(mdio_np, NULL, dev); >>>> + of_node_put(mdio_np); >>>> + if (!priv->mdio_pdev) { >>>> + dev_err(dev, "failed to register MDIO bus device\n"); >>>> + goto out_free_netdev; >>>> + } >>>> + mdio_registered = true; >>>> + break; >>>> + } >>>> + } >>>> + >>>> + if (!mdio_registered) >>>> + dev_warn(dev, "MDIO subnode notfound. This is usually a bug.\n"); >>> I don't understand the architecture of this device yet... >>> >>> It seems like you have an integrated PHY? In the example, you used a >>> phy-handle to bind the MAC to the PHY. So why is the MDIO bus >>> optional? >> Because the MAC can also support external PHY according to the datasheet. >> Maybe some other SoCs didn't implement this internal PHY and used an >> external PHY instead. >>> Do the MII signals from the MAC also go to SoC pins, so you could use >>> an external PHY? Is there a SERDES so you could connect to an SFP >>> cage? >> No. MII signals is not accessible outside of the SoC. The SoC only exports >> FEPHY pins (i.e. RXN(P) and TXN(P)). >>> Also, do the MDIO pins go to SoC pins? Can the MDIO bus master be used >>> to control external PHYs? >> It can, but not for Hi3798MV200. The datasheet said it can use both internal >> phy or external phy. But for Hi3798MV200, seems impossible. > So for the Hi3798MV200 this is not optional, the MDIO bus is > mandatory. > > Also, it sounds like it exists in the silicon. So it is better to > always describe it in the .dtsi file. > > And i took a quick look at mdio-hisi-femac.c. It has a probe function > which does: > > data->membase = devm_platform_ioremap_resource(pdev, 0); > > meaning it expects to have its own address range. It is a device of > its own. That also explains the compatible. So please move the MDIO > bus to a node of its own, rather than embedding it within the MAC > node. It won't work. Hi3798MV200 does not have a dedicated MDIO bus clock. this clock is merged to MAC clock for this SoC. We need to enable CLK_BUS and CLK_MAC before MDIO bus access. Assigning CLK_MAC and CLK_BUS to MDIO bus device does not work either. Because the clocks will be enabled twice, the MAC controller will be unable to disable the clocks during PHY reset. Note that the source is contributed by HiSilicon almost 8 yrs ago. And they had never proved that this driver works. They are not doing things like this in downstream. > > Andrew
On Fri, Feb 16, 2024 at 02:01:08PM +0100, Andrew Lunn wrote: > > For me, it's a bit lucky to find a (partially) working driver in mainline. > > It'll take me even more time if no mainline driver is available. In fact, i > > wrote the driver for mainline u-boot from scratch and it has been merged. So > > it's good to have this binding accepted unmodified, or i'll have to modify > > u-boot side driver code to keep them sync. > > Sorry, but that is not how it works. If during review we decided it > needs to be modified, you will need to modify it. > > I would suggest you first mainstream bindings to the kernel, because > it has active DT maintainers how really care about bindings. Then get > is merged to u-boot. Just to note, the U-Boot folk are currently working on a model where they will be importing the kernel's dts files directly into their tree along with the bindings. I think they're adding dtbs_check too. Although that will be opt-in per board, it does point to an increased desire for compliance there too, which is great.
On 2/17/2024 4:05 AM, Conor Dooley wrote: > On Fri, Feb 16, 2024 at 02:01:08PM +0100, Andrew Lunn wrote: >>> For me, it's a bit lucky to find a (partially) working driver in mainline. >>> It'll take me even more time if no mainline driver is available. In fact, i >>> wrote the driver for mainline u-boot from scratch and it has been merged. So >>> it's good to have this binding accepted unmodified, or i'll have to modify >>> u-boot side driver code to keep them sync. >> Sorry, but that is not how it works. If during review we decided it >> needs to be modified, you will need to modify it. >> >> I would suggest you first mainstream bindings to the kernel, because >> it has active DT maintainers how really care about bindings. Then get >> is merged to u-boot. > Just to note, the U-Boot folk are currently working on a model where > they will be importing the kernel's dts files directly into their tree > along with the bindings. I think they're adding dtbs_check too. > Although that will be opt-in per board, it does point to an increased > desire for compliance there too, which is great. Of course. I'll sync this stuff back to u-boot once this gets accepted and merged. I begin working from u-boot simply because the Driver Model of U-Boot is much simpler than Linux's. I wrote the driver for U-Boot first to figure out how the hardware is working, then port it to Linux.
Signed-off-by: Yang Xiwen <forbidden405@outlook.com> --- Yang Xiwen (6): net: hisilicon: add support for hisi_femac core on Hi3798MV200 net: hisi_femac: remove unused compatible strings dt-bindings: net: remove outdated hisilicon-femac dt-bindings: net: add hisilicon-femac net: mdio: hisi-femac: make clock optional dt-bindings: net: hisilicon-femac-mdio: make clock optional .../bindings/net/hisilicon-femac-mdio.txt | 2 + .../devicetree/bindings/net/hisilicon-femac.txt | 41 ------- .../devicetree/bindings/net/hisilicon-femac.yaml | 125 +++++++++++++++++++++ drivers/net/ethernet/hisilicon/hisi_femac.c | 93 ++++++++++----- drivers/net/mdio/mdio-hisi-femac.c | 2 +- 5 files changed, 196 insertions(+), 67 deletions(-) --- base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d change-id: 20240216-net-9a208e17c40f Best regards,