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

Cristian Ciocaltea
2023-02-15T00:08:11Z

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