Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a5e14871-db4f-54ce-b925-0787750ab6c2@ti.com>
Date:   Mon, 16 May 2022 10:39:15 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver
Content-Language: en-US
To:     Andrew Lunn <andrew@lunn.ch>
CC:     <linux-kernel@vger.kernel.org>, <davem@davemloft.net>,
        <edumazet@google.com>, <krzysztof.kozlowski+dt@linaro.org>,
        <netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <nm@ti.com>, <ssantosh@kernel.org>, <s-anna@ti.com>,
        <linux-arm-kernel@lists.infradead.org>, <rogerq@kernel.org>,
        <grygorii.strashko@ti.com>, <vigneshr@ti.com>, <kishon@ti.com>,
        <robh+dt@kernel.org>, <afd@ti.com>
References: <20220506052433.28087-1-p-mohan@ti.com>
 <20220506052433.28087-3-p-mohan@ti.com> <YnVQW7xpSWEE2/HP@lunn.ch>
 <f674c56c-0621-f471-9517-5c349940d362@ti.com> <YnkJ5bFd72d0FagD@lunn.ch>
From:   Puranjay Mohan <p-mohan@ti.com>
In-Reply-To: <YnkJ5bFd72d0FagD@lunn.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Andrew,

On 09/05/22 18:02, Andrew Lunn wrote:
>>>> +static void icssg_init_emac_mode(struct prueth *prueth)
>>>> +{
>>>> +	u8 mac[ETH_ALEN] = { 0 };
>>>> +
>>>> +	if (prueth->emacs_initialized)
>>>> +		return;
>>>> +
>>>> +	regmap_update_bits(prueth->miig_rt, FDB_GEN_CFG1, SMEM_VLAN_OFFSET_MASK, 0);
>>>> +	regmap_write(prueth->miig_rt, FDB_GEN_CFG2, 0);
>>>> +	/* Clear host MAC address */
>>>> +	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
>>>
>>> Seems an odd thing to do, set it to 00:00:00:00:00:00. You probably
>>> want to add a comment why you do this odd thing.
>>
>> Actually, this is when the device is configured as a bridge, the host
>> mac address has to be set to zero to while bringing it back to emac
>> mode. I will add a comment to explain this.
> 
> I don't see any switchdev interface. How does it get into bridge mode?

I will be sending patches to add the switch mode support after this
series gets merged.

> 
>>>> +	} else if (emac->link) {
>>>> +		new_state = true;
>>>> +		emac->link = 0;
>>>> +		/* defaults for no link */
>>>> +
>>>> +		/* f/w should support 100 & 1000 */
>>>> +		emac->speed = SPEED_1000;
>>>> +
>>>> +		/* half duplex may not supported by f/w */
>>>> +		emac->duplex = DUPLEX_FULL;
>>>
>>> Why set speed and duplex when you have just lost the link? They are
>>> meaningless until the link comes back.
>>
>> These were just the default values that we added.
>> What do you suggest I put here?
> 
> Nothing. If the link is down, they are meaningless. If something is
> accessing them when the link is down, that code is broken. So i
> suppose you could give them poison values to help find your broken
> code.

Okay, I will remove it in next version.

> 
>>>> +	for_each_child_of_node(eth_ports_node, eth_node) {
>>>> +		u32 reg;
>>>> +
>>>> +		if (strcmp(eth_node->name, "port"))
>>>> +			continue;
>>>> +		ret = of_property_read_u32(eth_node, "reg", &reg);
>>>> +		if (ret < 0) {
>>>> +			dev_err(dev, "%pOF error reading port_id %d\n",
>>>> +				eth_node, ret);
>>>> +		}
>>>> +
>>>> +		if (reg == 0)
>>>> +			eth0_node = eth_node;
>>>> +		else if (reg == 1)
>>>> +			eth1_node = eth_node;
>>>
>>> and if reg == 4
>>>
>>> Or reg 0 appears twice?
>>
>> In both of the cases that you mentioned, the device tree schema check
>> will fail, hence, we can safely assume that this will be 0 and 1 only.
> 
> Nothing forces you to run the scheme checker. It is not run by the
> kernel before it starts accessing the DT blob. You should assume it is
> invalid until you have proven it to be valid.

I will add error checking here to make sure it is handled.

> 
> 	Andrew

Thanks,
Puranjay Mohan