* [PATCH v2] ARM: mm: report both sections from PMD
From: Catalin Marinas @ 2014-02-14 10:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGXu5jKG=8Fwd=yJEFF0-8tO0v4H_3sr_x2JLW4jm1Y2ouDFHQ@mail.gmail.com>
On Thu, Feb 13, 2014 at 07:52:03PM +0000, Kees Cook wrote:
> On Thu, Feb 13, 2014 at 9:12 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Wed, Feb 12, 2014 at 10:46:38PM +0000, Kees Cook wrote:
> >> diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
> >> index 03243f7eeddf..fb3de59ee811 100644
> >> --- a/arch/arm/include/asm/pgtable-3level.h
> >> +++ b/arch/arm/include/asm/pgtable-3level.h
> >> @@ -138,10 +138,6 @@
> >> #define pud_none(pud) (!pud_val(pud))
> >> #define pud_bad(pud) (!(pud_val(pud) & 2))
> >> #define pud_present(pud) (pud_val(pud))
> >> -#define pmd_table(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \
> >> - PMD_TYPE_TABLE)
> >> -#define pmd_sect(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \
> >> - PMD_TYPE_SECT)
> >> #define pmd_large(pmd) pmd_sect(pmd)
> >>
> >> #define pud_clear(pudp) \
> >> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> >> index 7d59b524f2af..934aa5b60c7c 100644
> >> --- a/arch/arm/include/asm/pgtable.h
> >> +++ b/arch/arm/include/asm/pgtable.h
> >> @@ -183,6 +183,10 @@ extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
> >>
> >> #define pmd_none(pmd) (!pmd_val(pmd))
> >> #define pmd_present(pmd) (pmd_val(pmd))
> >> +#define pmd_table(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \
> >> + PMD_TYPE_TABLE)
> >> +#define pmd_sect(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \
> >> + PMD_TYPE_SECT)
> >
> > Do you still need to move these two if you only use pmd_large()? AFAICT,
> > it is equivalent to pmd_sect().
>
> Why does pmd_sect exist? I can reduce it to just using pmd_large.
pmd_sect/pmd_table were there first and used by kvm. The pmd_large() was
added by Russell because the dump.c code was derived from x86 which uses
such macro. And we have another close relative, pmd_huge(), though only
defined if CONFIG_HUGETLB_PAGE.
We could drop pmd_large() in favour of pmd_sect() but it's up to
Russell. OTOH, when someone will try to add arm64 support, I'll most
likely ask for part of the x86 code to be turned into a generic library
and we'll need some common naming for such macros (e.g. pmd_large()).
--
Catalin
^ permalink raw reply
* [PATCH v7 03/12] mfd: omap-usb-host: Use clock names as per function for reference clocks
From: Lee Jones @ 2014-02-14 10:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392200333-28397-4-git-send-email-rogerq@ti.com>
> Use a meaningful name for the reference clocks so that it indicates the function.
>
> CC: Lee Jones <lee.jones@linaro.org>
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/mfd/omap-usb-host.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> index 60a3bed..ce620a8 100644
> --- a/drivers/mfd/omap-usb-host.c
> +++ b/drivers/mfd/omap-usb-host.c
> @@ -714,21 +714,21 @@ static int usbhs_omap_probe(struct platform_device *pdev)
> goto err_mem;
> }
>
> - omap->xclk60mhsp1_ck = devm_clk_get(dev, "xclk60mhsp1_ck");
> + omap->xclk60mhsp1_ck = devm_clk_get(dev, "refclk_60m_ext_p1");
You can't do that. These changes will have to be in the same patch as
the core change i.e. where they are initialised.
> if (IS_ERR(omap->xclk60mhsp1_ck)) {
> ret = PTR_ERR(omap->xclk60mhsp1_ck);
> dev_err(dev, "xclk60mhsp1_ck failed error:%d\n", ret);
> goto err_mem;
> }
>
> - omap->xclk60mhsp2_ck = devm_clk_get(dev, "xclk60mhsp2_ck");
> + omap->xclk60mhsp2_ck = devm_clk_get(dev, "refclk_60m_ext_p2");
> if (IS_ERR(omap->xclk60mhsp2_ck)) {
> ret = PTR_ERR(omap->xclk60mhsp2_ck);
> dev_err(dev, "xclk60mhsp2_ck failed error:%d\n", ret);
> goto err_mem;
> }
>
> - omap->init_60m_fclk = devm_clk_get(dev, "init_60m_fclk");
> + omap->init_60m_fclk = devm_clk_get(dev, "refclk_60m_int");
> if (IS_ERR(omap->init_60m_fclk)) {
> ret = PTR_ERR(omap->init_60m_fclk);
> dev_err(dev, "init_60m_fclk failed error:%d\n", ret);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v7 02/12] mfd: omap-usb-host: Get clocks based on hardware revision
From: Lee Jones @ 2014-02-14 10:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392200333-28397-3-git-send-email-rogerq@ti.com>
> Not all revisions have all the clocks so get the necessary clocks
> based on hardware revision.
>
> This should avoid un-necessary clk_get failure messages that were
> observed earlier.
>
> Be more strict and always fail on clk_get() error.
It might have been clearer if you'd broken these two pieces of
functionality changes into two different patches. In future it would
be preferred.
> CC: Lee Jones <lee.jones@linaro.org>
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/mfd/omap-usb-host.c | 92 +++++++++++++++++++++++++++++++--------------
> 1 file changed, 64 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> index 0c3c9a0..60a3bed 100644
> --- a/drivers/mfd/omap-usb-host.c
> +++ b/drivers/mfd/omap-usb-host.c
> @@ -665,22 +665,41 @@ static int usbhs_omap_probe(struct platform_device *pdev)
> goto err_mem;
> }
>
> - need_logic_fck = false;
> + /* Set all clocks as invalid to begin with */
> + omap->ehci_logic_fck = omap->init_60m_fclk = ERR_PTR(-ENODEV);
> + omap->utmi_p1_gfclk = omap->utmi_p2_gfclk = ERR_PTR(-ENODEV);
> + omap->xclk60mhsp1_ck = omap->xclk60mhsp2_ck = ERR_PTR(-ENODEV);
> +
For readability you should probably do these one per line.
> for (i = 0; i < omap->nports; i++) {
> - if (is_ehci_phy_mode(i) || is_ehci_tll_mode(i) ||
> - is_ehci_hsic_mode(i))
> - need_logic_fck |= true;
> + omap->utmi_clk[i] = ERR_PTR(-ENODEV);
> + omap->hsic480m_clk[i] = ERR_PTR(-ENODEV);
> + omap->hsic60m_clk[i] = ERR_PTR(-ENODEV);
> }
>
> - omap->ehci_logic_fck = ERR_PTR(-EINVAL);
> - if (need_logic_fck) {
> - omap->ehci_logic_fck = devm_clk_get(dev, "ehci_logic_fck");
Has this clock been renamed, or is it no longer required?
Perhaps you should be explicit in the commit log as to which clocks
you're removing.
> - if (IS_ERR(omap->ehci_logic_fck)) {
> - ret = PTR_ERR(omap->ehci_logic_fck);
> - dev_dbg(dev, "ehci_logic_fck failed:%d\n", ret);
> + /* for OMAP3 i.e. USBHS REV1 */
> + if (omap->usbhs_rev == OMAP_USBHS_REV1) {
> + need_logic_fck = false;
> + for (i = 0; i < omap->nports; i++) {
> + if (is_ehci_phy_mode(pdata->port_mode[i]) ||
> + is_ehci_tll_mode(pdata->port_mode[i]) ||
> + is_ehci_hsic_mode(pdata->port_mode[i]))
> +
> + need_logic_fck |= true;
> + }
> +
> + if (need_logic_fck) {
> + omap->ehci_logic_fck = clk_get(dev, "usbhost_120m_fck");
devm_clk_get()?
<snip>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] arm: dtsi: am335x-bone-common, usb0 is peripheral only
From: Markus Pargmann @ 2014-02-14 10:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213232552.GR28216@atomide.com>
Hi,
On Thu, Feb 13, 2014 at 03:25:52PM -0800, Tony Lindgren wrote:
> * Markus Pargmann <mpa@pengutronix.de> [140213 15:16]:
> > Hi,
> >
> > On Thu, Feb 13, 2014 at 02:54:38PM -0800, Tony Lindgren wrote:
> > > * Markus Pargmann <mpa@pengutronix.de> [140111 06:03]:
> > > > The PMIC is using usb0 vbus line as power source. It is also connected
> > > > to the am335x processor as vbus sense. But there is no possibility to
> > > > pullup usb0 vbus to operate as host. This patch fixes the dr_mode of usb0.
> > >
> > > That's the MUSB? AFAIK it's not possible to operate MUSB in peripheral
> > > only mode because the hardware does what it wants based on the ID
> > > pin state.
> >
> > Yes that's MUSB. The am335x reference manual describes that it is
> > possible to force peripheral/host mode by setting bit 7 (IDDIG_MUX) in
> > register USBnMODE to 1. Then it uses the bit written in bit 8 (IDDIG) of
> > register USBnMODE to set host/peripheral mode.
>
> OK
>
> > I am not sure if the driver supports it yet but I think the DTS should
> > contain the correct mode nevertheless, especially to avoid starting the
> > otg loops in the musb driver.
>
> Well there's one more thing to consider.. I think in the OTG role change
> case the VBUS is still driven externally from the original host, so the
> lack of VBUS does not always mean that host mode should be disabled.
I thought more about the hardware description than the possible role
changes through software protocols. In a hardware perspective, this USB
port is only in peripheral mode, as it can't drive VBUS. However is
there any support for role change protocols in the kernel yet?
Perhaps we have to add a seperate DT binding for usb role changes when
they are supported. This would help to describe the hardware
capabilities (host, peripheral or OTG) and the role change protocols
supported.
Regards,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH v2] can: xilinx CAN controller support.
From: Marc Kleine-Budde @ 2014-02-14 9:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ad62e4f0-fe91-4dac-9a74-ffe9c9128cb2@CH1EHSMHS024.ehs.local>
On 02/14/2014 10:36 AM, Appana Durga Kedareswara Rao wrote:
>>> +/* CAN register bit masks - XCAN_<REG>_<BIT>_MASK */
>>> +#define XCAN_SRR_CEN_MASK 0x00000002 /* CAN enable */
>>> +#define XCAN_SRR_RESET_MASK 0x00000001 /* Soft Reset the
>> CAN core */
>>> +#define XCAN_MSR_LBACK_MASK 0x00000002 /* Loop back
>> mode select */
>>> +#define XCAN_MSR_SLEEP_MASK 0x00000001 /* Sleep mode
>> select */
>>> +#define XCAN_BRPR_BRP_MASK 0x000000FF /* Baud rate
>> prescaler */
>>> +#define XCAN_BTR_SJW_MASK 0x00000180 /* Synchronous
>> jump width */
>>> +#define XCAN_BTR_TS2_MASK 0x00000070 /* Time segment
>> 2 */
>>> +#define XCAN_BTR_TS1_MASK 0x0000000F /* Time segment
>> 1 */
>>> +#define XCAN_ECR_REC_MASK 0x0000FF00 /* Receive error
>> counter */
>>> +#define XCAN_ECR_TEC_MASK 0x000000FF /* Transmit error
>> counter */
>>> +#define XCAN_ESR_ACKER_MASK 0x00000010 /* ACK error */
>>> +#define XCAN_ESR_BERR_MASK 0x00000008 /* Bit error */
>>> +#define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */
>>> +#define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */
>>> +#define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */
>>> +#define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full
>> */
>>> +#define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */
>>> +#define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning
>> */
>>> +#define XCAN_SR_NORMAL_MASK 0x00000008 /* Normal mode
>> */
>>> +#define XCAN_SR_LBACK_MASK 0x00000002 /* Loop back
>> mode */
>>> +#define XCAN_SR_CONFIG_MASK 0x00000001 /* Configuration
>> mode */
>>> +#define XCAN_IXR_TXFEMP_MASK 0x00004000 /* TX FIFO Empty
>> */
>>> +#define XCAN_IXR_WKUP_MASK 0x00000800 /* Wake up
>> interrupt */
>>> +#define XCAN_IXR_SLP_MASK 0x00000400 /* Sleep
>> interrupt */
>>> +#define XCAN_IXR_BSOFF_MASK 0x00000200 /* Bus off
>> interrupt */
>>> +#define XCAN_IXR_ERROR_MASK 0x00000100 /* Error interrupt
>> */
>>> +#define XCAN_IXR_RXNEMP_MASK 0x00000080 /* RX FIFO
>> NotEmpty intr */
>>> +#define XCAN_IXR_RXOFLW_MASK 0x00000040 /* RX FIFO
>> Overflow intr */
>>> +#define XCAN_IXR_RXOK_MASK 0x00000010 /* Message
>> received intr */
>>> +#define XCAN_IXR_TXOK_MASK 0x00000002 /* TX successful
>> intr */
>>> +#define XCAN_IXR_ARBLST_MASK 0x00000001 /* Arbitration
>> lost intr */
>>> +#define XCAN_IDR_ID1_MASK 0xFFE00000 /* Standard msg
>> identifier */
>>> +#define XCAN_IDR_SRR_MASK 0x00100000 /* Substitute
>> remote TXreq */
>>> +#define XCAN_IDR_IDE_MASK 0x00080000 /* Identifier
>> extension */
>>> +#define XCAN_IDR_ID2_MASK 0x0007FFFE /* Extended
>> message ident */
>>> +#define XCAN_IDR_RTR_MASK 0x00000001 /* Remote TX
>> request */
>>> +#define XCAN_DLCR_DLC_MASK 0xF0000000 /* Data length
>> code */
>>> +
>
> Need to use BIT() Macro for the Masks?
You can, but it IMHO only makes sense where only a single bit is set.
[...]
>>> + int waiting_ech_skb_num;
>>> + int xcan_echo_skb_max_tx;
>>> + int xcan_echo_skb_max_rx;
>>> + struct napi_struct napi;
>>> + spinlock_t ech_skb_lock;
>>> + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
>>> + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>>
>> Please remove read_reg, write_reg, as long as there isn't any BE support in
>> the driver, call them directly.
>>
>
>
> As per yours and Michal discussion I am keeping this here (endianess
> of the IP is not fixed at compile time).
Ok.
[...]
>>> +/**
>>> + * xcan_start_xmit - Starts the transmission
>>> + * @skb: sk_buff pointer that contains data to be Txed
>>> + * @ndev: Pointer to net_device structure
>>> + *
>>> + * This function is invoked from upper layers to initiate
>>> +transmission. This
>>> + * function uses the next available free txbuff and populates their
>>> +fields to
>>> + * start the transmission.
>>> + *
>>> + * Return: 0 on success and failure value on error */ static int
>>> +xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev) {
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &ndev->stats;
>>> + struct can_frame *cf = (struct can_frame *)skb->data;
>>> + u32 id, dlc, data[2] = {0, 0}, rtr = 0;
>>
>> I think you can drop the rtr varibale and use cf->can_id & CAN_RTR_FLAG
>> instead.
>>
>
> OK
>>> + unsigned long flags;
>>> +
>>> + if (can_dropped_invalid_skb(ndev, skb))
>>> + return NETDEV_TX_OK;
>>> +
>>> + /* Watch carefully on the bit sequence */
>>> + if (cf->can_id & CAN_EFF_FLAG) {
>>> + /* Extended CAN ID format */
>>> + id = ((cf->can_id & CAN_EFF_MASK) << XCAN_IDR_ID2_SHIFT)
>> &
>>> + XCAN_IDR_ID2_MASK;
>>> + id |= (((cf->can_id & CAN_EFF_MASK) >>
>>> + (CAN_EFF_ID_BITS-CAN_SFF_ID_BITS)) <<
>>> + XCAN_IDR_ID1_SHIFT) & XCAN_IDR_ID1_MASK;
>>> +
>>> + /* The substibute remote TX request bit should be "1"
>>> + * for extended frames as in the Xilinx CAN datasheet
>>> + */
>>> + id |= XCAN_IDR_IDE_MASK | XCAN_IDR_SRR_MASK;
>>> +
>>> + if (cf->can_id & CAN_RTR_FLAG) {
>>> + /* Extended frames remote TX request */
>>> + id |= XCAN_IDR_RTR_MASK;
>>> + rtr = 1;
>>> + }
>>> + } else {
>>> + /* Standard CAN ID format */
>>> + id = ((cf->can_id & CAN_SFF_MASK) << XCAN_IDR_ID1_SHIFT)
>> &
>>> + XCAN_IDR_ID1_MASK;
>>> +
>>> + if (cf->can_id & CAN_RTR_FLAG) {
>>> + /* Extended frames remote TX request */
>>> + id |= XCAN_IDR_SRR_MASK;
>>> + rtr = 1;
>>> + }
>>> + }
>>> +
>>> + dlc = (cf->can_dlc & 0xf) << XCAN_DLCR_DLC_SHIFT;
>>
>> No need to mask dlc, it's valid.
>>
> OK
>
>>> +
>>> + if (dlc > 0)
>>
>> You've copied my speudo code :)
>> But you have to use (cf->can_dlc > 0) here, as dlc is the shifted value.
>
> Yes :) I missed it will change
>
>>
>>> + data[0] = be32_to_cpup((__be32 *)(cf->data + 0));
>>> + if (dlc > 4)
>>> + data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
>>> +
>>> + can_put_echo_skb(skb, ndev, priv->ech_skb_next);
>>
>> can_put_echo_skb(skb, ndev,
>> priv->tx_head % priv->xcan_echo_skb_max_tx);
>>
>> priv->tx_head++;
>>
>
> Ok
>>> +
>>> + /* Write the Frame to Xilinx CAN TX FIFO */
>>> + priv->write_reg(priv, XCAN_TXFIFO_ID_OFFSET, id);
>>> + priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
>>> + if (!rtr) {
>>> + priv->write_reg(priv, XCAN_TXFIFO_DW1_OFFSET, data[0]);
>>> + priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
>>> + stats->tx_bytes += cf->can_dlc;
>>
>> Please add a comment which write triggers the tx. What in case of the rtr?
>> Which write triggers the tx then?
>>
>
> Ok Will Add
>
> In RTR Case the below write triggers the trasmission
> priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
> In Normal case this write
> priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
> Triggers the transmission.
>
> In Btw: Due to the limitations in the IP( Tx DLC register is a write only Register)
> I can't put this stats->tx_bytes += cf->can_dlc; in the tx interrupt routine.
No problem.
[...]
>>> + if (work_done < quota) {
>>> + napi_complete(napi);
>>> + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
>>> + ier |= (XCAN_IXR_RXOK_MASK |
>> XCAN_IXR_RXNEMP_MASK);
>>> + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
>>
>> Is this a read-modify-write register? I mean will an interrupt get disabled, if
>> you write a 0-bit in the IER register? What does the ICR register?
>
> ISR- Interrupt Status Register (Read only register)
> IER- Interrupt Enable Register(R/w register)
> ICR- Interrupt Clear Register.(write only register)
>
>
> The Interrupt Status Register (ISR) contains bits that are set when a particular interrupt condition occurs. If
> the corresponding mask bit in the Interrupt Enable Register is set, an interrupt is generated.
> Interrupt bits in the ISR can be cleared by writing to the Interrupt Clear Register
Thanks.
[...]
>>> +/**
>>> + * xcan_open - Driver open routine
>>> + * @ndev: Pointer to net_device structure
>>> + *
>>> + * This is the driver open routine.
>>> + * Return: 0 on success and failure value on error */ static int
>>> +xcan_open(struct net_device *ndev) {
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + int ret;
>>> +
>>> + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
>>> + ndev->name, (void *)ndev);
>>> + if (ret < 0) {
>>> + netdev_err(ndev, "Irq allocation for CAN failed\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* Set chip into reset mode */
>>> + ret = set_reset_mode(ndev);
>>> + if (ret < 0)
>>> + netdev_err(ndev, "mode resetting failed failed!\n");
>>
>> Is this critical?
>
> This condition usually won't fail.
> But if the controller has a problems at the h/w level in that case putted this err print.
> If you want me to change it as a warning will do
If there is a hardware level problem, is it better to return here with
an error (and free the IRQ)?
[...]
>>> +/**
>>> + * xcan_probe - Platform registration call
>>> + * @pdev: Handle to the platform device structure
>>> + *
>>> + * This function does all the memory allocation and registration for
>>> +the CAN
>>> + * device.
>>> + *
>>> + * Return: 0 on success and failure value on error */ static int
>>> +xcan_probe(struct platform_device *pdev) {
>>> + struct resource *res; /* IO mem resources */
>>> + struct net_device *ndev;
>>> + struct xcan_priv *priv;
>>> + int ret, fifodep;
>>> +
>>> + /* Create a CAN device instance */
>>> + ndev = alloc_candev(sizeof(struct xcan_priv),
>> XCAN_ECHO_SKB_MAX);
>>> + if (!ndev)
>>> + return -ENOMEM;
>>> +
>>> + priv = netdev_priv(ndev);
>>> + priv->dev = ndev;
>>> + priv->can.bittiming_const = &xcan_bittiming_const;
>>> + priv->can.do_set_bittiming = xcan_set_bittiming;
>>> + priv->can.do_set_mode = xcan_do_set_mode;
>>> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
>>> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>>> + CAN_CTRLMODE_BERR_REPORTING;
>>> +
>>> + /* Get IRQ for the device */
>>> + ndev->irq = platform_get_irq(pdev, 0);
>>> +
>>> + spin_lock_init(&priv->ech_skb_lock);
>>> + ndev->flags |= IFF_ECHO; /* We support local echo */
>>> +
>>> + platform_set_drvdata(pdev, ndev);
>>> + SET_NETDEV_DEV(ndev, &pdev->dev);
>>> + ndev->netdev_ops = &xcan_netdev_ops;
>>> +
>>> + /* Get the virtual base address for the device */
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv->reg_base)) {
>>> + ret = PTR_ERR(priv->reg_base);
>>> + goto err_free;
>>> + }
>>> + ndev->mem_start = res->start;
>>> + ndev->mem_end = res->end;
>>> +
>>> + priv->write_reg = xcan_write_reg;
>>> + priv->read_reg = xcan_read_reg;
>>> +
>>> + ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
>>> + &fifodep);
>>> + if (ret < 0)
>>> + goto err_free;
>>> + priv->xcan_echo_skb_max_tx = fifodep;
>>> +
>>> + ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
>>> + &fifodep);
>>> + if (ret < 0)
>>> + goto err_free;
>>> + priv->xcan_echo_skb_max_rx = fifodep;
>>> +
>>> + /* Getting the CAN devclk info */
>>> + priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
>>> + if (IS_ERR(priv->devclk)) {
>>> + dev_err(&pdev->dev, "Device clock not found.\n");
>>> + ret = PTR_ERR(priv->devclk);
>>> + goto err_free;
>>> + }
>>> +
>>> + /* Check for type of CAN device */
>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>> + "xlnx,zynq-can-1.00.a")) {
>>> + priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
>>> + if (IS_ERR(priv->aperclk)) {
>>> + dev_err(&pdev->dev, "aper clock not found\n");
>>> + ret = PTR_ERR(priv->aperclk);
>>> + goto err_free;
>>> + }
>>> + } else {
>>> + priv->aperclk = priv->devclk;
>>> + }
>>> +
>>> + ret = clk_prepare_enable(priv->devclk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "unable to enable device clock\n");
>>> + goto err_free;
>>> + }
>>> +
>>> + ret = clk_prepare_enable(priv->aperclk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "unable to enable aper clock\n");
>>> + goto err_unprepar_disabledev;
>>> + }
>>
>> Can you keep your clocks disaled if the interface is not up?
>
> I didn't get it will you please explain?
This feature s optional, but a a good practice.
This function gets called when the driver is loaded, i.e. during boot.
So the complete CAN core will be enabled and powered, even if the
interface is down. To reduce power consumption it's better to enable the
clocks in the open() function and disable in close(). If you access some
CAN registers during probe you have to enable the clock and you probably
have to enable it in the do_get_berr_counter callback, as it may be
called if the interface is still down.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 242 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140214/05ad9f6c/attachment-0001.sig>
^ permalink raw reply
* [PATCH v2 3/3] PCI: ARM: add support for generic PCI host controller
From: Arnd Bergmann @ 2014-02-14 9:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213195317.GQ13576@mudshark.cambridge.arm.com>
On Thursday 13 February 2014 19:53:17 Will Deacon wrote:
> On Thu, Feb 13, 2014 at 06:26:54PM +0000, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2014 at 05:28:20PM +0100, Arnd Bergmann wrote:
> >
> > > > Huh? The reg property clearly has the size in it (as shown in the
> > > > example below). I guess I was just asking for the description
> > > > here to say what the size was for the 2 compatibles since its
> > > > fixed and known.
> > >
> > > It's still an open question whether the config space in the reg
> > > property should cover all 256 buses or just the ones in the
> > > bus-range. In the latter case, it would be variable (but
> > > predictable) size.
> >
> > The 'describe the hardware principle' says the reg should be the
> > entire available ECAM/CAM region the hardware is able to support.
> >
> > This may be less than 256 busses, as ECAM allows the implementor to
> > select how many upper address bits are actually supported.
>
> Ok, but the ECAM/CAM base always corresponds to bus 0, right?
Ah, plus I suppose it ought to be a power-of-two size?
> > IMHO, the bus-range should be used to indicate the range of busses
> > discovered by the firmware, but we have historically tweaked it to
> > indicate the max range of bus numbers available on this bus (I think
> > to support the hack where two physical PCI domains were roughly glued
> > into a single Linux domain).
There is an interesting point about the domain assignment, brought to
my attention by Russell's comment about the hw_pci struct: If we want
to support arbitrary combinations of pci host bridges described in DT,
we need a better policy to decide what domain to use. The approaches
I've seen so far are:
1. We assume each host bridge in DT is a domain by itself. I think
we do that for all DT probed bridges on ARM (aside from shmobile)
at the moment. In some cases, the the host bridge is a really a
fiction made up by the host driver to couple various identical
but independent PCIe root ports, but the same fiction is shared
between DT and the PCI core view of it. This requires that we
enable the PCI domain code unconditionally, and breaks all user
space that doesn't understand domains (this should be rare but
can still exist for x86 based software).
2. The architecture or platform code decides and uses a method equivalent
to ARM's pci_common_init_dev() after it has found all host bridges.
The architecture "knows" how many domains it wants and calls
pci_common_init_dev() for each domain, and then the setup() callbacks
grab as many buses as they need within the domain. For a generic
multiplatform kernel, this means we need to add a top-level driver
that looks at all pci hosts in DT before any of them are probed.
It also means the pci host drivers can't be loadable modules.
3. We assume there is only one domain, and require each host bridge
in DT to specify a bus-range that is a subset of the available 256
bus numbers. This should work for anything but really big systems
with many hot-pluggable ports, since we need to reserve a few bus
numbers on each port for hotplugging.
4. Like 3, but start a new domain if the bus-range properties don't
fit in the existing domains.
5. Like 3, but specify a generic "pci-domain" property for DT
that allows putting host bridges into explicit domains in
a predictable way.
Arnd
^ permalink raw reply
* [PATCH v7 01/12] mfd: omap-usb-host: Use resource managed clk_get()
From: Lee Jones @ 2014-02-14 9:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392200333-28397-2-git-send-email-rogerq@ti.com>
> Use devm_clk_get() instead of clk_get().
>
> CC: Lee Jones <lee.jones@linaro.org>
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> drivers/mfd/omap-usb-host.c | 81 +++++++++------------------------------------
> 1 file changed, 16 insertions(+), 65 deletions(-)
Applied, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 0/3] arm: at91: update defconfigs
From: Nicolas Ferre @ 2014-02-14 9:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392147841-7815-1-git-send-email-alexandre.belloni@free-electrons.com>
On 11/02/2014 20:43, Alexandre Belloni :
> A few at91 defconfigs where not updated in a long time, refresh the configs for
> at91sam9rl and at91sam9260/at91sam9g20.
>
> Alexandre Belloni (3):
> arm: at91sam9rl: refresh defconfig
> arm: at91sam9260_9g20: remove useless configuration
> arm: at91sam9260_9g20: refresh configuration
>
> arch/arm/configs/at91sam9260_9g20_defconfig | 9 ++-------
> arch/arm/configs/at91sam9rl_defconfig | 10 +++++++---
> 2 files changed, 9 insertions(+), 10 deletions(-)
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
stacked in at91-3.15-cleanup
Thanks, bye,
--
Nicolas Ferre
^ permalink raw reply
* [LTP] How to get the Linux kernel coverage data on ARM arch when I run LTP test?
From: Peter Oberparleiter @ 2014-02-14 9:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+G9fYuT4TSs=3j6K9zztsTgFNVp2YG6JT-2Q_L==1P5G+SBew@mail.gmail.com>
On 14.02.2014 10:02, Naresh Kamboju wrote:
> root at linaro-developer:/tmp/linux-linaro-3.13-2014.01# gcov kernel/gcov/base.c -o
> /sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/kernel/gcov/
> File 'kernel/gcov/base.c'
> Lines executed:43.18% of 44
> Creating 'base.c.gcov'
>
> root at linaro-developer:/tmp/linux-linaro-3.13-2014.01#
>
> The above experiment gives coverage of a single file base.c when i run
> gcov manually.
> Is there any way to get Linux kernel coverage of all files after
> running LTP test cases ?
1. Reset coverage data: lcov -z
2. Run LTP (or any other test case)
3. Capture coverage data: lcov -c -o coverage.info
4. Generate HTML output: genhtml coverage.info -o out
5. View HTML output: <browser> out/index.html
More information on lcov can be found on the page behind the second URL
your quoted in your mail.
Regards,
Peter Oberparleiter
--
Peter Oberparleiter
Linux on System z Development - IBM Germany
^ permalink raw reply
* [PATCH v2] can: xilinx CAN controller support.
From: Appana Durga Kedareswara Rao @ 2014-02-14 9:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52FD1B16.6020600@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
> Sent: Friday, February 14, 2014 12:51 AM
> To: Appana Durga Kedareswara Rao; wg at grandegger.com; Michal Simek;
> grant.likely at linaro.org; robh+dt at kernel.org; linux-can at vger.kernel.org
> Cc: netdev at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; devicetree at vger.kernel.org; Appana Durga
> Kedareswara Rao
> Subject: Re: [PATCH v2] can: xilinx CAN controller support.
>
> On 02/12/2014 08:10 AM, Kedareswara rao Appana wrote:
> > This patch adds xilinx CAN controller support.
> > This driver supports both ZYNQ CANPS IP and Soft IP AXI CAN
> > controller.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > This patch is rebased on the 3.14 rc2 kernel.
> > Changes for v2:
> > - Updated with the review comments.
> > - Removed unnecessary debug prints.
> > - included tx,rx fifo depths in ZYNQ CANPS case also.
> > ---
> > .../devicetree/bindings/net/can/xilinx_can.txt | 45 +
> > drivers/net/can/Kconfig | 7 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/xilinx_can.c | 1153 ++++++++++++++++++++
> > 4 files changed, 1206 insertions(+), 0 deletions(-) create mode
> > 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > create mode 100644 drivers/net/can/xilinx_can.c
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > new file mode 100644
> > index 0000000..0e57103
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > @@ -0,0 +1,45 @@
> > +Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
> > +---------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Should be "xlnx,zynq-can-1.00.a" for Zynq
> CAN
> > + controllers and "xlnx,axi-can-1.00.a" for Axi CAN
> > + controllers.
> > +- reg : Physical base address and size of the Axi CAN/Zynq
> > + CANPS registers map.
> > +- interrupts : Property with a value describing the interrupt
> > + number.
> > +- interrupt-parent : Must be core interrupt controller
> > +- clock-names : List of input clock names - "ref_clk",
> "aper_clk"
> > + (See clock bindings for details. Two clocks are
> > + required for Zynq CAN. For Axi CAN
> > + case it is one(ref_clk)).
> > +- clocks : Clock phandles (see clock bindings for details).
> > +- tx-fifo-depth : Can Tx fifo depth.
> > +- rx-fifo-depth : Can Rx fifo depth.
> > +
> > +
> > +Example:
> > +
> > +For Zynq CANPS Dts file:
> > + zynq_can_0: zynq-can at e0008000 {
> > + compatible = "xlnx,zynq-can-1.00.a";
> > + clocks = <&clkc 19>, <&clkc 36>;
> > + clock-names = "ref_clk", "aper_clk";
> > + reg = <0xe0008000 0x1000>;
> > + interrupts = <0 28 4>;
> > + interrupt-parent = <&intc>;
> > + tx-fifo-depth = <0x40>;
> > + rx-fifo-depth = <0x40>;
> > + };
> > +For Axi CAN Dts file:
> > + axi_can_0: axi-can at 40000000 {
> > + compatible = "xlnx,axi-can-1.00.a";
> > + clocks = <&clkc 0>;
> > + clock-names = "ref_clk" ;
> > + reg = <0x40000000 0x10000>;
> > + interrupt-parent = <&intc>;
> > + interrupts = <0 59 1>;
> > + tx-fifo-depth = <0x40>;
> > + rx-fifo-depth = <0x40>;
> > + };
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index
> > 9e7d95d..b180239 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -125,6 +125,13 @@ config CAN_GRCAN
> > endian syntheses of the cores would need some modifications on
> > the hardware level to work.
> >
> > +config CAN_XILINXCAN
> > + tristate "Xilinx CAN"
> > + depends on ARCH_ZYNQ || MICROBLAZE
> > + ---help---
> > + Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > + Zynq CANPS IP.
> > +
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index
> > c744039..0b8e11e 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-
> ican3.o
> > obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o
> > obj-$(CONFIG_PCH_CAN) += pch_can.o
> > obj-$(CONFIG_CAN_GRCAN) += grcan.o
> > +obj-$(CONFIG_CAN_XILINXCAN) += xilinx_can.o
> >
> > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG diff --git
> > a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c new file
> > mode 100644 index 0000000..642e6b4
> > --- /dev/null
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -0,0 +1,1153 @@
> > +/* Xilinx CAN device driver
> > + *
> > + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> > + * Copyright (C) 2009 PetaLogix. All rights reserved.
> > + *
> > + * Description:
> > + * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
> > + * This program is free software: you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/can/dev.h>
> > +#include <linux/can/error.h>
> > +#include <linux/can/led.h>
> > +
> > +#define DRIVER_NAME "XILINX_CAN"
> > +
> > +/* CAN registers set */
> > +#define XCAN_SRR_OFFSET 0x00 /* Software reset */
> > +#define XCAN_MSR_OFFSET 0x04 /* Mode select */
> > +#define XCAN_BRPR_OFFSET 0x08 /* Baud rate prescaler */
> > +#define XCAN_BTR_OFFSET 0x0C /* Bit timing */
> > +#define XCAN_ECR_OFFSET 0x10 /* Error counter */
> > +#define XCAN_ESR_OFFSET 0x14 /* Error status */
> > +#define XCAN_SR_OFFSET 0x18 /* Status */
> > +#define XCAN_ISR_OFFSET 0x1C /* Interrupt status */
> > +#define XCAN_IER_OFFSET 0x20 /* Interrupt enable */
> > +#define XCAN_ICR_OFFSET 0x24 /* Interrupt clear */
> > +#define XCAN_TXFIFO_ID_OFFSET 0x30 /* TX FIFO ID */
> > +#define XCAN_TXFIFO_DLC_OFFSET 0x34 /* TX FIFO DLC */
> > +#define XCAN_TXFIFO_DW1_OFFSET 0x38 /* TX FIFO Data
> Word 1 */
> > +#define XCAN_TXFIFO_DW2_OFFSET 0x3C /* TX FIFO Data
> Word 2 */
> > +#define XCAN_RXFIFO_ID_OFFSET 0x50 /* RX FIFO ID */
> > +#define XCAN_RXFIFO_DLC_OFFSET 0x54 /* RX FIFO DLC */
> > +#define XCAN_RXFIFO_DW1_OFFSET 0x58 /* RX FIFO Data
> Word 1 */
> > +#define XCAN_RXFIFO_DW2_OFFSET 0x5C /* RX FIFO Data
> Word 2 */
>
> Can you define all register offsets via an enum please.
>
Ok
> > +/* CAN register bit masks - XCAN_<REG>_<BIT>_MASK */
> > +#define XCAN_SRR_CEN_MASK 0x00000002 /* CAN enable */
> > +#define XCAN_SRR_RESET_MASK 0x00000001 /* Soft Reset the
> CAN core */
> > +#define XCAN_MSR_LBACK_MASK 0x00000002 /* Loop back
> mode select */
> > +#define XCAN_MSR_SLEEP_MASK 0x00000001 /* Sleep mode
> select */
> > +#define XCAN_BRPR_BRP_MASK 0x000000FF /* Baud rate
> prescaler */
> > +#define XCAN_BTR_SJW_MASK 0x00000180 /* Synchronous
> jump width */
> > +#define XCAN_BTR_TS2_MASK 0x00000070 /* Time segment
> 2 */
> > +#define XCAN_BTR_TS1_MASK 0x0000000F /* Time segment
> 1 */
> > +#define XCAN_ECR_REC_MASK 0x0000FF00 /* Receive error
> counter */
> > +#define XCAN_ECR_TEC_MASK 0x000000FF /* Transmit error
> counter */
> > +#define XCAN_ESR_ACKER_MASK 0x00000010 /* ACK error */
> > +#define XCAN_ESR_BERR_MASK 0x00000008 /* Bit error */
> > +#define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */
> > +#define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */
> > +#define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */
> > +#define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full
> */
> > +#define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */
> > +#define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning
> */
> > +#define XCAN_SR_NORMAL_MASK 0x00000008 /* Normal mode
> */
> > +#define XCAN_SR_LBACK_MASK 0x00000002 /* Loop back
> mode */
> > +#define XCAN_SR_CONFIG_MASK 0x00000001 /* Configuration
> mode */
> > +#define XCAN_IXR_TXFEMP_MASK 0x00004000 /* TX FIFO Empty
> */
> > +#define XCAN_IXR_WKUP_MASK 0x00000800 /* Wake up
> interrupt */
> > +#define XCAN_IXR_SLP_MASK 0x00000400 /* Sleep
> interrupt */
> > +#define XCAN_IXR_BSOFF_MASK 0x00000200 /* Bus off
> interrupt */
> > +#define XCAN_IXR_ERROR_MASK 0x00000100 /* Error interrupt
> */
> > +#define XCAN_IXR_RXNEMP_MASK 0x00000080 /* RX FIFO
> NotEmpty intr */
> > +#define XCAN_IXR_RXOFLW_MASK 0x00000040 /* RX FIFO
> Overflow intr */
> > +#define XCAN_IXR_RXOK_MASK 0x00000010 /* Message
> received intr */
> > +#define XCAN_IXR_TXOK_MASK 0x00000002 /* TX successful
> intr */
> > +#define XCAN_IXR_ARBLST_MASK 0x00000001 /* Arbitration
> lost intr */
> > +#define XCAN_IDR_ID1_MASK 0xFFE00000 /* Standard msg
> identifier */
> > +#define XCAN_IDR_SRR_MASK 0x00100000 /* Substitute
> remote TXreq */
> > +#define XCAN_IDR_IDE_MASK 0x00080000 /* Identifier
> extension */
> > +#define XCAN_IDR_ID2_MASK 0x0007FFFE /* Extended
> message ident */
> > +#define XCAN_IDR_RTR_MASK 0x00000001 /* Remote TX
> request */
> > +#define XCAN_DLCR_DLC_MASK 0xF0000000 /* Data length
> code */
> > +
Need to use BIT() Macro for the Masks?
> > +#define XCAN_INTR_ALL (XCAN_IXR_TXOK_MASK |
> XCAN_IXR_BSOFF_MASK |\
> > + XCAN_IXR_WKUP_MASK |
> XCAN_IXR_SLP_MASK | \
> > + XCAN_IXR_RXNEMP_MASK |
> XCAN_IXR_ERROR_MASK | \
> > + XCAN_IXR_ARBLST_MASK |
> XCAN_IXR_RXOK_MASK)
> > +
> > +/* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
> > +#define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width
> */
> > +#define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */
> > +#define XCAN_IDR_ID1_SHIFT 21 /* Standard Messg
> Identifier */
> > +#define XCAN_IDR_ID2_SHIFT 1 /* Extended Message
> Identifier */
> > +#define XCAN_DLCR_DLC_SHIFT 28 /* Data length code */
> > +#define XCAN_ESR_REC_SHIFT 8 /* Rx Error Count */
> > +
> > +/* CAN frame length constants */
> > +#define XCAN_ECHO_SKB_MAX 64
> > +#define XCAN_FRAME_MAX_DATA_LEN 8
> > +#define XCAN_TIMEOUT (50 * HZ)
>
> This is 50 seconds, is this intentional?
>
Sorry will make it as 1*HZ.
> > +
> > +/**
> > + * struct xcan_priv - This definition define CAN driver instance
> > + * @can: CAN private data structure.
> > + * @open_time: For holding timeout values
>
> Please remove open_time completely from the driver.
Ok
>
> > + * @waiting_ech_skb_index: Pointer for skb
> > + * @ech_skb_next: This tell the next packet in the queue
> > + * @waiting_ech_skb_num: Gives the number of packets waiting
> > + * @xcan_echo_skb_max_tx: Maximum number packets the driver
> can send
> > + * @xcan_echo_skb_max_rx: Maximum number packets the driver
> can receive
> > + * @napi: NAPI structure
> > + * @ech_skb_lock: For spinlock purpose
> > + * @read_reg: For reading data from CAN registers
> > + * @write_reg: For writing data to CAN registers
> > + * @dev: Network device data structure
> > + * @reg_base: Ioremapped address to registers
> > + * @irq_flags: For request_irq()
> > + * @aperclk: Pointer to struct clk
> > + * @devclk: Pointer to struct clk
> > + */
> > +struct xcan_priv {
> > + struct can_priv can;
> > + int open_time;
> > + int waiting_ech_skb_index;
> > + int ech_skb_next;
>
> please make them:
>
> unsigned int tx_head;
> unsigned int tx_tail;
>
> I'll explain how to use them later. Have a look at the ti_hecc driver.
Ok
>
> > + int waiting_ech_skb_num;
> > + int xcan_echo_skb_max_tx;
> > + int xcan_echo_skb_max_rx;
> > + struct napi_struct napi;
> > + spinlock_t ech_skb_lock;
> > + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
> > + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>
> Please remove read_reg, write_reg, as long as there isn't any BE support in
> the driver, call them directly.
>
As per yours and Michal discussion I am keeping this here (endianess of the IP is not fixed at compile time).
> > + struct net_device *dev;
> > + void __iomem *reg_base;
> > + unsigned long irq_flags;
> > + struct clk *aperclk;
> > + struct clk *devclk;
> > +};
> > +
> > +/* CAN Bittiming constants as per Xilinx CAN specs */ static const
> > +struct can_bittiming_const xcan_bittiming_const = {
> > + .name = DRIVER_NAME,
> > + .tseg1_min = 1,
> > + .tseg1_max = 16,
> > + .tseg2_min = 1,
> > + .tseg2_max = 8,
> > + .sjw_max = 4,
> > + .brp_min = 1,
> > + .brp_max = 256,
> > + .brp_inc = 1,
> > +};
> > +
> > +/**
> > + * xcan_write_reg - Write a value to the device register
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + * @val: Value to write at the Register offset
> > + *
> > + * Write data to the paricular CAN register */ static void
> > +xcan_write_reg(const struct xcan_priv *priv, int reg, u32 val)
>
> Please use the enum for instead of an int for the reg.
Ok
>
> > +{
> > + writel(val, priv->reg_base + reg);
> > +}
> > +
> > +/**
> > + * xcan_read_reg - Read a value from the device register
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + *
> > + * Read data from the particular CAN register
> > + * Return: value read from the CAN register */ static u32
> > +xcan_read_reg(const struct xcan_priv *priv, int reg) {
>
> same here
Ok
>
> > + return readl(priv->reg_base + reg);
> > +}
> > +
> > +/**
> > + * set_reset_mode - Resets the CAN device mode
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver reset mode routine.The driver
> > + * enters into configuration mode.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +set_reset_mode(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + unsigned long timeout;
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > +
> > + timeout = jiffies + XCAN_TIMEOUT;
> > + while (!(priv->read_reg(priv, XCAN_SR_OFFSET) &
> XCAN_SR_CONFIG_MASK)) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev, "timedout waiting for config
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_set_bittiming - CAN set bit timing routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver set bittiming routine.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_set_bittiming(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct can_bittiming *bt = &priv->can.bittiming;
> > + u32 btr0, btr1;
> > + u32 is_config_mode;
> > +
> > + /* Check whether Xilinx CAN is in configuration mode.
> > + * It cannot set bit timing if Xilinx CAN is not in configuration mode.
> > + */
> > + is_config_mode = priv->read_reg(priv, XCAN_SR_OFFSET) &
> > + XCAN_SR_CONFIG_MASK;
> > + if (!is_config_mode) {
> > + netdev_alert(ndev,
> > + "Cannot set bittiming can is not in config mode\n");
> > + return -EPERM;
> > + }
> > +
> > + /* Setting Baud Rate prescalar value in BRPR Register */
> > + btr0 = (bt->brp - 1) & XCAN_BRPR_BRP_MASK;
> > +
> > + /* Setting Time Segment 1 in BTR Register */
> > + btr1 = (bt->prop_seg + bt->phase_seg1 - 1) & XCAN_BTR_TS1_MASK;
> > +
> > + /* Setting Time Segment 2 in BTR Register */
> > + btr1 |= ((bt->phase_seg2 - 1) << XCAN_BTR_TS2_SHIFT) &
> > + XCAN_BTR_TS2_MASK;
> > +
> > + /* Setting Synchronous jump width in BTR Register */
> > + btr1 |= ((bt->sjw - 1) << XCAN_BTR_SJW_SHIFT) &
> XCAN_BTR_SJW_MASK;
>
> All the masking should not be needed, as the bit timing is calculated within
> the bounds you specified.
Ok
>
> > + priv->write_reg(priv, XCAN_BRPR_OFFSET, btr0);
> > + priv->write_reg(priv, XCAN_BTR_OFFSET, btr1);
> > +
> > + netdev_dbg(ndev, "BRPR=0x%08x, BTR=0x%08x\n",
> > + priv->read_reg(priv, XCAN_BRPR_OFFSET),
> > + priv->read_reg(priv, XCAN_BTR_OFFSET));
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_start - This the drivers start routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the drivers start routine.
> > + * Based on the State of the CAN device it puts
> > + * the CAN device into a proper mode.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_start(struct net_device *ndev)
>
> Please name the function xcan_chip_start(), to for a common naming like
> the flexcan and at91 driver.
>
OK
> > +{
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 err;
> > + unsigned long timeout;
> > +
> > + /* Check if it is in reset mode */
> > + if (priv->can.state != CAN_STATE_STOPPED)
>
> Don't depend on any state here, I suggest to do a softreset (or
> equivalent) of you CAN core and configure everything.
>
Ok
> > + err = set_reset_mode(ndev);
> > + if (err < 0)
> > + return err;
> > +
> > + /* Enable interrupts */
> > + priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
> > +
> > + /* Check whether it is loopback mode or normal mode */
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > + /* Put device into loopback mode */
> > + priv->write_reg(priv, XCAN_MSR_OFFSET,
> XCAN_MSR_LBACK_MASK);
> > + else
> > + /* The device is in normal mode */
> > + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > +
> > + if (priv->can.state == CAN_STATE_STOPPED) {
> > + /* Enable Xilinx CAN */
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_CEN_MASK);
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + timeout = jiffies + XCAN_TIMEOUT;
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> > + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> > + & XCAN_SR_LBACK_MASK) == 0) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev,
> > + "timedout for loopback
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > + } else {
> > + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> > + & XCAN_SR_NORMAL_MASK) == 0) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev,
> > + "timedout for normal
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > + }
> > + netdev_dbg(ndev, "status:#x%08x\n",
> > + priv->read_reg(priv, XCAN_SR_OFFSET));
> > + }
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_do_set_mode - This sets the mode of the driver
> > + * @ndev: Pointer to net_device structure
> > + * @mode: Tells the mode of the driver
> > + *
> > + * This check the drivers state and calls the
> > + * the corresponding modes to set.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_do_set_mode(struct net_device *ndev, enum can_mode mode) {
> > + int ret;
> > +
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + ret = xcan_start(ndev);
> > + if (ret < 0)
> > + netdev_err(ndev, "xcan_start failed!\n");
> > + netif_wake_queue(ndev);
> > + break;
> > + default:
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * xcan_start_xmit - Starts the transmission
> > + * @skb: sk_buff pointer that contains data to be Txed
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This function is invoked from upper layers to initiate
> > +transmission. This
> > + * function uses the next available free txbuff and populates their
> > +fields to
> > + * start the transmission.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + u32 id, dlc, data[2] = {0, 0}, rtr = 0;
>
> I think you can drop the rtr varibale and use cf->can_id & CAN_RTR_FLAG
> instead.
>
OK
> > + unsigned long flags;
> > +
> > + if (can_dropped_invalid_skb(ndev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + /* Watch carefully on the bit sequence */
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + /* Extended CAN ID format */
> > + id = ((cf->can_id & CAN_EFF_MASK) << XCAN_IDR_ID2_SHIFT)
> &
> > + XCAN_IDR_ID2_MASK;
> > + id |= (((cf->can_id & CAN_EFF_MASK) >>
> > + (CAN_EFF_ID_BITS-CAN_SFF_ID_BITS)) <<
> > + XCAN_IDR_ID1_SHIFT) & XCAN_IDR_ID1_MASK;
> > +
> > + /* The substibute remote TX request bit should be "1"
> > + * for extended frames as in the Xilinx CAN datasheet
> > + */
> > + id |= XCAN_IDR_IDE_MASK | XCAN_IDR_SRR_MASK;
> > +
> > + if (cf->can_id & CAN_RTR_FLAG) {
> > + /* Extended frames remote TX request */
> > + id |= XCAN_IDR_RTR_MASK;
> > + rtr = 1;
> > + }
> > + } else {
> > + /* Standard CAN ID format */
> > + id = ((cf->can_id & CAN_SFF_MASK) << XCAN_IDR_ID1_SHIFT)
> &
> > + XCAN_IDR_ID1_MASK;
> > +
> > + if (cf->can_id & CAN_RTR_FLAG) {
> > + /* Extended frames remote TX request */
> > + id |= XCAN_IDR_SRR_MASK;
> > + rtr = 1;
> > + }
> > + }
> > +
> > + dlc = (cf->can_dlc & 0xf) << XCAN_DLCR_DLC_SHIFT;
>
> No need to mask dlc, it's valid.
>
OK
> > +
> > + if (dlc > 0)
>
> You've copied my speudo code :)
> But you have to use (cf->can_dlc > 0) here, as dlc is the shifted value.
Yes :) I missed it will change
>
> > + data[0] = be32_to_cpup((__be32 *)(cf->data + 0));
> > + if (dlc > 4)
> > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
> > +
> > + can_put_echo_skb(skb, ndev, priv->ech_skb_next);
>
> can_put_echo_skb(skb, ndev,
> priv->tx_head % priv->xcan_echo_skb_max_tx);
>
> priv->tx_head++;
>
Ok
> > +
> > + /* Write the Frame to Xilinx CAN TX FIFO */
> > + priv->write_reg(priv, XCAN_TXFIFO_ID_OFFSET, id);
> > + priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
> > + if (!rtr) {
> > + priv->write_reg(priv, XCAN_TXFIFO_DW1_OFFSET, data[0]);
> > + priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
> > + stats->tx_bytes += cf->can_dlc;
>
> Please add a comment which write triggers the tx. What in case of the rtr?
> Which write triggers the tx then?
>
Ok Will Add
In RTR Case the below write triggers the trasmission
priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
In Normal case this write
priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
Triggers the transmission.
In Btw: Due to the limitations in the IP( Tx DLC register is a write only Register)
I can't put this stats->tx_bytes += cf->can_dlc; in the tx interrupt routine.
> > + }
> > +
> > + priv->ech_skb_next = (priv->ech_skb_next + 1) %
> > + priv->xcan_echo_skb_max_tx;
>
> Please remove, it's not needed.
>
Ok
> > +
> > + spin_lock_irqsave(&priv->ech_skb_lock, flags);
> > + priv->waiting_ech_skb_num++;
> > + spin_unlock_irqrestore(&priv->ech_skb_lock, flags);
>
> All 3 not needed.
>
Ok
> > +
> > + /* Check if the TX buffer is full */
> > + if (priv->read_reg(priv, XCAN_SR_OFFSET) & XCAN_SR_TXFLL_MASK)
> {
> > + netif_stop_queue(ndev);
> > + netdev_err(ndev, "TX register is still full!\n");
> > + return NETDEV_TX_BUSY;
>
> If this is true, there is a Bug in the flow control. It should be moved to the
> beginning of the function, see at91_can's xmit function.
Yes this Condition Becomes true.
Will put the below check in the Beginning of this function
>
> /* Check if the TX buffer is full */
> if (unlikely(priv->read_reg(priv, XCAN_SR_OFFSET) &
> XCAN_SR_TXFLL_MASK)) {
> netif_stop_queue(ndev);
> netdev_err(ndev,
> "BUG!, TX FIFO full when queue awake!\n");
> return NETDEV_TX_BUSY;
> }
>
> > + } else if (priv->waiting_ech_skb_num == priv-
> >xcan_echo_skb_max_tx) {
> > + netif_stop_queue(ndev);
> > + netdev_err(ndev, "waiting:0x%08x, max:0x%08x\n",
> > + priv->waiting_ech_skb_num, priv-
> >xcan_echo_skb_max_tx);
> > + return NETDEV_TX_BUSY;
> > + }
>
> This is a the regular flow control function and must be called before a TX
> complete interrupt can trigger. Your tx-complete interrupt is probably
> always enabled?
>
Yes. The tx-completed interrupt always enabled.
> So here you check the fill level of the FIFO:
>
> if ((priv->tx_head - priv->tx_tail) ==
> priv->xcan_echo_skb_max_tx)
> netif_stop_queue(ndev);
>
> If it's full, stop the queue. The you trigger the tx, the tx complete interrupt
> gets called and the queue will be restarted.
>
Ok
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +/**
> > + * xcan_rx - Is called from CAN isr to complete the received
> > + * frame processing
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This function is invoked from the CAN isr(poll) to process the Rx
> > +frames. It
> > + * does minimal processing and invokes "netif_receive_skb" to
> > +complete further
> > + * processing.
> > + * Return: 0 on success and negative error value on error */ static
> > +int xcan_rx(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + u32 id_xcan, dlc, data[2] = {0, 0}, rtr = 0;
> > +
> > + skb = alloc_can_skb(ndev, &cf);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + /* Read a frame from Xilinx zynq CANPS */
> > + id_xcan = priv->read_reg(priv, XCAN_RXFIFO_ID_OFFSET);
> > + dlc = priv->read_reg(priv, XCAN_RXFIFO_DLC_OFFSET) &
> > +XCAN_DLCR_DLC_MASK;
>
> Better do the shift to dlc.
>
Ok
> > +
> > + /* Change Xilinx CAN data length format to socketCAN data format
> */
> > + cf->can_dlc = get_can_dlc((dlc & XCAN_DLCR_DLC_MASK) >>
> > + XCAN_DLCR_DLC_SHIFT);
>
> Then it's just: get_can_dlc(dlc);
Ok
>
> > +
> > + /* Change Xilinx CAN ID format to socketCAN ID format */
> > + if (id_xcan & XCAN_IDR_IDE_MASK) {
> > + /* The received frame is an Extended format frame */
> > + cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
> > + cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
> > + XCAN_IDR_ID2_SHIFT;
> > + cf->can_id |= CAN_EFF_FLAG;
> > + if (id_xcan & XCAN_IDR_RTR_MASK) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + rtr = 1;
> > + }
> > + } else {
> > + /* The received frame is a standard format frame */
> > + cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
> > + XCAN_IDR_ID1_SHIFT;
> > + if (id_xcan & XCAN_IDR_RTR_MASK) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + rtr = 1;
> > + }
> > + }
> > +
> > + if (!rtr) {
> > + data[0] = priv->read_reg(priv, XCAN_RXFIFO_DW1_OFFSET);
> > + data[1] = priv->read_reg(priv, XCAN_RXFIFO_DW2_OFFSET);
> > +
> > + /* Change Xilinx CAN data format to socketCAN data format
> */
> > + *(__be32 *)(cf->data) = cpu_to_be32(data[0]);
> > + if (cf->can_dlc > 4)
> > + *(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
> > + }
> > + can_led_event(ndev, CAN_LED_EVENT_RX);
> > +
> > + netif_receive_skb(skb);
> > +
> > + stats->rx_bytes += cf->can_dlc;
> > + stats->rx_packets++;
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_err_interrupt - error frame Isr
> > + * @ndev: net_device pointer
> > + * @isr: interrupt status register value
> > + *
> > + * This is the CAN error interrupt and it will
> > + * check the the type of error and forward the error
> > + * frame to upper layers.
> > + */
> > +static void xcan_err_interrupt(struct net_device *ndev, u32 isr) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + u32 err_status, status;
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > + if (!skb) {
> > + netdev_err(ndev, "alloc_can_err_skb() failed!\n");
> > + return;
> > + }
> > +
> > + err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
> > + priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
> > + status = priv->read_reg(priv, XCAN_SR_OFFSET);
> > +
> > + if (isr & XCAN_IXR_BSOFF_MASK) {
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + priv->can.can_stats.bus_off++;
> > + /* Leave device in Config Mode in bus-off state */
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> > + can_bus_off(ndev);
> > + } else if ((status & XCAN_SR_ESTAT_MASK) ==
> XCAN_SR_ESTAT_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + priv->can.can_stats.error_passive++;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE |
> > + CAN_ERR_CRTL_TX_PASSIVE;
> > + } else if (status & XCAN_SR_ERRWRN_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + priv->can.can_stats.error_warning++;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING |
> > + CAN_ERR_CRTL_TX_WARNING;
> > + }
> > +
> > + /* Check for Arbitration lost interrupt */
> > + if (isr & XCAN_IXR_ARBLST_MASK) {
> > + cf->can_id |= CAN_ERR_LOSTARB;
> > + cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> > + priv->can.can_stats.arbitration_lost++;
> > + }
> > +
> > + /* Check for RX FIFO Overflow interrupt */
> > + if (isr & XCAN_IXR_RXOFLW_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> > + }
> > +
> > + /* Check for error interrupt */
> > + if (isr & XCAN_IXR_ERROR_MASK) {
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > +
> > + /* Check for Ack error interrupt */
> > + if (err_status & XCAN_ESR_ACKER_MASK) {
> > + cf->can_id |= CAN_ERR_ACK;
> > + cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> > + stats->tx_errors++;
> > + }
> > +
> > + /* Check for Bit error interrupt */
> > + if (err_status & XCAN_ESR_BERR_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_BIT;
> > + stats->tx_errors++;
> > + }
> > +
> > + /* Check for Stuff error interrupt */
> > + if (err_status & XCAN_ESR_STER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_STUFF;
> > + stats->rx_errors++;
> > + }
> > +
> > + /* Check for Form error interrupt */
> > + if (err_status & XCAN_ESR_FMER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_FORM;
> > + stats->rx_errors++;
> > + }
> > +
> > + /* Check for CRC error interrupt */
> > + if (err_status & XCAN_ESR_CRCER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL;
> > + stats->rx_errors++;
> > + }
> > + priv->can.can_stats.bus_error++;
> > + }
> > +
> > + netif_rx(skb);
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +
> > + netdev_dbg(ndev, "%s: error status register:0x%x\n",
> > + __func__, priv->read_reg(priv, XCAN_ESR_OFFSET));
> }
> > +
> > +/**
> > + * xcan_state_interrupt - It will check the state of the CAN device
> > + * @ndev: net_device pointer
> > + * @isr: interrupt status register value
> > + *
> > + * This will checks the state of the CAN device
> > + * and puts the device into appropriate state.
> > + */
> > +static void xcan_state_interrupt(struct net_device *ndev, u32 isr) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + /* Check for Sleep interrupt if set put CAN device in sleep state */
> > + if (isr & XCAN_IXR_SLP_MASK)
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + /* Check for Wake up interrupt if set put CAN device in Active state
> */
> > + if (isr & XCAN_IXR_WKUP_MASK)
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE; }
> > +
> > +/**
> > + * xcan_rx_poll - Poll routine for rx packets (NAPI)
> > + * @napi: napi structure pointer
> > + * @quota: Max number of rx packets to be processed.
> > + *
> > + * This is the poll routine for rx part.
> > + * It will process the packets maximux quota value.
> > + *
> > + * Return: number of packets received */ static int
> > +xcan_rx_poll(struct napi_struct *napi, int quota) {
> > + struct net_device *ndev = napi->dev;
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 isr, ier;
> > + int work_done = 0;
> > +
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
> > + if (isr & XCAN_IXR_RXOK_MASK) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> > + XCAN_IXR_RXOK_MASK);
> > + if (xcan_rx(ndev) < 0)
> > + return work_done;
> > + work_done++;
> > + } else {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> > + XCAN_IXR_RXNEMP_MASK);
> > + break;
> > + }
>
> What does the XCAN_IXR_RXOK_MASK mean if it's send and undset?
XCAN_IXR_RXOK_MASK Means it is successfully received one packet
>
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_RXNEMP_MASK);
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + }
> > +
> > + if (work_done < quota) {
> > + napi_complete(napi);
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier |= (XCAN_IXR_RXOK_MASK |
> XCAN_IXR_RXNEMP_MASK);
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
>
> Is this a read-modify-write register? I mean will an interrupt get disabled, if
> you write a 0-bit in the IER register? What does the ICR register?
ISR- Interrupt Status Register (Read only register)
IER- Interrupt Enable Register(R/w register)
ICR- Interrupt Clear Register.(write only register)
The Interrupt Status Register (ISR) contains bits that are set when a particular interrupt condition occurs. If
the corresponding mask bit in the Interrupt Enable Register is set, an interrupt is generated.
Interrupt bits in the ISR can be cleared by writing to the Interrupt Clear Register
>
> > + }
> > + return work_done;
> > +}
> > +
> > +/**
> > + * xcan_tx_interrupt - Tx Done Isr
> > + * @ndev: net_device pointer
> > + */
> > +static void xcan_tx_interrupt(struct net_device *ndev) {
> > + unsigned long flags;
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + u32 processed = 0, txpackets;
> > +
> > + stats->tx_packets++;
> > + netdev_dbg(ndev, "%s: waiting total:%d,current:%d\n", __func__,
> > + priv->waiting_ech_skb_num, priv-
> >waiting_ech_skb_index);
> > +
> > + txpackets = priv->waiting_ech_skb_num;
> > +
> > + if (txpackets) {
> > + can_get_echo_skb(ndev, priv->waiting_ech_skb_index);
> > + priv->waiting_ech_skb_index =
> > + (priv->waiting_ech_skb_index + 1) %
> > + priv->xcan_echo_skb_max_tx;
> > + processed++;
> > + txpackets--;
> > + }
> > +
> > + spin_lock_irqsave(&priv->ech_skb_lock, flags);
> > + priv->waiting_ech_skb_num -= processed;
> > + spin_unlock_irqrestore(&priv->ech_skb_lock, flags);
>
> This all simplyfies to a:
> can_get_echo_skb(ndev, priv->tx_tail %
> priv->xcan_echo_skb_max_tx);
> priv->tx_tail++;
>
Ok
> I think you should add some kind of loop here, it there is more than one tx-
> complete per IRQ.
>
Ok
> > +
> > + netdev_dbg(ndev, "%s: waiting total:%d,current:%d\n", __func__,
> > + priv->waiting_ech_skb_num, priv-
> >waiting_ech_skb_index);
> > +
> > + netif_wake_queue(ndev);
> > +
> > + can_led_event(ndev, CAN_LED_EVENT_TX); }
> > +
> > +/**
> > + * xcan_interrupt - CAN Isr
> > + * @irq: irq number
> > + * @dev_id: device id poniter
> > + *
> > + * This is the xilinx CAN Isr. It checks for the type of interrupt
> > + * and invokes the corresponding ISR.
> > + *
> > + * Return:
> > + * IRQ_NONE - If CAN device is in sleep mode, IRQ_HANDLED otherwise
> > +*/ static irqreturn_t xcan_interrupt(int irq, void *dev_id) {
> > + struct net_device *ndev = (struct net_device *)dev_id;
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 isr, ier;
> > +
> > + /* Get the interrupt status from Xilinx CAN */
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + if (!isr)
> > + return IRQ_NONE;
> > +
> > + netdev_dbg(ndev, "%s: isr:#x%08x, err:#x%08x\n", __func__,
> > + isr, priv->read_reg(priv, XCAN_ESR_OFFSET));
> > +
> > + /* Check for the type of interrupt and Processing it */
> > + if (isr & (XCAN_IXR_SLP_MASK | XCAN_IXR_WKUP_MASK)) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> (XCAN_IXR_SLP_MASK |
> > + XCAN_IXR_WKUP_MASK));
> > + xcan_state_interrupt(ndev, isr);
> > + }
> > +
> > + /* Check for Tx interrupt and Processing it */
> > + if (isr & XCAN_IXR_TXOK_MASK) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_TXOK_MASK);
> > + xcan_tx_interrupt(ndev);
> > + }
> > +
> > + /* Check for the type of error interrupt and Processing it */
> > + if (isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> > + XCAN_IXR_BSOFF_MASK |
> XCAN_IXR_ARBLST_MASK)) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> (XCAN_IXR_ERROR_MASK |
> > + XCAN_IXR_RXOFLW_MASK |
> XCAN_IXR_BSOFF_MASK |
> > + XCAN_IXR_ARBLST_MASK));
> > + xcan_err_interrupt(ndev, isr);
> > + }
> > +
> > + /* Check for the type of receive interrupt and Processing it */
> > + if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) {
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier &= ~(XCAN_IXR_RXNEMP_MASK |
> XCAN_IXR_RXOK_MASK);
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> > + napi_schedule(&priv->napi);
> > + }
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * xcan_stop - Driver stop routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the drivers stop routine. It will disable the
> > + * interrupts and put the device into configuration mode.
> > + */
> > +static void xcan_stop(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 ier;
> > +
> > + /* Disable interrupts and leave the can in configuration mode */
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier &= ~XCAN_INTR_ALL;
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> > + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> > + priv->can.state = CAN_STATE_STOPPED; }
> > +
> > +/**
> > + * xcan_open - Driver open routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver open routine.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_open(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
> > + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> > + ndev->name, (void *)ndev);
> > + if (ret < 0) {
> > + netdev_err(ndev, "Irq allocation for CAN failed\n");
> > + return ret;
> > + }
> > +
> > + /* Set chip into reset mode */
> > + ret = set_reset_mode(ndev);
> > + if (ret < 0)
> > + netdev_err(ndev, "mode resetting failed failed!\n");
>
> Is this critical?
This condition usually won't fail.
But if the controller has a problems at the h/w level in that case putted this err print.
If you want me to change it as a warning will do
>
> > +
> > + /* Common open */
> > + ret = open_candev(ndev);
> > + if (ret)
> > + return ret;
>
> You should free the interrupt handler if this fails.
Ok
>
> > +
> > + ret = xcan_start(ndev);
> > + if (ret < 0)
> > + netdev_err(ndev, "xcan_start failed!\n");
> > +
> > +
> > + can_led_event(ndev, CAN_LED_EVENT_OPEN);
> > + napi_enable(&priv->napi);
> > + netif_start_queue(ndev);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_close - Driver close routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * Return: 0 always
> > + */
> > +static int xcan_close(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + netif_stop_queue(ndev);
> > + napi_disable(&priv->napi);
> > + xcan_stop(ndev);
> > + free_irq(ndev->irq, ndev);
> > + close_candev(ndev);
> > +
> > + can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_get_berr_counter - error counter routine
> > + * @ndev: Pointer to net_device structure
> > + * @bec: Pointer to can_berr_counter structure
> > + *
> > + * This is the driver error counter routine.
> > + * Return: 0 always
> > + */
> > +static int xcan_get_berr_counter(const struct net_device *ndev,
> > + struct can_berr_counter *bec)
> > +{
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_TEC_MASK;
> > + bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> > + XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> > + return 0;
> > +}
> > +
> > +static const struct net_device_ops xcan_netdev_ops = {
> > + .ndo_open = xcan_open,
> > + .ndo_stop = xcan_close,
> > + .ndo_start_xmit = xcan_start_xmit,
> > +};
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +/**
> > + * xcan_suspend - Suspend method for the driver
> > + * @_dev: Address of the platform_device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int xcan_suspend(struct device *_dev) {
> > + struct platform_device *pdev = container_of(_dev,
> > + struct platform_device, dev);
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + if (netif_running(ndev)) {
> > + netif_stop_queue(ndev);
> > + netif_device_detach(ndev);
> > + }
> > +
> > + priv->write_reg(priv, XCAN_MSR_OFFSET,
> XCAN_MSR_SLEEP_MASK);
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + clk_disable(priv->aperclk);
> > + clk_disable(priv->devclk);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev: Address of the platformdevice structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_resume(struct device *dev) {
> > + struct platform_device *pdev = container_of(dev,
> > + struct platform_device, dev);
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
> > + ret = clk_enable(priv->aperclk);
> > + if (ret) {
> > + dev_err(dev, "Cannot enable clock.\n");
> > + return ret;
> > + }
> > + ret = clk_enable(priv->devclk);
> > + if (ret) {
> > + dev_err(dev, "Cannot enable clock.\n");
> > + return ret;
> > + }
> > +
> > + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + if (netif_running(ndev)) {
> > + netif_device_attach(ndev);
> > + netif_start_queue(ndev);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> xcan_resume);
> > +
> > +/**
> > + * xcan_probe - Platform registration call
> > + * @pdev: Handle to the platform device structure
> > + *
> > + * This function does all the memory allocation and registration for
> > +the CAN
> > + * device.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_probe(struct platform_device *pdev) {
> > + struct resource *res; /* IO mem resources */
> > + struct net_device *ndev;
> > + struct xcan_priv *priv;
> > + int ret, fifodep;
> > +
> > + /* Create a CAN device instance */
> > + ndev = alloc_candev(sizeof(struct xcan_priv),
> XCAN_ECHO_SKB_MAX);
> > + if (!ndev)
> > + return -ENOMEM;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->dev = ndev;
> > + priv->can.bittiming_const = &xcan_bittiming_const;
> > + priv->can.do_set_bittiming = xcan_set_bittiming;
> > + priv->can.do_set_mode = xcan_do_set_mode;
> > + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_BERR_REPORTING;
> > +
> > + /* Get IRQ for the device */
> > + ndev->irq = platform_get_irq(pdev, 0);
> > +
> > + spin_lock_init(&priv->ech_skb_lock);
> > + ndev->flags |= IFF_ECHO; /* We support local echo */
> > +
> > + platform_set_drvdata(pdev, ndev);
> > + SET_NETDEV_DEV(ndev, &pdev->dev);
> > + ndev->netdev_ops = &xcan_netdev_ops;
> > +
> > + /* Get the virtual base address for the device */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv->reg_base)) {
> > + ret = PTR_ERR(priv->reg_base);
> > + goto err_free;
> > + }
> > + ndev->mem_start = res->start;
> > + ndev->mem_end = res->end;
> > +
> > + priv->write_reg = xcan_write_reg;
> > + priv->read_reg = xcan_read_reg;
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
> > + &fifodep);
> > + if (ret < 0)
> > + goto err_free;
> > + priv->xcan_echo_skb_max_tx = fifodep;
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
> > + &fifodep);
> > + if (ret < 0)
> > + goto err_free;
> > + priv->xcan_echo_skb_max_rx = fifodep;
> > +
> > + /* Getting the CAN devclk info */
> > + priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
> > + if (IS_ERR(priv->devclk)) {
> > + dev_err(&pdev->dev, "Device clock not found.\n");
> > + ret = PTR_ERR(priv->devclk);
> > + goto err_free;
> > + }
> > +
> > + /* Check for type of CAN device */
> > + if (of_device_is_compatible(pdev->dev.of_node,
> > + "xlnx,zynq-can-1.00.a")) {
> > + priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
> > + if (IS_ERR(priv->aperclk)) {
> > + dev_err(&pdev->dev, "aper clock not found\n");
> > + ret = PTR_ERR(priv->aperclk);
> > + goto err_free;
> > + }
> > + } else {
> > + priv->aperclk = priv->devclk;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->devclk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to enable device clock\n");
> > + goto err_free;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->aperclk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to enable aper clock\n");
> > + goto err_unprepar_disabledev;
> > + }
>
> Can you keep your clocks disaled if the interface is not up?
I didn't get it will you please explain?
Regards,
Kedar.
>
> > +
> > + priv->can.clock.freq = clk_get_rate(priv->devclk);
> > +
> > + netif_napi_add(ndev, &priv->napi, xcan_rx_poll,
> > + priv->xcan_echo_skb_max_rx);
> > + ret = register_candev(ndev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
> ret);
> > + goto err_unprepar_disableaper;
> > + }
> > +
> > + devm_can_led_init(ndev);
> > + dev_info(&pdev->dev,
> > + "reg_base=0x%p irq=%d clock=%d, tx fifo
> depth:%d\n",
> > + priv->reg_base, ndev->irq, priv->can.clock.freq,
> > + priv->xcan_echo_skb_max_tx);
> > +
> > + return 0;
> > +
> > +err_unprepar_disableaper:
> > + clk_disable_unprepare(priv->aperclk);
> > +err_unprepar_disabledev:
> > + clk_disable_unprepare(priv->devclk);
> > +err_free:
> > + free_candev(ndev);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * xcan_remove - Unregister the device after releasing the resources
> > + * @pdev: Handle to the platform device structure
> > + *
> > + * This function frees all the resources allocated to the device.
> > + * Return: 0 always
> > + */
> > +static int xcan_remove(struct platform_device *pdev) {
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + if (set_reset_mode(ndev) < 0)
> > + netdev_err(ndev, "mode resetting failed!\n");
> > +
> > + unregister_candev(ndev);
> > + netif_napi_del(&priv->napi);
> > + clk_disable_unprepare(priv->aperclk);
> > + clk_disable_unprepare(priv->devclk);
> > +
> > + free_candev(ndev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Match table for OF platform binding */ static struct of_device_id
> > +xcan_of_match[] = {
> > + { .compatible = "xlnx,zynq-can-1.00.a", },
> > + { .compatible = "xlnx,axi-can-1.00.a", },
> > + { /* end of list */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, xcan_of_match);
> > +
> > +static struct platform_driver xcan_driver = {
> > + .probe = xcan_probe,
> > + .remove = xcan_remove,
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = DRIVER_NAME,
> > + .pm = &xcan_dev_pm_ops,
> > + .of_match_table = xcan_of_match,
> > + },
> > +};
> > +
> > +module_platform_driver(xcan_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Xilinx Inc");
> > +MODULE_DESCRIPTION("Xilinx CAN interface");
> >
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* [PATCH 3/6] net: cpsw: Add control-module macid driver
From: Markus Pargmann @ 2014-02-14 9:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213194431.GF17650@pengutronix.de>
Hi,
On Thu, Feb 13, 2014 at 08:44:31PM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Dec 18, 2013 at 05:47:19PM +0100, Markus Pargmann wrote:
> > This driver extracts the hardware macid from the control module of
> > am335x processors. It exports a function cpsw_ctrl_macid_read for cpsw
> > to get the macid from within the processor.
> >
> > This driver is not used, unless it is defined in DT and referenced by a
> > cpsw slave with a phandle.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> > .../devicetree/bindings/net/cpsw-ctrl-macid.txt | 31 +++++
> > drivers/net/ethernet/ti/Kconfig | 8 ++
> > drivers/net/ethernet/ti/Makefile | 1 +
> > drivers/net/ethernet/ti/cpsw-ctrl-macid.c | 138 +++++++++++++++++++++
> > 4 files changed, 178 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/cpsw-ctrl-macid.txt
> > create mode 100644 drivers/net/ethernet/ti/cpsw-ctrl-macid.c
> >
> > diff --git a/Documentation/devicetree/bindings/net/cpsw-ctrl-macid.txt b/Documentation/devicetree/bindings/net/cpsw-ctrl-macid.txt
> > new file mode 100644
> > index 0000000..abff2af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/cpsw-ctrl-macid.txt
> > @@ -0,0 +1,31 @@
> > +TI CPSW ctrl macid Devicetree bindings
> > +--------------------------------------
> > +
> > +Required properties:
> > + - compatible : Should be "ti,am3352-cpsw-ctrl-macid"
> > + - reg : physical base address and size of the cpsw
> > + registers map
> > + - reg-names : names of the register map given in "reg" node
> > + - #ti,cpsw-ctrl-macid : Should be <1>
> #ti,mac-address-ctrl-cells?
Sounds better, will fix.
>
> > +
> > +When used from cpsw, "ti,mac-address-ctrl" should be a phandle to this device
> > +node with one argument, 0 or 1 to select the macid 0 or 1.
> > +
> > +Examples:
> > +
> > + cpsw_ctrl_macid: cpsw-ctrl-macid at 44e10630 {
> > + compatible = "ti,am3352-cpsw-ctrl-macid";
> > + #ti,mac-address-ctrl-cells = <1>;
> > + reg = <0x44e10630 0x16>;
> s/0x16/0x10/
Thanks, that's a bug, obviously we only have 4, not 5.5 registers.
>
> > + reg-names = "ctrl-macid";
> > + };
> > +
> > +Used in cpsw slave nodes like this:
> > +
> > + cpsw_emac0: slave at 4a100200 {
> > + ti,mac-address-ctrl = <&cpsw_ctrl_macid 0>;
> > + };
> > +
> > + cpsw_emac1: slave at 4a100300 {
> > + ti,mac-address-ctrl = <&cpsw_ctrl_macid 1>;
> > + };
> > diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> > index 53150c2..24819ef 100644
> > --- a/drivers/net/ethernet/ti/Kconfig
> > +++ b/drivers/net/ethernet/ti/Kconfig
> > @@ -56,12 +56,20 @@ config TI_CPSW_PHY_SEL
> > This driver supports configuring of the phy mode connected to
> > the CPSW.
> >
> > +config TI_CPSW_CTRL_MACID
> > + boolean "TI CPSW internal MACID support"
> > + depends on TI_CPSW
> > + ---help---
> > + This driver supports reading the hardcoded MACID from am33xx
> > + processors control module.
> > +
> Would it be nicer to put this after the TI_CPSW definition. (Think
> $(make config).)
I inserted TI_CPSW_CTRL_MACID here because the other TI_CPSW specific
subdriver (TI_CPSW_PHY_SEL) was above TI_CPSW. But I could change this.
>
> > config TI_CPSW
> > tristate "TI CPSW Switch Support"
> > depends on ARM && (ARCH_DAVINCI || SOC_AM33XX)
> > select TI_DAVINCI_CPDMA
> > select TI_DAVINCI_MDIO
> > select TI_CPSW_PHY_SEL
> > + select TI_CPSW_CTRL_MACID
> If TI_CPSW selects TI_CPSW_CTRL_MACID the latter doesn't need to depend
> on the former. So this optin is user visible but never
> user-(de)selectable. I'd say drop the Kconfig symbol and just add
> cpsw-ctrl-macid.o to ti_cpsw-y in the Makefile (or really make it
> optional).
As this is closely related to the cpsw driver, I think it's better to
make it non-optional and include it in the Makefile.
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH] mfd: vexpress-sysreg: initialize "site" variable
From: Lee Jones @ 2014-02-14 9:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392309594-28229-1-git-send-email-semen.protsenko@linaro.org>
> "site" variable should be initialized with 0 so that
> when "site" property doesn't exist in DTB it can be handled correctly.
>
> "0" value means board site number is motherboard (see
> Documentation/devicetree/bindings/arm/vexpress.txt for details).
>
> Signed-off-by: Semen Protsenko <semen.protsenko@linaro.org>
> ---
> drivers/mfd/vexpress-sysreg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 4/6] net: cpsw: Use cpsw-ctrl-macid driver
From: Markus Pargmann @ 2014-02-14 9:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213193702.GE17650@pengutronix.de>
Hi Uwe,
On Thu, Feb 13, 2014 at 08:37:02PM +0100, Uwe Kleine-K?nig wrote:
> Hello Markus,
>
> On Wed, Dec 18, 2013 at 05:47:20PM +0100, Markus Pargmann wrote:
> > Use ctrl-macid driver to obtain the macids stored in the processor. This
> > is only done when defined in DT.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> > Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
> > drivers/net/ethernet/ti/cpsw.c | 18 ++++++++++++++----
> > drivers/net/ethernet/ti/cpsw.h | 2 ++
> > 3 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> > index c39f077..b95c38b 100644
> > --- a/Documentation/devicetree/bindings/net/cpsw.txt
> > +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> > @@ -34,6 +34,11 @@ Required properties:
> > Optional properties:
> > - dual_emac_res_vlan : Specifies VID to be used to segregate the ports
> > - mac-address : Specifies slave MAC address
> > +- ti,mac-address-ctrl : When cpsw-ctrl-macid support is compiledin, this can
> > + be set to a phandle with one argument, see
> > + cpsw-ctrl-macid.txt. If this method fails, cpsw falls
> > + back to mac-address or random mac-address.
> > +
> >
> > Note: "ti,hwmods" field is used to fetch the base address and irq
> > resources from TI, omap hwmod data base during device registration.
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 5120d9c..382d793 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -1804,9 +1804,16 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
> > snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
> > PHY_ID_FMT, mdio->name, phyid);
> >
> > - mac_addr = of_get_mac_address(slave_node);
> > - if (mac_addr)
> > - memcpy(slave_data->mac_addr, mac_addr, ETH_ALEN);
> > + ret = cpsw_ctrl_macid_read(slave_node, slave_data->mac_addr);
> > + if (ret) {
> > + if (ret == -EPROBE_DEFER)
> > + return ret;
> > +
> > + mac_addr = of_get_mac_address(slave_node);
> > + if (mac_addr)
> > + memcpy(slave_data->mac_addr, mac_addr,
> > + ETH_ALEN);
> > + }
> I'd do it the other way round: Use the contents from an explicit
> "mac-address" or "local-mac-address" property (i.e. of_get_mac_address)
> and if that doesn't return anything use the mac-address-ctrl as
> fallback.
Yes you are right. In this case this wouldn't even influence any boots
with u-boot which already set the correct mac-address property.
I will fix this.
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH 08/14] net: axienet: Removed checkpatch errors/warnings
From: Michal Simek @ 2014-02-14 9:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392306707.2214.65.camel@joe-AO722>
On 02/13/2014 04:51 PM, Joe Perches wrote:
> On Thu, 2014-02-13 at 08:19 +0100, Michal Simek wrote:
>> On 02/13/2014 01:31 AM, Joe Perches wrote:
>>> On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote:
>
> Hi again Michal.
>
>>>> + netdev_warn(lp->ndev,
>>>> + "Could not find clock ethernet controller property.");
>>>
>>> here too. (and alignment)
>>
>> This is problematic. I would like to keep 80 char limits and keeping
>> this align just break it. That's why I was using tab alignment.
>> Probably the solution is just to shorten message.
>
> (overly long, tiresomely trivial stuff below)
>
> Your choice. I'm not an 80 column zealot but
> please don't shorten the message just to fit
> 80 columns if it impacts intelligibility.
I am trying to keep 80 chars and follow subsystem
standards.
> Generally, I'd write this something like:
>
> netdev_warn(lp->ndev,
> "Could not find clock ethernet controller property\n");
>
> (without the period) which is 83 columns.
ok.
> checkpatch makes exceptions for 80 column line
> length maximums for format strings.
yes but testing systems reports it because that 80 chars
is still default value.
>
> I've no real issue if you indent it back one.
>
> fyi: this is 77 columns
>
> netdev_warn(lp->ndev,
> "No clock ethernet controller property found\n");
>
> About the message itself.
>
> You dropped the "axienet_mdio_setup" function name.
>
> I believe the dmesg output will look something like:
>
> xilinx_temac 0000:01:00.0 (unregistered net_device): Could not find clock ethernet controller property.
> xilinx_temac 0000:01:00.0 (unregistered net_device): Setting MDIO clock divisor to default 29
>
> Because these 2 messages are effectively linked,
> my preference would be to emit them on a single line,
>
> Something like:
>
> xilinx_temac 0000:01:00.0 (unregistered net_device): of_get_property("clock-frequency") not found - setting MDIO clock divisor to default 29
>
> or
>
> netdev_warn(lp->ndev,
> "of_get_property(\"clock-frequency\") not found - setting MDIO clock divisor to default %u\n",
> DEFAULT_CLOCK_DIVISOR);
>
But then you are breaking 80 char limits a lot.
Thanks,
Michal
^ permalink raw reply
* [PATCH/RFC] ARM: shmobile: Add defconfig for shmobile mulitplatfom
From: Geert Uytterhoeven @ 2014-02-14 9:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392337944-4000-1-git-send-email-horms+renesas@verge.net.au>
On Fri, Feb 14, 2014 at 1:32 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
>[PATCH/RFC] ARM: shmobile: Add defconfig for shmobile mulitplatfom
multiplatform
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 23/27] clocksource: sh_cmt: Add DT support
From: Geert Uytterhoeven @ 2014-02-14 9:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392339605-20691-24-git-send-email-laurent.pinchart+renesas@ideasonboard.com>
On Fri, Feb 14, 2014 at 2:00 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt
> @@ -0,0 +1,75 @@
> +* Renesas R-Car Compare Match Timer (CMT)
> +
> +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable clock
16-bit is mentioned here ...
> +inputs and programmable compare match.
> +
> +Channels share hadware resources but their counter and compare match value are
hardware
> +independent. A particular CMT instance can implement only a subset of the
> +channels supported by the CMT model. Channels indices start from 0 and are
Channel indices
> +consecutive.
> +
> +Required Properties:
> +
> + - compatible: must contain one of the following.
... why not add "renesas,cmt-16" here (and one extra line in the actual driver),
while you're at it?
> + - "renesas,cmt-32" for the 32-bit CMT
> + (CMT0 on sh7372, sh73a0 and r8a7740)
> + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support
> + (CMT[234] on sh7372, sh73a0 and r8a7740)
> + - "renasas,cmt-48" for the 48-bit CMT
> + (CMT1 on sh7372, sh73a0 and r8a7740)
> + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT
> + (CMT[01] on r8a73a4, r8a7790 and r8a7791)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v2] can: xilinx CAN controller support.
From: Marc Kleine-Budde @ 2014-02-14 9:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52FDDE30.8020405@monstr.eu>
On 02/14/2014 10:13 AM, Michal Simek wrote:
>>> That's not entirely truth. If you look at Microblaze then you will see
>>> that Microblaze can be BE and LE.
>>> There is just missing endian detection which we will add to the next version.
>>
>> As far as I know the endianess of the kernel is fixed and known during
>> compile time. Correct me if I'm wrong. So there is no need for a runtime
>> detection of the endianess and so no need for {read,write}_reg function
>> pointers.
>
> Endianess of the kernel is fixed and know during compile time
> but what it is not fixed is endianess of that IP at compile time.
>
> On fpga you can use bridges, partial reconfiguration, etc where
> the only solution which is run-time endian detection via registers.
>
> For example: drivers/block/xsysace.c, drivers/spi/spi-xilinx.c, etc
Okay, now I get it. You can make it more complex then it used to be :D
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 242 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140214/7910cbee/attachment.sig>
^ permalink raw reply
* [PATCH v2] can: xilinx CAN controller support.
From: Michal Simek @ 2014-02-14 9:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52FDDC24.8070805@pengutronix.de>
On 02/14/2014 10:04 AM, Marc Kleine-Budde wrote:
> On 02/14/2014 09:55 AM, Michal Simek wrote:
>> Hi Marc,
>>
>>>> + int waiting_ech_skb_num;
>>>> + int xcan_echo_skb_max_tx;
>>>> + int xcan_echo_skb_max_rx;
>>>> + struct napi_struct napi;
>>>> + spinlock_t ech_skb_lock;
>>>> + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
>>>> + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>>>
>>> Please remove read_reg, write_reg, as long as there isn't any BE support
>>> in the driver, call them directly.
>>
>> That's not entirely truth. If you look at Microblaze then you will see
>> that Microblaze can be BE and LE.
>> There is just missing endian detection which we will add to the next version.
>
> As far as I know the endianess of the kernel is fixed and known during
> compile time. Correct me if I'm wrong. So there is no need for a runtime
> detection of the endianess and so no need for {read,write}_reg function
> pointers.
Endianess of the kernel is fixed and know during compile time
but what it is not fixed is endianess of that IP at compile time.
On fpga you can use bridges, partial reconfiguration, etc where
the only solution which is run-time endian detection via registers.
For example: drivers/block/xsysace.c, drivers/spi/spi-xilinx.c, etc
>> But because MB io helper functions are broken for a while you should be
>> able to use this driver on both endianess.
>>
>> btw: I would prefer to use ioread32 and ioread32be instead of readl.
>> Is it OK for you?
>
> Make it so. :)
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140214/6445e2d1/attachment-0001.sig>
^ permalink raw reply
* [PATCH v5 0/3] serial: fsl_lpuart: add DMA support
From: Arnd Bergmann @ 2014-02-14 9:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392362839-8004-1-git-send-email-yao.yuan@freescale.com>
On Friday 14 February 2014 15:27:16 Yuan Yao wrote:
> Changed in v5:
> - Revoke move dma properties from dtsi to dts.
> - Change the explanations for dmas, dma-names in document.
> - Change the description for [PATCH 3] serial: fsl_lpuart: documented the clock requirement.
Whole series
Acked-by: Arnd Bergmann <arnd@arndb.de>
> Changed in v4:
> - Move dma properties from dtsi to dts.
> - Cancle the macro(SERIAL_FSL_LPUART_DMA) .
> - Separate the document for clocks which undocumented before into a single patch.
> - Change some explanations in document(clocks, clock-names, dmas, dma-names).
> - Change "lpuart-tx" and "lpuart-rx" to "tx" and "rx".
>
> Changed in v3:
> - Use the streaming DMA API for receive.
> - Add the macro(SERIAL_FSL_LPUART_DMA) and dts node propertie for whether using the dma.
> - Adjust some coding style.
>
> Changed in v2:
> - Add eDMA support for lpuart receive.
> - Use dma_mapping_error test dma_map_single.
> - Change some names of variable.
> - Fix some bugs.
>
> Added in v1:
> - Add device tree bindings for lupart eDMA support.
> - Add eDMA support for lpuart send.
>
^ permalink raw reply
* use {readl|writel}_relaxed instead of readl/writel in i2c-designware-core ?
From: Arnd Bergmann @ 2014-02-14 9:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140214155438.4f749a79@xhacker>
On Friday 14 February 2014 15:54:38 Jisheng Zhang wrote:
> Hi all,
>
> The writel/readl is too expensive especially on Cortex A9 w/ outer L2 cache. This
> introduce i2c read/write error on Marvell Berlin SoCs when there are L2 cache
> maintenance operations at the same time.
>
> In our internal berlin bsp, we just replaced readl/writel with the relaxed
> version. But AFAIK, the "relaxed" version doesn't exist on all architectures. How
> to handle this issue?
In case of i2c-designware, this is safe because that driver does not perform
DMA. In other drivers, you may have to be more careful, to ensure that all MMIO
is serialized with DMA operations performed by the driver.
> Any suggestions are appreciated.
I would definitely welcome a patch that adds a default _relaxed implementation
to include/linux/io.h, like this:
#ifndef readb_relaxed
#define readb_relaxed(p) readb(p)
#endif
and then adds "#define readb_relaxed(p) readb_relaxed(p)" etc. to all
architectures that have a non-macro definition for readb.
Alternatively, we could have a CONFIG_ARCH_MMIO_RELAXED configuration
symbol that gets selected by any architecture that provides the _relaxed
accessors, and get linux/io.h to define all of them for the other
architectures.
Arnd
^ permalink raw reply
* [PATCH v4] ARM: shmobile: Add defconfig for shmobile mulitplatfom
From: Magnus Damm @ 2014-02-14 9:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392354403-30260-1-git-send-email-horms+renesas@verge.net.au>
On Fri, Feb 14, 2014 at 2:06 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> This is intended to be a base for a defconfig to cover all
> shmobile multiplatform boards. It currently includes configuration
> for the following boards:
>
> * KZM9D
> * Lager
> * Koelsch
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
> v4
> * Account for removal of CONFIG_KZM9D
> - Do not select CONFIG_MACH_KZM9D
> - Select CONFIG_SMSC_PHY and CONFIG_REGULATOR_FIXED_VOLTAGE
Thanks Simon!
Acked-by: Magnus Damm <damm@opensource.se>
Cheers,
/ magnus
^ permalink raw reply
* [PATCH v2] can: xilinx CAN controller support.
From: Marc Kleine-Budde @ 2014-02-14 9:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52FDD9FE.8040908@monstr.eu>
On 02/14/2014 09:55 AM, Michal Simek wrote:
> Hi Marc,
>
>>> + int waiting_ech_skb_num;
>>> + int xcan_echo_skb_max_tx;
>>> + int xcan_echo_skb_max_rx;
>>> + struct napi_struct napi;
>>> + spinlock_t ech_skb_lock;
>>> + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
>>> + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>>
>> Please remove read_reg, write_reg, as long as there isn't any BE support
>> in the driver, call them directly.
>
> That's not entirely truth. If you look at Microblaze then you will see
> that Microblaze can be BE and LE.
> There is just missing endian detection which we will add to the next version.
As far as I know the endianess of the kernel is fixed and known during
compile time. Correct me if I'm wrong. So there is no need for a runtime
detection of the endianess and so no need for {read,write}_reg function
pointers.
> But because MB io helper functions are broken for a while you should be
> able to use this driver on both endianess.
>
> btw: I would prefer to use ioread32 and ioread32be instead of readl.
> Is it OK for you?
Make it so. :)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 242 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140214/93e45d11/attachment.sig>
^ permalink raw reply
* How to get the Linux kernel coverage data on ARM arch when I run LTP test?
From: Naresh Kamboju @ 2014-02-14 9:02 UTC (permalink / raw)
To: linux-arm-kernel
Hi All,
Sorry for the wide distribution:
According to LTP page info,
http://ltp.sourceforge.net/coverage/gcov.php
http://ltp.sourceforge.net/coverage/lcov.php
My question is
How to get kernel coverage when I run LTP test?
Since we do not have direct GCOV support on ARM I have enabled ( just
for experiment)
$ git diff kernel/gcov/Kconfig
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index d4da55d..fceac85 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -35,7 +35,7 @@ config GCOV_KERNEL
config GCOV_PROFILE_ALL
bool "Profile entire Kernel"
depends on GCOV_KERNEL
- depends on SUPERH || S390 || X86 || PPC || MICROBLAZE
+ depends on SUPERH || S390 || X86 || PPC || MICROBLAZE || ARM
default n
---help---
This options activates profiling for the entire kernel.
I have enabled below kernel config and built the kernel.
CONFIG_GCOV_KERNEL=y
CONFIG_GCOV_PROFILE_ALL=y
CONFIG_GCOV_FORMAT_AUTODETECT=y
of course I have disabled gcov on below folders and files, because
since it is an experiment i would like to be on safe side
- arch/arm/mm/
- arch/arm/kernel/
- arch/arm/boot/
- arch/arm/boot/compressed/
- arch/arm/boot/bootp/
- arch/arm/boot/dts
Arndale booted with this configuration successfully.
Took more than 65 seconds to finish boot.
gcov related files are found under /sys/ similer to X86 machine.
And gcov supporting files filename.gcda are having links to kernel source
root at linaro-developer:~# ls /sys/kernel/debug/gcov/
reset tmp
root at linaro-developer:~#
root at linaro-developer:~# ls /sys/kernel/debug/gcov/tmp/
linux-linaro-3.13-2014.01
root at linaro-developer:~# ls
/sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/
arch block crypto drivers fs init ipc kernel lib mm security virt
root at linaro-developer:~# ls
/sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/kernel/time.* -l
-rw------- 1 root root 0 Jan 1 1970
/sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/kernel/time.gcda
lrwxrwxrwx 1 root root 0 Jan 1 1970
/sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/kernel/time.gcno
-> /tmp/linux-linaro-3.13-2014.01/kernel/time.gcno
root at linaro-developer:~#
Source are placed under /tmp/
root at linaro-developer:/tmp/linux-linaro-3.13-2014.01# ls kernel/time.* -l
-rw-rw-r-- 1 linaro linaro 19233 Jan 24 2014 kernel/time.c
-rw-rw-r-- 1 linaro linaro 12768 Feb 13 2014 kernel/time.gcno
-rw-rw-r-- 1 linaro linaro 136444 Feb 13 2014 kernel/time.o
root at linaro-developer:/tmp/linux-linaro-3.13-2014.01#
root at linaro-developer:/tmp/linux-linaro-3.13-2014.01# gcov kernel/gcov/base.c -o
/sys/kernel/debug/gcov/tmp/linux-linaro-3.13-2014.01/kernel/gcov/
File 'kernel/gcov/base.c'
Lines executed:43.18% of 44
Creating 'base.c.gcov'
root at linaro-developer:/tmp/linux-linaro-3.13-2014.01#
The above experiment gives coverage of a single file base.c when i run
gcov manually.
Is there any way to get Linux kernel coverage of all files after
running LTP test cases ?
Thanks in advance.
Best regards
Naresh Kamboju
^ permalink raw reply related
* [PATCH v2] can: xilinx CAN controller support.
From: Michal Simek @ 2014-02-14 8:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52FD1B16.6020600@pengutronix.de>
Hi Marc,
>> + int waiting_ech_skb_num;
>> + int xcan_echo_skb_max_tx;
>> + int xcan_echo_skb_max_rx;
>> + struct napi_struct napi;
>> + spinlock_t ech_skb_lock;
>> + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
>> + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>
> Please remove read_reg, write_reg, as long as there isn't any BE support
> in the driver, call them directly.
That's not entirely truth. If you look at Microblaze then you will see
that Microblaze can be BE and LE.
There is just missing endian detection which we will add to the next version.
But because MB io helper functions are broken for a while you should be
able to use this driver on both endianess.
btw: I would prefer to use ioread32 and ioread32be instead of readl.
Is it OK for you?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140214/5d8e681c/attachment.sig>
^ permalink raw reply
* [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-14 8:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140214075717.GA14682@core.coreip.homeip.net>
2014-02-14 15:57 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Barry,
>
> On Fri, Feb 14, 2014 at 11:20:01AM +0800, Barry Song wrote:
>> From: Xianglong Du <Xianglong.Du@csr.com>
>>
>> this patch adds a delayed_work to detect the untouch of onkey since HW will
>> not generate interrupt for it.
>>
>> at the same time, we move the KEY event to POWER instead of SUSPEND, which
>> will be suitable for both Android and Linux. Userspace PowerManager Daemon
>> will decide to suspend or shutdown based on how long we have touched onkey
>>
>> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
>> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> -v3: move to use custom devres action
>
> Thank you for making the changes, however it seems that we can control
> whether the device generates interrupts or not and so we can implement
> open and close methods. If patch below works then your
> cancel_delayed_work() call should go into sirfosc_pwrc_close() and we do
> not need to use devm_free_irq() not custom action.
this one looks making lots of senses. it makes sure HW will not
trigger any SW behaviour before probe() finishes, and also makes sure
HW will not trigger any SW behaviour in remove(). i'd like xianglong
to give a quick test on it.
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: sirfsoc-onkey - implement open and close methods
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We can control whetehr device generates interrupts or not so let's
> implement open and close methods of input device so that we do not do any
> processing until there are users.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/misc/sirfsoc-onkey.c | 50 +++++++++++++++++++++++++++++-------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index e8897c3..dc7db65 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -49,6 +49,35 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void sirfsoc_pwrc_toggle_interrupts(struct sirfsoc_pwrc_drvdata *pwrcdrv,
> + bool enable)
> +{
> + u32 int_mask;
> +
> + int_mask = sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + if (enable)
> + int_mask |= PWRC_ON_KEY_BIT;
> + else
> + int_mask &= ~PWRC_ON_KEY_BIT;
> + sirfsoc_rtc_iobrg_writel(int_mask, pwrcdrv->pwrc_base + PWRC_INT_MASK);
> +}
> +
> +static int sirfsoc_pwrc_open(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> +
> + return 0;
> +}
> +
> +static void sirfsoc_pwrc_close(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, false);
> +}
> +
> static const struct of_device_id sirfsoc_pwrc_of_match[] = {
> { .compatible = "sirf,prima2-pwrc" },
> {},
> @@ -70,7 +99,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> }
>
> /*
> - * we can't use of_iomap because pwrc is not mapped in memory,
> + * We can't use of_iomap because pwrc is not mapped in memory,
> * the so-called base address is only offset in rtciobrg
> */
> error = of_property_read_u32(np, "reg", &pwrcdrv->pwrc_base);
> @@ -88,6 +117,11 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> pwrcdrv->input->phys = "pwrc/input0";
> pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
>
> + pwrcdrv->input->open = sirfsoc_pwrc_open;
> + pwrcdrv->input->close = sirfsoc_pwrc_close;
> +
> + input_set_drvdata(pwrcdrv->input, pwrcdrv);
> +
> irq = platform_get_irq(pdev, 0);
> error = devm_request_irq(&pdev->dev, irq,
> sirfsoc_pwrc_isr, IRQF_SHARED,
> @@ -98,11 +132,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> return error;
> }
>
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK) |
> - PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> -
> error = input_register_device(pwrcdrv->input);
> if (error) {
> dev_err(&pdev->dev,
> @@ -129,15 +158,16 @@ static int pwrc_resume(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> + struct input_dev *input = pwrcdrv->input;
>
> /*
> * Do not mask pwrc interrupt as we want pwrc work as a wakeup source
> * if users touch X_ONKEY_B, see arch/arm/mach-prima2/pm.c
> */
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(
> - pwrcdrv->pwrc_base + PWRC_INT_MASK) | PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + mutex_lock(&input->mutex);
> + if (input->users)
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> + mutex_unlock(&input->mutex);
>
> return 0;
> }
-barry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox