Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III
 support.
To:     Andrew Lunn <andrew@lunn.ch>, David Daney <david.daney@cavium.com>
Cc:     linux-mips@linux-mips.org, ralf@linux-mips.org,
        James Hogan <james.hogan@mips.com>, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        devel@driverdev.osuosl.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org,
        "Steven J. Hill" <steven.hill@cavium.com>,
        devicetree@vger.kernel.org,
        Florian Fainelli <f.fainelli@gmail.com>,
        Carlos Munoz <cmunoz@cavium.com>
References: <20171129005540.28829-1-david.daney@cavium.com>
 <20171129005540.28829-8-david.daney@cavium.com>
 <20171129225609.GE1706@lunn.ch>
From:   David Daney <ddaney@caviumnetworks.com>
Message-ID: <d7fadd2f-3478-6db4-2c57-e6f441631ee2@caviumnetworks.com>
Date:   Wed, 29 Nov 2017 15:04:53 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171129225609.GE1706@lunn.ch>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2017 23:04:56.6271 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b7ec90e-00dd-4348-1579-08d5377d9c88
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR07MB3492
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/29/2017 02:56 PM, Andrew Lunn wrote:
> On Tue, Nov 28, 2017 at 04:55:39PM -0800, David Daney wrote:
>> +static int bgx_probe(struct platform_device *pdev)
>> +{
>> +	struct mac_platform_data platform_data;
>> +	const __be32 *reg;
>> +	u32 port;
>> +	u64 addr;
>> +	struct device_node *child;
>> +	struct platform_device *new_dev;
>> +	struct platform_device *pki_dev;
>> +	int numa_node, interface;
>> +	int i;
>> +	int r = 0;
>> +	char id[64];
>> +	u64 data;
>> +
>> +	reg = of_get_property(pdev->dev.of_node, "reg", NULL);
>> +	addr = of_translate_address(pdev->dev.of_node, reg);
>> +	interface = (addr >> 24) & 0xf;
>> +	numa_node = (addr >> 36) & 0x7;
> 
> Hi David
> 
> You have these two a few times in the code. Maybe add a helper to do
> it? The NUMA one i assume could go somewhere in the SoC code?
> 

Thanks for looking at it, I will try with helpers.


The rest of your comments below raise valid points, I will fix those too.


>> +static int bgx_mix_init_from_fdt(void)
>> +{
>> +	struct device_node	*node;
>> +	struct device_node	*parent = NULL;
>> +	int			mix = 0;
> 
>> +		/* Get the lmac index */
>> +		reg = of_get_property(lmac_fdt_node, "reg", NULL);
>> +		if (!reg)
>> +			goto err;
>> +
>> +		mix_port_lmacs[mix].lmac = *reg;
> 
> I don't think of_get_property() deals with endianness. Is there any
> danger of this driver being used on hardware with the other endianness
> to what you have tested?
> 
>> +/**
>> + * bgx_pki_init_from_param - Initialize the list of lmacs that connect to the
>> + *			     pki from information in the "pki_port" parameter.
>> + *
>> + *			     The pki_port parameter format is as follows:
>> + *			     pki_port=nbl
>> + *			     where:
>> + *				n = node
>> + *				b = bgx
>> + *				l = lmac
>> + *
>> + *			     Commas must be used to separate multiple lmacs:
>> + *			     pki_port=000,100,110
>> + *
>> + *			     Asterisks (*) specify all possible characters in
>> + *			     the subset:
>> + *			     pki_port=00* (all lmacs of node0 bgx0).
>> + *
>> + *			     Missing lmacs identifiers default to all
>> + *			     possible characters in the subset:
>> + *			     pki_port=00 (all lmacs on node0 bgx0)
>> + *
>> + *			     Brackets ('[' and ']') specify the valid
>> + *			     characters in the subset:
>> + *			     pki_port=00[01] (lmac0 and lmac1 of node0 bgx0).
>> + *
>> + * Returns 0 if successful.
>> + * Returns <0 for error codes.
> 
> I've not used kerneldoc much, but i suspect this is wrongly formated:
> 
> https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#function-documentation
> 
>> +int bgx_port_ethtool_set_settings(struct net_device	*netdev,
>> +				  struct ethtool_cmd	*cmd)
>> +{
>> +	struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
> 
> Not required. The enforces this. See dev_ethtool()
> 
>> +
>> +	if (p->phydev)
>> +		return phy_ethtool_sset(p->phydev, cmd);
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +EXPORT_SYMBOL(bgx_port_ethtool_set_settings);
>> +
>> +int bgx_port_ethtool_nway_reset(struct net_device *netdev)
>> +{
>> +	struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
> 
> Also not needed.
> 
>> +static void bgx_port_adjust_link(struct net_device *netdev)
>> +{
>> +	struct bgx_port_priv	*priv = bgx_port_netdev2priv(netdev);
>> +	bool			link_changed = false;
>> +	unsigned int		link;
>> +	unsigned int		speed;
>> +	unsigned int		duplex;
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	if (!priv->phydev->link && priv->last_status.link)
>> +		link_changed = true;
>> +
>> +	if (priv->phydev->link &&
>> +	    (priv->last_status.link != priv->phydev->link ||
>> +	     priv->last_status.duplex != priv->phydev->duplex ||
>> +	     priv->last_status.speed != priv->phydev->speed))
>> +		link_changed = true;
>> +
>> +	link = priv->phydev->link;
>> +	priv->last_status.link = priv->phydev->link;
>> +
>> +	speed = priv->phydev->speed;
>> +	priv->last_status.speed = priv->phydev->speed;
>> +
>> +	duplex = priv->phydev->duplex;
>> +	priv->last_status.duplex = priv->phydev->duplex;
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	if (link_changed) {
>> +		struct port_status status;
>> +
>> +		phy_print_status(priv->phydev);
>> +
>> +		status.link = link ? 1 : 0;
>> +		status.duplex = duplex;
>> +		status.speed = speed;
>> +		if (!link) {
>> +			netif_carrier_off(netdev);
>> +			 /* Let TX drain. FIXME check that it is drained. */
>> +			mdelay(50);
>> +		}
>> +		priv->set_link(priv, status);
>> +		if (link)
>> +			netif_carrier_on(netdev);
> 
> The code should do netif_carrier_on/off for you. See phy_link_change()
> 
>> +static void bgx_port_check_state(struct work_struct *work)
>> +{
>> +	struct bgx_port_priv	*priv;
>> +	struct port_status	status;
>> +
>> +	priv = container_of(work, struct bgx_port_priv, dwork.work);
>> +
>> +	status = priv->get_link(priv);
>> +
>> +	if (!status.link &&
>> +	    priv->mode != PORT_MODE_SGMII && priv->mode != PORT_MODE_RGMII)
>> +		bgx_port_init_xaui_link(priv);
>> +
>> +	if (priv->last_status.link != status.link) {
>> +		priv->last_status.link = status.link;
>> +		if (status.link)
>> +			netdev_info(priv->netdev, "Link is up - %d/%s\n",
>> +				    status.speed,
>> +				    status.duplex == DUPLEX_FULL ? "Full" : "Half");
> 
> You already have phy_print_status() in bgx_port_adjust_link(). Do you need this here?
> 
>> +		else
>> +			netdev_info(priv->netdev, "Link is down\n");
>> +	}
>> +
>> +	mutex_lock(&priv->lock);
>> +	if (priv->work_queued)
>> +		queue_delayed_work(check_state_wq, &priv->dwork, HZ);
>> +	mutex_unlock(&priv->lock);
>> +}
>> +
>> +int bgx_port_enable(struct net_device *netdev)
>> +{
> 
> 
>> +	} else {
>> +		priv->phydev = of_phy_connect(netdev, priv->phy_np,
>> +					      bgx_port_adjust_link, 0, priv->phy_mode);
>> +		if (!priv->phydev)
>> +			return -ENODEV;
>> +
>> +		netif_carrier_off(netdev);
>> +
>> +		if (priv->phydev)
> 
> You already checked this above.
> 
>> +			phy_start_aneg(priv->phydev);
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(bgx_port_enable);
>> +
>> +int bgx_port_change_mtu(struct net_device *netdev, int new_mtu)
>> +{
>> +	struct bgx_port_priv *priv = bgx_port_netdev2priv(netdev);
>> +	int max_frame;
>> +
>> +	if (new_mtu < 60 || new_mtu > 65392) {
> 
> See dev_set_mtu(). If you have done your initialisation correctly, this
> won't happen.
> 
>> +static int bgx_port_probe(struct platform_device *pdev)
>> +{
>> +	switch (priv->mode) {
>> +	case PORT_MODE_SGMII:
>> +		if (priv->phy_np &&
>> +		    priv->phy_mode != PHY_INTERFACE_MODE_SGMII)
>> +			dev_warn(&pdev->dev, "SGMII phy mode mismatch.\n");
>> +		goto set_link_functions;
>> +	case PORT_MODE_RGMII:
>> +		if (priv->phy_np &&
>> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII &&
>> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_ID &&
>> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_RXID &&
>> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_TXID)
> 
> phy_interface_mode_is_rgmii()
> 
> More later, maybe.
> 
>       Andrew
> 


From 1585442953424167582@xxx Wed Nov 29 22:57:04 +0000 2017
X-GM-THRID: 1585360038205872912
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread