| Message ID | 20230211031821.976408-9-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
> + > +#define JH7100_SYSMAIN_REGISTER28 0x70 > +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ > +#define JH7100_SYSMAIN_REGISTER49 0xc8 Seems like the comment should be one line earlier? There is value in basing the names on the datasheet, but you could append something meaningful on the end: #define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 ??? > + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", >xclk_dlychain)) { > + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain); > + if (ret) > + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n"); > + } You should probably document that if starfive,gtxclk-dlychain is not found in the DT blob, the value for the delay chain is undefined. It would actually be better to define it, set it to 0 for example. That way, you know you don't have any dependency on the bootloader for example. Andrew
On 11/02/2023 04:18, Cristian Ciocaltea wrote: > From: Emil Renner Berthing <kernel@esmil.dk> > > This adds a glue layer for the Synopsys DesignWare MAC IP core on the > StarFive JH7100 SoC. > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > [drop references to JH7110, update JH7100 compatible string] > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > MAINTAINERS | 1 + > drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 ++ > drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + > .../ethernet/stmicro/stmmac/dwmac-starfive.c | 155 ++++++++++++++++++ > 4 files changed, 169 insertions(+) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d48468b81b94..defedaff6041 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19820,6 +19820,7 @@ STARFIVE DWMAC GLUE LAYER > M: Emil Renner Berthing <kernel@esmil.dk> > S: Maintained > F: Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml > +F: drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > > STARFIVE JH7100 CLOCK DRIVERS > M: Emil Renner Berthing <kernel@esmil.dk> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig > index f77511fe4e87..2c81aa594291 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig > @@ -165,6 +165,18 @@ config DWMAC_SOCFPGA > for the stmmac device driver. This driver is used for > arria5 and cyclone5 FPGA SoCs. > > +config DWMAC_STARFIVE > + tristate "StarFive DWMAC support" Bring only one driver. https://lore.kernel.org/all/20230118061701.30047-6-yanhong.wang@starfivetech.com/ Best regards, Krzysztof
On 2/13/23 11:26, Krzysztof Kozlowski wrote: > On 11/02/2023 04:18, Cristian Ciocaltea wrote: >> From: Emil Renner Berthing <kernel@esmil.dk> >> >> This adds a glue layer for the Synopsys DesignWare MAC IP core on the >> StarFive JH7100 SoC. >> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> [drop references to JH7110, update JH7100 compatible string] >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> MAINTAINERS | 1 + >> drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 ++ >> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + >> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 155 ++++++++++++++++++ >> 4 files changed, 169 insertions(+) >> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index d48468b81b94..defedaff6041 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19820,6 +19820,7 @@ STARFIVE DWMAC GLUE LAYER >> M: Emil Renner Berthing <kernel@esmil.dk> >> S: Maintained >> F: Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml >> +F: drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> >> STARFIVE JH7100 CLOCK DRIVERS >> M: Emil Renner Berthing <kernel@esmil.dk> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> index f77511fe4e87..2c81aa594291 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig >> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> @@ -165,6 +165,18 @@ config DWMAC_SOCFPGA >> for the stmmac device driver. This driver is used for >> arria5 and cyclone5 FPGA SoCs. >> >> +config DWMAC_STARFIVE >> + tristate "StarFive DWMAC support" > > Bring only one driver. > > https://lore.kernel.org/all/20230118061701.30047-6-yanhong.wang@starfivetech.com/ Already mentioned in the cover letter that we have this overlap (will be merged into a single driver). Thanks, Cristian > > Best regards, > Krzysztof > >
On 2/11/23 18:11, Andrew Lunn wrote: >> + >> +#define JH7100_SYSMAIN_REGISTER28 0x70 >> +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ >> +#define JH7100_SYSMAIN_REGISTER49 0xc8 > > Seems like the comment should be one line earlier? > > There is value in basing the names on the datasheet, but you could > append something meaningful on the end: > > #define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 > > ??? Unfortunately the JH7100 datasheet I have access to doesn't provide any information regarding the SYSCTRL-MAINSYS related registers. Maybe Emil could provide some details here? >> + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", >xclk_dlychain)) { >> + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain); >> + if (ret) >> + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n"); >> + } > > You should probably document that if starfive,gtxclk-dlychain is not > found in the DT blob, the value for the delay chain is undefined. It > would actually be better to define it, set it to 0 for example. That > way, you know you don't have any dependency on the bootloader for > example. Sure, I will set it to 0. > > Andrew Thanks for reviewing, Cristian
On Wed, 15 Feb 2023 at 01:09, Cristian Ciocaltea <cristian.ciocaltea@collabora.com> wrote: > > On 2/11/23 18:11, Andrew Lunn wrote: > >> + > >> +#define JH7100_SYSMAIN_REGISTER28 0x70 > >> +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ > >> +#define JH7100_SYSMAIN_REGISTER49 0xc8 > > > > Seems like the comment should be one line earlier? Well yes, the very generic register names are also bad, but this comment refers to the fact that it kind of makes sense that register 28 has the offset 28 * 4 bytes pr. register = 0x70 ..but then register 49 is oddly out of place at offset 0xc8 instead of 49 * 4 bytes pr. register = 0xc4 > > There is value in basing the names on the datasheet, but you could > > append something meaningful on the end: > > > > #define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 > > Unfortunately the JH7100 datasheet I have access to doesn't provide any > information regarding the SYSCTRL-MAINSYS related registers. Maybe Emil > could provide some details here? This is reverse engineered from the auto generated headers in their u-boot: https://github.com/starfive-tech/u-boot/blob/JH7100_VisionFive_devel/arch/riscv/include/asm/arch-jh7100/syscon_sysmain_ctrl_macro.h Christian, I'm happy that you're working on this, but mess like this and waiting for the non-coherent dma to be sorted is why I didn't send it upstream yet. > >> + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", >xclk_dlychain)) { > >> + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain); > >> + if (ret) > >> + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n"); > >> + } > > > > You should probably document that if starfive,gtxclk-dlychain is not > > found in the DT blob, the value for the delay chain is undefined. It > > would actually be better to define it, set it to 0 for example. That > > way, you know you don't have any dependency on the bootloader for > > example. > > Sure, I will set it to 0. > > > > > Andrew > > Thanks for reviewing, > Cristian > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 2/15/23 13:20, Emil Renner Berthing wrote: > On Wed, 15 Feb 2023 at 01:09, Cristian Ciocaltea > <cristian.ciocaltea@collabora.com> wrote: >> >> On 2/11/23 18:11, Andrew Lunn wrote: >>>> + >>>> +#define JH7100_SYSMAIN_REGISTER28 0x70 >>>> +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ >>>> +#define JH7100_SYSMAIN_REGISTER49 0xc8 >>> >>> Seems like the comment should be one line earlier? > > Well yes, the very generic register names are also bad, but this > comment refers to the fact that it kind of makes sense that register > 28 has the offset > 28 * 4 bytes pr. register = 0x70 > ..but then register 49 is oddly out of place at offset 0xc8 instead of > 49 * 4 bytes pr. register = 0xc4 > >>> There is value in basing the names on the datasheet, but you could >>> append something meaningful on the end: >>> >>> #define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 >> >> Unfortunately the JH7100 datasheet I have access to doesn't provide any >> information regarding the SYSCTRL-MAINSYS related registers. Maybe Emil >> could provide some details here? > > This is reverse engineered from the auto generated headers in their u-boot: > https://github.com/starfive-tech/u-boot/blob/JH7100_VisionFive_devel/arch/riscv/include/asm/arch-jh7100/syscon_sysmain_ctrl_macro.h > > Christian, I'm happy that you're working on this, but mess like this > and waiting for the non-coherent dma to be sorted is why I didn't send > it upstream yet. Thank you for clarifying this and for all the work you have done so far, Emil! If you don't mind, I would be glad to continue helping with this mainlining effort. >>>> + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", >xclk_dlychain)) { >>>> + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain); >>>> + if (ret) >>>> + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n"); >>>> + } >>> >>> You should probably document that if starfive,gtxclk-dlychain is not >>> found in the DT blob, the value for the delay chain is undefined. It >>> would actually be better to define it, set it to 0 for example. That >>> way, you know you don't have any dependency on the bootloader for >>> example. >> >> Sure, I will set it to 0. >> >>> >>> Andrew >> >> Thanks for reviewing, >> Cristian >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Feb 15, 2023 at 02:08:01AM +0200, Cristian Ciocaltea wrote: > On 2/11/23 18:11, Andrew Lunn wrote: > > > + > > > +#define JH7100_SYSMAIN_REGISTER28 0x70 > > > +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ > > > +#define JH7100_SYSMAIN_REGISTER49 0xc8 > > > > Seems like the comment should be one line earlier? > > > > There is value in basing the names on the datasheet, but you could > > append something meaningful on the end: > > > > #define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 > > > > ??? > > Unfortunately the JH7100 datasheet I have access to doesn't provide any > information regarding the SYSCTRL-MAINSYS related registers. Maybe Emil > could provide some details here? If you have no reliable source of naming, just make a name up from how the register is used. This is why i suggested adding _DLYCHAIN, because that is what is written to it. You should be able to do the same with register 28. Andrew
diff --git a/MAINTAINERS b/MAINTAINERS index d48468b81b94..defedaff6041 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19820,6 +19820,7 @@ STARFIVE DWMAC GLUE LAYER M: Emil Renner Berthing <kernel@esmil.dk> S: Maintained F: Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml +F: drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c STARFIVE JH7100 CLOCK DRIVERS M: Emil Renner Berthing <kernel@esmil.dk> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig index f77511fe4e87..2c81aa594291 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig @@ -165,6 +165,18 @@ config DWMAC_SOCFPGA for the stmmac device driver. This driver is used for arria5 and cyclone5 FPGA SoCs. +config DWMAC_STARFIVE + tristate "StarFive DWMAC support" + default m if SOC_STARFIVE + depends on SOC_STARFIVE || COMPILE_TEST + select MFD_SYSCON + help + Support for ethernet controller on StarFive SOCs. + + This selects StarFive SoC glue layer support for the stmmac device + driver. This driver is used for the JH71x0 series GMAC ethernet + controller. + config DWMAC_STI tristate "STi GMAC support" default ARCH_STI diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile index 057e4bab5c08..8738fdbb4b2d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Makefile +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile @@ -23,6 +23,7 @@ obj-$(CONFIG_DWMAC_OXNAS) += dwmac-oxnas.o obj-$(CONFIG_DWMAC_QCOM_ETHQOS) += dwmac-qcom-ethqos.o obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-altr-socfpga.o +obj-$(CONFIG_DWMAC_STARFIVE) += dwmac-starfive.o obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c new file mode 100644 index 000000000000..d4c81f1a5482 --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * dwmac-starfive.c - DWMAC glue layer for StarFive JH7100 SoC + * + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> + */ + +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include "stmmac.h" +#include "stmmac_platform.h" + +#define JH7100_SYSMAIN_REGISTER28 0x70 +/* The value below is not a typo, just really bad naming by StarFive ¯\_(ツ)_/¯ */ +#define JH7100_SYSMAIN_REGISTER49 0xc8 + +struct dwmac_starfive { + struct device *dev; + struct clk *gtxc; +}; + +static int dwmac_starfive_jh7100_syscon_init(struct device *dev) +{ + struct device_node *np = dev->of_node; + struct regmap *sysmain; + u32 gtxclk_dlychain; + int ret; + + sysmain = syscon_regmap_lookup_by_phandle(np, "starfive,syscon"); + if (IS_ERR(sysmain)) + return dev_err_probe(dev, PTR_ERR(sysmain), + "error getting sysmain registers\n"); + + /* Choose RGMII interface to the phy. + * TODO: support other interfaces once we know the meaning of other + * values in the register + */ + ret = regmap_update_bits(sysmain, JH7100_SYSMAIN_REGISTER28, 0x7, 1); + if (ret) + return dev_err_probe(dev, ret, "error selecting gmac interface\n"); + + if (!of_property_read_u32(np, "starfive,gtxclk-dlychain", >xclk_dlychain)) { + ret = regmap_write(sysmain, JH7100_SYSMAIN_REGISTER49, gtxclk_dlychain); + if (ret) + return dev_err_probe(dev, ret, "error selecting gtxclk delay chain\n"); + } + + return 0; +} + +static void dwmac_starfive_fix_mac_speed(void *data, unsigned int speed) +{ + struct dwmac_starfive *dwmac = data; + unsigned long rate; + int ret; + + switch (speed) { + case SPEED_1000: + rate = 125000000; + break; + case SPEED_100: + rate = 25000000; + break; + case SPEED_10: + rate = 2500000; + break; + default: + dev_warn(dwmac->dev, "unsupported link speed %u\n", speed); + return; + } + + ret = clk_set_rate(dwmac->gtxc, rate); + if (ret) + dev_err(dwmac->dev, "error setting gtx clock rate: %d\n", ret); +} + +static int dwmac_starfive_probe(struct platform_device *pdev) +{ + struct stmmac_resources stmmac_res; + struct plat_stmmacenet_data *plat; + struct dwmac_starfive *dwmac; + struct clk *txclk; + int (*syscon_init)(struct device *dev); + int ret; + + dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL); + if (!dwmac) + return -ENOMEM; + + ret = stmmac_get_platform_resources(pdev, &stmmac_res); + if (ret) + return ret; + + syscon_init = of_device_get_match_data(&pdev->dev); + if (syscon_init) { + ret = syscon_init(&pdev->dev); + if (ret) + return ret; + } + + dwmac->gtxc = devm_clk_get_enabled(&pdev->dev, "gtxc"); + if (IS_ERR(dwmac->gtxc)) + return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->gtxc), + "error getting/enabling gtxc clock\n"); + + txclk = devm_clk_get_enabled(&pdev->dev, "tx"); + if (IS_ERR(txclk)) + return dev_err_probe(&pdev->dev, PTR_ERR(txclk), + "error getting/enabling tx clock\n"); + + plat = stmmac_probe_config_dt(pdev, stmmac_res.mac); + if (IS_ERR(plat)) + return dev_err_probe(&pdev->dev, PTR_ERR(plat), + "dt configuration failed\n"); + + dwmac->dev = &pdev->dev; + plat->bsp_priv = dwmac; + plat->fix_mac_speed = dwmac_starfive_fix_mac_speed; + + ret = stmmac_dvr_probe(&pdev->dev, plat, &stmmac_res); + if (ret) { + stmmac_remove_config_dt(pdev, plat); + return ret; + } + + return 0; +} + +static const struct of_device_id dwmac_starfive_match[] = { + { + .compatible = "starfive,jh7100-dwmac", + .data = dwmac_starfive_jh7100_syscon_init, + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, dwmac_starfive_match); + +static struct platform_driver dwmac_starfive_driver = { + .probe = dwmac_starfive_probe, + .remove = stmmac_pltfr_remove, + .driver = { + .name = "dwmac-starfive", + .pm = &stmmac_pltfr_pm_ops, + .of_match_table = dwmac_starfive_match, + }, +}; +module_platform_driver(dwmac_starfive_driver); + +MODULE_AUTHOR("Emil Renner Berthing <kernel@esmil.dk>"); +MODULE_DESCRIPTION("StarFive DWMAC Glue Layer"); +MODULE_LICENSE("GPL");