[PATCH 0/6] Add Support for MCAN transceivers in AM65x-evm

[PATCH 0/6] Add Support for MCAN transceivers in AM65x-evm

[Faiz Abbas]

The following patches add support for CAN transceivers in the AM65x-evm.

The legacy transceiver implementation has a transceiver node as a child of the
m_can node with a max-bitrate property which is read directly by the 
of_can_transceiver() API. The transceivers on the present platform however,
require some configuration (pulling the gpio connected to the stb line of
the transceiver low) before they can start sending messages.

The new implementation models the transceiver as a phy and implements the
max-bitrate as a phy attribute.

patch 1 adds the max_bitrate attribute to the phy core. It also implements the API
to be used by the consumer to get the attribute.

patches 2 & 3 implement a generic phy driver for simple implementations.

patches 4,5 & 6 implement the transceiver as a phy to the m_can driver.

Note: Pinmux and GPIO support for am65x-evm are not yet implemented in upstream.
So I tested this implementation with some out of tree patches on top of linux-next.
dts patches will be posted as soon as the above frameworks are available.

Faiz Abbas (6):
  phy: Add max_bitrate attribute & phy_get_max_bitrate()
  dt-bindings: phy: phy-of-simple: Document new binding
  phy: phy-of-simple: Add support for simple generic phy driver
  dt-bindings: can: m_can: Document transceiver implementation as a phy
  dt-bindings: can: can-transceiver: Remove legacy binding documentation
  can: m_can: Add support for transceiver as phy

 .../bindings/net/can/can-transceiver.txt      | 24 -----
 .../devicetree/bindings/net/can/m_can.txt     | 24 +++--
 .../devicetree/bindings/phy/phy-of-simple.txt | 29 ++++++
 drivers/net/can/m_can/m_can.c                 | 23 ++++-
 drivers/phy/Kconfig                           |  7 ++
 drivers/phy/Makefile                          |  1 +
 drivers/phy/phy-of-simple.c                   | 90 +++++++++++++++++++
 include/linux/phy/phy.h                       |  5 ++
 8 files changed, 170 insertions(+), 33 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
 create mode 100644 Documentation/devicetree/bindings/phy/phy-of-simple.txt
 create mode 100644 drivers/phy/phy-of-simple.c

-- 
2.18.0

[PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Faiz Abbas]

In some subsystems (eg. CAN) the physical layer capabilities are
the limiting factor in the datarate of the device. Typically, the
physical layer transceiver does not provide a way to discover this
limitation at runtime. Thus this information needs to be represented as
a phy attribute which is read from the device tree.

Therefore, add an optional max_bitrate attribute to the generic phy
sybsystem. Also add the complementary API which enables the consumer
to get max_bitrate.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 include/linux/phy/phy.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 03b319f89a34..31574464f09f 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -72,6 +72,7 @@ struct phy_ops {
  */
 struct phy_attrs {
 	u32			bus_width;
+	u32			max_bitrate;
 	enum phy_mode		mode;
 };
 
@@ -179,6 +180,10 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width)
 {
 	phy->attrs.bus_width = bus_width;
 }
+static inline int phy_get_max_bitrate(struct phy *phy)
+{
+	return phy->attrs.max_bitrate;
+}
 struct phy *phy_get(struct device *dev, const char *string);
 struct phy *phy_optional_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
-- 
2.18.0

[PATCH 2/6] dt-bindings: phy: phy-of-simple: Document new binding

[Faiz Abbas]

Add documentation for the generic simple phy implementation.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 .../devicetree/bindings/phy/phy-of-simple.txt | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-of-simple.txt

diff --git a/Documentation/devicetree/bindings/phy/phy-of-simple.txt b/Documentation/devicetree/bindings/phy/phy-of-simple.txt
new file mode 100644
index 000000000000..696f2763395c
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-of-simple.txt
@@ -0,0 +1,29 @@
+Generic simple phy device tree binding
+--------------------------------------
+
+A good number of phy implementations merely read dts properties,
+enable clocks, regulators or do resets without having a dedicated register
+map. This binding implements a generic phy driver which can be used for
+such simple implementations and avoid boilerplate code duplication.
+
+Required Properties:
+-  compatible	: must be "simple-phy"
+-  phy-cells    : must be 0
+
+Optional Properties:
+-  bus-width	: generic bus-width. Must be positive.
+-  max-bitrate	: generic max-bitrate. Must be positive.
+-  pwr		: phandle to phy pwr regulator node.
+
+Example:
+
+The following example is a can transceiver implemented as a generic phy.
+It has a max-bitrate property and a pwr regulator.
+
+
+transceiver1: can-transceiver {
+	compatible = "simple-phy";
+	max-bitrate = <5000000>;
+	pwr-supply = <&transceiver1_fixed>;
+	#phy-cells = <0>;
+};
-- 
2.18.0

[PATCH 3/6] phy: phy-of-simple: Add support for simple generic phy driver

[Faiz Abbas]

A good number of phy implementations don't have a dedicated register
map and only do simple clock or regulator configurations. Add support
for a generic driver which just does such simple configurations.

The driver optionally gets all the generic phy attributes and also the
pwr regulator node. It also includes a generic implementation of
phy_power_on() and phy_power_off().

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/phy/Kconfig         |  7 +++
 drivers/phy/Makefile        |  1 +
 drivers/phy/phy-of-simple.c | 90 +++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)
 create mode 100644 drivers/phy/phy-of-simple.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 60f949e2a684..e66039560da9 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -40,6 +40,13 @@ config PHY_XGENE
 	help
 	  This option enables support for APM X-Gene SoC multi-purpose PHY.
 
+config PHY_OF_SIMPLE
+	tristate "PHY Simple Driver"
+	select GENERIC_PHY
+	help
+	  This driver supports simple generic phy implementations which don't
+	  need a dedicated register map.
+
 source "drivers/phy/allwinner/Kconfig"
 source "drivers/phy/amlogic/Kconfig"
 source "drivers/phy/broadcom/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 0301e25d07c1..ffef0fdf2c68 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -4,6 +4,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+obj-$(CONFIG_PHY_OF_SIMPLE)		+= phy-of-simple.o
 obj-$(CONFIG_PHY_LPC18XX_USB_OTG)	+= phy-lpc18xx-usb-otg.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
 obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
diff --git a/drivers/phy/phy-of-simple.c b/drivers/phy/phy-of-simple.c
new file mode 100644
index 000000000000..0194aae90d3c
--- /dev/null
+++ b/drivers/phy/phy-of-simple.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * phy-of-simple.c - phy driver for simple implementations
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
+ *
+ */
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+
+static int phy_simple_power_on(struct phy *phy)
+{
+	if (phy->pwr)
+		return regulator_enable(phy->pwr);
+
+	return 0;
+}
+
+static int phy_simple_power_off(struct phy *phy)
+{
+	if (phy->pwr)
+		return regulator_disable(phy->pwr);
+
+	return 0;
+}
+
+static const struct phy_ops phy_simple_ops = {
+	.power_on	= phy_simple_power_on,
+	.power_off	= phy_simple_power_off,
+	.owner		= THIS_MODULE,
+};
+
+int phy_simple_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct regulator *pwr = NULL;
+	struct phy *phy;
+	u32 bus_width = 0;
+	u32 max_bitrate = 0;
+	int ret;
+
+	phy = devm_phy_create(dev, dev->of_node,
+				      &phy_simple_ops);
+
+	if (IS_ERR(phy)) {
+		dev_err(dev, "Failed to create phy\n");
+		return PTR_ERR(phy);
+	}
+
+	device_property_read_u32(dev, "bus-width", &bus_width);
+	phy->attrs.bus_width = bus_width;
+	device_property_read_u32(dev, "max-bitrate", &max_bitrate);
+	phy->attrs.max_bitrate = max_bitrate;
+
+	pwr = devm_regulator_get_optional(dev, "pwr");
+	if (IS_ERR(pwr)) {
+		ret = PTR_ERR(pwr);
+		dev_err(dev, "Couldn't get regulator. ret=%d\n", ret);
+		return ret;
+	}
+	phy->pwr = pwr;
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_simple_dt_ids[] = {
+	{ .compatible = "simple-phy"},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
+
+static struct platform_driver phy_simple_driver = {
+	.probe		= phy_simple_probe,
+	.driver		= {
+		.name	= "phy-of-simple",
+		.of_match_table = phy_simple_dt_ids,
+	},
+};
+
+module_platform_driver(phy_simple_driver);
+
+MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
+MODULE_DESCRIPTION("Simple PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

[PATCH 4/6] dt-bindings: can: m_can: Document transceiver implementation as a phy

[Faiz Abbas]

Some transceivers need a configuration step (for example, pulling
a standby line low) for them to start sending messages. Instead of a
parent child relationship, the transceiver can be implemented as a
phy with the configuration done in the phy driver. The bitrate
limitations can then be obtained by the driver using said phy
node.

Document the above implementation.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 .../devicetree/bindings/net/can/m_can.txt     | 24 ++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index ed614383af9c..c11548e74278 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -42,12 +42,14 @@ Required properties:
 
 			  Please refer to 2.4.1 Message RAM Configuration in
 			  Bosch M_CAN user manual for details.
+Optional properties:
+- phy-names		: must be "can_transceiver". The transceiver
+			  typically limits the maximum bitrate of the
+			  system. The phy node is used to discover this
+			  limitation.
+- phys			: phandle to the transceiver implemented as a phy
+			  node.
 
-Optional Subnode:
-- can-transceiver	: Can-transceiver subnode describing maximum speed
-			  that can be used for CAN/CAN-FD modes. See
-			  Documentation/devicetree/bindings/net/can/can-transceiver.txt
-			  for details.
 Example:
 SoC dtsi:
 m_can1: can@20e8000 {
@@ -64,12 +66,18 @@ m_can1: can@20e8000 {
 };
 
 Board dts:
+
 &m_can1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_m_can1>;
 	status = "enabled";
+	phy-names = "can_transceiver";
+	phys = <&transceiver1>;
+};
 
-	can-transceiver {
-		max-bitrate = <5000000>;
-	};
+transceiver1: can-transceiver {
+	compatible = "simple-phy";
+	max-bitrate = <5000000>;
+	pwr-supply = <&transceiver1_fixed>;
+	#phy-cells = <0>;
 };
-- 
2.18.0

[PATCH 5/6] dt-bindings: can: can-transceiver: Remove legacy binding documentation

[Faiz Abbas]

With the transceiver node being implemented as a phy, remove the legacy
dcoumentation. Don't remove the code implementing it to maintain dt
compatibility.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 .../bindings/net/can/can-transceiver.txt      | 24 -------------------
 1 file changed, 24 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt

diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
deleted file mode 100644
index 0011f53ff159..000000000000
--- a/Documentation/devicetree/bindings/net/can/can-transceiver.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-Generic CAN transceiver Device Tree binding
-------------------------------
-
-CAN transceiver typically limits the max speed in standard CAN and CAN FD
-modes. Typically these limitations are static and the transceivers themselves
-provide no way to detect this limitation at runtime. For this situation,
-the "can-transceiver" node can be used.
-
-Required Properties:
- max-bitrate:	a positive non 0 value that determines the max
-		speed that CAN/CAN-FD can run. Any other value
-		will be ignored.
-
-Examples:
-
-Based on Texas Instrument's TCAN1042HGV CAN Transceiver
-
-m_can0 {
-	....
-	can-transceiver {
-		max-bitrate = <5000000>;
-	};
-	...
-};
-- 
2.18.0

[PATCH 6/6] can: m_can: Add support for transceiver as phy

[Faiz Abbas]

Add support for implementing the transceiver device as a phy. Instead
of a child node, the max_bitrate information is obtained by getting a
phy attribute. Fallback to the legacy API if no phy node is found.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/net/can/m_can/m_can.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 9b449400376b..ed6d84acea20 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/iopoll.h>
 #include <linux/can/dev.h>
+#include <linux/phy/phy.h>
 #include <linux/pinctrl/consumer.h>
 
 /* napi related */
@@ -1582,6 +1583,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	struct device_node *np;
 	u32 mram_config_vals[MRAM_CFG_LEN];
 	u32 tx_fifo_size;
+	struct phy *transceiver;
 
 	np = pdev->dev.of_node;
 
@@ -1671,7 +1673,26 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 	devm_can_led_init(dev);
 
-	of_can_transceiver(dev);
+	transceiver = devm_phy_optional_get(&pdev->dev, "can_transceiver");
+	if (IS_ERR(transceiver)) {
+		ret = PTR_ERR(transceiver);
+		dev_err(&pdev->dev, "Couldn't get phy. err=%d\n", ret);
+		return ret;
+	}
+
+	if (!transceiver) {
+		dev_warn(&pdev->dev, "No transceiver phy. Falling back to legacy API\n");
+		of_can_transceiver(dev);
+	} else {
+		ret = phy_power_on(transceiver);
+		if (ret) {
+			dev_err(&pdev->dev, "phy_power_on err:%d\n", ret);
+			return ret;
+		}
+		priv->can.bitrate_max = phy_get_max_bitrate(transceiver);
+		if (!priv->can.bitrate_max)
+			dev_warn(&pdev->dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit\n");
+	}
 
 	dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
 		 KBUILD_MODNAME, dev->irq, priv->version);
-- 
2.18.0

Re: [PATCH 3/6] phy: phy-of-simple: Add support for simple generic phy driver

[kbuild test robot]

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

Hi Faiz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.19 next-20181102]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Faiz-Abbas/Add-Support-for-MCAN-transceivers-in-AM65x-evm/20181103-103548
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=sh 

All error/warnings (new ones prefixed by >>):

   In file included from drivers//phy/phy-of-simple.c:10:0:
>> drivers//phy/phy-of-simple.c:76:25: error: 'phy_simple_phy_dt_ids' undeclared here (not in a function); did you mean 'phy_simple_dt_ids'?
    MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
                            ^
   include/linux/module.h:213:15: note: in definition of macro 'MODULE_DEVICE_TABLE'
    extern typeof(name) __mod_##type##__##name##_device_table  \
                  ^~~~
>> include/linux/module.h:213:21: error: '__mod_of__phy_simple_phy_dt_ids_device_table' aliased to undefined symbol 'phy_simple_phy_dt_ids'
    extern typeof(name) __mod_##type##__##name##_device_table  \
                        ^
>> drivers//phy/phy-of-simple.c:76:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
    MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
    ^~~~~~~~~~~~~~~~~~~
--
   In file included from drivers/phy/phy-of-simple.c:10:0:
   drivers/phy/phy-of-simple.c:76:25: error: 'phy_simple_phy_dt_ids' undeclared here (not in a function); did you mean 'phy_simple_dt_ids'?
    MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
                            ^
   include/linux/module.h:213:15: note: in definition of macro 'MODULE_DEVICE_TABLE'
    extern typeof(name) __mod_##type##__##name##_device_table  \
                  ^~~~
>> include/linux/module.h:213:21: error: '__mod_of__phy_simple_phy_dt_ids_device_table' aliased to undefined symbol 'phy_simple_phy_dt_ids'
    extern typeof(name) __mod_##type##__##name##_device_table  \
                        ^
   drivers/phy/phy-of-simple.c:76:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
    MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
    ^~~~~~~~~~~~~~~~~~~

vim +76 drivers//phy/phy-of-simple.c

  > 10	#include <linux/module.h>
    11	#include <linux/regulator/consumer.h>
    12	
    13	static int phy_simple_power_on(struct phy *phy)
    14	{
    15		if (phy->pwr)
    16			return regulator_enable(phy->pwr);
    17	
    18		return 0;
    19	}
    20	
    21	static int phy_simple_power_off(struct phy *phy)
    22	{
    23		if (phy->pwr)
    24			return regulator_disable(phy->pwr);
    25	
    26		return 0;
    27	}
    28	
    29	static const struct phy_ops phy_simple_ops = {
    30		.power_on	= phy_simple_power_on,
    31		.power_off	= phy_simple_power_off,
    32		.owner		= THIS_MODULE,
    33	};
    34	
    35	int phy_simple_probe(struct platform_device *pdev)
    36	{
    37		struct phy_provider *phy_provider;
    38		struct device *dev = &pdev->dev;
    39		struct regulator *pwr = NULL;
    40		struct phy *phy;
    41		u32 bus_width = 0;
    42		u32 max_bitrate = 0;
    43		int ret;
    44	
    45		phy = devm_phy_create(dev, dev->of_node,
    46					      &phy_simple_ops);
    47	
    48		if (IS_ERR(phy)) {
    49			dev_err(dev, "Failed to create phy\n");
    50			return PTR_ERR(phy);
    51		}
    52	
    53		device_property_read_u32(dev, "bus-width", &bus_width);
    54		phy->attrs.bus_width = bus_width;
    55		device_property_read_u32(dev, "max-bitrate", &max_bitrate);
    56		phy->attrs.max_bitrate = max_bitrate;
    57	
    58		pwr = devm_regulator_get_optional(dev, "pwr");
    59		if (IS_ERR(pwr)) {
    60			ret = PTR_ERR(pwr);
    61			dev_err(dev, "Couldn't get regulator. ret=%d\n", ret);
    62			return ret;
    63		}
    64		phy->pwr = pwr;
    65	
    66		phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
    67	
    68		return PTR_ERR_OR_ZERO(phy_provider);
    69	}
    70	
    71	static const struct of_device_id phy_simple_dt_ids[] = {
    72		{ .compatible = "simple-phy"},
    73		{}
    74	};
    75	
  > 76	MODULE_DEVICE_TABLE(of, phy_simple_phy_dt_ids);
    77	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50346 bytes --]

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Marc Kleine-Budde]

On 11/02/2018 08:26 PM, Faiz Abbas wrote:
> In some subsystems (eg. CAN) the physical layer capabilities are
> the limiting factor in the datarate of the device. Typically, the
> physical layer transceiver does not provide a way to discover this
> limitation at runtime. Thus this information needs to be represented as
> a phy attribute which is read from the device tree.
> 
> Therefore, add an optional max_bitrate attribute to the generic phy
> sybsystem. Also add the complementary API which enables the consumer
> to get max_bitrate.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>

NACK - We already have such a functionality in the CAN subsystem.
Please have a look at the patches:

e759c626d826 can: m_can: Support higher speed CAN-FD bitrates
b54f9eea7667 dt-bindings: can: m_can: Document new can transceiver binding
2290aefa2e90 can: dev: Add support for limiting configured bitrate
54a7fbcc17bc dt-bindings: can: can-transceiver: Document new binding

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   |

Re: [PATCH 5/6] dt-bindings: can: can-transceiver: Remove legacy binding documentation

[Sergei Shtylyov]

On 11/2/2018 10:26 PM, Faiz Abbas wrote:

> With the transceiver node being implemented as a phy, remove the legacy
> dcoumentation. Don't remove the code implementing it to maintain dt

    Documentation.

> compatibility.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
[...]

