[PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Paul Kocialkowski]

Some PHY drivers like the generic one do not provide a read_status
callback on their own but rely on genphy_read_status being called
directly.

With the current code, this results in a NULL function pointer call.
Call genphy_read_status instead when there is no specific callback.

Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
Added Fixes tag and net label for resend.

 drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 74a8782313cf..bd6084e315de 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	u16 val = 0;
 	int err;
 
-	err = priv->phy_drv->read_status(phydev);
+	if (priv->phy_drv->read_status)
+		err = priv->phy_drv->read_status(phydev);
+	else
+		err = genphy_read_status(phydev);
 	if (err < 0)
 		return err;
 
-- 
2.20.1

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Andrew Lunn]

On Fri, Feb 15, 2019 at 05:32:20PM +0100, Paul Kocialkowski wrote:
> Some PHY drivers like the generic one do not provide a read_status
> callback on their own but rely on genphy_read_status being called
> directly.
> 
> With the current code, this results in a NULL function pointer call.
> Call genphy_read_status instead when there is no specific callback.
> 
> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Florian Fainelli]

On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
> Some PHY drivers like the generic one do not provide a read_status
> callback on their own but rely on genphy_read_status being called
> directly.
> 
> With the current code, this results in a NULL function pointer call.
> Call genphy_read_status instead when there is no specific callback.
> 
> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
> Added Fixes tag and net label for resend.

You would want to use phy_read_status() which encapsulates that check as
well as checks that the phy_drv pointer is not NULL.

> 
>  drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 74a8782313cf..bd6084e315de 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
>  	u16 val = 0;
>  	int err;
>  
> -	err = priv->phy_drv->read_status(phydev);
> +	if (priv->phy_drv->read_status)
> +		err = priv->phy_drv->read_status(phydev);
> +	else
> +		err = genphy_read_status(phydev);
>  	if (err < 0)
>  		return err;
>  
> 


-- 
Florian

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Paul Kocialkowski]

Hi,

On Fri, 2019-02-15 at 09:38 -0800, Florian Fainelli wrote:
> On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
> > Some PHY drivers like the generic one do not provide a read_status
> > callback on their own but rely on genphy_read_status being called
> > directly.
> > 
> > With the current code, this results in a NULL function pointer call.
> > Call genphy_read_status instead when there is no specific callback.
> > 
> > Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> > Added Fixes tag and net label for resend.
> 
> You would want to use phy_read_status() which encapsulates that check as
> well as checks that the phy_drv pointer is not NULL.

Well, this driver is a bit different and our priv->phy_drv != phydev-
>drv, so we can't use the helper directly. I should probably have
mentionned it in the commit message, sorry!

As I was mentionning to Andrew in the initial submission of this patch,
this driver is a bit unusual since it represents a GMII to RGMII
bridge, so it's not actually a PHY driver on its own -- it just sticks
itself in between the actual PHY and the MAC.

Cheers and thanks for the review,

Paul

> >  drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> > index 74a8782313cf..bd6084e315de 100644
> > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
> >  	u16 val = 0;
> >  	int err;
> >  
> > -	err = priv->phy_drv->read_status(phydev);
> > +	if (priv->phy_drv->read_status)
> > +		err = priv->phy_drv->read_status(phydev);
> > +	else
> > +		err = genphy_read_status(phydev);
> >  	if (err < 0)
> >  		return err;
> >  
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Florian Fainelli]

On 2/15/19 10:34 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri, 2019-02-15 at 09:38 -0800, Florian Fainelli wrote:
>> On 2/15/19 8:32 AM, Paul Kocialkowski wrote:
>>> Some PHY drivers like the generic one do not provide a read_status
>>> callback on their own but rely on genphy_read_status being called
>>> directly.
>>>
>>> With the current code, this results in a NULL function pointer call.
>>> Call genphy_read_status instead when there is no specific callback.
>>>
>>> Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
>>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>>> ---
>>> Added Fixes tag and net label for resend.
>>
>> You would want to use phy_read_status() which encapsulates that check as
>> well as checks that the phy_drv pointer is not NULL.
> 
> Well, this driver is a bit different and our priv->phy_drv != phydev-
>> drv, so we can't use the helper directly. I should probably have
> mentionned it in the commit message, sorry!
> 
> As I was mentionning to Andrew in the initial submission of this patch,
> this driver is a bit unusual since it represents a GMII to RGMII
> bridge, so it's not actually a PHY driver on its own -- it just sticks
> itself in between the actual PHY and the MAC.

Yes, my bad, you should still consider checking priv->phy_drv though, if
someone unbinds the PHY driver on either side, you are toast.

> 
> Cheers and thanks for the review,
> 
> Paul
> 
>>>  drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
>>> index 74a8782313cf..bd6084e315de 100644
>>> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
>>> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
>>> @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
>>>  	u16 val = 0;
>>>  	int err;
>>>  
>>> -	err = priv->phy_drv->read_status(phydev);
>>> +	if (priv->phy_drv->read_status)
>>> +		err = priv->phy_drv->read_status(phydev);
>>> +	else
>>> +		err = genphy_read_status(phydev);
>>>  	if (err < 0)
>>>  		return err;
>>>  
>>>
>>
>>


-- 
Florian

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Paul Kocialkowski]

Hi,

On Fri, 2019-02-15 at 10:53 -0800, Florian Fainelli wrote:
> On 2/15/19 10:34 AM, Paul Kocialkowski wrote:
> > As I was mentionning to Andrew in the initial submission of this patch,
> > this driver is a bit unusual since it represents a GMII to RGMII
> > bridge, so it's not actually a PHY driver on its own -- it just sticks
> > itself in between the actual PHY and the MAC.
> 
> Yes, my bad, you should still consider checking priv->phy_drv though, if
> someone unbinds the PHY driver on either side, you are toast.

Thanks for the suggestion! So I had a closer look at that driver to try
and see what could go wrong and it looks like I found a few things
there.

First, we have:
> priv->phy_dev->priv = priv;

Which basically overwrites that target PHY driver's priv with the
gmii2rgmii driver's. It looks like most PHY drivers don't use their
priv data so much so it kind of works in practice. But that's still a
receipe for disaster. I don't really see an immediate easy fix for that
one.

It might help to do things the other way round and bind the gmii2rgmii
PHY to the MAC, which itself would bind the actual PHY. That way we can
just have the gmii2rgmii redirect all ops to the actual PHY, except for
read_status. Maybe there was a reason I'm not seeing for doing things
the way they are done now though?

Then, it looks like there is no way for the gmii2rgmii driver to know
whether the PHY driver is still alive. It gets a pointer to the initial
priv->phy_dev->drv and then overwrites it. So when the target driver is
removed, the pointer will still be alive. Perhaps the memory backing it
will have been freed too.

How realistic does this scneario sound? I guess there are not many
cases where the PHY driver will be unregistered once it was picked up
by the gmii2rgmii driver, but I'm pretty new to the subsystem.

Cheers,

Paul

> > > >  drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > index 74a8782313cf..bd6084e315de 100644
> > > > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
> > > >  	u16 val = 0;
> > > >  	int err;
> > > >  
> > > > -	err = priv->phy_drv->read_status(phydev);
> > > > +	if (priv->phy_drv->read_status)
> > > > +		err = priv->phy_drv->read_status(phydev);
> > > > +	else
> > > > +		err = genphy_read_status(phydev);
> > > >  	if (err < 0)
> > > >  		return err;
> > > >  
> > > > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Andrew Lunn]

> Thanks for the suggestion! So I had a closer look at that driver to try
> and see what could go wrong and it looks like I found a few things
> there.

Hi Paul

Yes, this driver has issues. If i remember correctly, it got merged
while i was on vacation. I pointed out a few issues, but the authors
never responded. Feel free to fix it up.

      Andrew

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Michal Simek]

Hi,

On 19. 02. 19 18:25, Andrew Lunn wrote:
>> Thanks for the suggestion! So I had a closer look at that driver to try
>> and see what could go wrong and it looks like I found a few things
>> there.
> 
> Hi Paul
> 
> Yes, this driver has issues. If i remember correctly, it got merged
> while i was on vacation. I pointed out a few issues, but the authors
> never responded. Feel free to fix it up.

Will be good to know who was that person.

I can't do much this week with this because responsible person for this
driver is out of office this week. That's why please give us some time
to get back to this.

Thanks,
Michal

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Paul Kocialkowski]

Hi,

On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> Hi,
> 
> On 19. 02. 19 18:25, Andrew Lunn wrote:
> > > Thanks for the suggestion! So I had a closer look at that driver to try
> > > and see what could go wrong and it looks like I found a few things
> > > there.
> > 
> > Hi Paul
> > 
> > Yes, this driver has issues. If i remember correctly, it got merged
> > while i was on vacation. I pointed out a few issues, but the authors
> > never responded. Feel free to fix it up.
> 
> Will be good to know who was that person.
> 
> I can't do much this week with this because responsible person for this
> driver is out of office this week. That's why please give us some time
> to get back to this.

Understood. I think we need to start a discussion about how the general
design of this driver can be improved.

In particular, I wonder if it could work better to make this driver a
PHY driver that just redirects all its ops to the actual PHY driver,
except for read_status where it should also add some code.

Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
this way. Currently, the PHY mode has to be set to GMII for the MAC to
be configured correctly, but the PHY also gets this information while
it should be told that RGMII is in use. This doesn't seem to play a big
role in PHY configuration though, but it's still inadequate.

What do you think?

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Michal Simek]

On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>> Hi,
>>
>> On 19. 02. 19 18:25, Andrew Lunn wrote:
>>>> Thanks for the suggestion! So I had a closer look at that driver to try
>>>> and see what could go wrong and it looks like I found a few things
>>>> there.
>>>
>>> Hi Paul
>>>
>>> Yes, this driver has issues. If i remember correctly, it got merged
>>> while i was on vacation. I pointed out a few issues, but the authors
>>> never responded. Feel free to fix it up.
>>
>> Will be good to know who was that person.
>>
>> I can't do much this week with this because responsible person for this
>> driver is out of office this week. That's why please give us some time
>> to get back to this.
> 
> Understood. I think we need to start a discussion about how the general
> design of this driver can be improved.
> 
> In particular, I wonder if it could work better to make this driver a
> PHY driver that just redirects all its ops to the actual PHY driver,
> except for read_status where it should also add some code.

I didn't take a look at Linux driver but it should work in a way that it
checks description (more below) and then wait for attached phy to do its
work and on the way back just setup this bridge based on that.

> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> be configured correctly, but the PHY also gets this information while
> it should be told that RGMII is in use. This doesn't seem to play a big
> role in PHY configuration though, but it's still inadequate.
> 
> What do you think?

I stop the driver to be applied to u-boot because exactly this
gmii/rgmii configuration. There is a need that mac is configured to gmii
and this needs to be checked but to phy rgmii needs to be exposed.
I was trying to find out hardware design and board where I can do some
experiments but didn't find out. That's why I need to wait for
colleagues to point me to that.

Thanks,
Michal

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Michal Simek]

On 21. 02. 19 12:03, Michal Simek wrote:
> On 21. 02. 19 11:24, Paul Kocialkowski wrote:
>> Hi,
>>
>> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>>> Hi,
>>>
>>> On 19. 02. 19 18:25, Andrew Lunn wrote:
>>>>> Thanks for the suggestion! So I had a closer look at that driver to try
>>>>> and see what could go wrong and it looks like I found a few things
>>>>> there.
>>>>
>>>> Hi Paul
>>>>
>>>> Yes, this driver has issues. If i remember correctly, it got merged
>>>> while i was on vacation. I pointed out a few issues, but the authors
>>>> never responded. Feel free to fix it up.
>>>
>>> Will be good to know who was that person.
>>>
>>> I can't do much this week with this because responsible person for this
>>> driver is out of office this week. That's why please give us some time
>>> to get back to this.
>>
>> Understood. I think we need to start a discussion about how the general
>> design of this driver can be improved.
>>
>> In particular, I wonder if it could work better to make this driver a
>> PHY driver that just redirects all its ops to the actual PHY driver,
>> except for read_status where it should also add some code.
> 
> I didn't take a look at Linux driver but it should work in a way that it
> checks description (more below) and then wait for attached phy to do its
> work and on the way back just setup this bridge based on that.
> 
>> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
>> this way. Currently, the PHY mode has to be set to GMII for the MAC to
>> be configured correctly, but the PHY also gets this information while
>> it should be told that RGMII is in use. This doesn't seem to play a big
>> role in PHY configuration though, but it's still inadequate.
>>
>> What do you think?
> 
> I stop the driver to be applied to u-boot because exactly this
> gmii/rgmii configuration. There is a need that mac is configured to gmii
> and this needs to be checked but to phy rgmii needs to be exposed.
> I was trying to find out hardware design and board where I can do some
> experiments but didn't find out. That's why I need to wait for
> colleagues to point me to that.

Harini: Can you please respond this thread?

Thanks,
Michal

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Harini Katakam]

Hi Andrew, Paul,

On Wed, Feb 27, 2019 at 2:15 PM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 21. 02. 19 12:03, Michal Simek wrote:
> > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> >> Hi,
> >>
> >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> >>> Hi,
> >>>
> >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> >>>>> Thanks for the suggestion! So I had a closer look at that driver to try
> >>>>> and see what could go wrong and it looks like I found a few things
> >>>>> there.
> >>>>
> >>>> Hi Paul
> >>>>
> >>>> Yes, this driver has issues. If i remember correctly, it got merged
> >>>> while i was on vacation. I pointed out a few issues, but the authors
> >>>> never responded. Feel free to fix it up.
> >>>

Sorry for this - I've synced up with the author and got the comments from the
time this driver was upstreamed. I'll try to address those and Paul's
suggestions
going forward.

> >>> Will be good to know who was that person.
> >>>
> >>> I can't do much this week with this because responsible person for this
> >>> driver is out of office this week. That's why please give us some time
> >>> to get back to this.
> >>
> >> Understood. I think we need to start a discussion about how the general
> >> design of this driver can be improved.
> >>
> >> In particular, I wonder if it could work better to make this driver a
> >> PHY driver that just redirects all its ops to the actual PHY driver,
> >> except for read_status where it should also add some code.

Thanks, I'm looking into this option and also a way to expose the correct
interface mode setting as you mentioned below. I'll get back before the
end of the week. Please do let me know if you have any further suggestions.

Regards,
Harini

> >
> > I didn't take a look at Linux driver but it should work in a way that it
> > checks description (more below) and then wait for attached phy to do its
> > work and on the way back just setup this bridge based on that.
> >
> >> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> >> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> >> be configured correctly, but the PHY also gets this information while
> >> it should be told that RGMII is in use. This doesn't seem to play a big
> >> role in PHY configuration though, but it's still inadequate.
> >>
> >> What do you think?
<snip>

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Harini Katakam]

Hi,
On Wed, Feb 27, 2019 at 2:35 PM Harini Katakam <harinik@xilinx.com> wrote:
>
> Hi Andrew, Paul,
>
> On Wed, Feb 27, 2019 at 2:15 PM Michal Simek <michal.simek@xilinx.com> wrote:
> >
> > On 21. 02. 19 12:03, Michal Simek wrote:
> > > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> > >> Hi,
> > >>
> > >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> > >>> Hi,
> > >>>
> > >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
<snip>
> > >> Understood. I think we need to start a discussion about how the general
> > >> design of this driver can be improved.
> > >>
> > >> In particular, I wonder if it could work better to make this driver a
> > >> PHY driver that just redirects all its ops to the actual PHY driver,
> > >> except for read_status where it should also add some code.
>
> Thanks, I'm looking into this option and also a way to expose the correct
> interface mode setting as you mentioned below. I'll get back before the
> end of the week. Please do let me know if you have any further suggestions.
>
This IP does not have a PHY identifier or status register that can be accessed
from the phy framework. We could discuss with our design team to add these
in the future. But that would take sometime and this version should be still be
supported. Also, if this IP has a PHY driver, then two phy drivers would have
to be probed which are connected in a serial manner and I believe I'll have to
update the framework to support that. Could you please let me know if you have
any inputs on this?
OR since this is just a bridge IP, is it acceptable to address the error cases?
-> Module loading/unloading
-> Spinlocks for protection
-> Correct phy mode information to the driver.
-> Any other concerns
I could do a respin of this patch after addressing Andrew's comments:
https://patchwork.kernel.org/patch/9290231/

Regards,
Harini

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Harini Katakam]

Hi Andrew,
On Thu, Feb 28, 2019 at 1:03 PM Harini Katakam <harinik@xilinx.com> wrote:
>
> Hi,
> On Wed, Feb 27, 2019 at 2:35 PM Harini Katakam <harinik@xilinx.com> wrote:
> >
> > Hi Andrew, Paul,
> >
> > On Wed, Feb 27, 2019 at 2:15 PM Michal Simek <michal.simek@xilinx.com> wrote:
> > >
> > > On 21. 02. 19 12:03, Michal Simek wrote:
> > > > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> > > >> Hi,
> > > >>
> > > >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> > > >>> Hi,
> > > >>>
> > > >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> <snip>
> > > >> Understood. I think we need to start a discussion about how the general
> > > >> design of this driver can be improved.
> > > >>
> > > >> In particular, I wonder if it could work better to make this driver a
> > > >> PHY driver that just redirects all its ops to the actual PHY driver,
> > > >> except for read_status where it should also add some code.
> >
> > Thanks, I'm looking into this option and also a way to expose the correct
> > interface mode setting as you mentioned below. I'll get back before the
> > end of the week. Please do let me know if you have any further suggestions.
> >
> This IP does not have a PHY identifier or status register that can be accessed
> from the phy framework. We could discuss with our design team to add these
> in the future. But that would take sometime and this version should be still be
> supported. Also, if this IP has a PHY driver, then two phy drivers would have
> to be probed which are connected in a serial manner and I believe I'll have to
> update the framework to support that. Could you please let me know if you have
> any inputs on this?
> OR since this is just a bridge IP, is it acceptable to address the error cases?
> -> Module loading/unloading
> -> Spinlocks for protection
> -> Correct phy mode information to the driver.
> -> Any other concerns
> I could do a respin of this patch after addressing Andrew's comments:
> https://patchwork.kernel.org/patch/9290231/

Related to this, I have a query on how the DT node for gmii2rgmii should look.
One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
this piece of code to register this mdiobus:
+ mdio_np = of_get_child_by_name(np, "mdio");
+ if (mdio_np) {
+ of_node_put(mdio_np);
+ err = of_mdiobus_register(bp->mii_bus, mdio_np);
+ if (err)
+ goto err_out_unregister_bus;

And the DT node looks like this:
ethernet {
    phy-mode = "gmii";
    phy-handle = <&extphy>;

    mdio {
        extphy {
            reg = <x>;
        };
        gmii_to_rgmii{
            compatible = "xlnx,gmii-to-rgmii-1.0";
            phy-handle = <&extphy>;
            reg = <x>;
        };
    };
};

Could you please clarify if phy-handle in ethernet should point to
external PHY or gmii2rgmii?

Regards,
Harini

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Andrew Lunn]

> Related to this, I have a query on how the DT node for gmii2rgmii should look.
> One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
> this piece of code to register this mdiobus:
> + mdio_np = of_get_child_by_name(np, "mdio");
> + if (mdio_np) {
> + of_node_put(mdio_np);
> + err = of_mdiobus_register(bp->mii_bus, mdio_np);
> + if (err)
> + goto err_out_unregister_bus;
> 
> And the DT node looks like this:
> ethernet {
>     phy-mode = "gmii";
>     phy-handle = <&extphy>;
> 
>     mdio {
>         extphy {
>             reg = <x>;
>         };
>         gmii_to_rgmii{
>             compatible = "xlnx,gmii-to-rgmii-1.0";
>             phy-handle = <&extphy>;
>             reg = <x>;
>         };
>     };
> };

Hi Harini

You have this setup:
  
MAC <==> GMII2RGMII <==> RGMII_PHY

So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.

Feel free to submit a patch extending
Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt to include
a MAC node, etc.

   Andrew

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Harini Katakam]

Hi Andrew,
On Sat, Mar 9, 2019 at 9:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Related to this, I have a query on how the DT node for gmii2rgmii should look.
> > One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
> > this piece of code to register this mdiobus:
> > + mdio_np = of_get_child_by_name(np, "mdio");
> > + if (mdio_np) {
> > + of_node_put(mdio_np);
> > + err = of_mdiobus_register(bp->mii_bus, mdio_np);
> > + if (err)
> > + goto err_out_unregister_bus;
> >
> > And the DT node looks like this:
> > ethernet {
> >     phy-mode = "gmii";
> >     phy-handle = <&extphy>;
> >
> >     mdio {
> >         extphy {
> >             reg = <x>;
> >         };
> >         gmii_to_rgmii{
> >             compatible = "xlnx,gmii-to-rgmii-1.0";
> >             phy-handle = <&extphy>;
> >             reg = <x>;
> >         };
> >     };
> > };
>
> Hi Harini
>
> You have this setup:
>
> MAC <==> GMII2RGMII <==> RGMII_PHY
>
> So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.
>
> Feel free to submit a patch extending
> Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt to include
> a MAC node, etc.

Thank you, will do the same.

Regards,
Harini

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Harini Katakam]

Hi Andrew,
On Mon, Mar 11, 2019 at 11:34 AM Harini Katakam <harinik@xilinx.com> wrote:
>
> Hi Andrew,
> On Sat, Mar 9, 2019 at 9:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Related to this, I have a query on how the DT node for gmii2rgmii should look.
> > > One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
> > > this piece of code to register this mdiobus:
> > > + mdio_np = of_get_child_by_name(np, "mdio");
> > > + if (mdio_np) {
> > > + of_node_put(mdio_np);
> > > + err = of_mdiobus_register(bp->mii_bus, mdio_np);
> > > + if (err)
> > > + goto err_out_unregister_bus;
> > >
> > > And the DT node looks like this:
> > > ethernet {
> > >     phy-mode = "gmii";
> > >     phy-handle = <&extphy>;
> > >
> > >     mdio {
> > >         extphy {
> > >             reg = <x>;
> > >         };
> > >         gmii_to_rgmii{
> > >             compatible = "xlnx,gmii-to-rgmii-1.0";
> > >             phy-handle = <&extphy>;
> > >             reg = <x>;
> > >         };
> > >     };
> > > };
> >
> > Hi Harini
> >
> > You have this setup:
> >
> > MAC <==> GMII2RGMII <==> RGMII_PHY
> >
> > So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.
> >
> > Feel free to submit a patch extending
> > Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt to include
> > a MAC node, etc.
>
> Thank you, will do the same.

Thanks again for your input. So, I did some testing with this change.
But the issue is that, if I point the phy-handle to gmi2rgmii,
of_phy_connect will be called from the MAC and it will fail because gmii2rgmii
is not a PHY driver and it does not have a standard PHY register set or ID.
Which goes back to the discussion above whether this needs to changed in the IP.

But right now, it is a bridge device on the MDIO bus and has no PHY
functionality.
Moreover, any MAC is capable of accessing the external PHY with no interference
in the MDIO path (the gmii2rgmii bridge just acts like another device
on a common bus).

What Michal suggested below in uboot is that they register gmii2rgmii
with a dummy
PHY ID and then attach the external phy driver in its probe. I'm not
sure if this will work
in linux i.e. calling phy_connect_direct inside the gmii2rgmii probe.

Regards,
Harini

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Michal Simek]

