From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilias Apalodimas Subject: Re: [PATCH 4/4] cpsw: add switchdev support Date: Fri, 25 May 2018 07:56:47 +0300 Message-ID: <20180525045647.GA1758@apalos> References: <1527144984-31236-1-git-send-email-ilias.apalodimas@linaro.org> <1527144984-31236-5-git-send-email-ilias.apalodimas@linaro.org> <20180524131229.GC24557@lunn.ch> <20180524133234.GA15703@apalos> <20180524163904.GH5128@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, grygorii.strashko@ti.com, ivan.khoronzhuk@linaro.org, nsekhar@ti.com, jiri@resnulli.us, ivecera@redhat.com, francois.ozog@linaro.org, yogeshs@ti.com, spatton@ti.com To: Andrew Lunn Return-path: Received: from mail-wr0-f196.google.com ([209.85.128.196]:36271 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750934AbeEYE4w (ORCPT ); Fri, 25 May 2018 00:56:52 -0400 Received: by mail-wr0-f196.google.com with SMTP id k5-v6so6858186wrn.3 for ; Thu, 24 May 2018 21:56:52 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180524163904.GH5128@lunn.ch> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, May 24, 2018 at 06:39:04PM +0200, Andrew Lunn wrote: > On Thu, May 24, 2018 at 04:32:34PM +0300, Ilias Apalodimas wrote: > > On Thu, May 24, 2018 at 03:12:29PM +0200, Andrew Lunn wrote: > > > Device tree is supposed to describe the hardware. Using that hardware > > > in different ways is not something you should describe in DT. > > > > > The new switchdev mode is applied with a .config option in the kernel. What you > > see is pre-existing code, so i am not sure if i should change it in this > > patchset. > > If you break the code up into a library and two drivers, it becomes a > moot point. Agree > > But what i don't like here is that the device tree says to do dual > mac. But you ignore that and do sometime else. I would prefer that if > DT says dual mac, and switchdev is compiled in, the probe fails with > EINVAL. Rather than ignore something, make it clear it is invalid. The switch has 3 modes of operation as is. 1. switch mode, to enable that you don't need to add anything on the DTS and linux registers a single netdev interface. 2. dual mac mode, this is when you need to add dual_emac; on the DTS. 3. switchdev mode which is controlled by a .config option, since as you pointed out DTS was not made for controlling config options. I agree that this is far from beautiful. If the driver remains as in though, i'd prefer either keeping what's there or making "switchdev" a DTS option, following the pre-existing erroneous usage rather than making the device unusable. If we end up returning some error and refuse to initialize, users that remote upgrade their equipment, without taking a good look at changelog, will loose access to their devices with no means of remotely fixing that. Regards Ilias