Patchwork [v8,2/3] net: hisilicon: new hip04 MDIO driver

login
register
mail settings
Submitter Zhangfei Gao
Date April 19, 2014, 1:12 a.m.
Message ID <1397869980-21187-3-git-send-email-zhangfei.gao@linaro.org>
Download mbox | patch
Permalink /patch/340444/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Zhangfei Gao - April 19, 2014, 1:12 a.m.
Hisilicon hip04 platform mdio driver
Reuse Marvell phy drivers/net/phy/marvell.c

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/net/ethernet/Kconfig                |    1 +
 drivers/net/ethernet/Makefile               |    1 +
 drivers/net/ethernet/hisilicon/Kconfig      |   31 +++++
 drivers/net/ethernet/hisilicon/Makefile     |    5 +
 drivers/net/ethernet/hisilicon/hip04_mdio.c |  185 +++++++++++++++++++++++++++
 5 files changed, 223 insertions(+)
 create mode 100644 drivers/net/ethernet/hisilicon/Kconfig
 create mode 100644 drivers/net/ethernet/hisilicon/Makefile
 create mode 100644 drivers/net/ethernet/hisilicon/hip04_mdio.c
Sergei Shtylyov - April 21, 2014, 5:58 p.m.
Hello.

On 04/19/2014 05:12 AM, Zhangfei Gao wrote:

> Hisilicon hip04 platform mdio driver
> Reuse Marvell phy drivers/net/phy/marvell.c

> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
[...]

> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> new file mode 100644
> index 0000000..19826a3
> --- /dev/null
> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> @@ -0,0 +1,185 @@
> +

    Empty line not needed here.

> +/* Copyright (c) 2014 Linaro Ltd.
> + * Copyright (c) 2014 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

[...]

> +static int hip04_mdio_reset(struct mii_bus *bus)
> +{
> +	int temp, err, i;
> +
> +	for (i = 0; i < PHY_MAX_ADDR; i++) {
> +		hip04_mdio_write(bus, i, 22, 0);

    Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's 
MII_SREVISION...

> +		temp = hip04_mdio_read(bus, i, MII_BMCR);

    You're not checking for error...

> +		temp |= BMCR_RESET;
> +		err = hip04_mdio_write(bus, i, MII_BMCR, temp);

    Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this 
correctly...

> +		if (err < 0)
> +			return err;
> +	}
> +
> +	mdelay(500);
> +	return 0;
> +}
> +
> +static int hip04_mdio_probe(struct platform_device *pdev)
> +{
> +	struct resource *r;
> +	struct mii_bus *bus;
> +	struct hip04_mdio_priv *priv;
> +	int ret;
> +
> +	bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
> +	if (!bus) {
> +		dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	bus->name = "hip04_mdio_bus";
> +	bus->read = hip04_mdio_read;
> +	bus->write = hip04_mdio_write;
> +	bus->reset = hip04_mdio_reset;

    Ah... However I don't think it a good implementation of that bus method...

[...]

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli - April 21, 2014, 6:03 p.m.
2014-04-21 10:58 GMT-07:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 04/19/2014 05:12 AM, Zhangfei Gao wrote:
>
>> Hisilicon hip04 platform mdio driver
>> Reuse Marvell phy drivers/net/phy/marvell.c
>
>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>
> [...]
>
>
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> new file mode 100644
>> index 0000000..19826a3
>> --- /dev/null
>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>> @@ -0,0 +1,185 @@
>> +
>
>
>    Empty line not needed here.
>
>
>> +/* Copyright (c) 2014 Linaro Ltd.
>> + * Copyright (c) 2014 Hisilicon Limited.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>
>
> [...]
>
>
>> +static int hip04_mdio_reset(struct mii_bus *bus)
>> +{
>> +       int temp, err, i;
>> +
>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>> +               hip04_mdio_write(bus, i, 22, 0);
>
>
>    Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's
> MII_SREVISION...

I think this rather means clause 22 as opposed to clause 45.

>
>
>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);
>
>
>    You're not checking for error...
>
>
>> +               temp |= BMCR_RESET;
>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
>
>
>    Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this
> correctly...

Except that this runs way before we have created the PHY driver, so we
can't use that function just yet. I already asked about this, and he
explained that this was because the PHY devices he uses are not
responding correcty to MII_PHYSID1/2 reads.

>
>
>> +               if (err < 0)
>> +                       return err;
>> +       }
>> +
>> +       mdelay(500);
>> +       return 0;
>> +}
>> +
>> +static int hip04_mdio_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *r;
>> +       struct mii_bus *bus;
>> +       struct hip04_mdio_priv *priv;
>> +       int ret;
>> +
>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>> +       if (!bus) {
>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       bus->name = "hip04_mdio_bus";
>> +       bus->read = hip04_mdio_read;
>> +       bus->write = hip04_mdio_write;
>> +       bus->reset = hip04_mdio_reset;
>
>
>    Ah... However I don't think it a good implementation of that bus
> method...
>
> [...]
>
> WBR, Sergei
>
Sergei Shtylyov - April 21, 2014, 6:21 p.m.
On 04/21/2014 10:03 PM, Florian Fainelli wrote:

>>> Hisilicon hip04 platform mdio driver
>>> Reuse Marvell phy drivers/net/phy/marvell.c

>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> [...]

>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> new file mode 100644
>>> index 0000000..19826a3
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>> @@ -0,0 +1,185 @@

[...]

>>> +static int hip04_mdio_reset(struct mii_bus *bus)
>>> +{
>>> +       int temp, err, i;
>>> +
>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>>> +               hip04_mdio_write(bus, i, 22, 0);

>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me it's
>> MII_SREVISION...

> I think this rather means clause 22 as opposed to clause 45.

    No, the corresponding hip04_mdio_write()'s parameter is a register #, so 
this is a write of 0 to register #22. A comment certainly wouldn't hurt here...

>>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);

>>     You're not checking for error...

>>> +               temp |= BMCR_RESET;
>>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);

>>     Hmm, why you're open coding BMCR reset? There's phy_init_hw() doing this
>> correctly...

> Except that this runs way before we have created the PHY driver, so we
> can't use that function just yet.

    Ah, you're right.

> I already asked about this, and he
> explained that this was because the PHY devices he uses are not
> responding correcty to MII_PHYSID1/2 reads.

    So, this manual reset loop helps with reading the ID registers? A comment 
wouldn't hurt either...

>>> +               if (err < 0)
>>> +                       return err;

    I'm not at all sure we want to leave the reset loop on a first write error.

>>> +       }
>>> +
>>> +       mdelay(500);

    I'm not sure this is enough, given that in phy_init_hw() we poll for 600 
ms + 1 ms.

>>> +       return 0;
>>> +}
>>> +
>>> +static int hip04_mdio_probe(struct platform_device *pdev)
>>> +{
>>> +       struct resource *r;
>>> +       struct mii_bus *bus;
>>> +       struct hip04_mdio_priv *priv;
>>> +       int ret;
>>> +
>>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>>> +       if (!bus) {
>>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       bus->name = "hip04_mdio_bus";
>>> +       bus->read = hip04_mdio_read;
>>> +       bus->write = hip04_mdio_write;
>>> +       bus->reset = hip04_mdio_reset;

>>     Ah... However I don't think it a good implementation of that bus
>> method...

    I assumed this method exists to do a hardware reset of the whole bus, not 
to do a loop of soft-resetting all PHYs...

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao - April 22, 2014, 6:03 a.m.
On 04/22/2014 02:21 AM, Sergei Shtylyov wrote:
> On 04/21/2014 10:03 PM, Florian Fainelli wrote:
>
>>>> Hisilicon hip04 platform mdio driver
>>>> Reuse Marvell phy drivers/net/phy/marvell.c
>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>> [...]
>
>>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> new file mode 100644
>>>> index 0000000..19826a3
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
>>>> @@ -0,0 +1,185 @@
>
> [...]
>
>>>> +static int hip04_mdio_reset(struct mii_bus *bus)
>>>> +{
>>>> +       int temp, err, i;
>>>> +
>>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>>>> +               hip04_mdio_write(bus, i, 22, 0);
>
>>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me
>>> it's
>>> MII_SREVISION...
>
>> I think this rather means clause 22 as opposed to clause 45.
>
>     No, the corresponding hip04_mdio_write()'s parameter is a register
> #, so this is a write of 0 to register #22. A comment certainly wouldn't
> hurt here...

It's private register of the phy marvell 88e1512.
To make it clearer using define instead.
#define MII_MARVELL_PHY_PAGE    22

The registers has been grouped into several pages, access register need 
choose which page first.
>
>>>> +               temp = hip04_mdio_read(bus, i, MII_BMCR);
>
>>>     You're not checking for error...
>
>>>> +               temp |= BMCR_RESET;
>>>> +               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
>
>>>     Hmm, why you're open coding BMCR reset? There's phy_init_hw()
>>> doing this
>>> correctly...
>
>> Except that this runs way before we have created the PHY driver, so we
>> can't use that function just yet.
>
>     Ah, you're right.
>
>> I already asked about this, and he
>> explained that this was because the PHY devices he uses are not
>> responding correcty to MII_PHYSID1/2 reads.
>
>     So, this manual reset loop helps with reading the ID registers? A
> comment wouldn't hurt either...
Yes, it required for get_phy_id, will add comments.

>
>>>> +               if (err < 0)
>>>> +                       return err;
>
>     I'm not at all sure we want to leave the reset loop on a first write
> error.
Yes, continue can be used instead.

>
>>>> +       }
>>>> +
>>>> +       mdelay(500);
>
>     I'm not sure this is enough, given that in phy_init_hw() we poll for
> 600 ms + 1 ms.
Though not find clear delay time required from the phy spec, the 
experiment shows OK.
Also here can not verify the BMCR_RESET bit, 0xffff will returned if the 
phy is not exists.
>
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int hip04_mdio_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct resource *r;
>>>> +       struct mii_bus *bus;
>>>> +       struct hip04_mdio_priv *priv;
>>>> +       int ret;
>>>> +
>>>> +       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
>>>> +       if (!bus) {
>>>> +               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       bus->name = "hip04_mdio_bus";
>>>> +       bus->read = hip04_mdio_read;
>>>> +       bus->write = hip04_mdio_write;
>>>> +       bus->reset = hip04_mdio_reset;
>
>>>     Ah... However I don't think it a good implementation of that bus
>>> method...
>
>     I assumed this method exists to do a hardware reset of the whole
> bus, not to do a loop of soft-resetting all PHYs...
>

It is required for each phy, if no such reset, the specific phy can NOT be
detected and get_phy_id returns 0.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - April 22, 2014, 8:22 a.m.
On Tuesday 22 April 2014 14:03:40 zhangfei wrote:
> On 04/22/2014 02:21 AM, Sergei Shtylyov wrote:
> > On 04/21/2014 10:03 PM, Florian Fainelli wrote:
> >
> >>>> Hisilicon hip04 platform mdio driver
> >>>> Reuse Marvell phy drivers/net/phy/marvell.c
> >
> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>> [...]
> >
> >>>> diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> new file mode 100644
> >>>> index 0000000..19826a3
> >>>> --- /dev/null
> >>>> +++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
> >>>> @@ -0,0 +1,185 @@
> >>>> +static int hip04_mdio_reset(struct mii_bus *bus)
> >>>> +{
> >>>> +       int temp, err, i;
> >>>> +
> >>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
> >>>> +               hip04_mdio_write(bus, i, 22, 0);
> >
> >>>     Why? What kind of a register this is? <uapi/linux/mii.h> tells me
> >>> it's
> >>> MII_SREVISION...
> >
> >> I think this rather means clause 22 as opposed to clause 45.
> >
> >     No, the corresponding hip04_mdio_write()'s parameter is a register
> > #, so this is a write of 0 to register #22. A comment certainly wouldn't
> > hurt here...
> 
> It's private register of the phy marvell 88e1512.
> To make it clearer using define instead.
> #define MII_MARVELL_PHY_PAGE    22
> 
> The registers has been grouped into several pages, access register need 
> choose which page first.

You shouldn't touch the PHY private registers in the main driver though,
this should be purely handled by drivers/net/phy/marvell.c.

I don't see support for 88e1512 there, only 88e1510 and lots of older
ones, but I assume it isn't hard to add.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao - April 22, 2014, 2:16 p.m.
On 04/22/2014 04:22 PM, Arnd Bergmann wrote:

>> It's private register of the phy marvell 88e1512.
>> To make it clearer using define instead.
>> #define MII_MARVELL_PHY_PAGE    22
>>
>> The registers has been grouped into several pages, access register need
>> choose which page first.
>
> You shouldn't touch the PHY private registers in the main driver though,
> this should be purely handled by drivers/net/phy/marvell.c.
>
> I don't see support for 88e1512 there, only 88e1510 and lots of older
> ones, but I assume it isn't hard to add.
>

88e1512 driver is already supported, same as 88e1510.
#define MARVELL_PHY_ID_MASK             0xfffffff0
So it should support 88e151x.

Reset is required here for get_phy_id, otherwise only 0 can be get.
phy_device_create will not be called, and can not match any driver.

However in the experiment, it is found BMCR_RESET is not required in fact.
Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
88e151x registers are divided into pages.
Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
Unfortunately the default page is not 0, so get_phy_id will fail.

So bus->reset still required to set the page to 0, prepared for get_phy_id.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - April 22, 2014, 2:30 p.m.
On Tuesday 22 April 2014, zhangfei wrote:
> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
> 
> >> It's private register of the phy marvell 88e1512.
> >> To make it clearer using define instead.
> >> #define MII_MARVELL_PHY_PAGE    22
> >>
> >> The registers has been grouped into several pages, access register need
> >> choose which page first.
> >
> > You shouldn't touch the PHY private registers in the main driver though,
> > this should be purely handled by drivers/net/phy/marvell.c.
> >
> > I don't see support for 88e1512 there, only 88e1510 and lots of older
> > ones, but I assume it isn't hard to add.
> >
> 
> 88e1512 driver is already supported, same as 88e1510.
> #define MARVELL_PHY_ID_MASK             0xfffffff0
> So it should support 88e151x.
> 
> Reset is required here for get_phy_id, otherwise only 0 can be get.
> phy_device_create will not be called, and can not match any driver.
> 
> However in the experiment, it is found BMCR_RESET is not required in fact.
> Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
> 88e151x registers are divided into pages.
> Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
> Unfortunately the default page is not 0, so get_phy_id will fail.
> 
> So bus->reset still required to set the page to 0, prepared for get_phy_id.

But it means that the hip04_mdio driver potentially won't work if connected
to something other than a Marvell PHY.

I noticed that the marvell_of_reg_init() does this at init time:

saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
... /* perform init */
if (page_changed)
	phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);

Is this a bug? Maybe it should always set page 0 when leaving
this function.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao - April 22, 2014, 2:58 p.m.
On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
> On Tuesday 22 April 2014, zhangfei wrote:
>> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
>>
>>>> It's private register of the phy marvell 88e1512.
>>>> To make it clearer using define instead.
>>>> #define MII_MARVELL_PHY_PAGE    22
>>>>
>>>> The registers has been grouped into several pages, access register need
>>>> choose which page first.
>>>
>>> You shouldn't touch the PHY private registers in the main driver though,
>>> this should be purely handled by drivers/net/phy/marvell.c.
>>>
>>> I don't see support for 88e1512 there, only 88e1510 and lots of older
>>> ones, but I assume it isn't hard to add.
>>>
>>
>> 88e1512 driver is already supported, same as 88e1510.
>> #define MARVELL_PHY_ID_MASK             0xfffffff0
>> So it should support 88e151x.
>>
>> Reset is required here for get_phy_id, otherwise only 0 can be get.
>> phy_device_create will not be called, and can not match any driver.
>>
>> However in the experiment, it is found BMCR_RESET is not required in fact.
>> Only hip04_mdio_write(bus, i, MII_MARVELL_PHY_PAGE, 0) has to be set.
>> 88e151x registers are divided into pages.
>> Generic MII registers is in page 0, including MII_PHYSID1 and MII_PHYSID2.
>> Unfortunately the default page is not 0, so get_phy_id will fail.
>>
>> So bus->reset still required to set the page to 0, prepared for get_phy_id.
>
> But it means that the hip04_mdio driver potentially won't work if connected
> to something other than a Marvell PHY.
>
> I noticed that the marvell_of_reg_init() does this at init time:
>
> saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> ... /* perform init */
> if (page_changed)
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
>
> Is this a bug? Maybe it should always set page 0 when leaving
> this function.
>
It is correct behavior return to the original page.

First get_phy_id, then match driver according to the id, then operation 
in the specific driver have the chance to run, including 
marvell_of_reg_init etc.

Just unlucky the default value of PHY_PAGE after power on is not 0.

Thanks




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - April 24, 2014, 12:28 p.m.
On Tuesday 22 April 2014, zhangfei wrote:
> On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
> > On Tuesday 22 April 2014, zhangfei wrote:
> >> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:

> > But it means that the hip04_mdio driver potentially won't work if connected
> > to something other than a Marvell PHY.
> >
> > I noticed that the marvell_of_reg_init() does this at init time:
> >
> > saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> > ... /* perform init */
> > if (page_changed)
> >       phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
> >
> > Is this a bug? Maybe it should always set page 0 when leaving
> > this function.
> >
> It is correct behavior return to the original page.
> 
> First get_phy_id, then match driver according to the id, then operation 
> in the specific driver have the chance to run, including 
> marvell_of_reg_init etc.
> 
> Just unlucky the default value of PHY_PAGE after power on is not 0.

Ok, so it's actually not possible to detect this case prior to having
to work around it.

How about adding a DT property as a flag to tell the driver whether
to set the page to zero or not? That would let the driver work
with different types of PHY implementations, or skip the code if
there is firmware that can already set it up to page zero.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao - April 24, 2014, 1 p.m.
On 04/24/2014 08:28 PM, Arnd Bergmann wrote:
> On Tuesday 22 April 2014, zhangfei wrote:
>> On 04/22/2014 10:30 PM, Arnd Bergmann wrote:
>>> On Tuesday 22 April 2014, zhangfei wrote:
>>>> On 04/22/2014 04:22 PM, Arnd Bergmann wrote:
>
>>> But it means that the hip04_mdio driver potentially won't work if connected
>>> to something other than a Marvell PHY.
>>>
>>> I noticed that the marvell_of_reg_init() does this at init time:
>>>
>>> saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>>> ... /* perform init */
>>> if (page_changed)
>>>        phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page);
>>>
>>> Is this a bug? Maybe it should always set page 0 when leaving
>>> this function.
>>>
>> It is correct behavior return to the original page.
>>
>> First get_phy_id, then match driver according to the id, then operation
>> in the specific driver have the chance to run, including
>> marvell_of_reg_init etc.
>>
>> Just unlucky the default value of PHY_PAGE after power on is not 0.
>
> Ok, so it's actually not possible to detect this case prior to having
> to work around it.
>
> How about adding a DT property as a flag to tell the driver whether
> to set the page to zero or not? That would let the driver work
> with different types of PHY implementations, or skip the code if
> there is firmware that can already set it up to page zero.
>

Currently we suspect it maybe caused by bootloader (uefi) has accessed 
page register and forgot to recover, still in double confirm.
Even it not caused by uefi, we can set to page 0 in uefi, and let mdio 
driver common to other phy.
So we may remove the bus->reset, and let uefi do the preparation.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 506b024..1c8dc3d 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -54,6 +54,7 @@  source "drivers/net/ethernet/neterion/Kconfig"
 source "drivers/net/ethernet/faraday/Kconfig"
 source "drivers/net/ethernet/freescale/Kconfig"
 source "drivers/net/ethernet/fujitsu/Kconfig"
+source "drivers/net/ethernet/hisilicon/Kconfig"
 source "drivers/net/ethernet/hp/Kconfig"
 source "drivers/net/ethernet/ibm/Kconfig"
 source "drivers/net/ethernet/intel/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index c0b8789..da1a435 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_NET_VENDOR_EXAR) += neterion/
 obj-$(CONFIG_NET_VENDOR_FARADAY) += faraday/
 obj-$(CONFIG_NET_VENDOR_FREESCALE) += freescale/
 obj-$(CONFIG_NET_VENDOR_FUJITSU) += fujitsu/
+obj-$(CONFIG_NET_VENDOR_HISILICON) += hisilicon/
 obj-$(CONFIG_NET_VENDOR_HP) += hp/
 obj-$(CONFIG_NET_VENDOR_IBM) += ibm/
 obj-$(CONFIG_NET_VENDOR_INTEL) += intel/
diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
new file mode 100644
index 0000000..628537f
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -0,0 +1,31 @@ 
+#
+# HISILICON device configuration
+#
+
+config NET_VENDOR_HISILICON
+	bool "Hisilicon devices"
+	default y
+	depends on ARM
+	---help---
+	  If you have a network (Ethernet) card belonging to this class, say Y
+	  and read the Ethernet-HOWTO, available from
+	  <http://www.tldp.org/docs.html#howto>.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  the questions about Hisilicon devices. If you say Y, you will be asked
+	  for your specific card in the following questions.
+
+if NET_VENDOR_HISILICON
+
+config HIP04_ETH
+	tristate "HISILICON P04 Ethernet support"
+	select PHYLIB
+	select MARVELL_PHY
+	select MFD_SYSCON
+	---help---
+	  If you wish to compile a kernel for a hardware with hisilicon p04 SoC and
+	  want to use the internal ethernet then you should answer Y to this.
+
+
+endif # NET_VENDOR_HISILICON
diff --git a/drivers/net/ethernet/hisilicon/Makefile b/drivers/net/ethernet/hisilicon/Makefile
new file mode 100644
index 0000000..1d6eb6e
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/Makefile
@@ -0,0 +1,5 @@ 
+#
+# Makefile for the HISILICON network device drivers.
+#
+
+obj-$(CONFIG_HIP04_ETH) += hip04_mdio.o
diff --git a/drivers/net/ethernet/hisilicon/hip04_mdio.c b/drivers/net/ethernet/hisilicon/hip04_mdio.c
new file mode 100644
index 0000000..19826a3
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hip04_mdio.c
@@ -0,0 +1,185 @@ 
+
+/* Copyright (c) 2014 Linaro Ltd.
+ * Copyright (c) 2014 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of_mdio.h>
+#include <linux/delay.h>
+
+#define MDIO_CMD_REG		0x0
+#define MDIO_ADDR_REG		0x4
+#define MDIO_WDATA_REG		0x8
+#define MDIO_RDATA_REG		0xc
+#define MDIO_STA_REG		0x10
+
+#define MDIO_START		BIT(14)
+#define MDIO_R_VALID		BIT(1)
+#define MDIO_READ	        (BIT(12) | BIT(11) | MDIO_START)
+#define MDIO_WRITE	        (BIT(12) | BIT(10) | MDIO_START)
+
+struct hip04_mdio_priv {
+	void __iomem *base;
+};
+
+#define WAIT_TIMEOUT 10
+static int hip04_mdio_wait_ready(struct mii_bus *bus)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	int i;
+
+	for (i = 0; readl_relaxed(priv->base + MDIO_CMD_REG) & MDIO_START; i++) {
+		if (i == WAIT_TIMEOUT)
+			return -ETIMEDOUT;
+		msleep(20);
+	}
+
+	return 0;
+}
+
+static int hip04_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	val = regnum | (mii_id << 5) | MDIO_READ;
+	writel_relaxed(val, priv->base + MDIO_CMD_REG);
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	val = readl_relaxed(priv->base + MDIO_STA_REG);
+	if (val & MDIO_R_VALID) {
+		dev_err(bus->parent, "SMI bus read not valid\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	val = readl_relaxed(priv->base + MDIO_RDATA_REG);
+	ret = val & 0xFFFF;
+out:
+	return ret;
+}
+
+static int hip04_mdio_write(struct mii_bus *bus, int mii_id,
+			    int regnum, u16 value)
+{
+	struct hip04_mdio_priv *priv = bus->priv;
+	u32 val;
+	int ret;
+
+	ret = hip04_mdio_wait_ready(bus);
+	if (ret < 0)
+		goto out;
+
+	writel_relaxed(value, priv->base + MDIO_WDATA_REG);
+	val = regnum | (mii_id << 5) | MDIO_WRITE;
+	writel_relaxed(val, priv->base + MDIO_CMD_REG);
+out:
+	return ret;
+}
+
+static int hip04_mdio_reset(struct mii_bus *bus)
+{
+	int temp, err, i;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		hip04_mdio_write(bus, i, 22, 0);
+		temp = hip04_mdio_read(bus, i, MII_BMCR);
+		temp |= BMCR_RESET;
+		err = hip04_mdio_write(bus, i, MII_BMCR, temp);
+		if (err < 0)
+			return err;
+	}
+
+	mdelay(500);
+	return 0;
+}
+
+static int hip04_mdio_probe(struct platform_device *pdev)
+{
+	struct resource *r;
+	struct mii_bus *bus;
+	struct hip04_mdio_priv *priv;
+	int ret;
+
+	bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
+	if (!bus) {
+		dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
+		return -ENOMEM;
+	}
+
+	bus->name = "hip04_mdio_bus";
+	bus->read = hip04_mdio_read;
+	bus->write = hip04_mdio_write;
+	bus->reset = hip04_mdio_reset;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev->dev));
+	bus->parent = &pdev->dev;
+	priv = bus->priv;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(priv->base)) {
+		ret = PTR_ERR(priv->base);
+		goto out_mdio;
+	}
+
+	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
+		goto out_mdio;
+	}
+
+	platform_set_drvdata(pdev, bus);
+
+	return 0;
+
+out_mdio:
+	mdiobus_free(bus);
+	return ret;
+}
+
+static int hip04_mdio_remove(struct platform_device *pdev)
+{
+	struct mii_bus *bus = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(bus);
+	mdiobus_free(bus);
+
+	return 0;
+}
+
+static const struct of_device_id hip04_mdio_match[] = {
+	{ .compatible = "hisilicon,hip04-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, hip04_mdio_match);
+
+static struct platform_driver hip04_mdio_driver = {
+	.probe = hip04_mdio_probe,
+	.remove = hip04_mdio_remove,
+	.driver = {
+		.name = "hip04-mdio",
+		.owner = THIS_MODULE,
+		.of_match_table = hip04_mdio_match,
+	},
+};
+
+module_platform_driver(hip04_mdio_driver);
+
+MODULE_DESCRIPTION("HISILICON P04 MDIO interface driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hip04-mdio");