On 11. 03. 19 13:27, Harini Katakam wrote:
> Hi Andrew,
> On Mon, Mar 11, 2019 at 11:34 AM Harini Katakam <harinik@xilinx.com> wrote:
>>
>> Hi Andrew,
>> On Sat, Mar 9, 2019 at 9:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>>
>>>> Related to this, I have a query on how the DT node for gmii2rgmii should look.
>>>> One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
>>>> this piece of code to register this mdiobus:
>>>> + mdio_np = of_get_child_by_name(np, "mdio");
>>>> + if (mdio_np) {
>>>> + of_node_put(mdio_np);
>>>> + err = of_mdiobus_register(bp->mii_bus, mdio_np);
>>>> + if (err)
>>>> + goto err_out_unregister_bus;
>>>>
>>>> And the DT node looks like this:
>>>> ethernet {
>>>>     phy-mode = "gmii";
>>>>     phy-handle = <&extphy>;
>>>>
>>>>     mdio {
>>>>         extphy {
>>>>             reg = <x>;
>>>>         };
>>>>         gmii_to_rgmii{
>>>>             compatible = "xlnx,gmii-to-rgmii-1.0";
>>>>             phy-handle = <&extphy>;
>>>>             reg = <x>;
>>>>         };
>>>>     };
>>>> };
>>>
>>> Hi Harini
>>>
>>> You have this setup:
>>>
>>> MAC <==> GMII2RGMII <==> RGMII_PHY
>>>
>>> So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.
>>>
>>> Feel free to submit a patch extending
>>> Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt to include
>>> a MAC node, etc.
>>
>> Thank you, will do the same.
> 
> Thanks again for your input. So, I did some testing with this change.
> But the issue is that, if I point the phy-handle to gmi2rgmii,
> of_phy_connect will be called from the MAC and it will fail because gmii2rgmii
> is not a PHY driver and it does not have a standard PHY register set or ID.
> Which goes back to the discussion above whether this needs to changed in the IP.
> 
> But right now, it is a bridge device on the MDIO bus and has no PHY
> functionality.
> Moreover, any MAC is capable of accessing the external PHY with no interference
> in the MDIO path (the gmii2rgmii bridge just acts like another device
> on a common bus).
> 
> What Michal suggested below in uboot is that they register gmii2rgmii
> with a dummy
> PHY ID and then attach the external phy driver in its probe. I'm not
> sure if this will work
> in linux i.e. calling phy_connect_direct inside the gmii2rgmii probe.

In u-boot behavior and wiring is similar to fixed-link phy.

Thanks,
Michal

Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Michal Simek]

Hi,

On 09. 03. 19 17:19, Andrew Lunn wrote:
>> Related to this, I have a query on how the DT node for gmii2rgmii should look.
>> One of the users of gmii2rgmii is Cadence macb driver. In Xilinx tree, we use
>> this piece of code to register this mdiobus:
>> + mdio_np = of_get_child_by_name(np, "mdio");
>> + if (mdio_np) {
>> + of_node_put(mdio_np);
>> + err = of_mdiobus_register(bp->mii_bus, mdio_np);
>> + if (err)
>> + goto err_out_unregister_bus;
>>
>> And the DT node looks like this:
>> ethernet {
>>     phy-mode = "gmii";
>>     phy-handle = <&extphy>;
>>
>>     mdio {
>>         extphy {
>>             reg = <x>;
>>         };
>>         gmii_to_rgmii{
>>             compatible = "xlnx,gmii-to-rgmii-1.0";
>>             phy-handle = <&extphy>;
>>             reg = <x>;
>>         };
>>     };
>> };
> 
> Hi Harini
> 
> You have this setup:
>   
> MAC <==> GMII2RGMII <==> RGMII_PHY
> 
> So you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'.

That means that MAC should talk to GMII2RGMII bridge as normall talks to
RGMII_PHY and then GMII2RGMII bridge should talk to RGMII_PHY.

Anyway good to read it because that's exactly how we have done it in u-boot.

Thanks,
Michal