MBR, Sergei

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Faiz Abbas]

Hi Marc,

On Saturday 03 November 2018 03:06 PM, Marc Kleine-Budde wrote:
> On 11/02/2018 08:26 PM, Faiz Abbas wrote:
>> In some subsystems (eg. CAN) the physical layer capabilities are
>> the limiting factor in the datarate of the device. Typically, the
>> physical layer transceiver does not provide a way to discover this
>> limitation at runtime. Thus this information needs to be represented as
>> a phy attribute which is read from the device tree.
>>
>> Therefore, add an optional max_bitrate attribute to the generic phy
>> sybsystem. Also add the complementary API which enables the consumer
>> to get max_bitrate.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> 
> NACK - We already have such a functionality in the CAN subsystem.
> Please have a look at the patches:
> 
> e759c626d826 can: m_can: Support higher speed CAN-FD bitrates
> b54f9eea7667 dt-bindings: can: m_can: Document new can transceiver binding
> 2290aefa2e90 can: dev: Add support for limiting configured bitrate
> 54a7fbcc17bc dt-bindings: can: can-transceiver: Document new binding
> 

I remove the transceiver child node binding documentation in patch 5/6.

The existing implementation is pretty limiting as it just has a child
node with no associated device. What if a transceiver requires its own
configurations before it can start sending/receiving messages (for
example, my usecase requires it to pull the standby line low)?

I think that can be solved by implementing the transceiver as a phy and
exposing a generic get_max_bitrate API. That way, the transceiver device
can do all its startup configuration in the phy probe function.

In any case, do suggest if you have a better idea on how to implement
pull gpio low requirement.

Thanks,
Faiz

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1259 bytes --]

On 11/05/2018 07:27 AM, Faiz Abbas wrote:
> I remove the transceiver child node binding documentation in patch 5/6.
> 
> The existing implementation is pretty limiting as it just has a child
> node with no associated device. What if a transceiver requires its own
> configurations before it can start sending/receiving messages (for
> example, my usecase requires it to pull the standby line low)?
> 
> I think that can be solved by implementing the transceiver as a phy and
> exposing a generic get_max_bitrate API. That way, the transceiver device
> can do all its startup configuration in the phy probe function.
> 
> In any case, do suggest if you have a better idea on how to implement
> pull gpio low requirement.

As long as we don't have any proper transceiver/phy driver, that does
more than swtich on/off a GPIO, please add a "xceiver" regulator to your
driver. Look for:

> devm_regulator_get(&pdev->dev, "xceiver");

in the flexcan driver.

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

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Faiz Abbas]

Hi,

On Monday 05 November 2018 03:07 PM, Marc Kleine-Budde wrote:
> On 11/05/2018 07:27 AM, Faiz Abbas wrote:
>> I remove the transceiver child node binding documentation in patch 5/6.
>>
>> The existing implementation is pretty limiting as it just has a child
>> node with no associated device. What if a transceiver requires its own
>> configurations before it can start sending/receiving messages (for
>> example, my usecase requires it to pull the standby line low)?
>>
>> I think that can be solved by implementing the transceiver as a phy and
>> exposing a generic get_max_bitrate API. That way, the transceiver device
>> can do all its startup configuration in the phy probe function.
>>
>> In any case, do suggest if you have a better idea on how to implement
>> pull gpio low requirement.
> 
> As long as we don't have any proper transceiver/phy driver, that does
> more than swtich on/off a GPIO, please add a "xceiver" regulator to your
> driver. Look for:
> 
>> devm_regulator_get(&pdev->dev, "xceiver");
> 

The transceiver is not specific to m_can. The pull down would be
required even if it were connected to some other controller.

Thanks,
Faiz

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2320 bytes --]

On 11/05/2018 12:14 PM, Faiz Abbas wrote:
> Hi,
> 
> On Monday 05 November 2018 03:07 PM, Marc Kleine-Budde wrote:
>> On 11/05/2018 07:27 AM, Faiz Abbas wrote:
>>> I remove the transceiver child node binding documentation in patch 5/6.
>>>
>>> The existing implementation is pretty limiting as it just has a child
>>> node with no associated device. What if a transceiver requires its own
>>> configurations before it can start sending/receiving messages (for
>>> example, my usecase requires it to pull the standby line low)?
>>>
>>> I think that can be solved by implementing the transceiver as a phy and
>>> exposing a generic get_max_bitrate API. That way, the transceiver device
>>> can do all its startup configuration in the phy probe function.
>>>
>>> In any case, do suggest if you have a better idea on how to implement
>>> pull gpio low requirement.
>>
>> As long as we don't have any proper transceiver/phy driver, that does
>> more than swtich on/off a GPIO, please add a "xceiver" regulator to your
>> driver. Look for:
>>
>>> devm_regulator_get(&pdev->dev, "xceiver");
>>
> 
> The transceiver is not specific to m_can. The pull down would be
> required even if it were connected to some other controller.

Ok, this is a quite common pattern. For the fsl/nxp boards we add the
regulator to the board dts. See "imx28-evk.dts" for example:

>                         can0: can@80032000 {
>                                 pinctrl-names = "default";
>                                 pinctrl-0 = <&can0_pins_a>;
>                                 xceiver-supply = <&reg_can_3v3>;
>                                 status = "okay";
>                         };
> 
>                         can1: can@80034000 {
>                                 pinctrl-names = "default";
>                                 pinctrl-0 = <&can1_pins_a>;
>                                 xceiver-supply = <&reg_can_3v3>;
>                                 status = "okay";
>                         };

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

Re: [PATCH 1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()

[Faiz Abbas]

Hi Marc,

On Monday 05 November 2018 05:17 PM, Marc Kleine-Budde wrote:
> On 11/05/2018 12:14 PM, Faiz Abbas wrote:
>> Hi,
>>
>> On Monday 05 November 2018 03:07 PM, Marc Kleine-Budde wrote:
>>> On 11/05/2018 07:27 AM, Faiz Abbas wrote:
>>>> I remove the transceiver child node binding documentation in patch 5/6.
>>>>
>>>> The existing implementation is pretty limiting as it just has a child
>>>> node with no associated device. What if a transceiver requires its own
>>>> configurations before it can start sending/receiving messages (for
>>>> example, my usecase requires it to pull the standby line low)?
>>>>
>>>> I think that can be solved by implementing the transceiver as a phy and
>>>> exposing a generic get_max_bitrate API. That way, the transceiver device
>>>> can do all its startup configuration in the phy probe function.
>>>>
>>>> In any case, do suggest if you have a better idea on how to implement
>>>> pull gpio low requirement.
>>>
>>> As long as we don't have any proper transceiver/phy driver, that does
>>> more than swtich on/off a GPIO, please add a "xceiver" regulator to your
>>> driver. Look for:
>>>
>>>> devm_regulator_get(&pdev->dev, "xceiver");
>>>
>>
>> The transceiver is not specific to m_can. The pull down would be
>> required even if it were connected to some other controller.
> 
> Ok, this is a quite common pattern. For the fsl/nxp boards we add the
> regulator to the board dts. See "imx28-evk.dts" for example:
> 
>>                         can0: can@80032000 {
>>                                 pinctrl-names = "default";
>>                                 pinctrl-0 = <&can0_pins_a>;
>>                                 xceiver-supply = <&reg_can_3v3>;
>>                                 status = "okay";
>>                         };
>>
>>                         can1: can@80034000 {
>>                                 pinctrl-names = "default";
>>                                 pinctrl-0 = <&can1_pins_a>;
>>                                 xceiver-supply = <&reg_can_3v3>;
>>                                 status = "okay";
>>                         };
> 

Ok. Will implement that in v2.

Thanks,
Faiz

Re: [PATCH 2/6] dt-bindings: phy: phy-of-simple: Document new binding

[Rob Herring]

On Fri, Nov 2, 2018 at 2:23 PM Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> Add documentation for the generic simple phy implementation.

We don't do 'simple' or 'generic' bindings.

> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  .../devicetree/bindings/phy/phy-of-simple.txt | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-of-simple.txt
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-of-simple.txt b/Documentation/devicetree/bindings/phy/phy-of-simple.txt
> new file mode 100644
> index 000000000000..696f2763395c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-of-simple.txt
> @@ -0,0 +1,29 @@
> +Generic simple phy device tree binding
> +--------------------------------------
> +
> +A good number of phy implementations merely read dts properties,
> +enable clocks, regulators or do resets without having a dedicated register
> +map. This binding implements a generic phy driver which can be used for
> +such simple implementations and avoid boilerplate code duplication.

Sure, but then latter some needs certain timing/ordering of those
controls or some other DT additions. 'generic' or 'simple' never work
out for bindings. By all means though, write a simple/generic phy
driver. Just make it understand an explicit list of compatible
strings. Then when a phy turns out to be not so simple, we can write a
driver for it with changing the DT.

> +Required Properties:
> +-  compatible  : must be "simple-phy"
> +-  phy-cells    : must be 0

#phy-cells

> +
> +Optional Properties:
> +-  bus-width   : generic bus-width. Must be positive.
> +-  max-bitrate : generic max-bitrate. Must be positive.
> +-  pwr         : phandle to phy pwr regulator node.

That's not the regulator binding.

> +
> +Example:
> +
> +The following example is a can transceiver implemented as a generic phy.
> +It has a max-bitrate property and a pwr regulator.
> +
> +
> +transceiver1: can-transceiver {
> +       compatible = "simple-phy";
> +       max-bitrate = <5000000>;
> +       pwr-supply = <&transceiver1_fixed>;
> +       #phy-cells = <0>;
> +};
> --
> 2.18.0
>