[1/6] phy: Add max_bitrate attribute & phy_get_max_bitrate()
diff mbox series

Message ID 20181102192616.28291-2-faiz_abbas@ti.com
State New, archived
Headers show
Series
  • Add Support for MCAN transceivers in AM65x-evm
Related show

Commit Message

Faiz Abbas Nov. 2, 2018, 7:26 p.m. UTC
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(+)

Comments

Marc Kleine-Budde Nov. 3, 2018, 9:36 a.m. UTC | #1
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
Faiz Abbas Nov. 5, 2018, 6:27 a.m. UTC | #2
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
Marc Kleine-Budde Nov. 5, 2018, 9:37 a.m. UTC | #3
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
Faiz Abbas Nov. 5, 2018, 11:14 a.m. UTC | #4
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
Marc Kleine-Budde Nov. 5, 2018, 11:47 a.m. UTC | #5
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
Faiz Abbas Nov. 5, 2018, 1:22 p.m. UTC | #6
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

Patch
diff mbox series

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