On Wed, 12 Aug 2020 09:45:40 +0200 Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote: .... > > > > > > menus in order to enable them. > > > We communicate with the Hi6421 via memory-mapped I/O. > > > > > > +config MFD_HI6421_SPMI > > > + tristate "HiSilicon Hi6421v600 SPMI PMU/Codec IC" > > > + depends on OF > > > + select MFD_CORE > > > + select REGMAP_MMIO > > > > Nice thought, but it doesn't use it yet! > > What do you mean? Makefile should be using it: > > $ git grep MFD_HI6421_SPMI > drivers/mfd/Kconfig:config MFD_HI6421_SPMI > drivers/mfd/Makefile:obj-$(CONFIG_MFD_HI6421_SPMI) += hi6421-spmi-pmic.o Just the REGMAP bit. Driver doesn't seem to be using regmap. > > > > > + * > > > + */ > > > + > > > +#include <linux/slab.h> > > > +#include <linux/delay.h> > > > +#include <linux/device.h> > > > +#include <linux/module.h> > > > +#include <linux/err.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/io.h> > > > +#include <linux/mfd/core.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/of.h> > > > +#include <linux/of_address.h> > > > +#include <linux/of_device.h> > > > +#include <linux/of_gpio.h> > > > +#include <linux/of_irq.h> > > > +#include <linux/mfd/hi6421-spmi-pmic.h> > > > +#include <linux/irq.h> > > > +#include <linux/spmi.h> > > > +#ifndef NO_IRQ > > > +#define NO_IRQ 0 > > > > Drop > > Ok! I was in doubt about that, as there are a few other drivers with > the same pattern: > > drivers/ata/sata_dwc_460ex.c:#define NO_IRQ 0 > drivers/input/touchscreen/ucb1400_ts.c:#define NO_IRQ 0 > drivers/mmc/host/of_mmc_spi.c:#define NO_IRQ 0 > drivers/pcmcia/pd6729.c:#define NO_IRQ ((unsigned int)(0)) > drivers/rtc/rtc-m48t59.c:#define NO_IRQ (-1) > > Btw, this definition at the rtc driver sounds weird ;-) History I guess. IIRC, a long time back, 0 was a valid IRQ for some architectures. > > > > + /*SOC_PMIC_IRQ0_ADDR*/ > > > > These superficially feel like things that would come from > > the compatible, but maybe I'm missing something. > > My guess is that this is decided by the board manufacturer. I mean, > from the schematics: > > https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf > > This is a separate chipset, connected at the Kirin 970 SPMI bus. The SPMI bus > allows up to 16 PMICs. So, I would assume that a chip designed to support > SPMI would have the flexibility of using different IRQ lines, being > programmable either via some firmware or via some wiring. > > Without knowing more, IMO the best would be to keep those settings > at DT. Fair enough. Would be nice if they could be established from a bus ID of some type and the compatible but if we don't have the info to be able to do that I guess we will have to do it this way. > > > > > + ret = get_pmic_device_tree_data(np, pmic); > > > + if (ret) { > > > + dev_err(&pdev->dev, "Error reading hisi pmic dts\n"); > > > + return ret; > > > + } > > > + > > > + /* TODO: get and enable clk request */ > > > + spin_lock_init(&pmic->lock); > > > + > > > + pmic->dev = dev; > > > + > > > + pmic->gpio = of_get_gpio_flags(np, 0, &flags); > > > > Do we need flags for something? > > > > Can we use the gpio/consumer.h interfaces for all this? > > > > Though I'm not sure we need a gpio at all. Looks like we just > > use it as an interrupt. Get an interrupt directly instead. > > Yeah, from what I saw, the GPIOs supported right now by this driver > are just for IRQs. Guess they all need switching to just being interrupt lines then. > > > > + if (pmic->gpio < 0) > > > + return pmic->gpio; > > > + > > > + if (!gpio_is_valid(pmic->gpio)) > > > + return -EINVAL; > > > + > > > + ret = gpio_request_one(pmic->gpio, GPIOF_IN, "pmic"); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to request gpio%d\n", pmic->gpio); > > > + return ret; > > > + } > > > + > > > + ret = request_threaded_irq(pmic->irq, hi6421_spmi_irq_handler, NULL, > > > + IRQF_TRIGGER_LOW | IRQF_SHARED | IRQF_NO_SUSPEND, > > > + "pmic", pmic); > > > + if (ret < 0) { > > > + dev_err(dev, "could not claim pmic %d\n", ret); > > > + ret = -ENODEV; > > > + goto request_theaded_irq; > > > + } > > > + > > > + dev_set_drvdata(&pdev->dev, pmic); > > > + > > > + /* > > > + * The logic below will rely that the pmic is already stored at > > > + * drvdata. > > > + */ > > > + dev_dbg(&pdev->dev, "SPMI-PMIC: adding children for %pOF\n", > > > + pdev->dev.of_node); > > > + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, > > > + hi6421v600_devs, ARRAY_SIZE(hi6421v600_devs), > > > + NULL, 0, NULL); > > > > This is mixing and matching managed an unmanaged. Should be one or the other > > or we might be hiding some race conditions. I intended this as a more localized comment, though the following answers another question I had. request_threaded_irq is not using devm_ whilst the add devices is. As a result we might have a path in which the irq goes away before the device cleanup happens. > > Actually, it is just the opposite. It took me a lot of time to > figure out a good solution that will prevent all race conditions at > probe time. > > See, the SPMI variant of HiSilicon 6421 requires the drivers to be > probed on a very specific order: > > - The SPMI controller should be loaded first, as it provides > the low-level I/O access to the serial bus used to talk with the > PMICs. This bus is somewhat similar to the I2C bus. > > Once the controller is registered, the SPMI bus probes the PMIC > driver. > > - Then, the MFD PMIC driver should be loaded. This adds support for > a high level set of I/O operations, which are used by the regulator > driver. Again, this approach is similar to the one taken by the > I2C Kernel drivers. > > - Finally, the regulator drivers should come, as they rely on the > MFD I/O operations in order to talk with the SPMI bus. > > The OOT driver probing was based on a some dirty hacks: it had an > empty SPMI entry at the SoC, carrying on just the "compatible" line. > > Then, another entry at DT with the real SPMI settings. > > With such dirty hack, on Kernel 4.9, the PMIC driver were always > loading before the regulator ones, as the SPMI bus code were > serializing the probe there. > > However, such settings were too fragile and broke after porting to > upstream Kernels, because the regulator drivers were probed on > a random order, typically before the MFD one (and sometimes even > before the SPMI controller driver). Adding EPROBE_DEFER didn't > solve all the issues, and made a complex and hard to debug scenario. > Also, regulators were probed on a random order, making harder to > debug issues there. There are no ordering guarantees IIRC in mfd children coming up even in the normal path. It might currently happen in a particular order but relying on that seems fragile to me. > > So, I ended using the same solution used by the already-existing > drivers/mfd/hi6421-pmic-core.c driver[1]. > > [1] This variant of the 6421 chipset is a lot simpler, as it > doesn't use the SPMI bus. > > With such approach, the probing is warranted to happen the > way it is expected by the driver: > > SPMI controller code starts: > [ 0.416862] spmi_controller fff24000.spmi: HISI SPMI probe > [ 0.422419] spmi spmi-0: allocated controller 0x(____ptrval____) id 0 > [ 0.428929] spmi_controller fff24000.spmi: spmi_add_controller base addr=0xffff800012055000! > [ 0.437480] spmi spmi-0: adding child /spmi@fff24000/pmic@0 > [ 0.443109] spmi spmi-0: read usid 00 > [ 0.446821] spmi 2-00: device 2-00 registered > [ 0.451220] spmi spmi-0: spmi-2 registered: dev:(____ptrval____) > [ 0.457286] spmi_controller fff24000.spmi: spmi_add_controller initialized > > The PMIC probe happens sometime after spmi_controller registers itself > at the SPMI bus: > > [ 1.955838] [hi6421_spmi_pmic_probe]. pmic->irqs[0] = 43 > ... > [ 2.036298] [hi6421_spmi_pmic_probe]. pmic->irqs[15] = 58 > > After being ready to handle I/O requests, it starts probing the > regulators: > > [ 2.057815] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo3@16 > [ 2.199827] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo4@17 > [ 2.336284] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo9@1C > [ 2.472675] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo15@21 > [ 2.609402] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo16@22 > [ 2.746378] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo17@23 > [ 2.846707] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo33@32 > [ 2.988646] hi6421v600-regulator hi6421v600-regulator: adding child /spmi@fff24000/pmic@0/regulators/ldo34@33 > > As the current code serializes the regulator probing, it ensured that > they'll happen at the right order, avoiding race conditions at > probe time. Why do we need the regulators to come up in a particular order? That sounds suspicious as any relationships between different ones should be expressed either in DT or in the order they are enabled in the drivers using them. > > > > > > > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > > > index edb1c4f8b496..de8a78487bb9 100644 > > > --- a/drivers/regulator/Kconfig > > > +++ b/drivers/regulator/Kconfig > > > @@ -356,6 +356,14 @@ config REGULATOR_HI6421V530 > > > provides 5 general purpose LDOs, and all of them come with support > > > to either ECO (idle) or sleep mode. > > > > > > +config REGULATOR_HI6421V600 > > > + tristate "HiSilicon Hi6421v600 PMIC voltage regulator support" > > > + depends on MFD_HI6421_PMIC && OF > > > > Can we do a COMPILE_TEST here? > > Neither REGULATOR_HI6421V600 nor MFD_HI6421_PMIC depends on ARM or > ARM64, so they should build fine on any arch. So, I'm failing to see > why adding COMPILE_TEST would make any difference. cool. Good to get the extra build coverage. > > > > + > > > +#include <linux/slab.h> > > > +#include <linux/device.h> > > > +#include <linux/module.h> > > > +#include <linux/err.h> > > > +#include <linux/io.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/of.h> > > > +#include <linux/of_device.h> > > > +#include <linux/of_address.h> > > > +#include <linux/regmap.h> > > > +#include <linux/regulator/driver.h> > > > +#include <linux/regulator/machine.h> > > > +#include <linux/regulator/of_regulator.h> > > > +#include <linux/mfd/hi6421-spmi-pmic.h> > > > +#include <linux/delay.h> > > > +#include <linux/time.h> > > > +#include <linux/version.h> > > > +#include <linux/seq_file.h> > > > +#include <linux/uaccess.h> > > > +#include <linux/spmi.h> > > > + > > > +#define rdev_dbg(rdev, fmt, arg...) \ > > > + pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg) > > > > Not worth defining in my view. > > That was my first approach: to drop any existing printk macro, replacing > them by dev_dbg(). However, with dev_dbg(), the logs were like: > > [ 2.069301] platform ldo3: LDO doesn't support economy mode. > [ 2.074969] platform ldo3: voltage selector settings: reg: 0x51, mask: 0xf > [ 2.094593] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.105530] regulator.3: hi6421_spmi_regulator_enable: off_on_delay=20000 us, enable_reg=0x16, enable_mask=0x1 > [ 2.153484] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.163409] ldo3: 1500 <--> 2000 mV at 1800 mV normal > [ 2.181305] regulator.3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.191289] regulator.3: hi6421_spmi_regulator_probe_ldo: valid_modes_mask: 0x2, valid_ops_mask: 0x9 > > E. g. the messages printed by the regulator's core were using > (rdev)->desc->name on their printks, producing a nice debug > (when enabled at core level), while dev_dbg() were using a different name. > > In this specific case, one might assume that "regulator.3" is "ldo3", > but this is by coincidence, as it actually matches the sysfs entries: > > $ ls -l /sys/class/regulator/ > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.0 -> ../../devices/platform/reg-dummy/regulator/regulator.0 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.1 -> ../../devices/platform/regulator-1v8/regulator/regulator.1 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.10 -> ../../devices/platform/spmi-0/2-00/ldo34/regulator/regulator.10 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.2 -> ../../devices/platform/wlan-en-1-8v/regulator/regulator.2 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.3 -> ../../devices/platform/spmi-0/2-00/ldo3/regulator/regulator.3 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.4 -> ../../devices/platform/spmi-0/2-00/ldo4/regulator/regulator.4 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.5 -> ../../devices/platform/spmi-0/2-00/ldo9/regulator/regulator.5 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.6 -> ../../devices/platform/spmi-0/2-00/ldo15/regulator/regulator.6 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.7 -> ../../devices/platform/spmi-0/2-00/ldo16/regulator/regulator.7 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.8 -> ../../devices/platform/spmi-0/2-00/ldo17/regulator/regulator.8 > lrwxrwxrwx 1 root root 0 ago 12 08:26 regulator.9 -> ../../devices/platform/spmi-0/2-00/ldo33/regulator/regulator.9 > > When playing with DT, the "regulator.[\d+]" namespace may change, > making harder to debug stuff. So, basically, enabling just the logs > at the regulator driver were requiring to double-check the sysfs > entries in order to see what "regulator.3" were actually meaning > with a particular DT setting. > > After adding the macro, the output is a lot easier to be understood: > > [ 2.069301] platform ldo3: LDO doesn't support economy mode. > [ 2.074969] platform ldo3: voltage selector settings: reg: 0x51, mask: 0xf > [ 2.094593] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.105530] ldo3: hi6421_spmi_regulator_enable: off_on_delay=20000 us, enable_reg=0x16, enable_mask=0x1 > [ 2.153484] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.163409] ldo3: 1500 <--> 2000 mV at 1800 mV normal > [ 2.181305] ldo3: hi6421_spmi_regulator_get_voltage_sel: vsel_reg=0x51, value=0x8, entry=0x8, voltage=1800 mV > [ 2.191289] ldo3: hi6421_spmi_regulator_probe_ldo: valid_modes_mask: 0x2, valid_ops_mask: 0x9 > > As the name which got printed is the one that actually makes sense > for someone working with the driver, as it matches the DT entries > and the hardware power supply lines. Fair enough. Guess this is one for Mark to ultimately decide. > > > > +static int hi6421_spmi_regulator_set_voltage_sel(struct regulator_dev *rdev, > > > + unsigned int selector) > > > +{ > > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev); > > > + struct hi6421_spmi_pmic *pmic = sreg->pmic; > > > + u32 reg_val; > > > + > > > + /* unlikely to happen. sanity test done by regulator core */ > > > > Unlikely or can't? > > > > > + if (unlikely(selector >= rdev->desc->n_voltages)) > > > + return -EINVAL; > > Good question. I almost removed this check, but I didn't check the > regulator code with enough care to be 100% sure. So, I opted to keep it > here. I'd drop the comment then :) If someone else wants to figure it out in future then they are welcome to. > > > > + > > > + reg_val = selector << (ffs(rdev->desc->vsel_mask) - 1); > > > + > > > + /* set voltage selector */ > > > + rdev_dbg(rdev, > > > + "vsel_reg=0x%x, mask=0x%x, value=0x%x, voltage=%d mV\n", > > > + rdev->desc->vsel_reg, rdev->desc->vsel_mask, reg_val, > > > + rdev->desc->ops->list_voltage(rdev, selector) / 1000); > > > + > > > + hi6421_spmi_pmic_rmw(pmic, rdev->desc->vsel_reg, > > > + rdev->desc->vsel_mask, reg_val); > > > + > > > + return 0; > > > +} > > > + > > > +static unsigned int hi6421_spmi_regulator_get_mode(struct regulator_dev *rdev) > > > +{ > > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev); > > > + struct hi6421_spmi_pmic *pmic = sreg->pmic; > > > + u32 reg_val; > > > + unsigned int mode; > > > + > > > + reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg); > > > + > > > + if (reg_val & sreg->eco_mode_mask) > > > + mode = REGULATOR_MODE_IDLE; > > > + else > > > + mode = REGULATOR_MODE_NORMAL; > > > + > > > + rdev_dbg(rdev, > > > + "enable_reg=0x%x, eco_mode_mask=0x%x, reg_val=0x%x, %s mode\n", > > > + rdev->desc->enable_reg, sreg->eco_mode_mask, reg_val, > > > + mode == REGULATOR_MODE_IDLE ? "idle" : "normal"); > > > + > > > + return mode; > > > +} > > > + > > > +static int hi6421_spmi_regulator_set_mode(struct regulator_dev *rdev, > > > + unsigned int mode) > > > +{ > > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev); > > > + struct hi6421_spmi_pmic *pmic = sreg->pmic; > > > + u32 val; > > > + > > > + switch (mode) { > > > + case REGULATOR_MODE_NORMAL: > > > + val = 0; > > > + break; > > > + case REGULATOR_MODE_IDLE: > > > + val = sreg->eco_mode_mask << (ffs(sreg->eco_mode_mask) - 1); > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + /* set mode */ > > > + rdev_dbg(rdev, "enable_reg=0x%x, eco_mode_mask=0x%x, value=0x%x\n", > > > + rdev->desc->enable_reg, sreg->eco_mode_mask, val); > > > + > > > + hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg, > > > + sreg->eco_mode_mask, val); > > > + > > > + return 0; > > > +} > > > + > > > +static unsigned int > > > +hi6421_spmi_regulator_get_optimum_mode(struct regulator_dev *rdev, > > > + int input_uV, int output_uV, > > > + int load_uA) > > > +{ > > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev); > > > + > > > + if (load_uA || ((unsigned int)load_uA > sreg->eco_uA)) { > > > + rdev_dbg(rdev, "normal mode"); > > > > Debug seems unnecessary to me, but maybe keep it if you want. > > I actually used this debug. There are some LDO lines which don't > support eco mode. The original driver was hard to understand that. > So, I ended by re-writing the part of the code which sets/uses it[1]: > > + /* hisi regulator supports two modes */ > + constraint = &initdata->constraints; > + > + constraint->valid_modes_mask = REGULATOR_MODE_NORMAL; > + if (sreg->eco_mode_mask) { > + constraint->valid_modes_mask |= REGULATOR_MODE_IDLE; > + constraint->valid_ops_mask |= REGULATOR_CHANGE_MODE; > + } > + > > [1] https://lore.kernel.org/lkml/176043f329dfa9889f014feec04e7e1553077873.1597160086.git.mchehab+huawei@xxxxxxxxxx/T/#m337e09adf04e4b8ce56af93ba37e3720b2a3002b > > Those debug messages are useful to double-check if something bad is > not happening with the modes/ops masks. That's fine, but is it useful to have it upstream now you have debugged those issues? I'm not completely convinced it is and debug prints have a habit of rotting just like comments. > > > > > + /* register regulator */ > > > + rdev = regulator_register(rdesc, &config); > > > + if (IS_ERR(rdev)) { > > > + dev_err(dev, "failed to register %s\n", > > > + rdesc->name); > > > + ret = PTR_ERR(rdev); > > > + goto probe_end; > > > + } > > > + > > > + rdev_dbg(rdev, "valid_modes_mask: 0x%x, valid_ops_mask: 0x%x\n", > > > + constraint->valid_modes_mask, constraint->valid_ops_mask); > > > + > > > + dev_set_drvdata(dev, rdev); > > I'd do separate error path. > > > > return 0; > > > > > +probe_end: > > > + if (ret) > > > > ret is set if separate error path. > > I haven't figured out lifetime but can we not use devm for sreg? > > > > > + kfree(sreg); > > I guess we can. I'll check and change if ok. > > > > +static int hi6421_spmi_regulator_remove(struct platform_device *pdev) > > > +{ > > > + struct regulator_dev *rdev = dev_get_drvdata(&pdev->dev); > > > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev); > > > + > > > + regulator_unregister(rdev); > > > + > > > + /* TODO: should i worry about that? devm_kzalloc */ > > > > Answer that TODO. No unless something odd going on. > > Yeah, agreed. I'll cleanup useless comments on this driver. > > > > > > + if (rdev->desc->volt_table) > > > + devm_kfree(&pdev->dev, (unsigned int *)rdev->desc->volt_table); > > > + > > > + kfree(sreg); > > > > This is a worrying mix of devm and not. Never a good sign. > > Lifetime of sreg seems strange. > > I guess we can just get rid of both, converting sreg to use devm alloc. > > Btw, at least on Hikey 970 (which happens to be the only board using > it so far), in practice this driver should never be removed, as, among > other things, it controls the power supply for storage (both SD and > MMC). Who needs storage? :) ... > > > > + ret = spmi_controller_add(ctrl); > > > + if (ret) > > > + goto err_add_controller; > > > + > > > + dev_info(&pdev->dev, "spmi_add_controller initialized\n"); > > > > Too noisy. > > That's helpful to check if drivers initialized at the right order. > > I'll change it to dev_dbg(). More reasonable. thanks, Jonathan
