Re: [U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Joe Hershberger]

On Fri, Dec 9, 2016 at 6:41 AM, Stefan Roese  wrote:
> Hi Phil,
>
>
> On 09.12.2016 13:38, Phil Edworthy wrote:
>>
>> On 09 December 2016 12:16, Stefan Roese wrote:
>>>
>>> On 09.12.2016 11:40, Phil Edworthy wrote:

 Signed-off-by: Phil Edworthy 
 ---
  drivers/net/phy/marvell.c | 11 +++
  1 file changed, 11 insertions(+)

 diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
 index 06029c0..a7ea435 100644
 --- a/drivers/net/phy/marvell.c
 +++ b/drivers/net/phy/marvell.c
 @@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
 .shutdown = _shutdown,
  };

 +static struct phy_driver M88E1512_driver = {
 +   .name = "Marvell 88E1512",
 +   .uid = 0x01410dd4,
 +   .mask = 0xfff,
 +   .features = PHY_GBIT_FEATURES,
 +   .config = _config,
 +   .startup = _startup,
 +   .shutdown = _shutdown,
 +};
 +
  static struct phy_driver M88E1518_driver = {
 .name = "Marvell 88E1518",
 .uid = 0x1410dd1,
 @@ -591,6 +601,7 @@ int phy_marvell_init(void)
 phy_register(_driver);
 phy_register(_driver);
 phy_register(_driver);
 +   phy_register(_driver);
 phy_register(_driver);
>>>
>>>
>>> Do you need some special handling for the M88E1512 or is it identical
>>> to the 1518 handling? If its identical, why do we need a new entry
>>> for this PHY?
>>
>> Good point, they are the same except for the uid so no need for a new
>> entry. I'll change the mask of the 1518 to cover it.

I'm not sure I get it. The previous patch was to pay attention to the
last nibble of the ID (unlike Linux) so that the special 1518 handler
is run. Now you want to have a 1510 + 1512 handler? Are you planning a
funky mask like 0xff1? Maybe it would be better to just have them
checked explicitly since there is need for discrimination within the
151x family.

>
> Thanks.
>
>> Btw, what's the best way to indicate that the code supports the 1512
>> as well? It's not so easy to see if the code supports a particular device
>> ahead of hardware arriving, other than trying to match the uid's
>> manually.
>
>
> A comment would be welcome here. Not sure if this is done in a more
> generic way in the Linux PHY drivers. You might want to check there
> as well...
>
> Thanks,
> Stefan
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Joe Hershberger]

On Fri, Dec 9, 2016 at 12:54 PM, Joe Hershberger
 wrote:
> On Fri, Dec 9, 2016 at 6:41 AM, Stefan Roese  wrote:
>> Hi Phil,
>>
>>
>> On 09.12.2016 13:38, Phil Edworthy wrote:
>>>
>>> On 09 December 2016 12:16, Stefan Roese wrote:

 On 09.12.2016 11:40, Phil Edworthy wrote:
>
> Signed-off-by: Phil Edworthy 
> ---
>  drivers/net/phy/marvell.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 06029c0..a7ea435 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
> .shutdown = _shutdown,
>  };
>
> +static struct phy_driver M88E1512_driver = {
> +   .name = "Marvell 88E1512",
> +   .uid = 0x01410dd4,
> +   .mask = 0xfff,
> +   .features = PHY_GBIT_FEATURES,
> +   .config = _config,
> +   .startup = _startup,
> +   .shutdown = _shutdown,
> +};
> +
>  static struct phy_driver M88E1518_driver = {
> .name = "Marvell 88E1518",
> .uid = 0x1410dd1,
> @@ -591,6 +601,7 @@ int phy_marvell_init(void)
> phy_register(_driver);
> phy_register(_driver);
> phy_register(_driver);
> +   phy_register(_driver);
> phy_register(_driver);


 Do you need some special handling for the M88E1512 or is it identical
 to the 1518 handling? If its identical, why do we need a new entry
 for this PHY?
>>>
>>> Good point, they are the same except for the uid so no need for a new
>>> entry. I'll change the mask of the 1518 to cover it.
>
> I'm not sure I get it. The previous patch was to pay attention to the
> last nibble of the ID (unlike Linux) so that the special 1518 handler
> is run. Now you want to have a 1510 + 1512 handler? Are you planning a
> funky mask like 0xff1? Maybe it would be better to just have them

I guess you were making the 1512 behave like the 1518, not the 1510 -
I see your follow-up patch used 0xffb as a mask.

Still seems a bit smelly to me, but I guess we can see where it ends
up with some future marvell phy.

> checked explicitly since there is need for discrimination within the
> 151x family.
>
>>
>> Thanks.
>>
>>> Btw, what's the best way to indicate that the code supports the 1512
>>> as well? It's not so easy to see if the code supports a particular device
>>> ahead of hardware arriving, other than trying to match the uid's
>>> manually.
>>
>>
>> A comment would be welcome here. Not sure if this is done in a more
>> generic way in the Linux PHY drivers. You might want to check there
>> as well...
>>
>> Thanks,
>> Stefan
>>
>> ___
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Stefan Roese]

Hi Phil,

