From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grygorii Strashko Subject: Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver Date: Mon, 18 Jun 2018 18:20:37 -0500 Message-ID: References: <1528974690-31600-1-git-send-email-ilias.apalodimas@linaro.org> <1528974690-31600-5-git-send-email-ilias.apalodimas@linaro.org> <20180618161627.GC5865@lunn.ch> <20180618201940.GA5890@apalos> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , , , , , , , To: Ilias Apalodimas , Andrew Lunn Return-path: Received: from fllnx209.ext.ti.com ([198.47.19.16]:47571 "EHLO fllnx209.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755376AbeFRXUu (ORCPT ); Mon, 18 Jun 2018 19:20:50 -0400 In-Reply-To: <20180618201940.GA5890@apalos> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/18/2018 03:19 PM, Ilias Apalodimas wrote: > On Mon, Jun 18, 2018 at 06:16:27PM +0200, Andrew Lunn wrote: >>> @@ -2711,6 +2789,10 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data, >>> if (of_property_read_bool(node, "dual_emac")) >>> data->switch_mode = CPSW_DUAL_EMAC; >>> >>> + /* switchdev overrides DTS */ >>> + if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV)) >>> + data->switch_mode = CPSW_SWITCHDEV; >>> + >> >> I know you discussed this a bit with Jiri, but i still think if >> 'dual_mac" is found, you should do dual mac. The DT clearly requests >> dual mac, and doing anything else is going to cause confusion. >> >> The device tree binding is ambiguous what should happen when dual-mac >> is missing. So i would only enable swithdev mode in that case. > At the moment if no 'dual_emac;' is found on DTS the driver operates in "switch > mode". It only registers 1 ethernet interface with no ability (unless you patch > the kernel) to configure the switch. If we use DTS instead of a .config option > we should add parsing for something like 'switchdev;' in the DTS. > Jiri proposed using devlink, which makes sense, but i am not sure it's > applicable on this patchset. This will change the driver completely and will > totally break backwards compatibility. > > Ideally i'd prefer something like: > 1. Add a DTS option and continue the current behavior. I agree with you that > this will cause less confusion (in fact i prefer it for the current state of the > driver compared to the .config). > or > 2. Keep the .config option which is better suited over DTS but might cause some > confusion. >> >> But ideally, it should be a new driver with a new binding. > TI is better suited to comment on this. The work proposed here is mostly to > accomodate future TSN related configuration for switches. This patchset has > been tested against Ivan's patchset for CBS and is working as expected > configuration wise. > (http://lkml.iu.edu/hypermail/linux/kernel/1806.1/05302.html) We'd try the new driver in the future -- regards, -grygorii