[PATCH v2 0/2] can: flexcan: upgrade the flexcan.c to support i.MX6

[PATCH v2 0/2] can: flexcan: upgrade the flexcan.c to support i.MX6

[Hui Wang]

The first patch is an incremental one basing on the "can: flexcan: use be32_to_cpup to handle the value of dt entry".

The second patch is inspired from your suggestion, it is a simple implementation and so far only add p1010 and imx6q in the support list. We can extend this list in a later time if it is necessary.


Regards,
Hui.

[PATCH v2 1/2] can: flexcan: use of_property_read_u32 to get DT entry value

[Hui Wang]

of_property_read_u32() can auto handle endian problems, use this
function can make code clean and simple.

No need to check return value here since the following got value
check will handle this.

Cc: linux-can@vger.kernel.org
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
 drivers/net/can/flexcan.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 81d4741..f63f826 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -938,14 +938,9 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	if (IS_ERR(pinctrl))
 		return PTR_ERR(pinctrl);
 
-	if (pdev->dev.of_node) {
-		const __be32 *clock_freq_p;
-
-		clock_freq_p = of_get_property(pdev->dev.of_node,
-						"clock-frequency", NULL);
-		if (clock_freq_p)
-			clock_freq = be32_to_cpup(clock_freq_p);
-	}
+	if (pdev->dev.of_node)
+		of_property_read_u32(pdev->dev.of_node,
+						"clock-frequency", &clock_freq);
 
 	if (!clock_freq) {
 		clk = clk_get(&pdev->dev, NULL);
-- 
1.7.6

[PATCH v2 2/2] can: flexcan: add hardware controller version support

[Hui Wang]

At least in the i.MX series, the flexcan contrller divides into ver_3
and ver_10, current driver is for ver_3 controller.

i.MX6 has ver_10 controller, it has more reigsters than ver_3 has.
The rxfgmask (Rx FIFO Global Mask) register is one of the new added.
Its reset value is 0xffffffff, this means ID Filter Table must be
checked when receive a packet, but the driver is designed to accept
everything during the chip start, we need to clear this register to
follow this design.

Use the data entry of the struct of_device_id to point chip specific
info, we can set hardware version for each platform.

Cc: linux-can@vger.kernel.org
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
 drivers/net/can/flexcan.c |   41 +++++++++++++++++++++++++++++++++--------
 1 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f63f826..9712bdb 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -34,6 +34,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/consumer.h>
 
@@ -165,10 +166,20 @@ struct flexcan_regs {
 	u32 imask1;		/* 0x28 */
 	u32 iflag2;		/* 0x2c */
 	u32 iflag1;		/* 0x30 */
-	u32 _reserved2[19];
+	u32 crl2;		/* 0x34 */
+	u32 esr2;		/* 0x38 */
+	u32 _reserved2[2];
+	u32 crcr;		/* 0x44 */
+	u32 rxfgmask;		/* 0x48 */
+	u32 rxfir;		/* 0x4c */
+	u32 _reserved3[12];
 	struct flexcan_mb cantxfg[64];
 };
 
+struct flexcan_devtype_data {
+	u32 hw_ver;	/* hardware controller version */
+};
+
 struct flexcan_priv {
 	struct can_priv can;
 	struct net_device *dev;
@@ -180,6 +191,15 @@ struct flexcan_priv {
 
 	struct clk *clk;
 	struct flexcan_platform_data *pdata;
+	struct flexcan_devtype_data *devtype_data;
+};
+
+static struct flexcan_devtype_data fsl_p1010_devtype_data = {
+	.hw_ver = 3,
+};
+
+static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
+	.hw_ver = 10,
 };
 
 static struct can_bittiming_const flexcan_bittiming_const = {
@@ -750,6 +770,9 @@ static int flexcan_chip_start(struct net_device *dev)
 	flexcan_write(0x0, &regs->rx14mask);
 	flexcan_write(0x0, &regs->rx15mask);
 
+	if (priv->devtype_data && priv->devtype_data->hw_ver >= 10)
+		flexcan_write(0x0, &regs->rxfgmask);
+
 	flexcan_transceiver_switch(priv, 1);
 
 	/* synchronize with the can bus */
@@ -922,8 +945,16 @@ static void __devexit unregister_flexcandev(struct net_device *dev)
 	unregister_candev(dev);
 }
 
+static const struct of_device_id flexcan_of_match[] = {
+	{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
+	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
+	{},
+};
+
 static int __devinit flexcan_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+			of_match_device(flexcan_of_match, &pdev->dev);
 	struct net_device *dev;
 	struct flexcan_priv *priv;
 	struct resource *mem;
@@ -982,6 +1013,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	dev->flags |= IFF_ECHO;
 
 	priv = netdev_priv(dev);
+	priv->devtype_data = of_id ? of_id->data : NULL;
 	priv->can.clock.freq = clock_freq;
 	priv->can.bittiming_const = &flexcan_bittiming_const;
 	priv->can.do_set_mode = flexcan_set_mode;
@@ -1044,13 +1076,6 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct of_device_id flexcan_of_match[] = {
-	{
-		.compatible = "fsl,p1010-flexcan",
-	},
-	{},
-};
-
 static struct platform_driver flexcan_driver = {
 	.driver = {
 		.name = DRV_NAME,
-- 
1.7.6

Re: [PATCH v2 2/2] can: flexcan: add hardware controller version support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4978 bytes --]

On 06/28/2012 10:21 AM, Hui Wang wrote:
> At least in the i.MX series, the flexcan contrller divides into ver_3
> and ver_10, current driver is for ver_3 controller.
> 
> i.MX6 has ver_10 controller, it has more reigsters than ver_3 has.
> The rxfgmask (Rx FIFO Global Mask) register is one of the new added.
> Its reset value is 0xffffffff, this means ID Filter Table must be
> checked when receive a packet, but the driver is designed to accept
> everything during the chip start, we need to clear this register to
> follow this design.
> 
> Use the data entry of the struct of_device_id to point chip specific
> info, we can set hardware version for each platform.

The patch looks quite good. Can you please add an id_table for the
platform binding, too. Just a single entry with .nam = DRV_NAME and
.driver_data = fsl_p1010_devtype_data, have a look at c_can_platform.c
for example[1]. This way, it's no special case to have the hw_ver info.

[1]
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=69927fccd96b15bd228bb82d356a7a2a0cfaeefb;hp=33f8100977693fa09c2a32b1ca6dbf4d6eabdd0c

Some nitpicks inline,
Marc

> 
> Cc: linux-can@vger.kernel.org
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Hui Wang <jason77.wang@gmail.com>
> ---
>  drivers/net/can/flexcan.c |   41 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index f63f826..9712bdb 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -34,6 +34,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pinctrl/consumer.h>
>  
> @@ -165,10 +166,20 @@ struct flexcan_regs {
>  	u32 imask1;		/* 0x28 */
>  	u32 iflag2;		/* 0x2c */
>  	u32 iflag1;		/* 0x30 */
> -	u32 _reserved2[19];
> +	u32 crl2;		/* 0x34 */
> +	u32 esr2;		/* 0x38 */
> +	u32 _reserved2[2];
> +	u32 crcr;		/* 0x44 */
> +	u32 rxfgmask;		/* 0x48 */
> +	u32 rxfir;		/* 0x4c */
> +	u32 _reserved3[12];
>  	struct flexcan_mb cantxfg[64];
>  };
>  
> +struct flexcan_devtype_data {
> +	u32 hw_ver;	/* hardware controller version */
> +};
> +
>  struct flexcan_priv {
>  	struct can_priv can;
>  	struct net_device *dev;
> @@ -180,6 +191,15 @@ struct flexcan_priv {
>  
>  	struct clk *clk;
>  	struct flexcan_platform_data *pdata;
> +	struct flexcan_devtype_data *devtype_data;
> +};
> +
> +static struct flexcan_devtype_data fsl_p1010_devtype_data = {
const?
> +	.hw_ver = 3,
> +};
> +
> +static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
const
> +	.hw_ver = 10,
>  };
>  
>  static struct can_bittiming_const flexcan_bittiming_const = {
> @@ -750,6 +770,9 @@ static int flexcan_chip_start(struct net_device *dev)
>  	flexcan_write(0x0, &regs->rx14mask);
>  	flexcan_write(0x0, &regs->rx15mask);
>  
> +	if (priv->devtype_data && priv->devtype_data->hw_ver >= 10)

If you have always devtype_data, this should simplify

> +		flexcan_write(0x0, &regs->rxfgmask);
> +
>  	flexcan_transceiver_switch(priv, 1);
>  
>  	/* synchronize with the can bus */
> @@ -922,8 +945,16 @@ static void __devexit unregister_flexcandev(struct net_device *dev)
>  	unregister_candev(dev);
>  }
>  
> +static const struct of_device_id flexcan_of_match[] = {
> +	{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
> +	{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
> +	{},
> +};
> +
>  static int __devinit flexcan_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *of_id =
> +			of_match_device(flexcan_of_match, &pdev->dev);
>  	struct net_device *dev;
>  	struct flexcan_priv *priv;
>  	struct resource *mem;
> @@ -982,6 +1013,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  	dev->flags |= IFF_ECHO;
>  
>  	priv = netdev_priv(dev);
> +	priv->devtype_data = of_id ? of_id->data : NULL;
>  	priv->can.clock.freq = clock_freq;
>  	priv->can.bittiming_const = &flexcan_bittiming_const;
>  	priv->can.do_set_mode = flexcan_set_mode;
> @@ -1044,13 +1076,6 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static struct of_device_id flexcan_of_match[] = {
> -	{
> -		.compatible = "fsl,p1010-flexcan",
> -	},
> -	{},
> -};
> -
>  static struct platform_driver flexcan_driver = {
>  	.driver = {
>  		.name = DRV_NAME,


-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 1/2] can: flexcan: use of_property_read_u32 to get DT entry value

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On 06/28/2012 10:21 AM, Hui Wang wrote:
> of_property_read_u32() can auto handle endian problems, use this
> function can make code clean and simple.
> 
> No need to check return value here since the following got value
> check will handle this.

Should this go to v3.5 or to net-next? If it goes via net-next, I have
to delay it until David merges net (which has the be32_to_cpup fix).

Marc

> 
> Cc: linux-can@vger.kernel.org
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Hui Wang <jason77.wang@gmail.com>
> ---
>  drivers/net/can/flexcan.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 81d4741..f63f826 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -938,14 +938,9 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  	if (IS_ERR(pinctrl))
>  		return PTR_ERR(pinctrl);
>  
> -	if (pdev->dev.of_node) {
> -		const __be32 *clock_freq_p;
> -
> -		clock_freq_p = of_get_property(pdev->dev.of_node,
> -						"clock-frequency", NULL);
> -		if (clock_freq_p)
> -			clock_freq = be32_to_cpup(clock_freq_p);
> -	}
> +	if (pdev->dev.of_node)
> +		of_property_read_u32(pdev->dev.of_node,
> +						"clock-frequency", &clock_freq);
>  
>  	if (!clock_freq) {
>  		clk = clk_get(&pdev->dev, NULL);


-- 
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 1/2] can: flexcan: use of_property_read_u32 to get DT entry value

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On 06/28/2012 10:21 AM, Hui Wang wrote:
> of_property_read_u32() can auto handle endian problems, use this
> function can make code clean and simple.
> 
> No need to check return value here since the following got value
> check will handle this.
> 
> Cc: linux-can@vger.kernel.org
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Hui Wang <jason77.wang@gmail.com>

Applied to linux-can-next

tnx, 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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2 1/2] can: flexcan: use of_property_read_u32 to get DT entry value

[Hui Wang]

Marc Kleine-Budde wrote:
> On 06/28/2012 10:21 AM, Hui Wang wrote:
>   
>> of_property_read_u32() can auto handle endian problems, use this
>> function can make code clean and simple.
>>
>> No need to check return value here since the following got value
>> check will handle this.
>>     
>
> Should this go to v3.5 or to net-next? If it goes via net-next, I have
> to delay it until David merges net (which has the be32_to_cpup fix).
>
> Marc
>
>   
It is for net-next.

Thanks,
Hui.