| Message ID | e825b50a843ffe40e33f34e4d858c07c1b2ff259.1678280913.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] can: rcar_canfd: Add transceiver support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject, async |
On Wed. 8 Mar. 2023 at 22:20, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Add support for CAN transceivers described as PHYs. > > While simple CAN transceivers can do without, this is needed for CAN > transceivers like NXP TJR1443 that need a configuration step (like > pulling standby or enable lines), and/or impose a bitrate limit. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Simon Horman <simon.horman@corigine.com> I have one nitpick (see below). Aside from that: Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > v2: > - Add Reviewed-by. > --- > drivers/net/can/rcar/rcar_canfd.c | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > index ef4e1b9a9e1ee280..6df9a259e5e4f92c 100644 > --- a/drivers/net/can/rcar/rcar_canfd.c > +++ b/drivers/net/can/rcar/rcar_canfd.c > @@ -35,6 +35,7 @@ > #include <linux/netdevice.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/phy/phy.h> > #include <linux/platform_device.h> > #include <linux/reset.h> > #include <linux/types.h> > @@ -530,6 +531,7 @@ struct rcar_canfd_channel { > struct net_device *ndev; > struct rcar_canfd_global *gpriv; /* Controller reference */ > void __iomem *base; /* Register base address */ > + struct phy *transceiver; /* Optional transceiver */ > struct napi_struct napi; > u32 tx_head; /* Incremented on xmit */ > u32 tx_tail; /* Incremented on xmit done */ > @@ -1413,11 +1415,17 @@ static int rcar_canfd_open(struct net_device *ndev) > struct rcar_canfd_global *gpriv = priv->gpriv; > int err; > > + err = phy_power_on(priv->transceiver); > + if (err) { > + netdev_err(ndev, "failed to power on PHY, error %d\n", err); > + return err; > + } > + > /* Peripheral clock is already enabled in probe */ > err = clk_prepare_enable(gpriv->can_clk); > if (err) { > netdev_err(ndev, "failed to enable CAN clock, error %d\n", err); ^^ Nitpick: can you print the mnemotechnic instead of the error value? netdev_err(ndev, "failed to enable CAN clock, error %pe\n", ERR_PTR(err)); > - goto out_clock; > + goto out_phy; > } > > err = open_candev(ndev); > @@ -1437,7 +1445,8 @@ static int rcar_canfd_open(struct net_device *ndev) > close_candev(ndev); > out_can_clock: > clk_disable_unprepare(gpriv->can_clk); > -out_clock: > +out_phy: > + phy_power_off(priv->transceiver); > return err; > } > > @@ -1480,6 +1489,7 @@ static int rcar_canfd_close(struct net_device *ndev) > napi_disable(&priv->napi); > clk_disable_unprepare(gpriv->can_clk); > close_candev(ndev); > + phy_power_off(priv->transceiver); > return 0; > } > > @@ -1711,7 +1721,7 @@ static const struct ethtool_ops rcar_canfd_ethtool_ops = { > }; > > static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > - u32 fcan_freq) > + u32 fcan_freq, struct phy *transceiver) > { > const struct rcar_canfd_hw_info *info = gpriv->info; > struct platform_device *pdev = gpriv->pdev; > @@ -1732,8 +1742,11 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, > ndev->flags |= IFF_ECHO; > priv->ndev = ndev; > priv->base = gpriv->base; > + priv->transceiver = transceiver; > priv->channel = ch; > priv->gpriv = gpriv; > + if (transceiver) > + priv->can.bitrate_max = transceiver->attrs.max_link_rate; > priv->can.clock.freq = fcan_freq; > dev_info(dev, "can_clk rate is %u\n", priv->can.clock.freq); > > @@ -1836,6 +1849,7 @@ static void rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch) > > static int rcar_canfd_probe(struct platform_device *pdev) > { > + struct phy *transceivers[RCANFD_NUM_CHANNELS] = { 0, }; > const struct rcar_canfd_hw_info *info; > struct device *dev = &pdev->dev; > void __iomem *addr; > @@ -1857,9 +1871,14 @@ static int rcar_canfd_probe(struct platform_device *pdev) > for (i = 0; i < info->max_channels; ++i) { > name[7] = '0' + i; > of_child = of_get_child_by_name(dev->of_node, name); > - if (of_child && of_device_is_available(of_child)) > + if (of_child && of_device_is_available(of_child)) { > channels_mask |= BIT(i); > + transceivers[i] = devm_of_phy_optional_get(dev, > + of_child, NULL); > + } > of_node_put(of_child); > + if (IS_ERR(transceivers[i])) > + return PTR_ERR(transceivers[i]); > } > > if (info->shared_global_irqs) { > @@ -2035,7 +2054,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) > } > > for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels) { > - err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq); > + err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq, > + transceivers[ch]); > if (err) > goto fail_channel; > } > -- > 2.34.1 >
Hi Vincent, On Wed, Mar 8, 2023 at 4:55 PM Vincent Mailhol <vincent.mailhol@gmail.com> wrote: > On Wed. 8 Mar. 2023 at 22:20, Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > Add support for CAN transceivers described as PHYs. > > > > While simple CAN transceivers can do without, this is needed for CAN > > transceivers like NXP TJR1443 that need a configuration step (like > > pulling standby or enable lines), and/or impose a bitrate limit. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > I have one nitpick (see below). Aside from that: > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Thanks! > > > --- > > v2: > > - Add Reviewed-by. > > --- > > drivers/net/can/rcar/rcar_canfd.c | 30 +++++++++++++++++++++++++----- > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > > index ef4e1b9a9e1ee280..6df9a259e5e4f92c 100644 > > --- a/drivers/net/can/rcar/rcar_canfd.c > > +++ b/drivers/net/can/rcar/rcar_canfd.c > > @@ -35,6 +35,7 @@ > > #include <linux/netdevice.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > +#include <linux/phy/phy.h> > > #include <linux/platform_device.h> > > #include <linux/reset.h> > > #include <linux/types.h> > > @@ -530,6 +531,7 @@ struct rcar_canfd_channel { > > struct net_device *ndev; > > struct rcar_canfd_global *gpriv; /* Controller reference */ > > void __iomem *base; /* Register base address */ > > + struct phy *transceiver; /* Optional transceiver */ > > struct napi_struct napi; > > u32 tx_head; /* Incremented on xmit */ > > u32 tx_tail; /* Incremented on xmit done */ > > @@ -1413,11 +1415,17 @@ static int rcar_canfd_open(struct net_device *ndev) > > struct rcar_canfd_global *gpriv = priv->gpriv; > > int err; > > > > + err = phy_power_on(priv->transceiver); > > + if (err) { > > + netdev_err(ndev, "failed to power on PHY, error %d\n", err); > > + return err; > > + } > > + > > /* Peripheral clock is already enabled in probe */ > > err = clk_prepare_enable(gpriv->can_clk); > > if (err) { > > netdev_err(ndev, "failed to enable CAN clock, error %d\n", err); > ^^ > > Nitpick: can you print the mnemotechnic instead of the error value? > > netdev_err(ndev, "failed to enable CAN clock, error > %pe\n", ERR_PTR(err)); Thanks for the suggestion! As you're pointing to pre-existing code, and there are several cases like that, I sent a follow-up patch to fix all of them at once: https://lore.kernel.org/r/8a39f99fc28967134826dff141b51a5df824b034.1678349267.git.geert+renesas@glider.be Gr{oetje,eeting}s, Geert
On Tue. 9 Mar 2023 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Vincent, > > On Wed, Mar 8, 2023 at 4:55 PM Vincent Mailhol > <vincent.mailhol@gmail.com> wrote: > > On Wed. 8 Mar. 2023 at 22:20, Geert Uytterhoeven > > <geert+renesas@glider.be> wrote: > > > Add support for CAN transceivers described as PHYs. > > > > > > While simple CAN transceivers can do without, this is needed for CAN > > > transceivers like NXP TJR1443 that need a configuration step (like > > > pulling standby or enable lines), and/or impose a bitrate limit. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > > I have one nitpick (see below). Aside from that: > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > Thanks! > > > > > > --- > > > v2: > > > - Add Reviewed-by. > > > --- > > > drivers/net/can/rcar/rcar_canfd.c | 30 +++++++++++++++++++++++++----- > > > 1 file changed, 25 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c > > > index ef4e1b9a9e1ee280..6df9a259e5e4f92c 100644 > > > --- a/drivers/net/can/rcar/rcar_canfd.c > > > +++ b/drivers/net/can/rcar/rcar_canfd.c > > > @@ -35,6 +35,7 @@ > > > #include <linux/netdevice.h> > > > #include <linux/of.h> > > > #include <linux/of_device.h> > > > +#include <linux/phy/phy.h> > > > #include <linux/platform_device.h> > > > #include <linux/reset.h> > > > #include <linux/types.h> > > > @@ -530,6 +531,7 @@ struct rcar_canfd_channel { > > > struct net_device *ndev; > > > struct rcar_canfd_global *gpriv; /* Controller reference */ > > > void __iomem *base; /* Register base address */ > > > + struct phy *transceiver; /* Optional transceiver */ > > > struct napi_struct napi; > > > u32 tx_head; /* Incremented on xmit */ > > > u32 tx_tail; /* Incremented on xmit done */ > > > @@ -1413,11 +1415,17 @@ static int rcar_canfd_open(struct net_device *ndev) > > > struct rcar_canfd_global *gpriv = priv->gpriv; > > > int err; > > > > > > + err = phy_power_on(priv->transceiver); > > > + if (err) { > > > + netdev_err(ndev, "failed to power on PHY, error %d\n", err); Actually, I wanted to comment on this line… > > > + return err; > > > + } > > > + > > > /* Peripheral clock is already enabled in probe */ > > > err = clk_prepare_enable(gpriv->can_clk); > > > if (err) { > > > netdev_err(ndev, "failed to enable CAN clock, error %d\n", err); > > ^^ > > > > Nitpick: can you print the mnemotechnic instead of the error value? …but instead pointed to that line when writing my comment. > > > > netdev_err(ndev, "failed to enable CAN clock, error > > %pe\n", ERR_PTR(err)); > > Thanks for the suggestion! > > As you're pointing to pre-existing code, and there are several cases > like that, I sent a follow-up patch to fix all of them at once: > https://lore.kernel.org/r/8a39f99fc28967134826dff141b51a5df824b034.1678349267.git.geert+renesas@glider.be Thanks for fixing the pre-existing code as well! My intent was to point to the newly introduced code but inadvertently commented on the old code.
diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c index ef4e1b9a9e1ee280..6df9a259e5e4f92c 100644 --- a/drivers/net/can/rcar/rcar_canfd.c +++ b/drivers/net/can/rcar/rcar_canfd.c @@ -35,6 +35,7 @@ #include <linux/netdevice.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/phy/phy.h> #include <linux/platform_device.h> #include <linux/reset.h> #include <linux/types.h> @@ -530,6 +531,7 @@ struct rcar_canfd_channel { struct net_device *ndev; struct rcar_canfd_global *gpriv; /* Controller reference */ void __iomem *base; /* Register base address */ + struct phy *transceiver; /* Optional transceiver */ struct napi_struct napi; u32 tx_head; /* Incremented on xmit */ u32 tx_tail; /* Incremented on xmit done */ @@ -1413,11 +1415,17 @@ static int rcar_canfd_open(struct net_device *ndev) struct rcar_canfd_global *gpriv = priv->gpriv; int err; + err = phy_power_on(priv->transceiver); + if (err) { + netdev_err(ndev, "failed to power on PHY, error %d\n", err); + return err; + } + /* Peripheral clock is already enabled in probe */ err = clk_prepare_enable(gpriv->can_clk); if (err) { netdev_err(ndev, "failed to enable CAN clock, error %d\n", err); - goto out_clock; + goto out_phy; } err = open_candev(ndev); @@ -1437,7 +1445,8 @@ static int rcar_canfd_open(struct net_device *ndev) close_candev(ndev); out_can_clock: clk_disable_unprepare(gpriv->can_clk); -out_clock: +out_phy: + phy_power_off(priv->transceiver); return err; } @@ -1480,6 +1489,7 @@ static int rcar_canfd_close(struct net_device *ndev) napi_disable(&priv->napi); clk_disable_unprepare(gpriv->can_clk); close_candev(ndev); + phy_power_off(priv->transceiver); return 0; } @@ -1711,7 +1721,7 @@ static const struct ethtool_ops rcar_canfd_ethtool_ops = { }; static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, - u32 fcan_freq) + u32 fcan_freq, struct phy *transceiver) { const struct rcar_canfd_hw_info *info = gpriv->info; struct platform_device *pdev = gpriv->pdev; @@ -1732,8 +1742,11 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch, ndev->flags |= IFF_ECHO; priv->ndev = ndev; priv->base = gpriv->base; + priv->transceiver = transceiver; priv->channel = ch; priv->gpriv = gpriv; + if (transceiver) + priv->can.bitrate_max = transceiver->attrs.max_link_rate; priv->can.clock.freq = fcan_freq; dev_info(dev, "can_clk rate is %u\n", priv->can.clock.freq); @@ -1836,6 +1849,7 @@ static void rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch) static int rcar_canfd_probe(struct platform_device *pdev) { + struct phy *transceivers[RCANFD_NUM_CHANNELS] = { 0, }; const struct rcar_canfd_hw_info *info; struct device *dev = &pdev->dev; void __iomem *addr; @@ -1857,9 +1871,14 @@ static int rcar_canfd_probe(struct platform_device *pdev) for (i = 0; i < info->max_channels; ++i) { name[7] = '0' + i; of_child = of_get_child_by_name(dev->of_node, name); - if (of_child && of_device_is_available(of_child)) + if (of_child && of_device_is_available(of_child)) { channels_mask |= BIT(i); + transceivers[i] = devm_of_phy_optional_get(dev, + of_child, NULL); + } of_node_put(of_child); + if (IS_ERR(transceivers[i])) + return PTR_ERR(transceivers[i]); } if (info->shared_global_irqs) { @@ -2035,7 +2054,8 @@ static int rcar_canfd_probe(struct platform_device *pdev) } for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels) { - err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq); + err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq, + transceivers[ch]); if (err) goto fail_channel; }