On Thu, Aug 04, 2011 at 10:21:50PM +0200, Marc Kleine-Budde wrote:
> On 08/04/2011 09:15 PM, Robin Holt wrote:
> > The freescale P1010RDB board does not have a
> > clk_{get,put,get_rate,enable,disable} set of functions. Wrap these with a
> > flexcan_ #define for arm, and implement a more complete function for ppc.
>
> Powerpc has these functions, too....wonder if they do the expected :)
On the p1010, I think the clock source for the CAN interface can be
different from the system clock. In the 'p1010rdb.dts' file which
describes the can interface, there is a separate entry for the
clock-source and clock-divider:
can0@1c000 {
compatible = "fsl,flexcan-v1.0";
reg = <0x1c000 0x1000>;
interrupts = <48 0x2>;
interrupt-parent = <&mpic>;
fsl,flexcan-clock-source = <1>;
fsl,flexcan-clock-divider = <2>;
};
can1@1d000 {
compatible = "fsl,flexcan-v1.0";
reg = <0x1d000 0x1000>;
interrupts = <61 0x2>;
interrupt-parent = <&mpic>;
fsl,flexcan-clock-source = <1>;
fsl,flexcan-clock-divider = <2>;
};
To me, that reads like this is the right way to handle the clock for the
p1010 processor. I am not sure that has any correlation to __powerpc__,
but it is the best I can do with my very limited knowledge.
I will address the rest of your comments in a bit. It seems like
most of them are fallout from my quick cut-and-paste of the freescale
flexcan_iface.c file.
Thanks,
Robin
>
> I should have added: more comments inline :)....
>
> > Signed-off-by: Robin Holt <[email protected]>
> > To: Marc Kleine-Budde <[email protected]>
> > To: Wolfgang Grandegger <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/net/can/flexcan.c | 90
> > ++++++++++++++++++++++++++++++++++++++++----
> > 1 files changed, 81 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 75e4d9c..5fd1d37 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -220,6 +220,78 @@ static inline void flexcan_write(u32 val, void __iomem
> > *addr)
> > }
> > #endif
> >
> > +#if defined(__powerpc__)
> > +struct flexcan_clk {
> > + unsigned long rate;
> > + void *data;
>
> no indenten here, please
>
> > +};
> > +
> > +#define DIV_ROUND(n, d) (((n) + ((d)/2)) / (d))
>
> Have a look at kernel.h, maybe there is already a definition line this.
>
> > +
> > +static struct clk *flexcan_clk_get(struct device *dev, const char *id)
> > +{
> > + struct flexcan_clk *clock;
> > + u32 *clock_freq = NULL;
> > + u32 *clock_divider = NULL;
>
> I think these initialisations are not needed.
>
> > + int err;
> > +
> > + clock = kmalloc(sizeof(struct flexcan_clk), GFP_KERNEL);
> > + if (!clock) {
> > + dev_err(dev, "Cannot allocate memory\n");
> > + err = -ENOMEM;
> > + goto failed_clock;
> > + }
> > + clock_freq = (u32 *) of_get_property(dev->of_node, "clock_freq", NULL);
> ^
> please remove
>
> > + if (clock_freq == NULL) {
> !clock_freq
> > + dev_err(dev, "Cannot find clock_freq property\n");
> > + err = -EINVAL;
> > + kfree(clock);
>
> please move the kfree at the end of the function
>
> > + goto failed_clock;
> > + }
> > +
> > + clock_divider = (u32 *) of_get_property(dev->of_node, \
> > + "fsl,flexcan-clock-divider", NULL);
>
> no '\' at the end of the line. We're coding C here not shell :)
>
> > + if (clock_divider == NULL) {
> dito
> > + dev_err(dev, "Cannot find fsl,flexcan-clock-divider \
> > + property\n");
> dito
> > + err = -EINVAL;
> > + kfree(clock);
> dito
> > + goto failed_clock;
> > + }
> > +
> > + clock->rate = DIV_ROUND(*clock_freq / *clock_divider, 1000) * 1000;
> > +
> > + return (struct clk *)clock;
> > +
> > + failed_clock:
> > + return ERR_PTR(err);
> > +}
> > +
> > +static void flexcan_clk_put(struct clk *_clk)
> > +{
> > + struct flexcan_clk *clk = (struct flexcan_clk *)_clk;
> > +
> > + kfree(clk);
> > +}
> > +
> > +static unsigned long flexcan_clk_get_rate(struct clk *_clk)
> > +{
> > + struct flexcan_clk *clk = (struct flexcan_clk *)_clk;
> > +
> > + return clk->rate;
> > +}
> > +
> > +#define flexcan_clk_enable(_p) do {} while (0)
> > +#define flexcan_clk_disable(_p) do {} while (0)
>
> No need for clock handling on ppc?
> If so make it static inline functions, please
>
> > +
> > +#else
> > +#define flexcan_clk_get_rate clk_get_rate
> > +#define flexcan_clk_put clk_put
> > +#define flexcan_clk_get clk_get
> > +#define flexcan_clk_enable clk_enable
> > +#define flexcan_clk_disable clk_disable
>
> static inlines, please
>
> > +#endif
> > +
> > /*
> > * Swtich transceiver on or off
> > */
> > @@ -807,7 +879,7 @@ static int flexcan_open(struct net_device *dev)
> > struct flexcan_priv *priv = netdev_priv(dev);
> > int err;
> >
> > - clk_enable(priv->clk);
> > + flexcan_clk_enable(priv->clk);
> >
> > err = open_candev(dev);
> > if (err)
> > @@ -829,7 +901,7 @@ static int flexcan_open(struct net_device *dev)
> > out_close:
> > close_candev(dev);
> > out:
> > - clk_disable(priv->clk);
> > + flexcan_clk_disable(priv->clk);
> >
> > return err;
> > }
> > @@ -843,7 +915,7 @@ static int flexcan_close(struct net_device *dev)
> > flexcan_chip_stop(dev);
> >
> > free_irq(dev->irq, dev);
> > - clk_disable(priv->clk);
> > + flexcan_clk_disable(priv->clk);
> >
> > close_candev(dev);
> >
> > @@ -882,7 +954,7 @@ static int __devinit register_flexcandev(struct
> > net_device *dev)
> > struct flexcan_regs __iomem *regs = priv->base;
> > u32 reg, err;
> >
> > - clk_enable(priv->clk);
> > + flexcan_clk_enable(priv->clk);
> >
> > /* select "bus clock", chip must be disabled */
> > flexcan_chip_disable(priv);
> > @@ -916,7 +988,7 @@ static int __devinit register_flexcandev(struct
> > net_device *dev)
> > out:
> > /* disable core and turn off clocks */
> > flexcan_chip_disable(priv);
> > - clk_disable(priv->clk);
> > + flexcan_clk_disable(priv->clk);
> >
> > return err;
> > }
> > @@ -936,7 +1008,7 @@ static int __devinit flexcan_probe(struct
> > platform_device *pdev)
> > resource_size_t mem_size;
> > int err, irq;
> >
> > - clk = clk_get(&pdev->dev, NULL);
> > + clk = flexcan_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(clk)) {
> > dev_err(&pdev->dev, "no clock defined\n");
> > err = PTR_ERR(clk);
> > @@ -973,7 +1045,7 @@ static int __devinit flexcan_probe(struct
> > platform_device *pdev)
> > dev->flags |= IFF_ECHO; /* we support local echo in hardware */
> >
> > priv = netdev_priv(dev);
> > - priv->can.clock.freq = clk_get_rate(clk);
> > + priv->can.clock.freq = flexcan_clk_get_rate(clk);
> > priv->can.bittiming_const = &flexcan_bittiming_const;
> > priv->can.do_set_mode = flexcan_set_mode;
> > priv->can.do_get_berr_counter = flexcan_get_berr_counter;
> > @@ -1008,7 +1080,7 @@ static int __devinit flexcan_probe(struct
> > platform_device *pdev)
> > failed_map:
> > release_mem_region(mem->start, mem_size);
> > failed_get:
> > - clk_put(clk);
> > + flexcan_clk_put(clk);
> > failed_clock:
> > return err;
> > }
> > @@ -1026,7 +1098,7 @@ static int __devexit flexcan_remove(struct
> > platform_device *pdev)
> > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > release_mem_region(mem->start, resource_size(mem));
> >
> > - clk_put(priv->clk);
> > + flexcan_clk_put(priv->clk);
> >
> > free_candev(dev);
> >
>
> 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 |
>
>
>
_______________________________________________
Socketcan-users mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-users
