| Message ID | TYZPR01MB5556D5568546D6DA4313209EC9762@TYZPR01MB5556.apcprd01.prod.exchangelabs.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipq5018: enable ethernet support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: > Signed-off-by: Ziyang Huang <hzyitc@outlook.com> You need to say something in the commit message. One obvious thing is to justify not using the at803x driver, since > +#define IPQ5018_PHY_ID 0x004dd0c0 is in the Atheros OUI range. > +static int ipq5018_probe(struct phy_device *phydev) > +{ > + struct ipq5018_phy *priv; > + struct device *dev = &phydev->mdio.dev; > + char name[64]; > + int ret; I guess you are new to mainline network. Please read: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html Section 1.6.4. > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return dev_err_probe(dev, -ENOMEM, > + "failed to allocate priv\n"); Please read the documentation of dev_err_probe() and this fix the obvious problem with this. > + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); > + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > + TX_RX_CLK_RATE); > + if (IS_ERR_OR_NULL(priv->clk_rx)) > + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), > + "failed to register rx clock\n"); > + > + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); > + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > + TX_RX_CLK_RATE); > + if (IS_ERR_OR_NULL(priv->clk_tx)) > + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), > + "failed to register tx clock\n"); > + > + priv->clk_data = devm_kzalloc(dev, > + struct_size(priv->clk_data, hws, 2), > + GFP_KERNEL); > + if (!priv->clk_data) > + return dev_err_probe(dev, -ENOMEM, > + "failed to allocate clk_data\n"); > + > + priv->clk_data->num = 2; > + priv->clk_data->hws[0] = priv->clk_rx; > + priv->clk_data->hws[1] = priv->clk_tx; > + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, > + priv->clk_data); > + if (ret) > + return dev_err_probe(dev, ret, > + "fail to register clock provider\n"); This needs an explanation. Why register two fixed clocks, which you never use? Why not put these two clocks in DT? > + > + return 0; > +} > + > +static int ipq5018_soft_reset(struct phy_device *phydev) > +{ > + int ret; > + > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > + IPQ5018_PHY_FIFO_RESET, 0); > + if (ret < 0) > + return ret; > + > + msleep(50); > + > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); > + if (ret < 0) > + return ret; > + > + return 0; > +} This needs an explanation. It is also somewhat like qca808x_link_change_notify(). Is it really sufficient to only do this reset FIFO during a soft reset, or is it needed when ever the link changes? You also appear to be missing device tree bindings. Andrew --- pw-bot: cr
在 2024/1/22 0:19, Andrew Lunn 写道: > On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: >> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > > You need to say something in the commit message. One obvious thing is > to justify not using the at803x driver, since I want add more descriptions here. But I have no idea what to write. This is a mininal driver for a special phy. Here is the thing, at first, I was tring to add these into at803x driver, then I found that the IPQ5018 internel phy is a special device. The initialization sequence is initing GCC clock and reset control, then registering clocks providers, which is very different from other devices. What's more, I remembered it *wrongly* and thought it need to be accessed through MMIO. After checking the vendor code again, this doesn't exist. So it seem like that it's a good idea to move it back to at803x driver. > >> +#define IPQ5018_PHY_ID 0x004dd0c0 > > is in the Atheros OUI range. > >> +static int ipq5018_probe(struct phy_device *phydev) >> +{ >> + struct ipq5018_phy *priv; >> + struct device *dev = &phydev->mdio.dev; >> + char name[64]; >> + int ret; > > I guess you are new to mainline network. Please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > Section 1.6.4. Sorry for missing it, will read it later. > >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate priv\n"); > > Please read the documentation of dev_err_probe() and this fix the > obvious problem with this. I had read it and I had known this helper function is to resolve the duplicate code for EPROBE_DEFER. But it also say "Note that it is deemed acceptable to use this function for error prints during probe even if the @err is known to never be -EPROBE_DEFER. The benefit compared to a normal dev_err() is the standardized format of the error code and the fact that the error code is returned." And I can find the same code in other driver, so I thought it is a standard. Or should I just return -ENOMEM? Please let me known. > >> + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); >> + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_rx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), >> + "failed to register rx clock\n"); >> + >> + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); >> + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_tx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), >> + "failed to register tx clock\n"); >> + >> + priv->clk_data = devm_kzalloc(dev, >> + struct_size(priv->clk_data, hws, 2), >> + GFP_KERNEL); >> + if (!priv->clk_data) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate clk_data\n"); >> + >> + priv->clk_data->num = 2; >> + priv->clk_data->hws[0] = priv->clk_rx; >> + priv->clk_data->hws[1] = priv->clk_tx; >> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, >> + priv->clk_data); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "fail to register clock provider\n"); > > This needs an explanation. Why register two fixed clocks, which you > never use? Why not put these two clocks in DT? Without documentions, here is my guess: This is required by GCC controller. GCC driver require TX and RX clocks from GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered in GCC controller. Will add a figure describing the clock tree in UNIPHY driver. The frequency of these clocks is depends on the mode. For GEPHY, it only supports SGMII ( Or something similar, this is a internal bus ) and the clock is fixed at 1.25G. But for UNIPHY, is supports more mode like SGMII+ which used 3.125G. > >> + >> + return 0; >> +} >> + >> +static int ipq5018_soft_reset(struct phy_device *phydev) >> +{ >> + int ret; >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, 0); >> + if (ret < 0) >> + return ret; >> + >> + msleep(50); >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} > > This needs an explanation. It is also somewhat like > qca808x_link_change_notify(). Is it really sufficient to only do this > reset FIFO during a soft reset, or is it needed when ever the link > changes? I'm not sure here, this is what u-boot does. But I guess, we can reset GCC_GEPHY_* serial reset_controls. > > You also appear to be missing device tree bindings. Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in next patches. > > Andrew > > --- > pw-bot: cr >
On Mon, Jan 22, 2024 at 11:37:29PM +0800, Ziyang Huang wrote: > 在 2024/1/22 0:19, Andrew Lunn 写道: > > On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: > > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > > > > You need to say something in the commit message. One obvious thing is > > to justify not using the at803x driver, since > > I want add more descriptions here. But I have no idea what to write. This is > a mininal driver for a special phy. So say how it is special. Indicate why it needs a minimal driver. Does the hardware support cable test? WoL? Does it perform downshift and you can read the actual speed from the AT803X_SPECIFIC_STATUS registers? What we want to avoid is that you start with a special driver, and then start copying bits of the at803x driver to support the hardware features. The at803x.c driver already supports a few internal PHYs: "Qualcomm Atheros AR9331 built-in", "Qualcomm Atheros QCA9561 built-in PHY", "Qualcomm Atheros 8337 internal PHY", "Qualcomm Atheros 8327-A internal PHY", "Qualcomm Atheros 8327-B internal PHY", so please add it to the driver and test what additional features work for it. > Here is the thing, at first, I was tring to add these into at803x driver, > then I found that the IPQ5018 internel phy is a special device. The > initialization sequence is initing GCC clock and reset control, then > registering clocks providers, which is very different from other devices. That is a different story all together, and part of the problems we had with Qualcomm patches. It might be you need to use ID values in the compatible to get this driver loaded. The PHY driver can then enable the clocks it needs and take itself out of reset. What is important here is an accurate device tree representation. What clocks and resets does this device consume. > > > + if (!priv) > > > + return dev_err_probe(dev, -ENOMEM, > > > + "failed to allocate priv\n"); > > > > Please read the documentation of dev_err_probe() and this fix the > > obvious problem with this. > And I can find the same code in other driver, so I thought it is a standard. > Or should I just return -ENOMEM? Please let me known. -ENOMEM is one of the error codes you are unlikely to see. And if it does happen, you are going to have cascading errors. So just return -ENOMEM. > > > + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); > > > + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > > > + TX_RX_CLK_RATE); > > > + if (IS_ERR_OR_NULL(priv->clk_rx)) > > > + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), > > > + "failed to register rx clock\n"); > > > + > > > + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); > > > + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, > > > + TX_RX_CLK_RATE); > > > + if (IS_ERR_OR_NULL(priv->clk_tx)) > > > + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), > > > + "failed to register tx clock\n"); > > > + > > > + priv->clk_data = devm_kzalloc(dev, > > > + struct_size(priv->clk_data, hws, 2), > > > + GFP_KERNEL); > > > + if (!priv->clk_data) > > > + return dev_err_probe(dev, -ENOMEM, > > > + "failed to allocate clk_data\n"); > > > + > > > + priv->clk_data->num = 2; > > > + priv->clk_data->hws[0] = priv->clk_rx; > > > + priv->clk_data->hws[1] = priv->clk_tx; > > > + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, > > > + priv->clk_data); > > > + if (ret) > > > + return dev_err_probe(dev, ret, > > > + "fail to register clock provider\n"); > > > > This needs an explanation. Why register two fixed clocks, which you > > never use? Why not put these two clocks in DT? > > Without documentions, here is my guess: So you don't have the data sheet? Are you working from the Qualcomm vendor tree? > This is required by GCC controller. GCC driver require TX and RX clocks from > GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to > GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered > in GCC controller. Will add a figure describing the clock tree in UNIPHY > driver. So in this case, the GCC is a clock consumer and the PHY is a clock provider. Does GCC use DT properties clocks/clock-names and phandles to reference these clocks it is consuming? If so, you can just use fixed-clocks in DT. > > > +static int ipq5018_soft_reset(struct phy_device *phydev) > > > +{ > > > + int ret; > > > + > > > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > > > + IPQ5018_PHY_FIFO_RESET, 0); > > > + if (ret < 0) > > > + return ret; > > > + > > > + msleep(50); > > > + > > > + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, > > > + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); > > > + if (ret < 0) > > > + return ret; > > > + > > > + return 0; > > > +} > > > > This needs an explanation. It is also somewhat like > > qca808x_link_change_notify(). Is it really sufficient to only do this > > reset FIFO during a soft reset, or is it needed when ever the link > > changes? > > I'm not sure here, this is what u-boot does. But I guess, we can reset > GCC_GEPHY_* serial reset_controls. Please add a comment with your best guess what it is doing and why. Is this vendor u-boot, or upstream u-boot? Maybe the git history will give you more details. > > You also appear to be missing device tree bindings. > > Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in > next patches. The DT binding is just as important as the code. Often the DT binding is so broken we don't even bother looking at the code... Andrew
在 2024/1/23 1:18, Andrew Lunn 写道: > On Mon, Jan 22, 2024 at 11:37:29PM +0800, Ziyang Huang wrote: >> 在 2024/1/22 0:19, Andrew Lunn 写道: >>> On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: >>>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> >>> >>> You need to say something in the commit message. One obvious thing is >>> to justify not using the at803x driver, since >> >> I want add more descriptions here. But I have no idea what to write. This is >> a mininal driver for a special phy. > > So say how it is special. Indicate why it needs a minimal driver. > > Does the hardware support cable test? WoL? Does it perform downshift > and you can read the actual speed from the AT803X_SPECIFIC_STATUS > registers? > > What we want to avoid is that you start with a special driver, and > then start copying bits of the at803x driver to support the hardware > features. The at803x.c driver already supports a few internal PHYs: > "Qualcomm Atheros AR9331 built-in", "Qualcomm Atheros QCA9561 built-in > PHY", "Qualcomm Atheros 8337 internal PHY", "Qualcomm Atheros 8327-A > internal PHY", "Qualcomm Atheros 8327-B internal PHY", so please add > it to the driver and test what additional features work for it. After rechecking the vendor code, you are right. The only special thing of this device is that it's a combined device of UNIPHY and at803x general phy. So it needs the UNIPHY initialization sequence. But for the PHY part, it's almost same as others, just has some special registers. I will merge it into at803x driver. > >> Here is the thing, at first, I was tring to add these into at803x driver, >> then I found that the IPQ5018 internel phy is a special device. The >> initialization sequence is initing GCC clock and reset control, then >> registering clocks providers, which is very different from other devices. > > That is a different story all together, and part of the problems we > had with Qualcomm patches. It might be you need to use ID values in > the compatible to get this driver loaded. The PHY driver can then > enable the clocks it needs and take itself out of reset. What is > important here is an accurate device tree representation. What clocks > and resets does this device consume. Ok, will try to do this. > >>>> + if (!priv) >>>> + return dev_err_probe(dev, -ENOMEM, >>>> + "failed to allocate priv\n"); >>> >>> Please read the documentation of dev_err_probe() and this fix the >>> obvious problem with this. > >> And I can find the same code in other driver, so I thought it is a standard. >> Or should I just return -ENOMEM? Please let me known. > > -ENOMEM is one of the error codes you are unlikely to see. And if it > does happen, you are going to have cascading errors. So just return > -ENOMEM. ok, got it. Thanks. > >>>> + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); >>>> + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >>>> + TX_RX_CLK_RATE); >>>> + if (IS_ERR_OR_NULL(priv->clk_rx)) >>>> + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), >>>> + "failed to register rx clock\n"); >>>> + >>>> + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); >>>> + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >>>> + TX_RX_CLK_RATE); >>>> + if (IS_ERR_OR_NULL(priv->clk_tx)) >>>> + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), >>>> + "failed to register tx clock\n"); >>>> + >>>> + priv->clk_data = devm_kzalloc(dev, >>>> + struct_size(priv->clk_data, hws, 2), >>>> + GFP_KERNEL); >>>> + if (!priv->clk_data) >>>> + return dev_err_probe(dev, -ENOMEM, >>>> + "failed to allocate clk_data\n"); >>>> + >>>> + priv->clk_data->num = 2; >>>> + priv->clk_data->hws[0] = priv->clk_rx; >>>> + priv->clk_data->hws[1] = priv->clk_tx; >>>> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, >>>> + priv->clk_data); >>>> + if (ret) >>>> + return dev_err_probe(dev, ret, >>>> + "fail to register clock provider\n"); >>> >>> This needs an explanation. Why register two fixed clocks, which you >>> never use? Why not put these two clocks in DT? >> >> Without documentions, here is my guess: > > So you don't have the data sheet? Are you working from the Qualcomm > vendor tree? Unfortunately, Yes. I couldn't find any documentions about this part. So I read the Qualcomm code, tried to realize the logic and guessed the functions of registers. Base on my understand, I use MACRO to describe these registers for human-readable and examined them. > >> This is required by GCC controller. GCC driver require TX and RX clocks from >> GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to >> GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered >> in GCC controller. Will add a figure describing the clock tree in UNIPHY >> driver. > > So in this case, the GCC is a clock consumer and the PHY is a clock > provider. Does GCC use DT properties clocks/clock-names and phandles > to reference these clocks it is consuming? If so, you can just use > fixed-clocks in DT. Yes, GCC use DT handler to refer these clocks. Will try as your said. > >>>> +static int ipq5018_soft_reset(struct phy_device *phydev) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >>>> + IPQ5018_PHY_FIFO_RESET, 0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + msleep(50); >>>> + >>>> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >>>> + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + return 0; >>>> +} >>> >>> This needs an explanation. It is also somewhat like >>> qca808x_link_change_notify(). Is it really sufficient to only do this >>> reset FIFO during a soft reset, or is it needed when ever the link >>> changes? >> >> I'm not sure here, this is what u-boot does. But I guess, we can reset >> GCC_GEPHY_* serial reset_controls. > > Please add a comment with your best guess what it is doing and why. Is > this vendor u-boot, or upstream u-boot? Maybe the git history will > give you more details. Ok, I will also try to replace them with the series of GCC_GEPHY_* reset_controls and check whether it work. > >>> You also appear to be missing device tree bindings. >> >> Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in >> next patches. > > The DT binding is just as important as the code. Often the DT binding > is so broken we don't even bother looking at the code... Will write them. > > Andrew
> After rechecking the vendor code, you are right. The only special thing of > this device is that it's a combined device of UNIPHY and at803x general phy. > So it needs the UNIPHY initialization sequence. But for the PHY part, it's > almost same as others, just has some special registers. I will merge it into > at803x driver. The UNIPHY is a separate driver, its a generic PHY driver? Can we keep them separate for this internal PHY as well? The initialisation sequence is what is going to be most 'interesting' here. How UNIPHY, this PHY and the GCC all come together to make it work. But for the moment, i think its best the PHY driver controls its own clock input and reset, using standard Linux APIs, once the driver has probed via compatible IDs. Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 107880d13d21..2d068fea7008 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -202,6 +202,11 @@ config INTEL_XWAY_PHY PEF 7061, PEF 7071 and PEF 7072 or integrated into the Intel SoCs xRX200, xRX300, xRX330, xRX350 and xRX550. +config IPQ5018_INTERNAL_PHY + tristate "Qualcomm IPQ5018 internal PHY" + help + Supports for the Qualcomm IPQ5018 internal PHY. + config LSI_ET1011C_PHY tristate "LSI ET1011C PHY" help diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index c945ed9bd14b..16d65378ae34 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_DP83TD510_PHY) += dp83td510.o obj-$(CONFIG_FIXED_PHY) += fixed_phy.o obj-$(CONFIG_ICPLUS_PHY) += icplus.o obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o +obj-$(CONFIG_IPQ5018_INTERNAL_PHY) += ipq5018-internal.o obj-$(CONFIG_LSI_ET1011C_PHY) += et1011c.o obj-$(CONFIG_LXT_PHY) += lxt.o obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o diff --git a/drivers/net/phy/ipq5018-internal.c b/drivers/net/phy/ipq5018-internal.c new file mode 100644 index 000000000000..d1331951b4d8 --- /dev/null +++ b/drivers/net/phy/ipq5018-internal.c @@ -0,0 +1,125 @@ +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/phy.h> +#include <linux/reset.h> + +#define IPQ5018_PHY_ID 0x004dd0c0 + +#define TX_RX_CLK_RATE 125000000 /* 125M */ + +#define IPQ5018_PHY_FIFO_CONTROL 0x19 +#define IPQ5018_PHY_FIFO_RESET GENMASK(1, 0) + +struct ipq5018_phy { + int num_clks; + struct clk_bulk_data *clks; + struct reset_control *rst; + + struct clk_hw *clk_rx, *clk_tx; + struct clk_hw_onecell_data *clk_data; +}; + +static int ipq5018_probe(struct phy_device *phydev) +{ + struct ipq5018_phy *priv; + struct device *dev = &phydev->mdio.dev; + char name[64]; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return dev_err_probe(dev, -ENOMEM, + "failed to allocate priv\n"); + + priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks); + if (priv->num_clks < 0) + return dev_err_probe(dev, priv->num_clks, + "failed to acquire clocks\n"); + + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); + if (ret) + return dev_err_probe(dev, ret, + "failed to enable clocks\n"); + + priv->rst = devm_reset_control_array_get_exclusive(dev); + if (IS_ERR_OR_NULL(priv->rst)) + return dev_err_probe(dev, PTR_ERR(priv->rst), + "failed to acquire reset\n"); + + ret = reset_control_reset(priv->rst); + if (ret) + return dev_err_probe(dev, ret, + "failed to reset\n"); + + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, + TX_RX_CLK_RATE); + if (IS_ERR_OR_NULL(priv->clk_rx)) + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), + "failed to register rx clock\n"); + + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, + TX_RX_CLK_RATE); + if (IS_ERR_OR_NULL(priv->clk_tx)) + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), + "failed to register tx clock\n"); + + priv->clk_data = devm_kzalloc(dev, + struct_size(priv->clk_data, hws, 2), + GFP_KERNEL); + if (!priv->clk_data) + return dev_err_probe(dev, -ENOMEM, + "failed to allocate clk_data\n"); + + priv->clk_data->num = 2; + priv->clk_data->hws[0] = priv->clk_rx; + priv->clk_data->hws[1] = priv->clk_tx; + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, + priv->clk_data); + if (ret) + return dev_err_probe(dev, ret, + "fail to register clock provider\n"); + + return 0; +} + +static int ipq5018_soft_reset(struct phy_device *phydev) +{ + int ret; + + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, + IPQ5018_PHY_FIFO_RESET, 0); + if (ret < 0) + return ret; + + msleep(50); + + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); + if (ret < 0) + return ret; + + return 0; +} + +static struct phy_driver ipq5018_internal_phy_driver[] = { + { + PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID), + .name = "Qualcomm IPQ5018 internal PHY", + .flags = PHY_IS_INTERNAL, + .probe = ipq5018_probe, + .soft_reset = ipq5018_soft_reset, + }, +}; +module_phy_driver(ipq5018_internal_phy_driver); + +static struct mdio_device_id __maybe_unused ipq5018_internal_phy_ids[] = { + { PHY_ID_MATCH_EXACT(IPQ5018_PHY_ID) }, + { } +}; +MODULE_DEVICE_TABLE(mdio, ipq5018_internal_phy_ids); + +MODULE_DESCRIPTION("Qualcomm IPQ5018 internal PHY driver"); +MODULE_AUTHOR("Ziyang Huang <hzyitc@outlook.com>");
Signed-off-by: Ziyang Huang <hzyitc@outlook.com> --- drivers/net/phy/Kconfig | 5 ++ drivers/net/phy/Makefile | 1 + drivers/net/phy/ipq5018-internal.c | 125 +++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100644 drivers/net/phy/ipq5018-internal.c