svn commit: r365054 - in head/sys: conf dev/sdhci
Marcin Wojtas
mw at semihalf.com
Wed Sep 2 17:51:45 UTC 2020
Hi Justin,
Thanks for your input. Please see inline.
wt., 1 wrz 2020 o 23:30 Justin Hibbits <chmeeedalf at gmail.com> napisał(a):
>
> Sep 1, 2020 11:17:35 Marcin Wojtas <mw at 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 board),
> > which is incompatible with the existing sdhci_fsl driver (aiming at older
> > 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 that
> > 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 at 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
> > ==============================================================================
> > --- 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
> > ==============================================================================
> > --- /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 (r365054)
> > @@ -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 copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY 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_data = {
> > + .quirks = SDHCI_QUIRK_DONT_SET_HISPD_BIT | SDHCI_QUIRK_BROKEN_AUTO_STOP
> > +};
> > +
> > +static const struct sdhci_fsl_fdt_soc_data sdhci_fsl_fdt_gen_data = {
> > + .quirks = 0,
> > +};
> > +
> > +static const struct ofw_compat_data sdhci_fsl_fdt_compat_data[] = {
> > + {"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 improvements 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
More information about the svn-src-head
mailing list