| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Mar 2014 13:48:54 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
CC: "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"grant.likely@...aro.org" <grant.likely@...aro.org>,
Michal Simek <michals@...inx.com>,
"wg@...ndegger.com" <wg@...ndegger.com>,
"fengguang.wu@...el.com" <fengguang.wu@...el.com>
Subject: Re: [PATCH v5] can: xilinx CAN controller support.
On 03/11/2014 01:34 PM, Appana Durga Kedareswara Rao wrote:
>>> 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
>>
>> Is Zynq multiarch already?
> Discussions are going on this
> So the final thing that Fengguang ( fengguang.wu@...el.com)
> Proposed is
> config CAN_XILINX
> tristate "Xilinx CAN"
> depends on ARCH_ZYNQ || MICROBLAZE || COMPILE_TEST
> depends on COMMON_CLK && HAS_MMIO # whatever you need for other architectures
> Are you Ok for this?
You have to fill the 2nd depends on with some sane values, though.
[...]
>>> +/**
>>> + * struct xcan_priv - This definition define CAN driver instance
>>> + * @can: CAN private data structure.
>>> + * @tx_head: Tx CAN packets ready to send on the
>> queue
>>> + * @tx_tail: Tx CAN packets successfully sended on the
>> queue
>>> + * @tx_max: Maximum number packets the driver can
>> send
>>> + * @napi: NAPI structure
>>> + * @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;
>>> + unsigned int tx_head;
>>> + unsigned int tx_tail;
>>> + u32 tx_max;
>> Please make it an unsigned int, too.
>>
> Ok
>
>
>>> + struct napi_struct napi;
>>> + u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
>>> + void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
>>> + u32 val);
>>> + struct net_device *dev;
>>> + void __iomem *reg_base;
>>> + unsigned long irq_flags;
>>> + struct clk *aperclk;
>>> + struct clk *devclk;
>>
>> Please rename the clock variables to match the names in the DT.
>>
> The clock names are different for axi CAN and CANPS case.
> So will make them as busclk and devclk
> Are you ok with this?
Why not "ref_clk" and "aper_clk" as used in the DT?
>>> +};
>>> +
[...]
>>> +/**
>>> + * xcan_chip_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_chip_start(struct net_device *ndev) {
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + u32 err;
>>> + unsigned long timeout;
>>> +
>>> + /* Check if it is in reset mode */
>>> + err = set_reset_mode(ndev);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + err = xcan_set_bittiming(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) {
>>
>> Your device is in stopped mode, add you go though set_reset_mode()
>> already.
>
> Will remove this if condition I putted this condition here because in set_reset_mode we are putting the
> Device state in Stopped state.
So the device is in CAN_STATE_STOPPED, as this code just went through
set_reset_mode() some lines ago. so it makes no sense to check if it
(still) is (the code runs serialized here).
>>
>>> + /* 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;
>>> +}
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 |
Download attachment "signature.asc" of type "application/pgp-signature" (243 bytes)
Powered by blists - more mailing lists