From owner-svn-src-all@freebsd.org Sat Dec 7 17:17:35 2019 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 247D51D75F7; Sat, 7 Dec 2019 17:17:35 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47Vbk70907z40Qt; Sat, 7 Dec 2019 17:17:35 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id D8A631D090; Sat, 7 Dec 2019 17:17:34 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id xB7HHYPs017016; Sat, 7 Dec 2019 17:17:34 GMT (envelope-from ian@FreeBSD.org) Received: (from ian@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id xB7HHY7D017013; Sat, 7 Dec 2019 17:17:34 GMT (envelope-from ian@FreeBSD.org) Message-Id: <201912071717.xB7HHY7D017013@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: ian set sender to ian@FreeBSD.org using -f From: Ian Lepore Date: Sat, 7 Dec 2019 17:17:34 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r355492 - in stable/12/sys/arm/ti: . am335x X-SVN-Group: stable-12 X-SVN-Commit-Author: ian X-SVN-Commit-Paths: in stable/12/sys/arm/ti: . am335x X-SVN-Commit-Revision: 355492 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Sat, 07 Dec 2019 17:17:35 -0000 Author: ian Date: Sat Dec 7 17:17:34 2019 New Revision: 355492 URL: https://svnweb.freebsd.org/changeset/base/355492 Log: MFC r352196, r352333, r352342, r353653 r352196: In am335x_dmtpps, use a spin mutex to interlock between PPS capture and PPS ioctl(2) handling. This allows doing the pps_event() work in the polling routine, instead of using a taskqueue task to do that work. Also, add PNPINFO, and switch to using make_dev_s() to create the cdev. Using a spin mutex and calling pps_event() from the polling function works around the situation which requires more than 2 sets of timecounter timehands in a single-core system to get reliable PPS capture. That problem would happen when a single-core system is idle in cpu_idle() then gets woken up with an event timer event which was scheduled to handle a hardclock tick. That processing path would end up calling tc_windup 3 or 4 times between when the tc polling function was called and when the taskqueue task would eventually run, and with only two sets of timehands, the th_generation count would always be too old to allow the captured PPS data to be used. r352333: Include , required to use spinlocks in this code. r352342: Make the ti_sysc device quiet. It's an internal utility pseudo-device that makes the upstream FDT data work right, so we don't need to see a couple dozen instances of it spam the dmesg at boot time unless it's a verbose boot. r353653: Update some comments; no functional changes. Some historical old comments in this driver indicate that the SD_CAPA register is write-once and after being set one time the values in it cannot be changed. That turns out not to be the case -- the values written to it survive a reset, but they can be rewritten/changed at any time. Modified: stable/12/sys/arm/ti/am335x/am335x_dmtpps.c stable/12/sys/arm/ti/ti_sdhci.c stable/12/sys/arm/ti/ti_sysc.c Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/arm/ti/am335x/am335x_dmtpps.c ============================================================================== --- stable/12/sys/arm/ti/am335x/am335x_dmtpps.c Sat Dec 7 17:14:33 2019 (r355491) +++ stable/12/sys/arm/ti/am335x/am335x_dmtpps.c Sat Dec 7 17:17:34 2019 (r355492) @@ -48,11 +48,11 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include #include #include #include -#include #include #include #include @@ -79,7 +79,6 @@ struct dmtpps_softc { uint32_t tclr; /* Cached TCLR register. */ struct timecounter tc; int pps_curmode; /* Edge mode now set in hw. */ - struct task pps_task; /* For pps_event handling. */ struct cdev * pps_cdev; struct pps_state pps_state; struct mtx pps_mtx; @@ -93,6 +92,7 @@ static struct ofw_compat_data compat_data[] = { {"ti,am335x-timer-1ms", 1}, {NULL, 0}, }; +SIMPLEBUS_PNP_INFO(compat_data); /* * A table relating pad names to the hardware timer number they can be mux'd to. @@ -285,48 +285,29 @@ dmtpps_poll(struct timecounter *tc) * populates it from the current DMT_TCRR register) with the latched * value from the TCAR1 register. * - * There is no locking here, by design. pps_capture() writes into an - * area of struct pps_state which is read only by pps_event(). The - * synchronization of access to that area is temporal rather than - * interlock based... we write in this routine and trigger the task that - * will read the data, so no simultaneous access can occur. - * * Note that we don't have the TCAR interrupt enabled, but the hardware * still provides the status bits in the "RAW" status register even when * they're masked from generating an irq. However, when clearing the * TCAR status to re-arm the capture for the next second, we have to * write to the IRQ status register, not the RAW register. Quirky. + * + * We do not need to hold a lock while capturing the pps data, because + * it is captured into an area of the pps_state struct which is read + * only by pps_event(). We do need to hold a lock while calling + * pps_event(), because it manipulates data which is also accessed from + * the ioctl(2) context by userland processes. */ if (DMTIMER_READ4(sc, DMT_IRQSTATUS_RAW) & DMT_IRQ_TCAR) { pps_capture(&sc->pps_state); sc->pps_state.capcount = DMTIMER_READ4(sc, DMT_TCAR1); DMTIMER_WRITE4(sc, DMT_IRQSTATUS, DMT_IRQ_TCAR); - taskqueue_enqueue(taskqueue_fast, &sc->pps_task); + + mtx_lock_spin(&sc->pps_mtx); + pps_event(&sc->pps_state, PPS_CAPTUREASSERT); + mtx_unlock_spin(&sc->pps_mtx); } } -static void -dmtpps_event(void *arg, int pending) -{ - struct dmtpps_softc *sc; - - sc = arg; - - /* This is the task function that gets enqueued by poll_pps. Once the - * time has been captured by the timecounter polling code which runs in - * primary interrupt context, the remaining (more expensive) work to - * process the event is done later in a threaded context. - * - * Here there is an interlock that protects the event data in struct - * pps_state. That data can be accessed at any time from userland via - * ioctl() calls so we must ensure that there is no read access to - * partially updated data while pps_event() does its work. - */ - mtx_lock(&sc->pps_mtx); - pps_event(&sc->pps_state, PPS_CAPTUREASSERT); - mtx_unlock(&sc->pps_mtx); -} - static int dmtpps_open(struct cdev *dev, int flags, int fmt, struct thread *td) @@ -374,9 +355,9 @@ dmtpps_ioctl(struct cdev *dev, u_long cmd, caddr_t dat sc = dev->si_drv1; /* Let the kernel do the heavy lifting for ioctl. */ - mtx_lock(&sc->pps_mtx); + mtx_lock_spin(&sc->pps_mtx); err = pps_ioctl(cmd, data, &sc->pps_state); - mtx_unlock(&sc->pps_mtx); + mtx_unlock_spin(&sc->pps_mtx); if (err != 0) return (err); @@ -436,6 +417,7 @@ static int dmtpps_attach(device_t dev) { struct dmtpps_softc *sc; + struct make_dev_args mda; clk_ident_t timer_id; int err, sysclk_freq; @@ -502,22 +484,27 @@ dmtpps_attach(device_t dev) * now, just say we can only capture assert events (the positive-going * edge of the pulse). */ - mtx_init(&sc->pps_mtx, "dmtpps", NULL, MTX_DEF); + mtx_init(&sc->pps_mtx, "dmtpps", NULL, MTX_SPIN); + sc->pps_state.flags = PPSFLAG_MTX_SPIN; sc->pps_state.ppscap = PPS_CAPTUREASSERT; sc->pps_state.driver_abi = PPS_ABI_VERSION; sc->pps_state.driver_mtx = &sc->pps_mtx; pps_init_abi(&sc->pps_state); - /* - * Init the task that does deferred pps_event() processing after - * the polling routine has captured a pps pulse time. - */ - TASK_INIT(&sc->pps_task, 0, dmtpps_event, sc); - /* Create the PPS cdev. */ - sc->pps_cdev = make_dev(&dmtpps_cdevsw, 0, UID_ROOT, GID_WHEEL, 0600, - PPS_CDEV_NAME); - sc->pps_cdev->si_drv1 = sc; + make_dev_args_init(&mda); + mda.mda_flags = MAKEDEV_WAITOK; + mda.mda_devsw = &dmtpps_cdevsw; + mda.mda_cr = NULL; + mda.mda_uid = UID_ROOT; + mda.mda_gid = GID_WHEEL; + mda.mda_mode = 0600; + mda.mda_unit = device_get_unit(dev); + mda.mda_si_drv1 = sc; + if ((err = make_dev_s(&mda, &sc->pps_cdev, PPS_CDEV_NAME)) != 0) { + device_printf(dev, "Failed to create cdev %s\n", PPS_CDEV_NAME); + return (err); + } if (bootverbose) device_printf(sc->dev, "Using %s for PPS device /dev/%s\n", Modified: stable/12/sys/arm/ti/ti_sdhci.c ============================================================================== --- stable/12/sys/arm/ti/ti_sdhci.c Sat Dec 7 17:14:33 2019 (r355491) +++ stable/12/sys/arm/ti/ti_sdhci.c Sat Dec 7 17:17:34 2019 (r355492) @@ -481,10 +481,10 @@ ti_sdhci_hw_init(device_t dev) /* * The attach() routine has examined fdt data and set flags in * slot.host.caps to reflect what voltages we can handle. Set those - * values in the CAPA register. The manual says that these values can - * only be set once, "before initialization" whatever that means, and - * that they survive a reset. So maybe doing this will be a no-op if - * u-boot has already initialized the hardware. + * values in the CAPA register. Empirical testing shows that the + * values in this register can be overwritten at any time, but the + * manual says that these values should only be set once, "before + * initialization" whatever that means, and that they survive a reset. */ regval = ti_mmchs_read_4(sc, MMCHS_SD_CAPA); if (sc->slot.host.caps & MMC_OCR_LOW_VOLTAGE) @@ -528,8 +528,7 @@ ti_sdhci_attach(device_t dev) * device, and only 1p8v on other devices unless an external transceiver * is used. The only way we could know about a transceiver is fdt data. * Note that we have to do this before calling ti_sdhci_hw_init() so - * that it can set the right values in the CAPA register, which can only - * be done once and never reset. + * that it can set the right values in the CAPA register. */ sc->slot.host.caps |= MMC_OCR_LOW_VOLTAGE; if (sc->mmchs_clk_id == MMC1_CLK || OF_hasprop(node, "ti,dual-volt")) { Modified: stable/12/sys/arm/ti/ti_sysc.c ============================================================================== --- stable/12/sys/arm/ti/ti_sysc.c Sat Dec 7 17:14:33 2019 (r355491) +++ stable/12/sys/arm/ti/ti_sysc.c Sat Dec 7 17:17:34 2019 (r355492) @@ -71,6 +71,9 @@ ti_sysc_probe(device_t dev) return (ENXIO); device_set_desc(dev, "TI SYSC Interconnect"); + if (!bootverbose) + device_quiet(dev); + return (BUS_PROBE_DEFAULT); }