[PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Heiko Schocher]

add DT support for the ti hecc controller, used on
am3517 SoCs.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
 arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
 drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
 3 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt

diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
new file mode 100644
index 0000000..09fab59
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
@@ -0,0 +1,20 @@
+* TI HECC CAN *
+
+Required properties:
+  - compatible: Should be "ti,hecc"
+  - reg: Should contain CAN controller registers location and length
+  - interrupts: Should contain IRQ line for the CAN controller
+
+Example:
+
+	can0: hecc@5c050000 {
+		compatible = "ti,hecc";
+		reg = <0x5c050000 0x4000>;
+		interrupts = <24>;
+		ti,hecc_scc_offset = <0>;
+		ti,hecc_scc_ram_offset = <0x3000>;
+		ti,hecc_ram_offset = <0x3000>;
+		ti,hecc_mbx_offset = <0x2000>;
+		ti,hecc_int_line = <0>;
+		ti,hecc_version = <1>;
+	};
diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
index 5e3f5e8..47bc429 100644
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -25,6 +25,19 @@
 			interrupt-names = "mc";
 		};
 
+		can0: hecc@5c050000 {
+			compatible = "ti,hecc";
+			reg = <0x5c050000 0x4000>;
+			interrupts = <24>;
+			ti,hecc_scc_offset = <0>;
+			ti,hecc_scc_ram_offset = <0x3000>;
+			ti,hecc_ram_offset = <0x3000>;
+			ti,hecc_mbx_offset = <0x2000>;
+			ti,hecc_int_line = <0>;
+			ti,hecc_version = <1>;
+			status = "disabled";
+		};
+
 		davinci_emac: ethernet@0x5c000000 {
 			compatible = "ti,am3517-emac";
 			ti,hwmods = "davinci_emac";
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index c08e8ea..f1705d5 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -875,16 +875,56 @@ static const struct net_device_ops ti_hecc_netdev_ops = {
 	.ndo_change_mtu		= can_change_mtu,
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id ti_hecc_can_dt_ids[] = {
+	{
+		.compatible = "ti,hecc",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, ti_hecc_can_dt_ids);
+#endif
+
+static const struct ti_hecc_platform_data
+*ti_hecc_can_get_driver_data(struct platform_device *pdev)
+{
+	if (pdev->dev.of_node) {
+		struct ti_hecc_platform_data *data;
+		struct device_node *np = pdev->dev.of_node;
+
+		data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+		if (!data)
+			return NULL;
+
+		of_property_read_u32(np, "ti,hecc_scc_offset",
+				     &data->scc_hecc_offset);
+		of_property_read_u32(np, "ti,hecc_scc_ram_offset",
+				     &data->scc_ram_offset);
+		of_property_read_u32(np, "ti,hecc_ram_offset",
+				     &data->hecc_ram_offset);
+		of_property_read_u32(np, "ti,hecc_mbx_offset",
+				     &data->mbx_offset);
+		of_property_read_u32(np, "ti,hecc_int_line",
+				     &data->int_line);
+		of_property_read_u32(np, "ti,hecc_version",
+				     &data->version);
+		return data;
+	}
+	return (const struct ti_hecc_platform_data *)
+		dev_get_platdata(&pdev->dev);
+}
+
 static int ti_hecc_probe(struct platform_device *pdev)
 {
 	struct net_device *ndev = (struct net_device *)0;
 	struct ti_hecc_priv *priv;
-	struct ti_hecc_platform_data *pdata;
+	const struct ti_hecc_platform_data *pdata;
 	struct resource *mem, *irq;
 	void __iomem *addr;
 	int err = -ENODEV;
 
-	pdata = dev_get_platdata(&pdev->dev);
+	pdata = ti_hecc_can_get_driver_data(pdev);
 	if (!pdata) {
 		dev_err(&pdev->dev, "No platform data\n");
 		goto probe_exit;
@@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
 static struct platform_driver ti_hecc_driver = {
 	.driver = {
 		.name    = DRV_NAME,
+		.of_match_table = of_match_ptr(ti_hecc_can_dt_ids),
 	},
 	.probe = ti_hecc_probe,
 	.remove = ti_hecc_remove,
-- 
2.1.0

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Marc Kleine-Budde]

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

On 10/19/2015 08:39 AM, Heiko Schocher wrote:
> add DT support for the ti hecc controller, used on
> am3517 SoCs.

A similar patch was posted a few days ago, see
http://comments.gmane.org/gmane.linux.can/8616 and my comments.

Please coordinate with Anton Glukhov (Cc'ed) and/or pick up his patches
as they are in better shape.

Marc
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
>  .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>  arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>  drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>  3 files changed, 76 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
> new file mode 100644
> index 0000000..09fab59
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
> @@ -0,0 +1,20 @@
> +* TI HECC CAN *
> +
> +Required properties:
> +  - compatible: Should be "ti,hecc"

We usually put the name of the first SoC this IP core appears in to the
compatible.

> +  - reg: Should contain CAN controller registers location and length
> +  - interrupts: Should contain IRQ line for the CAN controller

I'm missing the description of the ti,* properties. I think they are
required, too. Although the code doesn't enforce it.

> +
> +Example:
> +
> +	can0: hecc@5c050000 {
> +		compatible = "ti,hecc";
> +		reg = <0x5c050000 0x4000>;
> +		interrupts = <24>;
> +		ti,hecc_scc_offset = <0>;
> +		ti,hecc_scc_ram_offset = <0x3000>;
> +		ti,hecc_ram_offset = <0x3000>;
> +		ti,hecc_mbx_offset = <0x2000>;
> +		ti,hecc_int_line = <0>;
> +		ti,hecc_version = <1>;

Versioning in the OF world is done via the compatible. Are the offsets a
per SoC parameter? I'm not sure if it's better to put
the offsets into the driver.

> +	};
> diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
> index 5e3f5e8..47bc429 100644
> --- a/arch/arm/boot/dts/am3517.dtsi
> +++ b/arch/arm/boot/dts/am3517.dtsi
> @@ -25,6 +25,19 @@
>  			interrupt-names = "mc";
>  		};
>  
> +		can0: hecc@5c050000 {
> +			compatible = "ti,hecc";
> +			reg = <0x5c050000 0x4000>;
> +			interrupts = <24>;
> +			ti,hecc_scc_offset = <0>;
> +			ti,hecc_scc_ram_offset = <0x3000>;
> +			ti,hecc_ram_offset = <0x3000>;
> +			ti,hecc_mbx_offset = <0x2000>;
> +			ti,hecc_int_line = <0>;
> +			ti,hecc_version = <1>;
> +			status = "disabled";
> +		};
> +
>  		davinci_emac: ethernet@0x5c000000 {
>  			compatible = "ti,am3517-emac";
>  			ti,hwmods = "davinci_emac";
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index c08e8ea..f1705d5 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -875,16 +875,56 @@ static const struct net_device_ops ti_hecc_netdev_ops = {
>  	.ndo_change_mtu		= can_change_mtu,
>  };
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id ti_hecc_can_dt_ids[] = {
> +	{
> +		.compatible = "ti,hecc",
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, ti_hecc_can_dt_ids);
> +#endif

Please remove the ifdef, use __maybe_unused instead.

> +
> +static const struct ti_hecc_platform_data
> +*ti_hecc_can_get_driver_data(struct platform_device *pdev)
> +{
> +	if (pdev->dev.of_node) {
> +		struct ti_hecc_platform_data *data;
> +		struct device_node *np = pdev->dev.of_node;
> +
> +		data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			return NULL;
> +
> +		of_property_read_u32(np, "ti,hecc_scc_offset",
> +				     &data->scc_hecc_offset);
> +		of_property_read_u32(np, "ti,hecc_scc_ram_offset",
> +				     &data->scc_ram_offset);
> +		of_property_read_u32(np, "ti,hecc_ram_offset",
> +				     &data->hecc_ram_offset);
> +		of_property_read_u32(np, "ti,hecc_mbx_offset",
> +				     &data->mbx_offset);
> +		of_property_read_u32(np, "ti,hecc_int_line",
> +				     &data->int_line);
> +		of_property_read_u32(np, "ti,hecc_version",
> +				     &data->version);

I'm missing error handling here.

> +		return data;
> +	}
> +	return (const struct ti_hecc_platform_data *)
> +		dev_get_platdata(&pdev->dev);

Is this cast needed?

> +}
> +
>  static int ti_hecc_probe(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = (struct net_device *)0;
>  	struct ti_hecc_priv *priv;
> -	struct ti_hecc_platform_data *pdata;
> +	const struct ti_hecc_platform_data *pdata;
>  	struct resource *mem, *irq;
>  	void __iomem *addr;
>  	int err = -ENODEV;
>  
> -	pdata = dev_get_platdata(&pdev->dev);
> +	pdata = ti_hecc_can_get_driver_data(pdev);
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "No platform data\n");
>  		goto probe_exit;
> @@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
>  static struct platform_driver ti_hecc_driver = {
>  	.driver = {
>  		.name    = DRV_NAME,
> +		.of_match_table = of_match_ptr(ti_hecc_can_dt_ids),
>  	},
>  	.probe = ti_hecc_probe,
>  	.remove = ti_hecc_remove,
> 

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: 455 bytes --]

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Heiko Schocher]

Hello Marc,

Am 19.10.2015 um 08:58 schrieb Marc Kleine-Budde:
> On 10/19/2015 08:39 AM, Heiko Schocher wrote:
>> add DT support for the ti hecc controller, used on
>> am3517 SoCs.
>
> A similar patch was posted a few days ago, see
> http://comments.gmane.org/gmane.linux.can/8616 and my comments.

Uh, sorry! Seems I missed them ...

> Please coordinate with Anton Glukhov (Cc'ed) and/or pick up his patches
> as they are in better shape.

Yes, I try the patchset from Anton ... thanks for pointing to them.

@Anton: Do you have a newer version, which contains the comments
         from Marc?

bye,
Heiko
>
> Marc
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>>   .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>   arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>   drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>   3 files changed, 76 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>> new file mode 100644
>> index 0000000..09fab59
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>> @@ -0,0 +1,20 @@
>> +* TI HECC CAN *
>> +
>> +Required properties:
>> +  - compatible: Should be "ti,hecc"
>
> We usually put the name of the first SoC this IP core appears in to the
> compatible.

Ok, so "ti,am335xx-hecc" would be OK?
@Anton: you used "am35x" ... it should be "am35xx"

>> +  - reg: Should contain CAN controller registers location and length
>> +  - interrupts: Should contain IRQ line for the CAN controller
>
> I'm missing the description of the ti,* properties. I think they are
> required, too. Although the code doesn't enforce it.

Ok.

>> +
>> +Example:
>> +
>> +	can0: hecc@5c050000 {
>> +		compatible = "ti,hecc";
>> +		reg = <0x5c050000 0x4000>;
>> +		interrupts = <24>;
>> +		ti,hecc_scc_offset = <0>;
>> +		ti,hecc_scc_ram_offset = <0x3000>;
>> +		ti,hecc_ram_offset = <0x3000>;
>> +		ti,hecc_mbx_offset = <0x2000>;
>> +		ti,hecc_int_line = <0>;
>> +		ti,hecc_version = <1>;
>
> Versioning in the OF world is done via the compatible. Are the offsets a
> per SoC parameter? I'm not sure if it's better to put
> the offsets into the driver.

I am unsure here too..

>> +	};
>> diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
>> index 5e3f5e8..47bc429 100644
>> --- a/arch/arm/boot/dts/am3517.dtsi
>> +++ b/arch/arm/boot/dts/am3517.dtsi
>> @@ -25,6 +25,19 @@
>>   			interrupt-names = "mc";
>>   		};
>>
>> +		can0: hecc@5c050000 {
>> +			compatible = "ti,hecc";
>> +			reg = <0x5c050000 0x4000>;
>> +			interrupts = <24>;
>> +			ti,hecc_scc_offset = <0>;
>> +			ti,hecc_scc_ram_offset = <0x3000>;
>> +			ti,hecc_ram_offset = <0x3000>;
>> +			ti,hecc_mbx_offset = <0x2000>;
>> +			ti,hecc_int_line = <0>;
>> +			ti,hecc_version = <1>;
>> +			status = "disabled";
>> +		};
>> +
>>   		davinci_emac: ethernet@0x5c000000 {
>>   			compatible = "ti,am3517-emac";
>>   			ti,hwmods = "davinci_emac";
>> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
>> index c08e8ea..f1705d5 100644
>> --- a/drivers/net/can/ti_hecc.c
>> +++ b/drivers/net/can/ti_hecc.c
>> @@ -875,16 +875,56 @@ static const struct net_device_ops ti_hecc_netdev_ops = {
>>   	.ndo_change_mtu		= can_change_mtu,
>>   };
>>
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id ti_hecc_can_dt_ids[] = {
>> +	{
>> +		.compatible = "ti,hecc",
>> +	}, {
>> +		/* sentinel */
>> +	}
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_hecc_can_dt_ids);
>> +#endif
>
> Please remove the ifdef, use __maybe_unused instead.
>
>> +
>> +static const struct ti_hecc_platform_data
>> +*ti_hecc_can_get_driver_data(struct platform_device *pdev)
>> +{
>> +	if (pdev->dev.of_node) {
>> +		struct ti_hecc_platform_data *data;
>> +		struct device_node *np = pdev->dev.of_node;
>> +
>> +		data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +		if (!data)
>> +			return NULL;
>> +
>> +		of_property_read_u32(np, "ti,hecc_scc_offset",
>> +				     &data->scc_hecc_offset);
>> +		of_property_read_u32(np, "ti,hecc_scc_ram_offset",
>> +				     &data->scc_ram_offset);
>> +		of_property_read_u32(np, "ti,hecc_ram_offset",
>> +				     &data->hecc_ram_offset);
>> +		of_property_read_u32(np, "ti,hecc_mbx_offset",
>> +				     &data->mbx_offset);
>> +		of_property_read_u32(np, "ti,hecc_int_line",
>> +				     &data->int_line);
>> +		of_property_read_u32(np, "ti,hecc_version",
>> +				     &data->version);
>
> I'm missing error handling here.
>
>> +		return data;
>> +	}
>> +	return (const struct ti_hecc_platform_data *)
>> +		dev_get_platdata(&pdev->dev);
>
> Is this cast needed?
>
>> +}
>> +
>>   static int ti_hecc_probe(struct platform_device *pdev)
>>   {
>>   	struct net_device *ndev = (struct net_device *)0;
>>   	struct ti_hecc_priv *priv;
>> -	struct ti_hecc_platform_data *pdata;
>> +	const struct ti_hecc_platform_data *pdata;
>>   	struct resource *mem, *irq;
>>   	void __iomem *addr;
>>   	int err = -ENODEV;
>>
>> -	pdata = dev_get_platdata(&pdev->dev);
>> +	pdata = ti_hecc_can_get_driver_data(pdev);
>>   	if (!pdata) {
>>   		dev_err(&pdev->dev, "No platform data\n");
>>   		goto probe_exit;
>> @@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
>>   static struct platform_driver ti_hecc_driver = {
>>   	.driver = {
>>   		.name    = DRV_NAME,
>> +		.of_match_table = of_match_ptr(ti_hecc_can_dt_ids),
>>   	},
>>   	.probe = ti_hecc_probe,
>>   	.remove = ti_hecc_remove,
>>
>
> Marc
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Marc Kleine-Budde]

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

On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>>   .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>>   arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>>   drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>>   3 files changed, 76 insertions(+), 2 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>> new file mode 100644
>>> index 0000000..09fab59
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>> @@ -0,0 +1,20 @@
>>> +* TI HECC CAN *
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "ti,hecc"
>>
>> We usually put the name of the first SoC this IP core appears in to the
>> compatible.
> 
> Ok, so "ti,am335xx-hecc" would be OK?
> @Anton: you used "am35x" ... it should be "am35xx"

The "xx" is not okay. Give precisely the first SoC Version this IP core
was implemented in.

> 
>>> +  - reg: Should contain CAN controller registers location and length
>>> +  - interrupts: Should contain IRQ line for the CAN controller
>>
>> I'm missing the description of the ti,* properties. I think they are
>> required, too. Although the code doesn't enforce it.
> 
> Ok.
> 
>>> +
>>> +Example:
>>> +
>>> +	can0: hecc@5c050000 {
>>> +		compatible = "ti,hecc";
>>> +		reg = <0x5c050000 0x4000>;
>>> +		interrupts = <24>;
>>> +		ti,hecc_scc_offset = <0>;
>>> +		ti,hecc_scc_ram_offset = <0x3000>;
>>> +		ti,hecc_ram_offset = <0x3000>;
>>> +		ti,hecc_mbx_offset = <0x2000>;
>>> +		ti,hecc_int_line = <0>;
>>> +		ti,hecc_version = <1>;
>>
>> Versioning in the OF world is done via the compatible. Are the offsets a
>> per SoC parameter? I'm not sure if it's better to put
>> the offsets into the driver.
> 
> I am unsure here too..

The devicetree people will hopefully help here.

regards,
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: 455 bytes --]

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Anton.Glukhov]

Hello Marc, Heiko!
I'm sorry for the delay!

On 19.10.2015 10:31, Marc Kleine-Budde wrote:
> On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>>>   .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>>>   arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>>>   drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>>>   3 files changed, 76 insertions(+), 2 deletions(-)
>>>>   create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>> new file mode 100644
>>>> index 0000000..09fab59
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>> @@ -0,0 +1,20 @@
>>>> +* TI HECC CAN *
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Should be "ti,hecc"
>>>
>>> We usually put the name of the first SoC this IP core appears in to the
>>> compatible.
>>
>> Ok, so "ti,am335xx-hecc" would be OK?
>> @Anton: you used "am35x" ... it should be "am35xx"
> 
> The "xx" is not okay. Give precisely the first SoC Version this IP core
> was implemented in.
> 

It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
So, I'm confused about what's "name" should I use.

>>
>>>> +  - reg: Should contain CAN controller registers location and length
>>>> +  - interrupts: Should contain IRQ line for the CAN controller
>>>
>>> I'm missing the description of the ti,* properties. I think they are
>>> required, too. Although the code doesn't enforce it.
>>
>> Ok.
>>
>>>> +
>>>> +Example:
>>>> +
>>>> +	can0: hecc@5c050000 {
>>>> +		compatible = "ti,hecc";
>>>> +		reg = <0x5c050000 0x4000>;
>>>> +		interrupts = <24>;
>>>> +		ti,hecc_scc_offset = <0>;
>>>> +		ti,hecc_scc_ram_offset = <0x3000>;
>>>> +		ti,hecc_ram_offset = <0x3000>;
>>>> +		ti,hecc_mbx_offset = <0x2000>;
>>>> +		ti,hecc_int_line = <0>;
>>>> +		ti,hecc_version = <1>;
>>>
>>> Versioning in the OF world is done via the compatible. Are the offsets a
>>> per SoC parameter? I'm not sure if it's better to put
>>> the offsets into the driver.
>>
>> I am unsure here too..
> 
> The devicetree people will hopefully help here.
> 

I added offsets here just make it consistent with platform data in machine file.
Actually it seems that it's not necessary to put offsets in DT file and I can move it to driver.
But again, it was added to keep consistency.

> regards,
> Marc
> 

regards,
Anton

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Marc Kleine-Budde]

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

On 10/20/2015 04:57 PM, Anton.Glukhov wrote:
> Hello Marc, Heiko!
> I'm sorry for the delay!
> 
> On 19.10.2015 10:31, Marc Kleine-Budde wrote:
>> On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>>>>   .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>>>>   arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>>>>   drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>>>>   3 files changed, 76 insertions(+), 2 deletions(-)
>>>>>   create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>> new file mode 100644
>>>>> index 0000000..09fab59
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>> @@ -0,0 +1,20 @@
>>>>> +* TI HECC CAN *
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: Should be "ti,hecc"
>>>>
>>>> We usually put the name of the first SoC this IP core appears in to the
>>>> compatible.
>>>
>>> Ok, so "ti,am335xx-hecc" would be OK?
>>> @Anton: you used "am35x" ... it should be "am35xx"
>>
>> The "xx" is not okay. Give precisely the first SoC Version this IP core
>> was implemented in.
>>
> 
> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
> So, I'm confused about what's "name" should I use.

Which SoC was available first? Pick that.

>>>>> +  - reg: Should contain CAN controller registers location and length
>>>>> +  - interrupts: Should contain IRQ line for the CAN controller
>>>>
>>>> I'm missing the description of the ti,* properties. I think they are
>>>> required, too. Although the code doesn't enforce it.
>>>
>>> Ok.
>>>
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +	can0: hecc@5c050000 {
>>>>> +		compatible = "ti,hecc";
>>>>> +		reg = <0x5c050000 0x4000>;
>>>>> +		interrupts = <24>;
>>>>> +		ti,hecc_scc_offset = <0>;
>>>>> +		ti,hecc_scc_ram_offset = <0x3000>;
>>>>> +		ti,hecc_ram_offset = <0x3000>;
>>>>> +		ti,hecc_mbx_offset = <0x2000>;
>>>>> +		ti,hecc_int_line = <0>;
>>>>> +		ti,hecc_version = <1>;
>>>>
>>>> Versioning in the OF world is done via the compatible. Are the offsets a
>>>> per SoC parameter? I'm not sure if it's better to put
>>>> the offsets into the driver.
>>>
>>> I am unsure here too..
>>
>> The devicetree people will hopefully help here.
>>
> 
> I added offsets here just make it consistent with platform data in machine file.
> Actually it seems that it's not necessary to put offsets in DT file and I can move it to driver.
> But again, it was added to keep consistency.

The DT is supposed to be OS independent, copying from platform data to
DT is sometimes not the best way to go. Make yourself heard on the
devicetree mailinglist and figure out what's the best way to go here.

Are the offsets for the AM3505 and AM3517 identical?

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: 455 bytes --]

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Anton.Glukhov]

On 20.10.2015 18:05, Marc Kleine-Budde wrote:
> On 10/20/2015 04:57 PM, Anton.Glukhov wrote:
>> Hello Marc, Heiko!
>> I'm sorry for the delay!
>>
>> On 19.10.2015 10:31, Marc Kleine-Budde wrote:
>>> On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>>>>>   .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>>>>>   arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>>>>>   drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>>>>>   3 files changed, 76 insertions(+), 2 deletions(-)
>>>>>>   create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..09fab59
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +* TI HECC CAN *
>>>>>> +
>>>>>> +Required properties:
>>>>>> +  - compatible: Should be "ti,hecc"
>>>>>
>>>>> We usually put the name of the first SoC this IP core appears in to the
>>>>> compatible.
>>>>
>>>> Ok, so "ti,am335xx-hecc" would be OK?
>>>> @Anton: you used "am35x" ... it should be "am35xx"
>>>
>>> The "xx" is not okay. Give precisely the first SoC Version this IP core
>>> was implemented in.
>>>
>>
>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
>> So, I'm confused about what's "name" should I use.
> 
> Which SoC was available first? Pick that.
> 

What do you mean available? I know only that HECC appear in AM3505 and AM3517. Nowhere else.

>>>>>> +  - reg: Should contain CAN controller registers location and length
>>>>>> +  - interrupts: Should contain IRQ line for the CAN controller
>>>>>
>>>>> I'm missing the description of the ti,* properties. I think they are
>>>>> required, too. Although the code doesn't enforce it.
>>>>
>>>> Ok.
>>>>
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +	can0: hecc@5c050000 {
>>>>>> +		compatible = "ti,hecc";
>>>>>> +		reg = <0x5c050000 0x4000>;
>>>>>> +		interrupts = <24>;
>>>>>> +		ti,hecc_scc_offset = <0>;
>>>>>> +		ti,hecc_scc_ram_offset = <0x3000>;
>>>>>> +		ti,hecc_ram_offset = <0x3000>;
>>>>>> +		ti,hecc_mbx_offset = <0x2000>;
>>>>>> +		ti,hecc_int_line = <0>;
>>>>>> +		ti,hecc_version = <1>;
>>>>>
>>>>> Versioning in the OF world is done via the compatible. Are the offsets a
>>>>> per SoC parameter? I'm not sure if it's better to put
>>>>> the offsets into the driver.
>>>>
>>>> I am unsure here too..
>>>
>>> The devicetree people will hopefully help here.
>>>
>>
>> I added offsets here just make it consistent with platform data in machine file.
>> Actually it seems that it's not necessary to put offsets in DT file and I can move it to driver.
>> But again, it was added to keep consistency.
> 
> The DT is supposed to be OS independent, copying from platform data to
> DT is sometimes not the best way to go. Make yourself heard on the
> devicetree mailinglist and figure out what's the best way to go here.
> 
> Are the offsets for the AM3505 and AM3517 identical?

Yes, absolutely, and there is no HECC module in another SoCs. Ok, anyway I'll try to figure out what should we use here.

> 
> Marc
> 

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Marc Kleine-Budde]

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

On 10/20/2015 05:09 PM, Anton.Glukhov wrote:
>>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
>>> So, I'm confused about what's "name" should I use.
>>
>> Which SoC was available first? Pick that.

> What do you mean available? I know only that HECC appear in AM3505 and AM3517. Nowhere else.

Available on the market. Which one was produced first or was soldered
first to some PCB?

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: 455 bytes --]

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Marc Kleine-Budde]

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

On 10/20/2015 05:18 PM, Marc Kleine-Budde wrote:
> On 10/20/2015 05:09 PM, Anton.Glukhov wrote:
>>>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
>>>> So, I'm confused about what's "name" should I use.
>>>
>>> Which SoC was available first? Pick that.
> 
>> What do you mean available? I know only that HECC appear in AM3505 and AM3517. Nowhere else.
> 
> Available on the market. Which one was produced first or was soldered
> first to some PCB?

A colleague just looked it up, a AM3517 is a AM3505 with 3d graphics. So
pick AM3505.

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: 455 bytes --]

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Anton.Glukhov]

On 20.10.2015 18:20, Marc Kleine-Budde wrote:
> On 10/20/2015 05:18 PM, Marc Kleine-Budde wrote:
>> On 10/20/2015 05:09 PM, Anton.Glukhov wrote:
>>>>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
>>>>> So, I'm confused about what's "name" should I use.
>>>>
>>>> Which SoC was available first? Pick that.
>>
>>> What do you mean available? I know only that HECC appear in AM3505 and AM3517. Nowhere else.
>>
>> Available on the market. Which one was produced first or was soldered
>> first to some PCB?
> 
> A colleague just looked it up, a AM3517 is a AM3505 with 3d graphics. So
> pick AM3505.

Ok, thank you for help!

> 
> Marc
> 

Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller

[Rob Herring]

On Mon, Oct 19, 2015 at 1:58 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 10/19/2015 08:39 AM, Heiko Schocher wrote:
>> add DT support for the ti hecc controller, used on
>> am3517 SoCs.
>
> A similar patch was posted a few days ago, see
> http://comments.gmane.org/gmane.linux.can/8616 and my comments.

I don't seem to have that in my inbox. Please send DT bindings to the
DT list and maintainers.

Rob

>
> Please coordinate with Anton Glukhov (Cc'ed) and/or pick up his patches
> as they are in better shape.
>
> Marc
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>>  .../devicetree/bindings/net/can/ti_hecc-can.txt    | 20 ++++++++++
>>  arch/arm/boot/dts/am3517.dtsi                      | 13 +++++++
>>  drivers/net/can/ti_hecc.c                          | 45 +++++++++++++++++++++-
>>  3 files changed, 76 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>> new file mode 100644
>> index 0000000..09fab59
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>> @@ -0,0 +1,20 @@
>> +* TI HECC CAN *
>> +
>> +Required properties:
>> +  - compatible: Should be "ti,hecc"
>
> We usually put the name of the first SoC this IP core appears in to the
> compatible.
>
>> +  - reg: Should contain CAN controller registers location and length
>> +  - interrupts: Should contain IRQ line for the CAN controller
>
> I'm missing the description of the ti,* properties. I think they are
> required, too. Although the code doesn't enforce it.
>
>> +
>> +Example:
>> +
>> +     can0: hecc@5c050000 {
>> +             compatible = "ti,hecc";
>> +             reg = <0x5c050000 0x4000>;
>> +             interrupts = <24>;
>> +             ti,hecc_scc_offset = <0>;
>> +             ti,hecc_scc_ram_offset = <0x3000>;
>> +             ti,hecc_ram_offset = <0x3000>;
>> +             ti,hecc_mbx_offset = <0x2000>;
>> +             ti,hecc_int_line = <0>;
>> +             ti,hecc_version = <1>;
>
> Versioning in the OF world is done via the compatible. Are the offsets a
> per SoC parameter? I'm not sure if it's better to put
> the offsets into the driver.
>
>> +     };
>> diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
>> index 5e3f5e8..47bc429 100644
>> --- a/arch/arm/boot/dts/am3517.dtsi
>> +++ b/arch/arm/boot/dts/am3517.dtsi
>> @@ -25,6 +25,19 @@
>>                       interrupt-names = "mc";
>>               };
>>
>> +             can0: hecc@5c050000 {
>> +                     compatible = "ti,hecc";
>> +                     reg = <0x5c050000 0x4000>;
>> +                     interrupts = <24>;
>> +                     ti,hecc_scc_offset = <0>;
>> +                     ti,hecc_scc_ram_offset = <0x3000>;
>> +                     ti,hecc_ram_offset = <0x3000>;
>> +                     ti,hecc_mbx_offset = <0x2000>;
>> +                     ti,hecc_int_line = <0>;
>> +                     ti,hecc_version = <1>;
>> +                     status = "disabled";
>> +             };
>> +
>>               davinci_emac: ethernet@0x5c000000 {
>>                       compatible = "ti,am3517-emac";
>>                       ti,hwmods = "davinci_emac";
>> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
>> index c08e8ea..f1705d5 100644
>> --- a/drivers/net/can/ti_hecc.c
>> +++ b/drivers/net/can/ti_hecc.c
>> @@ -875,16 +875,56 @@ static const struct net_device_ops ti_hecc_netdev_ops = {
>>       .ndo_change_mtu         = can_change_mtu,
>>  };
>>
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id ti_hecc_can_dt_ids[] = {
>> +     {
>> +             .compatible = "ti,hecc",
>> +     }, {
>> +             /* sentinel */
>> +     }
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_hecc_can_dt_ids);
>> +#endif
>
> Please remove the ifdef, use __maybe_unused instead.
>
>> +
>> +static const struct ti_hecc_platform_data
>> +*ti_hecc_can_get_driver_data(struct platform_device *pdev)
>> +{
>> +     if (pdev->dev.of_node) {
>> +             struct ti_hecc_platform_data *data;
>> +             struct device_node *np = pdev->dev.of_node;
>> +
>> +             data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +             if (!data)
>> +                     return NULL;
>> +
>> +             of_property_read_u32(np, "ti,hecc_scc_offset",
>> +                                  &data->scc_hecc_offset);
>> +             of_property_read_u32(np, "ti,hecc_scc_ram_offset",
>> +                                  &data->scc_ram_offset);
>> +             of_property_read_u32(np, "ti,hecc_ram_offset",
>> +                                  &data->hecc_ram_offset);
>> +             of_property_read_u32(np, "ti,hecc_mbx_offset",
>> +                                  &data->mbx_offset);
>> +             of_property_read_u32(np, "ti,hecc_int_line",
>> +                                  &data->int_line);
>> +             of_property_read_u32(np, "ti,hecc_version",
>> +                                  &data->version);
>
> I'm missing error handling here.
>
>> +             return data;
>> +     }
>> +     return (const struct ti_hecc_platform_data *)
>> +             dev_get_platdata(&pdev->dev);
>
> Is this cast needed?
>
>> +}
>> +
>>  static int ti_hecc_probe(struct platform_device *pdev)
>>  {
>>       struct net_device *ndev = (struct net_device *)0;
>>       struct ti_hecc_priv *priv;
>> -     struct ti_hecc_platform_data *pdata;
>> +     const struct ti_hecc_platform_data *pdata;
>>       struct resource *mem, *irq;
>>       void __iomem *addr;
>>       int err = -ENODEV;
>>
>> -     pdata = dev_get_platdata(&pdev->dev);
>> +     pdata = ti_hecc_can_get_driver_data(pdev);
>>       if (!pdata) {
>>               dev_err(&pdev->dev, "No platform data\n");
>>               goto probe_exit;
>> @@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
>>  static struct platform_driver ti_hecc_driver = {
>>       .driver = {
>>               .name    = DRV_NAME,
>> +             .of_match_table = of_match_ptr(ti_hecc_can_dt_ids),
>>       },
>>       .probe = ti_hecc_probe,
>>       .remove = ti_hecc_remove,
>>
>
> 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   |
>