Re: [net-next PATCH 3/4] netdev: replace napi_reschedule with napi_schedule
On 03.10.2023 13:18:33, Christian Marangi wrote:
> On Tue, Oct 03, 2023 at 09:16:33AM +0200, Marc Kleine-Budde wrote:
> > On 02.10.2023 17:10:22, Christian Marangi wrote:
> > > Now that napi_schedule return a bool, we can drop napi_reschedule that
> > > does the same exact function. The function comes from a very old commit
> > > bfe13f54f502 ("ibm_emac: Convert to use napi_struct independent of struct
> > > net_device") and the purpose is actually deprecated in favour of
> > > different logic.
> > >
> > > Convert every user of napi_reschedule to napi_schedule.
> > >
> > > Signed-off-by: Christian Marangi
> > > ---
> > > drivers/infiniband/ulp/ipoib/ipoib_ib.c | 4 ++--
> > > drivers/net/can/dev/rx-offload.c | 2 +-
> >
> > Acked-by: Marc Kleine-Budde # for can/dev/rx-offload.c
>
> Just to make sure can I use the correct tag: (you didn't include the
> mail)
Doh! Sure.
> Acked-by: Marc Kleine-Budde # for can/dev/rx-offload.c
Acked-by: Marc Kleine-Budde # for can/dev/rx-offload.c
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
signature.asc
Description: PGP signature
Re: [net-next PATCH 3/4] netdev: replace napi_reschedule with napi_schedule
On 02.10.2023 17:10:22, Christian Marangi wrote:
> Now that napi_schedule return a bool, we can drop napi_reschedule that
> does the same exact function. The function comes from a very old commit
> bfe13f54f502 ("ibm_emac: Convert to use napi_struct independent of struct
> net_device") and the purpose is actually deprecated in favour of
> different logic.
>
> Convert every user of napi_reschedule to napi_schedule.
>
> Signed-off-by: Christian Marangi
> ---
> drivers/infiniband/ulp/ipoib/ipoib_ib.c| 4 ++--
> drivers/net/can/dev/rx-offload.c | 2 +-
Acked-by: Marc Kleine-Budde # for can/dev/rx-offload.c
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
signature.asc
Description: PGP signature
Re: [PATCH] net: Explicitly include correct DT includes
On 14.07.2023 11:48:00, Rob Herring wrote: > The DT of_device.h and of_platform.h date back to the separate > of_platform_bus_type before it as merged into the regular platform bus. > As part of that merge prepping Arm DT support 13 years ago, they > "temporarily" include each other. They also include platform_device.h > and of.h. As a result, there's a pretty much random mix of those include > files used throughout the tree. In order to detangle these headers and > replace the implicit includes with struct declarations, users need to > explicitly include the correct includes. > > Signed-off-by: Rob Herring > --- > drivers/net/can/bxcan.c | 1 - > drivers/net/can/ifi_canfd/ifi_canfd.c | 1 - > drivers/net/can/m_can/m_can.c | 1 - > drivers/net/can/m_can/m_can.h | 1 - > drivers/net/can/rcar/rcar_canfd.c | 1 - > drivers/net/can/sja1000/sja1000_platform.c | 1 - > drivers/net/can/sun4i_can.c | 1 - > drivers/net/can/ti_hecc.c | 1 - Acked-by: Marc Kleine-Budde # for drivers/net/can regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | signature.asc Description: PGP signature
Re: [PATCH] net: Use of_property_read_bool() for boolean properties
On 10.03.2023 08:47:16, Rob Herring wrote: > It is preferred to use typed property access functions (i.e. > of_property_read_ functions) rather than low-level > of_get_property/of_find_property functions for reading properties. > Convert reading boolean properties to to of_property_read_bool(). > > Signed-off-by: Rob Herring > --- > drivers/net/can/cc770/cc770_platform.c | 12 ++-- Acked-by: Marc Kleine-Budde # for net/can regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: PGP signature
Re: [PATCH v1 2/4] powerpc/mpc5xxx: Switch mpc5xxx_get_bus_frequency() to use fwnode
On 04.05.2022 16:44:47, Andy Shevchenko wrote: > Switch mpc5xxx_get_bus_frequency() to use fwnode in order to help > cleaning up other parts of the kernel from OF specific code. > > No functional change intended. > > Signed-off-by: Andy Shevchenko > --- > arch/powerpc/include/asm/mpc5xxx.h| 9 +++- > arch/powerpc/platforms/52xx/mpc52xx_gpt.c | 2 +- > arch/powerpc/sysdev/mpc5xxx_clocks.c | 41 ++- > drivers/ata/pata_mpc52xx.c| 2 +- > drivers/i2c/busses/i2c-mpc.c | 7 ++-- > drivers/net/can/mscan/mpc5xxx_can.c | 2 +- > drivers/net/ethernet/freescale/fec_mpc52xx.c | 2 +- > .../net/ethernet/freescale/fec_mpc52xx_phy.c | 3 +- > .../net/ethernet/freescale/fs_enet/mii-fec.c | 4 +- > drivers/spi/spi-mpc52xx.c | 2 +- > drivers/tty/serial/mpc52xx_uart.c | 4 +- > 11 files changed, 44 insertions(+), 34 deletions(-) Acked-by: Marc Kleine-Budde # for mscan/mpc5xxx_can regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: PGP signature
Re: [PATCH v6] flexcan: add err_irq handler for flexcan
On 07/03/2014 11:22 AM, Zhao Qiang wrote:
> when flexcan is not physically linked, command 'cantest' will
> trigger an err_irq, add err_irq handler for it.
>
> Signed-off-by: Zhao Qiang
> ---
> Changes for v2:
> - use a space instead of tab
> - use flexcan_poll_state instead of print
> Changes for v3:
> - return IRQ_HANDLED if err is triggered
> - stop transmitted packets when there is an err_interrupt
> Changes for v4:
> - call flexcan_irq
> Changes for v5:
> - move err_int_handling code from flexcan_irq to flexcan_err_irq
> - call flexcan_err_irq from flexcan_irq
> Changes for v6:
> - move RX_IRQ handling back to flexcan_irq
>
> drivers/net/can/flexcan.c | 52
> +--
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..f6c92bc 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -208,6 +208,7 @@ struct flexcan_priv {
> void __iomem *base;
> u32 reg_esr;
> u32 reg_ctrl_default;
> + int err_irq;
>
> struct clk *clk_ipg;
> struct clk *clk_per;
> @@ -690,6 +691,28 @@ static int flexcan_poll(struct napi_struct *napi, int
> quota)
> return work_done;
> }
>
> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 reg_ctrl, reg_esr;
> + irqreturn_t ret = IRQ_NONE;
> +
> + reg_esr = flexcan_read(®s->esr);
> + reg_ctrl = flexcan_read(®s->ctrl);
> +
> + if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> + flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, ®s->esr);
> + ret = IRQ_HANDLED;
> + }
> +
> + if (reg_esr & FLEXCAN_ESR_ERR_INT)
> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
Now you just ACK the interrupts, but you're not triggering the NAPI,
where the actual error interrupts are handled.
In the current interrupt handler we are looking at two registers for
interrupts, that is "iflag1" and "esr". "iflag1" is triggered on RX,
RX-FIFO overflow and TX-complete. "esr" is triggered by errors in
general. In the interrupt handler we distinguish between bus errors
(FLEXCAN_ESR_ERR_BUS) and errors effecting a change of state
(FLEXCAN_ESR_ERR_STATE).
The next step is to figure out which bits in which register trigger the
error irq. Then move the code handling this register into the error irq
handler. As the seconds half of the error irq handling takes place in
the napi function, you have to duplicate the napi_schedule(&priv->napi).
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5] flexcan: add err_irq handler for flexcan
On 07/03/2014 04:23 AM, Zhao Qiang wrote: > when flexcan is not physically linked, command 'cantest' will > trigger an err_irq, add err_irq handler for it. > > Signed-off-by: Zhao Qiang > --- > Changes for v2: > - use a space instead of tab > - use flexcan_poll_state instead of print > Changes for v3: > - return IRQ_HANDLED if err is triggered > - stop transmitted packets when there is an err_interrupt > Changes for v4: > - call flexcan_irq > Changes for v5: > - move err_int_handling code from flexcan_irq to flexcan_err_irq > - call flexcan_err_irq from flexcan_irq Why do you move the RX IRQ handling into the error IRQ function? If I understand you correctly you only want to do error interrupt handling the error IRQ function, right? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4] flexcan: add err_irq handler for flexcan
On 07/02/2014 10:32 AM, qiang.z...@freescale.com wrote:
> On 07/02/2014 03:04 PM, Marc Kleine-Budde wrote:
>> -Original Message-
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Wednesday, July 02, 2014 3:04 PM
>> To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; w...@grandegger.com;
>> linux-...@vger.kernel.org; Wood Scott-B07421
>> Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan
>>
>> On 07/02/2014 04:00 AM, qiang.z...@freescale.com wrote:
>>>>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) {
>>>>> + struct net_device *dev = dev_id;
>>>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>>>> + struct flexcan_regs __iomem *regs = priv->base;
>>>>> + u32 reg_ctrl, reg_esr;
>>>>> +
>>>>> + reg_esr = flexcan_read(®s->esr);
>>>>> + reg_ctrl = flexcan_read(®s->ctrl);
>>>>> +
>>>>> + if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>>>>> + if (reg_esr & FLEXCAN_ESR_ERR_INT)
>>>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
>>>>> + ®s->ctrl);
>>>>> + flexcan_irq(irq, dev);
>>>>
>>>> I still don't understand why you need a special flexcan_err_irq()
>>>> function. Why don't you just call:
>>>>
>>>> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>
>>>> instead?
>>>
>>> Flexcan_irq is for flexcan normal interrupt(such as Message Buffer,
>> Wake up and so on).
>>> And it will return IRQ_HANDLED if flexcan_irq is triggered.
>>> But err_irq is shared with other devices, it should return IRQ_HANDLED
>>> when the interrupt is triggered by flexcan device, if not return
>> IRQ_NONE.
>>
>> What about fixing flexcan_irq() first and the make use of it?
>
> Err_irq is a shared interrupt with other device,
> I hope that its handler is independent.
> However, if you persist in your opinion, I will do it as you said.
There is another option, you can move all of the error interrupt
handling code from flexcan_irq() to flexcan_irq_err(). To keep the ARM
SoCs supported, you need to call flexcan_irq_err() form the
flexcan_irq() handler.
What I don't want is a) code duplication and b) no fishy wrapper
functions around flexcan_irq() that work around flexcan_irq() returning
IRQ_HANDLED unconditionally.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4] flexcan: add err_irq handler for flexcan
On 07/02/2014 04:00 AM, qiang.z...@freescale.com wrote:
>>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) {
>>> + struct net_device *dev = dev_id;
>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>> + struct flexcan_regs __iomem *regs = priv->base;
>>> + u32 reg_ctrl, reg_esr;
>>> +
>>> + reg_esr = flexcan_read(®s->esr);
>>> + reg_ctrl = flexcan_read(®s->ctrl);
>>> +
>>> + if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>>> + if (reg_esr & FLEXCAN_ESR_ERR_INT)
>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
>>> + ®s->ctrl);
>>> + flexcan_irq(irq, dev);
>>
>> I still don't understand why you need a special flexcan_err_irq()
>> function. Why don't you just call:
>>
>> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>
>> instead?
>
> Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, Wake up
> and so on).
> And it will return IRQ_HANDLED if flexcan_irq is triggered.
> But err_irq is shared with other devices, it should return IRQ_HANDLED when
> the interrupt
> is triggered by flexcan device, if not return IRQ_NONE.
What about fixing flexcan_irq() first and the make use of it?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4] flexcan: add err_irq handler for flexcan
On 07/01/2014 10:03 AM, Zhao Qiang wrote:
> when flexcan is not physically linked, command 'cantest' will
> trigger an err_irq, add err_irq handler for it.
>
> Signed-off-by: Zhao Qiang
> ---
> Changes for v2:
> - use a space instead of tab
> - use flexcan_poll_state instead of print
> Changes for v3:
> - return IRQ_HANDLED if err is triggered
> - stop transmitted packets when there is an err_interrupt
> Changes for v4:
> - call flexcan_irq
>
> drivers/net/can/flexcan.c | 38 +-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..098fcac 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -208,6 +208,7 @@ struct flexcan_priv {
> void __iomem *base;
> u32 reg_esr;
> u32 reg_ctrl_default;
> + int err_irq;
>
> struct clk *clk_ipg;
> struct clk *clk_per;
> @@ -744,6 +745,27 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 reg_ctrl, reg_esr;
> +
> + reg_esr = flexcan_read(®s->esr);
> + reg_ctrl = flexcan_read(®s->ctrl);
> +
> + if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> + if (reg_esr & FLEXCAN_ESR_ERR_INT)
> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> + ®s->ctrl);
> + flexcan_irq(irq, dev);
I still don't understand why you need a special flexcan_err_irq()
function. Why don't you just call:
request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
instead?
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> static void flexcan_set_bittiming(struct net_device *dev)
> {
> const struct flexcan_priv *priv = netdev_priv(dev);
> @@ -944,6 +966,15 @@ static int flexcan_open(struct net_device *dev)
> if (err)
> goto out_close;
>
> + if (priv->err_irq) {
> + err = request_irq(priv->err_irq, flexcan_err_irq, IRQF_SHARED,
> + dev->name, dev);
> + if (err) {
> + free_irq(priv->err_irq, dev);
Why do you want to free the err_irq, if requesting the err_irq fails?
However you should adjust all error cleanup path following this request
irq, so that it will be freed.
BTW: where is the corresponding free_irq() in flexcan_close()?
> + goto out_free_irq;
> + }
> + }
> +
> /* start chip and queuing */
> err = flexcan_chip_start(dev);
> if (err)
> @@ -1099,7 +1130,7 @@ static int flexcan_probe(struct platform_device *pdev)
> struct resource *mem;
> struct clk *clk_ipg = NULL, *clk_per = NULL;
> void __iomem *base;
> - int err, irq;
> + int err, irq, err_irq;
> u32 clock_freq = 0;
>
> if (pdev->dev.of_node)
> @@ -1126,6 +1157,10 @@ static int flexcan_probe(struct platform_device *pdev)
> if (irq <= 0)
> return -ENODEV;
>
> + err_irq = platform_get_irq(pdev, 1);
> + if (err_irq <= 0)
> + err_irq = 0;
> +
> base = devm_ioremap_resource(&pdev->dev, mem);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -1149,6 +1184,7 @@ static int flexcan_probe(struct platform_device *pdev)
> dev->flags |= IFF_ECHO;
>
> priv = netdev_priv(dev);
> + priv->err_irq = err_irq;
> priv->can.clock.freq = clock_freq;
> priv->can.bittiming_const = &flexcan_bittiming_const;
> priv->can.do_set_mode = flexcan_set_mode;
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/2] flexcan: add err_irq handler for flexcan
On 06/23/2014 10:15 AM, qiang.z...@freescale.com wrote:
[...]
>>>>> + reg_esr = flexcan_read(®s->esr);
>>>>> + reg_ctrl = flexcan_read(®s->ctrl);
>>>>> + if (reg_esr & FLEXCAN_ESR_TX_WRN) {
>>>>
>>>> When does the hardware trigger the interrupt?
>>>
>>> When there is no wire link between tx and rx, tx start transfer and
>> doesn’t get the ack.
>>
>> You are testing for the warning interrupt, not for the
>> FLEXCAN_ESR_ACK_ERR (which is triggered there isn't any ACK).
>>
>>>>> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr);
>>>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
>>>>> + netif_stop_queue(dev);
>>>>
>>>> Why are you stopping the txqueue?
>>>
>>> There is no wire link, tx can't transfer successfully.
>>
>> You are testing for the warning interrupt, which is triggered if the
>> error counter increases from 95 to 96. And the error counter can increase
>> due to several reasons. No link is only one of them. If the CAN core
>> cannot transmit new packages any more the flow control in the driver will
>> take care.
>
> When Tx error counter increases from 95 to 96, there must be issue for tx,
> So why can't I stop the txqueue?
Why do you want to stop the queue? It's compliant with the CAN spec to
keep sending CAN frames if the error counter increases to 96. If there
isn't any problem with the CAN bus anymore, the TX error counters will
decrease with every successfully transmitted CAN frame.
> You said that there are several reasons, would you like to take some examples?
See CAN Error Confinement Rules in
http://www.can-wiki.info/doku.php?id=can_faq_erors
>> What about calling the normal interrupt if er err_irq occurs, as this
>> function will take care of both normal and error interrupts anyway?
>
> Calling the normal interrupt doesn't work.
Why?
flexcan_irq() if FLEXCAN_CTRL_TWRN_MSK is set and will schedule the NAPI
routine:
> if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
> (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
> flexcan_has_and_handle_berr(priv, reg_esr)) {
[...]
> napi_schedule(&priv->napi);
> }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/2] flexcan: add err_irq handler for flexcan
On 06/23/2014 09:26 AM, qiang.z...@freescale.com wrote:
>
> On 06/23/2014 03:18 PM, Marc Kleine-Budde wrote:
>
>>
>> On 06/23/2014 09:11 AM, Zhao Qiang wrote:
>>> when flexcan is not physically linked, command 'cantest' will trigger
>>> an err_irq, add err_irq handler for it.
>>>
>>> Signed-off-by: Zhao Qiang
>>> ---
>>> Changes for v2:
>>> - use a space instead of tab
>>> - use flexcan_poll_state instead of print Changes for v3:
>>> - return IRQ_HANDLED if err is triggered
>>> - stop transmitted packets when there is an err_interrupt
>>>
>>> drivers/net/can/flexcan.c | 35 ++-
>>> 1 file changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index f425ec2..6802a25 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -208,6 +208,7 @@ struct flexcan_priv {
>>> void __iomem *base;
>>> u32 reg_esr;
>>> u32 reg_ctrl_default;
>>> + unsigned int err_irq;
>>>
>>> struct clk *clk_ipg;
>>> struct clk *clk_per;
>>> @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void
>> *dev_id)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) {
>>> + struct net_device *dev = dev_id;
>>> + struct flexcan_priv *priv = netdev_priv(dev);
>>> + struct flexcan_regs __iomem *regs = priv->base;
>>> + u32 reg_ctrl, reg_esr;
>>> +
>>> + reg_esr = flexcan_read(®s->esr);
>>> + reg_ctrl = flexcan_read(®s->ctrl);
>>> + if (reg_esr & FLEXCAN_ESR_TX_WRN) {
>>
>> When does the hardware trigger the interrupt?
>
> When there is no wire link between tx and rx, tx start transfer and doesn’t
> get the ack.
You are testing for the warning interrupt, not for the
FLEXCAN_ESR_ACK_ERR (which is triggered there isn't any ACK).
>>> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr);
>>> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
>>> + netif_stop_queue(dev);
>>
>> Why are you stopping the txqueue?
>
> There is no wire link, tx can't transfer successfully.
You are testing for the warning interrupt, which is triggered if the
error counter increases from 95 to 96. And the error counter can
increase due to several reasons. No link is only one of them. If the CAN
core cannot transmit new packages any more the flow control in the
driver will take care.
What about calling the normal interrupt if er err_irq occurs, as this
function will take care of both normal and error interrupts anyway?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/2] flexcan: add err_irq handler for flexcan
On 06/23/2014 09:11 AM, Zhao Qiang wrote:
> when flexcan is not physically linked, command 'cantest' will
> trigger an err_irq, add err_irq handler for it.
>
> Signed-off-by: Zhao Qiang
> ---
> Changes for v2:
> - use a space instead of tab
> - use flexcan_poll_state instead of print
> Changes for v3:
> - return IRQ_HANDLED if err is triggered
> - stop transmitted packets when there is an err_interrupt
>
> drivers/net/can/flexcan.c | 35 ++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..6802a25 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -208,6 +208,7 @@ struct flexcan_priv {
> void __iomem *base;
> u32 reg_esr;
> u32 reg_ctrl_default;
> + unsigned int err_irq;
>
> struct clk *clk_ipg;
> struct clk *clk_per;
> @@ -744,6 +745,24 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 reg_ctrl, reg_esr;
> +
> + reg_esr = flexcan_read(®s->esr);
> + reg_ctrl = flexcan_read(®s->ctrl);
> + if (reg_esr & FLEXCAN_ESR_TX_WRN) {
When does the hardware trigger the interrupt?
> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr);
> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
> + netif_stop_queue(dev);
Why are you stopping the txqueue?
> + return IRQ_HANDLED;
> + }
> + return IRQ_NONE;
> +}
> +
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
On 06/20/2014 04:01 AM, Zhao Qiang wrote:
> when flexcan is not physically linked, command 'cantest' will
> trigger an err_irq, add err_irq handler for it.
>
> Signed-off-by: Zhao Qiang
> ---
> Changes for v2:
> - use a space instead of tab
> - use flexcan_poll_state instead of print
>
> drivers/net/can/flexcan.c | 31 ++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f425ec2..7432ba4 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -208,6 +208,7 @@ struct flexcan_priv {
> void __iomem *base;
> u32 reg_esr;
> u32 reg_ctrl_default;
> + unsigned int err_irq;
>
> struct clk *clk_ipg;
> struct clk *clk_per;
> @@ -744,6 +745,23 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 reg_ctrl, reg_esr;
> +
> + reg_esr = flexcan_read(®s->esr);
> + reg_ctrl = flexcan_read(®s->ctrl);
> + if (reg_esr & FLEXCAN_ESR_TX_WRN) {
When is the err_irq triggered?
> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr);
> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
> + flexcan_poll_state(dev, reg_esr);
poll_state() is does not work, if called from an IRQ
handler.Depending when err_irq is triggered, is it worth to just
call the normal interrupt handler?
> + }
> + return IRQ_HANDLED;
Please return IRQ_HANDLED, only if there really was an interrupt.
> +}
> +
> static void flexcan_set_bittiming(struct net_device *dev)
> {
> const struct flexcan_priv *priv = netdev_priv(dev);
> @@ -944,6 +962,12 @@ static int flexcan_open(struct net_device *dev)
> if (err)
> goto out_close;
>
> + if (priv->err_irq)
> + err = request_irq(priv->err_irq, flexcan_err_irq, IRQF_SHARED,
> + dev->name, dev);
> + if (err)
> + goto out_close;
In case of an error, you don't free() dev->irq.
> +
> /* start chip and queuing */
> err = flexcan_chip_start(dev);
> if (err)
> @@ -1099,7 +1123,7 @@ static int flexcan_probe(struct platform_device *pdev)
> struct resource *mem;
> struct clk *clk_ipg = NULL, *clk_per = NULL;
> void __iomem *base;
> - int err, irq;
> + int err, irq, err_irq;
> u32 clock_freq = 0;
>
> if (pdev->dev.of_node)
> @@ -1126,6 +1150,10 @@ static int flexcan_probe(struct platform_device *pdev)
> if (irq <= 0)
> return -ENODEV;
>
> + err_irq = platform_get_irq(pdev, 1);
> + if (err_irq <= 0)
> + err_irq = 0;
> +
> base = devm_ioremap_resource(&pdev->dev, mem);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -1149,6 +1177,7 @@ static int flexcan_probe(struct platform_device *pdev)
> dev->flags |= IFF_ECHO;
>
> priv = netdev_priv(dev);
> + priv->err_irq = err_irq;
> priv->can.clock.freq = clock_freq;
> priv->can.bittiming_const = &flexcan_bittiming_const;
> priv->can.do_set_mode = flexcan_set_mode;
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] flexcan: add err interrupt for p1010rdb
On 06/20/2014 04:01 AM, Zhao Qiang wrote: > add err interrupt for p1010rdb into dts. > > Signed-off-by: Zhao Qiang > --- > Changes for v2: > - add binding documentation update > > Documentation/devicetree/bindings/net/can/fsl-flexcan.txt | 7 +-- > arch/powerpc/boot/dts/fsl/p1010si-post.dtsi | 6 -- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > index 56d6cc3..81929e5 100644 > --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > @@ -10,7 +10,9 @@ Required properties: >- fsl,p1010-flexcan > > - reg : Offset and length of the register set for this device > -- interrupts : Interrupt tuple for this device > +- interrupts : Interrupt tuple for this device. > + The first interrupt is for FlexCAN(Message Buffer and Wake Up) > + The second is for error(Shared with IFC, PEX1 and some other device) The second interrupt is optional, at least on ARM we don't need it, please reflect this in the documentation update. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] flexcan: add err_irq handler for flexcan
On 06/19/2014 09:46 AM, Zhao Qiang wrote:
> when flexcan is not physically linked, command 'cantest' will
> trigger an err_irq, add err_irq handler for it.
>
> Signed-off-by: Zhao Qiang
> ---
> drivers/net/can/flexcan.c | 31 ++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index aaed97b..e3c6cfd 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -206,6 +206,7 @@ struct flexcan_priv {
> void __iomem *base;
> u32 reg_esr;
> u32 reg_ctrl_default;
> + unsigned interr_irq;
Please use a space instead of a tab before err_irq.
>
> struct clk *clk_ipg;
> struct clk *clk_per;
> @@ -654,6 +655,23 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t flexcan_err_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct flexcan_priv *priv = netdev_priv(dev);
> + struct flexcan_regs __iomem *regs = priv->base;
> + u32 reg_ctrl, reg_esr;
> +
> + reg_esr = flexcan_read(®s->esr);
> + reg_ctrl = flexcan_read(®s->ctrl);
> + if (reg_esr & FLEXCAN_ESR_TX_WRN) {
> + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr);
> + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl);
> + netdev_err(dev, "No physical link!\n");
A warning IRQ does not imply that you have no physical link. I suggest
to make use of the existing flexcan_poll_state() function.
> + }
> + return IRQ_HANDLED;
> +}
> +
> static void flexcan_set_bittiming(struct net_device *dev)
> {
> const struct flexcan_priv *priv = netdev_priv(dev);
> @@ -860,6 +878,12 @@ static int flexcan_open(struct net_device *dev)
> if (err)
> goto out_close;
>
> + if (priv->err_irq)
> + err = request_irq(priv->err_irq, flexcan_err_irq, IRQF_SHARED,
> + dev->name, dev);
> + if (err)
> + goto out_close;
> +
> /* start chip and queuing */
> err = flexcan_chip_start(dev);
> if (err)
> @@ -1007,7 +1031,7 @@ static int flexcan_probe(struct platform_device *pdev)
> struct resource *mem;
> struct clk *clk_ipg = NULL, *clk_per = NULL;
> void __iomem *base;
> - int err, irq;
> + int err, irq, err_irq;
> u32 clock_freq = 0;
>
> if (pdev->dev.of_node)
> @@ -1034,6 +1058,10 @@ static int flexcan_probe(struct platform_device *pdev)
> if (irq <= 0)
> return -ENODEV;
>
> + err_irq = platform_get_irq(pdev, 1);
> + if (err_irq <= 0)
> + err_irq = 0;
> +
> base = devm_ioremap_resource(&pdev->dev, mem);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -1057,6 +1085,7 @@ static int flexcan_probe(struct platform_device *pdev)
> dev->flags |= IFF_ECHO;
>
> priv = netdev_priv(dev);
> + priv->err_irq = err_irq;
> priv->can.clock.freq = clock_freq;
> priv->can.bittiming_const = &flexcan_bittiming_const;
> priv->can.do_set_mode = flexcan_set_mode;
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] flexcan: add err interrupt for p1010rdb
On 06/19/2014 09:46 AM, Zhao Qiang wrote:
> add err interrupt for p1010rdb into dts.
Please update the flexcan binding documentation, too.
Marc
>
> Signed-off-by: Zhao Qiang
> ---
> arch/powerpc/boot/dts/fsl/p1010si-post.dtsi | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi
> b/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi
> index 4313ff6..bcd95ba 100644
> --- a/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi
> +++ b/arch/powerpc/boot/dts/fsl/p1010si-post.dtsi
> @@ -141,13 +141,15 @@
> can0: can@1c000 {
> compatible = "fsl,p1010-flexcan";
> reg = <0x1c000 0x1000>;
> - interrupts = <48 0x2 0 0>;
> + interrupts = <48 0x2 0 0
> + 16 0x2 0 0>;
> };
>
> can1: can@1d000 {
> compatible = "fsl,p1010-flexcan";
> reg = <0x1d000 0x1000>;
> - interrupts = <61 0x2 0 0>;
> + interrupts = <61 0x2 0 0
> + 16 0x2 0 0>;
> };
>
> L2: l2-cache-controller@2 {
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 0/2] usb: Reuse fsl driver code for synopsys usb controller
On 04/20/2014 06:27 PM, Punnaiah Choudary Kalluri wrote: > Zynq soc contains a dual role usb controller and this IP is from synopsys. We > observed that there is driver available for this controller from freescale and > decided to reuse this driver for zynq use. Have a look drivers/usb/chipidea. It's maintained by Peter Chen (Cc'ed) and is in better shape than the freescale driver. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 11/31] net: can: mscan: improve clock API use
On 08/07/2013 09:30 AM, Marc Kleine-Budde wrote: > On 08/06/2013 10:43 PM, Gerhard Sittig wrote: >> the .get_clock() callback is run from probe() and might allocate >> resources, introduce a .put_clock() callback that is run from remove() >> to undo any allocation activities > > AFAICS With this patch put_clock() is still a no-op, is there a patch > which adds some code there? If not, please remove. I missed patch 27. sorry for the noise. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 11/31] net: can: mscan: improve clock API use
On 08/06/2013 10:43 PM, Gerhard Sittig wrote: > the .get_clock() callback is run from probe() and might allocate > resources, introduce a .put_clock() callback that is run from remove() > to undo any allocation activities AFAICS With this patch put_clock() is still a no-op, is there a patch which adds some code there? If not, please remove. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 11/31] net: can: mscan: improve clock API use
an.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -573,10 +573,24 @@ static int mscan_open(struct net_device *dev)
> struct mscan_priv *priv = netdev_priv(dev);
> struct mscan_regs __iomem *regs = priv->reg_base;
>
> + if (priv->clk_ipg) {
> + ret = clk_prepare_enable(priv->clk_ipg);
> + if (ret)
> + goto exit_retcode;
> + }
> + if (priv->clk_can) {
> + ret = clk_prepare_enable(priv->clk_can);
> + if (ret) {
> + if (priv->clk_ipg)
> + clk_disable_unprepare(priv->clk_ipg);
> + goto exit_retcode;
Why don't you add another jump label and jump to that to disable the
ipkg clock?
> + }
> + }
> +
> /* common open */
> ret = open_candev(dev);
> if (ret)
> - return ret;
> + goto exit_dis_clock;
>
> napi_enable(&priv->napi);
>
> @@ -604,6 +618,12 @@ exit_free_irq:
> exit_napi_disable:
> napi_disable(&priv->napi);
> close_candev(dev);
> +exit_dis_clock:
> + if (priv->clk_can)
> + clk_disable_unprepare(priv->clk_can);
> + if (priv->clk_ipg)
> + clk_disable_unprepare(priv->clk_ipg);
> +exit_retcode:
> return ret;
> }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 27/31] net: can: mscan: add common clock support for mpc512x
On 07/23/2013 02:07 PM, Gerhard Sittig wrote: [...] >>> + return freq_calc; >>> + >>> +err_invalid: >>> + dev_err(&ofdev->dev, "invalid clock source specification\n"); >>> + return 0; >> >> return 0 in case of error? Please add a comment what this 0 means here. > > The .get_clock() callback is supposed to return the resulting > rate after the clock source was determined and the clock subtree > was setup. It wasn't (explicitly) documented before (in the > non-common-clock case), so I did not bother to comment it in the > parallel common-clock case. Yes, but returning 0 in case of error should be documented, as it's very uncommon. > But it's true that returning zero in the error case may be > unexpected, and I will add a comment in v4 (in all the > .get_clock() implementations). I think a one liner stating that the return value is the clock rate and "0" indicates is an invalid clock rate, this meaning an error here, is enough. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
On 07/23/2013 01:53 PM, Gerhard Sittig wrote: > On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote: >> >> On 07/22/2013 02:14 PM, Gerhard Sittig wrote: >>> the .get_clock() callback is run from probe() and might allocate >>> resources, introduce a .put_clock() callback that is run from remove() >>> to undo any allocation activities >> >> looks good >> >>> use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put >>> upon driver unload >> >> fine >> >>> assume that resources get prepared but not necessarily enabled in the >>> setup phase, make the open() and close() callbacks of the CAN network >>> device enable and disable a previously acquired and prepared clock >> >> I think you should call prepare_enable and disable_unprepare in the >> open/close functions. > > After more local research, which totally eliminated the need to > pre-enable the CAN related clocks, but might need more discussion > as it touches the common gate support, I've learned something > more: > > The CAN clock needs to get enabled during probe() already, since > registers get accessed between probe() for the driver and open() > for the network device -- while access to peripheral registers > crashes the kernel when clocks still are disabled (other hardware > may just hang or provide fake data, neither of this is OK). Then call prepare_enable(); before and disable_unprepare(); after accessing the registers. Have a look at the flexcan driver. > But I see the point in your suggestion to prepare _and_ enable > the clock during open() as well -- to have open() cope with > whatever probe() did, after all the driver is shared among > platforms, which may differ in what they do during probe(). If you enable a clock to access the registers before open() (and disable it afterwards), it should not harm any architecture that doesn't need this clock enabled. > So I will: > - make open() of the network device prepare _and_ enable the > clock for the peripheral (if acquired during probe()) good > - adjust open() because ATM it leaves the clock enabled when the > network device operation fails (the error path is incomplete in > v3) yes, clock should be disabled if open() fails. > - make the MPC512x specific probe() time .get_clock() routine not > just prepare but enable the clock as well If needed enable the clock, but disable after probe() has finished. > - and of course address all the shutdown counter parts of the > above setup paths > This results in: > - specific chip drivers only need to balance their private get > and put clock routines which are called from probe and remove, > common paths DTRT for all of them Yes, but clock should not stay enabled between probe() and open(). [...] > Removing unnecessary devm_put_clk() calls is orthogonal to that. > Putting these in isn't totally wrong (they won't harm, and they > do signal "visual balance" more clearly such that the next person > won't stop and wonder), but it's true that they are redundant. > "Trained persons" will wonder as much about their presence as > untrained persons wonder about their absence. :) Apparently I'm > not well trained yet. The whole point about devm_* is to get rid of auto manually tear down functions. So please remove all devm_put_clk() calls, as it will be called automatically if a driver instance is removed. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 27/31] net: can: mscan: add common clock support for mpc512x
if (IS_ERR(clk_in))
> + goto err_notavail;
> + clk_from = CLK_FROM_REF;
> + freq_calc = clk_get_rate(clk_in);
> + dev_dbg(&ofdev->dev,
> + "clk fit, ref[%lu] (no div) freq[%lu]\n",
> + freq_calc, freq_calc);
> + }
> +
> + /* select IPS or MCLK as the MSCAN input (returned to the caller),
> + * setup the MCLK mux source and rate if applicable, apply the
> + * optionally specified or derived above divider, and determine
> + * the actual resulting clock rate to return to the caller
> + */
> + switch (clk_from) {
> + case CLK_FROM_IPS:
> + clk_can = devm_clk_get(&ofdev->dev, "ips");
> + if (IS_ERR(clk_can))
> + goto err_notavail;
> + if (clk_prepare(clk_can)) {
I would just call prepare_enable in the main mscan driver, then we don't
need a special "clock is prepared but not enabled" contract.
> + devm_clk_put(&ofdev->dev, clk_can);
not needed, as this driver instance will fail, doesn't it?
> + goto err_notavail;
> + }
> + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> + priv->clk_can = clk_can;
> + freq_calc = clk_get_rate(clk_can);
> + *mscan_clksrc = MSCAN_CLKSRC_IPS;
> + dev_dbg(&ofdev->dev, "clk from IPS, clksrc[%d] freq[%lu]\n",
> + *mscan_clksrc, freq_calc);
> + break;
> + case CLK_FROM_SYS:
> + case CLK_FROM_REF:
> + clk_can = devm_clk_get(&ofdev->dev, "mclk");
> + if (IS_ERR(clk_can))
> + goto err_notavail;
> + if (clk_prepare(clk_can)) {
> + devm_clk_put(&ofdev->dev, clk_can);
same here
> + goto err_notavail;
> + }
> + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> + priv->clk_can = clk_can;
> + if (clk_from == CLK_FROM_SYS)
> + clk_in = devm_clk_get(&ofdev->dev, "sys");
> + if (clk_from == CLK_FROM_REF)
> + clk_in = devm_clk_get(&ofdev->dev, "ref");
> + if (IS_ERR(clk_in))
> + goto err_notavail;
> + clk_set_parent(clk_can, clk_in);
> + freq_calc = clk_get_rate(clk_in);
> + freq_calc /= clockdiv;
> + clk_set_rate(clk_can, freq_calc);
> + freq_calc = clk_get_rate(clk_can);
> + *mscan_clksrc = MSCAN_CLKSRC_BUS;
> + dev_dbg(&ofdev->dev, "clk from MCLK, clksrc[%d] freq[%lu]\n",
> + *mscan_clksrc, freq_calc);
> + break;
> + default:
> + goto err_invalid;
> + }
> +
> + return freq_calc;
> +
> +err_invalid:
> + dev_err(&ofdev->dev, "invalid clock source specification\n");
> + return 0;
return 0 in case of error? Please add a comment what this 0 means here.
> +
> +err_notavail:
> + dev_err(&ofdev->dev, "cannot acquire or setup clock source\n");
> + return 0;
> +}
> +
> +static void mpc512x_can_put_clock(struct platform_device *ofdev)
> +{
> + struct mscan_priv *priv;
> +
> + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> + if (priv->clk_can) {
> + clk_unprepare(priv->clk_can);
> + devm_clk_put(&ofdev->dev, priv->clk_can);
devm_clk_put can be removed, it's called automatically.
> + }
> +}
> +
> +#else/* COMMON_CLK */
> +
> struct mpc512x_clockctl {
> u32 spmr; /* System PLL Mode Reg */
> u32 sccr[2];/* System Clk Ctrl Reg 1 & 2 */
> @@ -239,12 +400,18 @@ exit_put:
> of_node_put(np_clock);
> return freq;
> }
> +
> +#define mpc512x_can_put_clock NULL
> +
> +#endif /* COMMON_CLK */
> +
> #else /* !CONFIG_PPC_MPC512x */
> static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
>const char *clock_name, int *mscan_clksrc)
> {
> return 0;
> }
> +#define mpc512x_can_put_clock NULL
> #endif /* CONFIG_PPC_MPC512x */
>
> static const struct of_device_id mpc5xxx_can_table[];
> @@ -386,11 +553,13 @@ static int mpc5xxx_can_resume(struct platform_device
> *ofdev)
> static const struct mpc5xxx_can_data mpc5200_can_data = {
> .type = MSCAN_TYPE_MPC5200,
> .get_clock = mpc52xx_can_get_clock,
> + /* .put_clock not applicable */
> };
>
> static const struct mpc5xxx_can_data mpc5121_can_data = {
> .type = MSCAN_TYPE_MPC5121,
> .get_clock = mpc512x_can_get_clock,
> + .put_clock = mpc512x_can_put_clock,
> };
>
> static const struct of_device_id mpc5xxx_can_table[] = {
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
On 07/22/2013 02:14 PM, Gerhard Sittig wrote: > the .get_clock() callback is run from probe() and might allocate > resources, introduce a .put_clock() callback that is run from remove() > to undo any allocation activities looks good > use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put > upon driver unload fine > assume that resources get prepared but not necessarily enabled in the > setup phase, make the open() and close() callbacks of the CAN network > device enable and disable a previously acquired and prepared clock I think you should call prepare_enable and disable_unprepare in the open/close functions. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 08/31] fs_enet: cleanup clock API use
On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
> make the Freescale ethernet driver get, prepare and enable the FEC clock
> during probe(); disable, unprepare and put the clock upon remove(); hold
> a reference to the clock over the period of use; use devm_{get,put}_clk()
There's no need for devm_clk_put(), devm will take care of this.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 16/24] net: can: mscan: make mpc512x code use common clock
On 07/19/2013 11:41 AM, Gerhard Sittig wrote:
> On Fri, Jul 19, 2013 at 09:34 +0200, Marc Kleine-Budde wrote:
>>
>> On 07/18/2013 10:20 PM, Gerhard Sittig wrote:
>>> extend the mscan(4) driver with alternative support for the COMMON_CLK
>>> approach which is an option in the MPC512x platform, keep the existing
>>> clock support implementation in place since the driver is shared with
>>> other MPC5xxx SoCs which don't have common clock support
>>>
>>> one byproduct of this change is that the CAN driver no longer needs to
>>> access the SoC's clock control registers, which shall be the domain of
>>> the platform's clock driver
>>>
>>> Signed-off-by: Gerhard Sittig
>>> ---
>>> drivers/net/can/mscan/mpc5xxx_can.c | 139
>>> +++
>>> 1 file changed, 139 insertions(+)
>>>
>>> diff --git a/drivers/net/can/mscan/mpc5xxx_can.c
>>> b/drivers/net/can/mscan/mpc5xxx_can.c
>>> index bc422ba..dd26ab6 100644
>>> --- a/drivers/net/can/mscan/mpc5xxx_can.c
>>> +++ b/drivers/net/can/mscan/mpc5xxx_can.c
>>> @@ -108,6 +108,143 @@ static u32 mpc52xx_can_get_clock(struct
>>> platform_device *ofdev,
>>> #endif /* CONFIG_PPC_MPC52xx */
>>>
>>> #ifdef CONFIG_PPC_MPC512x
>>> +
>>> +#if IS_ENABLED(CONFIG_COMMON_CLK)
>>> +
>>> +static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
>>> +const char *clock_source, int *mscan_clksrc)
>>> +{
>>> + struct device_node *np;
>>> + u32 clockdiv;
>>> + enum {
>>> + CLK_FROM_AUTO,
>>> + CLK_FROM_IPS,
>>> + CLK_FROM_SYS,
>>> + CLK_FROM_REF,
>>> + } clk_from;
>>> + struct clk *clk_in, *clk_can;
>>> + unsigned long freq_calc;
>>> +
>>> + /* the caller passed in the clock source spec that was read from
>>> +* the device tree, get the optional clock divider as well
>>> +*/
>>> + np = ofdev->dev.of_node;
>>> + clockdiv = 1;
>>> + of_property_read_u32(np, "fsl,mscan-clock-divider", &clockdiv);
>>> + dev_dbg(&ofdev->dev, "device tree specs: clk src[%s] div[%d]\n",
>>> + clock_source ? clock_source : "", clockdiv);
>>> +
>>> + /* when clock-source is 'ip', the CANCTL1[CLKSRC] bit needs to
>>> +* get set, and the 'ips' clock is the input to the MSCAN
>>> +* component
>>> +*
>>> +* for clock-source values of 'ref' or 'sys' the CANCTL1[CLKSRC]
>>> +* bit needs to get cleared, an optional clock-divider may have
>>> +* been specified (the default value is 1), the appropriate
>>> +* MSCAN related MCLK is the input to the MSCAN component
>>> +*
>>> +* in the absence of a clock-source spec, first an optimal clock
>>> +* gets determined based on the 'sys' clock, if that fails the
>>> +* 'ref' clock is used
>>> +*/
>>> + clk_from = CLK_FROM_AUTO;
>>> + if (clock_source) {
>>> + /* interpret the device tree's spec for the clock source */
>>> + if (!strcmp(clock_source, "ip"))
>>> + clk_from = CLK_FROM_IPS;
>>> + else if (!strcmp(clock_source, "sys"))
>>> + clk_from = CLK_FROM_SYS;
>>> + else if (!strcmp(clock_source, "ref"))
>>> + clk_from = CLK_FROM_REF;
>>> + else
>>> + goto err_invalid;
>>> + dev_dbg(&ofdev->dev, "got a clk source spec[%d]\n", clk_from);
>>> + }
>>> + if (clk_from == CLK_FROM_AUTO) {
>>> + /* no spec so far, try the 'sys' clock; round to the
>>> +* next MHz and see if we can get a multiple of 16MHz
>>> +*/
>>> + dev_dbg(&ofdev->dev, "no clk source spec, trying SYS\n");
>>> + clk_in = clk_get(&ofdev->dev, "sys");
>>> + if (IS_ERR(clk_in))
>>> + goto err_notavail;
>>> + freq_calc = clk_get_rate(clk_in);
>>> + freq_calc += 49;
>>> + freq_calc /= 100;
>>> + freq_calc *= 100;
>>> + if ((freq
Re: [PATCH v2 16/24] net: can: mscan: make mpc512x code use common clock
> + goto err_notavail;
> + clk_from = CLK_FROM_REF;
> + freq_calc = clk_get_rate(clk_in);
> + dev_dbg(&ofdev->dev,
> + "clk fit, ref[%lu] (no div) freq[%lu]\n",
> + freq_calc, freq_calc);
> + }
> +
> + /* select IPS or MCLK as the MSCAN input (returned to the caller),
> + * setup the MCLK mux source and rate if applicable, apply the
> + * optionally specified or derived above divider, and determine
> + * the actual resulting clock rate to return to the caller
> + */
> + switch (clk_from) {
> + case CLK_FROM_IPS:
> + clk_can = clk_get(&ofdev->dev, "ips");
> + if (IS_ERR(clk_can))
> + goto err_notavail;
> + clk_prepare_enable(clk_can);
Where is the corresponding clk_disable_unprepare()?a
> + freq_calc = clk_get_rate(clk_can);
> + *mscan_clksrc = MSCAN_CLKSRC_IPS;
> + dev_dbg(&ofdev->dev, "clk from IPS, clksrc[%d] freq[%lu]\n",
> + *mscan_clksrc, freq_calc);
> + break;
> + case CLK_FROM_SYS:
> + case CLK_FROM_REF:
> + clk_can = clk_get(&ofdev->dev, "mclk");
> + if (IS_ERR(clk_can))
> + goto err_notavail;
> + clk_prepare_enable(clk_can);
dito
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] can: flexcan: remove HAVE_CAN_FLEXCAN Kconfig symbol
On 06/01/2013 10:59 AM, Arnd Bergmann wrote:
> On Friday 17 May 2013 10:59:17 Marc Kleine-Budde wrote:
>> This patch removes the Kconfig symbol HAVE_CAN_FLEXCAN from
>> arch/{arm,powerpc}
>> and allowing compilation unconditionally on all arm and powerpc platforms.
>>
>> This brings a bigger compile time coverage and removes the following
>> dependency
>> warning found by Arnd Bergmann:
>>
>> warning: (SOC_IMX28 && SOC_IMX25 && SOC_IMX35 &&
>> IMX_HAVE_PLATFORM_FLEXCAN &&
>> SOC_IMX53 && SOC_IMX6Q) selects HAVE_CAN_FLEXCAN
>> which has unmet direct dependencies (NET && CAN && CAN_DEV)
>>
>> Cc: Arnd Bergmann
>> Cc: Shawn Guo
>> Cc: Sascha Hauer
>> Cc: Kumar Gala
>> Cc: U Bhaskar-B22300
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Marc Kleine-Budde
>
> Acked-by: Arnd Bergmann
>
> Thanks for addressing this!
I'll include this patch in my next pull request to David Miller.
Tnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] can: flexcan: remove HAVE_CAN_FLEXCAN Kconfig symbol
On 06/01/2013 01:40 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-05-20 at 18:06 +0200, Marc Kleine-Budde wrote:
>> On 05/17/2013 11:09 AM, Shawn Guo wrote:
>>> On Fri, May 17, 2013 at 10:59:17AM +0200, Marc Kleine-Budde wrote:
>>>> This patch removes the Kconfig symbol HAVE_CAN_FLEXCAN from
>>>> arch/{arm,powerpc}
>>>> and allowing compilation unconditionally on all arm and powerpc platforms.
>>>>
>>>> This brings a bigger compile time coverage and removes the following
>>>> dependency
>>>> warning found by Arnd Bergmann:
>>>>
>>>> warning: (SOC_IMX28 && SOC_IMX25 && SOC_IMX35 &&
>>>> IMX_HAVE_PLATFORM_FLEXCAN &&
>>>> SOC_IMX53 && SOC_IMX6Q) selects HAVE_CAN_FLEXCAN
>>>> which has unmet direct dependencies (NET && CAN && CAN_DEV)
>>>>
>>>> Cc: Arnd Bergmann
>>>> Cc: Shawn Guo
>>>
>>> Acked-by: Shawn Guo
>>
>> Thanks.
>>
>> An Acked-by by the powerpc people would be fine. However, if nobody
>> doesn't object, I'm sending this patch via linux-can and net-next upstream.
>
> Sorry, missed it, if it's still out there, add my
>
> Acked-by: Benjamin Herrenschmidt
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] can: flexcan: remove HAVE_CAN_FLEXCAN Kconfig symbol
On 05/17/2013 11:09 AM, Shawn Guo wrote:
> On Fri, May 17, 2013 at 10:59:17AM +0200, Marc Kleine-Budde wrote:
>> This patch removes the Kconfig symbol HAVE_CAN_FLEXCAN from
>> arch/{arm,powerpc}
>> and allowing compilation unconditionally on all arm and powerpc platforms.
>>
>> This brings a bigger compile time coverage and removes the following
>> dependency
>> warning found by Arnd Bergmann:
>>
>> warning: (SOC_IMX28 && SOC_IMX25 && SOC_IMX35 &&
>> IMX_HAVE_PLATFORM_FLEXCAN &&
>> SOC_IMX53 && SOC_IMX6Q) selects HAVE_CAN_FLEXCAN
>> which has unmet direct dependencies (NET && CAN && CAN_DEV)
>>
>> Cc: Arnd Bergmann
>> Cc: Shawn Guo
>
> Acked-by: Shawn Guo
Thanks.
An Acked-by by the powerpc people would be fine. However, if nobody
doesn't object, I'm sending this patch via linux-can and net-next upstream.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2] can: flexcan: remove HAVE_CAN_FLEXCAN Kconfig symbol
This patch removes the Kconfig symbol HAVE_CAN_FLEXCAN from arch/{arm,powerpc}
and allowing compilation unconditionally on all arm and powerpc platforms.
This brings a bigger compile time coverage and removes the following dependency
warning found by Arnd Bergmann:
warning: (SOC_IMX28 && SOC_IMX25 && SOC_IMX35 && IMX_HAVE_PLATFORM_FLEXCAN
&&
SOC_IMX53 && SOC_IMX6Q) selects HAVE_CAN_FLEXCAN
which has unmet direct dependencies (NET && CAN && CAN_DEV)
Cc: Arnd Bergmann
Cc: Shawn Guo
Cc: Sascha Hauer
Cc: Kumar Gala
Cc: U Bhaskar-B22300
Cc: linux-arm-ker...@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Marc Kleine-Budde
---
Changes since v1:
- don't remove IMX_HAVE_PLATFORM_FLEXCAN, which breaks non DT imx platforms
tnx Shawn
arch/arm/mach-imx/Kconfig | 4
arch/arm/mach-imx/devices/Kconfig | 1 -
arch/arm/mach-mxs/Kconfig | 1 -
arch/powerpc/Kconfig | 1 -
drivers/net/can/Kconfig | 5 +
5 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index ba44328..af8e109 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -111,7 +111,6 @@ config SOC_IMX25
select ARCH_MXC_IOMUX_V3
select COMMON_CLK
select CPU_ARM926T
- select HAVE_CAN_FLEXCAN if CAN
select MXC_AVIC
config SOC_IMX27
@@ -137,7 +136,6 @@ config SOC_IMX35
select ARCH_MXC_IOMUX_V3
select COMMON_CLK
select CPU_V6K
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_EPIT
select MXC_AVIC
select SMP_ON_UP if SMP
@@ -776,7 +774,6 @@ comment "Device tree only"
config SOC_IMX53
bool "i.MX53 support"
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_IMX_SRC
select IMX_HAVE_PLATFORM_IMX2_WDT
select PINCTRL
@@ -799,7 +796,6 @@ config SOC_IMX6Q
select CPU_V7
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if LOCAL_TIMERS
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_IMX_ANATOP
select HAVE_IMX_GPC
select HAVE_IMX_MMDC
diff --git a/arch/arm/mach-imx/devices/Kconfig
b/arch/arm/mach-imx/devices/Kconfig
index 3dd2b1b..68c74fb 100644
--- a/arch/arm/mach-imx/devices/Kconfig
+++ b/arch/arm/mach-imx/devices/Kconfig
@@ -4,7 +4,6 @@ config IMX_HAVE_PLATFORM_FEC
config IMX_HAVE_PLATFORM_FLEXCAN
bool
- select HAVE_CAN_FLEXCAN if CAN
config IMX_HAVE_PLATFORM_FSL_USB2_UDC
bool
diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index 4dc2fbb..ce6e7d6 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -11,7 +11,6 @@ config SOC_IMX28
select ARM_AMBA
select ARM_CPU_SUSPEND if PM
select CPU_ARM926T
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_PWM
select PINCTRL_IMX28
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c33e3ad..7754c6b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -674,7 +674,6 @@ config SBUS
config FSL_SOC
bool
- select HAVE_CAN_FLEXCAN if NET && CAN
config FSL_PCI
bool
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e456b70..3c06947 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -102,12 +102,9 @@ config CAN_JANZ_ICAN3
This driver can also be built as a module. If so, the module will be
called janz-ican3.ko.
-config HAVE_CAN_FLEXCAN
- bool
-
config CAN_FLEXCAN
tristate "Support for Freescale FLEXCAN based chips"
- depends on HAVE_CAN_FLEXCAN
+ depends on ARM || PPC
---help---
Say Y here if you want to support for Freescale FlexCAN.
--
1.8.2.rc2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] can: flexcan: allow compilation on arm and powerpc
On 05/17/2013 04:03 AM, Shawn Guo wrote:
> Hi Marc,
>
> On Thu, May 16, 2013 at 03:42:36PM +0200, Marc Kleine-Budde wrote:
>> This patch removes the Kconfig symbols HAVE_CAN_FLEXCAN and
>> IMX_HAVE_PLATFORM_FLEXCAN from arch/{arm,powerpc} and allowing compilation on
>> all arm and powerpc platforms.
>
> I'm generally fine with the approach. But with Kconfig symbol
> IMX_HAVE_PLATFORM_FLEXCAN removed, how does the build of
> platform-flexcan.o work?
>
> arch/arm/mach-imx/devices/Makefile:obj-$(CONFIG_IMX_HAVE_PLATFORM_FLEXCAN) +=
> platform-flexcan.o
Doh! - I've removed too much, will change.
Tnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] can: flexcan: allow compilation on arm and powerpc
This patch removes the Kconfig symbols HAVE_CAN_FLEXCAN and
IMX_HAVE_PLATFORM_FLEXCAN from arch/{arm,powerpc} and allowing compilation on
all arm and powerpc platforms.
This brings a bigger compile time coverage and removes the following dependency
warning found by Arnd Bergmann:
warning: (SOC_IMX28 && SOC_IMX25 && SOC_IMX35 && IMX_HAVE_PLATFORM_FLEXCAN
&&
SOC_IMX53 && SOC_IMX6Q) selects HAVE_CAN_FLEXCAN
which has unmet direct dependencies (NET && CAN && CAN_DEV)
Cc: Arnd Bergmann
Cc: Shawn Guo
Cc: Sascha Hauer
Cc: Kumar Gala
Cc: U Bhaskar-B22300
Cc: linux-arm-ker...@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Marc Kleine-Budde
---
Hello,
if the arm and powerpc people are okay with this, I'm taking this patch (and
then will go upstream via David Miller's net-next).
regards,
Marc
arch/arm/mach-imx/Kconfig | 8
arch/arm/mach-imx/devices/Kconfig | 4
arch/arm/mach-mxs/Kconfig | 1 -
arch/powerpc/Kconfig | 1 -
drivers/net/can/Kconfig | 5 +
5 files changed, 1 insertion(+), 18 deletions(-)
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index ba44328..239d084 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -111,7 +111,6 @@ config SOC_IMX25
select ARCH_MXC_IOMUX_V3
select COMMON_CLK
select CPU_ARM926T
- select HAVE_CAN_FLEXCAN if CAN
select MXC_AVIC
config SOC_IMX27
@@ -137,7 +136,6 @@ config SOC_IMX35
select ARCH_MXC_IOMUX_V3
select COMMON_CLK
select CPU_V6K
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_EPIT
select MXC_AVIC
select SMP_ON_UP if SMP
@@ -208,7 +206,6 @@ comment "MX25 platforms:"
config MACH_MX25_3DS
bool "Support MX25PDK (3DS) Platform"
- select IMX_HAVE_PLATFORM_FLEXCAN
select IMX_HAVE_PLATFORM_FSL_USB2_UDC
select IMX_HAVE_PLATFORM_IMX2_WDT
select IMX_HAVE_PLATFORM_IMXDI_RTC
@@ -223,7 +220,6 @@ config MACH_MX25_3DS
config MACH_EUKREA_CPUIMX25SD
bool "Support Eukrea CPUIMX25 Platform"
- select IMX_HAVE_PLATFORM_FLEXCAN
select IMX_HAVE_PLATFORM_FSL_USB2_UDC
select IMX_HAVE_PLATFORM_IMX2_WDT
select IMX_HAVE_PLATFORM_IMXDI_RTC
@@ -629,7 +625,6 @@ comment "MX35 platforms:"
config MACH_PCM043
bool "Support Phytec pcm043 (i.MX35) platforms"
- select IMX_HAVE_PLATFORM_FLEXCAN
select IMX_HAVE_PLATFORM_FSL_USB2_UDC
select IMX_HAVE_PLATFORM_IMX2_WDT
select IMX_HAVE_PLATFORM_IMX_I2C
@@ -665,7 +660,6 @@ config MACH_MX35_3DS
config MACH_EUKREA_CPUIMX35SD
bool "Support Eukrea CPUIMX35 Platform"
- select IMX_HAVE_PLATFORM_FLEXCAN
select IMX_HAVE_PLATFORM_FSL_USB2_UDC
select IMX_HAVE_PLATFORM_IMX2_WDT
select IMX_HAVE_PLATFORM_IMX_I2C
@@ -776,7 +770,6 @@ comment "Device tree only"
config SOC_IMX53
bool "i.MX53 support"
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_IMX_SRC
select IMX_HAVE_PLATFORM_IMX2_WDT
select PINCTRL
@@ -799,7 +792,6 @@ config SOC_IMX6Q
select CPU_V7
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if LOCAL_TIMERS
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_IMX_ANATOP
select HAVE_IMX_GPC
select HAVE_IMX_MMDC
diff --git a/arch/arm/mach-imx/devices/Kconfig
b/arch/arm/mach-imx/devices/Kconfig
index 3dd2b1b..b0a629d 100644
--- a/arch/arm/mach-imx/devices/Kconfig
+++ b/arch/arm/mach-imx/devices/Kconfig
@@ -2,10 +2,6 @@ config IMX_HAVE_PLATFORM_FEC
bool
default y if ARCH_MX25 || SOC_IMX27 || SOC_IMX35 || SOC_IMX51 ||
SOC_IMX53
-config IMX_HAVE_PLATFORM_FLEXCAN
- bool
- select HAVE_CAN_FLEXCAN if CAN
-
config IMX_HAVE_PLATFORM_FSL_USB2_UDC
bool
diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index 4dc2fbb..ce6e7d6 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -11,7 +11,6 @@ config SOC_IMX28
select ARM_AMBA
select ARM_CPU_SUSPEND if PM
select CPU_ARM926T
- select HAVE_CAN_FLEXCAN if CAN
select HAVE_PWM
select PINCTRL_IMX28
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c33e3ad..7754c6b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -674,7 +674,6 @@ config SBUS
config FSL_SOC
bool
- select HAVE_CAN_FLEXCAN if NET && CAN
config FSL_PCI
bool
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index e456b70..3c06947 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -102,12 +102,9 @@ config CAN_JANZ_ICAN3
This driver can also be built as a module. If so, the module will be
called janz-ican3.ko.
-c
Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
On 01/14/2013 06:40 PM, Felipe Balbi wrote: > Hi, > > On Mon, Jan 14, 2013 at 08:56:33PM +0800, Peter Chen wrote: > > > >>>> Usually there isn't any Changelog between IP cores used in the different >>>> fsl processors (at least available outside of fsl), that makes it quite >>>> difficult to say if something found on one imx is really the same as on >>>> the other one. And they (usually) don't provide any versioning >>>> information in a register or the documentation. >>>> >>>> just my 2¢ >>> >>> $SUBJECT is trying to differentiate a single feature (or maybe two) to >>> replace cpu_is_xxx(), then expose that on driver_data without creating >>> one enum value for each release from fsl. >> >> Felipe, every one or two SoCs may have their special operations for >> integrate PHY interface, clk operation, or workaround for IC >> limitation. > > the particular PHY and clk used should be hidden by phy layer and clk > API respectively. Workarounds, fair enough, we need to handle them; but > ideally those should be based on runtime revision detection, not some > hackery using driver_data. If this is actually possible, I'd love to do this. But IP vendor don't include a version register in their cores. :( >> Maybe, it will add more future or SoCs (maybe not for this driver) in >> the future, using enum is easier than string comparison for expanding >> something. > > a) I never told you to *not* use enum. I said that creating DEVICE_A, > DEVICE_B, DEVICE_C, DEVICE_D and DEVICE_E values when DEVICE_B, > DEVICE_C and DEVICE_E behave exactly the same is unnecessary. > > b) you can't be expecting to add future SoCs support to fsl udc, I have > already said and will repeat for the last time: move to chipidea ASAP. > > New SoCs cannot be added to fsl udc, you *must* use chipidea for > anything new and move the legacy to chipidea eventually. I will wait for > a full year for you to do that, but after that I will have to start > deleting drivers for the sake of avoid duplication of effort. +1 Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
On 01/14/2013 11:53 AM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jan 14, 2013 at 11:50:41AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:39 AM, Felipe Balbi wrote:
>>> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
>>>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
>>>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>>>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device
>>>>>>>> *dev)
>>>>>>>>
>>>>>>>>return fsl_udc_resume(NULL);
>>>>>>>> }
>>>>>>>> -
>>>>>>>>
>>>>>>>> /*-
>>>>>>>>Register entry point for the peripheral controller driver
>>>>>>>>
>>>>>>>> --*/
>>>>>>>> -
>>>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>>>>>> + {
>>>>>>>> + .name = "imx-udc-mx25",
>>>>>>>> + .driver_data = IMX25_UDC,
>>>>>>>> + }, {
>>>>>>>> + .name = "imx-udc-mx27",
>>>>>>>> + .driver_data = IMX27_UDC,
>>>>>>>> + }, {
>>>>>>>> + .name = "imx-udc-mx31",
>>>>>>>> + .driver_data = IMX31_UDC,
>>>>>>>> + }, {
>>>>>>>> + .name = "imx-udc-mx35",
>>>>>>>> + .driver_data = IMX35_UDC,
>>>>>>>> + }, {
>>>>>>>> + .name = "imx-udc-mx51",
>>>>>>>> + .driver_data = IMX51_UDC,
>>>>>>>> + }
>>>>>>>> +};
>>>>>>>
>>>>>>> I wonder if your driver-data is actually needed since you can use string
>>>>>>> comparisson to achieve the exact same outcome.
>>>>>>
>>>>>> Why use a string compare, if the kernel infrastructure already does this
>>>>>> for you?
>>>>>
>>>>> what do you mean ? What kernel infrastructure is doing waht for me ?
>>>>
>>>> The kernel infrastructure is doing the string compare for you to match
>>>> the device against the driver (via platform_device_id->name). You get
>>>> the a pointer to the driver_data for free. So you don't need any string
>>>> compare in the driver later.
>>>
>>> but current driver data is just duplicating name with an integer, it's
>>> pretty useless driver data.
>>
>> I don't think so - another argument:
>> Less code. As struct platform_device_id is a static array the space is
>> allocated anyway. So it doesn't make any difference if driver_data is
>> NULL or not. Later you just need to make an integer comparison instead
>> of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
>> enum, the compiler will warn you if you've missed an IMX variant.
>
> fair enough, but then don't create a different enum value for each imx
> instance if they're mostly the same. Differentiate only what's actually
> different.
Usually there isn't any Changelog between IP cores used in the different
fsl processors (at least available outside of fsl), that makes it quite
difficult to say if something found on one imx is really the same as on
the other one. And they (usually) don't provide any versioning
information in a register or the documentation.
just my 2¢
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
On 01/14/2013 11:39 AM, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 11:34:05AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:24 AM, Felipe Balbi wrote:
>>> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>>>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>>>>>
>>>>>> return fsl_udc_resume(NULL);
>>>>>> }
>>>>>> -
>>>>>>
>>>>>> /*-
>>>>>> Register entry point for the peripheral controller driver
>>>>>>
>>>>>> --*/
>>>>>> -
>>>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>>>> +{
>>>>>> +.name = "imx-udc-mx25",
>>>>>> +.driver_data = IMX25_UDC,
>>>>>> +}, {
>>>>>> +.name = "imx-udc-mx27",
>>>>>> +.driver_data = IMX27_UDC,
>>>>>> +}, {
>>>>>> +.name = "imx-udc-mx31",
>>>>>> +.driver_data = IMX31_UDC,
>>>>>> +}, {
>>>>>> +.name = "imx-udc-mx35",
>>>>>> +.driver_data = IMX35_UDC,
>>>>>> +}, {
>>>>>> +.name = "imx-udc-mx51",
>>>>>> +.driver_data = IMX51_UDC,
>>>>>> +}
>>>>>> +};
>>>>>
>>>>> I wonder if your driver-data is actually needed since you can use string
>>>>> comparisson to achieve the exact same outcome.
>>>>
>>>> Why use a string compare, if the kernel infrastructure already does this
>>>> for you?
>>>
>>> what do you mean ? What kernel infrastructure is doing waht for me ?
>>
>> The kernel infrastructure is doing the string compare for you to match
>> the device against the driver (via platform_device_id->name). You get
>> the a pointer to the driver_data for free. So you don't need any string
>> compare in the driver later.
>
> but current driver data is just duplicating name with an integer, it's
> pretty useless driver data.
I don't think so - another argument:
Less code. As struct platform_device_id is a static array the space is
allocated anyway. So it doesn't make any difference if driver_data is
NULL or not. Later you just need to make an integer comparison instead
of a call to a strcmp(), if you have a switch/case and IMX*_UDC is an
enum, the compiler will warn you if you've missed an IMX variant.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
On 01/14/2013 11:24 AM, Felipe Balbi wrote:
> On Mon, Jan 14, 2013 at 11:18:53AM +0100, Marc Kleine-Budde wrote:
>> On 01/14/2013 11:16 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>>>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>>>
>>>>return fsl_udc_resume(NULL);
>>>> }
>>>> -
>>>>
>>>> /*-
>>>>Register entry point for the peripheral controller driver
>>>>
>>>> --*/
>>>> -
>>>> +static const struct platform_device_id fsl_udc_devtype[] = {
>>>> + {
>>>> + .name = "imx-udc-mx25",
>>>> + .driver_data = IMX25_UDC,
>>>> + }, {
>>>> + .name = "imx-udc-mx27",
>>>> + .driver_data = IMX27_UDC,
>>>> + }, {
>>>> + .name = "imx-udc-mx31",
>>>> + .driver_data = IMX31_UDC,
>>>> + }, {
>>>> + .name = "imx-udc-mx35",
>>>> + .driver_data = IMX35_UDC,
>>>> + }, {
>>>> + .name = "imx-udc-mx51",
>>>> + .driver_data = IMX51_UDC,
>>>> + }
>>>> +};
>>>
>>> I wonder if your driver-data is actually needed since you can use string
>>> comparisson to achieve the exact same outcome.
>>
>> Why use a string compare, if the kernel infrastructure already does this
>> for you?
>
> what do you mean ? What kernel infrastructure is doing waht for me ?
The kernel infrastructure is doing the string compare for you to match
the device against the driver (via platform_device_id->name). You get
the a pointer to the driver_data for free. So you don't need any string
compare in the driver later.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] usb: fsl-mxc-udc: replace cpu_is_xxx() with platform_device_id
On 01/14/2013 11:16 AM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jan 14, 2013 at 06:12:39PM +0800, Peter Chen wrote:
>> @@ -2756,22 +2753,41 @@ static int fsl_udc_otg_resume(struct device *dev)
>>
>> return fsl_udc_resume(NULL);
>> }
>> -
>> /*-
>> Register entry point for the peripheral controller driver
>> --*/
>> -
>> +static const struct platform_device_id fsl_udc_devtype[] = {
>> +{
>> +.name = "imx-udc-mx25",
>> +.driver_data = IMX25_UDC,
>> +}, {
>> +.name = "imx-udc-mx27",
>> +.driver_data = IMX27_UDC,
>> +}, {
>> +.name = "imx-udc-mx31",
>> +.driver_data = IMX31_UDC,
>> +}, {
>> +.name = "imx-udc-mx35",
>> +.driver_data = IMX35_UDC,
>> +}, {
>> +.name = "imx-udc-mx51",
>> +.driver_data = IMX51_UDC,
>> +}
>> +};
>
> I wonder if your driver-data is actually needed since you can use string
> comparisson to achieve the exact same outcome.
Why use a string compare, if the kernel infrastructure already does this
for you?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH net-next v3 3/4] can: cc770: add platform bus driver for the CC770 and AN82527
On 11/28/2011 12:30 PM, Wolfgang Grandegger wrote:
> This driver works with both, static platform data and device tree
> bindings. It has been tested on a TQM855L board with two AN82527
> CAN controllers on the local bus.
>
> CC: devicetree-disc...@lists.ozlabs.org
> CC: linuxppc-...@ozlabs.org
> CC: Kumar Gala
> Signed-off-by: Wolfgang Grandegger
See comment in the _remove function. Otherwise good. Add my:
Acked-by: Marc Kleine-Budde
> ---
> .../devicetree/bindings/net/can/cc770.txt | 56
> drivers/net/can/cc770/Kconfig |7 +
> drivers/net/can/cc770/Makefile |1 +
> drivers/net/can/cc770/cc770_platform.c | 280
>
> 4 files changed, 344 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/cc770.txt
> create mode 100644 drivers/net/can/cc770/cc770_platform.c
>
> diff --git a/Documentation/devicetree/bindings/net/can/cc770.txt
> b/Documentation/devicetree/bindings/net/can/cc770.txt
> new file mode 100644
> index 000..01e282d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/cc770.txt
> @@ -0,0 +1,56 @@
> +Memory mapped Bosch CC770 and Intel AN82527 CAN controller
> +
> +Note: The CC770 is a CAN controller from Bosch, which is 100%
> +compatible with the old AN82527 from Intel, but with "bugs" being fixed.
> +
> +Required properties:
> +
> +- compatible : should be "bosch,cc770" for the CC770 and "intc,82527"
> + for the AN82527.
> +
> +- reg : should specify the chip select, address offset and size required
> + to map the registers of the controller. The size is usually 0x80.
> +
> +- interrupts : property with a value describing the interrupt source
> + (number and sensitivity) required for the controller.
> +
> +Optional properties:
> +
> +- bosch,external-clock-frequency : frequency of the external oscillator
> + clock in Hz. Note that the internal clock frequency used by the
> + controller is half of that value. If not specified, a default
> + value of 1600 (16 MHz) is used.
> +
> +- bosch,clock-out-frequency : slock frequency in Hz on the CLKOUT pin.
> + If not specified or if the specified value is 0, the CLKOUT pin
> + will be disabled.
> +
> +- bosch,slew-rate : slew rate of the CLKOUT signal. If not specified,
> + a resonable value will be calculated.
> +
> +- bosch,disconnect-rx0-input : see data sheet.
> +
> +- bosch,disconnect-rx1-input : see data sheet.
> +
> +- bosch,disconnect-tx1-output : see data sheet.
> +
> +- bosch,polarity-dominant : see data sheet.
> +
> +- bosch,divide-memory-clock : see data sheet.
> +
> +- bosch,iso-low-speed-mux : see data sheet.
> +
> +For further information, please have a look to the CC770 or AN82527.
> +
> +Examples:
> +
> +can@3,100 {
> + compatible = "bosch,cc770";
> + reg = <3 0x100 0x80>;
> + interrupts = <2 0>;
> + interrupt-parent = <&mpic>;
> + bosch,external-clock-frequency = <1600>;
> +};
> +
> +
> +
> diff --git a/drivers/net/can/cc770/Kconfig b/drivers/net/can/cc770/Kconfig
> index 28e4d48..22c07a8 100644
> --- a/drivers/net/can/cc770/Kconfig
> +++ b/drivers/net/can/cc770/Kconfig
> @@ -11,4 +11,11 @@ config CAN_CC770_ISA
> connected to the ISA bus using I/O port, memory mapped or
> indirect access.
>
> +config CAN_CC770_PLATFORM
> + tristate "Generic Platform Bus based CC770 driver"
> + ---help---
> + This driver adds support for the CC770 and AN82527 chips
> + connected to the "platform bus" (Linux abstraction for directly
> + to the processor attached devices).
> +
> endif
> diff --git a/drivers/net/can/cc770/Makefile b/drivers/net/can/cc770/Makefile
> index 872ecff..9fb8321 100644
> --- a/drivers/net/can/cc770/Makefile
> +++ b/drivers/net/can/cc770/Makefile
> @@ -4,5 +4,6 @@
>
> obj-$(CONFIG_CAN_CC770) += cc770.o
> obj-$(CONFIG_CAN_CC770_ISA) += cc770_isa.o
> +obj-$(CONFIG_CAN_CC770_PLATFORM) += cc770_platform.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/cc770/cc770_platform.c
> b/drivers/net/can/cc770/cc770_platform.c
> new file mode 100644
> index 000..65e177e
> --- /dev/null
> +++ b/drivers/net/can/cc770/cc770_platform.c
> @@ -0,0 +1,280 @@
> +/*
> + * Driver for CC770 and AN82527 CAN controllers on the platform bus
> + *
> + * Copyright (C) 2009, 2011 Wolfgang Grandegger
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it unde
Re: [PATCH v13 0/6] flexcan: Add support for powerpc flexcan (freescale p1010)
On 10/18/2011 01:43 PM, Kumar Gala wrote: > Thanks, I'll look into this internally at FSL. I think its confusing > as hell to have "fsl,p1010-flexcan" in an ARM .dts and don't think > any reasonable ARM customer of FSL would know to put a PPC SOC name > in their .dts. I'll ask the HW guys what's going on so we can come > up with a bit more generic name so we don't have to constantly change > this. Even if its just: > > fsl,ppc-flexcan & fsl,arm-flexcan. If you talk the HW guys ask them for a name for the coldfire flexcan core, too please ;) cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v12 3/6] flexcan: Fix up fsl-flexcan device tree binding.
On 08/15/2011 05:03 PM, Robin Holt wrote: > Earlier, you had asked for a more specific name for the compatible > property of the Freescale flexcan device. I still have not gotten a > more specific answer. Hopefully Marc can give you more details about > the flexcan implementations. There are at least 2 versions of the flexcan ip core in the wild. Due to lack of version numbers or other names I call them old and new here :). The newer one supports rx fifo mode, whereas the older one doesn't. The mainline flexcan driver just supports the new core [1]. The older core is found on coldfire processors. I don't know if there are coldfire cpus with the new flexcan core, too. The driver can be adopted to the old core if needed. The first cpus with the new core I got in touch with was the mx35 (arm11) and mx25 (arm9) both at the same time. Ask fsl which one was released first. After this there was mx28 (arm9) and there should be an mx53 (coretexa8) with flexcan too. > Other than an agreement on the compatible property, I believe we have > agreement on all the other code changes in these patches. Is this change > acceptable as is and if we get a better resolution on the fsl,flexcan > name later, we can update the documentation and driver then? cheers, Marc [1] http://lxr.linux.no/linux+v3.0.1/drivers/net/can/flexcan.c#L871 -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v11 5/6] flexcan: Prefer device tree clock frequency if available.
On 08/11/2011 06:07 PM, Robin Holt wrote:
> If our CAN device's device tree node has a clock-frequency property,
> then use that value for the can devices clock frequency. If not, fall
> back to asking the platform/mach code for the clock frequency associated
> with the flexcan device.
nitpicking follows inline:
> Signed-off-by: Robin Holt
> To: Kumar Gala
> To: Wolfgang Grandegger ,
> To: Marc Kleine-Budde ,
> To: U Bhaskar-B22300
> To: Scott Wood
> To: Grant Likely
> Cc: socketcan-c...@lists.berlios.de,
> Cc: net...@vger.kernel.org,
> Cc: PPC list
> Cc: devicetree-disc...@lists.ozlabs.org
> ---
> .../devicetree/bindings/net/can/fsl-flexcan.txt|2 +
> drivers/net/can/flexcan.c | 33
> +++-
> 2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> index c78dcbb..a4382c7 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
> @@ -11,6 +11,7 @@ Required properties:
>
> - reg : Offset and length of the register set for this device
> - interrupts : Interrupt tuple for this device
> +- clock-frequency : The oscillator frequency driving the flexcan device
>
> Example:
>
> @@ -19,4 +20,5 @@ Example:
>reg = <0x1c000 0x1000>;
>interrupts = <48 0x2>;
>interrupt-parent = <&mpic>;
> + clock-frequency = <0x0bebc1fc>;
Does the device tree support dec coded integers? IMHO a frequency is
best expressed in decimal.
>};
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 662f832..d40c38e 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define DRV_NAME "flexcan"
> @@ -929,12 +930,26 @@ static int __devinit flexcan_probe(struct
> platform_device *pdev)
> void __iomem *base;
> resource_size_t mem_size;
> int err, irq;
> + u32 clock_freq = 0;
>
> - clk = clk_get(&pdev->dev, NULL);
> - if (IS_ERR(clk)) {
> - dev_err(&pdev->dev, "no clock defined\n");
> - err = PTR_ERR(clk);
> - goto failed_clock;
> + if (pdev->dev.of_node) {
> + const u32 *clock_freq_p;
> +
> + clk = NULL;
Hmmm - what about moving the clk = NULL into the definition of clk?
> + clock_freq_p = of_get_property(pdev->dev.of_node,
> + "clock-frequency", NULL);
> + if (clock_freq_p)
> + clock_freq = *clock_freq_p;
> + }
> +
> + if (!clock_freq) {
> + clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock defined\n");
> + err = PTR_ERR(clk);
> + goto failed_clock;
> + }
> + clock_freq = clk_get_rate(clk);
> }
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -967,7 +982,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 = clock_freq;
> 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;
> @@ -1002,7 +1017,8 @@ static int __devinit flexcan_probe(struct
> platform_device *pdev)
> failed_map:
> release_mem_region(mem->start, mem_size);
> failed_get:
> - clk_put(clk);
> + if (clk)
> + clk_put(clk);
> failed_clock:
> return err;
> }
> @@ -1020,7 +1036,8 @@ 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);
> + if (priv->clk)
> + 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- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 0/5] [flexcan/powerpc] Add support for powerpc flexcan (freescale p1010)
On 08/10/2011 05:05 AM, Robin Holt wrote: > With all the patches applied, my p1010rdb works for communicating between > its two can ports and also can communicate with an external PSOC. I have > done no testing beyond compile testing on an arm system as I have no > access to an arm based system. > > For the first three patches in the series, I believe they are all ready > for forwarding to David S. Miller for the netdev tree. I think patch > 4 is ready for submission to the PPC85xx maintainer. Patch 5 changed > from the previous post by adding a second compatible string for the > fsl,p1010_flexcan. This patch series is working on phytec's pcm043 (mx35) based on current net-next. Feel free to add my Acked-by to all patches. good work, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 0/5] [flexcan/powerpc] Add support for powerpc flexcan (freescale p1010)
On 08/10/2011 12:11 PM, Robin Holt wrote: > On Wed, Aug 10, 2011 at 12:01:58PM +0200, Marc Kleine-Budde wrote: >> On 08/10/2011 05:05 AM, Robin Holt wrote: >>> With all the patches applied, my p1010rdb works for communicating between >>> its two can ports and also can communicate with an external PSOC. I have >>> done no testing beyond compile testing on an arm system as I have no >>> access to an arm based system. >>> >>> For the first three patches in the series, I believe they are all ready >>> for forwarding to David S. Miller for the netdev tree. I think patch >>> 4 is ready for submission to the PPC85xx maintainer. Patch 5 changed >>> from the previous post by adding a second compatible string for the >>> fsl,p1010_flexcan. >> >> One remark for the subjects. It seems no one is using "[$subsystem]" >> anymore, but rather "$subsystem:". > > I will, one day, get these patches subject lines correct ;) > > Did you happen to get a chance to test the patches on arm? It's still compiling... Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 0/5] [flexcan/powerpc] Add support for powerpc flexcan (freescale p1010)
On 08/10/2011 05:05 AM, Robin Holt wrote: > With all the patches applied, my p1010rdb works for communicating between > its two can ports and also can communicate with an external PSOC. I have > done no testing beyond compile testing on an arm system as I have no > access to an arm based system. > > For the first three patches in the series, I believe they are all ready > for forwarding to David S. Miller for the netdev tree. I think patch > 4 is ready for submission to the PPC85xx maintainer. Patch 5 changed > from the previous post by adding a second compatible string for the > fsl,p1010_flexcan. One remark for the subjects. It seems no one is using "[$subsystem]" anymore, but rather "$subsystem:". cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 4/5] [powerpc] Add flexcan device support for p1010rdb.
On 08/10/2011 05:06 AM, Robin Holt wrote:
> I added a simple clock source for the p1010rdb so the flexcan driver
> could determine a clock frequency. The p1010 can device only has an
> oscillator of system bus frequency divided by 2.
>
> Signed-off-by: Robin Holt
> Acked-by: Marc Kleine-Budde ,
> Acked-by: Wolfgang Grandegger ,
> To: U Bhaskar-B22300
> Cc: socketcan-c...@lists.berlios.de,
> Cc: net...@vger.kernel.org,
> Cc: PPC list
> Cc: Kumar Gala
> ---
> arch/powerpc/platforms/85xx/Kconfig|2 +
> arch/powerpc/platforms/85xx/Makefile |2 +
> arch/powerpc/platforms/85xx/clock.c| 53
>
> arch/powerpc/platforms/85xx/p1010rdb.c |8 +
> 4 files changed, 65 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/clock.c
>
> diff --git a/arch/powerpc/platforms/85xx/Kconfig
> b/arch/powerpc/platforms/85xx/Kconfig
> index 498534c..c4304ae 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -70,6 +70,8 @@ config MPC85xx_RDB
> config P1010_RDB
> bool "Freescale P1010RDB"
> select DEFAULT_UIMAGE
> + select HAVE_CAN_FLEXCAN if NET && CAN
> + select PPC_CLOCK if CAN_FLEXCAN
> help
> This option enables support for the MPC85xx RDB (P1010 RDB) board
>
> diff --git a/arch/powerpc/platforms/85xx/Makefile
> b/arch/powerpc/platforms/85xx/Makefile
> index a971b32..cc7f381 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -3,6 +3,8 @@
> #
> obj-$(CONFIG_SMP) += smp.o
>
> +obj-$(CONFIG_PPC_CLOCK) += clock.o
> +
> obj-$(CONFIG_MPC8540_ADS) += mpc85xx_ads.o
> obj-$(CONFIG_MPC8560_ADS) += mpc85xx_ads.o
> obj-$(CONFIG_MPC85xx_CDS) += mpc85xx_cds.o
> diff --git a/arch/powerpc/platforms/85xx/clock.c
> b/arch/powerpc/platforms/85xx/clock.c
> new file mode 100644
> index 000..16fae04
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/clock.c
> @@ -0,0 +1,53 @@
> +/*
> + * Copyright 2011 SGI, inc.
> + *
> + * This code is licensed for use under the GPL V2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +
> +/*
> + * p1010 needs to provide a clock source for the flexcan driver. The
> + * oscillator for the p1010 processor is only ever the system clock / 2.
> + */
> +
> +static struct clk *mpc85xx_clk_get(struct device *dev, const char *id)
> +{
> + if (!dev)
> + return ERR_PTR(-ENOENT);
> +
> +if (!dev->of_node ||
> +!of_device_is_compatible(dev->of_node, "fsl,flexcan"))
> +return ERR_PTR(-ENOENT);
> +
> + return NULL;
> +}
> +
> +static void mpc85xx_clk_put(struct clk *clk)
> +{
> + return;
> +}
> +
> +static unsigned long mpc85xx_clk_get_rate(struct clk *clk)
> +{
> + return fsl_get_sys_freq() / 2;
> +}
> +
> +static struct clk_interface mpc85xx_clk_functions = {
> + .clk_get = mpc85xx_clk_get,
> + .clk_get_rate = mpc85xx_clk_get_rate,
> + .clk_put = mpc85xx_clk_put,
> +};
> +
> +void __init mpc85xx_clk_init(void)
> +{
> + clk_functions = mpc85xx_clk_functions;
> +}
> +
git is even picker then me: "new blank line at EOF."
please fix
> diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c
> b/arch/powerpc/platforms/85xx/p1010rdb.c
> index d7387fa..5e52122 100644
> --- a/arch/powerpc/platforms/85xx/p1010rdb.c
> +++ b/arch/powerpc/platforms/85xx/p1010rdb.c
> @@ -81,6 +81,13 @@ static void __init p1010_rdb_setup_arch(void)
> printk(KERN_INFO "P1010 RDB board from Freescale Semiconductor\n");
> }
>
> +extern void mpc85xx_clk_init(void);
> +
> +static void __init p1010_rdb_init(void)
> +{
> + mpc85xx_clk_init();
> +}
> +
> static struct of_device_id __initdata p1010rdb_ids[] = {
> { .type = "soc", },
> { .compatible = "soc", },
> @@ -111,6 +118,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,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/4] [powerpc] Add flexcan device support for p1010rdb.
On 08/09/2011 04:55 PM, Robin Holt wrote: > On Tue, Aug 09, 2011 at 02:45:58PM +, U Bhaskar-B22300 wrote: >> Hi Robin, >> Where are you doing the irq handling ie request_irq() for the powerpc >> based P1010. >> Or the existing code of ARM based FlexCAN will work for P1010 ?? > > It appears that the of_device stuff got moved under the struct device > and that allows the request_irq() to just magically work. cool :) Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.
On 08/09/2011 02:40 PM, Robin Holt wrote: > On Tue, Aug 09, 2011 at 02:33:31PM +0200, Marc Kleine-Budde wrote: >> On 08/09/2011 07:55 AM, Robin Holt wrote: >>> I added a clock source for the p1010rdb so the flexcan driver >>> could find its clock frequency. >>> >>> Signed-off-by: Robin Holt >>> To: Marc Kleine-Budde , >>> To: Wolfgang Grandegger , >>> To: U Bhaskar-B22300 >>> Cc: socketcan-c...@lists.berlios.de, >>> Cc: net...@vger.kernel.org, >>> Cc: PPC list >> >> After fixing Wolfgangs remarks add by Acked-by, for what it's worth in >> the ppc-world :) > > Does that go for the other two patches in the series you have not > commented on in many revs as well? They look pretty good so far! I'll compile the next round on arm and hopefully find time to test on HW. cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.
On 08/09/2011 07:55 AM, Robin Holt wrote:
> I added a clock source for the p1010rdb so the flexcan driver
> could find its clock frequency.
>
> Signed-off-by: Robin Holt
> To: Marc Kleine-Budde ,
> To: Wolfgang Grandegger ,
> To: U Bhaskar-B22300
> Cc: socketcan-c...@lists.berlios.de,
> Cc: net...@vger.kernel.org,
> Cc: PPC list
After fixing Wolfgangs remarks add by Acked-by, for what it's worth in
the ppc-world :)
Some nitpicking from me :) - Darn you send another patch. This comments
still apply.
>
> ---
>
> Could I also get a ruling on the Kconfig language. I could do it either
> with the 85xx_HAVE_CAN_FLEXCAN in or out of the Kconfig file. It felt
> like the right thing to do was without, but the arm Kconfig files do
> it this way and a patch from freescale had something similar so I went
> this route.
>
> arch/powerpc/platforms/85xx/Kconfig|6 +++
> arch/powerpc/platforms/85xx/Makefile |1 +
> arch/powerpc/platforms/85xx/clock.c| 59
>
> arch/powerpc/platforms/85xx/p1010rdb.c |8
> 4 files changed, 74 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/platforms/85xx/clock.c
>
> diff --git a/arch/powerpc/platforms/85xx/Kconfig
> b/arch/powerpc/platforms/85xx/Kconfig
> index 498534c..ed4cf92 100644
> --- a/arch/powerpc/platforms/85xx/Kconfig
> +++ b/arch/powerpc/platforms/85xx/Kconfig
> @@ -26,6 +26,10 @@ config MPC8560_ADS
> help
> This option enables support for the MPC 8560 ADS board
>
> +config 85xx_HAVE_CAN_FLEXCAN
> + bool
> + select HAVE_CAN_FLEXCAN if NET && CAN
> +
> config MPC85xx_CDS
> bool "Freescale MPC85xx CDS"
> select DEFAULT_UIMAGE
> @@ -70,6 +74,8 @@ config MPC85xx_RDB
> config P1010_RDB
> bool "Freescale P1010RDB"
> select DEFAULT_UIMAGE
> + select 85xx_HAVE_CAN_FLEXCAN
> + select PPC_CLOCK if CAN_FLEXCAN
> help
> This option enables support for the MPC85xx RDB (P1010 RDB) board
>
> diff --git a/arch/powerpc/platforms/85xx/Makefile
> b/arch/powerpc/platforms/85xx/Makefile
> index a971b32..64ad7a4 100644
> --- a/arch/powerpc/platforms/85xx/Makefile
> +++ b/arch/powerpc/platforms/85xx/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_MPC85xx_DS) += mpc85xx_ds.o
> obj-$(CONFIG_MPC85xx_MDS) += mpc85xx_mds.o
> obj-$(CONFIG_MPC85xx_RDB) += mpc85xx_rdb.o
> obj-$(CONFIG_P1010_RDB) += p1010rdb.o
> +obj-$(CONFIG_PPC_CLOCK) += clock.o
> obj-$(CONFIG_P1022_DS)+= p1022_ds.o
> obj-$(CONFIG_P1023_RDS) += p1023_rds.o
> obj-$(CONFIG_P2040_RDB) += p2040_rdb.o corenet_ds.o
> diff --git a/arch/powerpc/platforms/85xx/clock.c
> b/arch/powerpc/platforms/85xx/clock.c
> new file mode 100644
> index 000..a25cbf3
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/clock.c
> @@ -0,0 +1,59 @@
Please add your copyright notice and add a license, e.g. "This file is
released under GPLv2"
> +
> +#include
> +#include
> +
> +#include
> +
> +#include
> +
> +/*
> + * p1010rdb needs to provide a clock source for the flexcan driver.
> + */
> +struct clk {
> + unsigned long rate;
> +} p1010_rdb_system_clock;
> +
> +static struct clk *p1010_rdb_clk_get(struct device *dev, const char *id)
> +{
> + 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);
> +
> + return &p1010_rdb_system_clock;
> +}
> +
> +static void p1010_rdb_clk_put(struct clk *clk)
> +{
> + return;
> +}
> +
> +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,
no indention here please, just one space before the "=". YMMV
> +};
> +
> +void __init p1010_rdb_clk_init(void)
> +{
> + p1010_rdb_system_clock.rate = fsl_get_sys_freq();
> + clk_functions = p1010_rdb_clk_functions;
> +}
> +
> diff --git a/arch/powerpc/platforms/85xx/p1010rdb.c
&g
Re: [RFC 4/4] [powerpc] Add flexcan device support for p1010rdb.
On 08/09/2011 01:40 PM, Robin Holt wrote: > On Tue, Aug 09, 2011 at 09:11:33AM +0200, Wolfgang Grandegger wrote: >>> + return &p1010_rdb_system_clock; >> >> Just returning fsl_get_sys_freq() here would already be fine. I'm also >> missing the factor of two here: >> >> return fsl_get_sys_freq() / 2; > > I am working on the other comments right now as well, but this one > brought up a good question. The old algorithm in the original freescale > patches I started with actually did, essentially: > > ...clock.freq = DIV_ROUND_CLOSEST(fsl_get_sys_freq() / 2, 1000) * 1000 > > The end result was before: > ...clock.freq=0x0bebc200 That's exactly 200 MHz > After: > ...clock.freq=0x0bebc1fe this is 199 999 998 Hz > Is that rounding relavent? _If_ 200 MHz is correct, this would be an error of 0.01%. But the fsl code rounds to closest KHz. Rounding introduces errors in most of the cases. IMHO it's better not to round here. For example the usual at91can is clocked with 99.532800 MHz, this is a least the value the arm clock framework reports. Cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC 4/4] [powerpc] Implement a p1010rdb clock source.
On 08/08/2011 10:49 AM, Wolfgang Grandegger wrote: > On 08/06/2011 10:59 PM, Kumar Gala wrote: >> >> On Aug 6, 2011, at 3:50 PM, Robin Holt wrote: >> >>> On Sat, Aug 06, 2011 at 11:52:45AM -0500, Kumar Gala wrote: >>>> >>>> On Aug 6, 2011, at 8:58 AM, Marc Kleine-Budde wrote: >>>> >>>>> On 08/06/2011 06:05 AM, Robin Holt wrote: >>>>>> flexcan driver needs the clk_get, clk_get_rate, etc functions >>>>>> to work. This patch provides the minimum functionality. >>>>> >>>>> This patch has to go via the powerpc git tree. Added >>>>> linuxppc-dev@lists.ozlabs.org on CC. >>>>> >>>>>> Signed-off-by: Robin Holt >>>>>> To: Marc Kleine-Budde >>>>>> To: Wolfgang Grandegger >>>>>> To: U Bhaskar-B22300 >>>>>> Cc: socketcan-c...@lists.berlios.de >>>>>> Cc: net...@vger.kernel.org >>>>>> --- >>>>>> arch/powerpc/platforms/85xx/p1010rdb.c | 78 >>>>>> >>>>>> 1 files changed, 78 insertions(+), 0 deletions(-) >>>> >>>> NAK. >>>> >>>> This doesn't look right at all. We should be doing something based on the >>>> device tree node that isn't board specific. >>>> >>>> I believe Bhaskar has a version of flexcan support that he's been working >>>> on cleanup up for upstream. >>> >>> That version may be similar to what is in the freescale BSP which puts >>> the clock functions inside flexcan.c >>> >>> The powerpc arch already provides a means for individual boards to provide >>> the clock functions. I am not posting this patch here for acceptance >>> for powerpc and I am sure I will get feedback there when I post to >>> their mailing list. I am posting it here only to show that the flexcan >>> developers earlier assertion that this can and should be done in the arch >>> tree is correct and will work for the p1010 assuming we can get changes >>> into the arch/powerpc directory to implement these clk_* functions. >> >> My point is that I don't think they should live in the arch code. The clk_* >> functions you want to implement are tied more the FlexCAN IP than anything >> arch specific. As such I believe they should be in the driver. >> >> For example when FSL has a P with FlexCAN on it, we should NOT have to >> add any arch code to support it. > > The Flexcan is found on ARM and now also on PowerPC SOCs. My current > understanding is that the ability to set the clock source and divider is > only available on PowerPC SOCs and therefore it's clearly arch specific > and should go to arch/powerpc/sysdev/fsl_soc.c if it's common for all > PowerPC platforms. What do you think? There is a bit in the CAN-Controller that selects the clock (at least on arm). The current driver just supports the bus clock. Support for the other, the oscillator clock, has not been implemented so far. IIRC the oscillator clock has a frequency that results in worse standard timing than the bus clock. cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC 4/4] [powerpc] Implement a p1010rdb clock source.
On 08/06/2011 06:05 AM, Robin Holt wrote:
> flexcan driver needs the clk_get, clk_get_rate, etc functions
> to work. This patch provides the minimum functionality.
This patch has to go via the powerpc git tree. Added
linuxppc-dev@lists.ozlabs.org on CC.
> Signed-off-by: Robin Holt
> To: Marc Kleine-Budde
> To: Wolfgang Grandegger
> To: U Bhaskar-B22300
> Cc: socketcan-c...@lists.berlios.de
> Cc: net...@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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Image generation for Efika is broken in 2.6.26
Scott Wood wrote: > On Tue, Jun 10, 2008 at 10:38:31AM -0400, Jon Smirl wrote: >> Why is my vmlinux.strip 3.2GB? Could a missing address be interpreted >> as -1 (0xFFF..) and cause the image to contain all of memory? >> >> -rw-r--r-- 1 jonsmirl jonsmirl 61738015 2008-06-10 10:31 vmlinux.o >> -rwxr-xr-x 1 jonsmirl jonsmirl 3224175192 2008-06-10 10:31 >> vmlinux.strip.3651 >> >> After it is packed vmlinux is 32MB. Packing gigabytes of zeros, maybe? >> >> -rwxr-xr-x 1 jonsmirl jonsmirl 32430673 2008-06-10 10:31 vmlinux >> >> This image won't load onto the Efika probably because it is trying to >> expand back to 3.2GB. >> >> With the same .config vmlinux is 4.1MB on 2.6.25 >> >> -rwxr-xr-x 1 jonsmirl jonsmirl 4119174 2008-06-09 20:09 vmlinux > > Are you using binutils 2.17? It has a bug that was triggered by the p_paddr > change. Do you get a bunch of warnings from the strip command? Same problem here with binutils-2.17. But no warnings from strip. Can you give me some pointers to the original bug report or the fix of the problem in the binutils svn. I've searched this mailinglist and the binutils bugzilla, but without success. cheers, Marc -- Marc Kleine-Budde Phone: +49-231-2826-924 Pengutronix - Linux Solutions for Science and Industry Vertretung West/Dortmund http://www.pengutronix.de signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Image generation for Efika is broken in 2.6.26
Robert Schwebel wrote: > On Tue, Jun 10, 2008 at 03:08:05PM -0400, Jon Smirl wrote: >>> I'm trying to convince OSELAS to rebuild the toolchain on Ubuntu >>> hardy but I'm having problems. >> After much mucking with my toolchain, I got it built with bin-utils >> 2.18. That fixed the Efika problem. Thanks for the tip. > > Marc, do we have a fix for that 2.17 problem? No, I just have heard yesterday that there is a problem, but have not seen any further details (I'm not reading this list). Is there a fix w/o update binutils to 2.18? Marc -- Marc Kleine-Budde Phone: +49-231-2826-924 Pengutronix - Linux Solutions for Science and Industry Vertretung West/Dortmund http://www.pengutronix.de signature.asc Description: OpenPGP digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
