[RFC,net-next,v4,01/28] net: mdio: ipq8064: clean whitespaces in define

Message ID 20210508002920.19945-1-ansuelsmth@gmail.com
State Superseded
Headers show
Series
  • Multiple improvement to qca8k stability
Related show

Commit Message

Ansuel Smith May 8, 2021, 12:28 a.m.
Fix mixed whitespace and tab for define spacing.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/mdio/mdio-ipq8064.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Ansuel Smith May 8, 2021, 11:30 a.m. | #1
On Sat, May 08, 2021 at 12:35:35PM +0800, DENG Qingfang wrote:
> On Sat, May 08, 2021 at 02:29:18AM +0200, Ansuel Smith wrote:
> > Add initial support for qca8k internal PHYs. The internal PHYs requires
> > special mmd and debug values to be set based on the switch revision
> > passwd using the dev_flags. Supports output of idle, receive and eee_wake
> > errors stats.
> > Some debug values sets can't be translated as the documentation lacks any
> > reference about them.
> 
> I think this can be merged into at803x.c, as they have almost the same
> registers, and some features such as interrupt handler and cable test
> can be reused.
>

Wouldn't this be a little bit confusing? But actually yes... interrupt
handler and cable test have the same regs. My main concern is about the
phy_dev flags and the dbg regs that I think are different and would
create some confusion. If this It's not a proble, sure I can rework this
a put in the at803x.c phy driver.

> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
Florian Fainelli May 8, 2021, 3:45 p.m. | #2
On 5/7/2021 5:28 PM, Ansuel Smith wrote:
> Fix mixed whitespace and tab for define spacing.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli May 8, 2021, 3:53 p.m. | #3
On 5/7/2021 5:28 PM, Ansuel Smith wrote:
> With the use of the qca8k dsa driver, some problem arised related to
> port status detection. With a load on a specific port (for example a
> simple speed test), the driver starts to behave in a strange way and
> garbage data is produced. To address this, enlarge the sleep delay and
> address a bug for the reg offset 31 that require additional delay for
> this specific reg.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

I am still curious whether the problem is that you have lots of traffic
going through the same bus fabric (AXI?) and that eventually puts the
register accesses to a lower priority to get through. We would most
likely need someone from QCA to tell if this is even remotely a
possibility and this is unlikely to happen.
Ansuel Smith May 8, 2021, 6:05 p.m. | #4
On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:
> On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:
> > Fix mixed whitespace and tab for define spacing.
> 
> Please add a patch [0/28] which describes the big picture of what
> these changes are doing.
> 
> Also, this series is getting big. You might want to split it into two,
> One containing the cleanup, and the second adding support for the new
> switch.
> 
> 	Andrew

There is a 0/28. With all the changes. Could be that I messed the cc?
I agree think it's better to split this for the mdio part, the cleanup
and the changes needed for the internal phy.

Can I keep the review tag?
Jonathan McDowell May 15, 2021, 5 p.m. | #5
On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:
> On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > Fix mixed whitespace and tab for define spacing.

> > 

> > Please add a patch [0/28] which describes the big picture of what

> > these changes are doing.

> > 

> > Also, this series is getting big. You might want to split it into two,

> > One containing the cleanup, and the second adding support for the new

> > switch.

> > 

> > 	Andrew

> 

> There is a 0/28. With all the changes. Could be that I messed the cc?

> I agree think it's better to split this for the mdio part, the cleanup

> and the changes needed for the internal phy.


FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011
today. I currently use the GPIO MDIO driver because I saw issues with
the IPQ8064 driver in the past, and sticking with the GPIO driver I see
both QCA8337 devices and traffic flows as expected, so no obvious
regressions from your changes.

I also tried switching to the IPQ8064 MDIO driver for my first device
(which is on the MDIO0 bus), but it's still not happy:

qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

J.

-- 
One of the nice things about standards is that there are so many of
them.
Florian Fainelli May 15, 2021, 5:03 p.m. | #6
On 5/15/2021 10:00 AM, Jonathan McDowell wrote:
> On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

>> On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

>>> On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

>>>> Fix mixed whitespace and tab for define spacing.

>>>

>>> Please add a patch [0/28] which describes the big picture of what

>>> these changes are doing.

>>>

>>> Also, this series is getting big. You might want to split it into two,

>>> One containing the cleanup, and the second adding support for the new

>>> switch.

>>>

>>> 	Andrew

>>

>> There is a 0/28. With all the changes. Could be that I messed the cc?

>> I agree think it's better to split this for the mdio part, the cleanup

>> and the changes needed for the internal phy.

> 

> FWIW I didn't see the 0/28 mail either.I tried these out on my RB3011

> today. I currently use the GPIO MDIO driver because I saw issues with

> the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> both QCA8337 devices and traffic flows as expected, so no obvious

> regressions from your changes.


The cover letter somehow appeared as the final patch in the submission
instead of having all patches in-reply-to it as one would expect.

Russell had some additional feedback that came in during or after the
patches being applied so it would be nice to address that.

> 

> I also tried switching to the IPQ8064 MDIO driver for my first device

> (which is on the MDIO0 bus), but it's still not happy:

> 

> qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13


If you do repeated reads of the revision register to you eventually get
13 as intended?
-- 
Florian
Ansuel Smith May 15, 2021, 5:30 p.m. | #7
On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:
> On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

> > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > > Fix mixed whitespace and tab for define spacing.

> > > 

> > > Please add a patch [0/28] which describes the big picture of what

> > > these changes are doing.

> > > 

> > > Also, this series is getting big. You might want to split it into two,

> > > One containing the cleanup, and the second adding support for the new

> > > switch.

> > > 

> > > 	Andrew

> > 

> > There is a 0/28. With all the changes. Could be that I messed the cc?

> > I agree think it's better to split this for the mdio part, the cleanup

> > and the changes needed for the internal phy.

> 

> FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011

> today. I currently use the GPIO MDIO driver because I saw issues with

> the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> both QCA8337 devices and traffic flows as expected, so no obvious

> regressions from your changes.

> 

> I also tried switching to the IPQ8064 MDIO driver for my first device

> (which is on the MDIO0 bus), but it's still not happy:

> 

> qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

> 

> J.

> 

> -- 

> One of the nice things about standards is that there are so many of

> them.


Can you try the v6 version of this series? Anyway the fact that 0 is
produced instead of a wrong value let me think that there is some
problem with the mdio read. (on other device there was a problem of
wrong id read but nerver 0). Could be that the bootloader on your board
set the mdio MASTER disabled. I experienced this kind of problem when
switching from the dsa driver and the legacy swconfig driver. On remove
of the dsa driver, the swconfig didn't work as the bit was never cleared
by the dsa driver and resulted in your case. (id 0 read from the mdio)

Do you want to try a quick patch so we can check if this is the case?
(about the cover letter... sorry will check why i'm pushing this wrong)
Jonathan McDowell May 15, 2021, 6:08 p.m. | #8
On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:

> > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

> > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > > > Fix mixed whitespace and tab for define spacing.

> > > > 

> > > > Please add a patch [0/28] which describes the big picture of what

> > > > these changes are doing.

> > > > 

> > > > Also, this series is getting big. You might want to split it into two,

> > > > One containing the cleanup, and the second adding support for the new

> > > > switch.

> > > > 

> > > > 	Andrew

> > > 

> > > There is a 0/28. With all the changes. Could be that I messed the cc?

> > > I agree think it's better to split this for the mdio part, the cleanup

> > > and the changes needed for the internal phy.

> > 

> > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011

> > today. I currently use the GPIO MDIO driver because I saw issues with

> > the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> > both QCA8337 devices and traffic flows as expected, so no obvious

> > regressions from your changes.

> > 

> > I also tried switching to the IPQ8064 MDIO driver for my first device

> > (which is on the MDIO0 bus), but it's still not happy:

> > 

> > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

> > 

> 

> Can you try the v6 version of this series?


Both the v6 qca8k series and the separate ipq806x mdio series, yes?

> Anyway the fact that 0 is produced instead of a wrong value let me

> think that there is some problem with the mdio read. (on other device

> there was a problem of wrong id read but nerver 0). Could be that the

> bootloader on your board set the mdio MASTER disabled. I experienced

> this kind of problem when switching from the dsa driver and the legacy

> swconfig driver. On remove of the dsa driver, the swconfig didn't work

> as the bit was never cleared by the dsa driver and resulted in your

> case. (id 0 read from the mdio)

> 

> Do you want to try a quick patch so we can check if this is the case?

> (about the cover letter... sorry will check why i'm pushing this

> wrong)


There's definitely something odd going on here. I went back to mainline
to see what the situation is there. With the GPIO MDIO driver both
switches work (expected, as this is what I run with). I changed switch0
over to use the IPQ MDIO driver and it wasn't detected (but switch1
still on the GPIO MDIO driver was fine).

I then tried putting both switches onto the IPQ MDIO driver and in that
instance switch0 came up fine, while switch1 wasn't detected.

If there's a simple patch that might give more debug I can try it out.

J.

-- 
   "Reality Or Nothing!" -- Cold   |  .''`.  Debian GNU/Linux Developer
              Lazarus              | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.
Ansuel Smith May 15, 2021, 6:20 p.m. | #9
On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:

> > On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:

> > > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

> > > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > > > > Fix mixed whitespace and tab for define spacing.

> > > > > 

> > > > > Please add a patch [0/28] which describes the big picture of what

> > > > > these changes are doing.

> > > > > 

> > > > > Also, this series is getting big. You might want to split it into two,

> > > > > One containing the cleanup, and the second adding support for the new

> > > > > switch.

> > > > > 

> > > > > 	Andrew

> > > > 

> > > > There is a 0/28. With all the changes. Could be that I messed the cc?

> > > > I agree think it's better to split this for the mdio part, the cleanup

> > > > and the changes needed for the internal phy.

> > > 

> > > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011

> > > today. I currently use the GPIO MDIO driver because I saw issues with

> > > the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> > > both QCA8337 devices and traffic flows as expected, so no obvious

> > > regressions from your changes.

> > > 

> > > I also tried switching to the IPQ8064 MDIO driver for my first device

> > > (which is on the MDIO0 bus), but it's still not happy:

> > > 

> > > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

> > > 

> > 

> > Can you try the v6 version of this series?

> 

> Both the v6 qca8k series and the separate ipq806x mdio series, yes?

> 

> > Anyway the fact that 0 is produced instead of a wrong value let me

> > think that there is some problem with the mdio read. (on other device

> > there was a problem of wrong id read but nerver 0). Could be that the

> > bootloader on your board set the mdio MASTER disabled. I experienced

> > this kind of problem when switching from the dsa driver and the legacy

> > swconfig driver. On remove of the dsa driver, the swconfig didn't work

> > as the bit was never cleared by the dsa driver and resulted in your

> > case. (id 0 read from the mdio)

> > 

> > Do you want to try a quick patch so we can check if this is the case?

> > (about the cover letter... sorry will check why i'm pushing this

> > wrong)

> 

> There's definitely something odd going on here. I went back to mainline

> to see what the situation is there. With the GPIO MDIO driver both

> switches work (expected, as this is what I run with). I changed switch0

> over to use the IPQ MDIO driver and it wasn't detected (but switch1

> still on the GPIO MDIO driver was fine).

> 

> I then tried putting both switches onto the IPQ MDIO driver and in that

> instance switch0 came up fine, while switch1 wasn't detected.

> 


Oh wait, your board have 2 different switch? So they both use the master
bit when used... Mhhh I need to think about this if there is a clean way
to handle this. The idea would be that one of the 2 dsa switch should
use the already defined mdio bus.

The problem here is that to use the internal mdio bus, a bit must be
set or 0 is read on every value (as the bit actually disable the internal
mdio). This is good if one dsa driver is used but when 2 or more are
used I think this clash and only one of them work. The gpio mdio path is
not affected by this. Will check if I can find some way to address this.

> If there's a simple patch that might give more debug I can try it out.

> 

> J.

>

> -- 

>    "Reality Or Nothing!" -- Cold   |  .''`.  Debian GNU/Linux Developer

>               Lazarus              | : :' :  Happy to accept PGP signed

>                                    | `. `'   or encrypted mail - RSA

> |   `-    key on the keyservers.
Jonathan McDowell May 15, 2021, 7:40 p.m. | #10
On Sat, May 15, 2021 at 08:20:40PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:

> > On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:

> > > Do you want to try a quick patch so we can check if this is the case?

> > > (about the cover letter... sorry will check why i'm pushing this

> > > wrong)

> > 

> > There's definitely something odd going on here. I went back to mainline

> > to see what the situation is there. With the GPIO MDIO driver both

> > switches work (expected, as this is what I run with). I changed switch0

> > over to use the IPQ MDIO driver and it wasn't detected (but switch1

> > still on the GPIO MDIO driver was fine).

> > 

> > I then tried putting both switches onto the IPQ MDIO driver and in that

> > instance switch0 came up fine, while switch1 wasn't detected.

> > 

> 

> Oh wait, your board have 2 different switch? So they both use the master

> bit when used... Mhhh I need to think about this if there is a clean way

> to handle this. The idea would be that one of the 2 dsa switch should

> use the already defined mdio bus.

> 

> The problem here is that to use the internal mdio bus, a bit must be

> set or 0 is read on every value (as the bit actually disable the internal

> mdio). This is good if one dsa driver is used but when 2 or more are

> used I think this clash and only one of them work. The gpio mdio path is

> not affected by this. Will check if I can find some way to address this.


They're on 2 separate sets of GPIOs if that makes a difference - switch0
is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic
shared between these? Also even if that's the case it seems odd that
enabling the MDIO for just switch0 doesn't work?

J.

-- 
101 things you can't have too much of : 3 - Sleep.
Ansuel Smith May 15, 2021, 7:47 p.m. | #11
On Sat, May 15, 2021 at 08:40:47PM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 08:20:40PM +0200, Ansuel Smith wrote:

> > On Sat, May 15, 2021 at 07:08:57PM +0100, Jonathan McDowell wrote:

> > > On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:

> > > > Do you want to try a quick patch so we can check if this is the case?

> > > > (about the cover letter... sorry will check why i'm pushing this

> > > > wrong)

> > > 

> > > There's definitely something odd going on here. I went back to mainline

> > > to see what the situation is there. With the GPIO MDIO driver both

> > > switches work (expected, as this is what I run with). I changed switch0

> > > over to use the IPQ MDIO driver and it wasn't detected (but switch1

> > > still on the GPIO MDIO driver was fine).

> > > 

> > > I then tried putting both switches onto the IPQ MDIO driver and in that

> > > instance switch0 came up fine, while switch1 wasn't detected.

> > > 

> > 

> > Oh wait, your board have 2 different switch? So they both use the master

> > bit when used... Mhhh I need to think about this if there is a clean way

> > to handle this. The idea would be that one of the 2 dsa switch should

> > use the already defined mdio bus.

> > 

> > The problem here is that to use the internal mdio bus, a bit must be

> > set or 0 is read on every value (as the bit actually disable the internal

> > mdio). This is good if one dsa driver is used but when 2 or more are

> > used I think this clash and only one of them work. The gpio mdio path is

> > not affected by this. Will check if I can find some way to address this.

> 

> They're on 2 separate sets of GPIOs if that makes a difference - switch0

> is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic

> shared between these? Also even if that's the case it seems odd that

> enabling the MDIO for just switch0 doesn't work?

> 


The dedicated internal mdio on ipq8064 is unique and present on the
gmac0 address so yes it's shared between them. And this seems to be the
problem... As you notice the fact that different gpio are used for the
different switch fix the problem. So think that to use the dedicated
mdio bus with both switch we need to introduce some type of
syncronization or something like that.
Andrew Lunn May 15, 2021, 11:52 p.m. | #12
> > They're on 2 separate sets of GPIOs if that makes a difference - switch0

> > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic

> > shared between these? Also even if that's the case it seems odd that

> > enabling the MDIO for just switch0 doesn't work?

> > 

> 

> The dedicated internal mdio on ipq8064 is unique and present on the

> gmac0 address so yes it's shared between them. And this seems to be the

> problem... As you notice the fact that different gpio are used for the

> different switch fix the problem. So think that to use the dedicated

> mdio bus with both switch we need to introduce some type of

> syncronization or something like that.


Please could you describe the hardware in a bit more details. Or point
me at a datasheet. It sounds like you have an MDIO mux? Linux has this
concept, so you might need to implement a mux driver.

	 Andrew
Ansuel Smith May 16, 2021, 12:23 a.m. | #13
On Sun, May 16, 2021 at 01:52:05AM +0200, Andrew Lunn wrote:
> > > They're on 2 separate sets of GPIOs if that makes a difference - switch0

> > > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic

> > > shared between these? Also even if that's the case it seems odd that

> > > enabling the MDIO for just switch0 doesn't work?

> > > 

> > 

> > The dedicated internal mdio on ipq8064 is unique and present on the

> > gmac0 address so yes it's shared between them. And this seems to be the

> > problem... As you notice the fact that different gpio are used for the

> > different switch fix the problem. So think that to use the dedicated

> > mdio bus with both switch we need to introduce some type of

> > syncronization or something like that.

> 

> Please could you describe the hardware in a bit more details. Or point

> me at a datasheet. It sounds like you have an MDIO mux? Linux has this

> concept, so you might need to implement a mux driver.

> 

> 	 Andrew


Datasheet of ipq8064 are hard to find and pricey.
Will try hoping I don't write something very wrong.
Anyway on the SoC there are 4 gmac (most of the time 2 are used
and represent the 2 cpu port) and one mdio bus present on the gmac0
address. 
Normally on these SoC they add a qca8337 switch and it's common to use
2 gmac port as cpu port. The switch can be interfaced using UART or MDIO.
Normally the uart interface is used, but it's slower than using the mdio
dedicated interface. Only mdio or uart can be used as the switch use the
same pins.
So I think the problem here is that only one switch can use the mdio bus
exposed by gmac0 and any other must use a gpio slow path.
Anyway about the use of the MASTER path and all this mess, the 
qca8k: extend slave-bus implementations series contains lots of info
about this[1].

[1] http://patchwork.ozlabs.org/project/netdev/patch/20190319195419.12746-3-chunkeey@gmail.com/
Jonathan McDowell May 16, 2021, 9:37 a.m. | #14
On Sun, May 16, 2021 at 02:23:18AM +0200, Ansuel Smith wrote:
> On Sun, May 16, 2021 at 01:52:05AM +0200, Andrew Lunn wrote:

> > > > They're on 2 separate sets of GPIOs if that makes a difference - switch0

> > > > is in gpio0/1 and switch1 is on gpio10/11. Is the internal MDIO logic

> > > > shared between these? Also even if that's the case it seems odd that

> > > > enabling the MDIO for just switch0 doesn't work?

> > > > 

> > > 

> > > The dedicated internal mdio on ipq8064 is unique and present on the

> > > gmac0 address so yes it's shared between them. And this seems to be the

> > > problem... As you notice the fact that different gpio are used for the

> > > different switch fix the problem. So think that to use the dedicated

> > > mdio bus with both switch we need to introduce some type of

> > > syncronization or something like that.

> > 

> > Please could you describe the hardware in a bit more details. Or point

> > me at a datasheet. It sounds like you have an MDIO mux? Linux has this

> > concept, so you might need to implement a mux driver.

> > 

> > 	 Andrew

> 

> Datasheet of ipq8064 are hard to find and pricey.

> Will try hoping I don't write something very wrong.

> Anyway on the SoC there are 4 gmac (most of the time 2 are used

> and represent the 2 cpu port) and one mdio bus present on the gmac0

> address. 


There's a suggestion of an additional mdio bus on the gmac1 address at:

https://github.com/adron-s/openwrt-rb3011/commit/dd63b3ef563fa77fd2fb7d6ca12ca9411cd18740

is that not accurate?

J.

-- 
   Funny how life imitates LSD.    |  .''`.  Debian GNU/Linux Developer
                                   | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.
Jonathan McDowell May 16, 2021, 9:49 a.m. | #15
On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:
> On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:

> > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

> > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > > > Fix mixed whitespace and tab for define spacing.

> > > > 

> > > > Please add a patch [0/28] which describes the big picture of what

> > > > these changes are doing.

> > > > 

> > > > Also, this series is getting big. You might want to split it into two,

> > > > One containing the cleanup, and the second adding support for the new

> > > > switch.

> > > > 

> > > > 	Andrew

> > > 

> > > There is a 0/28. With all the changes. Could be that I messed the cc?

> > > I agree think it's better to split this for the mdio part, the cleanup

> > > and the changes needed for the internal phy.

> > 

> > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011

> > today. I currently use the GPIO MDIO driver because I saw issues with

> > the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> > both QCA8337 devices and traffic flows as expected, so no obvious

> > regressions from your changes.

> > 

> > I also tried switching to the IPQ8064 MDIO driver for my first device

> > (which is on the MDIO0 bus), but it's still not happy:

> > 

> > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

> > 

> Can you try the v6 version of this series?


FWIW I tried v6 without altering my DT at all (so still using the GPIO
MDIO driver, and not switching to use the alternate PHY support) and got
an oops due to a NULL pointer dereference, apparently in the mdio_bus
locking code. I'm back porting to 5.10.37 (because I track LTS on the
device) so I might be missing something, but the v4 version I tried
previously worked ok.

