Our patch to add support for the FU540-C000 broke compilation on at
least powerpc allyesconfig, which was found as part of the linux-next
build regression tests. This must have somehow slipped through the
cracks, as the patch has been reverted in linux-next for a while now. This
patch applies on top of the offending commit, which is the only one I've even
tried it on as I'm not sure how this subsystem makes it to Linus.
This patch set fixes the issue by adding another Kconfig entry to
conditionally enable the FU540-C000 support. It would be less code to
just make MACB depend on COMMON_CLK, but I'm not sure if that dependency
would cause trouble for anyone so I didn't do it that way. I'm happy to
re-spin the patch to add the dependency, but as it's a very small change
I'm also happy if you do it yourself :).
I've also included a second patch to indicate this is a Cadence driver,
not an Atmel driver. As far as I know the controller is from Cadence,
but it looks like maybe it showed up first on some Atmel chips. I'm
fine either way, so feel free to just drop it if you think the old name
is better. The only relation is that I stumbled across it when writing the
first patch.
The patch to add support for the FU540-C000 added a dependency on
COMMON_CLK, but didn't express that via Kconfig. This fixes the build
failure by adding CONFIG_MACB_FU540, which depends on COMMON_CLK and
conditionally enables the FU540-C000 support.
I've built this with a powerpc allyesconfig (which pointed out the bug)
and on RISC-V, manually checking to ensure the code was built. I
haven't even booted the resulting kernels.
Fixes: c218ad559020 ("macb: Add support for SiFive FU540-C000")
Signed-off-by: Palmer Dabbelt <[email protected]>
---
drivers/net/ethernet/cadence/Kconfig | 11 +++++++++++
drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++++
2 files changed, 23 insertions(+)
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 1766697c9c5a..74ee2bfd2369 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -40,6 +40,17 @@ config MACB_USE_HWSTAMP
---help---
Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
+config MACB_FU540
+ bool "Enable support for the SiFive FU540 clock controller"
+ depends on MACB && COMMON_CLK
+ default y
+ ---help---
+ Enable support for the MACB/GEM clock controller on the SiFive
+ FU540-C000. This device is necessary for switching between 10/100
+ and gigabit modes on the FU540-C000 SoC, without which it is only
+ possible to bring up the Ethernet link in whatever mode the
+ bootloader probed.
+
config MACB_PCI
tristate "Cadence PCI MACB/GEM support"
depends on MACB && PCI && COMMON_CLK
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c545c5b435d8..a903dfdd4183 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -41,6 +41,7 @@
#include <linux/pm_runtime.h>
#include "macb.h"
+#ifdef CONFIG_MACB_FU540
/* This structure is only used for MACB on SiFive FU540 devices */
struct sifive_fu540_macb_mgmt {
void __iomem *reg;
@@ -49,6 +50,7 @@ struct sifive_fu540_macb_mgmt {
};
static struct sifive_fu540_macb_mgmt *mgmt;
+#endif
#define MACB_RX_BUFFER_SIZE 128
#define RX_BUFFER_MULTIPLE 64 /* bytes */
@@ -3956,6 +3958,7 @@ static int at91ether_init(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_MACB_FU540
static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
{
@@ -4056,7 +4059,9 @@ static int fu540_c000_init(struct platform_device *pdev)
return macb_init(pdev);
}
+#endif
+#ifdef CONFIG_MACB_FU540
static const struct macb_config fu540_c000_config = {
.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
MACB_CAPS_GEM_HAS_PTP,
@@ -4065,6 +4070,7 @@ static const struct macb_config fu540_c000_config = {
.init = fu540_c000_init,
.jumbo_max_len = 10240,
};
+#endif
static const struct macb_config at91sam9260_config = {
.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
@@ -4155,7 +4161,9 @@ static const struct of_device_id macb_dt_ids[] = {
{ .compatible = "cdns,emac", .data = &emac_config },
{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
{ .compatible = "cdns,zynq-gem", .data = &zynq_config },
+#ifdef CONFIG_MACB_FU540
{ .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
+#endif
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, macb_dt_ids);
@@ -4363,7 +4371,9 @@ static int macb_probe(struct platform_device *pdev)
err_disable_clocks:
clk_disable_unprepare(tx_clk);
+#ifdef CONFIG_MACB_FU540
clk_unregister(tx_clk);
+#endif
clk_disable_unprepare(hclk);
clk_disable_unprepare(pclk);
clk_disable_unprepare(rx_clk);
@@ -4398,7 +4408,9 @@ static int macb_remove(struct platform_device *pdev)
pm_runtime_dont_use_autosuspend(&pdev->dev);
if (!pm_runtime_suspended(&pdev->dev)) {
clk_disable_unprepare(bp->tx_clk);
+#ifdef CONFIG_MACB_FU540
clk_unregister(bp->tx_clk);
+#endif
clk_disable_unprepare(bp->hclk);
clk_disable_unprepare(bp->pclk);
clk_disable_unprepare(bp->rx_clk);
--
2.21.0
When touching the Kconfig for this driver I noticed that both the
Kconfig help text and a comment referred to this being an Atmel driver.
As far as I know, this is a Cadence driver. The fix is just
s/Atmel/Cadence/, but I did go and re-wrap the Kconfig help text as that
change caused it to go over 80 characters.
Signed-off-by: Palmer Dabbelt <[email protected]>
---
drivers/net/ethernet/cadence/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 74ee2bfd2369..29b6132b418e 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
#
-# Atmel device configuration
+# Cadence device configuration
#
config NET_VENDOR_CADENCE
@@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
If unsure, say Y.
Note that the answer to this question doesn't directly affect the
- kernel: saying N will just cause the configurator to skip all
- the remaining Atmel network card questions. If you say Y, you will be
+ kernel: saying N will just cause the configurator to skip all the
+ remaining Cadence network card questions. If you say Y, you will be
asked for your specific card in the following questions.
if NET_VENDOR_CADENCE
--
2.21.0
On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
> External E-Mail
>
>
> The patch to add support for the FU540-C000 added a dependency on
> COMMON_CLK, but didn't express that via Kconfig. This fixes the build
> failure by adding CONFIG_MACB_FU540, which depends on COMMON_CLK and
> conditionally enables the FU540-C000 support.
Let's try to limit the use of #ifdef's throughout the code. We are
using them in this driver but only for the hot paths and things that
have an impact on performance. I don't think it's the case here: so
please find another option => NACK.
> I've built this with a powerpc allyesconfig (which pointed out the bug)
> and on RISC-V, manually checking to ensure the code was built. I
> haven't even booted the resulting kernels.
>
> Fixes: c218ad559020 ("macb: Add support for SiFive FU540-C000")
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> drivers/net/ethernet/cadence/Kconfig | 11 +++++++++++
> drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 1766697c9c5a..74ee2bfd2369 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -40,6 +40,17 @@ config MACB_USE_HWSTAMP
> ---help---
> Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
>
> +config MACB_FU540
> + bool "Enable support for the SiFive FU540 clock controller"
> + depends on MACB && COMMON_CLK
> + default y
> + ---help---
> + Enable support for the MACB/GEM clock controller on the SiFive
> + FU540-C000. This device is necessary for switching between 10/100
> + and gigabit modes on the FU540-C000 SoC, without which it is only
> + possible to bring up the Ethernet link in whatever mode the
> + bootloader probed.
> +
> config MACB_PCI
> tristate "Cadence PCI MACB/GEM support"
> depends on MACB && PCI && COMMON_CLK
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c545c5b435d8..a903dfdd4183 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -41,6 +41,7 @@
> #include <linux/pm_runtime.h>
> #include "macb.h"
>
> +#ifdef CONFIG_MACB_FU540
> /* This structure is only used for MACB on SiFive FU540 devices */
> struct sifive_fu540_macb_mgmt {
> void __iomem *reg;
> @@ -49,6 +50,7 @@ struct sifive_fu540_macb_mgmt {
> };
>
> static struct sifive_fu540_macb_mgmt *mgmt;
> +#endif
>
> #define MACB_RX_BUFFER_SIZE 128
> #define RX_BUFFER_MULTIPLE 64 /* bytes */
> @@ -3956,6 +3958,7 @@ static int at91ether_init(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_MACB_FU540
> static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
> unsigned long parent_rate)
> {
> @@ -4056,7 +4059,9 @@ static int fu540_c000_init(struct platform_device *pdev)
>
> return macb_init(pdev);
> }
> +#endif
>
> +#ifdef CONFIG_MACB_FU540
> static const struct macb_config fu540_c000_config = {
> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> MACB_CAPS_GEM_HAS_PTP,
> @@ -4065,6 +4070,7 @@ static const struct macb_config fu540_c000_config = {
> .init = fu540_c000_init,
> .jumbo_max_len = 10240,
> };
> +#endif
>
> static const struct macb_config at91sam9260_config = {
> .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
> @@ -4155,7 +4161,9 @@ static const struct of_device_id macb_dt_ids[] = {
> { .compatible = "cdns,emac", .data = &emac_config },
> { .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
> { .compatible = "cdns,zynq-gem", .data = &zynq_config },
> +#ifdef CONFIG_MACB_FU540
> { .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
> +#endif
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, macb_dt_ids);
> @@ -4363,7 +4371,9 @@ static int macb_probe(struct platform_device *pdev)
>
> err_disable_clocks:
> clk_disable_unprepare(tx_clk);
> +#ifdef CONFIG_MACB_FU540
> clk_unregister(tx_clk);
> +#endif
> clk_disable_unprepare(hclk);
> clk_disable_unprepare(pclk);
> clk_disable_unprepare(rx_clk);
> @@ -4398,7 +4408,9 @@ static int macb_remove(struct platform_device *pdev)
> pm_runtime_dont_use_autosuspend(&pdev->dev);
> if (!pm_runtime_suspended(&pdev->dev)) {
> clk_disable_unprepare(bp->tx_clk);
> +#ifdef CONFIG_MACB_FU540
> clk_unregister(bp->tx_clk);
> +#endif
> clk_disable_unprepare(bp->hclk);
> clk_disable_unprepare(bp->pclk);
> clk_disable_unprepare(bp->rx_clk);
>
--
Nicolas Ferre
On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
> External E-Mail
>
>
> When touching the Kconfig for this driver I noticed that both the
> Kconfig help text and a comment referred to this being an Atmel driver.
> As far as I know, this is a Cadence driver. The fix is just
Indeed: was written and then maintained by Atmel (now Microchip) for
years... So I would say that more than a "Cadence driver" it's a driver
that applies to a Cadence peripheral.
I won't hold the patch just for this as the patch makes perfect sense,
but would love that it's been highlighted...
> s/Atmel/Cadence/, but I did go and re-wrap the Kconfig help text as that
> change caused it to go over 80 characters.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> drivers/net/ethernet/cadence/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 74ee2bfd2369..29b6132b418e 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> #
> -# Atmel device configuration
> +# Cadence device configuration
> #
>
> config NET_VENDOR_CADENCE
> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
> If unsure, say Y.
>
> Note that the answer to this question doesn't directly affect the
> - kernel: saying N will just cause the configurator to skip all
> - the remaining Atmel network card questions. If you say Y, you will be
> + kernel: saying N will just cause the configurator to skip all the
> + remaining Cadence network card questions. If you say Y, you will be
> asked for your specific card in the following questions.
>
> if NET_VENDOR_CADENCE
>
--
Nicolas Ferre
On Mon, 24 Jun 2019 02:49:16 PDT (-0700), [email protected] wrote:
> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
>> External E-Mail
>>
>>
>> When touching the Kconfig for this driver I noticed that both the
>> Kconfig help text and a comment referred to this being an Atmel driver.
>> As far as I know, this is a Cadence driver. The fix is just
>
> Indeed: was written and then maintained by Atmel (now Microchip) for
> years... So I would say that more than a "Cadence driver" it's a driver
> that applies to a Cadence peripheral.
>
> I won't hold the patch just for this as the patch makes perfect sense,
> but would love that it's been highlighted...
OK, I don't mind changing it. Does this look OK? I have to submit a v2 anyway
for the first patch.
Author: Palmer Dabbelt <[email protected]>
Date: Sun Jun 23 23:04:14 2019 -0700
net: macb: Kconfig: Rename Atmel to Cadence
The help text makes it look like NET_VENDOR_CADENCE enables support for
Atmel devices, when in reality it's a driver written by Atmel that
supports Cadence devices. This may confuse users that have this device
on a non-Atmel SoC.
The fix is just s/Atmel/Cadence/, but I did go and re-wrap the Kconfig
help text as that change caused it to go over 80 characters.
Signed-off-by: Palmer Dabbelt <[email protected]>
diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index 74ee2bfd2369..29b6132b418e 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
#
-# Atmel device configuration
+# Cadence device configuration
#
config NET_VENDOR_CADENCE
@@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
If unsure, say Y.
Note that the answer to this question doesn't directly affect the
- kernel: saying N will just cause the configurator to skip all
- the remaining Atmel network card questions. If you say Y, you will be
+ kernel: saying N will just cause the configurator to skip all the
+ remaining Cadence network card questions. If you say Y, you will be
asked for your specific card in the following questions.
if NET_VENDOR_CADENCE
>
>> s/Atmel/Cadence/, but I did go and re-wrap the Kconfig help text as that
>> change caused it to go over 80 characters.
>>
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>> ---
>> drivers/net/ethernet/cadence/Kconfig | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
>> index 74ee2bfd2369..29b6132b418e 100644
>> --- a/drivers/net/ethernet/cadence/Kconfig
>> +++ b/drivers/net/ethernet/cadence/Kconfig
>> @@ -1,6 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> #
>> -# Atmel device configuration
>> +# Cadence device configuration
>> #
>>
>> config NET_VENDOR_CADENCE
>> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
>> If unsure, say Y.
>>
>> Note that the answer to this question doesn't directly affect the
>> - kernel: saying N will just cause the configurator to skip all
>> - the remaining Atmel network card questions. If you say Y, you will be
>> + kernel: saying N will just cause the configurator to skip all the
>> + remaining Cadence network card questions. If you say Y, you will be
>> asked for your specific card in the following questions.
>>
>> if NET_VENDOR_CADENCE
>>
>
>
> --
> Nicolas Ferre
On Mon, 24 Jun 2019 02:40:21 PDT (-0700), [email protected] wrote:
> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
>> External E-Mail
>>
>>
>> The patch to add support for the FU540-C000 added a dependency on
>> COMMON_CLK, but didn't express that via Kconfig. This fixes the build
>> failure by adding CONFIG_MACB_FU540, which depends on COMMON_CLK and
>> conditionally enables the FU540-C000 support.
>
> Let's try to limit the use of #ifdef's throughout the code. We are
> using them in this driver but only for the hot paths and things that
> have an impact on performance. I don't think it's the case here: so
> please find another option => NACK.
OK. Would you accept adding a Kconfig dependency of the generic MACB driver on
COMMON_CLK, as suggested in the cover letter?
>
>> I've built this with a powerpc allyesconfig (which pointed out the bug)
>> and on RISC-V, manually checking to ensure the code was built. I
>> haven't even booted the resulting kernels.
>>
>> Fixes: c218ad559020 ("macb: Add support for SiFive FU540-C000")
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>> ---
>> drivers/net/ethernet/cadence/Kconfig | 11 +++++++++++
>> drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
>> index 1766697c9c5a..74ee2bfd2369 100644
>> --- a/drivers/net/ethernet/cadence/Kconfig
>> +++ b/drivers/net/ethernet/cadence/Kconfig
>> @@ -40,6 +40,17 @@ config MACB_USE_HWSTAMP
>> ---help---
>> Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
>>
>> +config MACB_FU540
>> + bool "Enable support for the SiFive FU540 clock controller"
>> + depends on MACB && COMMON_CLK
>> + default y
>> + ---help---
>> + Enable support for the MACB/GEM clock controller on the SiFive
>> + FU540-C000. This device is necessary for switching between 10/100
>> + and gigabit modes on the FU540-C000 SoC, without which it is only
>> + possible to bring up the Ethernet link in whatever mode the
>> + bootloader probed.
>> +
>> config MACB_PCI
>> tristate "Cadence PCI MACB/GEM support"
>> depends on MACB && PCI && COMMON_CLK
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index c545c5b435d8..a903dfdd4183 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -41,6 +41,7 @@
>> #include <linux/pm_runtime.h>
>> #include "macb.h"
>>
>> +#ifdef CONFIG_MACB_FU540
>> /* This structure is only used for MACB on SiFive FU540 devices */
>> struct sifive_fu540_macb_mgmt {
>> void __iomem *reg;
>> @@ -49,6 +50,7 @@ struct sifive_fu540_macb_mgmt {
>> };
>>
>> static struct sifive_fu540_macb_mgmt *mgmt;
>> +#endif
>>
>> #define MACB_RX_BUFFER_SIZE 128
>> #define RX_BUFFER_MULTIPLE 64 /* bytes */
>> @@ -3956,6 +3958,7 @@ static int at91ether_init(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_MACB_FU540
>> static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
>> unsigned long parent_rate)
>> {
>> @@ -4056,7 +4059,9 @@ static int fu540_c000_init(struct platform_device *pdev)
>>
>> return macb_init(pdev);
>> }
>> +#endif
>>
>> +#ifdef CONFIG_MACB_FU540
>> static const struct macb_config fu540_c000_config = {
>> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>> MACB_CAPS_GEM_HAS_PTP,
>> @@ -4065,6 +4070,7 @@ static const struct macb_config fu540_c000_config = {
>> .init = fu540_c000_init,
>> .jumbo_max_len = 10240,
>> };
>> +#endif
>>
>> static const struct macb_config at91sam9260_config = {
>> .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
>> @@ -4155,7 +4161,9 @@ static const struct of_device_id macb_dt_ids[] = {
>> { .compatible = "cdns,emac", .data = &emac_config },
>> { .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
>> { .compatible = "cdns,zynq-gem", .data = &zynq_config },
>> +#ifdef CONFIG_MACB_FU540
>> { .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
>> +#endif
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, macb_dt_ids);
>> @@ -4363,7 +4371,9 @@ static int macb_probe(struct platform_device *pdev)
>>
>> err_disable_clocks:
>> clk_disable_unprepare(tx_clk);
>> +#ifdef CONFIG_MACB_FU540
>> clk_unregister(tx_clk);
>> +#endif
>> clk_disable_unprepare(hclk);
>> clk_disable_unprepare(pclk);
>> clk_disable_unprepare(rx_clk);
>> @@ -4398,7 +4408,9 @@ static int macb_remove(struct platform_device *pdev)
>> pm_runtime_dont_use_autosuspend(&pdev->dev);
>> if (!pm_runtime_suspended(&pdev->dev)) {
>> clk_disable_unprepare(bp->tx_clk);
>> +#ifdef CONFIG_MACB_FU540
>> clk_unregister(bp->tx_clk);
>> +#endif
>> clk_disable_unprepare(bp->hclk);
>> clk_disable_unprepare(bp->pclk);
>> clk_disable_unprepare(bp->rx_clk);
>>
>
>
> --
> Nicolas Ferre
On 24/06/2019 at 11:57, Palmer Dabbelt wrote:
> External E-Mail
>
>
> On Mon, 24 Jun 2019 02:40:21 PDT (-0700), [email protected] wrote:
>> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
>>> External E-Mail
>>>
>>>
>>> The patch to add support for the FU540-C000 added a dependency on
>>> COMMON_CLK, but didn't express that via Kconfig. This fixes the build
>>> failure by adding CONFIG_MACB_FU540, which depends on COMMON_CLK and
>>> conditionally enables the FU540-C000 support.
>>
>> Let's try to limit the use of #ifdef's throughout the code. We are
>> using them in this driver but only for the hot paths and things that
>> have an impact on performance. I don't think it's the case here: so
>> please find another option => NACK.
>
> OK. Would you accept adding a Kconfig dependency of the generic MACB driver on
> COMMON_CLK, as suggested in the cover letter?
Yes: all users of this peripheral have COMMON_CLK set.
You can remove it from the PCI wrapper then.
Best regards,
Nicolas
>>> I've built this with a powerpc allyesconfig (which pointed out the bug)
>>> and on RISC-V, manually checking to ensure the code was built. I
>>> haven't even booted the resulting kernels.
>>>
>>> Fixes: c218ad559020 ("macb: Add support for SiFive FU540-C000")
>>> Signed-off-by: Palmer Dabbelt <[email protected]>
>>> ---
>>> drivers/net/ethernet/cadence/Kconfig | 11 +++++++++++
>>> drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++++
>>> 2 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
>>> index 1766697c9c5a..74ee2bfd2369 100644
>>> --- a/drivers/net/ethernet/cadence/Kconfig
>>> +++ b/drivers/net/ethernet/cadence/Kconfig
>>> @@ -40,6 +40,17 @@ config MACB_USE_HWSTAMP
>>> ---help---
>>> Enable IEEE 1588 Precision Time Protocol (PTP) support for MACB.
>>>
>>> +config MACB_FU540
>>> + bool "Enable support for the SiFive FU540 clock controller"
>>> + depends on MACB && COMMON_CLK
>>> + default y
>>> + ---help---
>>> + Enable support for the MACB/GEM clock controller on the SiFive
>>> + FU540-C000. This device is necessary for switching between 10/100
>>> + and gigabit modes on the FU540-C000 SoC, without which it is only
>>> + possible to bring up the Ethernet link in whatever mode the
>>> + bootloader probed.
>>> +
>>> config MACB_PCI
>>> tristate "Cadence PCI MACB/GEM support"
>>> depends on MACB && PCI && COMMON_CLK
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index c545c5b435d8..a903dfdd4183 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -41,6 +41,7 @@
>>> #include <linux/pm_runtime.h>
>>> #include "macb.h"
>>>
>>> +#ifdef CONFIG_MACB_FU540
>>> /* This structure is only used for MACB on SiFive FU540 devices */
>>> struct sifive_fu540_macb_mgmt {
>>> void __iomem *reg;
>>> @@ -49,6 +50,7 @@ struct sifive_fu540_macb_mgmt {
>>> };
>>>
>>> static struct sifive_fu540_macb_mgmt *mgmt;
>>> +#endif
>>>
>>> #define MACB_RX_BUFFER_SIZE 128
>>> #define RX_BUFFER_MULTIPLE 64 /* bytes */
>>> @@ -3956,6 +3958,7 @@ static int at91ether_init(struct platform_device *pdev)
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_MACB_FU540
>>> static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
>>> unsigned long parent_rate)
>>> {
>>> @@ -4056,7 +4059,9 @@ static int fu540_c000_init(struct platform_device *pdev)
>>>
>>> return macb_init(pdev);
>>> }
>>> +#endif
>>>
>>> +#ifdef CONFIG_MACB_FU540
>>> static const struct macb_config fu540_c000_config = {
>>> .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>>> MACB_CAPS_GEM_HAS_PTP,
>>> @@ -4065,6 +4070,7 @@ static const struct macb_config fu540_c000_config = {
>>> .init = fu540_c000_init,
>>> .jumbo_max_len = 10240,
>>> };
>>> +#endif
>>>
>>> static const struct macb_config at91sam9260_config = {
>>> .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
>>> @@ -4155,7 +4161,9 @@ static const struct of_device_id macb_dt_ids[] = {
>>> { .compatible = "cdns,emac", .data = &emac_config },
>>> { .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
>>> { .compatible = "cdns,zynq-gem", .data = &zynq_config },
>>> +#ifdef CONFIG_MACB_FU540
>>> { .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
>>> +#endif
>>> { /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, macb_dt_ids);
>>> @@ -4363,7 +4371,9 @@ static int macb_probe(struct platform_device *pdev)
>>>
>>> err_disable_clocks:
>>> clk_disable_unprepare(tx_clk);
>>> +#ifdef CONFIG_MACB_FU540
>>> clk_unregister(tx_clk);
>>> +#endif
>>> clk_disable_unprepare(hclk);
>>> clk_disable_unprepare(pclk);
>>> clk_disable_unprepare(rx_clk);
>>> @@ -4398,7 +4408,9 @@ static int macb_remove(struct platform_device *pdev)
>>> pm_runtime_dont_use_autosuspend(&pdev->dev);
>>> if (!pm_runtime_suspended(&pdev->dev)) {
>>> clk_disable_unprepare(bp->tx_clk);
>>> +#ifdef CONFIG_MACB_FU540
>>> clk_unregister(bp->tx_clk);
>>> +#endif
>>> clk_disable_unprepare(bp->hclk);
>>> clk_disable_unprepare(bp->pclk);
>>> clk_disable_unprepare(bp->rx_clk);
>>>
>>
>>
>> --
>> Nicolas Ferre
--
Nicolas Ferre
On 24/06/2019 at 11:57, Palmer Dabbelt wrote:
> External E-Mail
>
>
> On Mon, 24 Jun 2019 02:49:16 PDT (-0700), [email protected] wrote:
>> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
>>> External E-Mail
>>>
>>>
>>> When touching the Kconfig for this driver I noticed that both the
>>> Kconfig help text and a comment referred to this being an Atmel driver.
>>> As far as I know, this is a Cadence driver. The fix is just
>>
>> Indeed: was written and then maintained by Atmel (now Microchip) for
>> years... So I would say that more than a "Cadence driver" it's a driver
>> that applies to a Cadence peripheral.
>>
>> I won't hold the patch just for this as the patch makes perfect sense,
>> but would love that it's been highlighted...
>
> OK, I don't mind changing it. Does this look OK? I have to submit a v2 anyway
> for the first patch.
Yep, nice!
Thanks,
Nicolas
>
> Author: Palmer Dabbelt <[email protected]>
> Date: Sun Jun 23 23:04:14 2019 -0700
>
> net: macb: Kconfig: Rename Atmel to Cadence
>
> The help text makes it look like NET_VENDOR_CADENCE enables support for
> Atmel devices, when in reality it's a driver written by Atmel that
> supports Cadence devices. This may confuse users that have this device
> on a non-Atmel SoC.
>
> The fix is just s/Atmel/Cadence/, but I did go and re-wrap the Kconfig
> help text as that change caused it to go over 80 characters.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
>
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 74ee2bfd2369..29b6132b418e 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> #
> -# Atmel device configuration
> +# Cadence device configuration
> #
>
> config NET_VENDOR_CADENCE
> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
> If unsure, say Y.
>
> Note that the answer to this question doesn't directly affect the
> - kernel: saying N will just cause the configurator to skip all
> - the remaining Atmel network card questions. If you say Y, you will be
> + kernel: saying N will just cause the configurator to skip all the
> + remaining Cadence network card questions. If you say Y, you will be
> asked for your specific card in the following questions.
>
> if NET_VENDOR_CADENCE
>
>>
>>> s/Atmel/Cadence/, but I did go and re-wrap the Kconfig help text as that
>>> change caused it to go over 80 characters.
>>>
>>> Signed-off-by: Palmer Dabbelt <[email protected]>
>>> ---
>>> drivers/net/ethernet/cadence/Kconfig | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
>>> index 74ee2bfd2369..29b6132b418e 100644
>>> --- a/drivers/net/ethernet/cadence/Kconfig
>>> +++ b/drivers/net/ethernet/cadence/Kconfig
>>> @@ -1,6 +1,6 @@
>>> # SPDX-License-Identifier: GPL-2.0-only
>>> #
>>> -# Atmel device configuration
>>> +# Cadence device configuration
>>> #
>>>
>>> config NET_VENDOR_CADENCE
>>> @@ -13,8 +13,8 @@ config NET_VENDOR_CADENCE
>>> If unsure, say Y.
>>>
>>> Note that the answer to this question doesn't directly affect the
>>> - kernel: saying N will just cause the configurator to skip all
>>> - the remaining Atmel network card questions. If you say Y, you will be
>>> + kernel: saying N will just cause the configurator to skip all the
>>> + remaining Cadence network card questions. If you say Y, you will be
>>> asked for your specific card in the following questions.
>>>
>>> if NET_VENDOR_CADENCE
>>>
>>
>>
>> --
>> Nicolas Ferre
--
Nicolas Ferre
On Tue, Jun 25, 2019 at 4:17 AM <[email protected]> wrote:
>
> On 24/06/2019 at 11:57, Palmer Dabbelt wrote:
> > External E-Mail
> >
> >
> > On Mon, 24 Jun 2019 02:40:21 PDT (-0700), [email protected] wrote:
> >> On 24/06/2019 at 08:16, Palmer Dabbelt wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> The patch to add support for the FU540-C000 added a dependency on
> >>> COMMON_CLK, but didn't express that via Kconfig. This fixes the build
> >>> failure by adding CONFIG_MACB_FU540, which depends on COMMON_CLK and
> >>> conditionally enables the FU540-C000 support.
> >>
> >> Let's try to limit the use of #ifdef's throughout the code. We are
> >> using them in this driver but only for the hot paths and things that
> >> have an impact on performance. I don't think it's the case here: so
> >> please find another option => NACK.
> >
> > OK. Would you accept adding a Kconfig dependency of the generic MACB driver on
> > COMMON_CLK, as suggested in the cover letter?
>
> Yes: all users of this peripheral have COMMON_CLK set.
> You can remove it from the PCI wrapper then.
>
Yes, +1, both Zynq and ZynqMP have COMMON_CLK set, thanks.
Regards,
Harini