On 09.12.2016 13:38, Phil Edworthy wrote:

On 09 December 2016 12:16, Stefan Roese wrote:

On 09.12.2016 11:40, Phil Edworthy wrote:

Signed-off-by: Phil Edworthy 
---
 drivers/net/phy/marvell.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 06029c0..a7ea435 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
.shutdown = _shutdown,
 };

+static struct phy_driver M88E1512_driver = {
+   .name = "Marvell 88E1512",
+   .uid = 0x01410dd4,
+   .mask = 0xfff,
+   .features = PHY_GBIT_FEATURES,
+   .config = _config,
+   .startup = _startup,
+   .shutdown = _shutdown,
+};
+
 static struct phy_driver M88E1518_driver = {
.name = "Marvell 88E1518",
.uid = 0x1410dd1,
@@ -591,6 +601,7 @@ int phy_marvell_init(void)
phy_register(_driver);
phy_register(_driver);
phy_register(_driver);
+   phy_register(_driver);
phy_register(_driver);


Do you need some special handling for the M88E1512 or is it identical
to the 1518 handling? If its identical, why do we need a new entry
for this PHY?

Good point, they are the same except for the uid so no need for a new
entry. I'll change the mask of the 1518 to cover it.


Thanks.


Btw, what's the best way to indicate that the code supports the 1512
as well? It's not so easy to see if the code supports a particular device
ahead of hardware arriving, other than trying to match the uid's
manually.


A comment would be welcome here. Not sure if this is done in a more
generic way in the Linux PHY drivers. You might want to check there
as well...

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Phil Edworthy]

Hi Stefan,

On 09 December 2016 12:16, Stefan Roese wrote:
> On 09.12.2016 11:40, Phil Edworthy wrote:
> > Signed-off-by: Phil Edworthy 
> > ---
> >  drivers/net/phy/marvell.c | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 06029c0..a7ea435 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
> > .shutdown = _shutdown,
> >  };
> >
> > +static struct phy_driver M88E1512_driver = {
> > +   .name = "Marvell 88E1512",
> > +   .uid = 0x01410dd4,
> > +   .mask = 0xfff,
> > +   .features = PHY_GBIT_FEATURES,
> > +   .config = _config,
> > +   .startup = _startup,
> > +   .shutdown = _shutdown,
> > +};
> > +
> >  static struct phy_driver M88E1518_driver = {
> > .name = "Marvell 88E1518",
> > .uid = 0x1410dd1,
> > @@ -591,6 +601,7 @@ int phy_marvell_init(void)
> > phy_register(_driver);
> > phy_register(_driver);
> > phy_register(_driver);
> > +   phy_register(_driver);
> > phy_register(_driver);
> 
> Do you need some special handling for the M88E1512 or is it identical
> to the 1518 handling? If its identical, why do we need a new entry
> for this PHY?
Good point, they are the same except for the uid so no need for a new
entry. I'll change the mask of the 1518 to cover it.

Btw, what's the best way to indicate that the code supports the 1512
as well? It's not so easy to see if the code supports a particular device
ahead of hardware arriving, other than trying to match the uid's
manually.

Thanks
Phil
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Stefan Roese]

On 09.12.2016 11:40, Phil Edworthy wrote:

Signed-off-by: Phil Edworthy 
---
 drivers/net/phy/marvell.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 06029c0..a7ea435 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
.shutdown = _shutdown,
 };

+static struct phy_driver M88E1512_driver = {
+   .name = "Marvell 88E1512",
+   .uid = 0x01410dd4,
+   .mask = 0xfff,
+   .features = PHY_GBIT_FEATURES,
+   .config = _config,
+   .startup = _startup,
+   .shutdown = _shutdown,
+};
+
 static struct phy_driver M88E1518_driver = {
.name = "Marvell 88E1518",
.uid = 0x1410dd1,
@@ -591,6 +601,7 @@ int phy_marvell_init(void)
phy_register(_driver);
phy_register(_driver);
phy_register(_driver);
+   phy_register(_driver);
phy_register(_driver);


Do you need some special handling for the M88E1512 or is it identical
to the 1518 handling? If its identical, why do we need a new entry
for this PHY?

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/5] net: phy: Add support for Marvell M88E1512

[Phil Edworthy]

Signed-off-by: Phil Edworthy 
---
 drivers/net/phy/marvell.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 06029c0..a7ea435 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -560,6 +560,16 @@ static struct phy_driver M88E1510_driver = {
.shutdown = _shutdown,
 };
 
+static struct phy_driver M88E1512_driver = {
+   .name = "Marvell 88E1512",
+   .uid = 0x01410dd4,
+   .mask = 0xfff,
+   .features = PHY_GBIT_FEATURES,
+   .config = _config,
+   .startup = _startup,
+   .shutdown = _shutdown,
+};
+
 static struct phy_driver M88E1518_driver = {
.name = "Marvell 88E1518",
.uid = 0x1410dd1,
@@ -591,6 +601,7 @@ int phy_marvell_init(void)
phy_register(_driver);
phy_register(_driver);
phy_register(_driver);
+   phy_register(_driver);
phy_register(_driver);
 
return 0;
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot