Re: [PATCH] arm64: dts: mt8195: Add Ethernet controller

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 18/10/2022 02:37, Biao Huang wrote:
> Dear Krzysztof,
> 	Thanks for your comments!
> 
> On Mon, 2022-10-17 at 22:01 -0400, Krzysztof Kozlowski wrote:
>> On 17/10/2022 05:58, Biao Huang wrote:
>>> Add Ethernet controller node for mt8195.
>>>
>>> Signed-off-by: Biao Huang <biao.huang@mediatek.com>
>>> ---
>>>  arch/arm64/boot/dts/mediatek/mt8195-demo.dts | 88
>>> ++++++++++++++++++++
>>>  arch/arm64/boot/dts/mediatek/mt8195.dtsi     | 87
>>> +++++++++++++++++++
>>>  2 files changed, 175 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> index 4fbd99eb496a..02e04f82a4ae 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> @@ -258,6 +258,72 @@ &mt6359_vsram_others_ldo_reg {
>>>  };
>>>  
>>>  &pio {
>>> +	eth_default: eth_default {
>>
>> No underscores in node names. Please also be sure your patch does not
>> bring new warnings with `dtbs_check` (lack of suffix above could mean
>> it
>> brings...)
> OK, I'll fix the underscores issue in next send.
> As to "lack of suffix" issue, do you mean I should write it like:
> 	eth-default: eth-default@0 {

I don't know whether you should have here suffix or not - please check
your bindings. Several pinctrl bindings require suffixes (or prefixes),
thus I asked.

BTW, In the label you must use underscore.

> 		...
> 	}
> If yes, other nodes in current file don't have such suffix.
> e.g.
> 	gpio_keys_pins: gpio-keys-pins
> 
> Should I keep unified style with other nodes?

Check what bindings are requiring.

>>
>>> +		txd_pins {
>>

(...)

>>> +
>>> +		eth: ethernet@11021000 {
>>> +			compatible = "mediatek,mt8195-gmac",
>>> "snps,dwmac-5.10a";
>>> +			reg = <0 0x11021000 0 0x4000>;
>>> +			interrupts = <GIC_SPI 716 IRQ_TYPE_LEVEL_HIGH
>>> 0>;
>>> +			interrupt-names = "macirq";
>>> +			mac-address = [00 55 7b b5 7d f7];
>>
>> How is this property of a SoC? Are you saying now that all MT8195
>> SoCs
>> have the same MAC address?
> The mac-address here is taken as a default mac address in eth driver
> rather than a randome one.
> Actually, there will be a tool to customize eth mac address (e.g
> through "ifconfig eth0 hw ether xx:xx:xx:xx:xx:xx"), so every
> MT8195 SoCs will get their specified mac address in real product.

So this means this is not one MAC address for all SoCs, so this does not
belong to DTSI. Actually it doesn't belong to DTS either. Look how
others are doing...


Best regards,
Krzysztof