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

[Biao Huang]

Dear Krzysztof,
	Thanks for your comments~

On Tue, 2022-10-18 at 08:51 -0400, Krzysztof Kozlowski wrote:
> 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@xxxxxxxxxxxx>
> > > > ---
> > > >  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.
OK, I'll check the pinctrl-mt8195.yaml, and modify the eth related
node.
> 
> > 		...
> > 	}
> > 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.
OK.
> 
> > > 
> > > > +		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...
OK, will remove it in next send.
> 
> 
> Best regards,
> Krzysztof
>