| Message ID | 1454884753-4560-4-git-send-email-helmut.buchsbaum@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 07/02/2016 14:39, Helmut Buchsbaum wrote: > Since several use cases need to setup at least some basic control > registers add the ability to configure an array containing such > register initialization values within the platform data of the switch. > Furthermore expose this capabilty to the devicetree. > > Platform data now contains a pointer to an array and the array length > where each member contains the register to be initialized, the > initialization value and a register mask, since in many use cases there > is only the need to init some bits of a register, e.g. disabling unused > ports. > > The devicetree notation add the property 'settings' to the SPI node of the > ks8985 driver, which is a list of triple values (register, value, mask), > e.g.: > > settings = <0x4D 0x08 0x08 > 0x5D 0x08 0x08>; You encode way too much in the Device Tree that should be knowledge to the driver on how to configure the switch. This is very tempting, because you do not dictate any use case and let people define it based on their Device Tree source, but at the same time, this is very error prone and does not provide what a proper device driver needs to be doing by defining a standard and predictable behavior. Right now this driver is a PHY driver, but it should be moved to a DSA driver eventually such that each port is exposed as a network interface, and you have hooks to power on/off ports based on whether a corresponding network interface is up/down. -- Florian
On 02/08/2016 05:38 AM, Florian Fainelli wrote: > On 07/02/2016 14:39, Helmut Buchsbaum wrote: >> Since several use cases need to setup at least some basic control >> registers add the ability to configure an array containing such >> register initialization values within the platform data of the switch. >> Furthermore expose this capabilty to the devicetree. >> >> Platform data now contains a pointer to an array and the array length >> where each member contains the register to be initialized, the >> initialization value and a register mask, since in many use cases there >> is only the need to init some bits of a register, e.g. disabling unused >> ports. >> >> The devicetree notation add the property 'settings' to the SPI node of the >> ks8985 driver, which is a list of triple values (register, value, mask), >> e.g.: >> >> settings = <0x4D 0x08 0x08 >> 0x5D 0x08 0x08>; > > You encode way too much in the Device Tree that should be knowledge to > the driver on how to configure the switch. This is very tempting, > because you do not dictate any use case and let people define it based > on their Device Tree source, but at the same time, this is very error > prone and does not provide what a proper device driver needs to be doing > by defining a standard and predictable behavior. > > Right now this driver is a PHY driver, but it should be moved to a DSA > driver eventually such that each port is exposed as a network interface, > and you have hooks to power on/off ports based on whether a > corresponding network interface is up/down. > -- > Florian > The way I built these initialization settings was inspired by the way it is done in the pinctrl subsystem: there you also configure the pin functions in a very hardware specific way (dependent on the underlying pinctrl hardware). Thus this was just extending a principle we can find in other subsystems of the kernel to this driver. Furthermore the register interface is already exposed to the user space via sysfs, which, in my opinion, is even more error prone then setting up the Device Tree carefully. Nevertheless, I can perfectly understand your point of view. This is just what thought when I saw all registers are accessible from user space! At the moment I use this driver with a KSZ8795CLX, port 5 directly connected to a MACB/GEM of a Zynq SOC, with the need to enable the RGMII internal clock delay (register 0x56, bit 4), otherwise the the Zynq cannot talk to the switch on its RGMII interface (being able to switch off unused ports is just a nice add-on I use). Using the sysfs capabilities of this driver might be an alternative, but contradicts our requirement to set up the network interfaces as fast as possible. Furthermore stuff like IP_PNP or nfs root won't work. But maybe I should try to move this kind of basic setup to bootloader - I'll investigate this idea! Since I'm not at all (yet) familiar with the DSA subsystem I wonder how I could manage setting the clock delay bit with DSA. Would this be a driver specific setting or can it be fulfilled within the subsystem? Since I still want to share my work for the PHY only driver, is it ok if I'll resend the patch series just without part 3 to get support for the KSZ8795? Let's talk about the part 3 functionality and moving the driver to DSA separately! BTW, are there any additional links about DSA complementing the kernel documentation? Thanks for your comments, Helmut
> At the moment I use this driver with a KSZ8795CLX, port 5 directly > connected to a MACB/GEM of a Zynq SOC, with the need to enable the > RGMII internal clock delay (register 0x56, bit 4), otherwise the > the Zynq cannot talk to the switch on its RGMII interface Hi Helmut This is possible with DSA. Documentation/devicetree/bindings/net/dsa/dsa.txt says you can include a phy-mode setting. phy-mode is defined in Documentation/devicetree/bindings/net/ethernet.txt and includes "rgmii-id", "rgmii-rxid", "rgmii-txid" which control these delays. Andrew
diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c index 2803c8e..d50f091 100644 --- a/drivers/net/phy/spi_ks8995.c +++ b/drivers/net/phy/spi_ks8995.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/of.h> #include <linux/spi/spi.h> @@ -120,7 +121,12 @@ static const struct ks8995_chip_params ks8995_chip[] = { }; struct ks8995_pdata { - /* not yet implemented */ + struct reg_init { + int reg; + int val; + int mask; + } *settings; + int nsettings; }; struct ks8995_switch { @@ -235,6 +241,47 @@ static int ks8995_start(struct ks8995_switch *ks) return ks8995_write_reg(ks, KS8995_REG_ID1, 1); } +/* ks8995_register_init - initialize registers + * @ks: pointer to switch instance + * + * Initialize registers from platform data if available. + */ +static int ks8995_register_init(struct ks8995_switch *ks) +{ + int i; + int err = 0; + struct ks8995_pdata *pdata = ks->pdata; + + if (!pdata || !pdata->settings) + return 0; + + for (i = 0; i < pdata->nsettings; ++i) { + u8 oldval, newval; + struct reg_init *s = &pdata->settings[i]; + + err = ks8995_read_reg(ks, s->reg, &oldval); + if (err) { + dev_err(&ks->spi->dev, + "Reading register 0x%02x failed\n", + s->reg); + err = -EIO; + break; + } + + oldval &= ~s->mask; + newval = oldval | (s->val & s->mask); + + err = ks8995_write_reg(ks, s->reg, newval); + if (err) { + dev_err(&ks->spi->dev, + "Writing register 0x%02x failed\n", + s->reg); + err = -EIO; + break; + } + } + return err; +} static int ks8995_reset(struct ks8995_switch *ks) { int err; @@ -245,6 +292,10 @@ static int ks8995_reset(struct ks8995_switch *ks) udelay(KS8995_RESET_DELAY); + err = ks8995_register_init(ks); + if (err) + return err; + return ks8995_start(ks); } @@ -339,6 +390,64 @@ err_out: return err; } +/* ks8995_parse_dt - setup platform data from devicetree + * @ks: pointer to switch instance + * + * Parses supported DT properties and sets up platform data + * accordingly. + */ +static int ks8995_parse_dt(struct ks8995_switch *ks) +{ + const __be32 *settings; + int size, nsettings, i; + struct device_node *np = ks->spi->dev.of_node; + struct ks8995_pdata *pdata = ks->pdata; + + if (!np) + return 0; + + /* we have something like: + * settings = <0x22 0x80 0xF0>; + * ^ ^ ^ + * | | | + * | | + register bit mask + * | + register value + * + register number + * + * for multiple registers it is + * + * settings = <0x22 0x80 0xF0 0x23 0x01 0xFF>; + */ + settings = of_get_property(np, "settings", &size); + if (!settings) + return 0; + + if (size < sizeof(*settings) * 2) { + dev_err(&ks->spi->dev, "bad data for settings\n"); + return -EINVAL; + } + + size /= sizeof(*settings); /* Number of elements in DT array */ + nsettings = size / 3; /* Number of register settings */ + + pdata->settings = devm_kzalloc(&ks->spi->dev, + sizeof(*pdata->settings) * nsettings, GFP_KERNEL); + + if (!pdata->settings) + return -ENOMEM; + + for (i = 0; i < nsettings; ++i) { + struct reg_init *s = &pdata->settings[i]; + + s->reg = be32_to_cpup(settings + 3 * i); + s->val = be32_to_cpup(settings + 3 * i + 1); + s->mask = be32_to_cpup(settings + 3 * i + 2); + } + pdata->nsettings = nsettings; + + return 0; +} + static const struct bin_attribute ks8995_registers_attr = { .attr = { .name = "registers", @@ -352,14 +461,10 @@ static const struct bin_attribute ks8995_registers_attr = { /* ------------------------------------------------------------------------ */ static int ks8995_probe(struct spi_device *spi) { - struct ks8995_switch *ks; - struct ks8995_pdata *pdata; - int err; + struct ks8995_switch *ks; + int err; int variant = spi_get_device_id(spi)->driver_data; - /* Chip description */ - pdata = spi->dev.platform_data; - if (variant >= max_variant) { dev_err(&spi->dev, "bad chip variant %d\n", variant); return -ENODEV; @@ -370,10 +475,25 @@ static int ks8995_probe(struct spi_device *spi) return -ENOMEM; mutex_init(&ks->lock); - ks->pdata = pdata; ks->spi = spi_dev_get(spi); ks->chip = &ks8995_chip[variant]; + if (ks->spi->dev.of_node) { + ks->pdata = devm_kzalloc(&spi->dev, sizeof(*ks->pdata), + GFP_KERNEL); + if (!ks->pdata) + return -ENOMEM; + + err = ks8995_parse_dt(ks); + if (err) { + dev_err(&ks->spi->dev, "bad data DT data\n"); + return err; + } + } + + if (!ks->pdata) + ks->pdata = spi->dev.platform_data; + spi_set_drvdata(spi, ks); spi->mode = SPI_MODE_0;
Since several use cases need to setup at least some basic control registers add the ability to configure an array containing such register initialization values within the platform data of the switch. Furthermore expose this capabilty to the devicetree. Platform data now contains a pointer to an array and the array length where each member contains the register to be initialized, the initialization value and a register mask, since in many use cases there is only the need to init some bits of a register, e.g. disabling unused ports. The devicetree notation add the property 'settings' to the SPI node of the ks8985 driver, which is a list of triple values (register, value, mask), e.g.: settings = <0x4D 0x08 0x08 0x5D 0x08 0x08>; to power down port 3 and 4 of a KSZ8864RMN. Signed-off-by: Helmut Buchsbaum <helmut.buchsbaum@gmail.com> --- drivers/net/phy/spi_ks8995.c | 136 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 128 insertions(+), 8 deletions(-)