| Message ID | 1312641270-6018-6-git-send-email-holt@sgi.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/06/2011 04:34 PM, Robin Holt wrote: > flexcan driver needs the clk_get, clk_get_rate, etc functions > to work. This patch provides the minimum functionality. This needs some more general thoughts... apart from the question where the code should go. Like for the MSCAN on the MPC5200, the user should be *able* to select an appropriate clock source and divider via DTS node properties. Currently it seems, that the DTS properties must match some pre-configured values, most likely set by the boot loader. Please correct me if I'm wrong. For me this is generic and should go into the Flexcan driver. From there, a platform specific function, e.g. flexcan_set_clock() might be called. > Signed-off-by: Robin Holt <holt@sgi.com> > To: Marc Kleine-Budde <mkl@pengutronix.de> > To: Wolfgang Grandegger <wg@grandegger.com> > To: U Bhaskar-B22300 <B22300@freescale.com> > Cc: socketcan-core@lists.berlios.de > Cc: netdev@vger.kernel.org > --- > arch/powerpc/platforms/85xx/p1010rdb.c | 78 ++++++++++++++++++++++++++++++++ > 1 files changed, 78 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c > index 3540a88..8f78ddd 100644 > --- a/arch/powerpc/platforms/85xx/p1010rdb.c > +++ b/arch/powerpc/platforms/85xx/p1010rdb.c > @@ -28,6 +28,7 @@ > #include <asm/udbg.h> > #include <asm/mpic.h> > #include <asm/swiotlb.h> > +#include <asm/clk_interface.h> > > #include <sysdev/fsl_soc.h> > #include <sysdev/fsl_pci.h> > @@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void) > printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n"); > } > > +/* > + * p1010rdb needs to provide a clock source for the flexcan driver. > + */ > +struct clk { > + unsigned long rate; > +} p1010rdb_system_clk; > + > +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id) > +{ I persoanlly don't like the > + struct clk *clk; > + u32 *of_property; > + unsigned long clock_freq, clock_divider; > + const char *dev_init_name; > + > + if (!dev) > + return ERR_PTR(-ENOENT); > + > + /* > + * The can devices are named ffe1c000.can0 and ffe1d000.can1 on > + * the p1010rdb. Check for the "can" portion of that name before > + * returning a clock source. > + */ > + dev_init_name = dev_name(dev); > + if (strlen(dev_init_name) != 13) > + return ERR_PTR(-ENOENT); > + dev_init_name += 9; > + if (strncmp(dev_init_name, "can", 3)) > + return ERR_PTR(-ENOENT); There are dedicated functions to find the of node. But with my above proposal we do not need to provide p1010_rdb_clk_get(). More comments on the other patches soon... Wolfgang. -- 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 Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: > On 08/06/2011 04:34 PM, Robin Holt wrote: > > flexcan driver needs the clk_get, clk_get_rate, etc functions > > to work. This patch provides the minimum functionality. > > This needs some more general thoughts... apart from the question where > the code should go. > > Like for the MSCAN on the MPC5200, the user should be *able* to select > an appropriate clock source and divider via DTS node properties. > Currently it seems, that the DTS properties must match some > pre-configured values, most likely set by the boot loader. Please > correct me if I'm wrong. For me this is generic and should go into the > Flexcan driver. From there, a platform specific function, e.g. > flexcan_set_clock() might be called. The P1010 really only supports the system bus clock for a source. I was wrong last week when I said it supported either. That was a confusion I have because of a task I was assigned a couple months ago. It can select different divider values. Robin > > > > Signed-off-by: Robin Holt <holt@sgi.com> > > To: Marc Kleine-Budde <mkl@pengutronix.de> > > To: Wolfgang Grandegger <wg@grandegger.com> > > To: U Bhaskar-B22300 <B22300@freescale.com> > > Cc: socketcan-core@lists.berlios.de > > Cc: netdev@vger.kernel.org > > --- > > arch/powerpc/platforms/85xx/p1010rdb.c | 78 ++++++++++++++++++++++++++++++++ > > 1 files changed, 78 insertions(+), 0 deletions(-) > > > > diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c > > index 3540a88..8f78ddd 100644 > > --- a/arch/powerpc/platforms/85xx/p1010rdb.c > > +++ b/arch/powerpc/platforms/85xx/p1010rdb.c > > @@ -28,6 +28,7 @@ > > #include <asm/udbg.h> > > #include <asm/mpic.h> > > #include <asm/swiotlb.h> > > +#include <asm/clk_interface.h> > > > > #include <sysdev/fsl_soc.h> > > #include <sysdev/fsl_pci.h> > > @@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void) > > printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n"); > > } > > > > +/* > > + * p1010rdb needs to provide a clock source for the flexcan driver. > > + */ > > +struct clk { > > + unsigned long rate; > > +} p1010rdb_system_clk; > > + > > +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id) > > +{ > > I persoanlly don't like the > > > + struct clk *clk; > > + u32 *of_property; > > + unsigned long clock_freq, clock_divider; > > + const char *dev_init_name; > > + > > + if (!dev) > > + return ERR_PTR(-ENOENT); > > + > > + /* > > + * The can devices are named ffe1c000.can0 and ffe1d000.can1 on > > + * the p1010rdb. Check for the "can" portion of that name before > > + * returning a clock source. > > + */ > > + dev_init_name = dev_name(dev); > > + if (strlen(dev_init_name) != 13) > > + return ERR_PTR(-ENOENT); > > + dev_init_name += 9; > > + if (strncmp(dev_init_name, "can", 3)) > > + return ERR_PTR(-ENOENT); > > There are dedicated functions to find the of node. But with my above > proposal we do not need to provide p1010_rdb_clk_get(). > > More comments on the other patches soon... > > Wolfgang. -- 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 Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: > On 08/06/2011 04:34 PM, Robin Holt wrote: > > flexcan driver needs the clk_get, clk_get_rate, etc functions > > to work. This patch provides the minimum functionality. > > This needs some more general thoughts... apart from the question where > the code should go. > > Like for the MSCAN on the MPC5200, the user should be *able* to select > an appropriate clock source and divider via DTS node properties. > Currently it seems, that the DTS properties must match some > pre-configured values, most likely set by the boot loader. Please > correct me if I'm wrong. For me this is generic and should go into the > Flexcan driver. From there, a platform specific function, e.g. > flexcan_set_clock() might be called. OK. Dug a bit more. The p1010 built-in clocksource seems to be the periphereal clock frequency which is system bus frequency divided by 2. The clock source can not be changed, but the clock divider can by freezing the interface and setting the CTRL register. This appears to only be done by the boot loader. I do not see why we can not leave that functionality in the boot loader and then go back to a variation on my earlier flexcan_clk_* patch. Is that close to the direction you think we should go or have I completely misunderstood your wishes? Thanks, Robin -- 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 08/08/2011 01:31 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: >> On 08/06/2011 04:34 PM, Robin Holt wrote: >>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>> to work. This patch provides the minimum functionality. >> >> This needs some more general thoughts... apart from the question where >> the code should go. >> >> Like for the MSCAN on the MPC5200, the user should be *able* to select >> an appropriate clock source and divider via DTS node properties. >> Currently it seems, that the DTS properties must match some >> pre-configured values, most likely set by the boot loader. Please >> correct me if I'm wrong. For me this is generic and should go into the >> Flexcan driver. From there, a platform specific function, e.g. >> flexcan_set_clock() might be called. > > OK. Dug a bit more. The p1010 built-in clocksource seems to be the > periphereal clock frequency which is system bus frequency divided > by 2. The clock source can not be changed, but the clock divider can > by freezing the interface and setting the CTRL register. This appears Which bit(s) in the CTRL register is/are this? Marc
On Mon, Aug 08, 2011 at 02:07:32PM +0200, Marc Kleine-Budde wrote: > On 08/08/2011 01:31 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: > >> On 08/06/2011 04:34 PM, Robin Holt wrote: > >>> flexcan driver needs the clk_get, clk_get_rate, etc functions > >>> to work. This patch provides the minimum functionality. > >> > >> This needs some more general thoughts... apart from the question where > >> the code should go. > >> > >> Like for the MSCAN on the MPC5200, the user should be *able* to select > >> an appropriate clock source and divider via DTS node properties. > >> Currently it seems, that the DTS properties must match some > >> pre-configured values, most likely set by the boot loader. Please > >> correct me if I'm wrong. For me this is generic and should go into the > >> Flexcan driver. From there, a platform specific function, e.g. > >> flexcan_set_clock() might be called. > > > > OK. Dug a bit more. The p1010 built-in clocksource seems to be the > > periphereal clock frequency which is system bus frequency divided > > by 2. The clock source can not be changed, but the clock divider can > > by freezing the interface and setting the CTRL register. This appears > > Which bit(s) in the CTRL register is/are this? PRESDIV bits 24-31. Documented on the P1010 reference manual section 21.3.3.2. Robin -- 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 08/08/2011 02:48 PM, Robin Holt wrote: >>> OK. Dug a bit more. The p1010 built-in clocksource seems to be the >>> periphereal clock frequency which is system bus frequency divided >>> by 2. The clock source can not be changed, but the clock divider can >>> by freezing the interface and setting the CTRL register. This appears >> Which bit(s) in the CTRL register is/are this? > PRESDIV bits 24-31. Documented on the P1010 reference manual section 21.3.3.2. We have and use these bits on arm, too. These bits are calculated and set by the driver[1]. These bits are touched in freescale's 2.6.35 version of the flexcan driver, too. From my point of view it makes no sense to specify this divider in the OF-tree. We just need the frequency of the clock entering the flexcan core. Which is, as you say, the system bus freq/2. [1] http://lxr.linux.no/linux+v3.0.1/drivers/net/can/flexcan.c#L597
On 08/08/2011 01:31 PM, Robin Holt wrote: > OK. Dug a bit more. The p1010 built-in clocksource seems to be the > periphereal clock frequency which is system bus frequency divided > by 2. The clock source can not be changed, but the clock divider can On arm the clock source is selected by bit 13 of CTRL: > This bit selects the clock source to the CAN protocol interface (CPI) > to be either the peripheral clock (driven by the PLL) or the crystal > oscillator clock. The selected clock is the one fed to the prescaler > to generate the SCLK (SCLK). In order to guarantee reliable > operation, this bit must only be changed while the module is in > disable mode. See Section 24.4.8.4, “Protocol Timing,” for more > information. > 0 The CAN engine clock source is the oscillator clock (24.576 MHz) > 1 The CAN engine clock source is the bus clock (66.5 MHz) cheers, Marc
On 08/08/2011 01:31 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: >> On 08/06/2011 04:34 PM, Robin Holt wrote: >>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>> to work. This patch provides the minimum functionality. >> >> This needs some more general thoughts... apart from the question where >> the code should go. >> >> Like for the MSCAN on the MPC5200, the user should be *able* to select >> an appropriate clock source and divider via DTS node properties. >> Currently it seems, that the DTS properties must match some >> pre-configured values, most likely set by the boot loader. Please >> correct me if I'm wrong. For me this is generic and should go into the >> Flexcan driver. From there, a platform specific function, e.g. >> flexcan_set_clock() might be called. > > OK. Dug a bit more. The p1010 built-in clocksource seems to be the > periphereal clock frequency which is system bus frequency divided > by 2. The clock source can not be changed, but the clock divider can > by freezing the interface and setting the CTRL register. This appears > to only be done by the boot loader. I do not see why we can not leave And likely Freescale's bootloader does also fixup the DTS Flexcan node. Ah, oh, there's already someting in the mainline U-BOOT: commit 65bb8b060a873fa4f5188f2951081f6011259614 Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> Date: Fri Mar 4 20:27:58 2011 +0530 powerpc/85xx: Fix up clock_freq property in CAN node of dts Fix up the device tree property associated with the Flexcan clock frequency. This property is used to calculate the bit timing parameters for Flexcan. Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> > that functionality in the boot loader and then go back to a variation > on my earlier flexcan_clk_* patch. Is that close to the direction you > think we should go or have I completely misunderstood your wishes? The boot loader might not chose the optimum clock source and frequency, which might even be application dependent. Therefore it would be nice to allow the user to change it if necessary. Some CAN interfaces do even allow to use an external clock source. The main question is where we add that functionality. As more as I think of it, the clock interface would not be that bad, especially if it's available. Furthermore, if the bootloader sets the clock source and divider, we do not need device tree properties for it. A simply register lookup would reveal what values are used. We may just need the input clock source. Wolfgang. -- 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 08/08/2011 03:00 PM, Marc Kleine-Budde wrote: > On 08/08/2011 02:48 PM, Robin Holt wrote: >>>> OK. Dug a bit more. The p1010 built-in clocksource seems to be the >>>> periphereal clock frequency which is system bus frequency divided >>>> by 2. The clock source can not be changed, but the clock divider can >>>> by freezing the interface and setting the CTRL register. This appears > >>> Which bit(s) in the CTRL register is/are this? > >> PRESDIV bits 24-31. Documented on the P1010 reference manual section 21.3.3.2. > > We have and use these bits on arm, too. These bits are calculated and > set by the driver[1]. These bits are touched in freescale's 2.6.35 > version of the flexcan driver, too. OK, now it starts making sense. And these bits are set according to relevant DTS properties, I assume. And the input frequency is derived from the "clock_freq" property entered by the U-Boot bootloader. > From my point of view it makes no sense to specify this divider in the > OF-tree. We just need the frequency of the clock entering the flexcan > core. Which is, as you say, the system bus freq/2. > > [1] > http://lxr.linux.no/linux+v3.0.1/drivers/net/can/flexcan.c#L597 Yep, I just wrote a similar comment in my previous mail. So, of we want to allow the user to define the clock source and divider, we should provide an interface for both, ARM and PPC. Wolfgang. -- 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 08/08/2011 03:08 PM, Wolfgang Grandegger wrote: > On 08/08/2011 01:31 PM, Robin Holt wrote: >> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: >>> On 08/06/2011 04:34 PM, Robin Holt wrote: >>>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>>> to work. This patch provides the minimum functionality. >>> >>> This needs some more general thoughts... apart from the question where >>> the code should go. >>> >>> Like for the MSCAN on the MPC5200, the user should be *able* to select >>> an appropriate clock source and divider via DTS node properties. >>> Currently it seems, that the DTS properties must match some >>> pre-configured values, most likely set by the boot loader. Please >>> correct me if I'm wrong. For me this is generic and should go into the >>> Flexcan driver. From there, a platform specific function, e.g. >>> flexcan_set_clock() might be called. >> >> OK. Dug a bit more. The p1010 built-in clocksource seems to be the >> periphereal clock frequency which is system bus frequency divided >> by 2. The clock source can not be changed, but the clock divider can >> by freezing the interface and setting the CTRL register. This appears >> to only be done by the boot loader. I do not see why we can not leave > > And likely Freescale's bootloader does also fixup the DTS Flexcan node. > Ah, oh, there's already someting in the mainline U-BOOT: > > commit 65bb8b060a873fa4f5188f2951081f6011259614 > Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > Date: Fri Mar 4 20:27:58 2011 +0530 > > powerpc/85xx: Fix up clock_freq property in CAN node of dts > > Fix up the device tree property associated with the Flexcan clock > frequency. This property is used to calculate the bit timing parameters > for Flexcan. > > Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > Signed-off-by: Kumar Gala <galak@kernel.crashing.org> > > >> that functionality in the boot loader and then go back to a variation >> on my earlier flexcan_clk_* patch. Is that close to the direction you >> think we should go or have I completely misunderstood your wishes? > > The boot loader might not chose the optimum clock source and frequency, > which might even be application dependent. Therefore it would be nice to > allow the user to change it if necessary. Some CAN interfaces do even > allow to use an external clock source. The main question is where we add > that functionality. As more as I think of it, the clock interface would > not be that bad, especially if it's available. > > Furthermore, if the bootloader sets the clock source and divider, we do > not need device tree properties for it. A simply register lookup would > reveal what values are used. We may just need the input clock source. If the bootloader touches the divider _in_ the flexcan core, that would make absolutely no sense. The clock divider in the flexcan core (in the CTRL register) is the bitrate pre-scaler calculated by the bit-timing algorithm. What we need in the device tree is, from my point of view. a) the used clock source (bus clock or xtal clock) b) the frequency of that clock These problems are solved on arm via: a) bus clock is hard coded [1] b) get that clock frequency via clk_get_rate(). Marc [1] I just talked to Sascha (the i.mx maintainer), there's no support for the xtal clock, which is the OSC_AUDIO on mx35, in the i.mx clock framework so far.
> commit 65bb8b060a873fa4f5188f2951081f6011259614 > Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > Date: Fri Mar 4 20:27:58 2011 +0530 On a side note, that commit fixes up "fsl,flexcan-v1.0" ... + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", + "clock_freq", gd->bus_clk, 1); Should I go back to flexcan-v1.0 in my patches? Robin -- 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 Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote: > On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote: > > On 08/08/2011 01:31 PM, Robin Holt wrote: > >> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: > >>> On 08/06/2011 04:34 PM, Robin Holt wrote: > >>>> flexcan driver needs the clk_get, clk_get_rate, etc functions > >>>> to work. This patch provides the minimum functionality. > >>> > >>> This needs some more general thoughts... apart from the question where > >>> the code should go. > >>> > >>> Like for the MSCAN on the MPC5200, the user should be *able* to select > >>> an appropriate clock source and divider via DTS node properties. > >>> Currently it seems, that the DTS properties must match some > >>> pre-configured values, most likely set by the boot loader. Please > >>> correct me if I'm wrong. For me this is generic and should go into the > >>> Flexcan driver. From there, a platform specific function, e.g. > >>> flexcan_set_clock() might be called. > >> > >> OK. Dug a bit more. The p1010 built-in clocksource seems to be the > >> periphereal clock frequency which is system bus frequency divided > >> by 2. The clock source can not be changed, but the clock divider can > >> by freezing the interface and setting the CTRL register. This appears > >> to only be done by the boot loader. I do not see why we can not leave > > > > And likely Freescale's bootloader does also fixup the DTS Flexcan node. > > Ah, oh, there's already someting in the mainline U-BOOT: > > > > commit 65bb8b060a873fa4f5188f2951081f6011259614 > > Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > > Date: Fri Mar 4 20:27:58 2011 +0530 > > > > powerpc/85xx: Fix up clock_freq property in CAN node of dts > > > > Fix up the device tree property associated with the Flexcan clock > > frequency. This property is used to calculate the bit timing parameters > > for Flexcan. > > > > Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > > Signed-off-by: Kumar Gala <galak@kernel.crashing.org> > > > > > >> that functionality in the boot loader and then go back to a variation > >> on my earlier flexcan_clk_* patch. Is that close to the direction you > >> think we should go or have I completely misunderstood your wishes? > > > > The boot loader might not chose the optimum clock source and frequency, > > which might even be application dependent. Therefore it would be nice to > > allow the user to change it if necessary. Some CAN interfaces do even > > allow to use an external clock source. The main question is where we add > > that functionality. As more as I think of it, the clock interface would > > not be that bad, especially if it's available. > > > > Furthermore, if the bootloader sets the clock source and divider, we do > > not need device tree properties for it. A simply register lookup would > > reveal what values are used. We may just need the input clock source. > > If the bootloader touches the divider _in_ the flexcan core, that would > make absolutely no sense. The clock divider in the flexcan core (in the > CTRL register) is the bitrate pre-scaler calculated by the bit-timing > algorithm. > > What we need in the device tree is, from my point of view. > a) the used clock source (bus clock or xtal clock) > b) the frequency of that clock > > These problems are solved on arm via: > a) bus clock is hard coded [1] > b) get that clock frequency via clk_get_rate(). Just to make sure I understand correctly, the clk_get_rate() return value comes from the device tree and a mach specific handler, right? And 'mach-specific' really means what, a processor family? Thanks, Robin -- 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 08/08/2011 03:56 PM, Robin Holt wrote: >> commit 65bb8b060a873fa4f5188f2951081f6011259614 >> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >> Date: Fri Mar 4 20:27:58 2011 +0530 > > On a side note, that commit fixes up "fsl,flexcan-v1.0" > ... > + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > + "clock_freq", gd->bus_clk, 1); > > Should I go back to flexcan-v1.0 in my patches? Well, no. Let's wait. I don't think we need it. Also, it sets "clock_freq" while http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt documents "clock-frequencies"... :-(. Wolfgang, -- 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 08/08/2011 03:44 PM, Marc Kleine-Budde wrote: > On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote: >> On 08/08/2011 01:31 PM, Robin Holt wrote: >>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: >>>> On 08/06/2011 04:34 PM, Robin Holt wrote: >>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>>>> to work. This patch provides the minimum functionality. >>>> >>>> This needs some more general thoughts... apart from the question where >>>> the code should go. >>>> >>>> Like for the MSCAN on the MPC5200, the user should be *able* to select >>>> an appropriate clock source and divider via DTS node properties. >>>> Currently it seems, that the DTS properties must match some >>>> pre-configured values, most likely set by the boot loader. Please >>>> correct me if I'm wrong. For me this is generic and should go into the >>>> Flexcan driver. From there, a platform specific function, e.g. >>>> flexcan_set_clock() might be called. >>> >>> OK. Dug a bit more. The p1010 built-in clocksource seems to be the >>> periphereal clock frequency which is system bus frequency divided >>> by 2. The clock source can not be changed, but the clock divider can >>> by freezing the interface and setting the CTRL register. This appears >>> to only be done by the boot loader. I do not see why we can not leave >> >> And likely Freescale's bootloader does also fixup the DTS Flexcan node. >> Ah, oh, there's already someting in the mainline U-BOOT: >> >> commit 65bb8b060a873fa4f5188f2951081f6011259614 >> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >> Date: Fri Mar 4 20:27:58 2011 +0530 >> >> powerpc/85xx: Fix up clock_freq property in CAN node of dts >> >> Fix up the device tree property associated with the Flexcan clock >> frequency. This property is used to calculate the bit timing parameters >> for Flexcan. >> >> Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> >> >> >>> that functionality in the boot loader and then go back to a variation >>> on my earlier flexcan_clk_* patch. Is that close to the direction you >>> think we should go or have I completely misunderstood your wishes? >> >> The boot loader might not chose the optimum clock source and frequency, >> which might even be application dependent. Therefore it would be nice to >> allow the user to change it if necessary. Some CAN interfaces do even >> allow to use an external clock source. The main question is where we add >> that functionality. As more as I think of it, the clock interface would >> not be that bad, especially if it's available. >> >> Furthermore, if the bootloader sets the clock source and divider, we do >> not need device tree properties for it. A simply register lookup would >> reveal what values are used. We may just need the input clock source. > > If the bootloader touches the divider _in_ the flexcan core, that would > make absolutely no sense. The clock divider in the flexcan core (in the > CTRL register) is the bitrate pre-scaler calculated by the bit-timing > algorithm. Right, as I realized in the meantime. I'm still looking for some special p1010 registers for the divider. Unfortunately, the manual is only available under NDA :-(. > What we need in the device tree is, from my point of view. > a) the used clock source (bus clock or xtal clock) > b) the frequency of that clock Yes, and maybe an additional divider, like available for the MPC512x: http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/mpc5xxx-mscan.txt http://lxr.linux.no/#linux+v3.0.1/drivers/net/can/mscan/mpc5xxx_can.c#L132 Here is documented what you can expect from the PowerPC SOCs: http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt And there they also speak an *additional" clock divider. Maybe that forseen for the next generations. U Bhaska, could you clarify that? Thanks? > These problems are solved on arm via: > a) bus clock is hard coded [1] > b) get that clock frequency via clk_get_rate(). OK. The clk interface is fine and it should derive the frequency from the relevant register settings and the bus clock frequency. > Marc > > [1] I just talked to Sascha (the i.mx maintainer), there's no support > for the xtal clock, which is the OSC_AUDIO on mx35, in the i.mx clock > framework so far. OK. We may want to provide an interface to select taht sometimes later, also because the P1010 does only support *one* clock source. Wolfgang. -- 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 08/08/2011 04:03 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote: >> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote: >>> On 08/08/2011 01:31 PM, Robin Holt wrote: >>>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: >>>>> On 08/06/2011 04:34 PM, Robin Holt wrote: >>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>>>>> to work. This patch provides the minimum functionality. >>>>> >>>>> This needs some more general thoughts... apart from the question where >>>>> the code should go. >>>>> >>>>> Like for the MSCAN on the MPC5200, the user should be *able* to select >>>>> an appropriate clock source and divider via DTS node properties. >>>>> Currently it seems, that the DTS properties must match some >>>>> pre-configured values, most likely set by the boot loader. Please >>>>> correct me if I'm wrong. For me this is generic and should go into the >>>>> Flexcan driver. From there, a platform specific function, e.g. >>>>> flexcan_set_clock() might be called. >>>> >>>> OK. Dug a bit more. The p1010 built-in clocksource seems to be the >>>> periphereal clock frequency which is system bus frequency divided >>>> by 2. The clock source can not be changed, but the clock divider can >>>> by freezing the interface and setting the CTRL register. This appears >>>> to only be done by the boot loader. I do not see why we can not leave >>> >>> And likely Freescale's bootloader does also fixup the DTS Flexcan node. >>> Ah, oh, there's already someting in the mainline U-BOOT: >>> >>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>> Date: Fri Mar 4 20:27:58 2011 +0530 >>> >>> powerpc/85xx: Fix up clock_freq property in CAN node of dts >>> >>> Fix up the device tree property associated with the Flexcan clock >>> frequency. This property is used to calculate the bit timing parameters >>> for Flexcan. >>> >>> Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> >>> >>> >>>> that functionality in the boot loader and then go back to a variation >>>> on my earlier flexcan_clk_* patch. Is that close to the direction you >>>> think we should go or have I completely misunderstood your wishes? >>> >>> The boot loader might not chose the optimum clock source and frequency, >>> which might even be application dependent. Therefore it would be nice to >>> allow the user to change it if necessary. Some CAN interfaces do even >>> allow to use an external clock source. The main question is where we add >>> that functionality. As more as I think of it, the clock interface would >>> not be that bad, especially if it's available. >>> >>> Furthermore, if the bootloader sets the clock source and divider, we do >>> not need device tree properties for it. A simply register lookup would >>> reveal what values are used. We may just need the input clock source. >> >> If the bootloader touches the divider _in_ the flexcan core, that would >> make absolutely no sense. The clock divider in the flexcan core (in the >> CTRL register) is the bitrate pre-scaler calculated by the bit-timing >> algorithm. >> >> What we need in the device tree is, from my point of view. >> a) the used clock source (bus clock or xtal clock) >> b) the frequency of that clock >> >> These problems are solved on arm via: >> a) bus clock is hard coded [1] >> b) get that clock frequency via clk_get_rate(). > > Just to make sure I understand correctly, the clk_get_rate() return > value comes from the device tree and a mach specific handler, right? > And 'mach-specific' really means what, a processor family? I'm talking about the mainline driver, that has no device tree support. The clock stuff on arm currently goes like this: The driver asks for the clock related to the device. The architecture code has previously connected the flexcan device to an arch specific (i.mx25, i.mx35) clock. That clock is returned. Enable/disable/get_rate are working on that specific clock. hope that helps, Marc
On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 03:56 PM, Robin Holt wrote: > >> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >> Date: Fri Mar 4 20:27:58 2011 +0530 > > > > On a side note, that commit fixes up "fsl,flexcan-v1.0" > > ... > > + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > > + "clock_freq", gd->bus_clk, 1); > > > > Should I go back to flexcan-v1.0 in my patches? > > Well, no. Let's wait. I don't think we need it. Also, it sets > "clock_freq" while > > http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > documents "clock-frequencies"... :-(. You answered a different question that I was asking. I was asking if I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on line 5. The clock_freq looks like a uboot change will need to be made as well. Robin -- 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 Mon, Aug 08, 2011 at 04:19:20PM +0200, Marc Kleine-Budde wrote: > On 08/08/2011 04:03 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 03:44:36PM +0200, Marc Kleine-Budde wrote: > >> On 08/08/2011 03:08 PM, Wolfgang Grandegger wrote: > >>> On 08/08/2011 01:31 PM, Robin Holt wrote: > >>>> On Mon, Aug 08, 2011 at 10:37:58AM +0200, Wolfgang Grandegger wrote: > >>>>> On 08/06/2011 04:34 PM, Robin Holt wrote: > >>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions > >>>>>> to work. This patch provides the minimum functionality. > >>>>> > >>>>> This needs some more general thoughts... apart from the question where > >>>>> the code should go. > >>>>> > >>>>> Like for the MSCAN on the MPC5200, the user should be *able* to select > >>>>> an appropriate clock source and divider via DTS node properties. > >>>>> Currently it seems, that the DTS properties must match some > >>>>> pre-configured values, most likely set by the boot loader. Please > >>>>> correct me if I'm wrong. For me this is generic and should go into the > >>>>> Flexcan driver. From there, a platform specific function, e.g. > >>>>> flexcan_set_clock() might be called. > >>>> > >>>> OK. Dug a bit more. The p1010 built-in clocksource seems to be the > >>>> periphereal clock frequency which is system bus frequency divided > >>>> by 2. The clock source can not be changed, but the clock divider can > >>>> by freezing the interface and setting the CTRL register. This appears > >>>> to only be done by the boot loader. I do not see why we can not leave > >>> > >>> And likely Freescale's bootloader does also fixup the DTS Flexcan node. > >>> Ah, oh, there's already someting in the mainline U-BOOT: > >>> > >>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>> > >>> powerpc/85xx: Fix up clock_freq property in CAN node of dts > >>> > >>> Fix up the device tree property associated with the Flexcan clock > >>> frequency. This property is used to calculate the bit timing parameters > >>> for Flexcan. > >>> > >>> Signed-off-by: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org> > >>> > >>> > >>>> that functionality in the boot loader and then go back to a variation > >>>> on my earlier flexcan_clk_* patch. Is that close to the direction you > >>>> think we should go or have I completely misunderstood your wishes? > >>> > >>> The boot loader might not chose the optimum clock source and frequency, > >>> which might even be application dependent. Therefore it would be nice to > >>> allow the user to change it if necessary. Some CAN interfaces do even > >>> allow to use an external clock source. The main question is where we add > >>> that functionality. As more as I think of it, the clock interface would > >>> not be that bad, especially if it's available. > >>> > >>> Furthermore, if the bootloader sets the clock source and divider, we do > >>> not need device tree properties for it. A simply register lookup would > >>> reveal what values are used. We may just need the input clock source. > >> > >> If the bootloader touches the divider _in_ the flexcan core, that would > >> make absolutely no sense. The clock divider in the flexcan core (in the > >> CTRL register) is the bitrate pre-scaler calculated by the bit-timing > >> algorithm. > >> > >> What we need in the device tree is, from my point of view. > >> a) the used clock source (bus clock or xtal clock) > >> b) the frequency of that clock > >> > >> These problems are solved on arm via: > >> a) bus clock is hard coded [1] > >> b) get that clock frequency via clk_get_rate(). > > > > Just to make sure I understand correctly, the clk_get_rate() return > > value comes from the device tree and a mach specific handler, right? > > And 'mach-specific' really means what, a processor family? > > I'm talking about the mainline driver, that has no device tree support. > The clock stuff on arm currently goes like this: What is the difference between device tree support and the clkdev based clock sources using of_match to find a clock source for a particular device. It looks to me like those are filled in based upon device tree information, but I _TRULY_ do not know what I am talking about. > The driver asks for the clock related to the device. The architecture > code has previously connected the flexcan device to an arch specific > (i.mx25, i.mx35) clock. That clock is returned. Enable/disable/get_rate > are working on that specific clock. I will go and study that some more. I did my cross-compile using mxs_defconfig. Is there a better config I should be using? I typically compile a kernel with the drivers I desire and then build my cscope database using the files used in that build. Thanks, Robin -- 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 08/08/2011 04:21 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>> >>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>> ... >>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>> + "clock_freq", gd->bus_clk, 1); >>> >>> Should I go back to flexcan-v1.0 in my patches? >> >> Well, no. Let's wait. I don't think we need it. Also, it sets >> "clock_freq" while >> >> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >> >> documents "clock-frequencies"... :-(. > > You answered a different question that I was asking. I was asking if > I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > line 5. The clock_freq looks like a uboot change will need to be made > as well. Well, I wrote above: "Well, no. Let's wait. I don't think we need it." For the P1010 we can sinmply derive the clock frequency from "fsl_get_sys_freq()", which is fine for the time being. No extra properties, etc. The clk implemetation might go into http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c or http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c And may depend on HAVE_CAN_FLEXCAN BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are you using? Wolfgang. -- 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 Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 04:21 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>> > >>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>> ... > >>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>> + "clock_freq", gd->bus_clk, 1); > >>> > >>> Should I go back to flexcan-v1.0 in my patches? > >> > >> Well, no. Let's wait. I don't think we need it. Also, it sets > >> "clock_freq" while > >> > >> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >> > >> documents "clock-frequencies"... :-(. > > > > You answered a different question that I was asking. I was asking if > > I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > > line 5. The clock_freq looks like a uboot change will need to be made > > as well. > > Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > > For the P1010 we can sinmply derive the clock frequency from > "fsl_get_sys_freq()", which is fine for the time being. No extra > properties, etc. The clk implemetation might go into > > http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > > or > > http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > > And may depend on HAVE_CAN_FLEXCAN > > BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > you using? I am starting with the v3.0 kernel, apply one patch from the freescale BSP we receive under NDA which introduces the P1010RDB board into the QorIQ platform, and then work from there for the flexcan stuff. That patch introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured that Kconfig bit, so I have tweaked it to be selected automatically when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN selection to determine is we are going to build the flexcan.c file. Our contact with Freescale would prefer that I not post that patch until we get the OK from freescale to do so since we received it under NDA. Robin > > Wolfgang. -- 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 Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 04:21 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>> > >>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>> ... > >>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>> + "clock_freq", gd->bus_clk, 1); > >>> > >>> Should I go back to flexcan-v1.0 in my patches? > >> > >> Well, no. Let's wait. I don't think we need it. Also, it sets > >> "clock_freq" while > >> > >> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >> > >> documents "clock-frequencies"... :-(. > > > > You answered a different question that I was asking. I was asking if > > I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > > line 5. The clock_freq looks like a uboot change will need to be made > > as well. > > Well, I wrote above: "Well, no. Let's wait. I don't think we need it." My question remains "What should we be naming the device tree node in general. Line 5 of the fsl-flexcan.txt file specifically calls the node "fsl,flexcan-v1.0" In the .dts file the freescale patches introduces into the arch/powerpc portion of the kernel, they call it that same thing. Likewise, in the code already checked into uboot it is the same name. Whether it is needed or not for the clock frequency, it does need to be consistent between the .dts file and the driver for device discovery to work. Thanks, Robin -- 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 08/08/2011 04:44 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 04:21 PM, Robin Holt wrote: >>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>> >>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>> ... >>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>> + "clock_freq", gd->bus_clk, 1); >>>>> >>>>> Should I go back to flexcan-v1.0 in my patches? >>>> >>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>> "clock_freq" while >>>> >>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>> >>>> documents "clock-frequencies"... :-(. >>> >>> You answered a different question that I was asking. I was asking if >>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>> line 5. The clock_freq looks like a uboot change will need to be made >>> as well. >> >> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >> >> For the P1010 we can sinmply derive the clock frequency from >> "fsl_get_sys_freq()", which is fine for the time being. No extra >> properties, etc. The clk implemetation might go into >> >> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >> >> or >> >> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >> >> And may depend on HAVE_CAN_FLEXCAN >> >> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >> you using? > > I am starting with the v3.0 kernel, apply one patch from the freescale BSP > we receive under NDA which introduces the P1010RDB board into the QorIQ > platform, and then work from there for the flexcan stuff. That patch > introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > that Kconfig bit, so I have tweaked it to be selected automatically > when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > selection to determine is we are going to build the flexcan.c file. ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why we should do it differently for PowerPC. For mainline inclusion, you should provide your patches against the David Millers "net-next-2.6" tree, which already seems to have support for the P1010RDB: config P1010_RDB bool "Freescale P1010RDB" select DEFAULT_UIMAGE help This option enables support for the MPC85xx RDB (P1010 RDB) board P1010RDB contains P1010Si, which provides CPU performance up to 800 MHz and 1600 DMIPS, additional functionality and faster interfaces (DDR3/3L, SATA II, and PCI Express). > Our contact with Freescale would prefer that I not post that patch until > we get the OK from freescale to do so since we received it under NDA. I don't think we currently need it. I prefer dropping and cleaning up the device tree stuff as it is not needed for the P1010 anyway. If a new processor shows up with enhanced capabilities requiring configuration via device tree, we or somebody else can provide a patch. Marc, what do you think? Wolfgang. -- 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 Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 04:44 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > >> On 08/08/2011 04:21 PM, Robin Holt wrote: > >>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >>>> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>>>> > >>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>>>> ... > >>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>>>> + "clock_freq", gd->bus_clk, 1); > >>>>> > >>>>> Should I go back to flexcan-v1.0 in my patches? > >>>> > >>>> Well, no. Let's wait. I don't think we need it. Also, it sets > >>>> "clock_freq" while > >>>> > >>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >>>> > >>>> documents "clock-frequencies"... :-(. > >>> > >>> You answered a different question that I was asking. I was asking if > >>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > >>> line 5. The clock_freq looks like a uboot change will need to be made > >>> as well. > >> > >> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > >> > >> For the P1010 we can sinmply derive the clock frequency from > >> "fsl_get_sys_freq()", which is fine for the time being. No extra > >> properties, etc. The clk implemetation might go into > >> > >> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > >> > >> or > >> > >> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > >> > >> And may depend on HAVE_CAN_FLEXCAN > >> > >> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > >> you using? > > > > I am starting with the v3.0 kernel, apply one patch from the freescale BSP > > we receive under NDA which introduces the P1010RDB board into the QorIQ > > platform, and then work from there for the flexcan stuff. That patch > > introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > > that Kconfig bit, so I have tweaked it to be selected automatically > > when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > > selection to determine is we are going to build the flexcan.c file. > > ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > we should do it differently for PowerPC. > > For mainline inclusion, you should provide your patches against the > David Millers "net-next-2.6" tree, which already seems to have support > for the P1010RDB: > > config P1010_RDB > bool "Freescale P1010RDB" > select DEFAULT_UIMAGE > help > This option enables support for the MPC85xx RDB (P1010 RDB) board > > P1010RDB contains P1010Si, which provides CPU performance up to 800 > MHz and 1600 DMIPS, additional functionality and faster interfaces > (DDR3/3L, SATA II, and PCI Express). > > > > Our contact with Freescale would prefer that I not post that patch until > > we get the OK from freescale to do so since we received it under NDA. > > I don't think we currently need it. I prefer dropping and cleaning up > the device tree stuff as it is not needed for the P1010 anyway. If a > new processor shows up with enhanced capabilities requiring > configuration via device tree, we or somebody else can provide a patch. > Marc, what do you think? I will rebase shortly and provide a newer set of patches. I do think powerpc does need the device tree support. That is how the flexcan_probe is getting called. How would you suggest I do it otherwise? Robin -- 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 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: > On 08/08/2011 04:44 PM, Robin Holt wrote: >> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>> >>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>> ... >>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>> >>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>> >>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>> "clock_freq" while >>>>> >>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>> >>>>> documents "clock-frequencies"... :-(. >>>> >>>> You answered a different question that I was asking. I was asking if >>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>> line 5. The clock_freq looks like a uboot change will need to be made >>>> as well. >>> >>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>> >>> For the P1010 we can sinmply derive the clock frequency from >>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>> properties, etc. The clk implemetation might go into >>> >>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>> >>> or >>> >>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>> >>> And may depend on HAVE_CAN_FLEXCAN >>> >>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>> you using? >> >> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >> we receive under NDA which introduces the P1010RDB board into the QorIQ >> platform, and then work from there for the flexcan stuff. That patch >> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >> that Kconfig bit, so I have tweaked it to be selected automatically >> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >> selection to determine is we are going to build the flexcan.c file. > > ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > we should do it differently for PowerPC. > > For mainline inclusion, you should provide your patches against the > David Millers "net-next-2.6" tree, which already seems to have support > for the P1010RDB: > > config P1010_RDB > bool "Freescale P1010RDB" > select DEFAULT_UIMAGE > help > This option enables support for the MPC85xx RDB (P1010 RDB) board > > P1010RDB contains P1010Si, which provides CPU performance up to 800 > MHz and 1600 DMIPS, additional functionality and faster interfaces > (DDR3/3L, SATA II, and PCI Express). > > >> Our contact with Freescale would prefer that I not post that patch until >> we get the OK from freescale to do so since we received it under NDA. > > I don't think we currently need it. I prefer dropping and cleaning up > the device tree stuff as it is not needed for the P1010 anyway. If a > new processor shows up with enhanced capabilities requiring > configuration via device tree, we or somebody else can provide a patch. > Marc, what do you think? ACK - The device tree bindings as in mainline's Documentation is a mess. If the powerpc guys are happy with a clock interfaces based approach somewhere in arch/ppc, I'm more than happy to remove: - fsl,flexcan-clock-source (not implemented, even in the fsl driver) - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or - clock-frequency / a single clock-frequency attribute Marc
On 08/08/2011 04:48 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 04:21 PM, Robin Holt wrote: ... >> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > > My question remains "What should we be naming the device tree node in > general. Line 5 of the fsl-flexcan.txt file specifically calls the node > "fsl,flexcan-v1.0" In the .dts file the freescale patches introduces into > the arch/powerpc portion of the kernel, they call it that same thing. We should provide a patch removing that doc. The version suffix does not follow the device tree convention. A proper compatibility string would be: "fsl,p1010-flexcan", "fsl,flexcan" But as the Flexcan on the P1010 is not treated differently, "fsl,flexcan" is just fine. Also, the v1.0 is only for the PowerPC SOCs (ignoring ARM). > Likewise, in the code already checked into uboot it is the same name. > Whether it is needed or not for the clock frequency, it does need to > be consistent between the .dts file and the driver for device discovery > to work. Yes, depending on what we decide we need to clean that up as well. Wolfgang. -- 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 08/08/2011 05:09 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 04:44 PM, Robin Holt wrote: >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>>> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>>> ... >>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>>> >>>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>>> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>>> "clock_freq" while >>>>>> >>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>>> >>>>>> documents "clock-frequencies"... :-(. >>>>> >>>>> You answered a different question that I was asking. I was asking if >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>>> line 5. The clock_freq looks like a uboot change will need to be made >>>>> as well. >>>> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>>> >>>> For the P1010 we can sinmply derive the clock frequency from >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>>> properties, etc. The clk implemetation might go into >>>> >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>>> >>>> or >>>> >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>>> >>>> And may depend on HAVE_CAN_FLEXCAN >>>> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>>> you using? >>> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >>> we receive under NDA which introduces the P1010RDB board into the QorIQ >>> platform, and then work from there for the flexcan stuff. That patch >>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >>> that Kconfig bit, so I have tweaked it to be selected automatically >>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >>> selection to determine is we are going to build the flexcan.c file. >> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why >> we should do it differently for PowerPC. >> >> For mainline inclusion, you should provide your patches against the >> David Millers "net-next-2.6" tree, which already seems to have support >> for the P1010RDB: >> >> config P1010_RDB >> bool "Freescale P1010RDB" >> select DEFAULT_UIMAGE >> help >> This option enables support for the MPC85xx RDB (P1010 RDB) board >> >> P1010RDB contains P1010Si, which provides CPU performance up to 800 >> MHz and 1600 DMIPS, additional functionality and faster interfaces >> (DDR3/3L, SATA II, and PCI Express). >> >> >>> Our contact with Freescale would prefer that I not post that patch until >>> we get the OK from freescale to do so since we received it under NDA. >> >> I don't think we currently need it. I prefer dropping and cleaning up >> the device tree stuff as it is not needed for the P1010 anyway. If a >> new processor shows up with enhanced capabilities requiring >> configuration via device tree, we or somebody else can provide a patch. >> Marc, what do you think? > > I will rebase shortly and provide a newer set of patches. > > I do think powerpc does need the device tree support. That is how the flexcan_probe > is getting called. How would you suggest I do it otherwise? Why do you think that? Wolfgang. -- 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 08/08/2011 05:18 PM, Wolfgang Grandegger wrote: > On 08/08/2011 05:09 PM, Robin Holt wrote: >> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote: >>> On 08/08/2011 04:44 PM, Robin Holt wrote: >>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>>>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>>>> >>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>>>> ... >>>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>>>> >>>>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>>>> >>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>>>> "clock_freq" while >>>>>>> >>>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>>>> >>>>>>> documents "clock-frequencies"... :-(. >>>>>> >>>>>> You answered a different question that I was asking. I was asking if >>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>>>> line 5. The clock_freq looks like a uboot change will need to be made >>>>>> as well. >>>>> >>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>>>> >>>>> For the P1010 we can sinmply derive the clock frequency from >>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>>>> properties, etc. The clk implemetation might go into >>>>> >>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>>>> >>>>> or >>>>> >>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>>>> >>>>> And may depend on HAVE_CAN_FLEXCAN >>>>> >>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>>>> you using? >>>> >>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >>>> we receive under NDA which introduces the P1010RDB board into the QorIQ >>>> platform, and then work from there for the flexcan stuff. That patch >>>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >>>> that Kconfig bit, so I have tweaked it to be selected automatically >>>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >>>> selection to determine is we are going to build the flexcan.c file. >>> >>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why >>> we should do it differently for PowerPC. >>> >>> For mainline inclusion, you should provide your patches against the >>> David Millers "net-next-2.6" tree, which already seems to have support >>> for the P1010RDB: >>> >>> config P1010_RDB >>> bool "Freescale P1010RDB" >>> select DEFAULT_UIMAGE >>> help >>> This option enables support for the MPC85xx RDB (P1010 RDB) board >>> >>> P1010RDB contains P1010Si, which provides CPU performance up to 800 >>> MHz and 1600 DMIPS, additional functionality and faster interfaces >>> (DDR3/3L, SATA II, and PCI Express). >>> >>> >>>> Our contact with Freescale would prefer that I not post that patch until >>>> we get the OK from freescale to do so since we received it under NDA. >>> >>> I don't think we currently need it. I prefer dropping and cleaning up >>> the device tree stuff as it is not needed for the P1010 anyway. If a >>> new processor shows up with enhanced capabilities requiring >>> configuration via device tree, we or somebody else can provide a patch. >>> Marc, what do you think? >> >> I will rebase shortly and provide a newer set of patches. >> >> I do think powerpc does need the device tree support. That is how the flexcan_probe >> is getting called. How would you suggest I do it otherwise? > > Why do you think that? To be clear. I mean we do not need the extra "fsl," properties for the clock source and divider and frequency. Wolfgang. -- 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 08/08/2011 05:18 PM, Wolfgang Grandegger wrote: >>>> Our contact with Freescale would prefer that I not post that patch until >>>> we get the OK from freescale to do so since we received it under NDA. >>> >>> I don't think we currently need it. I prefer dropping and cleaning up >>> the device tree stuff as it is not needed for the P1010 anyway. If a >>> new processor shows up with enhanced capabilities requiring >>> configuration via device tree, we or somebody else can provide a patch. >>> Marc, what do you think? >> >> I will rebase shortly and provide a newer set of patches. >> >> I do think powerpc does need the device tree support. That is how the flexcan_probe >> is getting called. How would you suggest I do it otherwise? I think Wolfgang was talking about removing the clock* attributes from the device tree, not the device tree bindings at all. Marc
On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote: > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: >> On 08/08/2011 04:44 PM, Robin Holt wrote: >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>>> >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>>> ... >>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>>> >>>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>>> >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>>> "clock_freq" while >>>>>> >>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>>> >>>>>> documents "clock-frequencies"... :-(. >>>>> >>>>> You answered a different question that I was asking. I was asking if >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>>> line 5. The clock_freq looks like a uboot change will need to be made >>>>> as well. >>>> >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>>> >>>> For the P1010 we can sinmply derive the clock frequency from >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>>> properties, etc. The clk implemetation might go into >>>> >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>>> >>>> or >>>> >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>>> >>>> And may depend on HAVE_CAN_FLEXCAN >>>> >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>>> you using? >>> >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >>> we receive under NDA which introduces the P1010RDB board into the QorIQ >>> platform, and then work from there for the flexcan stuff. That patch >>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >>> that Kconfig bit, so I have tweaked it to be selected automatically >>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >>> selection to determine is we are going to build the flexcan.c file. >> >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why >> we should do it differently for PowerPC. >> >> For mainline inclusion, you should provide your patches against the >> David Millers "net-next-2.6" tree, which already seems to have support >> for the P1010RDB: >> >> config P1010_RDB >> bool "Freescale P1010RDB" >> select DEFAULT_UIMAGE >> help >> This option enables support for the MPC85xx RDB (P1010 RDB) board >> >> P1010RDB contains P1010Si, which provides CPU performance up to 800 >> MHz and 1600 DMIPS, additional functionality and faster interfaces >> (DDR3/3L, SATA II, and PCI Express). >> >> >>> Our contact with Freescale would prefer that I not post that patch until >>> we get the OK from freescale to do so since we received it under NDA. >> >> I don't think we currently need it. I prefer dropping and cleaning up >> the device tree stuff as it is not needed for the P1010 anyway. If a >> new processor shows up with enhanced capabilities requiring >> configuration via device tree, we or somebody else can provide a patch. >> Marc, what do you think? > > ACK - The device tree bindings as in mainline's Documentation is a mess. > If the powerpc guys are happy with a clock interfaces based approach > somewhere in arch/ppc, I'm more than happy to remove: > - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > - clock-frequency / a single clock-frequency attribute In the "net-next-2.6" tree there is also: $ grep flexcan arch/powerpc/boots/dts/*.dts p1010rdb.dts: fsl,flexcan-clock-source = "platform"; p1010rdb.dts: fsl,flexcan-clock-source = "platform"; p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; p1010si.dtsi: fsl,flexcan-clock-divider = <2>; p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; p1010si.dtsi: fsl,flexcan-clock-divider = <2>; Especially the fsl,flexcan-clock-divider = <2>; might make people think, that they could set something else. Wolfgang. > Marc > > > > > _______________________________________________ > Socketcan-core mailing list > Socketcan-core@lists.berlios.de > https://lists.berlios.de/mailman/listinfo/socketcan-core -- 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 Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote: > > On 08/08/2011 05:09 PM, Robin Holt wrote: > >> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote: > >>> On 08/08/2011 04:44 PM, Robin Holt wrote: > >>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > >>>>> On 08/08/2011 04:21 PM, Robin Holt wrote: > >>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>>>>>>> > >>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>>>>>>> ... > >>>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>>>>>>> + "clock_freq", gd->bus_clk, 1); > >>>>>>>> > >>>>>>>> Should I go back to flexcan-v1.0 in my patches? > >>>>>>> > >>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets > >>>>>>> "clock_freq" while > >>>>>>> > >>>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >>>>>>> > >>>>>>> documents "clock-frequencies"... :-(. > >>>>>> > >>>>>> You answered a different question that I was asking. I was asking if > >>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > >>>>>> line 5. The clock_freq looks like a uboot change will need to be made > >>>>>> as well. > >>>>> > >>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > >>>>> > >>>>> For the P1010 we can sinmply derive the clock frequency from > >>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra > >>>>> properties, etc. The clk implemetation might go into > >>>>> > >>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > >>>>> > >>>>> or > >>>>> > >>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > >>>>> > >>>>> And may depend on HAVE_CAN_FLEXCAN > >>>>> > >>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > >>>>> you using? > >>>> > >>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP > >>>> we receive under NDA which introduces the P1010RDB board into the QorIQ > >>>> platform, and then work from there for the flexcan stuff. That patch > >>>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > >>>> that Kconfig bit, so I have tweaked it to be selected automatically > >>>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > >>>> selection to determine is we are going to build the flexcan.c file. > >>> > >>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > >>> we should do it differently for PowerPC. > >>> > >>> For mainline inclusion, you should provide your patches against the > >>> David Millers "net-next-2.6" tree, which already seems to have support > >>> for the P1010RDB: > >>> > >>> config P1010_RDB > >>> bool "Freescale P1010RDB" > >>> select DEFAULT_UIMAGE > >>> help > >>> This option enables support for the MPC85xx RDB (P1010 RDB) board > >>> > >>> P1010RDB contains P1010Si, which provides CPU performance up to 800 > >>> MHz and 1600 DMIPS, additional functionality and faster interfaces > >>> (DDR3/3L, SATA II, and PCI Express). > >>> > >>> > >>>> Our contact with Freescale would prefer that I not post that patch until > >>>> we get the OK from freescale to do so since we received it under NDA. > >>> > >>> I don't think we currently need it. I prefer dropping and cleaning up > >>> the device tree stuff as it is not needed for the P1010 anyway. If a > >>> new processor shows up with enhanced capabilities requiring > >>> configuration via device tree, we or somebody else can provide a patch. > >>> Marc, what do you think? > >> > >> I will rebase shortly and provide a newer set of patches. > >> > >> I do think powerpc does need the device tree support. That is how the flexcan_probe > >> is getting called. How would you suggest I do it otherwise? > > > > Why do you think that? > > To be clear. I mean we do not need the extra "fsl," properties for the > clock source and divider and frequency. I agree with that. The can definition in the .dts file, however, should be can0@... "fsl,flexcan" in an ideal world, correct? If that is correct, then I will make the of_match string match fsl,flexcan and update the .dts file accordingly. Thanks, Robin -- 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 08/08/2011 05:38 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 05:22:41PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 05:18 PM, Wolfgang Grandegger wrote: >>> On 08/08/2011 05:09 PM, Robin Holt wrote: >>>> On Mon, Aug 08, 2011 at 04:59:54PM +0200, Wolfgang Grandegger wrote: >>>>> On 08/08/2011 04:44 PM, Robin Holt wrote: >>>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>>>>>> >>>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>>>>>> ... >>>>>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>>>>>> >>>>>>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>>>>>> >>>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>>>>>> "clock_freq" while >>>>>>>>> >>>>>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>>>>>> >>>>>>>>> documents "clock-frequencies"... :-(. >>>>>>>> >>>>>>>> You answered a different question that I was asking. I was asking if >>>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>>>>>> line 5. The clock_freq looks like a uboot change will need to be made >>>>>>>> as well. >>>>>>> >>>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>>>>>> >>>>>>> For the P1010 we can sinmply derive the clock frequency from >>>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>>>>>> properties, etc. The clk implemetation might go into >>>>>>> >>>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>>>>>> >>>>>>> or >>>>>>> >>>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>>>>>> >>>>>>> And may depend on HAVE_CAN_FLEXCAN >>>>>>> >>>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>>>>>> you using? >>>>>> >>>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >>>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ >>>>>> platform, and then work from there for the flexcan stuff. That patch >>>>>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >>>>>> that Kconfig bit, so I have tweaked it to be selected automatically >>>>>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >>>>>> selection to determine is we are going to build the flexcan.c file. >>>>> >>>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why >>>>> we should do it differently for PowerPC. >>>>> >>>>> For mainline inclusion, you should provide your patches against the >>>>> David Millers "net-next-2.6" tree, which already seems to have support >>>>> for the P1010RDB: >>>>> >>>>> config P1010_RDB >>>>> bool "Freescale P1010RDB" >>>>> select DEFAULT_UIMAGE >>>>> help >>>>> This option enables support for the MPC85xx RDB (P1010 RDB) board >>>>> >>>>> P1010RDB contains P1010Si, which provides CPU performance up to 800 >>>>> MHz and 1600 DMIPS, additional functionality and faster interfaces >>>>> (DDR3/3L, SATA II, and PCI Express). >>>>> >>>>> >>>>>> Our contact with Freescale would prefer that I not post that patch until >>>>>> we get the OK from freescale to do so since we received it under NDA. >>>>> >>>>> I don't think we currently need it. I prefer dropping and cleaning up >>>>> the device tree stuff as it is not needed for the P1010 anyway. If a >>>>> new processor shows up with enhanced capabilities requiring >>>>> configuration via device tree, we or somebody else can provide a patch. >>>>> Marc, what do you think? >>>> >>>> I will rebase shortly and provide a newer set of patches. >>>> >>>> I do think powerpc does need the device tree support. That is how the flexcan_probe >>>> is getting called. How would you suggest I do it otherwise? >>> >>> Why do you think that? >> >> To be clear. I mean we do not need the extra "fsl," properties for the >> clock source and divider and frequency. > > I agree with that. The can definition in the .dts file, however, > should be can0@... "fsl,flexcan" in an ideal world, correct? If that No, it's normally <device-type>@<address>. > is correct, then I will make the of_match string match fsl,flexcan and > update the .dts file accordingly. As I said. For the P1010 the clock get function just needs to return "fsl_get_sys_freq()". No need to inspect the device tree. And I would provide the clk implementation in http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c or even: http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c Wolfgang. -- 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 Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote: ... > > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: > >> On 08/08/2011 04:44 PM, Robin Holt wrote: > >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > >>>> On 08/08/2011 04:21 PM, Robin Holt wrote: > >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>>>>>> > >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>>>>>> ... > >>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>>>>>> + "clock_freq", gd->bus_clk, 1); > >>>>>>> > >>>>>>> Should I go back to flexcan-v1.0 in my patches? > >>>>>> > >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets > >>>>>> "clock_freq" while > >>>>>> > >>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >>>>>> > >>>>>> documents "clock-frequencies"... :-(. > >>>>> > >>>>> You answered a different question that I was asking. I was asking if > >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > >>>>> line 5. The clock_freq looks like a uboot change will need to be made > >>>>> as well. > >>>> > >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > >>>> > >>>> For the P1010 we can sinmply derive the clock frequency from > >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra > >>>> properties, etc. The clk implemetation might go into > >>>> > >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > >>>> > >>>> or > >>>> > >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > >>>> > >>>> And may depend on HAVE_CAN_FLEXCAN > >>>> > >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > >>>> you using? > >>> > >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP > >>> we receive under NDA which introduces the P1010RDB board into the QorIQ > >>> platform, and then work from there for the flexcan stuff. That patch > >>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > >>> that Kconfig bit, so I have tweaked it to be selected automatically > >>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > >>> selection to determine is we are going to build the flexcan.c file. > >> > >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > >> we should do it differently for PowerPC. > >> > >> For mainline inclusion, you should provide your patches against the > >> David Millers "net-next-2.6" tree, which already seems to have support > >> for the P1010RDB: > >> > >> config P1010_RDB > >> bool "Freescale P1010RDB" > >> select DEFAULT_UIMAGE > >> help > >> This option enables support for the MPC85xx RDB (P1010 RDB) board > >> > >> P1010RDB contains P1010Si, which provides CPU performance up to 800 > >> MHz and 1600 DMIPS, additional functionality and faster interfaces > >> (DDR3/3L, SATA II, and PCI Express). > >> > >> > >>> Our contact with Freescale would prefer that I not post that patch until > >>> we get the OK from freescale to do so since we received it under NDA. > >> > >> I don't think we currently need it. I prefer dropping and cleaning up > >> the device tree stuff as it is not needed for the P1010 anyway. If a > >> new processor shows up with enhanced capabilities requiring > >> configuration via device tree, we or somebody else can provide a patch. > >> Marc, what do you think? > > > > ACK - The device tree bindings as in mainline's Documentation is a mess. > > If the powerpc guys are happy with a clock interfaces based approach > > somewhere in arch/ppc, I'm more than happy to remove: > > - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > > > > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > > - clock-frequency / a single clock-frequency attribute > > In the "net-next-2.6" tree there is also: > > $ grep flexcan arch/powerpc/boots/dts/*.dts > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > Especially the fsl,flexcan-clock-divider = <2>; might make people think, > that they could set something else. I am currently lost on the direction. I think I need something like: 1) Patch 1/5 removing the "#include <mach/clock.h>" stays. 2) Patch 2/5 abstracting readl/writel stays. 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays. 4) Patch 4/5 I have not been given clear direction to not do it but have not gotten a favorable response. 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c patch which determines the clock source not using the device tree information, but rather from some system register. I need more detail on how this would work for both arm and powerpc. How would I absract that or am I providing a flexcan_clk_* set of functions like I have in earlier generations of the patch set? Thanks, Robin -- 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
Ignore this. We cross in the mail. I will go back to your other thread. Robin On Mon, Aug 08, 2011 at 10:55:40AM -0500, Robin Holt wrote: > On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote: > > On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote: > ... > > > > On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: > > >> On 08/08/2011 04:44 PM, Robin Holt wrote: > > >>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > > >>>> On 08/08/2011 04:21 PM, Robin Holt wrote: > > >>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > > >>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: > > >>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > > >>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > > >>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 > > >>>>>>> > > >>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > > >>>>>>> ... > > >>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > > >>>>>>> + "clock_freq", gd->bus_clk, 1); > > >>>>>>> > > >>>>>>> Should I go back to flexcan-v1.0 in my patches? > > >>>>>> > > >>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets > > >>>>>> "clock_freq" while > > >>>>>> > > >>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > >>>>>> > > >>>>>> documents "clock-frequencies"... :-(. > > >>>>> > > >>>>> You answered a different question that I was asking. I was asking if > > >>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > > >>>>> line 5. The clock_freq looks like a uboot change will need to be made > > >>>>> as well. > > >>>> > > >>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > > >>>> > > >>>> For the P1010 we can sinmply derive the clock frequency from > > >>>> "fsl_get_sys_freq()", which is fine for the time being. No extra > > >>>> properties, etc. The clk implemetation might go into > > >>>> > > >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > > >>>> > > >>>> or > > >>>> > > >>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > > >>>> > > >>>> And may depend on HAVE_CAN_FLEXCAN > > >>>> > > >>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > > >>>> you using? > > >>> > > >>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP > > >>> we receive under NDA which introduces the P1010RDB board into the QorIQ > > >>> platform, and then work from there for the flexcan stuff. That patch > > >>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > > >>> that Kconfig bit, so I have tweaked it to be selected automatically > > >>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > > >>> selection to determine is we are going to build the flexcan.c file. > > >> > > >> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > > >> we should do it differently for PowerPC. > > >> > > >> For mainline inclusion, you should provide your patches against the > > >> David Millers "net-next-2.6" tree, which already seems to have support > > >> for the P1010RDB: > > >> > > >> config P1010_RDB > > >> bool "Freescale P1010RDB" > > >> select DEFAULT_UIMAGE > > >> help > > >> This option enables support for the MPC85xx RDB (P1010 RDB) board > > >> > > >> P1010RDB contains P1010Si, which provides CPU performance up to 800 > > >> MHz and 1600 DMIPS, additional functionality and faster interfaces > > >> (DDR3/3L, SATA II, and PCI Express). > > >> > > >> > > >>> Our contact with Freescale would prefer that I not post that patch until > > >>> we get the OK from freescale to do so since we received it under NDA. > > >> > > >> I don't think we currently need it. I prefer dropping and cleaning up > > >> the device tree stuff as it is not needed for the P1010 anyway. If a > > >> new processor shows up with enhanced capabilities requiring > > >> configuration via device tree, we or somebody else can provide a patch. > > >> Marc, what do you think? > > > > > > ACK - The device tree bindings as in mainline's Documentation is a mess. > > > If the powerpc guys are happy with a clock interfaces based approach > > > somewhere in arch/ppc, I'm more than happy to remove: > > > - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > > > > > > - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > > > - clock-frequency / a single clock-frequency attribute > > > > In the "net-next-2.6" tree there is also: > > > > $ grep flexcan arch/powerpc/boots/dts/*.dts > > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > > > Especially the fsl,flexcan-clock-divider = <2>; might make people think, > > that they could set something else. > > I am currently lost on the direction. I think I need something like: > > 1) Patch 1/5 removing the "#include <mach/clock.h>" stays. > 2) Patch 2/5 abstracting readl/writel stays. > 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays. > 4) Patch 4/5 I have not been given clear direction to not do it but have > not gotten a favorable response. > 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c > patch which determines the clock source not using the device tree > information, but rather from some system register. I need more detail > on how this would work for both arm and powerpc. How would I absract > that or am I providing a flexcan_clk_* set of functions like I have > in earlier generations of the patch set? > > Thanks, > Robin > _______________________________________________ > Socketcan-core mailing list > Socketcan-core@lists.berlios.de > https://lists.berlios.de/mailman/listinfo/socketcan-core -- 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 08/08/2011 05:55 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote: >> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote: > ... > >>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: >>>> On 08/08/2011 04:44 PM, Robin Holt wrote: >>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: >>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote: >>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: >>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: >>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 >>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> >>>>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 >>>>>>>>> >>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" >>>>>>>>> ... >>>>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", >>>>>>>>> + "clock_freq", gd->bus_clk, 1); >>>>>>>>> >>>>>>>>> Should I go back to flexcan-v1.0 in my patches? >>>>>>>> >>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets >>>>>>>> "clock_freq" while >>>>>>>> >>>>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt >>>>>>>> >>>>>>>> documents "clock-frequencies"... :-(. >>>>>>> >>>>>>> You answered a different question that I was asking. I was asking if >>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on >>>>>>> line 5. The clock_freq looks like a uboot change will need to be made >>>>>>> as well. >>>>>> >>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." >>>>>> >>>>>> For the P1010 we can sinmply derive the clock frequency from >>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra >>>>>> properties, etc. The clk implemetation might go into >>>>>> >>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c >>>>>> >>>>>> or >>>>>> >>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c >>>>>> >>>>>> And may depend on HAVE_CAN_FLEXCAN >>>>>> >>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are >>>>>> you using? >>>>> >>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP >>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ >>>>> platform, and then work from there for the flexcan stuff. That patch >>>>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured >>>>> that Kconfig bit, so I have tweaked it to be selected automatically >>>>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN >>>>> selection to determine is we are going to build the flexcan.c file. >>>> >>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why >>>> we should do it differently for PowerPC. >>>> >>>> For mainline inclusion, you should provide your patches against the >>>> David Millers "net-next-2.6" tree, which already seems to have support >>>> for the P1010RDB: >>>> >>>> config P1010_RDB >>>> bool "Freescale P1010RDB" >>>> select DEFAULT_UIMAGE >>>> help >>>> This option enables support for the MPC85xx RDB (P1010 RDB) board >>>> >>>> P1010RDB contains P1010Si, which provides CPU performance up to 800 >>>> MHz and 1600 DMIPS, additional functionality and faster interfaces >>>> (DDR3/3L, SATA II, and PCI Express). >>>> >>>> >>>>> Our contact with Freescale would prefer that I not post that patch until >>>>> we get the OK from freescale to do so since we received it under NDA. >>>> >>>> I don't think we currently need it. I prefer dropping and cleaning up >>>> the device tree stuff as it is not needed for the P1010 anyway. If a >>>> new processor shows up with enhanced capabilities requiring >>>> configuration via device tree, we or somebody else can provide a patch. >>>> Marc, what do you think? >>> >>> ACK - The device tree bindings as in mainline's Documentation is a mess. >>> If the powerpc guys are happy with a clock interfaces based approach >>> somewhere in arch/ppc, I'm more than happy to remove: >>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) >>> >>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>> - clock-frequency / a single clock-frequency attribute >> >> In the "net-next-2.6" tree there is also: >> >> $ grep flexcan arch/powerpc/boots/dts/*.dts >> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >> >> Especially the fsl,flexcan-clock-divider = <2>; might make people think, >> that they could set something else. > > I am currently lost on the direction. I think I need something like: > > 1) Patch 1/5 removing the "#include <mach/clock.h>" stays. OK. > 2) Patch 2/5 abstracting readl/writel stays. OK. > 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays. Yep. > 4) Patch 4/5 I have not been given clear direction to not do it but have > not gotten a favorable response. Please drop this one for mainline. > 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c No, I just would prefer a more general place, e.g.: http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c Furthermore you need patches to cleanup some DTS and platform files and the Documentation. Wolfgang. -- 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 Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote: > On 08/08/2011 05:55 PM, Robin Holt wrote: > > On Mon, Aug 08, 2011 at 05:33:53PM +0200, Wolfgang Grandegger wrote: > >> On 08/08/2011 05:14 PM, Marc Kleine-Budde wrote: > > ... > > > >>> On 08/08/2011 04:59 PM, Wolfgang Grandegger wrote: > >>>> On 08/08/2011 04:44 PM, Robin Holt wrote: > >>>>> On Mon, Aug 08, 2011 at 04:37:44PM +0200, Wolfgang Grandegger wrote: > >>>>>> On 08/08/2011 04:21 PM, Robin Holt wrote: > >>>>>>> On Mon, Aug 08, 2011 at 04:16:27PM +0200, Wolfgang Grandegger wrote: > >>>>>>>> On 08/08/2011 03:56 PM, Robin Holt wrote: > >>>>>>>>>> commit 65bb8b060a873fa4f5188f2951081f6011259614 > >>>>>>>>>> Author: Bhaskar Upadhaya <Bhaskar.Upadhaya@freescale.com> > >>>>>>>>>> Date: Fri Mar 4 20:27:58 2011 +0530 > >>>>>>>>> > >>>>>>>>> On a side note, that commit fixes up "fsl,flexcan-v1.0" > >>>>>>>>> ... > >>>>>>>>> + do_fixup_by_compat_u32(blob, "fsl,flexcan-v1.0", > >>>>>>>>> + "clock_freq", gd->bus_clk, 1); > >>>>>>>>> > >>>>>>>>> Should I go back to flexcan-v1.0 in my patches? > >>>>>>>> > >>>>>>>> Well, no. Let's wait. I don't think we need it. Also, it sets > >>>>>>>> "clock_freq" while > >>>>>>>> > >>>>>>>> http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > >>>>>>>> > >>>>>>>> documents "clock-frequencies"... :-(. > >>>>>>> > >>>>>>> You answered a different question that I was asking. I was asking if > >>>>>>> I should change fsl,flexcan back to fsl,flexcan-v1.0 as documented on > >>>>>>> line 5. The clock_freq looks like a uboot change will need to be made > >>>>>>> as well. > >>>>>> > >>>>>> Well, I wrote above: "Well, no. Let's wait. I don't think we need it." > >>>>>> > >>>>>> For the P1010 we can sinmply derive the clock frequency from > >>>>>> "fsl_get_sys_freq()", which is fine for the time being. No extra > >>>>>> properties, etc. The clk implemetation might go into > >>>>>> > >>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > >>>>>> > >>>>>> or > >>>>>> > >>>>>> http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/sysdev/fsl_soc.c > >>>>>> > >>>>>> And may depend on HAVE_CAN_FLEXCAN > >>>>>> > >>>>>> BTW, I have not found HAVE_CAN_FLEXCAN in your patch. What kernel are > >>>>>> you using? > >>>>> > >>>>> I am starting with the v3.0 kernel, apply one patch from the freescale BSP > >>>>> we receive under NDA which introduces the P1010RDB board into the QorIQ > >>>>> platform, and then work from there for the flexcan stuff. That patch > >>>>> introduces the HAVE_CAN_FLEXCAN. I do not like how freescale structured > >>>>> that Kconfig bit, so I have tweaked it to be selected automatically > >>>>> when P1010RDB, NET, and CAN are selected. That allows the CAN_FLEXCAN > >>>>> selection to determine is we are going to build the flexcan.c file. > >>>> > >>>> ARM boards select HAVE_CAN_FLEXCAN and I do not see a good reason why > >>>> we should do it differently for PowerPC. > >>>> > >>>> For mainline inclusion, you should provide your patches against the > >>>> David Millers "net-next-2.6" tree, which already seems to have support > >>>> for the P1010RDB: > >>>> > >>>> config P1010_RDB > >>>> bool "Freescale P1010RDB" > >>>> select DEFAULT_UIMAGE > >>>> help > >>>> This option enables support for the MPC85xx RDB (P1010 RDB) board > >>>> > >>>> P1010RDB contains P1010Si, which provides CPU performance up to 800 > >>>> MHz and 1600 DMIPS, additional functionality and faster interfaces > >>>> (DDR3/3L, SATA II, and PCI Express). > >>>> > >>>> > >>>>> Our contact with Freescale would prefer that I not post that patch until > >>>>> we get the OK from freescale to do so since we received it under NDA. > >>>> > >>>> I don't think we currently need it. I prefer dropping and cleaning up > >>>> the device tree stuff as it is not needed for the P1010 anyway. If a > >>>> new processor shows up with enhanced capabilities requiring > >>>> configuration via device tree, we or somebody else can provide a patch. > >>>> Marc, what do you think? > >>> > >>> ACK - The device tree bindings as in mainline's Documentation is a mess. > >>> If the powerpc guys are happy with a clock interfaces based approach > >>> somewhere in arch/ppc, I'm more than happy to remove: > >>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > >>> > >>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>> - clock-frequency / a single clock-frequency attribute > >> > >> In the "net-next-2.6" tree there is also: > >> > >> $ grep flexcan arch/powerpc/boots/dts/*.dts > >> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > >> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > >> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >> > >> Especially the fsl,flexcan-clock-divider = <2>; might make people think, > >> that they could set something else. > > > > I am currently lost on the direction. I think I need something like: > > > > 1) Patch 1/5 removing the "#include <mach/clock.h>" stays. > > OK. Is that an Acked-by: or not? > > > 2) Patch 2/5 abstracting readl/writel stays. > > OK. Is that an Acked-by: or not? > > > 3) Patch 3/5 of_match for ppc and the match string is "fsl,flexcan" stays. > > Yep. Done. > > > 4) Patch 4/5 I have not been given clear direction to not do it but have > > not gotten a favorable response. > > Please drop this one for mainline. Done. > > > 5) Patch 5/5 goes from being a powerpc patch back to being a flexcan.c > > No, I just would prefer a more general place, e.g.: > > http://lxr.linux.no/#linux+v3.0.1/arch/powerpc/platforms/85xx/clock.c > > Furthermore you need patches to cleanup some DTS and platform files and > the Documentation. So we would stay with the clk_* functions. I assume clk_get() would return NULL, clk_get_rate() would just return fsl_get_sys_freq() and the other functions would do nothing. Doesn't this really polute what clk_* functions are supposed to do? Aren't we making flexcan dictate a different behavior for powerpc than for the arm (and possibly other) architectures? Thanks, Robin -- 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 08/08/2011 06:08 PM, Robin Holt wrote: > On Mon, Aug 08, 2011 at 06:03:06PM +0200, Wolfgang Grandegger wrote: ... > So we would stay with the clk_* functions. I assume clk_get() would > return NULL, clk_get_rate() would just return fsl_get_sys_freq() and > the other functions would do nothing. Doesn't this really polute what > clk_* functions are supposed to do? Aren't we making flexcan dictate > a different behavior for powerpc than for the arm (and possibly other) > architectures? Well, I see it as one way to provide compatibility with the ARM port. If the PowerPC people don't like it, we can switch to something else, whatever they suggest. Wolfgang. -- 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 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >> ACK - The device tree bindings as in mainline's Documentation is a mess. >> If the powerpc guys are happy with a clock interfaces based approach >> somewhere in arch/ppc, I'm more than happy to remove: >> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) >> >> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >> - clock-frequency / a single clock-frequency attribute > > In the "net-next-2.6" tree there is also: > > $ grep flexcan arch/powerpc/boots/dts/*.dts > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > Especially the fsl,flexcan-clock-divider = <2>; might make people think, > that they could set something else. ARGH... :D Marc
> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Tuesday, August 09, 2011 12:23 AM > To: Wolfgang Grandegger > Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U Bhaskar- > B22300 > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >> ACK - The device tree bindings as in mainline's Documentation is a > mess. > >> If the powerpc guys are happy with a clock interfaces based approach > >> somewhere in arch/ppc, I'm more than happy to remove: > >> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) [Bhaskar]I have pushed the FlexCAN series of patches, It contains the usage of all the fields posted in the FlexCAN bindings at http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274 > >> > >> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >> - clock-frequency / a single clock-frequency attribute > > > > In the "net-next-2.6" tree there is also: > > > > $ grep flexcan arch/powerpc/boots/dts/*.dts > > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > > p1010rdb.dts: fsl,flexcan-clock-source = "platform"; > > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > > > Especially the fsl,flexcan-clock-divider = <2>; might make people > > think, that they could set something else. > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan clock source is the platform clock and it is CCB/2 If the "2" is misleading, the bindings can be changed or some text can be written to make the meaning of "2" Understandable , Please suggest .. > ARGH... :D > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -- 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 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>> ACK - The device tree bindings as in mainline's Documentation is a >> mess. >>>> If the powerpc guys are happy with a clock interfaces based approach >>>> somewhere in arch/ppc, I'm more than happy to remove: >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the > usage of all the fields posted in the FlexCAN bindings at > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274 I've commented the patches. They are in a very bad shape. Please test Robin's patches. >>>> >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>>> - clock-frequency / a single clock-frequency attribute >>> >>> In the "net-next-2.6" tree there is also: >>> >>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >>> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>> >>> Especially the fsl,flexcan-clock-divider = <2>; might make people >>> think, that they could set something else. >> > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan > clock source is the platform clock and it is CCB/2 If the "2" is > misleading, the bindings can be changed or some text can be written > to make the meaning of "2" Understandable , Please suggest .. The clock devider is crap. Why not specify the clockrate that goes into the flexcan core? cheers, Marc
Hi Bhaskar, On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > > >> -----Original Message----- >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >> Sent: Tuesday, August 09, 2011 12:23 AM >> To: Wolfgang Grandegger >> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U Bhaskar- >> B22300 >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >> >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>> ACK - The device tree bindings as in mainline's Documentation is a >> mess. >>>> If the powerpc guys are happy with a clock interfaces based approach >>>> somewhere in arch/ppc, I'm more than happy to remove: >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl driver) > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the usage of all the fields posted in the FlexCAN bindings at > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blob;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cbebdc8274 As Marc already pointed out, Robin already has a much more advanced patch stack in preparation. Especially your patches do not care about the already existing Flexcan core on the Freescale's ARM socks. >>>> >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>>> - clock-frequency / a single clock-frequency attribute >>> >>> In the "net-next-2.6" tree there is also: >>> >>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >>> p1010rdb.dts: fsl,flexcan-clock-source = "platform"; >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>> >>> Especially the fsl,flexcan-clock-divider = <2>; might make people >>> think, that they could set something else. >> > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of fsl,flexcan-clock-divider = <2>; > But I kept it as "2" because FlexCan clock source is the platform clock and it is CCB/2 > If the "2" is misleading, the bindings can be changed or some text can be written to make the meaning of "2" > Understandable , Please suggest .. The clock source and frequency is fixed. Why do we need an extra properties for that. We have panned to remove these bogus bindings from the Linux kernel, which sneaked in *without* any review on the relevant mailing lists (at least I have not realized any posting). We do not think they are really needed. They just confuse the user. We also prefer to use the compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". It's unusual to add a version number, which is for the Flexcan on the PowerPC cores only, I assume, but there will be device tree for ARM soon. A proper compatibility string would be "fsl,p1010-flexcan" if we really need to distinguish. Please join the discussion on the mailing list helping to get this driver mainline. Thanks, Wolfgang. -- 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
> -----Original Message----- > From: Wolfgang Grandegger [mailto:wg@grandegger.com] > Sent: Tuesday, August 09, 2011 2:03 PM > To: U Bhaskar-B22300 > Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > Hi Bhaskar, > > On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > > > > > >> -----Original Message----- > >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > >> Sent: Tuesday, August 09, 2011 12:23 AM > >> To: Wolfgang Grandegger > >> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > >> Bhaskar- B22300 > >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >> > >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>> ACK - The device tree bindings as in mainline's Documentation is a > >> mess. > >>>> If the powerpc guys are happy with a clock interfaces based > >>>> approach somewhere in arch/ppc, I'm more than happy to remove: > >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>> driver) > > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the > > usage of all the fields posted in the FlexCAN bindings at > > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo > > b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f > > 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb > > ebdc8274 > > As Marc already pointed out, Robin already has a much more advanced patch > stack in preparation. Especially your patches do not care about the > already existing Flexcan core on the Freescale's ARM socks. [Bhaskar] No, the patches are taking care of the existing ARM functionality. I have not tested on the ARM based board, but the patches are made in a Manner that it should not break the ARM based functionality. > > >>>> > >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>>> - clock-frequency / a single clock-frequency attribute > >>> > >>> In the "net-next-2.6" tree there is also: > >>> > >>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>> p1010rdb.dts: fsl,flexcan-clock-source = > "platform"; > >>> p1010rdb.dts: fsl,flexcan-clock-source = > "platform"; > >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>> > >>> Especially the fsl,flexcan-clock-divider = <2>; might make people > >>> think, that they could set something else. > >> > > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > fsl,flexcan-clock-divider = <2>; > > But I kept it as "2" because FlexCan clock source is the > platform clock and it is CCB/2 > > If the "2" is misleading, the bindings can be changed or some > text can be written to make the meaning of "2" > > Understandable , Please suggest .. > > The clock source and frequency is fixed. Why do we need an extra > properties for that. We have panned to remove these bogus bindings from > the Linux kernel, which sneaked in *without* any review on the relevant > mailing lists (at least I have not realized any posting). We do not think > they are really needed. They just confuse the user. We also prefer to use > the compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". It's > unusual to add a version number, which is for the Flexcan on the PowerPC > cores only, I assume, but there will be device tree for ARM soon. A > proper compatibility string would be "fsl,p1010-flexcan" if we really > need to distinguish. > [Bhaskar] About clock source.. There can be two sources of clock for the CAN. Oscillator or the platform clock, but at present only platform clock is supported in P1010.If we remove the fsl,flexcan-clock-source property, we will lost the flexibility of changing the clock source .. About clock-frequency... it is also not fixed. It depends on the platform clock which in turns Depends on the CCB clock. So it will be better to keep clock-frequency property which is getting fixed via u-boot. Agreed on the discussion of changing "fsl,flexcan-v1.0" to "fsl,flexcan" > Please join the discussion on the mailing list helping to get this driver > mainline. > > Thanks, > > Wolfgang. -- 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
> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Tuesday, August 09, 2011 1:43 PM > To: U Bhaskar-B22300 > Cc: Wolfgang Grandegger; socketcan-core@lists.berlios.de; > netdev@vger.kernel.org > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > >> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>> ACK - The device tree bindings as in mainline's Documentation is a > >> mess. > >>>> If the powerpc guys are happy with a clock interfaces based > >>>> approach somewhere in arch/ppc, I'm more than happy to remove: > >>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>> driver) > > > [Bhaskar]I have pushed the FlexCAN series of patches, It contains the > > usage of all the fields posted in the FlexCAN bindings at > > http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo > > b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f > > 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb > > ebdc8274 > > I've commented the patches. They are in a very bad shape. Please test > Robin's patches. > > >>>> > >>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>>> - clock-frequency / a single clock-frequency attribute > >>> > >>> In the "net-next-2.6" tree there is also: > >>> > >>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>> p1010rdb.dts: fsl,flexcan-clock-source = > "platform"; > >>> p1010rdb.dts: fsl,flexcan-clock-source = > "platform"; > >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>> > >>> Especially the fsl,flexcan-clock-divider = <2>; might make people > >>> think, that they could set something else. > >> > > [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > > fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan > > clock source is the platform clock and it is CCB/2 If the "2" is > > misleading, the bindings can be changed or some text can be written to > > make the meaning of "2" Understandable , Please suggest .. > > The clock devider is crap. Why not specify the clockrate that goes into > the flexcan core? [Bhaskar] The reason why I placed the "fsl,flexcan-clock-divider" property is just because the earlier implementations Of CAN also follows the same approach. Please see below the approach of mscan. compatible = "fsl,mpc5121-mscan"; interrupts = <13 0x8>; interrupt-parent = <&ipic>; reg = <0x1380 0x80>; fsl,mscan-clock-source = "ref"; fsl,mscan-clock-divider = <3>; }; If you want we can remove the fsl,flexcan-clock-divider property. Please comment .. > > cheers, Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -- 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 08/09/2011 11:34 AM, U Bhaskar-B22300 wrote: > > >> -----Original Message----- >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >> Sent: Tuesday, August 09, 2011 1:43 PM >> To: U Bhaskar-B22300 >> Cc: Wolfgang Grandegger; socketcan-core@lists.berlios.de; >> netdev@vger.kernel.org >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >> >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>>>> ACK - The device tree bindings as in mainline's Documentation is a >>>> mess. >>>>>> If the powerpc guys are happy with a clock interfaces based >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl >>>>>> driver) >> >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the >>> usage of all the fields posted in the FlexCAN bindings at >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb >>> ebdc8274 >> >> I've commented the patches. They are in a very bad shape. Please test >> Robin's patches. >> >>>>>> >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>>>>> - clock-frequency / a single clock-frequency attribute >>>>> >>>>> In the "net-next-2.6" tree there is also: >>>>> >>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>>>> p1010rdb.dts: fsl,flexcan-clock-source = >> "platform"; >>>>> p1010rdb.dts: fsl,flexcan-clock-source = >> "platform"; >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>> >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people >>>>> think, that they could set something else. >>>> >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of >>> fsl,flexcan-clock-divider = <2>; But I kept it as "2" because FlexCan >>> clock source is the platform clock and it is CCB/2 If the "2" is >>> misleading, the bindings can be changed or some text can be written to >>> make the meaning of "2" Understandable , Please suggest .. >> >> The clock devider is crap. Why not specify the clockrate that goes into >> the flexcan core? > [Bhaskar] The reason why I placed the "fsl,flexcan-clock-divider" property is just because the earlier implementations > Of CAN also follows the same approach. Please see below the approach of mscan. > compatible = "fsl,mpc5121-mscan"; > interrupts = <13 0x8>; > interrupt-parent = <&ipic>; > reg = <0x1380 0x80>; > fsl,mscan-clock-source = "ref"; > fsl,mscan-clock-divider = <3>; > }; > If you want we can remove the fsl,flexcan-clock-divider property. Please comment .. For that platform the user can *change* these properties so select another clock-source or clock-divider. This is not the case for the P1010. Therefore these properties are not needed. Wolfgang. -- 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 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > > >> -----Original Message----- >> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >> Sent: Tuesday, August 09, 2011 2:03 PM >> To: U Bhaskar-B22300 >> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >> >> Hi Bhaskar, >> >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >>> >>> >>>> -----Original Message----- >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >>>> Sent: Tuesday, August 09, 2011 12:23 AM >>>> To: Wolfgang Grandegger >>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U >>>> Bhaskar- B22300 >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>> >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>>>> ACK - The device tree bindings as in mainline's Documentation is a >>>> mess. >>>>>> If the powerpc guys are happy with a clock interfaces based >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl >>>>>> driver) >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains the >>> usage of all the fields posted in the FlexCAN bindings at >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=blo >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a729f >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972cb >>> ebdc8274 >> >> As Marc already pointed out, Robin already has a much more advanced patch >> stack in preparation. Especially your patches do not care about the >> already existing Flexcan core on the Freescale's ARM socks. > [Bhaskar] No, the patches are taking care of the existing ARM functionality. > I have not tested on the ARM based board, but the patches are made in a > Manner that it should not break the ARM based functionality. >> >>>>>> >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>>>>> - clock-frequency / a single clock-frequency attribute >>>>> >>>>> In the "net-next-2.6" tree there is also: >>>>> >>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>>>> p1010rdb.dts: fsl,flexcan-clock-source = >> "platform"; >>>>> p1010rdb.dts: fsl,flexcan-clock-source = >> "platform"; >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>> >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people >>>>> think, that they could set something else. >>>> >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of >> fsl,flexcan-clock-divider = <2>; >>> But I kept it as "2" because FlexCan clock source is the >> platform clock and it is CCB/2 >>> If the "2" is misleading, the bindings can be changed or some >> text can be written to make the meaning of "2" >>> Understandable , Please suggest .. >> >> The clock source and frequency is fixed. Why do we need an extra >> properties for that. We have panned to remove these bogus bindings from >> the Linux kernel, which sneaked in *without* any review on the relevant >> mailing lists (at least I have not realized any posting). We do not think >> they are really needed. They just confuse the user. We also prefer to use >> the compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". It's >> unusual to add a version number, which is for the Flexcan on the PowerPC >> cores only, I assume, but there will be device tree for ARM soon. A >> proper compatibility string would be "fsl,p1010-flexcan" if we really >> need to distinguish. >> > [Bhaskar] About clock source.. There can be two sources of clock for the CAN. > Oscillator or the platform clock, but at present only platform clock is supported > in P1010.If we remove the fsl,flexcan-clock-source property, we will lost the flexibility > of changing the clock source .. > > About clock-frequency... it is also not fixed. It depends on the platform clock which in turns > Depends on the CCB clock. So it will be better to keep clock-frequency property which is getting fixed via u-boot. The frequency is fixed to CCB-frequency / 2. Will that ever change? What can we expect from future Flexcan hardware? Will it support further clock sources? Wolfgang. -- 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
> -----Original Message----- > From: Wolfgang Grandegger [mailto:wg@grandegger.com] > Sent: Tuesday, August 09, 2011 4:19 PM > To: U Bhaskar-B22300 > Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > > > > > >> -----Original Message----- > >> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >> Sent: Tuesday, August 09, 2011 2:03 PM > >> To: U Bhaskar-B22300 > >> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >> > >> Hi Bhaskar, > >> > >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > >>>> Sent: Tuesday, August 09, 2011 12:23 AM > >>>> To: Wolfgang Grandegger > >>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > >>>> Bhaskar- B22300 > >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>> > >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>>>> ACK - The device tree bindings as in mainline's Documentation is > >>>>>> a > >>>> mess. > >>>>>> If the powerpc guys are happy with a clock interfaces based > >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: > >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>>>> driver) > >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains > >>> the usage of all the fields posted in the FlexCAN bindings at > >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b > >>> lo > >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72 > >>> 9f > >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972 > >>> cb > >>> ebdc8274 > >> > >> As Marc already pointed out, Robin already has a much more advanced > >> patch stack in preparation. Especially your patches do not care about > >> the already existing Flexcan core on the Freescale's ARM socks. > > [Bhaskar] No, the patches are taking care of the existing ARM > functionality. > > I have not tested on the ARM based board, but the patches are made > in a > > Manner that it should not break the ARM based functionality. > >> > >>>>>> > >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>>>>> - clock-frequency / a single clock-frequency attribute > >>>>> > >>>>> In the "net-next-2.6" tree there is also: > >>>>> > >>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >> "platform"; > >>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >> "platform"; > >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>> > >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people > >>>>> think, that they could set something else. > >>>> > >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > >> fsl,flexcan-clock-divider = <2>; > >>> But I kept it as "2" because FlexCan clock source is the > >> platform clock and it is CCB/2 > >>> If the "2" is misleading, the bindings can be changed or some > >> text can be written to make the meaning of "2" > >>> Understandable , Please suggest .. > >> > >> The clock source and frequency is fixed. Why do we need an extra > >> properties for that. We have panned to remove these bogus bindings > >> from the Linux kernel, which sneaked in *without* any review on the > >> relevant mailing lists (at least I have not realized any posting). We > >> do not think they are really needed. They just confuse the user. We > >> also prefer to use the compatibility string "fsl,flexcan" instead > >> "fsl,flexcan-v1.0". It's unusual to add a version number, which is > >> for the Flexcan on the PowerPC cores only, I assume, but there will > >> be device tree for ARM soon. A proper compatibility string would be > >> "fsl,p1010-flexcan" if we really need to distinguish. > >> > > [Bhaskar] About clock source.. There can be two sources of clock for > the CAN. > > Oscillator or the platform clock, but at present only platform > clock is supported > > in P1010.If we remove the fsl,flexcan-clock-source property, we > will lost the flexibility > > of changing the clock source .. > > > > About clock-frequency... it is also not fixed. It depends on > the platform clock which in turns > > Depends on the CCB clock. So it will be better to keep clock- > frequency property which is getting fixed via u-boot. > > The frequency is fixed to CCB-frequency / 2. Will that ever change? What > can we expect from future Flexcan hardware? Will it support further clock > sources? [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it appropriately > > Wolfgang. -- 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 Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: > > > > -----Original Message----- > > From: Wolfgang Grandegger [mailto:wg@grandegger.com] > > Sent: Tuesday, August 09, 2011 4:19 PM > > To: U Bhaskar-B22300 > > Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > > > On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > > > > > > > > >> -----Original Message----- > > >> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > > >> Sent: Tuesday, August 09, 2011 2:03 PM > > >> To: U Bhaskar-B22300 > > >> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > > >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > > >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > >> > > >> Hi Bhaskar, > > >> > > >> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > > >>> > > >>> > > >>>> -----Original Message----- > > >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > > >>>> Sent: Tuesday, August 09, 2011 12:23 AM > > >>>> To: Wolfgang Grandegger > > >>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > > >>>> Bhaskar- B22300 > > >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > >>>> > > >>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > > >>>>>> ACK - The device tree bindings as in mainline's Documentation is > > >>>>>> a > > >>>> mess. > > >>>>>> If the powerpc guys are happy with a clock interfaces based > > >>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: > > >>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > > >>>>>> driver) > > >>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains > > >>> the usage of all the fields posted in the FlexCAN bindings at > > >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b > > >>> lo > > >>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72 > > >>> 9f > > >>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972 > > >>> cb > > >>> ebdc8274 > > >> > > >> As Marc already pointed out, Robin already has a much more advanced > > >> patch stack in preparation. Especially your patches do not care about > > >> the already existing Flexcan core on the Freescale's ARM socks. > > > [Bhaskar] No, the patches are taking care of the existing ARM > > functionality. > > > I have not tested on the ARM based board, but the patches are made > > in a > > > Manner that it should not break the ARM based functionality. > > >> > > >>>>>> > > >>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > > >>>>>> - clock-frequency / a single clock-frequency attribute > > >>>>> > > >>>>> In the "net-next-2.6" tree there is also: > > >>>>> > > >>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > > >>>>> p1010rdb.dts: fsl,flexcan-clock-source = > > >> "platform"; > > >>>>> p1010rdb.dts: fsl,flexcan-clock-source = > > >> "platform"; > > >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > >>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > > >>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > > >>>>> > > >>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people > > >>>>> think, that they could set something else. > > >>>> > > >>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > > >> fsl,flexcan-clock-divider = <2>; > > >>> But I kept it as "2" because FlexCan clock source is the > > >> platform clock and it is CCB/2 > > >>> If the "2" is misleading, the bindings can be changed or some > > >> text can be written to make the meaning of "2" > > >>> Understandable , Please suggest .. > > >> > > >> The clock source and frequency is fixed. Why do we need an extra > > >> properties for that. We have panned to remove these bogus bindings > > >> from the Linux kernel, which sneaked in *without* any review on the > > >> relevant mailing lists (at least I have not realized any posting). We > > >> do not think they are really needed. They just confuse the user. We > > >> also prefer to use the compatibility string "fsl,flexcan" instead > > >> "fsl,flexcan-v1.0". It's unusual to add a version number, which is > > >> for the Flexcan on the PowerPC cores only, I assume, but there will > > >> be device tree for ARM soon. A proper compatibility string would be > > >> "fsl,p1010-flexcan" if we really need to distinguish. > > >> > > > [Bhaskar] About clock source.. There can be two sources of clock for > > the CAN. > > > Oscillator or the platform clock, but at present only platform > > clock is supported > > > in P1010.If we remove the fsl,flexcan-clock-source property, we > > will lost the flexibility > > > of changing the clock source .. > > > > > > About clock-frequency... it is also not fixed. It depends on > > the platform clock which in turns > > > Depends on the CCB clock. So it will be better to keep clock- > > frequency property which is getting fixed via u-boot. > > > > The frequency is fixed to CCB-frequency / 2. Will that ever change? What > > can we expect from future Flexcan hardware? Will it support further clock > > sources? > [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for > clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. > For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it > appropriately Speaking of the dts file, I have left the p1010si.dtsi file with the fsl,flexcan-v1.0 .compatible definition. The flexcan folks (IIRC Wolfgang) objected to that as it does not follow the standard which should be just fsl,flexcan. How would you like to change that? Should I add it as part of this patch, add another patch to the series, or let you take care of it? Also, I assume the uboot project will need to be changed as well to reflect the corrected name. Thanks, Robin -- 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
Bhaskar, On 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote: >> The frequency is fixed to CCB-frequency / 2. Will that ever change? What >> can we expect from future Flexcan hardware? Will it support further clock >> sources? > [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for > clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. > For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it > appropriately are you actually following the discussion about Robin's patches? Robin has provided patch that work without the clock frequency in the DT at all.. Marc
> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > Sent: Tuesday, August 09, 2011 6:21 PM > To: U Bhaskar-B22300 > Cc: Wolfgang Grandegger; socketcan-core@lists.berlios.de; > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > Bhaskar, > > On 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote: > >> The frequency is fixed to CCB-frequency / 2. Will that ever change? > >> What can we expect from future Flexcan hardware? Will it support > >> further clock sources? > > [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the > CCB gets changed that will be taken care by the u-boot fixup code for > > clock-frequency. clock-frequency is not filled by somebody in the > dts file. It will be done by u-boot. > > For clock source,I can't say right now, that's why I have kept a > property for this in the can node. So that in future, we need to fill it > > appropriately > > are you actually following the discussion about Robin's patches? Robin > has provided patch that work without the clock frequency in the DT at > all.. [Bhaskar] Hi Marc, Yes I am following the discussion on Robin's patch. Just going tru the patches. I will respond > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | -- 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 08/09/2011 02:49 PM, Robin Holt wrote: > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: >> >> >>> -----Original Message----- >>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>> Sent: Tuesday, August 09, 2011 4:19 PM >>> To: U Bhaskar-B22300 >>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>> >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>>>> Sent: Tuesday, August 09, 2011 2:03 PM >>>>> To: U Bhaskar-B22300 >>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>>> >>>>> Hi Bhaskar, >>>>> >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM >>>>>>> To: Wolfgang Grandegger >>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U >>>>>>> Bhaskar- B22300 >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>>>>> >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is >>>>>>>>> a >>>>>>> mess. >>>>>>>>> If the powerpc guys are happy with a clock interfaces based >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl >>>>>>>>> driver) >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains >>>>>> the usage of all the fields posted in the FlexCAN bindings at >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b >>>>>> lo >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72 >>>>>> 9f >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972 >>>>>> cb >>>>>> ebdc8274 >>>>> >>>>> As Marc already pointed out, Robin already has a much more advanced >>>>> patch stack in preparation. Especially your patches do not care about >>>>> the already existing Flexcan core on the Freescale's ARM socks. >>>> [Bhaskar] No, the patches are taking care of the existing ARM >>> functionality. >>>> I have not tested on the ARM based board, but the patches are made >>> in a >>>> Manner that it should not break the ARM based functionality. >>>>> >>>>>>>>> >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or >>>>>>>>> - clock-frequency / a single clock-frequency attribute >>>>>>>> >>>>>>>> In the "net-next-2.6" tree there is also: >>>>>>>> >>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>> "platform"; >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>> "platform"; >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; >>>>>>>> >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people >>>>>>>> think, that they could set something else. >>>>>>> >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of >>>>> fsl,flexcan-clock-divider = <2>; >>>>>> But I kept it as "2" because FlexCan clock source is the >>>>> platform clock and it is CCB/2 >>>>>> If the "2" is misleading, the bindings can be changed or some >>>>> text can be written to make the meaning of "2" >>>>>> Understandable , Please suggest .. >>>>> >>>>> The clock source and frequency is fixed. Why do we need an extra >>>>> properties for that. We have panned to remove these bogus bindings >>>>> from the Linux kernel, which sneaked in *without* any review on the >>>>> relevant mailing lists (at least I have not realized any posting). We >>>>> do not think they are really needed. They just confuse the user. We >>>>> also prefer to use the compatibility string "fsl,flexcan" instead >>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is >>>>> for the Flexcan on the PowerPC cores only, I assume, but there will >>>>> be device tree for ARM soon. A proper compatibility string would be >>>>> "fsl,p1010-flexcan" if we really need to distinguish. >>>>> >>>> [Bhaskar] About clock source.. There can be two sources of clock for >>> the CAN. >>>> Oscillator or the platform clock, but at present only platform >>> clock is supported >>>> in P1010.If we remove the fsl,flexcan-clock-source property, we >>> will lost the flexibility >>>> of changing the clock source .. >>>> >>>> About clock-frequency... it is also not fixed. It depends on >>> the platform clock which in turns >>>> Depends on the CCB clock. So it will be better to keep clock- >>> frequency property which is getting fixed via u-boot. >>> >>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What >>> can we expect from future Flexcan hardware? Will it support further clock >>> sources? >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for >> clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. >> For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it >> appropriately > > Speaking of the dts file, I have left the p1010si.dtsi file with > the fsl,flexcan-v1.0 .compatible definition. The flexcan folks > (IIRC Wolfgang) objected to that as it does not follow the standard > which should be just fsl,flexcan. > > How would you like to change that? Should I add it as part of this patch, > add another patch to the series, or let you take care of it? > > Also, I assume the uboot project will need to be changed as well to > reflect the corrected name. I think you should provide patches within this series to cleanup the obsolete stuff, dts and binding doc. Wolfgang. -- 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 Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: > On 08/09/2011 02:49 PM, Robin Holt wrote: > > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: > >> > >> > >>> -----Original Message----- > >>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>> Sent: Tuesday, August 09, 2011 4:19 PM > >>> To: U Bhaskar-B22300 > >>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>> > >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>>>> Sent: Tuesday, August 09, 2011 2:03 PM > >>>>> To: U Bhaskar-B22300 > >>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>>> > >>>>> Hi Bhaskar, > >>>>> > >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > >>>>>> > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM > >>>>>>> To: Wolfgang Grandegger > >>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > >>>>>>> Bhaskar- B22300 > >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>>>>> > >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is > >>>>>>>>> a > >>>>>>> mess. > >>>>>>>>> If the powerpc guys are happy with a clock interfaces based > >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: > >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>>>>>>> driver) > >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains > >>>>>> the usage of all the fields posted in the FlexCAN bindings at > >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b > >>>>>> lo > >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72 > >>>>>> 9f > >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972 > >>>>>> cb > >>>>>> ebdc8274 > >>>>> > >>>>> As Marc already pointed out, Robin already has a much more advanced > >>>>> patch stack in preparation. Especially your patches do not care about > >>>>> the already existing Flexcan core on the Freescale's ARM socks. > >>>> [Bhaskar] No, the patches are taking care of the existing ARM > >>> functionality. > >>>> I have not tested on the ARM based board, but the patches are made > >>> in a > >>>> Manner that it should not break the ARM based functionality. > >>>>> > >>>>>>>>> > >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>>>>>>>> - clock-frequency / a single clock-frequency attribute > >>>>>>>> > >>>>>>>> In the "net-next-2.6" tree there is also: > >>>>>>>> > >>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>> "platform"; > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>> "platform"; > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>>>>> > >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people > >>>>>>>> think, that they could set something else. > >>>>>>> > >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > >>>>> fsl,flexcan-clock-divider = <2>; > >>>>>> But I kept it as "2" because FlexCan clock source is the > >>>>> platform clock and it is CCB/2 > >>>>>> If the "2" is misleading, the bindings can be changed or some > >>>>> text can be written to make the meaning of "2" > >>>>>> Understandable , Please suggest .. > >>>>> > >>>>> The clock source and frequency is fixed. Why do we need an extra > >>>>> properties for that. We have panned to remove these bogus bindings > >>>>> from the Linux kernel, which sneaked in *without* any review on the > >>>>> relevant mailing lists (at least I have not realized any posting). We > >>>>> do not think they are really needed. They just confuse the user. We > >>>>> also prefer to use the compatibility string "fsl,flexcan" instead > >>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is > >>>>> for the Flexcan on the PowerPC cores only, I assume, but there will > >>>>> be device tree for ARM soon. A proper compatibility string would be > >>>>> "fsl,p1010-flexcan" if we really need to distinguish. > >>>>> > >>>> [Bhaskar] About clock source.. There can be two sources of clock for > >>> the CAN. > >>>> Oscillator or the platform clock, but at present only platform > >>> clock is supported > >>>> in P1010.If we remove the fsl,flexcan-clock-source property, we > >>> will lost the flexibility > >>>> of changing the clock source .. > >>>> > >>>> About clock-frequency... it is also not fixed. It depends on > >>> the platform clock which in turns > >>>> Depends on the CCB clock. So it will be better to keep clock- > >>> frequency property which is getting fixed via u-boot. > >>> > >>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What > >>> can we expect from future Flexcan hardware? Will it support further clock > >>> sources? > >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for > >> clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. > >> For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it > >> appropriately > > > > Speaking of the dts file, I have left the p1010si.dtsi file with > > the fsl,flexcan-v1.0 .compatible definition. The flexcan folks > > (IIRC Wolfgang) objected to that as it does not follow the standard > > which should be just fsl,flexcan. > > > > How would you like to change that? Should I add it as part of this patch, > > add another patch to the series, or let you take care of it? > > > > Also, I assume the uboot project will need to be changed as well to > > reflect the corrected name. > > I think you should provide patches within this series to cleanup the > obsolete stuff, dts and binding doc. Will do. Robin -- 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 08/09/2011 02:41 PM, U Bhaskar-B22300 wrote: > > >> -----Original Message----- >> From: Wolfgang Grandegger [mailto:wg@grandegger.com] ... >> The frequency is fixed to CCB-frequency / 2. Will that ever change? What >> can we expect from future Flexcan hardware? Will it support further clock >> sources? > [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for > clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. U-Boot fills in "clock_freq", anyway... > For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it > appropriately This just confirms that we currently can live without clock-frequency, clock-source and clock-divider properties. If it's really required sometime in the future, we can add it, I think. Wolfgang. -- 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 Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: > On 08/09/2011 02:49 PM, Robin Holt wrote: > > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: > >> > >> > >>> -----Original Message----- > >>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>> Sent: Tuesday, August 09, 2011 4:19 PM > >>> To: U Bhaskar-B22300 > >>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>> > >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>>>> Sent: Tuesday, August 09, 2011 2:03 PM > >>>>> To: U Bhaskar-B22300 > >>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>>> > >>>>> Hi Bhaskar, > >>>>> > >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > >>>>>> > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM > >>>>>>> To: Wolfgang Grandegger > >>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > >>>>>>> Bhaskar- B22300 > >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>>>>> > >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>>>>>>> ACK - The device tree bindings as in mainline's Documentation is > >>>>>>>>> a > >>>>>>> mess. > >>>>>>>>> If the powerpc guys are happy with a clock interfaces based > >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to remove: > >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>>>>>>> driver) > >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It contains > >>>>>> the usage of all the fields posted in the FlexCAN bindings at > >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=b > >>>>>> lo > >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h=1a72 > >>>>>> 9f > >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb8516972 > >>>>>> cb > >>>>>> ebdc8274 > >>>>> > >>>>> As Marc already pointed out, Robin already has a much more advanced > >>>>> patch stack in preparation. Especially your patches do not care about > >>>>> the already existing Flexcan core on the Freescale's ARM socks. > >>>> [Bhaskar] No, the patches are taking care of the existing ARM > >>> functionality. > >>>> I have not tested on the ARM based board, but the patches are made > >>> in a > >>>> Manner that it should not break the ARM based functionality. > >>>>> > >>>>>>>>> > >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in arch/ppc, or > >>>>>>>>> - clock-frequency / a single clock-frequency attribute > >>>>>>>> > >>>>>>>> In the "net-next-2.6" tree there is also: > >>>>>>>> > >>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>> "platform"; > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>> "platform"; > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan-v1.0"; > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = <2>; > >>>>>>>> > >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make people > >>>>>>>> think, that they could set something else. > >>>>>>> > >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need of > >>>>> fsl,flexcan-clock-divider = <2>; > >>>>>> But I kept it as "2" because FlexCan clock source is the > >>>>> platform clock and it is CCB/2 > >>>>>> If the "2" is misleading, the bindings can be changed or some > >>>>> text can be written to make the meaning of "2" > >>>>>> Understandable , Please suggest .. > >>>>> > >>>>> The clock source and frequency is fixed. Why do we need an extra > >>>>> properties for that. We have panned to remove these bogus bindings > >>>>> from the Linux kernel, which sneaked in *without* any review on the > >>>>> relevant mailing lists (at least I have not realized any posting). We > >>>>> do not think they are really needed. They just confuse the user. We > >>>>> also prefer to use the compatibility string "fsl,flexcan" instead > >>>>> "fsl,flexcan-v1.0". It's unusual to add a version number, which is > >>>>> for the Flexcan on the PowerPC cores only, I assume, but there will > >>>>> be device tree for ARM soon. A proper compatibility string would be > >>>>> "fsl,p1010-flexcan" if we really need to distinguish. > >>>>> > >>>> [Bhaskar] About clock source.. There can be two sources of clock for > >>> the CAN. > >>>> Oscillator or the platform clock, but at present only platform > >>> clock is supported > >>>> in P1010.If we remove the fsl,flexcan-clock-source property, we > >>> will lost the flexibility > >>>> of changing the clock source .. > >>>> > >>>> About clock-frequency... it is also not fixed. It depends on > >>> the platform clock which in turns > >>>> Depends on the CCB clock. So it will be better to keep clock- > >>> frequency property which is getting fixed via u-boot. > >>> > >>> The frequency is fixed to CCB-frequency / 2. Will that ever change? What > >>> can we expect from future Flexcan hardware? Will it support further clock > >>> sources? > >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if the CCB gets changed that will be taken care by the u-boot fixup code for > >> clock-frequency. clock-frequency is not filled by somebody in the dts file. It will be done by u-boot. > >> For clock source,I can't say right now, that's why I have kept a property for this in the can node. So that in future, we need to fill it > >> appropriately > > > > Speaking of the dts file, I have left the p1010si.dtsi file with > > the fsl,flexcan-v1.0 .compatible definition. The flexcan folks > > (IIRC Wolfgang) objected to that as it does not follow the standard > > which should be just fsl,flexcan. > > > > How would you like to change that? Should I add it as part of this patch, > > add another patch to the series, or let you take care of it? > > > > Also, I assume the uboot project will need to be changed as well to > > reflect the corrected name. > > I think you should provide patches within this series to cleanup the > obsolete stuff, dts and binding doc. It reads to me that the binding doc now reduces just the required properties. Should I remove the file entirely? Robin -- 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
> -----Original Message----- > From: Robin Holt [mailto:holt@sgi.com] > Sent: Tuesday, August 09, 2011 7:06 PM > To: Wolfgang Grandegger > Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de; > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine- > Budde > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: > > On 08/09/2011 02:49 PM, Robin Holt wrote: > > > On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: > > >> > > >> > > >>> -----Original Message----- > > >>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > > >>> Sent: Tuesday, August 09, 2011 4:19 PM > > >>> To: U Bhaskar-B22300 > > >>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > > >>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > > >>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > >>> > > >>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > > >>>> > > >>>> > > >>>>> -----Original Message----- > > >>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > > >>>>> Sent: Tuesday, August 09, 2011 2:03 PM > > >>>>> To: U Bhaskar-B22300 > > >>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > > >>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > > >>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock > source. > > >>>>> > > >>>>> Hi Bhaskar, > > >>>>> > > >>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > > >>>>>> > > >>>>>> > > >>>>>>> -----Original Message----- > > >>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > > >>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM > > >>>>>>> To: Wolfgang Grandegger > > >>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > > >>>>>>> Bhaskar- B22300 > > >>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock > source. > > >>>>>>> > > >>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > > >>>>>>>>> ACK - The device tree bindings as in mainline's > > >>>>>>>>> Documentation is a > > >>>>>>> mess. > > >>>>>>>>> If the powerpc guys are happy with a clock interfaces based > > >>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to > remove: > > >>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > > >>>>>>>>> driver) > > >>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It > > >>>>>> contains the usage of all the fields posted in the FlexCAN > > >>>>>> bindings at > > >>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi > > >>>>>> t;a=b > > >>>>>> lo > > >>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h > > >>>>>> =1a72 > > >>>>>> 9f > > >>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85 > > >>>>>> 16972 > > >>>>>> cb > > >>>>>> ebdc8274 > > >>>>> > > >>>>> As Marc already pointed out, Robin already has a much more > > >>>>> advanced patch stack in preparation. Especially your patches do > > >>>>> not care about the already existing Flexcan core on the > Freescale's ARM socks. > > >>>> [Bhaskar] No, the patches are taking care of the existing ARM > > >>> functionality. > > >>>> I have not tested on the ARM based board, but the patches are > > >>>> made > > >>> in a > > >>>> Manner that it should not break the ARM based functionality. > > >>>>> > > >>>>>>>>> > > >>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in > arch/ppc, or > > >>>>>>>>> - clock-frequency / a single clock-frequency > attribute > > >>>>>>>> > > >>>>>>>> In the "net-next-2.6" tree there is also: > > >>>>>>>> > > >>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > > >>>>> "platform"; > > >>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > > >>>>> "platform"; > > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- > v1.0"; > > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = > <2>; > > >>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- > v1.0"; > > >>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = > <2>; > > >>>>>>>> > > >>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make > > >>>>>>>> people think, that they could set something else. > > >>>>>>> > > >>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need > > >>>>>> of > > >>>>> fsl,flexcan-clock-divider = <2>; > > >>>>>> But I kept it as "2" because FlexCan clock source is the > > >>>>> platform clock and it is CCB/2 > > >>>>>> If the "2" is misleading, the bindings can be changed or > > >>>>>> some > > >>>>> text can be written to make the meaning of "2" > > >>>>>> Understandable , Please suggest .. > > >>>>> > > >>>>> The clock source and frequency is fixed. Why do we need an extra > > >>>>> properties for that. We have panned to remove these bogus > > >>>>> bindings from the Linux kernel, which sneaked in *without* any > > >>>>> review on the relevant mailing lists (at least I have not > > >>>>> realized any posting). We do not think they are really needed. > > >>>>> They just confuse the user. We also prefer to use the > > >>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". > > >>>>> It's unusual to add a version number, which is for the Flexcan > > >>>>> on the PowerPC cores only, I assume, but there will be device > > >>>>> tree for ARM soon. A proper compatibility string would be > "fsl,p1010-flexcan" if we really need to distinguish. > > >>>>> > > >>>> [Bhaskar] About clock source.. There can be two sources of clock > > >>>> for > > >>> the CAN. > > >>>> Oscillator or the platform clock, but at present only > platform > > >>> clock is supported > > >>>> in P1010.If we remove the fsl,flexcan-clock-source property, > we > > >>> will lost the flexibility > > >>>> of changing the clock source .. > > >>>> > > >>>> About clock-frequency... it is also not fixed. It > > >>>> depends on > > >>> the platform clock which in turns > > >>>> Depends on the CCB clock. So it will be better to keep > > >>>> clock- > > >>> frequency property which is getting fixed via u-boot. > > >>> > > >>> The frequency is fixed to CCB-frequency / 2. Will that ever > > >>> change? What can we expect from future Flexcan hardware? Will it > > >>> support further clock sources? > > >> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if > the CCB gets changed that will be taken care by the u-boot fixup code for > > >> clock-frequency. clock-frequency is not filled by somebody in the > dts file. It will be done by u-boot. > > >> For clock source,I can't say right now, that's why I have kept a > property for this in the can node. So that in future, we need to fill it > > >> appropriately > > > > > > Speaking of the dts file, I have left the p1010si.dtsi file with the > > > fsl,flexcan-v1.0 .compatible definition. The flexcan folks (IIRC > > > Wolfgang) objected to that as it does not follow the standard which > > > should be just fsl,flexcan. > > > > > > How would you like to change that? Should I add it as part of this > > > patch, add another patch to the series, or let you take care of it? > > > > > > Also, I assume the uboot project will need to be changed as well to > > > reflect the corrected name. > > > > I think you should provide patches within this series to cleanup the > > obsolete stuff, dts and binding doc. > > It reads to me that the binding doc now reduces just the required > properties. Should I remove the file entirely? [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity about the CAN functionality can0@1c000 { compatible = "fsl,flexcan"; reg = <0x1c000 0x1000>; interrupts = <48 0x2>; interrupt-parent = <&mpic>; clock-frequency = <fixed by u-boot>; }; > > Robin -- 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 Tue, Aug 09, 2011 at 01:44:16PM +0000, U Bhaskar-B22300 wrote: > > > > -----Original Message----- > > From: Robin Holt [mailto:holt@sgi.com] > > Sent: Tuesday, August 09, 2011 7:06 PM > > To: Wolfgang Grandegger > > Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de; > > netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine- > > Budde > > Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > > > > On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: > > > On 08/09/2011 02:49 PM, Robin Holt wrote: ... > > > > How would you like to change that? Should I add it as part of this > > > > patch, add another patch to the series, or let you take care of it? > > > > > > > > Also, I assume the uboot project will need to be changed as well to > > > > reflect the corrected name. > > > > > > I think you should provide patches within this series to cleanup the > > > obsolete stuff, dts and binding doc. > > > > It reads to me that the binding doc now reduces just the required > > properties. Should I remove the file entirely? > [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity > about the CAN functionality > can0@1c000 { > compatible = "fsl,flexcan"; > reg = <0x1c000 0x1000>; > interrupts = <48 0x2>; > interrupt-parent = <&mpic>; > clock-frequency = <fixed by u-boot>; > }; I am not sure what clarity we get for it. Here it is as a work in progress: CAN Device Tree Bindings ------------------------ 2011 Freescale Semiconductor, Inc. fsl,flexcan nodes ----------------------- Only the required compatible-, reg- and interrupt-properties are supported. Examples: can0@1c000 { compatible = "fsl,flexcan"; reg = <0x1c000 0x1000>; interrupts = <48 0x2>; interrupt-parent = <&mpic>; }; -- 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 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote: > > >> -----Original Message----- >> From: Robin Holt [mailto:holt@sgi.com] >> Sent: Tuesday, August 09, 2011 7:06 PM >> To: Wolfgang Grandegger >> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de; >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine- >> Budde >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >> >> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: >>> On 08/09/2011 02:49 PM, Robin Holt wrote: >>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>>>>> Sent: Tuesday, August 09, 2011 4:19 PM >>>>>> To: U Bhaskar-B22300 >>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>>>> >>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM >>>>>>>> To: U Bhaskar-B22300 >>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock >> source. >>>>>>>> >>>>>>>> Hi Bhaskar, >>>>>>>> >>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM >>>>>>>>>> To: Wolfgang Grandegger >>>>>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U >>>>>>>>>> Bhaskar- B22300 >>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock >> source. >>>>>>>>>> >>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>>>>>>>>>> ACK - The device tree bindings as in mainline's >>>>>>>>>>>> Documentation is a >>>>>>>>>> mess. >>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based >>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to >> remove: >>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl >>>>>>>>>>>> driver) >>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It >>>>>>>>> contains the usage of all the fields posted in the FlexCAN >>>>>>>>> bindings at >>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi >>>>>>>>> t;a=b >>>>>>>>> lo >>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h >>>>>>>>> =1a72 >>>>>>>>> 9f >>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85 >>>>>>>>> 16972 >>>>>>>>> cb >>>>>>>>> ebdc8274 >>>>>>>> >>>>>>>> As Marc already pointed out, Robin already has a much more >>>>>>>> advanced patch stack in preparation. Especially your patches do >>>>>>>> not care about the already existing Flexcan core on the >> Freescale's ARM socks. >>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM >>>>>> functionality. >>>>>>> I have not tested on the ARM based board, but the patches are >>>>>>> made >>>>>> in a >>>>>>> Manner that it should not break the ARM based functionality. >>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in >> arch/ppc, or >>>>>>>>>>>> - clock-frequency / a single clock-frequency >> attribute >>>>>>>>>>> >>>>>>>>>>> In the "net-next-2.6" tree there is also: >>>>>>>>>>> >>>>>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>>>>> "platform"; >>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>>>>> "platform"; >>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- >> v1.0"; >>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = >> <2>; >>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- >> v1.0"; >>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = >> <2>; >>>>>>>>>>> >>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make >>>>>>>>>>> people think, that they could set something else. >>>>>>>>>> >>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need >>>>>>>>> of >>>>>>>> fsl,flexcan-clock-divider = <2>; >>>>>>>>> But I kept it as "2" because FlexCan clock source is the >>>>>>>> platform clock and it is CCB/2 >>>>>>>>> If the "2" is misleading, the bindings can be changed or >>>>>>>>> some >>>>>>>> text can be written to make the meaning of "2" >>>>>>>>> Understandable , Please suggest .. >>>>>>>> >>>>>>>> The clock source and frequency is fixed. Why do we need an extra >>>>>>>> properties for that. We have panned to remove these bogus >>>>>>>> bindings from the Linux kernel, which sneaked in *without* any >>>>>>>> review on the relevant mailing lists (at least I have not >>>>>>>> realized any posting). We do not think they are really needed. >>>>>>>> They just confuse the user. We also prefer to use the >>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". >>>>>>>> It's unusual to add a version number, which is for the Flexcan >>>>>>>> on the PowerPC cores only, I assume, but there will be device >>>>>>>> tree for ARM soon. A proper compatibility string would be >> "fsl,p1010-flexcan" if we really need to distinguish. >>>>>>>> >>>>>>> [Bhaskar] About clock source.. There can be two sources of clock >>>>>>> for >>>>>> the CAN. >>>>>>> Oscillator or the platform clock, but at present only >> platform >>>>>> clock is supported >>>>>>> in P1010.If we remove the fsl,flexcan-clock-source property, >> we >>>>>> will lost the flexibility >>>>>>> of changing the clock source .. >>>>>>> >>>>>>> About clock-frequency... it is also not fixed. It >>>>>>> depends on >>>>>> the platform clock which in turns >>>>>>> Depends on the CCB clock. So it will be better to keep >>>>>>> clock- >>>>>> frequency property which is getting fixed via u-boot. >>>>>> >>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever >>>>>> change? What can we expect from future Flexcan hardware? Will it >>>>>> support further clock sources? >>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if >> the CCB gets changed that will be taken care by the u-boot fixup code for >>>>> clock-frequency. clock-frequency is not filled by somebody in the >> dts file. It will be done by u-boot. >>>>> For clock source,I can't say right now, that's why I have kept a >> property for this in the can node. So that in future, we need to fill it >>>>> appropriately >>>> >>>> Speaking of the dts file, I have left the p1010si.dtsi file with the >>>> fsl,flexcan-v1.0 .compatible definition. The flexcan folks (IIRC >>>> Wolfgang) objected to that as it does not follow the standard which >>>> should be just fsl,flexcan. >>>> >>>> How would you like to change that? Should I add it as part of this >>>> patch, add another patch to the series, or let you take care of it? >>>> >>>> Also, I assume the uboot project will need to be changed as well to >>>> reflect the corrected name. >>> >>> I think you should provide patches within this series to cleanup the >>> obsolete stuff, dts and binding doc. >> >> It reads to me that the binding doc now reduces just the required >> properties. Should I remove the file entirely? > [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity > about the CAN functionality > can0@1c000 { > compatible = "fsl,flexcan"; > reg = <0x1c000 0x1000>; > interrupts = <48 0x2>; > interrupt-parent = <&mpic>; > clock-frequency = <fixed by u-boot>; > }; Yes, I also find the introduction is quite useful, with some related correction. Wolfgang. -- 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 Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote: > On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote: > > > > > >> -----Original Message----- > >> From: Robin Holt [mailto:holt@sgi.com] > >> Sent: Tuesday, August 09, 2011 7:06 PM > >> To: Wolfgang Grandegger > >> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de; > >> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine- > >> Budde > >> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >> > >> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: > >>> On 08/09/2011 02:49 PM, Robin Holt wrote: > >>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: > >>>>> > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>>>>> Sent: Tuesday, August 09, 2011 4:19 PM > >>>>>> To: U Bhaskar-B22300 > >>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. > >>>>>> > >>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: > >>>>>>> > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] > >>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM > >>>>>>>> To: U Bhaskar-B22300 > >>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; > >>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org > >>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock > >> source. > >>>>>>>> > >>>>>>>> Hi Bhaskar, > >>>>>>>> > >>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> -----Original Message----- > >>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] > >>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM > >>>>>>>>>> To: Wolfgang Grandegger > >>>>>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U > >>>>>>>>>> Bhaskar- B22300 > >>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock > >> source. > >>>>>>>>>> > >>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: > >>>>>>>>>>>> ACK - The device tree bindings as in mainline's > >>>>>>>>>>>> Documentation is a > >>>>>>>>>> mess. > >>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based > >>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to > >> remove: > >>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl > >>>>>>>>>>>> driver) > >>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It > >>>>>>>>> contains the usage of all the fields posted in the FlexCAN > >>>>>>>>> bindings at > >>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi > >>>>>>>>> t;a=b > >>>>>>>>> lo > >>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h > >>>>>>>>> =1a72 > >>>>>>>>> 9f > >>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85 > >>>>>>>>> 16972 > >>>>>>>>> cb > >>>>>>>>> ebdc8274 > >>>>>>>> > >>>>>>>> As Marc already pointed out, Robin already has a much more > >>>>>>>> advanced patch stack in preparation. Especially your patches do > >>>>>>>> not care about the already existing Flexcan core on the > >> Freescale's ARM socks. > >>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM > >>>>>> functionality. > >>>>>>> I have not tested on the ARM based board, but the patches are > >>>>>>> made > >>>>>> in a > >>>>>>> Manner that it should not break the ARM based functionality. > >>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in > >> arch/ppc, or > >>>>>>>>>>>> - clock-frequency / a single clock-frequency > >> attribute > >>>>>>>>>>> > >>>>>>>>>>> In the "net-next-2.6" tree there is also: > >>>>>>>>>>> > >>>>>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts > >>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>>>>> "platform"; > >>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = > >>>>>>>> "platform"; > >>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- > >> v1.0"; > >>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = > >> <2>; > >>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- > >> v1.0"; > >>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = > >> <2>; > >>>>>>>>>>> > >>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make > >>>>>>>>>>> people think, that they could set something else. > >>>>>>>>>> > >>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need > >>>>>>>>> of > >>>>>>>> fsl,flexcan-clock-divider = <2>; > >>>>>>>>> But I kept it as "2" because FlexCan clock source is the > >>>>>>>> platform clock and it is CCB/2 > >>>>>>>>> If the "2" is misleading, the bindings can be changed or > >>>>>>>>> some > >>>>>>>> text can be written to make the meaning of "2" > >>>>>>>>> Understandable , Please suggest .. > >>>>>>>> > >>>>>>>> The clock source and frequency is fixed. Why do we need an extra > >>>>>>>> properties for that. We have panned to remove these bogus > >>>>>>>> bindings from the Linux kernel, which sneaked in *without* any > >>>>>>>> review on the relevant mailing lists (at least I have not > >>>>>>>> realized any posting). We do not think they are really needed. > >>>>>>>> They just confuse the user. We also prefer to use the > >>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". > >>>>>>>> It's unusual to add a version number, which is for the Flexcan > >>>>>>>> on the PowerPC cores only, I assume, but there will be device > >>>>>>>> tree for ARM soon. A proper compatibility string would be > >> "fsl,p1010-flexcan" if we really need to distinguish. > >>>>>>>> > >>>>>>> [Bhaskar] About clock source.. There can be two sources of clock > >>>>>>> for > >>>>>> the CAN. > >>>>>>> Oscillator or the platform clock, but at present only > >> platform > >>>>>> clock is supported > >>>>>>> in P1010.If we remove the fsl,flexcan-clock-source property, > >> we > >>>>>> will lost the flexibility > >>>>>>> of changing the clock source .. > >>>>>>> > >>>>>>> About clock-frequency... it is also not fixed. It > >>>>>>> depends on > >>>>>> the platform clock which in turns > >>>>>>> Depends on the CCB clock. So it will be better to keep > >>>>>>> clock- > >>>>>> frequency property which is getting fixed via u-boot. > >>>>>> > >>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever > >>>>>> change? What can we expect from future Flexcan hardware? Will it > >>>>>> support further clock sources? > >>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if > >> the CCB gets changed that will be taken care by the u-boot fixup code for > >>>>> clock-frequency. clock-frequency is not filled by somebody in the > >> dts file. It will be done by u-boot. > >>>>> For clock source,I can't say right now, that's why I have kept a > >> property for this in the can node. So that in future, we need to fill it > >>>>> appropriately > >>>> > >>>> Speaking of the dts file, I have left the p1010si.dtsi file with the > >>>> fsl,flexcan-v1.0 .compatible definition. The flexcan folks (IIRC > >>>> Wolfgang) objected to that as it does not follow the standard which > >>>> should be just fsl,flexcan. > >>>> > >>>> How would you like to change that? Should I add it as part of this > >>>> patch, add another patch to the series, or let you take care of it? > >>>> > >>>> Also, I assume the uboot project will need to be changed as well to > >>>> reflect the corrected name. > >>> > >>> I think you should provide patches within this series to cleanup the > >>> obsolete stuff, dts and binding doc. > >> > >> It reads to me that the binding doc now reduces just the required > >> properties. Should I remove the file entirely? > > [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity > > about the CAN functionality > > can0@1c000 { > > compatible = "fsl,flexcan"; > > reg = <0x1c000 0x1000>; > > interrupts = <48 0x2>; > > interrupt-parent = <&mpic>; > > clock-frequency = <fixed by u-boot>; > > }; > > Yes, I also find the introduction is quite useful, with some related > correction. I am not sure what is useful. The clock source bits are all wrong. When that is removed, you end up with a discussion about the prescaler which is actually related to the flexcan.c file and has nothing to do with the device node. Maybe I am just going back into my not-communicating-well mode. Could you follow up with what you think belongs in the introduction of the binding file? Thanks, Robin -- 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 08/09/2011 04:09 PM, Robin Holt wrote: > On Tue, Aug 09, 2011 at 04:03:38PM +0200, Wolfgang Grandegger wrote: >> On 08/09/2011 03:44 PM, U Bhaskar-B22300 wrote: >>> >>> >>>> -----Original Message----- >>>> From: Robin Holt [mailto:holt@sgi.com] >>>> Sent: Tuesday, August 09, 2011 7:06 PM >>>> To: Wolfgang Grandegger >>>> Cc: Robin Holt; U Bhaskar-B22300; socketcan-core@lists.berlios.de; >>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org; Marc Kleine- >>>> Budde >>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>> >>>> On Tue, Aug 09, 2011 at 03:03:50PM +0200, Wolfgang Grandegger wrote: >>>>> On 08/09/2011 02:49 PM, Robin Holt wrote: >>>>>> On Tue, Aug 09, 2011 at 12:41:39PM +0000, U Bhaskar-B22300 wrote: >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>>>>>>> Sent: Tuesday, August 09, 2011 4:19 PM >>>>>>>> To: U Bhaskar-B22300 >>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock source. >>>>>>>> >>>>>>>> On 08/09/2011 11:27 AM, U Bhaskar-B22300 wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Wolfgang Grandegger [mailto:wg@grandegger.com] >>>>>>>>>> Sent: Tuesday, August 09, 2011 2:03 PM >>>>>>>>>> To: U Bhaskar-B22300 >>>>>>>>>> Cc: Marc Kleine-Budde; socketcan-core@lists.berlios.de; >>>>>>>>>> netdev@vger.kernel.org; Devicetree-discuss@lists.ozlabs.org >>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock >>>> source. >>>>>>>>>> >>>>>>>>>> Hi Bhaskar, >>>>>>>>>> >>>>>>>>>> On 08/09/2011 09:57 AM, U Bhaskar-B22300 wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] >>>>>>>>>>>> Sent: Tuesday, August 09, 2011 12:23 AM >>>>>>>>>>>> To: Wolfgang Grandegger >>>>>>>>>>>> Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; U >>>>>>>>>>>> Bhaskar- B22300 >>>>>>>>>>>> Subject: Re: [RFC 5/5] [powerpc] Implement a p1010rdb clock >>>> source. >>>>>>>>>>>> >>>>>>>>>>>> On 08/08/2011 05:33 PM, Wolfgang Grandegger wrote: >>>>>>>>>>>>>> ACK - The device tree bindings as in mainline's >>>>>>>>>>>>>> Documentation is a >>>>>>>>>>>> mess. >>>>>>>>>>>>>> If the powerpc guys are happy with a clock interfaces based >>>>>>>>>>>>>> approach somewhere in arch/ppc, I'm more than happy to >>>> remove: >>>>>>>>>>>>>> - fsl,flexcan-clock-source (not implemented, even in the fsl >>>>>>>>>>>>>> driver) >>>>>>>>>>> [Bhaskar]I have pushed the FlexCAN series of patches, It >>>>>>>>>>> contains the usage of all the fields posted in the FlexCAN >>>>>>>>>>> bindings at >>>>>>>>>>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.gi >>>>>>>>>>> t;a=b >>>>>>>>>>> lo >>>>>>>>>>> b;f=Documentation/devicetree/bindings/net/can/fsl-flexcan.txt;h >>>>>>>>>>> =1a72 >>>>>>>>>>> 9f >>>>>>>>>>> 089866259ef82d0db5893ff7a8c54d5ccf;hb=94ed5b4788a7cdbe68bc7cb85 >>>>>>>>>>> 16972 >>>>>>>>>>> cb >>>>>>>>>>> ebdc8274 >>>>>>>>>> >>>>>>>>>> As Marc already pointed out, Robin already has a much more >>>>>>>>>> advanced patch stack in preparation. Especially your patches do >>>>>>>>>> not care about the already existing Flexcan core on the >>>> Freescale's ARM socks. >>>>>>>>> [Bhaskar] No, the patches are taking care of the existing ARM >>>>>>>> functionality. >>>>>>>>> I have not tested on the ARM based board, but the patches are >>>>>>>>> made >>>>>>>> in a >>>>>>>>> Manner that it should not break the ARM based functionality. >>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> - fsl,flexcan-clock-divider \__ replace with code in >>>> arch/ppc, or >>>>>>>>>>>>>> - clock-frequency / a single clock-frequency >>>> attribute >>>>>>>>>>>>> >>>>>>>>>>>>> In the "net-next-2.6" tree there is also: >>>>>>>>>>>>> >>>>>>>>>>>>> $ grep flexcan arch/powerpc/boots/dts/*.dts >>>>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>>>>>>> "platform"; >>>>>>>>>>>>> p1010rdb.dts: fsl,flexcan-clock-source = >>>>>>>>>> "platform"; >>>>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- >>>> v1.0"; >>>>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = >>>> <2>; >>>>>>>>>>>>> p1010si.dtsi: compatible = "fsl,flexcan- >>>> v1.0"; >>>>>>>>>>>>> p1010si.dtsi: fsl,flexcan-clock-divider = >>>> <2>; >>>>>>>>>>>>> >>>>>>>>>>>>> Especially the fsl,flexcan-clock-divider = <2>; might make >>>>>>>>>>>>> people think, that they could set something else. >>>>>>>>>>>> >>>>>>>>>>> [Bhaskar] As it is mentioned in the Flexcan bindings, the need >>>>>>>>>>> of >>>>>>>>>> fsl,flexcan-clock-divider = <2>; >>>>>>>>>>> But I kept it as "2" because FlexCan clock source is the >>>>>>>>>> platform clock and it is CCB/2 >>>>>>>>>>> If the "2" is misleading, the bindings can be changed or >>>>>>>>>>> some >>>>>>>>>> text can be written to make the meaning of "2" >>>>>>>>>>> Understandable , Please suggest .. >>>>>>>>>> >>>>>>>>>> The clock source and frequency is fixed. Why do we need an extra >>>>>>>>>> properties for that. We have panned to remove these bogus >>>>>>>>>> bindings from the Linux kernel, which sneaked in *without* any >>>>>>>>>> review on the relevant mailing lists (at least I have not >>>>>>>>>> realized any posting). We do not think they are really needed. >>>>>>>>>> They just confuse the user. We also prefer to use the >>>>>>>>>> compatibility string "fsl,flexcan" instead "fsl,flexcan-v1.0". >>>>>>>>>> It's unusual to add a version number, which is for the Flexcan >>>>>>>>>> on the PowerPC cores only, I assume, but there will be device >>>>>>>>>> tree for ARM soon. A proper compatibility string would be >>>> "fsl,p1010-flexcan" if we really need to distinguish. >>>>>>>>>> >>>>>>>>> [Bhaskar] About clock source.. There can be two sources of clock >>>>>>>>> for >>>>>>>> the CAN. >>>>>>>>> Oscillator or the platform clock, but at present only >>>> platform >>>>>>>> clock is supported >>>>>>>>> in P1010.If we remove the fsl,flexcan-clock-source property, >>>> we >>>>>>>> will lost the flexibility >>>>>>>>> of changing the clock source .. >>>>>>>>> >>>>>>>>> About clock-frequency... it is also not fixed. It >>>>>>>>> depends on >>>>>>>> the platform clock which in turns >>>>>>>>> Depends on the CCB clock. So it will be better to keep >>>>>>>>> clock- >>>>>>>> frequency property which is getting fixed via u-boot. >>>>>>>> >>>>>>>> The frequency is fixed to CCB-frequency / 2. Will that ever >>>>>>>> change? What can we expect from future Flexcan hardware? Will it >>>>>>>> support further clock sources? >>>>>>> [Bhaskar] Yes the frequency will always be CCB-frequency/2.Even if >>>> the CCB gets changed that will be taken care by the u-boot fixup code for >>>>>>> clock-frequency. clock-frequency is not filled by somebody in the >>>> dts file. It will be done by u-boot. >>>>>>> For clock source,I can't say right now, that's why I have kept a >>>> property for this in the can node. So that in future, we need to fill it >>>>>>> appropriately >>>>>> >>>>>> Speaking of the dts file, I have left the p1010si.dtsi file with the >>>>>> fsl,flexcan-v1.0 .compatible definition. The flexcan folks (IIRC >>>>>> Wolfgang) objected to that as it does not follow the standard which >>>>>> should be just fsl,flexcan. >>>>>> >>>>>> How would you like to change that? Should I add it as part of this >>>>>> patch, add another patch to the series, or let you take care of it? >>>>>> >>>>>> Also, I assume the uboot project will need to be changed as well to >>>>>> reflect the corrected name. >>>>> >>>>> I think you should provide patches within this series to cleanup the >>>>> obsolete stuff, dts and binding doc. >>>> >>>> It reads to me that the binding doc now reduces just the required >>>> properties. Should I remove the file entirely? >>> [Bhaskar] I think the binding doc should atleast be present with the required properties to give the clarity >>> about the CAN functionality >>> can0@1c000 { >>> compatible = "fsl,flexcan"; >>> reg = <0x1c000 0x1000>; >>> interrupts = <48 0x2>; >>> interrupt-parent = <&mpic>; >>> clock-frequency = <fixed by u-boot>; >>> }; >> >> Yes, I also find the introduction is quite useful, with some related >> correction. > > I am not sure what is useful. The clock source bits are all wrong. > When that is removed, you end up with a discussion about the prescaler > which is actually related to the flexcan.c file and has nothing > to do with the device node. Maybe I am just going back into my > not-communicating-well mode. Could you follow up with what you think > belongs in the introduction of the binding file? Yep, you are right. Keep it simple... Wolfgang, -- 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/arch/powerpc/platforms/85xx/p1010rdb.c b/arch/powerpc/platforms/85xx/p1010rdb.c index 3540a88..8f78ddd 100644 --- a/arch/powerpc/platforms/85xx/p1010rdb.c +++ b/arch/powerpc/platforms/85xx/p1010rdb.c @@ -28,6 +28,7 @@ #include <asm/udbg.h> #include <asm/mpic.h> #include <asm/swiotlb.h> +#include <asm/clk_interface.h> #include <sysdev/fsl_soc.h> #include <sysdev/fsl_pci.h> @@ -164,6 +165,82 @@ static void __init p1010_rdb_setup_arch(void) printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n"); } +/* + * p1010rdb needs to provide a clock source for the flexcan driver. + */ +struct clk { + unsigned long rate; +} p1010rdb_system_clk; + +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id) +{ + struct clk *clk; + u32 *of_property; + unsigned long clock_freq, clock_divider; + const char *dev_init_name; + + if (!dev) + return ERR_PTR(-ENOENT); + + /* + * The can devices are named ffe1c000.can0 and ffe1d000.can1 on + * the p1010rdb. Check for the "can" portion of that name before + * returning a clock source. + */ + dev_init_name = dev_name(dev); + if (strlen(dev_init_name) != 13) + return ERR_PTR(-ENOENT); + dev_init_name += 9; + if (strncmp(dev_init_name, "can", 3)) + return ERR_PTR(-ENOENT); + + of_property = (u32 *)of_get_property(dev->of_node, "clock_freq", NULL); + if (!of_property) + return ERR_PTR(-ENOENT); + clock_freq = *of_property; + + of_property = (u32 *)of_get_property(dev->of_node, + "fsl,flexcan-clock-divider", NULL); + if (!of_property) + return ERR_PTR(-ENOENT); + clock_divider = *of_property; + + clk = kmalloc(sizeof(struct clk), GFP_KERNEL); + if (!clk) + return ERR_PTR(-ENOMEM); + + clk->rate = DIV_ROUND_CLOSEST(clock_freq / clock_divider, 1000); + clk->rate *= 1000; + + return clk; +} + +static void p1010_rdb_clk_put(struct clk *clk) +{ + kfree(clk); +} + +static unsigned long p1010_rdb_clk_get_rate(struct clk *clk) +{ + return clk->rate; +} + +static struct clk_interface p1010_rdb_clk_functions = { + .clk_get = p1010_rdb_clk_get, + .clk_get_rate = p1010_rdb_clk_get_rate, + .clk_put = p1010_rdb_clk_put, +}; + +static void __init p1010_rdb_clk_init(void) +{ + clk_functions = p1010_rdb_clk_functions; +} + +static void __init p1010_rdb_init(void) +{ + p1010_rdb_clk_init(); +} + static struct of_device_id __initdata p1010rdb_ids[] = { { .type = "soc", }, { .compatible = "soc", }, @@ -195,6 +272,7 @@ define_machine(p1010_rdb) { .name = "P1010 RDB", .probe = p1010_rdb_probe, .setup_arch = p1010_rdb_setup_arch, + .init = p1010_rdb_init, .init_IRQ = p1010_rdb_pic_init, #ifdef CONFIG_PCI .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
flexcan driver needs the clk_get, clk_get_rate, etc functions to work. This patch provides the minimum functionality. Signed-off-by: Robin Holt <holt@sgi.com> To: Marc Kleine-Budde <mkl@pengutronix.de> To: Wolfgang Grandegger <wg@grandegger.com> To: U Bhaskar-B22300 <B22300@freescale.com> Cc: socketcan-core@lists.berlios.de Cc: netdev@vger.kernel.org --- arch/powerpc/platforms/85xx/p1010rdb.c | 78 ++++++++++++++++++++++++++++++++ 1 files changed, 78 insertions(+), 0 deletions(-)