diff mbox series

[1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver

Message ID TYZPR01MB5556D5568546D6DA4313209EC9762@TYZPR01MB5556.apcprd01.prod.exchangelabs.com (mailing list archive)
State New, archived
Headers show
Series ipq5018: enable ethernet support | expand

Commit Message

Ziyang Huang Jan. 21, 2024, 12:42 p.m. UTC
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

Comments

Andrew Lunn Jan. 21, 2024, 4:19 p.m. UTC | #1
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
Ziyang Huang Jan. 22, 2024, 3:37 p.m. UTC | #2
在 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
>
Andrew Lunn Jan. 22, 2024, 5:18 p.m. UTC | #3
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
Ziyang Huang Jan. 23, 2024, 3:38 p.m. UTC | #4
在 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
Andrew Lunn Jan. 23, 2024, 11:15 p.m. UTC | #5
> 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 mbox series

Patch

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>");