This series adds driver patches to support MCAN in TI's AM654x-idk.
Faiz Abbas (3):
dt-bindings: net: can: m_can: Add Documentation for stb-gpios
can: m_can: m_can_platform: Add support for enabling transceiver
through the STB line
arm64: defconfig: Add Support for Bosch M_CAN controllers
Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
arch/arm64/configs/defconfig | 3 +++
drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
3 files changed, 17 insertions(+)
--
2.19.2
CAN transceivers on some boards have an STB (standby) line which can be
toggled to enable/disable the transceiver. Add support for enabling the
transceiver using a GPIO connected to the STB line.
Signed-off-by: Faiz Abbas <[email protected]>
---
drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 38ea5e600fb8..b4e1423bd5d8 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -6,6 +6,7 @@
// Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
#include "m_can.h"
@@ -57,6 +58,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
{
struct m_can_classdev *mcan_class;
struct m_can_plat_priv *priv;
+ struct gpio_desc *stb;
struct resource *res;
void __iomem *addr;
void __iomem *mram_addr;
@@ -111,6 +113,16 @@ static int m_can_plat_probe(struct platform_device *pdev)
m_can_init_ram(mcan_class);
+ stb = devm_gpiod_get_optional(&pdev->dev, "stb", GPIOD_OUT_HIGH);
+ if (IS_ERR(stb)) {
+ ret = PTR_ERR(stb);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev,
+ "gpio request failed, ret %d\n", ret);
+
+ goto failed_ret;
+ }
+
ret = m_can_class_register(mcan_class);
failed_ret:
--
2.19.2
Enable configs for supporting Bosch M_CAN controllers.
Signed-off-by: Faiz Abbas <[email protected]>
---
arch/arm64/configs/defconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 458bddeba89c..9d2ea46cc4ae 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -159,6 +159,9 @@ CONFIG_QRTR=m
CONFIG_QRTR_SMD=m
CONFIG_QRTR_TUN=m
CONFIG_BPF_JIT=y
+CONFIG_CAN=m
+CONFIG_CAN_M_CAN=m
+CONFIG_CAN_M_CAN_PLATFORM=m
CONFIG_BT=m
CONFIG_BT_HIDP=m
# CONFIG_BT_HS is not set
--
2.19.2
The CAN transceiver on some boards has an STB pin which is
used to control its standby mode. Add an optional property
stb-gpios to toggle the same.
Signed-off-by: Faiz Abbas <[email protected]>
Signed-off-by: Sekhar Nori <[email protected]>
---
Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index ed614383af9c..cc8ba3f7a2aa 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -48,6 +48,8 @@ Optional Subnode:
that can be used for CAN/CAN-FD modes. See
Documentation/devicetree/bindings/net/can/can-transceiver.txt
for details.
+stb-gpios : gpio node to toggle the STB (standby) signal on the transceiver
+
Example:
SoC dtsi:
m_can1: can@20e8000 {
--
2.19.2
Faiz
On 1/22/20 2:03 AM, Faiz Abbas wrote:
> The CAN transceiver on some boards has an STB pin which is
> used to control its standby mode. Add an optional property
> stb-gpios to toggle the same.
>
> Signed-off-by: Faiz Abbas <[email protected]>
> Signed-off-by: Sekhar Nori <[email protected]>
> ---
> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> index ed614383af9c..cc8ba3f7a2aa 100644
> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -48,6 +48,8 @@ Optional Subnode:
> that can be used for CAN/CAN-FD modes. See
> Documentation/devicetree/bindings/net/can/can-transceiver.txt
> for details.
> +stb-gpios : gpio node to toggle the STB (standby) signal on the transceiver
> +
The m_can.txt is for the m_can framework. If this is specific to the
platform then it really does not belong here.
If the platform has specific nodes then maybe we need a
m_can_platform.txt binding for specific platform nodes. But I leave
that decision to Rob.
Also I prefer you spell out standby like the gpios are spelled out in
the tcan binding.
Dan
> Example:
> SoC dtsi:
> m_can1: can@20e8000 {
On 22/01/20 7:05 PM, Dan Murphy wrote:
> Faiz
>
> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>> The CAN transceiver on some boards has an STB pin which is
>> used to control its standby mode. Add an optional property
>> stb-gpios to toggle the same.
>>
>> Signed-off-by: Faiz Abbas <[email protected]>
>> Signed-off-by: Sekhar Nori <[email protected]>
>> ---
>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index ed614383af9c..cc8ba3f7a2aa 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -48,6 +48,8 @@ Optional Subnode:
>> that can be used for CAN/CAN-FD modes. See
>>
>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>> for details.
>> +stb-gpios : gpio node to toggle the STB (standby) signal on
>> the transceiver
>> +
>
> The m_can.txt is for the m_can framework. If this is specific to the
> platform then it really does not belong here.
>
> If the platform has specific nodes then maybe we need a
> m_can_platform.txt binding for specific platform nodes. But I leave
> that decision to Rob.
Since this is transceiver enable, should this not be in
Documentation/devicetree/bindings/net/can/can-transceiver.txt?
Thanks,
Sekhar
Sekhar
On 1/22/20 8:24 AM, Sekhar Nori wrote:
> On 22/01/20 7:05 PM, Dan Murphy wrote:
>> Faiz
>>
>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>> The CAN transceiver on some boards has an STB pin which is
>>> used to control its standby mode. Add an optional property
>>> stb-gpios to toggle the same.
>>>
>>> Signed-off-by: Faiz Abbas <[email protected]>
>>> Signed-off-by: Sekhar Nori <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>> that can be used for CAN/CAN-FD modes. See
>>>
>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>> for details.
>>> +stb-gpios : gpio node to toggle the STB (standby) signal on
>>> the transceiver
>>> +
>> The m_can.txt is for the m_can framework. If this is specific to the
>> platform then it really does not belong here.
>>
>> If the platform has specific nodes then maybe we need a
>> m_can_platform.txt binding for specific platform nodes. But I leave
>> that decision to Rob.
> Since this is transceiver enable, should this not be in
> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
+1
Dan
> From: [email protected] <[email protected]> On
> Behalf Of Faiz Abbas
> Subject: [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling
> transceiver through the STB line
>
> CAN transceivers on some boards have an STB (standby) line which can be
> toggled to enable/disable the transceiver. Add support for enabling the
> transceiver using a GPIO connected to the STB line.
>
Looks good to me.
Other than Dan's concern on stb as standby,
Acked-by: Sriram Dash <[email protected]>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/can/m_can/m_can_platform.c
> b/drivers/net/can/m_can/m_can_platform.c
> index 38ea5e600fb8..b4e1423bd5d8 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -6,6 +6,7 @@
> // Copyright (C) 2018-19 Texas Instruments Incorporated -
http://www.ti.com/
>
> #include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
>
> #include "m_can.h"
>
> @@ -57,6 +58,7 @@ static int m_can_plat_probe(struct platform_device
*pdev)
> {
> struct m_can_classdev *mcan_class;
> struct m_can_plat_priv *priv;
> + struct gpio_desc *stb;
> struct resource *res;
> void __iomem *addr;
> void __iomem *mram_addr;
> @@ -111,6 +113,16 @@ static int m_can_plat_probe(struct platform_device
> *pdev)
>
> m_can_init_ram(mcan_class);
>
> + stb = devm_gpiod_get_optional(&pdev->dev, "stb", GPIOD_OUT_HIGH);
> + if (IS_ERR(stb)) {
> + ret = PTR_ERR(stb);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev,
> + "gpio request failed, ret %d\n", ret);
> +
> + goto failed_ret;
> + }
> +
> ret = m_can_class_register(mcan_class);
>
> failed_ret:
> --
> 2.19.2
Hi,
On 22/01/20 8:04 pm, Dan Murphy wrote:
> Sekhar
>
> On 1/22/20 8:24 AM, Sekhar Nori wrote:
>> On 22/01/20 7:05 PM, Dan Murphy wrote:
>>> Faiz
>>>
>>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>>> The CAN transceiver on some boards has an STB pin which is
>>>> used to control its standby mode. Add an optional property
>>>> stb-gpios to toggle the same.
>>>>
>>>> Signed-off-by: Faiz Abbas <[email protected]>
>>>> Signed-off-by: Sekhar Nori <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>>> that can be used for CAN/CAN-FD modes. See
>>>>
>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>>> for details.
>>>> +stb-gpios : gpio node to toggle the STB (standby) signal on
>>>> the transceiver
>>>> +
>>> The m_can.txt is for the m_can framework. If this is specific to the
>>> platform then it really does not belong here.
>>>
>>> If the platform has specific nodes then maybe we need a
>>> m_can_platform.txt binding for specific platform nodes. But I leave
>>> that decision to Rob.
>> Since this is transceiver enable, should this not be in
>> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
>
The transceiver node is just a node without an associated device. I had
tried to convert it to a phy implementation but that idea got shot down
here:
https://lore.kernel.org/patchwork/patch/1006238/
Thanks,
Faiz
On 1/22/20 9:03 AM, Faiz Abbas wrote:
> This series adds driver patches to support MCAN in TI's AM654x-idk.
>
> Faiz Abbas (3):
> dt-bindings: net: can: m_can: Add Documentation for stb-gpios
> can: m_can: m_can_platform: Add support for enabling transceiver
> through the STB line
> arm64: defconfig: Add Support for Bosch M_CAN controllers
>
> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
> arch/arm64/configs/defconfig | 3 +++
> drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
> 3 files changed, 17 insertions(+)
What about adding support for xceiver-supply as done in several other
drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Marc,
On 23/01/20 4:47 pm, Marc Kleine-Budde wrote:
> On 1/22/20 9:03 AM, Faiz Abbas wrote:
>> This series adds driver patches to support MCAN in TI's AM654x-idk.
>>
>> Faiz Abbas (3):
>> dt-bindings: net: can: m_can: Add Documentation for stb-gpios
>> can: m_can: m_can_platform: Add support for enabling transceiver
>> through the STB line
>> arm64: defconfig: Add Support for Bosch M_CAN controllers
>>
>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>> arch/arm64/configs/defconfig | 3 +++
>> drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
>> 3 files changed, 17 insertions(+)
>
> What about adding support for xceiver-supply as done in several other
> drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?
Looks like you had given this feedback a long time ago and I forgot
about it. Sorry about that :-)
https://lore.kernel.org/patchwork/patch/1006238/
But now that I think about it, its kinda weird that we are modelling
part of the transceiver as a separate child node
(Documentation/devicetree/bindings/net/can/can-transceiver.txt) and the
other parts as a regulator.
Anyone looking at the transceiver node would figure thats where the
enable gpio/regulator node needs to go instead of the parent node.
Shouldn't we have all transceiver properties under the same node?
Thanks,
Faiz
On 1/23/20 12:46 PM, Faiz Abbas wrote:
> Marc,
>
> On 23/01/20 4:47 pm, Marc Kleine-Budde wrote:
>> On 1/22/20 9:03 AM, Faiz Abbas wrote:
>>> This series adds driver patches to support MCAN in TI's AM654x-idk.
>>>
>>> Faiz Abbas (3):
>>> dt-bindings: net: can: m_can: Add Documentation for stb-gpios
>>> can: m_can: m_can_platform: Add support for enabling transceiver
>>> through the STB line
>>> arm64: defconfig: Add Support for Bosch M_CAN controllers
>>>
>>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>> arch/arm64/configs/defconfig | 3 +++
>>> drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
>>> 3 files changed, 17 insertions(+)
>>
>> What about adding support for xceiver-supply as done in several other
>> drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?
>
> Looks like you had given this feedback a long time ago and I forgot
> about it. Sorry about that :-)
>
> https://lore.kernel.org/patchwork/patch/1006238/
>
> But now that I think about it, its kinda weird that we are modelling
> part of the transceiver as a separate child node
> (Documentation/devicetree/bindings/net/can/can-transceiver.txt) and the
> other parts as a regulator.
We need a regulator, as there are dual phy chips with a single enable line.
> Anyone looking at the transceiver node would figure thats where the
> enable gpio/regulator node needs to go instead of the parent node.
> Shouldn't we have all transceiver properties under the same node?
Feel free to add support for the regulator to the transceiver node and
convert the existing drivers to accept both bindings.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Jan 23, 2020 at 01:09:41PM +0530, Faiz Abbas wrote:
> Hi,
>
> On 22/01/20 8:04 pm, Dan Murphy wrote:
> > Sekhar
> >
> > On 1/22/20 8:24 AM, Sekhar Nori wrote:
> >> On 22/01/20 7:05 PM, Dan Murphy wrote:
> >>> Faiz
> >>>
> >>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
> >>>> The CAN transceiver on some boards has an STB pin which is
> >>>> used to control its standby mode. Add an optional property
> >>>> stb-gpios to toggle the same.
> >>>>
> >>>> Signed-off-by: Faiz Abbas <[email protected]>
> >>>> Signed-off-by: Sekhar Nori <[email protected]>
> >>>> ---
> >>>> ?? Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
> >>>> ?? 1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> index ed614383af9c..cc8ba3f7a2aa 100644
> >>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> @@ -48,6 +48,8 @@ Optional Subnode:
> >>>> ???????????????? that can be used for CAN/CAN-FD modes. See
> >>>> ???????????????
> >>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
> >>>> ???????????????? for details.
> >>>> +stb-gpios??????? : gpio node to toggle the STB (standby) signal on
> >>>> the transceiver
> >>>> +
> >>> The m_can.txt is for the m_can framework.? If this is specific to the
> >>> platform then it really does not belong here.
> >>>
> >>> If the platform has specific nodes then maybe we need a
> >>> m_can_platform.txt binding for specific platform nodes.? But I leave
> >>> that decision to Rob.
> >> Since this is transceiver enable, should this not be in
> >> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
> >
>
> The transceiver node is just a node without an associated device. I had
> tried to convert it to a phy implementation but that idea got shot down
> here:
>
> https://lore.kernel.org/patchwork/patch/1006238/
Nodes and drivers are not a 1-1 thing. Is the transceiver a separate h/w
device? If so, then it should be a separate node and properties of that
device go in its node. Also, nothing is stopping you from using the PHY
binding without using the kernel's PHY framework.
As to whether it should be a separate phy driver, I think probably the
wrong decision was made. We always seem to start out with no PHY on
these things and the complexity just grows until we need one.
Rob
Rob,
On 03/02/20 5:36 pm, Rob Herring wrote:
> On Thu, Jan 23, 2020 at 01:09:41PM +0530, Faiz Abbas wrote:
>> Hi,
>>
>> On 22/01/20 8:04 pm, Dan Murphy wrote:
>>> Sekhar
>>>
>>> On 1/22/20 8:24 AM, Sekhar Nori wrote:
>>>> On 22/01/20 7:05 PM, Dan Murphy wrote:
>>>>> Faiz
>>>>>
>>>>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>>>>> The CAN transceiver on some boards has an STB pin which is
>>>>>> used to control its standby mode. Add an optional property
>>>>>> stb-gpios to toggle the same.
>>>>>>
>>>>>> Signed-off-by: Faiz Abbas <[email protected]>
>>>>>> Signed-off-by: Sekhar Nori <[email protected]>
>>>>>> ---
>>>>>> Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>>>>> that can be used for CAN/CAN-FD modes. See
>>>>>>
>>>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>>>>> for details.
>>>>>> +stb-gpios : gpio node to toggle the STB (standby) signal on
>>>>>> the transceiver
>>>>>> +
>>>>> The m_can.txt is for the m_can framework. If this is specific to the
>>>>> platform then it really does not belong here.
>>>>>
>>>>> If the platform has specific nodes then maybe we need a
>>>>> m_can_platform.txt binding for specific platform nodes. But I leave
>>>>> that decision to Rob.
>>>> Since this is transceiver enable, should this not be in
>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
>>>
>>
>> The transceiver node is just a node without an associated device. I had
>> tried to convert it to a phy implementation but that idea got shot down
>> here:
>>
>> https://lore.kernel.org/patchwork/patch/1006238/
>
> Nodes and drivers are not a 1-1 thing. Is the transceiver a separate h/w
> device? If so, then it should be a separate node and properties of that
> device go in its node.
The transceiver is indeed a separate device.
Also, nothing is stopping you from using the PHY
> binding without using the kernel's PHY framework.
The phy framework seemed like the best code reuse to implement it.
>
> As to whether it should be a separate phy driver, I think probably the
> wrong decision was made. We always seem to start out with no PHY on
> these things and the complexity just grows until we need one.
>
We should be able to handle two properties (one max-datarate and the
other regulator node) for now. If we have to add more complex parts then
maybe we can think about the driver. I am just adding a xceiver
regulator for now.
Thanks,
Faiz