RE: [PATCH net-next 2/2] net: axienet: Enable more clocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the patch.

> -----Original Message-----
> From: Robert Hancock <robert.hancock@xxxxxxxxxx>
> Sent: Friday, March 12, 2021 5:21 AM
> To: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; Radhey Shyam Pandey
> <radheys@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next 2/2] net: axienet: Enable more clocks
> 
> On Thu, 2021-03-11 at 14:11 -0600, Robert Hancock wrote:
> > This driver was only enabling the first clock on the device, regardless
> > of its name. However, this controller logic can have multiple clocks
> > which should all be enabled. Add support for enabling additional clocks.
> > The clock names used are matching those used in the Xilinx version of this
> > driver as well as the Xilinx device tree generator, except for mgt_clk
> > which is not present there.
> >
> > For backward compatibility, if no named clocks are present, the first
> > clock present is used for determining the MDIO bus clock divider.
> >
> > Signed-off-by: Robert Hancock <robert.hancock@xxxxxxxxxx>
> >
> 
> Note that this patch is dependent on "net: axienet: Fix probe error cleanup" (
> https://patchwork.kernel.org/project/netdevbpf/patch/20210311200518.38
> 01916-1-robert.hancock@xxxxxxxxxx/
>  ) which is tagged for the net tree.
> 
> > ---
> >  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  8 +++--
> >  .../net/ethernet/xilinx/xilinx_axienet_main.c | 34 +++++++++++++++----
> >  .../net/ethernet/xilinx/xilinx_axienet_mdio.c |  4 +--
> >  3 files changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > index 1e966a39967e..92f7cefb345e 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> > @@ -376,6 +376,8 @@ struct axidma_bd {
> >  	struct sk_buff *skb;
> >  } __aligned(XAXIDMA_BD_MINIMUM_ALIGNMENT);
> >
> > +#define XAE_NUM_MISC_CLOCKS 3
> > +
> >  /**
> >   * struct axienet_local - axienet private per device data
> >   * @ndev:	Pointer for net_device to which it will be attached.
> > @@ -385,7 +387,8 @@ struct axidma_bd {
> >   * @phylink_config: phylink configuration settings
> >   * @pcs_phy:	Reference to PCS/PMA PHY if used
> >   * @switch_x_sgmii: Whether switchable 1000BaseX/SGMII mode is
> enabled in
> > the core
> > - * @clk:	Clock for AXI bus
> > + * @axi_clk:	AXI bus clock
Minor nit - Better to add " AXI4-Lite bus clock" 

> > + * @misc_clks:	Other device clocks

We can mention : Misc ethernet clocks (AXI4-Stream, Ref, MGT clocks)

Apart from it - rest looks fine. Please feel free to add:
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxxxxx>

> >   * @mii_bus:	Pointer to MII bus structure
> >   * @mii_clk_div: MII bus clock divider value
> >   * @regs_start: Resource start for axienet device addresses
> > @@ -434,7 +437,8 @@ struct axienet_local {
> >
> >  	bool switch_x_sgmii;
> >
> > -	struct clk *clk;
> > +	struct clk *axi_clk;
> > +	struct clk_bulk_data misc_clks[XAE_NUM_MISC_CLOCKS];
> >
> >  	struct mii_bus *mii_bus;
> >  	u8 mii_clk_div;
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > index 5d677db0aee5..9635101fbb88 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> > @@ -1863,17 +1863,35 @@ static int axienet_probe(struct
> platform_device
> > *pdev)
> >  	lp->rx_bd_num = RX_BD_NUM_DEFAULT;
> >  	lp->tx_bd_num = TX_BD_NUM_DEFAULT;
> >
> > -	lp->clk = devm_clk_get_optional(&pdev->dev, NULL);
> > -	if (IS_ERR(lp->clk)) {
> > -		ret = PTR_ERR(lp->clk);
> > +	lp->axi_clk = devm_clk_get_optional(&pdev->dev, "s_axi_lite_clk");
> > +	if (!lp->axi_clk) {
> > +		/* For backward compatibility, if named AXI clock is not
> > present,
> > +		 * treat the first clock specified as the AXI clock.
> > +		 */
> > +		lp->axi_clk = devm_clk_get_optional(&pdev->dev, NULL);
> > +	}
> > +	if (IS_ERR(lp->axi_clk)) {
> > +		ret = PTR_ERR(lp->axi_clk);
> >  		goto free_netdev;
> >  	}
> > -	ret = clk_prepare_enable(lp->clk);
> > +	ret = clk_prepare_enable(lp->axi_clk);
> >  	if (ret) {
> > -		dev_err(&pdev->dev, "Unable to enable clock: %d\n", ret);
> > +		dev_err(&pdev->dev, "Unable to enable AXI clock: %d\n",
> ret);
> >  		goto free_netdev;
> >  	}
> >
> > +	lp->misc_clks[0].id = "axis_clk";
> > +	lp->misc_clks[1].id = "ref_clk";
> > +	lp->misc_clks[2].id = "mgt_clk";
> > +
> > +	ret = devm_clk_bulk_get_optional(&pdev->dev,
> XAE_NUM_MISC_CLOCKS, lp-
> > >misc_clks);
> > +	if (ret)
> > +		goto cleanup_clk;
> > +
> > +	ret = clk_bulk_prepare_enable(XAE_NUM_MISC_CLOCKS, lp-
> >misc_clks);
> > +	if (ret)
> > +		goto cleanup_clk;
> > +
> >  	/* Map device registers */
> >  	ethres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	lp->regs = devm_ioremap_resource(&pdev->dev, ethres);
> > @@ -2109,7 +2127,8 @@ static int axienet_probe(struct platform_device
> *pdev)
> >  	of_node_put(lp->phy_node);
> >
> >  cleanup_clk:
> > -	clk_disable_unprepare(lp->clk);
> > +	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
> >misc_clks);
> > +	clk_disable_unprepare(lp->axi_clk);
> >
> >  free_netdev:
> >  	free_netdev(ndev);
> > @@ -2132,7 +2151,8 @@ static int axienet_remove(struct platform_device
> *pdev)
> >
> >  	axienet_mdio_teardown(lp);
> >
> > -	clk_disable_unprepare(lp->clk);
> > +	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp-
> >misc_clks);
> > +	clk_disable_unprepare(lp->axi_clk);
> >
> >  	of_node_put(lp->phy_node);
> >  	lp->phy_node = NULL;
> > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
> > b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
> > index 9c014cee34b2..48f544f6c999 100644
> > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
> > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
> > @@ -159,8 +159,8 @@ int axienet_mdio_enable(struct axienet_local *lp)
> >
> >  	lp->mii_clk_div = 0;
> >
> > -	if (lp->clk) {
> > -		host_clock = clk_get_rate(lp->clk);
> > +	if (lp->axi_clk) {
> > +		host_clock = clk_get_rate(lp->axi_clk);
> >  	} else {
> >  		struct device_node *np1;
> >
> --
> Robert Hancock
> Senior Hardware Designer, Calian Advanced Technologies
> www.calian.com




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux