| Message ID | 1453277183-5412-4-git-send-email-jszhang@marvell.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Dear Jisheng Zhang, On Wed, 20 Jan 2016 16:06:22 +0800, Jisheng Zhang wrote: > Some platforms may provide more than one clk for the mvneta IP, for > example Marvell BG4CT provides "core" clk for the mac core, and > "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to > be enabled. This patch adds this optional "axi" clk support. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> Typo in the title, you have "mmc: ", while this patch is not related to MMC, unless I'm missing something and MMC means something else in this context. > clk_prepare_enable(pp->clk); > > + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); > + if (!IS_ERR(pp->clk_axi)) > + clk_prepare_enable(pp->clk_axi); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pp->base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(pp->base)) { > @@ -3727,6 +3733,7 @@ err_free_ports: > free_percpu(pp->ports); > err_clk: > clk_disable_unprepare(pp->clk); > + clk_disable_unprepare(pp->clk_axi); For the error paths and cleanup steps, I very much prefer when things are done in the opposite order of the allocation/creation steps. So can you clk_disable_unprepare() the AXI clock before the core clock ? > err_put_phy_node: > of_node_put(phy_node); > err_free_irq: > @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) > > unregister_netdev(dev); > clk_disable_unprepare(pp->clk); > + clk_disable_unprepare(pp->clk_axi); Ditto. Thanks! Thomas
On Wed, 20 Jan 2016 09:51:32 +0100 Thomas Petazzoni wrote: > Dear Jisheng Zhang, > > On Wed, 20 Jan 2016 16:06:22 +0800, Jisheng Zhang wrote: > > Some platforms may provide more than one clk for the mvneta IP, for > > example Marvell BG4CT provides "core" clk for the mac core, and > > "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to > > be enabled. This patch adds this optional "axi" clk support. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > Typo in the title, you have "mmc: ", while this patch is not related to > MMC, unless I'm missing something and MMC means something else in this > context. oops, thanks for pointing out this. > > > clk_prepare_enable(pp->clk); > > > > + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); > > + if (!IS_ERR(pp->clk_axi)) > > + clk_prepare_enable(pp->clk_axi); > > + > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > pp->base = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(pp->base)) { > > @@ -3727,6 +3733,7 @@ err_free_ports: > > free_percpu(pp->ports); > > err_clk: > > clk_disable_unprepare(pp->clk); > > + clk_disable_unprepare(pp->clk_axi); > > For the error paths and cleanup steps, I very much prefer when things > are done in the opposite order of the allocation/creation steps. So can > you clk_disable_unprepare() the AXI clock before the core clock ? Both are fine. But I agree with your prefer. Will cook a v2 soon Thanks for reviewing. > > > err_put_phy_node: > > of_node_put(phy_node); > > err_free_irq: > > @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) > > > > unregister_netdev(dev); > > clk_disable_unprepare(pp->clk); > > + clk_disable_unprepare(pp->clk_axi); > > Ditto. > > Thanks! > > Thomas
On January 20, 2016 9:15:22 AM Jisheng Zhang <jszhang@marvell.com> wrote: > Some platforms may provide more than one clk for the mvneta IP, for > example Marvell BG4CT provides "core" clk for the mac core, and > "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to > be enabled. This patch adds this optional "axi" clk support. Jisheng, although I do not expect mvneta to appear on a non-AXI bus anytime soon, how about naming the clock "bus" instead? If you know the clock is only required for bus master DMA but not for register access, "dma" would be an even better name. Sebastian > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > drivers/net/ethernet/marvell/mvneta.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/mvneta.c > b/drivers/net/ethernet/marvell/mvneta.c > index aca0a73..6bb709a 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -373,6 +373,8 @@ struct mvneta_port { > > /* Core clock */ > struct clk *clk; > + /* AXI clock */ > + struct clk *clk_axi; > u8 mcast_count[256]; > u16 tx_ring_size; > u16 rx_ring_size; > @@ -3615,6 +3617,10 @@ static int mvneta_probe(struct platform_device *pdev) > > clk_prepare_enable(pp->clk); > > + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); > + if (!IS_ERR(pp->clk_axi)) > + clk_prepare_enable(pp->clk_axi); > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pp->base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(pp->base)) { > @@ -3727,6 +3733,7 @@ err_free_ports: > free_percpu(pp->ports); > err_clk: > clk_disable_unprepare(pp->clk); > + clk_disable_unprepare(pp->clk_axi); > err_put_phy_node: > of_node_put(phy_node); > err_free_irq: > @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) > > unregister_netdev(dev); > clk_disable_unprepare(pp->clk); > + clk_disable_unprepare(pp->clk_axi); > free_percpu(pp->ports); > free_percpu(pp->stats); > irq_dispose_mapping(dev->irq); > -- > 2.7.0.rc3 >
On Wed, 20 Jan 2016 10:31:18 +0100 Sebastian Hesselbarth wrote: > On January 20, 2016 9:15:22 AM Jisheng Zhang wrote: > > > Some platforms may provide more than one clk for the mvneta IP, for > > example Marvell BG4CT provides "core" clk for the mac core, and > > "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to > > be enabled. This patch adds this optional "axi" clk support. > > Jisheng, > > although I do not expect mvneta to appear on a non-AXI bus > anytime soon, how about naming the clock "bus" instead? Good question. IIRC, this IP expects AXI bus, but I'll check with HW people. Thanks a lot, Jisheng > > If you know the clock is only required for bus master DMA but > not for register access, "dma" would be an even better name. > > Sebastian > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > drivers/net/ethernet/marvell/mvneta.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c > > b/drivers/net/ethernet/marvell/mvneta.c > > index aca0a73..6bb709a 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > @@ -373,6 +373,8 @@ struct mvneta_port { > > > > /* Core clock */ > > struct clk *clk; > > + /* AXI clock */ > > + struct clk *clk_axi; > > u8 mcast_count[256]; > > u16 tx_ring_size; > > u16 rx_ring_size; > > @@ -3615,6 +3617,10 @@ static int mvneta_probe(struct platform_device *pdev) > > > > clk_prepare_enable(pp->clk); > > > > + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); > > + if (!IS_ERR(pp->clk_axi)) > > + clk_prepare_enable(pp->clk_axi); > > + > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > pp->base = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(pp->base)) { > > @@ -3727,6 +3733,7 @@ err_free_ports: > > free_percpu(pp->ports); > > err_clk: > > clk_disable_unprepare(pp->clk); > > + clk_disable_unprepare(pp->clk_axi); > > err_put_phy_node: > > of_node_put(phy_node); > > err_free_irq: > > @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) > > > > unregister_netdev(dev); > > clk_disable_unprepare(pp->clk); > > + clk_disable_unprepare(pp->clk_axi); > > free_percpu(pp->ports); > > free_percpu(pp->stats); > > irq_dispose_mapping(dev->irq); > > -- > > 2.7.0.rc3 > > > >
On 01/20/2016 10:42 AM, Jisheng Zhang wrote: > On Wed, 20 Jan 2016 10:31:18 +0100 Sebastian Hesselbarth wrote: > >> On January 20, 2016 9:15:22 AM Jisheng Zhang wrote: >> >>> Some platforms may provide more than one clk for the mvneta IP, for >>> example Marvell BG4CT provides "core" clk for the mac core, and >>> "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to >>> be enabled. This patch adds this optional "axi" clk support. >> >> Jisheng, >> >> although I do not expect mvneta to appear on a non-AXI bus >> anytime soon, how about naming the clock "bus" instead? > > Good question. IIRC, this IP expects AXI bus, but I'll check with HW people. Actually, I am quite sure the current IP requires AXI. But my comment was more about to make the binding a little bit more flexible to _future_ variants/SoCs we may stumble upon. Naming the clock "bus" or "dma" will work for the current _and_ future IPs, while "axi" may not. Sebastian >> >> If you know the clock is only required for bus master DMA but >> not for register access, "dma" would be an even better name. >> >> Sebastian >> >> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>> --- >>> drivers/net/ethernet/marvell/mvneta.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/marvell/mvneta.c >>> b/drivers/net/ethernet/marvell/mvneta.c >>> index aca0a73..6bb709a 100644 >>> --- a/drivers/net/ethernet/marvell/mvneta.c >>> +++ b/drivers/net/ethernet/marvell/mvneta.c >>> @@ -373,6 +373,8 @@ struct mvneta_port { >>> >>> /* Core clock */ >>> struct clk *clk; >>> + /* AXI clock */ >>> + struct clk *clk_axi; >>> u8 mcast_count[256]; >>> u16 tx_ring_size; >>> u16 rx_ring_size; >>> @@ -3615,6 +3617,10 @@ static int mvneta_probe(struct platform_device *pdev) >>> >>> clk_prepare_enable(pp->clk); >>> >>> + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); >>> + if (!IS_ERR(pp->clk_axi)) >>> + clk_prepare_enable(pp->clk_axi); >>> + >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> pp->base = devm_ioremap_resource(&pdev->dev, res); >>> if (IS_ERR(pp->base)) { >>> @@ -3727,6 +3733,7 @@ err_free_ports: >>> free_percpu(pp->ports); >>> err_clk: >>> clk_disable_unprepare(pp->clk); >>> + clk_disable_unprepare(pp->clk_axi); >>> err_put_phy_node: >>> of_node_put(phy_node); >>> err_free_irq: >>> @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) >>> >>> unregister_netdev(dev); >>> clk_disable_unprepare(pp->clk); >>> + clk_disable_unprepare(pp->clk_axi); >>> free_percpu(pp->ports); >>> free_percpu(pp->stats); >>> irq_dispose_mapping(dev->irq); >>> -- >>> 2.7.0.rc3 >>> >> >> >
Dear Sebastian, On Wed, 20 Jan 2016 12:03:03 +0100 Sebastian Hesselbarth wrote: > On 01/20/2016 10:42 AM, Jisheng Zhang wrote: > > On Wed, 20 Jan 2016 10:31:18 +0100 Sebastian Hesselbarth wrote: > > > >> On January 20, 2016 9:15:22 AM Jisheng Zhang wrote: > >> > >>> Some platforms may provide more than one clk for the mvneta IP, for > >>> example Marvell BG4CT provides "core" clk for the mac core, and > >>> "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to > >>> be enabled. This patch adds this optional "axi" clk support. > >> > >> Jisheng, > >> > >> although I do not expect mvneta to appear on a non-AXI bus > >> anytime soon, how about naming the clock "bus" instead? > > > > Good question. IIRC, this IP expects AXI bus, but I'll check with HW people. > > Actually, I am quite sure the current IP requires AXI. But my comment > was more about to make the binding a little bit more flexible to > _future_ variants/SoCs we may stumble upon. Got your points. PS: the clk is for AXI bus logic, so "bus" makes sense. > > Naming the clock "bus" or "dma" will work for the current _and_ future > IPs, while "axi" may not. Indeed, will cook a v3 Thanks a lot for review, Jisheng > > Sebastian > > >> > >> If you know the clock is only required for bus master DMA but > >> not for register access, "dma" would be an even better name. > >> > >> Sebastian > >> > >> > >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>> --- > >>> drivers/net/ethernet/marvell/mvneta.c | 8 ++++++++ > >>> 1 file changed, 8 insertions(+) > >>> > >>> diff --git a/drivers/net/ethernet/marvell/mvneta.c > >>> b/drivers/net/ethernet/marvell/mvneta.c > >>> index aca0a73..6bb709a 100644 > >>> --- a/drivers/net/ethernet/marvell/mvneta.c > >>> +++ b/drivers/net/ethernet/marvell/mvneta.c > >>> @@ -373,6 +373,8 @@ struct mvneta_port { > >>> > >>> /* Core clock */ > >>> struct clk *clk; > >>> + /* AXI clock */ > >>> + struct clk *clk_axi; > >>> u8 mcast_count[256]; > >>> u16 tx_ring_size; > >>> u16 rx_ring_size; > >>> @@ -3615,6 +3617,10 @@ static int mvneta_probe(struct platform_device *pdev) > >>> > >>> clk_prepare_enable(pp->clk); > >>> > >>> + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); > >>> + if (!IS_ERR(pp->clk_axi)) > >>> + clk_prepare_enable(pp->clk_axi); > >>> + > >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> pp->base = devm_ioremap_resource(&pdev->dev, res); > >>> if (IS_ERR(pp->base)) { > >>> @@ -3727,6 +3733,7 @@ err_free_ports: > >>> free_percpu(pp->ports); > >>> err_clk: > >>> clk_disable_unprepare(pp->clk); > >>> + clk_disable_unprepare(pp->clk_axi); > >>> err_put_phy_node: > >>> of_node_put(phy_node); > >>> err_free_irq: > >>> @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) > >>> > >>> unregister_netdev(dev); > >>> clk_disable_unprepare(pp->clk); > >>> + clk_disable_unprepare(pp->clk_axi); > >>> free_percpu(pp->ports); > >>> free_percpu(pp->stats); > >>> irq_dispose_mapping(dev->irq); > >>> -- > >>> 2.7.0.rc3 > >>> > >> > >> > >
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index aca0a73..6bb709a 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -373,6 +373,8 @@ struct mvneta_port { /* Core clock */ struct clk *clk; + /* AXI clock */ + struct clk *clk_axi; u8 mcast_count[256]; u16 tx_ring_size; u16 rx_ring_size; @@ -3615,6 +3617,10 @@ static int mvneta_probe(struct platform_device *pdev) clk_prepare_enable(pp->clk); + pp->clk_axi = devm_clk_get(&pdev->dev, "axi"); + if (!IS_ERR(pp->clk_axi)) + clk_prepare_enable(pp->clk_axi); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); pp->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(pp->base)) { @@ -3727,6 +3733,7 @@ err_free_ports: free_percpu(pp->ports); err_clk: clk_disable_unprepare(pp->clk); + clk_disable_unprepare(pp->clk_axi); err_put_phy_node: of_node_put(phy_node); err_free_irq: @@ -3744,6 +3751,7 @@ static int mvneta_remove(struct platform_device *pdev) unregister_netdev(dev); clk_disable_unprepare(pp->clk); + clk_disable_unprepare(pp->clk_axi); free_percpu(pp->ports); free_percpu(pp->stats); irq_dispose_mapping(dev->irq);
Some platforms may provide more than one clk for the mvneta IP, for example Marvell BG4CT provides "core" clk for the mac core, and "axi" clk for the AXI bus logic. Obviously this "axi" clk also need to be enabled. This patch adds this optional "axi" clk support. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- drivers/net/ethernet/marvell/mvneta.c | 8 ++++++++ 1 file changed, 8 insertions(+)