| Message ID | 1241641262-5202-6-git-send-email-wd@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> > Signed-off-by: Wolfgang Denk <wd@denx.de> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: John Rigby <jcrigby@gmail.com> > --- > This patch is NOT intended for inclusion into mainline, but rather a > request for help. For some reason which I don't understand yet, the > Ethernet interface on the ARIA board does not work in the default > configuration, because MII probing fails. > > What I'm seeing is this; the problem is with this part of code in > "drivers/net/fs_enet/mii-fec.c": > > 156 fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; > ... > 163 out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed); Just a stab in the dark: Perhaps the fec->fecp->fec_mii_speed field is misaligned or is 16 bits ? Jocke
Dear Joakim Tjernlund, In message <OF97606123.49B50465-ONC12575AF.002E2518-C12575AF.002E6300@transmode.se> you wrote: > > Just a stab in the dark: Perhaps the fec->fecp->fec_mii_speed field is > misaligned or is 16 bits ? Good idea. The RM documents the register at offset 0x44 and describes it as 32 bits... and it's working fine on the MPC5121ADS board, but fails on ARIA. Thanks. Best regards, Wolfgang Denk
Wolfgang Denk <wd@denx.de> wrote on 07/05/2009 11:19:48: > > Dear Joakim Tjernlund, > > In message <OF97606123.49B50465-ONC12575AF.002E2518-C12575AF.002E6300@transmode.se> you wrote: > > > > Just a stab in the dark: Perhaps the fec->fecp->fec_mii_speed field is > > misaligned or is 16 bits ? > > Good idea. The RM documents the register at offset 0x44 and describes > it as 32 bits... and it's working fine on the MPC5121ADS board, but > fails on ARIA. OK, this is just a guess too: Some odd byte order requirements? No more ideas, sorry. Jocke
I think the fec's parent clock is the ipb clock not the ppc core clock. Could that be the problem? On Wed, May 6, 2009 at 2:21 PM, Wolfgang Denk <wd@denx.de> wrote: > Signed-off-by: Wolfgang Denk <wd@denx.de> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: John Rigby <jcrigby@gmail.com> > --- > This patch is NOT intended for inclusion into mainline, but rather a > request for help. For some reason which I don't understand yet, the > Ethernet interface on the ARIA board does not work in the default > configuration, because MII probing fails. > > What I'm seeing is this; the problem is with this part of code in > "drivers/net/fs_enet/mii-fec.c": > > 156 fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; > ... > 163 out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed); > > I added some debug messages, and this is what I see: > > On the ADS5121, we have the CPU clocked at 400 MHz. I get: > ... > ## ppc_proc_freq = 399999996, fec->mii_speed = 160 > FEC MII Bus: probed > ... > It works fine. > > According to the Ref. Man.: > A value of 0 in this field turns off the MDC and leaves it in > a low-voltage state. Any non-zero value results in the MDC > frequency of 1/(mii_speed*2) of the system clock frequency. > that means we have a MDC frequency of > 400 MHz / (2 * 160) = 1.25 MHz > which is obviously within the 2.5 MHz limit. > > Now ARIA is currently running at 316.8 MHz, and this is what I get: > ... > ## ppc_proc_freq = 316800000, fec->mii_speed = 128 > fsl-fec-mdio: probe of 80002800.mdio failed with error -5 > ... > It fails. MDC frequency is > 316.8 MHz / (2 * 128) = 1.24 MHz > which should be fine. > > However, If I change the code to > > fec->mii_speed = (((ppc_proc_freq / 1000000) / 30) + 1) << 1; > > then I get: > ... > ## ppc_proc_freq = 316800000, fec->mii_speed = 22 > FEC MII Bus: probed > ... and it's working!!! However, I compute MDC frequency as > 316.8 MHz / (2 * 22) = 7.2 MHz > which is far above the maximum allowed clock of 2.5 MHz ??? > > Has anybody any idea what might be going on here? > > > drivers/net/fs_enet/mii-fec.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c > index 9d8bd97..a51dd83 100644 > --- a/drivers/net/fs_enet/mii-fec.c > +++ b/drivers/net/fs_enet/mii-fec.c > @@ -153,8 +153,12 @@ static int __devinit fs_enet_mdio_probe(struct > of_device *ofdev, > if (!fec->fecp) > goto out_fec; > > +#if 0 > fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; > - > +#else > + fec->mii_speed = (((ppc_proc_freq / 1000000) / 30) + 1) << 1; > + printk("## ppc_proc_freq = %d, fec->mii_speed = %d\n", > ppc_proc_freq, fec->mii_speed); > +#endif > setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE); > setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX | > FEC_ECNTRL_ETHER_EN); > -- > 1.6.0.6 > >
Dear John, In message <4b73d43f0905071909v6e6e8b2el9eb6d4a1b9038f45@mail.gmail.com> you wrote: > > I think the fec's parent clock is the ipb clock not the ppc core clock. > Could that be the problem? I don't think so. When debugging, I printed the actual clock frequencies, and they looked as expected. And "arch/powerpc/platforms/512x/clock.c" has this: 385 static struct clk fec_clk = { 386 .name = "fec_clk", 387 .flags = CLK_HAS_CTRL, 388 .reg = 0, 389 .bit = 13, 390 .calc = unity_clk_calc, 391 .parent = &ips_clk, 392 }; which looks OK to me. Best regards, Wolfgang Denk
Dear John, in message <4b73d43f0906061527p7ca1b301ybcfc576870a168d5@mail.gmail.com> you wrote: > > I noticed the latest BSP from Freescale has this patch: > > From: Chen Hongjun <Hong-jun.chen@freecale.com> > Date: Thu, 16 Apr 2009 20:22:52 +0800 > Subject: [PATCH] Fixed FEC bug for bluestone board. > > Signed-off-by: Chen Hongjun <Hong-jun.chen@freecale.com> > --- > drivers/net/fs_enet/mii-fec.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c > index 13a7d66..53d01a8 100644 > --- a/drivers/net/fs_enet/mii-fec.c > +++ b/drivers/net/fs_enet/mii-fec.c > @@ -208,7 +208,7 @@ static int __devinit fs_enet_mdio_probe(struct of_device > *ofdev, > if (!fec->fecp) > goto out_fec; > > - fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; > + fec->mii_speed = ((((ppc_proc_freq + 4999999) / 2500000) / 2) & 0x3F) << 1; Heh. So we now have 3 versions: mainline: fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; Freescale: fec->mii_speed = ((((ppc_proc_freq + 4999999) / 2500000) / 2) & 0x3F) << 1; we: fec->mii_speed = (((ppc_proc_freq / 1000000) / 30) + 1) << 1; So what does this give: ppc_proc_freq mii_speed mainline freescale we -------------------------------------------------- 50 MHz 0x14 -> 2.5 MHz 0x14 -> 2.5 MHz 0x04 -> 12.50 MHz 100 MHz 0x28 -> 2.5 MHz 0x28 -> 2.5 MHz 0x08 -> 12.50 MHz 150 MHz 0x3C -> 2.5 MHz 0x3C -> 2.5 MHz 0x0C -> 12.50 MHz 200 MHz 0x50 -> 2.5 MHz 0x50 -> 2.5 MHz 0x0E -> 14.29 MHz 250 MHz 0x64 -> 2.5 MHz 0x64 -> 2.5 MHz 0x12 -> 13.89 MHz 300 MHz 0x78 -> 2.5 MHz 0x78 -> 2.5 MHz 0x16 -> 13.36 MHz 316.8 MHz 0x80 -> 2.475 MHz 0x00 -> MDC off 0x16 -> 14.40 MHz 350 MHz 0x8C -> 2.5 MHz 0x0C -> 29.17 MHz 0x18 -> 14.58 MHz 400 MHz 0xA0 -> 2.5 MHz 0x20 -> 12.50 MHz 0x1C -> 14.29 MHz 450 MHz 0xB3 -> 2.5 MHz 0x34 -> 8.654 MHz 0x20 -> 14.06 MHz 500 MHz 0xC8 -> 2.5 MHz 0x48 -> 6.944 MHz 0x22 -> 14.71 MHz So - the mainline version and what we have don't take into account that MII_SPEED uses only bit 25...30, i.e. it must fit into the range from (1 << 1) ... (3F << 1). The Freescale code tries to address this, but just clipping the data is incorrect as we can see above. The funny thing is that the RefMan says: "...MDC frequency of 1/(mii_speed*2) of the system clock frequency" "To be compliant with the IEEE MII specification, the MII_SPEED field must be programmed with a value that provides an MDC frequency of less than or equal to 2.5 MHz." The big question seems to be what the RefMan means when talking about the "system clock frequency". Obiously it is NOT the CPU clock as code variants above assume. The examples in "Table 17-24. Programming Examples for MII_SPEED Register" list "system clock frequencies" of 25, 33, 40 and 50 MHz - which also indiocates that some other frequency might be referenced here. But which one is it? Best regards, Wolfgang Denk
> > > > The big question seems to be what the RefMan means when talking about > the "system clock frequency". Obiously it is NOT the CPU clock as > code variants above assume. The examples in "Table 17-24. Programming > Examples for MII_SPEED Register" list "system clock frequencies" of > 25, 33, 40 and 50 MHz - which also indiocates that some other > frequency might be referenced here. > > But which one is it? My best guess is still that it is ips clock. I think I stated in a previous email ipb, but I meant ips. 5200 has ibp and 5121 has ips. Have you looked at he MII clock on a scope to see how the calculated values compare to actual? > > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > "Today's robots are very primitive, capable of understanding only a > few simple instructions such as 'go left', 'go right', and 'build > car'." - John Sladek >
> My best guess is still that it is ips clock. I think I stated in a
I fully agree.
a) Table 16-18 in the manual mentions ips and the values given there look much
more like ips than ppc_proc_freq (25MHz?)
b) the excerpt from clock.c Wolfgang posted mentions ips as parent
c) I cannot imagine a divider wrapping around at a frequency which is inside the
range of what the processor is capable of. MII only up to 300MHz seems like a
show-stopper to me :)
d) According to the MPC5200B-manual, ipb _is_ used there (and the linux-driver
adheres to that). I'd guess those two CPUs are related enough to assume it is
ips here.
Regards,
Wolfram
Dear John, in message <4b73d43f0906061708o763409d0u10a344dfc30e32c1@mail.gmail.com> you wrote: > > > The big question seems to be what the RefMan means when talking about > > the "system clock frequency". Obiously it is NOT the CPU clock as ... > > But which one is it? > > My best guess is still that it is ips clock. I think I stated in a previous > email ipb, but I meant ips. 5200 has ibp and 5121 has ips. Have you looked > at he MII clock on a scope to see how the calculated values compare to > actual? Yes, it seems very much as if you were right again. When using ips/ibp everything makes sense, and works. Hm... so that means on MPC512x we should use mpc512x_find_ips_freq(), while on MPC5200 we should use mpc52xx_find_ipb_freq() - but hey, apart from the name these two functions are identical. Grant - how would you like to see this handled? Should we merge these two code-wise identical functions into one? What should be the name, and in which file should we put it? [We need this clock thing for "drivers/net/fs_enet/mii-fec.c"...] Best regards, Wolfgang Denk
Wolfgang Denk wrote: > Dear John, > > in message <4b73d43f0906061708o763409d0u10a344dfc30e32c1@mail.gmail.com> you wrote: >>> The big question seems to be what the RefMan means when talking about >>> the "system clock frequency". Obiously it is NOT the CPU clock as > ... >>> But which one is it? >> My best guess is still that it is ips clock. I think I stated in a previous >> email ipb, but I meant ips. 5200 has ibp and 5121 has ips. Have you looked >> at he MII clock on a scope to see how the calculated values compare to >> actual? > > Yes, it seems very much as if you were right again. > > When using ips/ibp everything makes sense, and works. > > Hm... so that means on MPC512x we should use mpc512x_find_ips_freq(), > while on MPC5200 we should use mpc52xx_find_ipb_freq() - but hey, > apart from the name these two functions are identical. > > Grant - how would you like to see this handled? Should we merge these > two code-wise identical functions into one? What should be the name, > and in which file should we put it? > > [We need this clock thing for "drivers/net/fs_enet/mii-fec.c"...] I2C and MSCAN need it as well. What about implementing the more generic clk api for the MPC5200 as done for the MPC512x? http://lxr.linux.no/linux+v2.6.29/arch/powerpc/platforms/512x/clock.c The MSCAN can also operate with other clock sources, e.g. the external oscillator (ref_clk). This would avoid cumbersome frequency calculations from the clock bit settings in the driver as shown below: /* * Get the frequency of the external oscillator clock connected * to the SYS_XTAL_IN pin, or retrun 0 if it cannot be determined. */ static unsigned int __devinit mpc52xx_can_xtal_freq(struct device_node *np) { struct mpc52xx_cdm __iomem *cdm; struct device_node *np_cdm; unsigned int freq; u32 val; freq = mpc52xx_find_ipb_freq(np); if (!freq) return 0; /* * Detemine SYS_XTAL_IN frequency from the clock domain settings */ np_cdm = of_find_matching_node(NULL, mpc52xx_cdm_ids); cdm = of_iomap(np_cdm, 0); of_node_put(np_cdm); if (!np_cdm) { printk(KERN_ERR "%s() failed abnormally\n", __func__); return 0; } if (in_8(&cdm->ipb_clk_sel) & 0x1) freq *= 2; val = in_be32(&cdm->rstcfg); if (val & (1 << 5)) freq *= 8; else freq *= 4; if (val & (1 << 6)) freq /= 12; else freq /= 16; iounmap(cdm); return freq; } Wolfgang.
Dear Wolfgang, In message <4A2CC1DE.5040704@grandegger.com> you wrote: > > > Hm... so that means on MPC512x we should use mpc512x_find_ips_freq(), > > while on MPC5200 we should use mpc52xx_find_ipb_freq() - but hey, > > apart from the name these two functions are identical. > > > > Grant - how would you like to see this handled? Should we merge these > > two code-wise identical functions into one? What should be the name, > > and in which file should we put it? > > > > [We need this clock thing for "drivers/net/fs_enet/mii-fec.c"...] > > I2C and MSCAN need it as well. What about implementing the more generic > clk api for the MPC5200 as done for the MPC512x? > http://lxr.linux.no/linux+v2.6.29/arch/powerpc/platforms/512x/clock.c Hmmm... I have to admit that I'm biased he. My primary interest at the moment is obviously just to get the MPC512x stuff into mainline, and thus I'm not happy about extending the scope of the isse to other processors. From the technical point of view you are right, but I have to admit that I don't see which direction we should go from here. If we keep in mind that the same FEC core is also used in various i.MX platforms (= ARM processors), we probably cannot come up with a clean, unique implementation that covers all uses. That's why I asked Grant how to address this. I also added Kumar to the Cc: list - mayby he has some ideas as well? Best regards, Wolfgang Denk
On Sun, Jun 7, 2009 at 2:34 PM, Wolfgang Denk<wd@denx.de> wrote: > Dear John, > > in message <4b73d43f0906061708o763409d0u10a344dfc30e32c1@mail.gmail.com> you wrote: >> >> > The big question seems to be what the RefMan means when talking about >> > the "system clock frequency". Obiously it is NOT the CPU clock as > ... >> > But which one is it? >> >> My best guess is still that it is ips clock. I think I stated in a previous >> email ipb, but I meant ips. 5200 has ibp and 5121 has ips. Have you looked >> at he MII clock on a scope to see how the calculated values compare to >> actual? > > Yes, it seems very much as if you were right again. > > When using ips/ibp everything makes sense, and works. > > Hm... so that means on MPC512x we should use mpc512x_find_ips_freq(), > while on MPC5200 we should use mpc52xx_find_ipb_freq() - but hey, > apart from the name these two functions are identical. > > Grant - how would you like to see this handled? Should we merge these > two code-wise identical functions into one? What should be the name, > and in which file should we put it? If you want to merge them... Seeing as the function just walks up the parent nodes looking for the "bus-frequency" property; how about "mpc5xxx_get_bus_frequency()"? It should go somewhere in arch/powerpc/sysdev/ Alternately, it is a pretty trivial function; 52xx and 512x could both have their own versions of it. Whichever makes the most sense to you (I haven't personally looked at the device driver impact). g.
On Mon, Jun 8, 2009 at 2:19 AM, Wolfgang Denk<wd@denx.de> wrote: > Dear Wolfgang, > > In message <4A2CC1DE.5040704@grandegger.com> you wrote: >> >> > Hm... so that means on MPC512x we should use mpc512x_find_ips_freq(), >> > while on MPC5200 we should use mpc52xx_find_ipb_freq() - but hey, >> > apart from the name these two functions are identical. >> > >> > Grant - how would you like to see this handled? Should we merge these >> > two code-wise identical functions into one? What should be the name, >> > and in which file should we put it? >> > >> > [We need this clock thing for "drivers/net/fs_enet/mii-fec.c"...] >> >> I2C and MSCAN need it as well. What about implementing the more generic >> clk api for the MPC5200 as done for the MPC512x? > >> http://lxr.linux.no/linux+v2.6.29/arch/powerpc/platforms/512x/clock.c > > Hmmm... I have to admit that I'm biased he. My primary interest at > the moment is obviously just to get the MPC512x stuff into mainline, > and thus I'm not happy about extending the scope of the isse to other > processors. > > From the technical point of view you are right, but I have to admit > that I don't see which direction we should go from here. If we keep in > mind that the same FEC core is also used in various i.MX platforms (= > ARM processors), we probably cannot come up with a clean, unique > implementation that covers all uses. > > That's why I asked Grant how to address this. I also added Kumar to > the Cc: list - mayby he has some ideas as well? I say use the simple approach to get it merged; It can always be changed later. g.
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c index 9d8bd97..a51dd83 100644 --- a/drivers/net/fs_enet/mii-fec.c +++ b/drivers/net/fs_enet/mii-fec.c @@ -153,8 +153,12 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev, if (!fec->fecp) goto out_fec; +#if 0 fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; - +#else + fec->mii_speed = (((ppc_proc_freq / 1000000) / 30) + 1) << 1; + printk("## ppc_proc_freq = %d, fec->mii_speed = %d\n", ppc_proc_freq, fec->mii_speed); +#endif setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE); setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
Signed-off-by: Wolfgang Denk <wd@denx.de> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: John Rigby <jcrigby@gmail.com> --- This patch is NOT intended for inclusion into mainline, but rather a request for help. For some reason which I don't understand yet, the Ethernet interface on the ARIA board does not work in the default configuration, because MII probing fails. What I'm seeing is this; the problem is with this part of code in "drivers/net/fs_enet/mii-fec.c": 156 fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1; ... 163 out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed); I added some debug messages, and this is what I see: On the ADS5121, we have the CPU clocked at 400 MHz. I get: ... ## ppc_proc_freq = 399999996, fec->mii_speed = 160 FEC MII Bus: probed ... It works fine. According to the Ref. Man.: A value of 0 in this field turns off the MDC and leaves it in a low-voltage state. Any non-zero value results in the MDC frequency of 1/(mii_speed*2) of the system clock frequency. that means we have a MDC frequency of 400 MHz / (2 * 160) = 1.25 MHz which is obviously within the 2.5 MHz limit. Now ARIA is currently running at 316.8 MHz, and this is what I get: ... ## ppc_proc_freq = 316800000, fec->mii_speed = 128 fsl-fec-mdio: probe of 80002800.mdio failed with error -5 ... It fails. MDC frequency is 316.8 MHz / (2 * 128) = 1.24 MHz which should be fine. However, If I change the code to fec->mii_speed = (((ppc_proc_freq / 1000000) / 30) + 1) << 1; then I get: ... ## ppc_proc_freq = 316800000, fec->mii_speed = 22 FEC MII Bus: probed ... and it's working!!! However, I compute MDC frequency as 316.8 MHz / (2 * 22) = 7.2 MHz which is far above the maximum allowed clock of 2.5 MHz ??? Has anybody any idea what might be going on here? drivers/net/fs_enet/mii-fec.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)