| Message ID | 20181102192616.28291-2-faiz_abbas@ti.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add Support for MCAN transceivers in AM65x-evm | expand |
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
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
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
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
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 = <®_can_3v3>; > status = "okay"; > }; > > can1: can@80034000 { > pinctrl-names = "default"; > pinctrl-0 = <&can1_pins_a>; > xceiver-supply = <®_can_3v3>; > status = "okay"; > }; Marc
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 = <®_can_3v3>; >> status = "okay"; >> }; >> >> can1: can@80034000 { >> pinctrl-names = "default"; >> pinctrl-0 = <&can1_pins_a>; >> xceiver-supply = <®_can_3v3>; >> status = "okay"; >> }; > Ok. Will implement that in v2. Thanks, Faiz
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);
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(+)