Date: Wed, 2 Sep 2020 19:51:29 +0200 From: Marcin Wojtas <mw@semihalf.com> To: Justin Hibbits <chmeeedalf@gmail.com> Cc: Marcin Wojtas <mw@freebsd.org>, Warner Losh <imp@bsdimp.com>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Artur Rojek <ar@semihalf.com> Subject: Re: svn commit: r365054 - in head/sys: conf dev/sdhci Message-ID: <CAPv3WKdCboE2YyWUET6hBZiX5%2B_XdZo-8QhgY1WUUOccrmcwMQ@mail.gmail.com> In-Reply-To: <b41cdaec-c572-4ff1-94c9-7b399dec9fd0@gmail.com> References: <202009011617.081GHL8e031671@repo.freebsd.org> <b41cdaec-c572-4ff1-94c9-7b399dec9fd0@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Justin, Thanks for your input. Please see inline. wt., 1 wrz 2020 o 23:30 Justin Hibbits <chmeeedalf@gmail.com> napisa=C5=82(= a): > > Sep 1, 2020 11:17:35 Marcin Wojtas <mw@FreeBSD.org>: > > > 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 <ar@semihalf.com> > > 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 <sys/cdefs.h> > > +__FBSDID("$FreeBSD$"); > > + > > +#include <sys/param.h> > > +#include <sys/endian.h> > > +#include <sys/kernel.h> > > +#include <sys/module.h> > > +#include <sys/rman.h> > > +#include <sys/sysctl.h> > > +#include <sys/taskqueue.h> > > + > > +#include <machine/bus.h> > > +#include <machine/resource.h> > > + > > +#include <dev/extres/clk/clk.h> > > +#include <dev/mmc/bridge.h> > > +#include <dev/mmc/mmcbrvar.h> > > +#include <dev/ofw/ofw_bus.h> > > +#include <dev/ofw/ofw_bus_subr.h> > > +#include <dev/sdhci/sdhci.h> > > +#include <dev/sdhci/sdhci_fdt_gpio.h> > > + > > +#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPv3WKdCboE2YyWUET6hBZiX5%2B_XdZo-8QhgY1WUUOccrmcwMQ>