Re: [PATCH 08/12] net: stmmac: Add glue layer for StarFive JH7100 SoC

[Emil Renner Berthing <emil.renner.berthing@canonical.com>]

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", &gtxclk_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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv