Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Sep 2022 19:58:09 -0700
From:   Colin Foster <colin.foster@in-advantage.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot
 switch control
Message-ID: <YyksQQrXb0xDuksE@euler>
References: <20220911200244.549029-1-colin.foster@in-advantage.com>
 <20220911200244.549029-1-colin.foster@in-advantage.com>
 <20220911200244.549029-9-colin.foster@in-advantage.com>
 <20220911200244.549029-9-colin.foster@in-advantage.com>
 <20220912172109.ezilo6su5w6dihrk@skbuf>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220912172109.ezilo6su5w6dihrk@skbuf>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?If6PJODuv5+W3UdFgsNKu5g4RhldMW802D9+oA8RqxKKMxWxaOkhecIQi+DH?=
 =?us-ascii?Q?0WerVdNYWgkPYHO6q7gv3Z6cbHjD4mSD/qtKJ5EcrdANemXLzClu/Hu+zQow?=
 =?us-ascii?Q?rnHqS59FC/eHbqBE0bM7D2lbSTFi9n95R+znb9/nTiJDNphob73X8avQ6+bY?=
 =?us-ascii?Q?1zAZTHquvivxn+1nsWyOxoKV3Ur7f6T438idB8rlSXJDqoPQIJpBSL2GoHhg?=
 =?us-ascii?Q?SRP/BuszNSnkMLOWGUtr76RkYnIuOYGwyQ85gMhjvjbzldsgewulQqLyaT73?=
 =?us-ascii?Q?rQX/DLpGYUXq20cFNNzHRR0twnBPU/laokv7rFdlZiccieygiNJmHj7sbcyF?=
 =?us-ascii?Q?F05EBV73qyiMZeCCr8/vIBFj4uMA4eMV3PYbvOlhyBLELB5Fq43//9KCY3Dk?=
 =?us-ascii?Q?S1K6glnpohzLTV9zLS9wDG9vWr5fFtSHi9YkMfGXWfctb/W/4mByg8t7T9pb?=
 =?us-ascii?Q?WdwZodxUqHRup2/VS3aRbUzCH5xOI1re+UPCEijaL/O6vQDcgzu8k7QUjWop?=
 =?us-ascii?Q?8xIvBA48b2chKunBhU6/eXvfo6YJOgBAQhV5H6r0OayRJxGtw53SmR+vf8ss?=
 =?us-ascii?Q?AqnoDUtXgSGblw6vEFbgtgdCfBK6shFK35N9PjM4mCpm8eZUK87hUMoea9so?=
 =?us-ascii?Q?ZQ4tR5Jmn7eND0rEgXn/AWKBoOnqbKKY8GfUwEsrhydPgV3UoS8eyz8rfXEC?=
 =?us-ascii?Q?QnxT2xiLvZHsej+oBnSOyrN/8cmdsvOCUUUknrPDQw0lkjvpX8aUPKsynlqR?=
 =?us-ascii?Q?OXwBZmYjdtKbdmZAkCDzK6zf6oftNmaxqordZRa745bJneThmXMSOQEsuP5o?=
 =?us-ascii?Q?jR9E64FL+/Fgw2jlA5jAJNOssnhMuL26AyMGxi8SfFZndzFgQwqcwArRw3tA?=
 =?us-ascii?Q?mAZU6V6rIgjkIJjU//742JabUlKsGWVJUTjAcfeuDWBuVIXzmyGtMVOJH4RE?=
 =?us-ascii?Q?JWVV563SmnmJMVj9ZX+ZrvRVFKaJqYyIi/XQBahVdBh4/NC3k87F04NMQ20j?=
 =?us-ascii?Q?5mICw8/gJdioRsO7aOCnsjiD4NRd6RYZNo9InGQkwvLgfdU9iL20eHlVChWT?=
 =?us-ascii?Q?W1NP6x1TBG3N5kPJpOPjf3yaAT7RRZ5GPoOk+1ATvxwJzFj8w9Yz31s9XKiH?=
 =?us-ascii?Q?odFfKRvsQheCq3TxByvY26QYmO5KsaZuKBnjVdr9kvsKxiBIL8GFMfg2XMcc?=
 =?us-ascii?Q?IB68QqmeddaIUg8lKcx535cz7Ttw+ve0reqkHd5W59CLQtlA2D2qkTabSOi0?=
 =?us-ascii?Q?k15qXSMcCzgWiPF4nnDM32C6nwy67V0FcJbLUK1JrWuEQI/B/JfoK8M+BHQZ?=
 =?us-ascii?Q?V/b43N6jN9/ANKZ58QAfg/Q6zEBy/Vpzfz8ZMnhad+u/bLMibNZNCEixcwEl?=
 =?us-ascii?Q?hIA3hHm1EeY+WHjIDjaTJJLvNn8CZikk+ALYVf5tXbdChfEJZIuoUhuv0/hY?=
 =?us-ascii?Q?JnmZ7swhez3fM7K02xelgQOEiO73j3poa8bOzhknZ5VxGFdrGWYEzgcNpPCA?=
 =?us-ascii?Q?ccPxc/hljP3aIROUf7xMMLwtcZFMJ7iRPHljhjGTZS6RgPTTJnSRbnvfGs6K?=
 =?us-ascii?Q?ZCHqZGHFooI+Dw9gWGwkRSto7Me5dPwuh7bRmPpsoqEuQ7MFRInPHSutxv5+?=
 =?us-ascii?Q?zw=3D=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 85d19c14-97ec-4a4b-c246-08da9ab3f588
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Sep 2022 02:58:13.9144
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: La653umvMtGovyiionsntINNvzAvflUuzqXrCtt/gHfZDkevG2Gl7GURtOs+M5OBa7QSgbtfktJDEoiEi7kjzexeiJ/J85do6Ac//CqgG1E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5104
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Sep 12, 2022 at 05:21:10PM +0000, Vladimir Oltean wrote:
> On Sun, Sep 11, 2022 at 01:02:44PM -0700, Colin Foster wrote:
> > +#include <linux/iopoll.h>
> > +#include <linux/mfd/ocelot.h>
> > +#include <linux/phylink.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <soc/mscc/ocelot_ana.h>
> > +#include <soc/mscc/ocelot_dev.h>
> > +#include <soc/mscc/ocelot_qsys.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <soc/mscc/ocelot_ptp.h>
> > +#include <soc/mscc/ocelot_sys.h>
> > +#include <soc/mscc/ocelot.h>
> > +#include <soc/mscc/vsc7514_regs.h>
> > +#include "felix.h"
> > +
> > +#define VSC7512_NUM_PORTS		11
> > +
> > +#define OCELOT_EXT_MEM_INIT_SLEEP_US	1000
> > +#define OCELOT_EXT_MEM_INIT_TIMEOUT_US	100000
> > +
> > +#define OCELOT_EXT_PORT_MODE_SERDES	(OCELOT_PORT_MODE_SGMII | \
> > +					 OCELOT_PORT_MODE_QSGMII)
> 
> There are places where OCELOT_EXT doesn't make too much sense, like here.
> The capabilities of the SERDES ports do not change depending on whether
> the switch is controlled externally or not. Same for the memory init
> delays. Maybe OCELOT_MEM_INIT_*, OCELOT_PORT_MODE_SERDES etc?
> 
> There are more places as well below in function names, I'll let you be
> the judge if whether ocelot is controlled externally is relevant to what
> they do in any way.
> 
> > +static int ocelot_ext_reset(struct ocelot *ocelot)
> > +{
> > +	int err, val;
> > +
> > +	ocelot_ext_reset_phys(ocelot);
> > +
> > +	/* Initialize chip memories */
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_ENA], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	err = regmap_field_write(ocelot->regfields[SYS_RESET_CFG_MEM_INIT], 1);
> > +	if (err)
> > +		return err;
> > +
> > +	/* MEM_INIT is a self-clearing bit. Wait for it to be clear (should be
> > +	 * 100us) before enabling the switch core
> > +	 */
> > +	err = readx_poll_timeout(ocelot_ext_mem_init_status, ocelot, val, !val,
> > +				 OCELOT_EXT_MEM_INIT_SLEEP_US,
> > +				 OCELOT_EXT_MEM_INIT_TIMEOUT_US);
> > +
> 
> I think you can eliminate the newline between the err assignment and
> checking for it.

In my upcoming v2 set, "ocelot_ext_reset" is moved to the shared
"ocelot_reset" routine. As such, iopoll.h isn't needed. And all
soc/mscc/ocelot_*.h includes aren't necessary either, since there are
literally zero register writes in ocelot_ext.c now.

I'll wait a couple days for everyone to go through their backlog. If
my "clean up ocelot_reset()" and your Documentation yaml patches get
approved, I'll be ready to send this out.