Message ID | 1321024239-18861-1-git-send-email-afleming@freescale.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Andy Fleming <afleming@freescale.com> Date: Fri, 11 Nov 2011 09:10:39 -0600 > The code for setting the address of the internal TBI PHY was > convoluted enough without a maze of ifdefs. Clean it up a bit > so we allow the logic to fail down to -ENODEV at the end of > the if/else ladder, rather than using ifdefs to repeat the same > failure code over and over. > > Also, remove the support for the auto-configuration. I'm not aware of > anyone using it, and it ends up using the bus mutex before it's been > initialized. > > Signed-off-by: Andy Fleming <afleming@freescale.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On Fri, Nov 11, 2011 at 05:10:39AM -0000, Andy Fleming wrote: > The code for setting the address of the internal TBI PHY was > convoluted enough without a maze of ifdefs. Clean it up a bit > so we allow the logic to fail down to -ENODEV at the end of > the if/else ladder, rather than using ifdefs to repeat the same > failure code over and over. > > Also, remove the support for the auto-configuration. I'm not aware of > anyone using it, and it ends up using the bus mutex before it's been > initialized. With this applied I'm getting fsl-pq_mdio: probe of ffe24000.mdio failed with error -16 on my P1011 board which does not have a "tbi-phy" node. I'll post a fix shortly. baruch > Signed-off-by: Andy Fleming <afleming@freescale.com> baruch
On Nov 13, 2011, at 11:26 PM, David Miller wrote: > From: Andy Fleming <afleming@freescale.com> > Date: Fri, 11 Nov 2011 09:10:39 -0600 > >> The code for setting the address of the internal TBI PHY was >> convoluted enough without a maze of ifdefs. Clean it up a bit >> so we allow the logic to fail down to -ENODEV at the end of >> the if/else ladder, rather than using ifdefs to repeat the same >> failure code over and over. >> >> Also, remove the support for the auto-configuration. I'm not aware of >> anyone using it, and it ends up using the bus mutex before it's been >> initialized. >> >> Signed-off-by: Andy Fleming <afleming@freescale.com> > > Applied, thanks. I believe we need this on mainline otherwise we get something like: Fixed MDIO Bus: probed Unable to handle kernel paging request for data at address 0x00000000 Faulting instruction address: 0xc0451630 Oops: Kernel access of bad area, sig: 11 [#1] SMP NR_CPUS=2 P1022 DS Modules linked in: NIP: c0451630 LR: c0451618 CTR: 00000007 REGS: ef03fce0 TRAP: 0300 Not tainted (3.2.0-rc3-00099-g883381d) MSR: 00029000 <EE,ME,CE> CR: 24042022 XER: 00000000 DEAR: 00000000, ESR: 00800000 TASK = ef040000[1] 'swapper' THREAD: ef03e000 CPU: 0 GPR00: ef03fd98 ef03fd90 ef040000 ef1ab22c 00000000 00000002 ffeb0000 0000fffe GPR08: b0541215 00000000 00000000 00000000 24042028 23c406c2 00000000 00000000 GPR16: c0000a00 00000014 3fffffff 03ff9000 00000015 7ff3a760 f1044030 fffffff4 GPR24: c053e128 ef1ab230 c056a3a8 ef03e000 ef040000 ef1ab22c 00000000 ef1ab228 NIP [c0451630] __mutex_lock_slowpath+0xb4/0x190 LR [c0451618] __mutex_lock_slowpath+0x9c/0x190 Call Trace: [ef03fdd0] [c0451758] mutex_lock+0x4c/0x50 [ef03fde0] [c02b5124] mdiobus_read+0x38/0x74 [ef03fe00] [c02b41f4] get_phy_id+0x24/0x80 [ef03fe20] [c02b9de4] fsl_pq_mdio_probe+0x3b4/0x580 [ef03feb0] [c0266120] platform_drv_probe+0x20/0x30 [ef03fec0] [c0264bbc] driver_probe_device+0xa4/0x1d4 [ef03fee0] [c0264da8] __driver_attach+0xbc/0xc0 [ef03ff00] [c0263ac0] bus_for_each_dev+0x60/0x9c [ef03ff30] [c02647f4] driver_attach+0x24/0x34 [ef03ff40] [c0264444] bus_add_driver+0x1ac/0x274 [ef03ff70] [c02651b0] driver_register+0x88/0x154 [ef03ff90] [c0266450] platform_driver_register+0x68/0x78 [ef03ffa0] [c05d93b8] fsl_pq_mdio_init+0x18/0x28 [ef03ffb0] [c0001eb8] do_one_initcall+0x34/0x1a8 [ef03ffe0] [c05bb82c] kernel_init+0xa0/0x13c [ef03fff0] [c000d878] kernel_thread+0x4c/0x68 Instruction dump: 801c0020 2f800063 419dffe8 3bbf0004 7fa3eb78 48001aad 813f000c 38010008 3b3f0008 901f000c 93210008 9121000c 3800ffff 93810010 7c0004ac ---[ end trace 1643a9a9c5097f8f ]--- Kernel panic - not syncing: Attempted to kill init! Call Trace: [ef03fbc0] [c0008044] show_stack+0x44/0x154 (unreliable) [ef03fc00] [c04532c8] panic+0xa4/0x1d8 [ef03fc50] [c0049a00] do_exit+0x5dc/0x684 [ef03fca0] [c000a6f0] die+0xdc/0x1b4 [ef03fcc0] [c00128d0] bad_page_fault+0xb4/0xfc [ef03fcd0] [c000ebe4] handle_page_fault+0x7c/0x80 --- Exception: 300 at __mutex_lock_slowpath+0xb4/0x190 LR = __mutex_lock_slowpath+0x9c/0x190 [ef03fd90] [00000000] (null) (unreliable) [ef03fdd0] [c0451758] mutex_lock+0x4c/0x50 [ef03fde0] [c02b5124] mdiobus_read+0x38/0x74 [ef03fe00] [c02b41f4] get_phy_id+0x24/0x80 [ef03fe20] [c02b9de4] fsl_pq_mdio_probe+0x3b4/0x580 [ef03feb0] [c0266120] platform_drv_probe+0x20/0x30 [ef03fec0] [c0264bbc] driver_probe_device+0xa4/0x1d4 [ef03fee0] [c0264da8] __driver_attach+0xbc/0xc0 [ef03ff00] [c0263ac0] bus_for_each_dev+0x60/0x9c [ef03ff30] [c02647f4] driver_attach+0x24/0x34 [ef03ff40] [c0264444] bus_add_driver+0x1ac/0x274 [ef03ff70] [c02651b0] driver_register+0x88/0x154 [ef03ff90] [c0266450] platform_driver_register+0x68/0x78 [ef03ffa0] [c05d93b8] fsl_pq_mdio_init+0x18/0x28 [ef03ffb0] [c0001eb8] do_one_initcall+0x34/0x1a8 [ef03ffe0] [c05bb82c] kernel_init+0xa0/0x13c [ef03fff0] [c000d878] kernel_thread+0x4c/0x68 Rebooting in 180 seconds.. - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 1, 2011 at 10:38 PM, Kumar Gala <galak@kernel.crashing.org> wrote: > > On Nov 13, 2011, at 11:26 PM, David Miller wrote: > >> From: Andy Fleming <afleming@freescale.com> >> Date: Fri, 11 Nov 2011 09:10:39 -0600 >> >>> The code for setting the address of the internal TBI PHY was >>> convoluted enough without a maze of ifdefs. Clean it up a bit >>> so we allow the logic to fail down to -ENODEV at the end of >>> the if/else ladder, rather than using ifdefs to repeat the same >>> failure code over and over. >>> >>> Also, remove the support for the auto-configuration. I'm not aware of >>> anyone using it, and it ends up using the bus mutex before it's been >>> initialized. >>> >>> Signed-off-by: Andy Fleming <afleming@freescale.com> >> >> Applied, thanks. > > I believe we need this on mainline otherwise we get something like: I concur. I also have a 2 separate patches I will send out tonight, which are independent of each other, but related. The first will revert c3e072f8a6c5625028531c40ec65f7e301531be2 (net: fsl_pq_mdio: fix non tbi phy access), which hides a bug. The second will fix the bug in our p1/p2 device trees so that they all have tbi nodes. The first patch only applies to net-next (reverts a commit in that tree). The second patch only applies to Kumar's "next" branch (applies changes on top of significant device-tree changes). I have no idea how you guys want to deal with that. Personally, I think that the patches can be applied to their respective trees, and anyone who is working off the bleeding edge can go find the appropriate patch in the other tree. If we get my first patch into mainline, then I imagine Kumar's tree will have all the necessary components... Anyway, I will leave that to you. I will submit the two patches in the next hour or so. Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Dec 1, 2011, at 10:38 PM, Kumar Gala wrote: > > On Nov 13, 2011, at 11:26 PM, David Miller wrote: > >> From: Andy Fleming <afleming@freescale.com> >> Date: Fri, 11 Nov 2011 09:10:39 -0600 >> >>> The code for setting the address of the internal TBI PHY was >>> convoluted enough without a maze of ifdefs. Clean it up a bit >>> so we allow the logic to fail down to -ENODEV at the end of >>> the if/else ladder, rather than using ifdefs to repeat the same >>> failure code over and over. >>> >>> Also, remove the support for the auto-configuration. I'm not aware of >>> anyone using it, and it ends up using the bus mutex before it's been >>> initialized. >>> >>> Signed-off-by: Andy Fleming <afleming@freescale.com> >> >> Applied, thanks. > > I believe we need this on mainline otherwise we get something like: > > Fixed MDIO Bus: probed > Unable to handle kernel paging request for data at address 0x00000000 > Faulting instruction address: 0xc0451630 > Oops: Kernel access of bad area, sig: 11 [#1] > SMP NR_CPUS=2 P1022 DS > Modules linked in: > NIP: c0451630 LR: c0451618 CTR: 00000007 > REGS: ef03fce0 TRAP: 0300 Not tainted (3.2.0-rc3-00099-g883381d) > MSR: 00029000 <EE,ME,CE> CR: 24042022 XER: 00000000 > DEAR: 00000000, ESR: 00800000 > TASK = ef040000[1] 'swapper' THREAD: ef03e000 CPU: 0 > GPR00: ef03fd98 ef03fd90 ef040000 ef1ab22c 00000000 00000002 ffeb0000 0000fffe > GPR08: b0541215 00000000 00000000 00000000 24042028 23c406c2 00000000 00000000 > GPR16: c0000a00 00000014 3fffffff 03ff9000 00000015 7ff3a760 f1044030 fffffff4 > GPR24: c053e128 ef1ab230 c056a3a8 ef03e000 ef040000 ef1ab22c 00000000 ef1ab228 > NIP [c0451630] __mutex_lock_slowpath+0xb4/0x190 > LR [c0451618] __mutex_lock_slowpath+0x9c/0x190 > Call Trace: > [ef03fdd0] [c0451758] mutex_lock+0x4c/0x50 > [ef03fde0] [c02b5124] mdiobus_read+0x38/0x74 > [ef03fe00] [c02b41f4] get_phy_id+0x24/0x80 > [ef03fe20] [c02b9de4] fsl_pq_mdio_probe+0x3b4/0x580 > [ef03feb0] [c0266120] platform_drv_probe+0x20/0x30 > [ef03fec0] [c0264bbc] driver_probe_device+0xa4/0x1d4 > [ef03fee0] [c0264da8] __driver_attach+0xbc/0xc0 > [ef03ff00] [c0263ac0] bus_for_each_dev+0x60/0x9c > [ef03ff30] [c02647f4] driver_attach+0x24/0x34 > [ef03ff40] [c0264444] bus_add_driver+0x1ac/0x274 > [ef03ff70] [c02651b0] driver_register+0x88/0x154 > [ef03ff90] [c0266450] platform_driver_register+0x68/0x78 > [ef03ffa0] [c05d93b8] fsl_pq_mdio_init+0x18/0x28 > [ef03ffb0] [c0001eb8] do_one_initcall+0x34/0x1a8 > [ef03ffe0] [c05bb82c] kernel_init+0xa0/0x13c > [ef03fff0] [c000d878] kernel_thread+0x4c/0x68 > Instruction dump: > 801c0020 2f800063 419dffe8 3bbf0004 7fa3eb78 48001aad 813f000c 38010008 > 3b3f0008 901f000c 93210008 9121000c > 3800ffff 93810010 7c0004ac > ---[ end trace 1643a9a9c5097f8f ]--- > Kernel panic - not syncing: Attempted to kill init! > Call Trace: > [ef03fbc0] [c0008044] show_stack+0x44/0x154 (unreliable) > [ef03fc00] [c04532c8] panic+0xa4/0x1d8 > [ef03fc50] [c0049a00] do_exit+0x5dc/0x684 > [ef03fca0] [c000a6f0] die+0xdc/0x1b4 > [ef03fcc0] [c00128d0] bad_page_fault+0xb4/0xfc > [ef03fcd0] [c000ebe4] handle_page_fault+0x7c/0x80 > --- Exception: 300 at __mutex_lock_slowpath+0xb4/0x190 > LR = __mutex_lock_slowpath+0x9c/0x190 > [ef03fd90] [00000000] (null) (unreliable) > [ef03fdd0] [c0451758] mutex_lock+0x4c/0x50 > [ef03fde0] [c02b5124] mdiobus_read+0x38/0x74 > [ef03fe00] [c02b41f4] get_phy_id+0x24/0x80 > [ef03fe20] [c02b9de4] fsl_pq_mdio_probe+0x3b4/0x580 > [ef03feb0] [c0266120] platform_drv_probe+0x20/0x30 > [ef03fec0] [c0264bbc] driver_probe_device+0xa4/0x1d4 > [ef03fee0] [c0264da8] __driver_attach+0xbc/0xc0 > [ef03ff00] [c0263ac0] bus_for_each_dev+0x60/0x9c > [ef03ff30] [c02647f4] driver_attach+0x24/0x34 > [ef03ff40] [c0264444] bus_add_driver+0x1ac/0x274 > [ef03ff70] [c02651b0] driver_register+0x88/0x154 > [ef03ff90] [c0266450] platform_driver_register+0x68/0x78 > [ef03ffa0] [c05d93b8] fsl_pq_mdio_init+0x18/0x28 > [ef03ffb0] [c0001eb8] do_one_initcall+0x34/0x1a8 > [ef03ffe0] [c05bb82c] kernel_init+0xa0/0x13c > [ef03fff0] [c000d878] kernel_thread+0x4c/0x68 > Rebooting in 180 seconds.. David, Any comment on getting this patch pulled into net.git? - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kumar Gala <galak@kernel.crashing.org> Date: Mon, 5 Dec 2011 23:38:14 -0600 > Any comment on getting this patch pulled into net.git? It requires a DT patch or related set of changes which have need to be done differently as per feedback. My understanding is there are two patches required, this revert plus the changes to add the tbi property to the DT which are missing it. That's why these tbi changes went in to begin with, to handle DT which lack the property, or something like that. So you can't just revert this thing without also fixing the DT that lack the tbi property, or else you break those systems. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Dec 5, 2011, at 11:41 PM, David Miller wrote: > From: Kumar Gala <galak@kernel.crashing.org> > Date: Mon, 5 Dec 2011 23:38:14 -0600 > >> Any comment on getting this patch pulled into net.git? > > It requires a DT patch or related set of changes which have need > to be done differently as per feedback. > > My understanding is there are two patches required, this revert plus > the changes to add the tbi property to the DT which are missing it. > > That's why these tbi changes went in to begin with, to handle DT > which lack the property, or something like that. So you can't > just revert this thing without also fixing the DT that lack the > tbi property, or else you break those systems. We need this patch in 3.2 regardless of DT. Currently we get the oops on 3.2 on ALL systems. Andy should be working up a device tree patch to fix the subset of .dts that are missing the tbi property. What happens in 'next' is slightly different issue. - k-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kumar Gala <galak@kernel.crashing.org> Date: Tue, 6 Dec 2011 23:35:31 -0600 > We need this patch in 3.2 regardless of DT. Currently we get the > oops on 3.2 on ALL systems. Andy should be working up a device tree > patch to fix the subset of .dts that are missing the tbi property. Ok, applied to 'net'. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c index 52f4e8a..4d9f84b 100644 --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c @@ -183,28 +183,10 @@ void fsl_pq_mdio_bus_name(char *name, struct device_node *np) } EXPORT_SYMBOL_GPL(fsl_pq_mdio_bus_name); -/* Scan the bus in reverse, looking for an empty spot */ -static int fsl_pq_mdio_find_free(struct mii_bus *new_bus) -{ - int i; - - for (i = PHY_MAX_ADDR; i > 0; i--) { - u32 phy_id; - - if (get_phy_id(new_bus, i, &phy_id)) - return -1; - - if (phy_id == 0xffffffff) - break; - } - - return i; -} - -#if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE) static u32 __iomem *get_gfar_tbipa(struct fsl_pq_mdio __iomem *regs, struct device_node *np) { +#if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE) struct gfar __iomem *enet_regs; /* @@ -220,15 +202,15 @@ static u32 __iomem *get_gfar_tbipa(struct fsl_pq_mdio __iomem *regs, struct devi } else if (of_device_is_compatible(np, "fsl,etsec2-mdio") || of_device_is_compatible(np, "fsl,etsec2-tbi")) { return of_iomap(np, 1); - } else - return NULL; -} + } #endif + return NULL; +} -#if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE) static int get_ucc_id_for_range(u64 start, u64 end, u32 *ucc_id) { +#if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE) struct device_node *np = NULL; int err = 0; @@ -261,9 +243,10 @@ static int get_ucc_id_for_range(u64 start, u64 end, u32 *ucc_id) return err; else return -EINVAL; -} +#else + return -ENODEV; #endif - +} static int fsl_pq_mdio_probe(struct platform_device *ofdev) { @@ -339,19 +322,13 @@ static int fsl_pq_mdio_probe(struct platform_device *ofdev) of_device_is_compatible(np, "fsl,etsec2-mdio") || of_device_is_compatible(np, "fsl,etsec2-tbi") || of_device_is_compatible(np, "gianfar")) { -#if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE) tbipa = get_gfar_tbipa(regs, np); if (!tbipa) { err = -EINVAL; goto err_free_irqs; } -#else - err = -ENODEV; - goto err_free_irqs; -#endif } else if (of_device_is_compatible(np, "fsl,ucc-mdio") || of_device_is_compatible(np, "ucc_geth_phy")) { -#if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE) u32 id; static u32 mii_mng_master; @@ -364,10 +341,6 @@ static int fsl_pq_mdio_probe(struct platform_device *ofdev) mii_mng_master = id; ucc_set_qe_mux_mii_mng(id - 1); } -#else - err = -ENODEV; - goto err_free_irqs; -#endif } else { err = -ENODEV; goto err_free_irqs; @@ -386,16 +359,6 @@ static int fsl_pq_mdio_probe(struct platform_device *ofdev) } if (tbiaddr == -1) { - out_be32(tbipa, 0); - - tbiaddr = fsl_pq_mdio_find_free(new_bus); - } - - /* - * We define TBIPA at 0 to be illegal, opting to fail for boards that - * have PHYs at 1-31, rather than change tbipa and rescan. - */ - if (tbiaddr == 0) { err = -EBUSY; goto err_free_irqs;
The code for setting the address of the internal TBI PHY was convoluted enough without a maze of ifdefs. Clean it up a bit so we allow the logic to fail down to -ENODEV at the end of the if/else ladder, rather than using ifdefs to repeat the same failure code over and over. Also, remove the support for the auto-configuration. I'm not aware of anyone using it, and it ends up using the bus mutex before it's been initialized. Signed-off-by: Andy Fleming <afleming@freescale.com> --- drivers/net/ethernet/freescale/fsl_pq_mdio.c | 53 ++++---------------------- 1 files changed, 8 insertions(+), 45 deletions(-)