Re: git: 2a9021898c4e - main - sff: Add SFP driver (fdt-based draft)

Reply: Jessica Clarke : "Re: git: 2a9021898c4e - main - sff: Add SFP driver (fdt-based draft)"
In reply to: Jessica Clarke : "Re: git: 2a9021898c4e - main - sff: Add SFP driver (fdt-based draft)"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Dmitry Salychev <dsl_at_FreeBSD.org>
Date: Fri, 18 Aug 2023 17:18:20 UTC
Jessica Clarke <jrtc27@freebsd.org> writes:

> On 18 Aug 2023, at 11:40, Dmitry Salychev <dsl@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by dsl:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=2a9021898c4ee2154787da862c238cfeccd655df
>> 
>> commit 2a9021898c4ee2154787da862c238cfeccd655df
>> Author:     Dmitry Salychev <dsl@FreeBSD.org>
>> AuthorDate: 2023-08-18 09:17:31 +0000
>> Commit:     Dmitry Salychev <dsl@FreeBSD.org>
>> CommitDate: 2023-08-18 10:40:11 +0000
>> 
>>    sff: Add SFP driver (fdt-based draft)
>> 
>>    This basic version of the driver obtains properties of the "sff,sfp"
>>    compatible devices and implements a simple interface to provide an I2C
>>    bus device for the rest of the drivers (e.g. to implement SIOCGI2C).
>> 
>>    Both of the interface and driver are subjects for a further
>>    generalization to be used in case of non-FDT and non-arm64 platforms.
>> 
>>    Reviewed by:            bz, manu
>>    Approved by:            bz (mentor)
>>    MFC after:              3 weeks
>>    Differential Revision:  https://reviews.freebsd.org/D41440
>> ---
>> sys/arm64/conf/std.nxp   |   3 +
>> sys/conf/files           |   2 +
>> sys/dev/sff/sff_if.m     |  35 +++++++++++
>> sys/dev/sff/sfp_fdt.c    | 155 +++++++++++++++++++++++++++++++++++++++++++++++
>> sys/modules/Makefile     |   2 +
>> sys/modules/sff/Makefile |  13 ++++
>> 6 files changed, 210 insertions(+)
>> 
>> diff --git a/sys/arm64/conf/std.nxp b/sys/arm64/conf/std.nxp
>> index 5b2e2b52d4e6..b4552fadaff4 100644
>> --- a/sys/arm64/conf/std.nxp
>> +++ b/sys/arm64/conf/std.nxp
>> @@ -25,6 +25,9 @@ device sdhci
>> device dpaa2 # Data Path Acceleration Architecture (2nd Gen)
>> device enetc # QorIQ LS1028A NIC
>> 
>> +# SFF/SFP
>> +device sff # Small Form Factor Transceivers
>> +
>> options FDT
>> device acpi
>> 
>> diff --git a/sys/conf/files b/sys/conf/files
>> index 0db5887e6a75..b5cd85cba0e4 100644
>> --- a/sys/conf/files
>> +++ b/sys/conf/files
>> @@ -3044,6 +3044,8 @@ dev/sdhci/sdhci_pci.c optional sdhci pci
>> dev/sdio/sdio_if.m optional mmccam
>> dev/sdio/sdio_subr.c optional mmccam
>> dev/sdio/sdiob.c optional mmccam
>> +dev/sff/sff_if.m optional sff
>> +dev/sff/sfp_fdt.c optional sff fdt
>> dev/sge/if_sge.c optional sge pci
>> dev/siis/siis.c optional siis pci
>> dev/sis/if_sis.c optional sis pci
>> diff --git a/sys/dev/sff/sff_if.m b/sys/dev/sff/sff_if.m
>> new file mode 100644
>> index 000000000000..823e557992c2
>> --- /dev/null
>> +++ b/sys/dev/sff/sff_if.m
>> @@ -0,0 +1,35 @@
>> +#-
>> +# SPDX-License-Identifier: BSD-2-Clause
>> +#
>> +# Copyright © 2023 Dmitry Salychev
>> +#
>> +# 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.
>> +#
>> +
>> +#include <machine/bus.h>
>> +
>> +INTERFACE sff;
>> +
>> +METHOD int get_i2c_bus {
>> + device_t dev;
>> + device_t *i2c_bus;
>> +};
>> diff --git a/sys/dev/sff/sfp_fdt.c b/sys/dev/sff/sfp_fdt.c
>> new file mode 100644
>> index 000000000000..7430282ede70
>> --- /dev/null
>> +++ b/sys/dev/sff/sfp_fdt.c
>> @@ -0,0 +1,155 @@
>> +/*-
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Copyright © 2023 Dmitry Salychev
>> + *
>> + * 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.
>> + */
>> +
>> +/*
>> + * Small Form Factor (SFF) Committee Pluggable (SFP) Transceiver (FDT-based).
>> + */
>> +
>> +#include <sys/param.h>
>> +#include <sys/kernel.h>
>> +#include <sys/bus.h>
>> +#include <sys/module.h>
>> +
>> +#include <dev/ofw/ofw_bus.h>
>> +#include <dev/ofw/ofw_bus_subr.h>
>> +#include <dev/fdt/simplebus.h>
>> +
>> +#include "sff_if.h"
>> +
>> +struct sfp_fdt_softc {
>> + phandle_t ofw_node;
>> + phandle_t i2c_bus;
>> +
>> + phandle_t mod_def;
>> + phandle_t los;
>> + phandle_t tx_fault;
>> + phandle_t tx_disable;
>> + phandle_t rx_rate;
>> + phandle_t tx_rate;
>
> These names sound like they should be numbers, not handles to GPIOs.
>

FDT example:

sfp_xg1: dpmac1-sfp {
        compatible = "sff,sfp";
        i2c-bus = <&sfpupper_i2c>;
        tx-fault-gpios = <&sfpgpio 4 GPIO_ACTIVE_HIGH>;
        tx-disable-gpios = <&sfpgpio 5 GPIO_ACTIVE_HIGH>;
        mod-def0-gpios = <&sfpgpio 6 GPIO_ACTIVE_LOW>;
        los-gpios = <&sfpgpio 7 GPIO_ACTIVE_HIGH>;
        maximum-power-milliwatt = <2000>;
};

>> + uint32_t max_power; /* in mW */
>> +};
>> +
>> +static int
>> +sfp_fdt_probe(device_t dev)
>> +{
>> + phandle_t node;
>> + ssize_t s;
>> +
>> + node = ofw_bus_get_node(dev);
>> + if (!ofw_bus_node_is_compatible(node, "sff,sfp"))
>> + return (ENXIO);
>> +
>> + s = device_get_property(dev, "i2c-bus", &node, sizeof(node),
>> +    DEVICE_PROP_HANDLE);
>> + if (s == -1) {
>> + device_printf(dev, "%s: '%s' has no 'i2c-bus' property, s %zd\n",
>> +    __func__, ofw_bus_get_name(dev), s);
>
> What’s the point of printing s when it’s known to be -1?
>

The only reason is to check the watchful eye of the project, of course :)

>> + return (ENXIO);
>> + }
>> +
>> + device_set_desc(dev, "Small Form-factor Pluggable Transceiver");
>> + return (BUS_PROBE_DEFAULT);
>> +}
>> +
>> +static int
>> +sfp_fdt_attach(device_t dev)
>> +{
>> + struct sfp_fdt_softc *sc;
>> + ssize_t s;
>> + int error;
>> +
>> + sc = device_get_softc(dev);
>> + sc->ofw_node = ofw_bus_get_node(dev);
>> +
>> + s = device_get_property(dev, "i2c-bus", &sc->i2c_bus,
>> +    sizeof(sc->i2c_bus), DEVICE_PROP_HANDLE);
>> + if (s == -1) {
>> + device_printf(dev, "%s: cannot find 'i2c-bus' property: %zd\n",
>> +    __func__, s);
>
> Ditto.
>
>> + return (ENXIO);
>> + }
>> +
>> + /* Optional properties */
>> + (void)device_get_property(dev, "mod-def0-gpios", &sc->mod_def,
>> +    sizeof(sc->mod_def), DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "los-gpios", &sc->los, sizeof(sc->los),
>> +    DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "tx-fault-gpios", &sc->tx_fault,
>> +    sizeof(sc->tx_fault), DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "tx-disable-gpios", &sc->tx_disable,
>> +    sizeof(sc->tx_disable), DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "rate-select0-gpios", &sc->rx_rate,
>> +    sizeof(sc->rx_rate), DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "rate-select1-gpios", &sc->tx_rate,
>> +    sizeof(sc->tx_rate), DEVICE_PROP_HANDLE);
>> + (void)device_get_property(dev, "maximum-power-milliwatt", &sc->max_power,
>> +    sizeof(sc->max_power), DEVICE_PROP_UINT32);
>> +
>> + error = OF_device_register_xref(OF_xref_from_node(sc->ofw_node), dev);
>> + if (error != 0)
>> + device_printf(dev, "%s: failed to register xref %#x\n",
>> +    __func__, OF_xref_from_node(sc->ofw_node));
>
> : %d, error?

Good catch, thanks!

>
> Jess
>
>> +
>> + return (error);
>> +}
>> +
>> +static int
>> +sfp_fdt_get_i2c_bus(device_t dev, device_t *i2c_bus)
>> +{
>> + struct sfp_fdt_softc *sc;
>> + device_t xdev;
>> +
>> + KASSERT(i2c_bus != NULL, ("%s: i2c_bus is NULL", __func__));
>> +
>> + sc = device_get_softc(dev);
>> + xdev = OF_device_from_xref(OF_xref_from_node(sc->i2c_bus));
>> + if (xdev == NULL)
>> + return (ENXIO);
>> +
>> + *i2c_bus = xdev;
>> + return (0);
>> +}
>> +
>> +static device_method_t sfp_fdt_methods[] = {
>> + /* Device interface */
>> + DEVMETHOD(device_probe, sfp_fdt_probe),
>> + DEVMETHOD(device_attach, sfp_fdt_attach),
>> + DEVMETHOD(device_detach, bus_generic_detach),
>> +
>> + /* SFF */
>> + DEVMETHOD(sff_get_i2c_bus, sfp_fdt_get_i2c_bus),
>> +
>> + DEVMETHOD_END
>> +};
>> +
>> +DEFINE_CLASS_0(sfp_fdt, sfp_fdt_driver, sfp_fdt_methods,
>> +    sizeof(struct sfp_fdt_softc));
>> +
>> +EARLY_DRIVER_MODULE(sfp_fdt, simplebus, sfp_fdt_driver, 0, 0,
>> +    BUS_PASS_SUPPORTDEV);
>> +EARLY_DRIVER_MODULE(sfp_fdt, ofwbus, sfp_fdt_driver, 0, 0,
>> +    BUS_PASS_SUPPORTDEV);
>> diff --git a/sys/modules/Makefile b/sys/modules/Makefile
>> index 201cfbcca725..4b98c7ed6e0d 100644
>> --- a/sys/modules/Makefile
>> +++ b/sys/modules/Makefile
>> @@ -355,6 +355,7 @@ SUBDIR= \
>> ${_sdhci_fdt} \
>> sdhci_pci \
>> sdio \
>> + ${_sff} \
>> sem \
>> send \
>> ${_sfxge} \
>> @@ -678,6 +679,7 @@ _cxgb= cxgb
>> .if ${MACHINE_CPUARCH} == "aarch64"
>> _armv8crypto= armv8crypto
>> _dpaa2= dpaa2
>> +_sff= sff
>> _em= em
>> _hyperv=  hyperv
>> 
>> diff --git a/sys/modules/sff/Makefile b/sys/modules/sff/Makefile
>> new file mode 100644
>> index 000000000000..96832070de63
>> --- /dev/null
>> +++ b/sys/modules/sff/Makefile
>> @@ -0,0 +1,13 @@
>> +.PATH: ${SRCTOP}/sys/dev/sff
>> +
>> +KMOD= sff
>> +
>> +SRCS+= sff_if.c sff_if.h
>> +SRCS+= bus_if.h device_if.h
>> +
>> +.if !empty(OPT_FDT)
>> +SRCS+= sfp_fdt.c \
>> + ofw_bus_if.h
>> +.endif
>> +
>> +.include <bsd.kmod.mk>

Regards,
Dmitry

-- 
https://wiki.freebsd.org/DmitrySalychev