| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <519fae84-cc40-47ab-8e6b-9967ce046104@collabora.com>
Date: Tue, 31 Oct 2023 20:07:22 +0200
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Emil Renner Berthing <kernel@...il.dk>,
Samin Guo <samin.guo@...rfivetech.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <joabreu@...opsys.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Richard Cochran <richardcochran@...il.com>,
Giuseppe Cavallaro <peppe.cavallaro@...com>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v2 05/12] net: stmmac: dwmac-starfive: Add support for
JH7100 SoC
On 10/31/23 16:33, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> Add a missing quirk to enable support for the StarFive JH7100 SoC.
>>
>> Additionally, for greater flexibility in operation, allow using the
>> rgmii-rxid and rgmii-txid phy modes.
>>
>> Co-developed-by: Emil Renner Berthing <kernel@...il.dk>
>> Signed-off-by: Emil Renner Berthing <kernel@...il.dk>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>
> Hi Cristian,
>
> Thanks for working on this! This driver has code to update the phy clock for
> different line speeds. I don't think that will work without the
> CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on
> [2].
>
> [1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666
> [2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae
>
> Two more comments below..
Hi Emil,
Thanks for the review!
I've been only testing this at 1000 Mbps and so far it seems to work
fine. I will try with different line speeds and report back.
>> ---
>> drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++--
>> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++---
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> index a2b9e289aa36..c3c2c8360047 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> @@ -165,9 +165,9 @@ config DWMAC_STARFIVE
>> help
>> Support for ethernet controllers on StarFive RISC-V SoCs
>>
>> - This selects the StarFive platform specific glue layer support for
>> - the stmmac device driver. This driver is used for StarFive JH7110
>> - ethernet controller.
>> + This selects the StarFive platform specific glue layer support
>> + for the stmmac device driver. This driver is used for the
>> + StarFive JH7100 and JH7110 ethernet controllers.
>>
>> config DWMAC_STI
>> tristate "STi GMAC support"
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> index 5d630affb4d1..88c431edcea0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> @@ -15,13 +15,20 @@
>>
>> #include "stmmac_platform.h"
>>
>> -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
>> -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
>> -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
>> +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
>> +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
>> +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
>> +
>> +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8
>> +
>> +struct starfive_dwmac_data {
>> + unsigned int gtxclk_dlychain;
>> +};
>>
>> struct starfive_dwmac {
>> struct device *dev;
>> struct clk *clk_tx;
>> + const struct starfive_dwmac_data *data;
>> };
>>
>> static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>> @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>>
>> case PHY_INTERFACE_MODE_RGMII:
>> case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> mode = STARFIVE_DWMAC_PHY_INFT_RGMII;
>> break;
>>
>> @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>> if (err)
>> return dev_err_probe(dwmac->dev, err, "error setting phy mode\n");
>>
>> + if (dwmac->data) {
>
> I think you want something like this so future quirks don't need to touch this
> code:
>
> if (dwmac->data && dwmac->data->gtxclk_dlychain)
Yes, but that would prevent having a quirk that explicitly wants to write 0.
I was initially thinking to something more generic, like providing a
list of register-value pairs, but I'm not sure this is going to be ever
needed.
I'm still open to apply the suggested change, though.
>> + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN,
>> + dwmac->data->gtxclk_dlychain);
>> + if (err)
>> + return dev_err_probe(dwmac->dev, err,
>> + "error selecting gtxclk delay chain\n");
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>> if (!dwmac)
>> return -ENOMEM;
>>
>> + dwmac->data = device_get_match_data(&pdev->dev);
>> +
>> dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
>> if (IS_ERR(dwmac->clk_tx))
>> return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
>> @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>> return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
>> }
>>
>> +static const struct starfive_dwmac_data jh7100_data = {
>> + .gtxclk_dlychain = 4
>
> Please add a , at the end of this line. I know it's unlikely that we need to
> add more properties, but it's still good practice to do. This way such patches
> won't need to touch this line.
Sure, will do.
>> +};
>> +
>> static const struct of_device_id starfive_dwmac_match[] = {
>> - { .compatible = "starfive,jh7110-dwmac" },
>> + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data },
>> + { .compatible = "starfive,jh7110-dwmac" },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, starfive_dwmac_match);
>> --
>> 2.42.0
>>
Powered by blists - more mailing lists