8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000558
pgd = (ptrval)
[00000558] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.10.37-00045-g7d01aa2545cb #2
Hardware name: Generic DT based system
Workqueue: events deferred_probe_work_func
PC is at mutex_lock+0x20/0x50
LR is at _cond_resched+0x28/0x4c
pc : [<c0a36b24>]    lr : [<c0a34530>]    psr: 60000013
sp : c1569c38  ip : 00000001  fp : c17bd1dc
r10: c17bd1c0  r9 : 00000000  r8 : 00000000
r7 : 00000002  r6 : 00000558  r5 : 00000000  r4 : 00000558
r3 : c1560000  r2 : c0e54389  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5787d  Table: 4220406a  DAC: 00000051
Process kworker/0:1 (pid: 20, stack limit = 0x(ptrval))
Stack: (0xc1569c38 to 0xc156a000)
9c20:                                                       c8020000 c07ee9b8
9c40: c17cc000 c17cc000 00000001 c07eea54 c17cc000 c07e25b8 c1569d14 c06d0f14
9c60: 00000000 30353630 ffff0a00 c17cc558 c17cc000 00000001 00000002 00000000
9c80: 00000000 c17bd1c0 c17bd1dc c07e2810 c0f04cc8 c17cc000 00000001 c0f04cc8
9ca0: 00000000 c07df518 ffffff08 ffff0a00 c0f0517c 00000000 00000000 ffffffff
9cc0: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9ce0: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9d00: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff
9d20: ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff 58cdc110
9d40: c17cc578 c0f04cc8 c17cc000 c17cc000 00000001 c17cc578 00000000 c17bd1c0
9d60: c17bd1dc c07e20a0 00000000 c17bd1c0 c17bd1dc 58cdc110 00000001 c17cc000
9d80: 00000001 c17cc008 c17cc578 00000000 c17bd1c0 c07e29a4 c17bc040 00000000
9da0: c17bc040 c17d2800 c17bd0cc 00000001 c17bd1c0 c0a11814 00000000 00000dc0
9dc0: c0b81b20 c0f04cc8 c17bd1dc c0ceca94 c0d6e2b4 c17bd1c0 00000005 c17d2640
9de0: 00000040 c177c400 00000000 c0fe8b08 c17bc000 00000040 c177c400 00000000
9e00: 00000000 58cdc110 c0b59f78 c0fc4e44 c177c400 c102f58c 00000000 c0fe8b08
9e20: c0fc4e44 00000001 00000000 c07e2d04 c177c400 c102f584 c102f58c c0755e50
9e40: 60000013 58cdc110 c0d2b58c c177c400 c0fc4e44 c1569ecc c0fe8b08 00000001
9e60: c0d2b58c c0fe8b08 ffffe000 c0756400 c177c400 00000001 00000001 00000000
9e80: c0f04cc8 c1569ecc c0756664 00000001 c0d2b58c c0fe8b08 ffffe000 c0754028
9ea0: c0d2b58c c15aac6c c17808b8 58cdc110 c177c400 c177c400 c0f04cc8 c177c444
9ec0: c0fba314 c0755ca8 00000002 c177c400 00000001 58cdc110 c177c400 c177c400
9ee0: c0fc21f4 c0fba314 c0fe8b08 c0754dac c177c400 c0fba300 c0fba300 c075530c
9f00: c0fba340 c1403f00 df6917c0 df694900 00000000 c0fdf300 00000000 c033c034
9f20: c1560000 df6917c0 00000008 c1403f00 c1403f14 df6917c0 00000008 c0f03d00
9f40: df6917d8 df6917c0 ffffe000 c033c3cc c1560000 c0fdeb1e c0cdfe28 c033c388
9f60: c1403f00 c14e6040 c1549280 00000000 c1568000 c033c388 c1403f00 c14bdea4
9f80: c14e6064 c034282c 00000001 c1549280 c03426dc 00000000 00000000 00000000
9fa0: 00000000 00000000 00000000 c0300148 00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c0a36b24>] (mutex_lock) from [<c07ee9b8>] (qca8k_mdio_read+0x30/0xac)
[<c07ee9b8>] (qca8k_mdio_read) from [<c07eea54>] (qca8k_phy_read+0x20/0x30)
[<c07eea54>] (qca8k_phy_read) from [<c07e25b8>] (__mdiobus_read+0x3c/0x1ac)
[<c07e25b8>] (__mdiobus_read) from [<c07e2810>] (mdiobus_read+0x30/0x44)
[<c07e2810>] (mdiobus_read) from [<c07df518>] (get_phy_device+0x13c/0x25c)
[<c07df518>] (get_phy_device) from [<c07e20a0>] (mdiobus_scan+0xb0/0x190)
[<c07e20a0>] (mdiobus_scan) from [<c07e29a4>] (__mdiobus_register+0x17c/0x2fc)
[<c07e29a4>] (__mdiobus_register) from [<c0a11814>] (dsa_register_switch+0xbb8/0xc6c)
[<c0a11814>] (dsa_register_switch) from [<c07e2d04>] (mdio_probe+0x2c/0x50)
[<c07e2d04>] (mdio_probe) from [<c0755e50>] (really_probe+0x108/0x4f4)
[<c0755e50>] (really_probe) from [<c0756400>] (driver_probe_device+0x78/0x1e4)
[<c0756400>] (driver_probe_device) from [<c0754028>] (bus_for_each_drv+0x80/0xc4)
[<c0754028>] (bus_for_each_drv) from [<c0755ca8>] (__device_attach+0xe0/0x178)
[<c0755ca8>] (__device_attach) from [<c0754dac>] (bus_probe_device+0x84/0x8c)
[<c0754dac>] (bus_probe_device) from [<c075530c>] (deferred_probe_work_func+0x9c/0xdc)
[<c075530c>] (deferred_probe_work_func) from [<c033c034>] (process_one_work+0x22c/0x580)
[<c033c034>] (process_one_work) from [<c033c3cc>] (worker_thread+0x44/0x5c8)
[<c033c3cc>] (worker_thread) from [<c034282c>] (kthread+0x150/0x154)
[<c034282c>] (kthread) from [<c0300148>] (ret_from_fork+0x14/0x2c)
Exception stack(0xc1569fb0 to 0xc1569ff8)
9fa0:                                     00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e3c33d7f e3c3303f e593300c f594f000 (e1941f9f)
---[ end trace cc9433437c472cd4 ]---

J.

-- 
101 things you can't have too much of : 42 - Pepsi.
This .sig brought to you by the letter P and the number  1
Product of the Republic of HuggieTag
Jonathan McDowell May 16, 2021, 10:12 a.m. | #16
On Sun, May 16, 2021 at 10:49:59AM +0100, Jonathan McDowell wrote:
> On Sat, May 15, 2021 at 07:30:26PM +0200, Ansuel Smith wrote:

> > On Sat, May 15, 2021 at 06:00:46PM +0100, Jonathan McDowell wrote:

> > > On Sat, May 08, 2021 at 08:05:58PM +0200, Ansuel Smith wrote:

> > > > On Sat, May 08, 2021 at 08:02:33PM +0200, Andrew Lunn wrote:

> > > > > On Sat, May 08, 2021 at 02:28:51AM +0200, Ansuel Smith wrote:

> > > > > > Fix mixed whitespace and tab for define spacing.

> > > > > 

> > > > > Please add a patch [0/28] which describes the big picture of what

> > > > > these changes are doing.

> > > > > 

> > > > > Also, this series is getting big. You might want to split it into two,

> > > > > One containing the cleanup, and the second adding support for the new

> > > > > switch.

> > > > > 

> > > > > 	Andrew

> > > > 

> > > > There is a 0/28. With all the changes. Could be that I messed the cc?

> > > > I agree think it's better to split this for the mdio part, the cleanup

> > > > and the changes needed for the internal phy.

> > > 

> > > FWIW I didn't see the 0/28 mail either. I tried these out on my RB3011

> > > today. I currently use the GPIO MDIO driver because I saw issues with

> > > the IPQ8064 driver in the past, and sticking with the GPIO driver I see

> > > both QCA8337 devices and traffic flows as expected, so no obvious

> > > regressions from your changes.

> > > 

> > > I also tried switching to the IPQ8064 MDIO driver for my first device

> > > (which is on the MDIO0 bus), but it's still not happy:

> > > 

> > > qca8k 37000000.mdio-mii:10: Switch id detected 0 but expected 13

> > > 

> > Can you try the v6 version of this series?

> 

> FWIW I tried v6 without altering my DT at all (so still using the GPIO

> MDIO driver, and not switching to use the alternate PHY support) and got

> an oops due to a NULL pointer dereference, apparently in the mdio_bus

> locking code. I'm back porting to 5.10.37 (because I track LTS on the

> device) so I might be missing something, but the v4 version I tried

> previously worked ok.


I dropped patches 20-25 of the series (i.e. the piece that adds the
internal phy/mdio support) and tried again and that works fine, so it
does look like I either managed to mismerge them somehow (and those
pieces weren't the ones with conflicts) or there's a problem (possibly
only when the DT hasn't been updated to use the internal bus?).

J.

-- 
   101 things you can't have too   |  .''`.  Debian GNU/Linux Developer
     much of : 45 - Toblerone.     | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.

Patch

diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c
index 1bd18857e1c5..fb1614242e13 100644
--- a/drivers/net/mdio/mdio-ipq8064.c
+++ b/drivers/net/mdio/mdio-ipq8064.c
@@ -15,25 +15,26 @@ 
 #include <linux/mfd/syscon.h>
 
 /* MII address register definitions */
-#define MII_ADDR_REG_ADDR                       0x10
-#define MII_BUSY                                BIT(0)
-#define MII_WRITE                               BIT(1)
-#define MII_CLKRANGE_60_100M                    (0 << 2)
-#define MII_CLKRANGE_100_150M                   (1 << 2)
-#define MII_CLKRANGE_20_35M                     (2 << 2)
-#define MII_CLKRANGE_35_60M                     (3 << 2)
-#define MII_CLKRANGE_150_250M                   (4 << 2)
-#define MII_CLKRANGE_250_300M                   (5 << 2)
+#define MII_ADDR_REG_ADDR			0x10
+#define MII_BUSY				BIT(0)
+#define MII_WRITE				BIT(1)
+#define MII_CLKRANGE(x)				((x) << 2)
+#define MII_CLKRANGE_60_100M			MII_CLKRANGE(0)
+#define MII_CLKRANGE_100_150M			MII_CLKRANGE(1)
+#define MII_CLKRANGE_20_35M			MII_CLKRANGE(2)
+#define MII_CLKRANGE_35_60M			MII_CLKRANGE(3)
+#define MII_CLKRANGE_150_250M			MII_CLKRANGE(4)
+#define MII_CLKRANGE_250_300M			MII_CLKRANGE(5)
 #define MII_CLKRANGE_MASK			GENMASK(4, 2)
 #define MII_REG_SHIFT				6
 #define MII_REG_MASK				GENMASK(10, 6)
 #define MII_ADDR_SHIFT				11
 #define MII_ADDR_MASK				GENMASK(15, 11)
 
-#define MII_DATA_REG_ADDR                       0x14
+#define MII_DATA_REG_ADDR			0x14
 
-#define MII_MDIO_DELAY_USEC                     (1000)
-#define MII_MDIO_RETRY_MSEC                     (10)
+#define MII_MDIO_DELAY_USEC			(1000)
+#define MII_MDIO_RETRY_MSEC			(10)
 
 struct ipq8064_mdio {
 	struct regmap *base; /* NSS_GMAC0_BASE */