From: Puranjay Mohan <p-mohan@ti.com>
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>
Subject: Re: [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver
Date: Mon, 16 May 2022 10:39:15 +0530 [thread overview]
Message-ID: <a5e14871-db4f-54ce-b925-0787750ab6c2@ti.com> (raw)
In-Reply-To: <YnkJ5bFd72d0FagD@lunn.ch>
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", ®);
>>>> + 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
next prev parent reply other threads:[~2022-05-16 5:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-06 5:24 [PATCH 0/2] Introduce ICSSG based ethernet Driver Puranjay Mohan
2022-05-06 5:24 ` [PATCH 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings Puranjay Mohan
2022-05-06 13:37 ` Rob Herring
2022-05-10 18:07 ` Rob Herring
2022-05-06 5:24 ` [PATCH 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver Puranjay Mohan
2022-05-06 16:44 ` Andrew Lunn
2022-05-09 10:20 ` Puranjay Mohan
2022-05-09 12:32 ` Andrew Lunn
2022-05-16 5:09 ` Puranjay Mohan [this message]
2022-05-06 16:49 ` Andrew Lunn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5e14871-db4f-54ce-b925-0787750ab6c2@ti.com \
--to=p-mohan@ti.com \
--cc=afd@ti.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=grygorii.strashko@ti.com \
--cc=kishon@ti.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nm@ti.com \
--cc=robh+dt@kernel.org \
--cc=rogerq@kernel.org \
--cc=s-anna@ti.com \
--cc=ssantosh@kernel.org \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).