| Message ID | 20210508002920.19945-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next,v4,01/28] net: mdio: ipq8064: clean whitespaces in define | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
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>
On Sat, 2021-05-08 at 02:28 +0200, Ansuel Smith wrote: > Fix mixed whitespace and tab for define spacing. Incomplete commit message here. > diff --git a/drivers/net/mdio/mdio-ipq8064.c b/drivers/net/mdio/mdio-ipq8064.c [] > -#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_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) Adding a define and using it isn't just whitespace.
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
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?
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.
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?
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)
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.
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.
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.
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.
> > 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
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/
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.
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.
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.
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 */