From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7D115C43461 for ; Tue, 4 May 2021 12:31:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 46F3F613B4 for ; Tue, 4 May 2021 12:31:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230357AbhEDMcr (ORCPT ); Tue, 4 May 2021 08:32:47 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:52298 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230110AbhEDMcq (ORCPT ); Tue, 4 May 2021 08:32:46 -0400 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1lduD8-002Tjq-Qa; Tue, 04 May 2021 14:31:34 +0200 Date: Tue, 4 May 2021 14:31:34 +0200 From: Andrew Lunn To: Colin Foster Cc: Rob Herring , Vladimir Oltean , Claudiu Manoil , Alexandre Belloni , "supporter:OCELOT ETHERNET SWITCH DRIVER" , Vivien Didelot , Florian Fainelli , "David S. Miller" , Jakub Kicinski , Russell King , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , "open list:OCELOT ETHERNET SWITCH DRIVER" Subject: Re: [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control Message-ID: References: <20210504051130.1207550-1-colin.foster@in-advantage.com> <20210504051130.1207550-2-colin.foster@in-advantage.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210504051130.1207550-2-colin.foster@in-advantage.com> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, May 03, 2021 at 10:11:27PM -0700, Colin Foster wrote: > Add support for control for VSC75XX chips over SPI control. Starting with the > VSC9959 code, this will utilize a spi bus instead of PCIe or memory-mapped IO to > control the chip. Hi Colin Please fix your subject line for the next version. vN should of been v1. The number is important so we can tell revisions apart. > > Signed-off-by: Colin Foster > --- > arch/arm/boot/dts/rpi-vsc7512-spi-overlay.dts | 124 ++ > drivers/net/dsa/ocelot/Kconfig | 11 + > drivers/net/dsa/ocelot/Makefile | 5 + > drivers/net/dsa/ocelot/felix_vsc7512_spi.c | 1214 +++++++++++++++++ > include/soc/mscc/ocelot.h | 15 + Please split this patch up. The DT overlay will probably be merged via ARM SOC, not netdev. You also need to document the device tree binding, as a separate patch. > + fragment@3 { > + target = <&spi0>; > + __overlay__ { > + #address-cells = <1>; > + #size-cells = <0>; > + cs-gpios = <&gpio 8 1>; > + status = "okay"; > + > + vsc7512: vsc7512@0{ > + compatible = "mscc,vsc7512"; > + spi-max-frequency = <250000>; > + reg = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + ethernet = <ðernet>; > + phy-mode = "internal"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > + }; > + > + port@1 { > + reg = <1>; > + label = "swp1"; > + status = "disabled"; > + }; > + > + port@2 { > + reg = <2>; > + label = "swp2"; > + status = "disabled"; > + }; I'm surprised all the ports are disabled. I could understand that for a DTSI file, but a DTS overlay? > +++ b/drivers/net/dsa/ocelot/felix_vsc7512_spi.c > @@ -0,0 +1,1214 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > +/* Copyright 2017 Microsemi Corporation > + * Copyright 2018-2019 NXP Semiconductors > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "felix.h" > + > +#define VSC7512_TAS_GCL_ENTRY_MAX 63 > + > +// Note: These addresses and offsets are all shifted down > +// by two. This is because the SPI protocol needs them to > +// be before they get sent out. > +// > +// An alternative is to keep them standardized, but then > +// a separate spi_bus regmap would need to be defined. > +// > +// That might be optimal though. The 'Read' protocol of > +// the VSC driver might be much quicker if we add padding > +// bytes, which I don't think regmap supports. C style comments please. > +static void vsc7512_phylink_validate(struct ocelot *ocelot, int port, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + struct ocelot_port *ocelot_port = ocelot->ports[port]; > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { > + 0, > + }; This function seems out of place. Why would SPI access change what the ports are capable of doing? Please split this up into more patches. Keep the focus of this patch as being adding SPI support. Andrew