From owner-svn-src-all@freebsd.org Wed Sep 2 17:51:44 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 91D9D3C0A3A for ; Wed, 2 Sep 2020 17:51:44 +0000 (UTC) (envelope-from mw@semihalf.com) Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BhWhv4gC9z3fKD for ; Wed, 2 Sep 2020 17:51:43 +0000 (UTC) (envelope-from mw@semihalf.com) Received: by mail-qk1-x744.google.com with SMTP id u3so488155qkd.9 for ; Wed, 02 Sep 2020 10:51:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=MFVil+lyegSDrvFxlAC1YBPsFrkXmC4aII509zx5OeU=; b=nOp9pzQdUyYlVgOxatuutDkl8ZKONEu7fQnfpG21PeqpeVwG1VSGT6H1J+scNL5dDe aOJIUz3xkaDONm4vAKxtKJBNdyAekkHyLROpTn+xOwK7Xt1tLCcbKfRGMZbPFbXIFf/H POI5NcmsgIgBhViZujxSqKSCHilMB+pfolCouIkHNzk1IK+IkStr/xNu2d8jnEY4sSBE rnKOZMeMEh6H9NwKpuIC5UK8nXJDwZD1+CFtTHEZExl/USIFnCnzTAGHe4AbbzwOH/U7 WVWxfXMg3/cFmf9bUAeOMRJDr+yK9l1a/uRLzbh7n/sR1C+Ur7mSrU6AQf3zJQwrq9uS vCHg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=MFVil+lyegSDrvFxlAC1YBPsFrkXmC4aII509zx5OeU=; b=m7eDyGEc3NV53rB+gk1UzVJOZV4TWJkd3JjnQbjFABFlXeV6l6QP1ek6fV545HQ26M KAllmFAuQ0vFLGl+EODA6CGe1sFIZeEPvAmhGKtDZC1xwqVsJQIH2ST0ZbPaj6QrWSQa Ulqqs6fuoiHdMwMz90Q1UM8jLHnw6yKzsiofhb/ihHle3xTXGYUGQq+K2oYGi68xzAY8 C1ec31CLWQQk2512Q16Y7Rdd7g9n/4DnfLtAZCbGSQxPh1Er4hefMHOYDlJsjvNyeYxa 6pONa8RMXReEPVcDJpCUQJhM7o18xXVV6VAlom8IciiTd4Tzh+v3tqxTM/OCOP2QzRZO crBg== X-Gm-Message-State: AOAM531KwpBVbfCGHM/vKEUc5ISt7PwGWmjt9wXm0M895ACLawjIzFub gEGyu0aYYlHzQm9sjkr9cF5277e0jNwLAoT1QrrFwg== X-Google-Smtp-Source: ABdhPJyx068SmGaQqC8GYL8oViwjFi77vMEdcv7yXhrgVr/9ZL4yij/W7XEISuiZBPOZByBcSYHYYgADJ5Y1cC2chUw= X-Received: by 2002:a37:4c15:: with SMTP id z21mr7644646qka.334.1599069102133; Wed, 02 Sep 2020 10:51:42 -0700 (PDT) MIME-Version: 1.0 References: <202009011617.081GHL8e031671@repo.freebsd.org> In-Reply-To: From: Marcin Wojtas Date: Wed, 2 Sep 2020 19:51:29 +0200 Message-ID: Subject: Re: svn commit: r365054 - in head/sys: conf dev/sdhci To: Justin Hibbits Cc: Marcin Wojtas , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org, Artur Rojek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4BhWhv4gC9z3fKD X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=nOp9pzQd; dmarc=none; spf=none (mx1.freebsd.org: domain of mw@semihalf.com has no SPF policy when checking 2607:f8b0:4864:20::744) smtp.mailfrom=mw@semihalf.com X-Spamd-Result: default: False [-2.47 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[semihalf-com.20150623.gappssmtp.com:s=20150623]; FREEFALL_USER(0.00)[mw]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_MEDIUM(-0.97)[-0.969]; NEURAL_HAM_LONG(-1.04)[-1.044]; MIME_GOOD(-0.10)[text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-all@freebsd.org]; DMARC_NA(0.00)[semihalf.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[semihalf-com.20150623.gappssmtp.com:+]; NEURAL_HAM_SHORT(-0.16)[-0.161]; RCPT_COUNT_SEVEN(0.00)[7]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::744:from]; R_SPF_NA(0.00)[no SPF record]; FREEMAIL_TO(0.00)[gmail.com]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; MAILMAN_DEST(0.00)[svn-src-all] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Sep 2020 17:51:44 -0000 Hi Justin, Thanks for your input. Please see inline. wt., 1 wrz 2020 o 23:30 Justin Hibbits napisa=C5=82(= a): > > Sep 1, 2020 11:17:35 Marcin Wojtas : > > > Author: mw > > Date: Tue Sep 1 16:17:21 2020 > > New Revision: 365054 > > URL: https://svnweb.freebsd.org/changeset/base/365054 > > > > Log: > > Introduce the SDHCI driver for NXP QorIQ Layerscape SoCs > > > > Implement support for an eSDHC controller found in NXP QorIQ Layerscape= SoCs. > > > > This driver has been tested with NXP LS1046A and LX2160A (Honeycomb boa= rd), > > which is incompatible with the existing sdhci_fsl driver (aiming at old= er > > chips from this family). As such, it is not intended as replacement for > > the old driver, but rather serves as an improved alternative for SoCs t= hat > > support it. > > It comes with support for both PIO and Single DMA modes and samples the > > clock from the extres clk API. > > How is it incompatible? > > > > > Submitted by: Artur Rojek > > Reviewed by: manu, mmel, kibab > > Obtained from: Semihalf > > Sponsored by: Alstom Group > > Differential Revision: https://reviews.freebsd.org/D26153 > > > > Added: > > head/sys/dev/sdhci/sdhci_fsl_fdt.c (contents, props changed) > > The name choice here is odd, given there is already fsl_sdhci.c > > > Modified: > > head/sys/conf/files > > > > Modified: head/sys/conf/files > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > --- head/sys/conf/files Tue Sep 1 16:13:09 2020 (r365053) > > +++ head/sys/conf/files Tue Sep 1 16:17:21 2020 (r365054) > > @@ -3058,6 +3058,7 @@ dev/scc/scc_dev_z8530.c optional scc > > dev/sdhci/sdhci.c optional sdhci > > dev/sdhci/sdhci_fdt.c optional sdhci fdt > > dev/sdhci/sdhci_fdt_gpio.c optional sdhci fdt gpio > > +dev/sdhci/sdhci_fsl_fdt.c optional sdhci fdt gpio > > dev/sdhci/sdhci_if.m optional sdhci > > dev/sdhci/sdhci_acpi.c optional sdhci acpi > > dev/sdhci/sdhci_pci.c optional sdhci pci > > > > Added: head/sys/dev/sdhci/sdhci_fsl_fdt.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > > +++ head/sys/dev/sdhci/sdhci_fsl_fdt.c Tue Sep 1 16:17:21 2020 (r365= 054) > > @@ -0,0 +1,680 @@ > > +/*- > > + * SPDX-License-Identifier: BSD-2-Clause-FreeBSD > > + * > > + * Copyright (c) 2020 Alstom Group. > > + * Copyright (c) 2020 Semihalf. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * 1. Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * 2. Redistributions in binary form must reproduce the above copyrigh= t > > + * notice, this list of conditions and the following disclaimer in = the > > + * documentation and/or other materials provided with the distribut= ion. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' = AND > > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, T= HE > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR = PURPOSE > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LI= ABLE > > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQ= UENTIAL > > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE G= OODS > > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTIO= N) > > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,= STRICT > > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN A= NY WAY > > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY= OF > > + * SUCH DAMAGE. > > + */ > > + > > +/* eSDHC controller driver for NXP QorIQ Layerscape SoCs. */ > > + > > +#include > > +__FBSDID("$FreeBSD$"); > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "mmcbr_if.h" > > +#include "sdhci_if.h" > > + > > +#define RD4 (sc->read) > > +#define WR4 (sc->write) > > + > > +#define SDHCI_FSL_PRES_STATE 0x24 > > +#define SDHCI_FSL_PRES_SDSTB (1 << 3) > > +#define SDHCI_FSL_PRES_COMPAT_MASK 0x000f0f07 > > + > > +#define SDHCI_FSL_PROT_CTRL 0x28 > > +#define SDHCI_FSL_PROT_CTRL_WIDTH_1BIT (0 << 1) > > +#define SDHCI_FSL_PROT_CTRL_WIDTH_4BIT (1 << 1) > > +#define SDHCI_FSL_PROT_CTRL_WIDTH_8BIT (2 << 1) > > +#define SDHCI_FSL_PROT_CTRL_WIDTH_MASK (3 << 1) > > +#define SDHCI_FSL_PROT_CTRL_BYTE_SWAP (0 << 4) > > +#define SDHCI_FSL_PROT_CTRL_BYTE_NATIVE (2 << 4) > > +#define SDHCI_FSL_PROT_CTRL_BYTE_MASK (3 << 4) > > +#define SDHCI_FSL_PROT_CTRL_DMA_MASK (3 << 8) > > + > > +#define SDHCI_FSL_SYS_CTRL 0x2c > > +#define SDHCI_FSL_CLK_IPGEN (1 << 0) > > +#define SDHCI_FSL_CLK_SDCLKEN (1 << 3) > > +#define SDHCI_FSL_CLK_DIVIDER_MASK 0x000000f0 > > +#define SDHCI_FSL_CLK_DIVIDER_SHIFT 4 > > +#define SDHCI_FSL_CLK_PRESCALE_MASK 0x0000ff00 > > +#define SDHCI_FSL_CLK_PRESCALE_SHIFT 8 > > + > > +#define SDHCI_FSL_WTMK_LVL 0x44 > > +#define SDHCI_FSL_WTMK_RD_512B (0 << 0) > > +#define SDHCI_FSL_WTMK_WR_512B (0 << 15) > > + > > +#define SDHCI_FSL_HOST_VERSION 0xfc > > +#define SDHCI_FSL_CAPABILITIES2 0x114 > > + > > +#define SDHCI_FSL_ESDHC_CTRL 0x40c > > +#define SDHCI_FSL_ESDHC_CTRL_SNOOP (1 << 6) > > +#define SDHCI_FSL_ESDHC_CTRL_CLK_DIV2 (1 << 19) > > + > > +struct sdhci_fsl_fdt_softc { > > + device_t dev; > > + const struct sdhci_fsl_fdt_soc_data *soc_data; > > + struct resource *mem_res; > > + struct resource *irq_res; > > + void *irq_cookie; > > + uint32_t baseclk_hz; > > + struct sdhci_fdt_gpio *gpio; > > + struct sdhci_slot slot; > > + bool slot_init_done; > > + uint32_t cmd_and_mode; > > + uint16_t sdclk_bits; > > + > > + uint32_t (* read)(struct sdhci_fsl_fdt_softc *, bus_size_t); > > + void (* write)(struct sdhci_fsl_fdt_softc *, bus_size_t, uint32_t); > > +}; > > + > > +struct sdhci_fsl_fdt_soc_data { > > + int quirks; > > +}; > > + > > +static const struct sdhci_fsl_fdt_soc_data sdhci_fsl_fdt_ls1046a_soc_d= ata =3D { > > + .quirks =3D SDHCI_QUIRK_DONT_SET_HISPD_BIT | SDHCI_QUIRK_BROKEN_AUTO_= STOP > > +}; > > + > > +static const struct sdhci_fsl_fdt_soc_data sdhci_fsl_fdt_gen_data =3D = { > > + .quirks =3D 0, > > +}; > > + > > +static const struct ofw_compat_data sdhci_fsl_fdt_compat_data[] =3D { > > + {"fsl,ls1046a-esdhc", (uintptr_t)&sdhci_fsl_fdt_ls1046a_soc_data}, > > + {"fsl,esdhc", (uintptr_t)&sdhci_fsl_fdt_gen_data}, > > + {NULL, 0} > > +}; > > The existing driver is compatible with fsl,esdhc. How are you preventing= collisions? The pure "fsl,esdhc" compatibility jumped in during review, as it was tested with the LX2K SoC by mmel. Frankly we didn't expect it would work without modifications, but I get your point and agree, this now may lead to confusion now. > > Why couldn't you make these changes to the existing driver? I see some i= mprovements in this one that would be nice to have in the other driver. To shed some light on the decision - the fsl_sdhci.c was initially considered, but we had following issues, causing the bring-up debug extremely painful (especially in terms of our DMA enablement requirement from our customer): * ext_resources * __arm__ and __powerpc__ ifdefs * fixed quirks and other hardcoded settings in attach * Another level of complexity is added by the USDHC/ESDHC Introducing the __aarch64__ and EXT_RESOURCES ifdef level would cause even more tangled and illegible code. We also did not have full clarity, whether we can consider the IP as 1:1, as the work was done on the LS1046A. Therefore we decided to do a clean version of the driver supporting ESDHC for ARM64-based NXP LayerScape SoCs (which eventually and surprisingly to us worked out of the box with "fsl,esdhc" on the LX2K...). On the other hand in the new driver there is a slightly improved endiannes handling, which may suggest that it would be easy to try the powerpc QoriQ machine with the "fsl,esdhc" (with only hack to avoid the ext_resources). Now I am wondering what to do with this (keeping required effort aside for now), e.g.: * update and switch to fsl_sdhci.c fully (likely to cause regressions, we also don't have iMX/QoriQ platforms to test) * switch partially - use the ext_resources conditionally and start to use at least some platforms that prove to work fine with the new driver, gaining the SDMA support * possibly rename and move build enablement only to files.arm64, depending additionally on the SOC_NXP_LS option * some other solution? Looking forward to your feedback. Best regards, Marcin