* Re: [PATCH v1 2/2] Add PWM driver for LGM
From: Uwe Kleine-König @ 2020-05-22 8:56 UTC (permalink / raw)
To: Rahul Tanwar
Cc: thierry.reding, p.zabel, linux-pwm, robh+dt, linux-kernel,
devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim,
qi-ming.wu
In-Reply-To: <3c1d2343b034325dbc185ccd23a35b40a62a4e7b.1590132733.git.rahul.tanwar@linux.intel.com>
Hello,
On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote:
> Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC.
>
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
> drivers/pwm/Kconfig | 9 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 366 insertions(+)
> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index eebbc917ac97..a582214f50b2 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -232,6 +232,15 @@ config PWM_IMX_TPM
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx-tpm.
>
> +config PWM_INTEL_LGM
> + tristate "Intel LGM PWM support"
> + depends on X86 || COMPILE_TEST
> + help
> + Generic PWM framework driver for LGM SoC.
I wouldn't call this "framework".
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-intel-lgm.
> +
> config PWM_JZ4740
> tristate "Ingenic JZ47xx PWM support"
> depends on MACH_INGENIC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9a475073dafc..c16a972a101d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> +obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index 000000000000..e307fd2457df
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,356 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Intel Corporation.
> + */
Please point out limitations of your driver here similar to e.g.
drivers/pwm/pwm-sifive.c (and in the same format).
You should mention the fixed period of the hardware here (assuming I
interpreted this correctly below) and missing support for polarity.
How does the PWM behave on disable? Does the output become inactive
then? Does it complete the currently running period? What about
reconfiguration? Does changing the duty cycle complete the currently
running period?
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#define PWM_FAN_CON0 0x0
> +#define PWM_FAN_EN_EN BIT(0)
> +#define PWM_FAN_EN_DIS 0x0
> +#define PWM_FAN_EN_MSK BIT(0)
> +#define PWM_FAN_MODE_2WIRE 0x0
> +#define PWM_FAN_MODE_4WIRE 0x1
> +#define PWM_FAN_MODE_MSK BIT(1)
> +#define PWM_FAN_PWM_DIS_DIS 0x0
> +#define PWM_FAN_PWM_DIS_MSK BIT(2)
> +#define PWM_TACH_EN_EN 0x1
> +#define PWM_TACH_EN_MSK BIT(4)
> +#define PWM_TACH_PLUS_2 0x0
> +#define PWM_TACH_PLUS_4 0x1
> +#define PWM_TACH_PLUS_MSK BIT(5)
> +#define PWM_FAN_DC_MSK GENMASK(23, 16)
> +
> +#define PWM_FAN_CON1 0x4
> +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
> +
> +#define PWM_FAN_STAT 0x10
> +#define PWM_FAN_TACH_MASK GENMASK(15, 0)
> +
> +#define MAX_RPM (BIT(16) - 1)
> +#define DFAULT_RPM 4000
> +#define MAX_DUTY_CYCLE (BIT(8) - 1)
> +
> +#define FRAC_BITS 10
> +#define TWO_TENTH 204
> +
> +#define TWO_SECONDS 2000
> +#define IGNORE_FIRST_ERR 1
> +#define THIRTY_SECS_WINDOW 15
> +#define ERR_CNT_THRESHOLD 6
> +
> +struct intel_pwm_chip {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + struct clk *clk;
> + struct reset_control *rst;
> + u32 tach_en;
> + u32 max_rpm;
> + u32 set_rpm;
> + u32 set_dc;
> + struct delayed_work work;
> +};
I don't like aligning the member names and prefer a single space here.
(But I'm aware this is subjective and others like it the why you did
it.)
> +
> +static inline struct intel_pwm_chip *to_intel_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct intel_pwm_chip, chip);
> +}
> +
> +static int pwm_update_dc(struct intel_pwm_chip *pc, u32 val)
Please use a common function prefix for all functions in your driver.
> +{
> + return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK,
> + FIELD_PREP(PWM_FAN_DC_MSK, val));
> +}
> +
> +static int intel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip);
> + u32 val;
> +
> + val = DIV_ROUND_CLOSEST(duty_ns * MAX_DUTY_CYCLE, period_ns);
> + val = min_t(u32, val, MAX_DUTY_CYCLE);
The period is fixed for this hardware type? If so you're supposed to
refuse requests for a period smaller than the actual period length.
Also duty_cycle is supposed to round down.
(That's something that PWM_DEBUG will tell you, too)
> + if (pc->tach_en) {
> + pc->set_dc = val;
> + pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE;
> + }
> +
> + return pwm_update_dc(pc, val);
> +}
> +
> +static int intel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip);
> + struct regmap *regmap = pc->regmap;
> +
> + regmap_update_bits(regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_MSK, PWM_FAN_EN_EN);
> +
> + if (pc->tach_en)
> + schedule_delayed_work(&pc->work, msecs_to_jiffies(10000));
> +
> + return 0;
> +}
> +
> +static void intel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct intel_pwm_chip *pc = to_intel_pwm_chip(chip);
> + struct regmap *regmap = pc->regmap;
> +
> + if (pc->tach_en)
> + cancel_delayed_work_sync(&pc->work);
> +
> + regmap_update_bits(regmap, PWM_FAN_CON0,
> + PWM_FAN_EN_MSK, PWM_FAN_EN_DIS);
> +}
> +
> +static const struct pwm_ops intel_pwm_ops = {
> + .config = intel_pwm_config,
> + .enable = intel_pwm_enable,
> + .disable = intel_pwm_disable,
> + .owner = THIS_MODULE,
Please implement .apply instead of .config/.enable/.disable. Please also
enable PWM_DEBUG and fix the issues pointed out with it.
> +};
> +
> +static void tach_work(struct work_struct *work)
> +{
> + struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip,
> + work.work);
> + struct regmap *regmap = pc->regmap;
> + u32 fan_tach, fan_dc, val;
> + s32 diff;
> + static u32 fanspeed_err_cnt, time_window, delta_dc;
> +
> + /*
> + * Fan speed is tracked by reading the active duty cycle of PWM output
> + * from the active duty cycle register. Some variance in the duty cycle
> + * register value is expected. So we set a time window of 30 seconds and
> + * if we detect inaccurate fan speed 6 times within 30 seconds then we
> + * mark it as fan speed problem and fix it by readjusting the duty cycle.
> + */
I'm a unhappy to have this in the PWM driver. The PWM driver is supposed
to be generic and I think this belongs into a dedicated driver.
> +
> + if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
> + /*
> + * Ignore first time we detect inaccurate fan speed
> + * because it is expected during bootup.
> + */
> + time_window++;
> +
> + if (time_window == THIRTY_SECS_WINDOW) {
> + /*
> + * This work is scheduled every 2 seconds i.e. each time_window
> + * counter step roughly mean 2 seconds. When the time window
> + * reaches 30 seconds, reset all the counters/logic.
> + */
> + fanspeed_err_cnt = 0;
> + delta_dc = 0;
> + time_window = 0;
> + }
> +
> + regmap_read(regmap, PWM_FAN_STAT, &fan_tach);
> + fan_tach &= PWM_FAN_TACH_MASK;
> + if (!fan_tach)
> + goto restart_work;
> +
> + val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach);
> + diff = val - BIT(FRAC_BITS);
> +
> + if (abs(diff) > TWO_TENTH) {
> + /* if duty cycle diff is more than two tenth, detect it as error */
> + if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
> + delta_dc += val;
> + fanspeed_err_cnt++;
> + }
> +
> + if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) {
> + /*
> + * We detected fan speed errors 6 times with 30 seconds.
> + * Fix the error by readjusting duty cycle and reset
> + * our counters/logic.
> + */
> + fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2);
> + fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE);
> + pwm_update_dc(pc, fan_dc);
> + fanspeed_err_cnt = 0;
> + delta_dc = 0;
> + time_window = 0;
> + }
> +
> +restart_work:
> + /*
> + * Fan speed doesn't need continous tracking. Schedule this work
> + * every two seconds so it doesn't steal too much cpu cycles.
> + */
> + schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS));
> +}
> +
> +static void pwm_init(struct intel_pwm_chip *pc)
> +{
> + struct device *dev = pc->chip.dev;
> + struct regmap *regmap = pc->regmap;
> + u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask;
> +
> + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
> + fan_wire = 2; /* default is 2 wire mode */
> +
> + con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS);
> + con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK;
> +
> + switch (fan_wire) {
> + case 2 ... 3:
> + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
> + break;
This can be dropped as it matches the default case.
> + case 4:
> + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) |
> + FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN);
> + con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK;
> + pc->tach_en = 1;
> + break;
> + default:
> + /* default is 2wire mode */
> + con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
> + break;
> + }
> +
> + if (pc->tach_en) {
> + if (device_property_read_u32(dev, "intel,tach-plus",
> + &tach_plus))
> + tach_plus = 2;
> +
> + switch (tach_plus) {
> + case 2:
> + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
> + PWM_TACH_PLUS_2);
> + break;
> + case 4:
> + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
> + PWM_TACH_PLUS_4);
> + break;
> + default:
> + con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
> + PWM_TACH_PLUS_2);
> + break;
Similarily here, case 2: and default are identical.
> + }
> +
> + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm))
> + max_rpm = DFAULT_RPM;
> +
> + max_rpm = min_t(u32, max_rpm, MAX_RPM);
> + if (max_rpm == 0)
> + max_rpm = DFAULT_RPM;
> +
> + pc->max_rpm = max_rpm;
> + INIT_DEFERRABLE_WORK(&pc->work, tach_work);
> + regmap_update_bits(regmap, PWM_FAN_CON1,
> + PWM_FAN_MAX_RPM_MSK, max_rpm);
> + }
> +
> + regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val);
> +}
> +
> +static const struct regmap_config pwm_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> +};
> +
> +static int intel_pwm_probe(struct platform_device *pdev)
> +{
> + struct intel_pwm_chip *pc;
> + struct device *dev = &pdev->dev;
> + void __iomem *io_base;
> + int ret;
> +
> + pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> + if (!pc)
> + return -ENOMEM;
> +
> + io_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(io_base))
error message here?
> + return PTR_ERR(io_base);
> +
> + pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
> + if (IS_ERR(pc->regmap)) {
> + ret = PTR_ERR(pc->regmap);
> + dev_err(dev, "failed to init register map: %d\n", ret);
Better use %pe for the error code to get a more descriptive message.
> + return ret;
> + }
> +
> + pc->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pc->clk)) {
> + ret = PTR_ERR(pc->clk);
> + dev_err(dev, "failed to get clock: %d\n", ret);
> + return ret;
> + }
> +
> + pc->rst = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(pc->rst)) {
> + ret = PTR_ERR(pc->rst);
> + dev_err(dev, "failed to get reset control: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(pc->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock\n");
> + return ret;
> + }
> +
> + reset_control_deassert(pc->rst);
return value check is missing.
> + pc->chip.dev = dev;
> + pc->chip.ops = &intel_pwm_ops;
> + pc->chip.npwm = 1;
> +
> + pwm_init(pc);
> +
> + ret = pwmchip_add(&pc->chip);
> + if (ret < 0) {
> + dev_err(dev, "failed to add PWM chip: %d\n", ret);
> + clk_disable_unprepare(pc->clk);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, pc);
> + return 0;
> +}
> +
> +static int intel_pwm_remove(struct platform_device *pdev)
> +{
> + struct intel_pwm_chip *pc = platform_get_drvdata(pdev);
> + int ret;
> +
> + if (pc->tach_en)
> + cancel_delayed_work_sync(&pc->work);
> +
> + ret = pwmchip_remove(&pc->chip);
> + if (ret < 0)
> + return ret;
> +
> + reset_control_assert(pc->rst);
> +
> + clk_disable_unprepare(pc->clk);
In .probe() you first release reset and then enable the clock. It's good
style to do it the other way round in .remove().
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v3 0/6] sc16is7xx: IrDA mode and threaded IRQs
From: Greg KH @ 2020-05-22 9:07 UTC (permalink / raw)
To: Daniel Mack
Cc: devicetree, linux-serial, robh+dt, jslaby, jringle, m.brock,
pascal.huerst
In-Reply-To: <20200521091152.404404-1-daniel@zonque.org>
On Thu, May 21, 2020 at 11:11:46AM +0200, Daniel Mack wrote:
> This is v3 of the series.
>
> v3:
>
> * Add my s-o-b to the first two patches
>
> v2:
>
> * Change single bool properties into an array
> (suggested by Rob Herring)
> * Add a patch first try TRIGGER_LOW and SHARED interrupts, and then
> fall back to FALLING edge if the IRQ controller fails to provide the
> former (suggested by Maarten Brock)
> * Add a patch to check for the device presence
>
> Daniel Mack (4):
> sc16is7xx: Always use falling edge IRQ
> sc16is7xx: Use threaded IRQ
> sc16is7xx: Allow sharing the IRQ line
> sc16is7xx: Read the LSR register for basic device presence check
>
> Pascal Huerst (2):
> dt-bindings: sc16is7xx: Add flag to activate IrDA mode
> sc16is7xx: Add flag to activate IrDA mode
As I have to wait for the DT addition to be reviewed before applying the
first 2 patches here, I've taken the other 4 instead at the moment. If
you could rebase the first two and resend when they get acked, so that I
could apply them, that would be great.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCHv8 0/6] n_gsm serdev support and GNSS driver for droid4
From: Greg Kroah-Hartman @ 2020-05-22 9:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Johan Hovold, Rob Herring, Alan Cox, Lee Jones, Jiri Slaby,
Merlijn Wajer, Pavel Machek, Peter Hurley, Sebastian Reichel,
linux-serial, devicetree, linux-kernel, linux-omap
In-Reply-To: <20200512214713.40501-1-tony@atomide.com>
On Tue, May 12, 2020 at 02:47:07PM -0700, Tony Lindgren wrote:
> Hi all,
>
> Here's the updated set of these patches fixed up for Johan's and
> Pavel's earlier comments.
>
> This series does the following:
>
> 1. Adds functions to n_gsm.c for serdev-ngsm.c driver to use
>
> 2. Adds a generic serdev-ngsm.c driver that brings up the TS 27.010
> TTY ports configured in devicetree with help of n_gsm.c
>
> 3. Allows the use of standard Linux device drivers for dedicated
> TS 27.010 channels for devices like GNSS and ALSA found on some
> modems for example
>
> 4. Adds a gnss-motmdm consumer driver for the GNSS device found on
> the Motorola Mapphone MDM6600 modem on devices like droid4
>
> I've placed the serdev-ngsm.c under drivers/tty/serdev as it still
> seems to make most sense with no better places available. It's no
> longer an MFD driver as it really does not need to care what channel
> specific consumer drivers might be configured for the generic driver.
> Now serdev-ngsm just uses of_platform_populate() to probe whatever
> child nodes it might find.
>
> I'm not attached having the driver in drivers/tty/serdev. I just
> don't have any better locations in mind. So using Johan's earlier
> i2c example, the drivers/tty/serdev/serdev-ngsm.c driver is now a
> generic protocol and bus driver, so it's getting closer to the
> the drivers/i2c/busses analogy maybe :) Please do suggest better
> locations other than MFD and misc if you have better ideas.
>
> Now without the chardev support, the /dev/gsmtty* using apps need
> to use "U1234AT+CFUN?" format for the packets. The advantage is
> less kernel code, and we keep the existing /dev/gsmtty* interface.
>
> If we still really need the custom chardev support, that can now
> be added as needed with the channel specific consumer driver(s),
> but looks like this won't be needed based on Pavel's ofono work.
Johan and Rob, any objection/review of this series?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v1 2/2] Add PWM driver for LGM
From: Andy Shevchenko @ 2020-05-22 9:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Rahul Tanwar, thierry.reding, p.zabel, linux-pwm, robh+dt,
linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu
In-Reply-To: <20200522085613.ktb2ruw2virj337v@pengutronix.de>
On Fri, May 22, 2020 at 10:56:13AM +0200, Uwe Kleine-König wrote:
> On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote:
> > + io_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(io_base))
>
> error message here?
platform core provides it. No need to duplicate (esp. taking into consideration
that it can issue IIRC three different error messages depending on actual error).
>
> > + return PTR_ERR(io_base);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 04/16] arm64: dts: arm: Fix node address fields
From: André Przywara @ 2020-05-22 9:25 UTC (permalink / raw)
To: Liviu Dudau, Robin Murphy
Cc: Rob Herring, Sudeep Holla, Lorenzo Pieralisi, Mark Rutland,
devicetree, linux-arm-kernel
In-Reply-To: <20200521144059.GZ159988@e110455-lin.cambridge.arm.com>
On 21/05/2020 15:40, Liviu Dudau wrote:
Hi,
> On Tue, May 05, 2020 at 06:18:19PM +0100, Robin Murphy wrote:
>> On 2020-05-05 5:52 pm, Andre Przywara wrote:
>>> The Arm Ltd. boards were using an outdated address convention in the DT
>>> node names, by separating the high from the low 32-bits of an address by
>>> a comma.
>>
>> I thought that historically that was deliberate, since the actual thing
>> being encoded is <chip select>,<address>, rather than just cosmetically
>> splitting a 64-bit address value?
>>
>> Or maybe I'm thinking too far back and things have already changed in the
>> meantime :/
>
> Robin is right, if you look in the "ARM Motherboard Express µATX Technical
> Reference Manual", in the RS1 memory map (aka "Cortex-A Series memory map")
> the Ethernet for example is at CS2 offset 0x02000000. CS2 area is between
> 0x18000000 and 0x1c000000. So actual physical address for LAN9118 is
> 0x1a000000.
>
> You might want to drop this patch or convert to physical addresses.
Well, this patch is purely cosmetically, because it just changes the
node names, which have no particular meaning. But the regexp in
dt-schema for simple-bus denies commas after the '@' sign, so we get a
warning.
The question whether we actually need to model the chip select lines in
the DT is separate: we could of course just use an empty ranges;
property here, but that would be an extra patch.
If people see some value in this, I can surely post something.
Cheers,
Andre.
>> Robin.
>>
>>> Remove the comma from the node name suffix to be DT spec compliant.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>> arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 10 +++++-----
>>> arch/arm64/boot/dts/arm/foundation-v8.dtsi | 4 ++--
>>> arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 6 +++---
>>> arch/arm64/boot/dts/arm/rtsm_ve-motherboard-rs2.dtsi | 2 +-
>>> arch/arm64/boot/dts/arm/rtsm_ve-motherboard.dtsi | 6 +++---
>>> 5 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>>> index 5c183483ec3b..8010cdcdb37a 100644
>>> --- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>>> +++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>>> @@ -31,7 +31,7 @@
>>> #interrupt-cells = <1>;
>>> ranges;
>>> - nor_flash: flash@0,00000000 {
>>> + nor_flash: flash@0 {
>>> compatible = "arm,vexpress-flash", "cfi-flash";
>>> reg = <0 0x00000000 0x04000000>,
>>> <4 0x00000000 0x04000000>;
>>> @@ -41,13 +41,13 @@
>>> };
>>> };
>>> - psram@1,00000000 {
>>> + psram@100000000 {
>>> compatible = "arm,vexpress-psram", "mtd-ram";
>>> reg = <1 0x00000000 0x02000000>;
>>> bank-width = <4>;
>>> };
>>> - ethernet@2,02000000 {
>>> + ethernet@202000000 {
>>> compatible = "smsc,lan9118", "smsc,lan9115";
>>> reg = <2 0x02000000 0x10000>;
>>> interrupts = <15>;
>>> @@ -59,14 +59,14 @@
>>> vddvario-supply = <&v2m_fixed_3v3>;
>>> };
>>> - usb@2,03000000 {
>>> + usb@203000000 {
>>> compatible = "nxp,usb-isp1761";
>>> reg = <2 0x03000000 0x20000>;
>>> interrupts = <16>;
>>> port1-otg;
>>> };
>>> - iofpga@3,00000000 {
>>> + iofpga@300000000 {
>>> compatible = "simple-bus";
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> diff --git a/arch/arm64/boot/dts/arm/foundation-v8.dtsi b/arch/arm64/boot/dts/arm/foundation-v8.dtsi
>>> index 12f039fa3dad..e26b492795c5 100644
>>> --- a/arch/arm64/boot/dts/arm/foundation-v8.dtsi
>>> +++ b/arch/arm64/boot/dts/arm/foundation-v8.dtsi
>>> @@ -151,7 +151,7 @@
>>> <0 0 41 &gic 0 0 GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
>>> <0 0 42 &gic 0 0 GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
>>> - ethernet@2,02000000 {
>>> + ethernet@202000000 {
>>> compatible = "smsc,lan91c111";
>>> reg = <2 0x02000000 0x10000>;
>>> interrupts = <15>;
>>> @@ -178,7 +178,7 @@
>>> clock-output-names = "v2m:refclk32khz";
>>> };
>>> - iofpga@3,00000000 {
>>> + iofpga@300000000 {
>>> compatible = "simple-bus";
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
>>> index e3983ded3c3c..d5cefddde08c 100644
>>> --- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
>>> +++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
>>> @@ -103,7 +103,7 @@
>>> };
>>> };
>>> - flash@0,00000000 {
>>> + flash@0 {
>>> /* 2 * 32MiB NOR Flash memory mounted on CS0 */
>>> compatible = "arm,vexpress-flash", "cfi-flash";
>>> reg = <0 0x00000000 0x04000000>;
>>> @@ -120,7 +120,7 @@
>>> };
>>> };
>>> - ethernet@2,00000000 {
>>> + ethernet@200000000 {
>>> compatible = "smsc,lan9118", "smsc,lan9115";
>>> reg = <2 0x00000000 0x10000>;
>>> interrupts = <3>;
>>> @@ -133,7 +133,7 @@
>>> vddvario-supply = <&mb_fixed_3v3>;
>>> };
>>> - iofpga@3,00000000 {
>>> + iofpga@300000000 {
>>> compatible = "simple-bus";
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> diff --git a/arch/arm64/boot/dts/arm/rtsm_ve-motherboard-rs2.dtsi b/arch/arm64/boot/dts/arm/rtsm_ve-motherboard-rs2.dtsi
>>> index 60703b5763c6..350cbf17e8b4 100644
>>> --- a/arch/arm64/boot/dts/arm/rtsm_ve-motherboard-rs2.dtsi
>>> +++ b/arch/arm64/boot/dts/arm/rtsm_ve-motherboard-rs2.dtsi
>>> @@ -9,7 +9,7 @@
>>> motherboard {
>>> arm,v2m-memory-map = "rs2";
>>> - iofpga@3,00000000 {
>>> + iofpga@300000000 {
>>> virtio-p9@140000 {
>>> compatible = "virtio,mmio";
>>> reg = <0x140000 0x200>;
>>> diff --git a/arch/arm64/boot/dts/arm/rtsm_ve-motherboard.dtsi b/arch/arm64/boot/dts/arm/rtsm_ve-motherboard.dtsi
>>> index e333c8d2d0e4..d1bfa62ca073 100644
>>> --- a/arch/arm64/boot/dts/arm/rtsm_ve-motherboard.dtsi
>>> +++ b/arch/arm64/boot/dts/arm/rtsm_ve-motherboard.dtsi
>>> @@ -17,14 +17,14 @@
>>> #interrupt-cells = <1>;
>>> ranges;
>>> - flash@0,00000000 {
>>> + flash@0 {
>>> compatible = "arm,vexpress-flash", "cfi-flash";
>>> reg = <0 0x00000000 0x04000000>,
>>> <4 0x00000000 0x04000000>;
>>> bank-width = <4>;
>>> };
>>> - ethernet@2,02000000 {
>>> + ethernet@202000000 {
>>> compatible = "smsc,lan91c111";
>>> reg = <2 0x02000000 0x10000>;
>>> interrupts = <15>;
>>> @@ -51,7 +51,7 @@
>>> clock-output-names = "v2m:refclk32khz";
>>> };
>>> - iofpga@3,00000000 {
>>> + iofpga@300000000 {
>>> compatible = "simple-bus";
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>>
>
^ permalink raw reply
* Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Dongchun Zhu @ 2020-05-22 9:26 UTC (permalink / raw)
To: Tomasz Figa
Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao,
srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
In-Reply-To: <20200521195113.GC14214@chromium.org>
Hi Tomasz,
Thanks for the review. My replies are as below.
On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> Hi Dongchun, Sakari,
>
> On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> > control to set the desired focus via IIC serial interface.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/i2c/Kconfig | 13 ++
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 530 insertions(+)
> > create mode 100644 drivers/media/i2c/dw9768.c
> [snip]
> > +/*
> > + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> > + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> > + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> > + */
> > +#define DW9768_MOVE_DELAY_US 8400
> > +#define DW9768_STABLE_TIME_US 20000
>
> These times are only valid with the specific settings mentioned in the
> comment. If one sets different settings in DT, the driver would apply
> incorrect delays. Rather than hardcoded, they should be computed based
> on the configured values.
>
> That said, I wonder if we're not digging too deep now. Sakari, do you
> think we could take a step back, remove the optional DT properties and
> just support the fixed values for now, so that we can get a basic driver
> upstream first without doubling the effort?
>
Thanks for the reminder.
Yes, here DW9768_MOVE_DELAY_US actually represents Operation Time,
which is dependent upon board-specific settings that defined in DT.
For instance, for one given board, if aac-mode is 2, aac-timing is 0x39,
clock-presc is 1, then Operation Time would be 0.70*(6.3ms+57*0.1ms)*1 =
8.4ms.
> > +
> > +static const char * const dw9768_supply_names[] = {
> > + "vin", /* I2C I/O interface power */
> > + "vdd", /* VCM power */
> > +};
> > +
> > +/* dw9768 device structure */
> > +struct dw9768 {
> > + struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> > + struct v4l2_ctrl_handler ctrls;
> > + struct v4l2_ctrl *focus;
> > + struct v4l2_subdev sd;
> > +
> > + u32 aac_mode;
> > + u32 aac_timing;
> > + u32 clock_dividing_rate;
> > + bool aac_mode_control_enable;
> > + bool aact_cnt_select_enable;
> > + bool clock_dividing_rate_select_enable;
>
> nit: Separate types from names with just 1 space.
>
Fixed in next release.
> > +};
> > +
> > +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> > +{
> > + return container_of(subdev, struct dw9768, sd);
> > +}
> > +
> > +struct regval_list {
> > + u8 reg_num;
> > + u8 value;
> > +};
> > +
> > +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> > + unsigned char *val)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > + int ret;
> > +
> > + ret = i2c_smbus_read_byte_data(client, reg);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = (unsigned char)ret;
> > +
> > + return 0;
> > +}
>
> Why do we need this function? Couldn't we just call
> i2c_smbus_read_byte_data() directly?
>
Fixed in next release.
> [snip]
> > +static int dw9768_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct dw9768 *dw9768;
> > + unsigned int aac_mode_select;
> > + unsigned int aac_timing_select;
> > + unsigned int clock_dividing_rate_select;
> > + unsigned int i;
> > + int ret;
> > +
> > + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > + if (!dw9768)
> > + return -ENOMEM;
> > +
> > + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > + dw9768->aac_mode_control_enable = false;
> > + dw9768->aact_cnt_select_enable = false;
> > + dw9768->clock_dividing_rate_select_enable = false;
>
> devm_kzalloc() initializes the memory to zero, so no need to set anything
> to false explicitly.
>
Thanks for the reminder.
Yes, these parameters shall not be needed to initialized as zeros.
> > +
> > + /* Optional indication of AAC mode select */
> > + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> > + &aac_mode_select);
> > +
> > + if (!ret) {
> > + dw9768->aac_mode_control_enable = true;
> > + dw9768->aac_mode = aac_mode_select;
>
> How about making aac_mode a signed int and assigning -1 by
> default? Then we don't need two separate fields in the struct.
>
Good idea.
> > + }
> > +
> > + /* Optional indication of VCM internal clock dividing rate select */
> > + ret = fwnode_property_read_u32(dev_fwnode(dev),
> > + "dongwoon,clock-dividing-rate",
> > + &clock_dividing_rate_select);
> > +
> > + if (!ret) {
> > + dw9768->clock_dividing_rate_select_enable = true;
> > + dw9768->clock_dividing_rate = clock_dividing_rate_select;
>
> Ditto.
>
Got it.
> > + }
> > +
> > + /* Optional indication of AAC Timing */
> > + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> > + &aac_timing_select);
> > +
> > + if (!ret) {
> > + dw9768->aact_cnt_select_enable = true;
> > + dw9768->aac_timing = aac_timing_select;
>
> Ditto.
>
Got it.
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> > + dw9768->supplies[i].supply = dw9768_supply_names[i];
> > +
> > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> > + dw9768->supplies);
> > + if (ret) {
> > + dev_err(dev, "failed to get regulators\n");
> > + return ret;
> > + }
> > +
> > + ret = dw9768_init_controls(dw9768);
> > + if (ret)
> > + goto entity_cleanup;
> > +
> > + dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + dw9768->sd.internal_ops = &dw9768_int_ops;
> > +
> > + ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> > + if (ret < 0)
> > + goto entity_cleanup;
> > +
> > + dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > + pm_runtime_enable(dev);
> > + if (!pm_runtime_enabled(dev)) {
> > + ret = dw9768_runtime_resume(dev);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to power on: %d\n", ret);
> > + goto entity_cleanup;
> > + }
> > + }
> > +
> > + ret = v4l2_async_register_subdev(&dw9768->sd);
> > + if (ret < 0)
> > + goto entity_cleanup;
> > +
> > + return 0;
> > +
> > +entity_cleanup:
>
> Need to power off if the code above powered on.
>
Thanks for the reminder.
If there is something wrong with runtime PM, actuator is to be powered
on via dw9768_runtime_resume() API.
When actuator sub-device is powered on completely and async registered
successfully, we shall power off it afterwards.
> > + v4l2_ctrl_handler_free(&dw9768->ctrls);
> > + media_entity_cleanup(&dw9768->sd.entity);
> > + return ret;
> > +}
> > +
> > +static int dw9768_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct dw9768 *dw9768 = sd_to_dw9768(sd);
> > +
> > + pm_runtime_disable(&client->dev);
>
> First the device must be unregistered from the userspace. Otherwise there
> is a race condition that risks the userspace accessing the device while the
> deinitialization is happening.
>
Fixed in next release by adjusting the sequence of unregistering and
runtime PM disable.
> Best regards,
> Tomasz
^ permalink raw reply
* Re: [PATCH v1 2/2] Add PWM driver for LGM
From: Uwe Kleine-König @ 2020-05-22 9:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rahul Tanwar, thierry.reding, p.zabel, linux-pwm, robh+dt,
linux-kernel, devicetree, songjun.Wu, cheol.yong.kim, qi-ming.wu
In-Reply-To: <20200522091824.GR1634618@smile.fi.intel.com>
Hello Andy,
On Fri, May 22, 2020 at 12:18:24PM +0300, Andy Shevchenko wrote:
> On Fri, May 22, 2020 at 10:56:13AM +0200, Uwe Kleine-König wrote:
> > On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote:
>
> > > + io_base = devm_platform_ioremap_resource(pdev, 0);
> > > + if (IS_ERR(io_base))
> >
> > error message here?
>
> platform core provides it. No need to duplicate (esp. taking into consideration
> that it can issue IIRC three different error messages depending on actual error).
Ah, missed that. Indeed that's fine as is in the patch.
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH V2 1/8] dt-bindings: mmc: Add new compatible string for sm8250 target
From: Sarthak Garg @ 2020-05-22 9:32 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: vbadigan, stummala, linux-mmc, linux-kernel, linux-arm-msm,
Sarthak Garg, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <1590139950-7288-1-git-send-email-sartgarg@codeaurora.org>
Add new compatible string for sm8250 target.
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
---
Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 5445931..481f692f 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -17,6 +17,7 @@ Required properties:
"qcom,msm8916-sdhci", "qcom,sdhci-msm-v4"
"qcom,msm8992-sdhci", "qcom,sdhci-msm-v4"
"qcom,msm8996-sdhci", "qcom,sdhci-msm-v4"
+ "qcom,sm8250-sdhci", "qcom,sdhci-msm-v5"
"qcom,sdm845-sdhci", "qcom,sdhci-msm-v5"
"qcom,qcs404-sdhci", "qcom,sdhci-msm-v5"
"qcom,sc7180-sdhci", "qcom,sdhci-msm-v5";
--
2.7.4
^ permalink raw reply related
* [PATCH V2 2/8] dt-bindings: mmc: Add information for DLL register properties
From: Sarthak Garg @ 2020-05-22 9:32 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: vbadigan, stummala, linux-mmc, linux-kernel, linux-arm-msm,
Sarthak Garg, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <1590139950-7288-1-git-send-email-sartgarg@codeaurora.org>
Add information regarding DLL register properties for getting board
specific configurations. These DLL register settings may vary from
board to board.
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
---
Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 481f692f..b8e1d2b 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -47,6 +47,13 @@ Required properties:
"cal" - reference clock for RCLK delay calibration (optional)
"sleep" - sleep clock for RCLK delay calibration (optional)
+- qcom,ddr-config: Certain chipsets and platforms require particular settings
+ for the DDR_CONFIG register. Use this field to specify the register
+ value as per the Hardware Programming Guide.
+
+- qcom,dll-config: Chipset and Platform specific value. Use this field to
+ specify the DLL_CONFIG register value as per Hardware Programming Guide.
+
Example:
sdhc_1: sdhci@f9824900 {
@@ -64,6 +71,9 @@ Example:
clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
clock-names = "core", "iface";
+
+ qcom,dll-config = <0x000f642c>;
+ qcom,ddr-config = <0x80040868>;
};
sdhc_2: sdhci@f98a4900 {
@@ -81,4 +91,7 @@ Example:
clocks = <&gcc GCC_SDCC2_APPS_CLK>, <&gcc GCC_SDCC2_AHB_CLK>;
clock-names = "core", "iface";
+
+ qcom,dll-config = <0x0007642c>;
+ qcom,ddr-config = <0x80040868>;
};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v8 3/3] PM / AVS: SVS: Introduce SVS engine
From: Roger Lu @ 2020-05-22 9:40 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Kevin Hilman, Rob Herring, Nicolas Boichat, Stephen Boyd,
Mark Rutland, Nishanth Menon, Angus Lin,
devicetree@vger.kernel.org, Linux PM list, linux-kernel,
Xiaoqing Liu, YT Lee, Fan Chen,
moderated list:ARM/Mediatek SoC support, HenryC Chen,
Charles Yang, Matthias Brugger, Linux ARM
In-Reply-To: <CAFqH_527ZJEmsvrk-n-uNSc+Bx87ZVppn=rNKDWPGYUuf+gvPA@mail.gmail.com>
Hi Enric,
On Tue, 2020-05-19 at 17:30 +0200, Enric Balletbo Serra wrote:
> Hi Roger,
>
> Thank you for your patch. I have the feeling that this driver is
> complex and difficult to follow and I am wondering if it wouldn't be
> better if you can send a version that simply adds basic functionality
> for now. Some comments below.
Thanks for the advices. I'll submit SVS v9 with basic functionality
patch + step by step functionalities' patches.
>
> Missatge de Roger Lu <roger.lu@mediatek.com> del dia dl., 18 de maig
> 2020 a les 11:25:
> >
> > The SVS (Smart Voltage Scaling) engine is a piece
> > of hardware which is used to calculate optimized
> > voltage values of several power domains,
> > e.g. CPU/GPU/CCI, according to chip process corner,
> > temperatures, and other factors. Then DVFS driver
> > could apply those optimized voltage values to reduce
> > power consumption.
> >
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> > drivers/power/avs/Kconfig | 10 +
> > drivers/power/avs/Makefile | 1 +
> > drivers/power/avs/mtk_svs.c | 2119 +++++++++++++++++++++++++++++++++
> > include/linux/power/mtk_svs.h | 23 +
> > 4 files changed, 2153 insertions(+)
> > create mode 100644 drivers/power/avs/mtk_svs.c
> > create mode 100644 include/linux/power/mtk_svs.h
> >
> > diff --git a/drivers/power/avs/Kconfig b/drivers/power/avs/Kconfig
> > index cdb4237bfd02..67089ac6040e 100644
> > --- a/drivers/power/avs/Kconfig
> > +++ b/drivers/power/avs/Kconfig
> > @@ -35,3 +35,13 @@ config ROCKCHIP_IODOMAIN
> > Say y here to enable support io domains on Rockchip SoCs. It is
> > necessary for the io domain setting of the SoC to match the
> > voltage supplied by the regulators.
> > +
> > +config MTK_SVS
> > + bool "MediaTek Smart Voltage Scaling(SVS)"
>
> Can't be this a module? Why? In such case, you should use tristate option
Generally, MTK_SVS is needed in MTK SoC(mt81xx) products. So, we don't provide
module option in config. If, somehow, SVS isn't needed, we suggest
CONFIG_MTK_SVS=n to be set.
>
> > + depends on POWER_AVS && MTK_EFUSE && NVMEM
> > + help
> > + The SVS engine is a piece of hardware which is used to calculate
> > + optimized voltage values of several power domains, e.g.
> > + CPU clusters/GPU/CCI, according to chip process corner, temperatures,
> > + and other factors. Then DVFS driver could apply those optimized voltage
> > + values to reduce power consumption.
> > diff --git a/drivers/power/avs/Makefile b/drivers/power/avs/Makefile
> > index 9007d05853e2..231adf078582 100644
> > --- a/drivers/power/avs/Makefile
> > +++ b/drivers/power/avs/Makefile
> > @@ -2,3 +2,4 @@
> > obj-$(CONFIG_POWER_AVS_OMAP) += smartreflex.o
> > obj-$(CONFIG_QCOM_CPR) += qcom-cpr.o
> > obj-$(CONFIG_ROCKCHIP_IODOMAIN) += rockchip-io-domain.o
> > +obj-$(CONFIG_MTK_SVS) += mtk_svs.o
>
> Will this driver be SoC specific or the idea is to support different
> SoCs? If the answer to the first question is yes, please name the file
> with the SoC prefix (i.e mt8183_svs). However, If the answer to the
> second question is yes, make sure you prefix common
> functions/structs/defines with a generic prefix mtk_svs but use the
> SoC prefix for the ones you expect will be different between SoC, i.e
> mt8183_svs_. This helps the readability of the driver. Also, try to
> avoid too generic names.
MTK_SVS is designed for supporting different MTK SoCs.Therefore, the answer is second
question and thanks for the heads-up.
>
> > diff --git a/drivers/power/avs/mtk_svs.c b/drivers/power/avs/mtk_svs.c
> > new file mode 100644
> > index 000000000000..a4083b3ef175
> > --- /dev/null
> > +++ b/drivers/power/avs/mtk_svs.c
> > @@ -0,0 +1,2119 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> I suspect you want this only GPLv2 compliant. Use GPL-2.0-only
OK. I'll use GPL-2.0-only.Thanks.
>
> > +/*
> > + * Copyright (C) 2020 MediaTek Inc.
> > + */
> > +
> > +#define pr_fmt(fmt) "[mtk_svs] " fmt
>
> I don't see any reason to use pr_fmt in this driver. Use dev_*
> functions instead and remove the above.
Ok. I will remove it. Thanks.
>
> > +
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kthread.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/pm_qos.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/power/mtk_svs.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/reset.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/thermal.h>
> > +#include <linux/uaccess.h>
> > +
> > +/* svs 1-line sw id */
> > +#define SVS_CPU_LITTLE BIT(0)
> > +#define SVS_CPU_BIG BIT(1)
> > +#define SVS_CCI BIT(2)
> > +#define SVS_GPU BIT(3)
> > +
> > +/* svs bank mode support */
> > +#define SVSB_MODE_ALL_DISABLE (0)
>
> nit: SVS_BMODE_?
Oh. If we add bank wording like SVS_Bxxx, it might cause some confusion when B combines
with other words. So, I'll keep SVSB for SVS Bank representation.
E.g: SVS_BDC_SIGNED_BIT might lead to be explained differently ("SVS bank + DC_SIGNED_BIT" or "SVS + BDC_SIGNED_BIT")
- "SVS bank + DC_SIGNED_BIT" is what we want for naming SVS_BDC_SIGNED_BIT but it might be misunderstood.
>
> > +#define SVSB_MODE_INIT01 BIT(1)
> > +#define SVSB_MODE_INIT02 BIT(2)
> > +#define SVSB_MODE_MON BIT(3)
> > +
> > +/* svs bank init01 condition */
> > +#define SVSB_INIT01_VOLT_IGNORE BIT(1)
> > +#define SVSB_INIT01_VOLT_INC_ONLY BIT(2)
> > +
> > +/* svs bank common setting */
> > +#define HIGH_TEMP_MAX (U32_MAX)
>
> nit: SVS_*
ok. I will add SVS or SVSB when it refers to SVS BANK.
>
> > +#define RUNCONFIG_DEFAULT (0x80000000)
>
> Btw, there is any public datasheet where I can see those addresses and
> registers and bit fields?
Excuse us, there is no public datasheet. We can reply it on patchwork. Thanks.
>
> > +#define DC_SIGNED_BIT (0x8000)
> > +#define INTEN_INIT0x (0x00005f01)
> > +#define INTEN_MONVOPEN (0x00ff0000)
> > +#define SVSEN_OFF (0x0)
> > +#define SVSEN_MASK (0x7)
> > +#define SVSEN_INIT01 (0x1)
> > +#define SVSEN_INIT02 (0x5)
> > +#define SVSEN_MON (0x2)
> > +#define INTSTS_MONVOP (0x00ff0000)
> > +#define INTSTS_COMPLETE (0x1)
> > +#define INTSTS_CLEAN (0x00ffffff)
> > +
> > +#define proc_fops_rw(name) \
> > + static int name ## _proc_open(struct inode *inode, \
> > + struct file *file) \
> > + { \
> > + return single_open(file, name ## _proc_show, \
> > + PDE_DATA(inode)); \
> > + } \
> > + static const struct proc_ops name ## _proc_fops = { \
> > + .proc_open = name ## _proc_open, \
> > + .proc_read = seq_read, \
> > + .proc_lseek = seq_lseek, \
> > + .proc_release = single_release, \
> > + .proc_write = name ## _proc_write, \
> > + }
> > +
> > +#define proc_fops_ro(name) \
> > + static int name ## _proc_open(struct inode *inode, \
> > + struct file *file) \
> > + { \
> > + return single_open(file, name ## _proc_show, \
> > + PDE_DATA(inode)); \
> > + } \
> > + static const struct proc_ops name ## _proc_fops = { \
> > + .proc_open = name ## _proc_open, \
> > + .proc_read = seq_read, \
> > + .proc_lseek = seq_lseek, \
> > + .proc_release = single_release, \
> > + }
> > +
> > +#define proc_entry(name) {__stringify(name), &name ## _proc_fops}
> > +
>
> /proc is usually the old way of exporting files to userspace, so
> unless you have a really good reason use sysfs instead, or even
> better, if it is only for debug purposes use debugfs. Also, you should
> document the entries in Documentation.
Ok. I'll change it to debugfs and could you give us an example about entries in documentation?
We can follow them. Thanks.
>
> > +static DEFINE_SPINLOCK(mtk_svs_lock);
> > +struct mtk_svs;
> > +
> > +enum svsb_phase {
>
> nit: mtk_svs_bphase?
ditto
>
> > + SVSB_PHASE_INIT01 = 0,
>
> nit: SVS_BPHASE_?
ditto
>
> > + SVSB_PHASE_INIT02,
> > + SVSB_PHASE_MON,
> > + SVSB_PHASE_ERROR,
> > +};
> > +
> > +enum reg_index {
>
> nit: svs_reg_index?
OK. Thanks.
>
> > + TEMPMONCTL0 = 0,
> > + TEMPMONCTL1,
> > + TEMPMONCTL2,
> > + TEMPMONINT,
> > + TEMPMONINTSTS,
> > + TEMPMONIDET0,
> > + TEMPMONIDET1,
> > + TEMPMONIDET2,
> > + TEMPH2NTHRE,
> > + TEMPHTHRE,
> > + TEMPCTHRE,
> > + TEMPOFFSETH,
> > + TEMPOFFSETL,
> > + TEMPMSRCTL0,
> > + TEMPMSRCTL1,
> > + TEMPAHBPOLL,
> > + TEMPAHBTO,
> > + TEMPADCPNP0,
> > + TEMPADCPNP1,
> > + TEMPADCPNP2,
> > + TEMPADCMUX,
> > + TEMPADCEXT,
> > + TEMPADCEXT1,
> > + TEMPADCEN,
> > + TEMPPNPMUXADDR,
> > + TEMPADCMUXADDR,
> > + TEMPADCEXTADDR,
> > + TEMPADCEXT1ADDR,
> > + TEMPADCENADDR,
> > + TEMPADCVALIDADDR,
> > + TEMPADCVOLTADDR,
> > + TEMPRDCTRL,
> > + TEMPADCVALIDMASK,
> > + TEMPADCVOLTAGESHIFT,
> > + TEMPADCWRITECTRL,
> > + TEMPMSR0,
> > + TEMPMSR1,
> > + TEMPMSR2,
> > + TEMPADCHADDR,
> > + TEMPIMMD0,
> > + TEMPIMMD1,
> > + TEMPIMMD2,
> > + TEMPMONIDET3,
> > + TEMPADCPNP3,
> > + TEMPMSR3,
> > + TEMPIMMD3,
> > + TEMPPROTCTL,
> > + TEMPPROTTA,
> > + TEMPPROTTB,
> > + TEMPPROTTC,
> > + TEMPSPARE0,
> > + TEMPSPARE1,
> > + TEMPSPARE2,
> > + TEMPSPARE3,
> > + TEMPMSR0_1,
> > + TEMPMSR1_1,
> > + TEMPMSR2_1,
> > + TEMPMSR3_1,
> > + DESCHAR,
> > + TEMPCHAR,
> > + DETCHAR,
> > + AGECHAR,
> > + DCCONFIG,
> > + AGECONFIG,
> > + FREQPCT30,
> > + FREQPCT74,
> > + LIMITVALS,
> > + VBOOT,
> > + DETWINDOW,
> > + CONFIG,
> > + TSCALCS,
> > + RUNCONFIG,
> > + SVSEN,
> > + INIT2VALS,
> > + DCVALUES,
> > + AGEVALUES,
> > + VOP30,
> > + VOP74,
> > + TEMP,
> > + INTSTS,
> > + INTSTSRAW,
> > + INTEN,
> > + CHKINT,
> > + CHKSHIFT,
> > + STATUS,
> > + VDESIGN30,
> > + VDESIGN74,
> > + DVT30,
> > + DVT74,
> > + AGECOUNT,
> > + SMSTATE0,
> > + SMSTATE1,
> > + CTL0,
> > + DESDETSEC,
> > + TEMPAGESEC,
> > + CTRLSPARE0,
> > + CTRLSPARE1,
> > + CTRLSPARE2,
> > + CTRLSPARE3,
> > + CORESEL,
> > + THERMINTST,
> > + INTST,
> > + THSTAGE0ST,
> > + THSTAGE1ST,
> > + THSTAGE2ST,
> > + THAHBST0,
> > + THAHBST1,
> > + SPARE0,
> > + SPARE1,
> > + SPARE2,
> > + SPARE3,
> > + THSLPEVEB,
> > + reg_num,
> > +};
> > +
> > +static const u32 svs_regs_v2[] = {
>
> Is this SoC specific or shared between SoCs?
Shared between SoCs. Some SVS in MTK SoCs use v2 register map.
>
> > + [TEMPMONCTL0] = 0x000,
> > + [TEMPMONCTL1] = 0x004,
> > + [TEMPMONCTL2] = 0x008,
> > + [TEMPMONINT] = 0x00c,
> > + [TEMPMONINTSTS] = 0x010,
> > + [TEMPMONIDET0] = 0x014,
> > + [TEMPMONIDET1] = 0x018,
> > + [TEMPMONIDET2] = 0x01c,
> > + [TEMPH2NTHRE] = 0x024,
> > + [TEMPHTHRE] = 0x028,
> > + [TEMPCTHRE] = 0x02c,
> > + [TEMPOFFSETH] = 0x030,
> > + [TEMPOFFSETL] = 0x034,
> > + [TEMPMSRCTL0] = 0x038,
> > + [TEMPMSRCTL1] = 0x03c,
> > + [TEMPAHBPOLL] = 0x040,
> > + [TEMPAHBTO] = 0x044,
> > + [TEMPADCPNP0] = 0x048,
> > + [TEMPADCPNP1] = 0x04c,
> > + [TEMPADCPNP2] = 0x050,
> > + [TEMPADCMUX] = 0x054,
> > + [TEMPADCEXT] = 0x058,
> > + [TEMPADCEXT1] = 0x05c,
> > + [TEMPADCEN] = 0x060,
> > + [TEMPPNPMUXADDR] = 0x064,
> > + [TEMPADCMUXADDR] = 0x068,
> > + [TEMPADCEXTADDR] = 0x06c,
> > + [TEMPADCEXT1ADDR] = 0x070,
> > + [TEMPADCENADDR] = 0x074,
> > + [TEMPADCVALIDADDR] = 0x078,
> > + [TEMPADCVOLTADDR] = 0x07c,
> > + [TEMPRDCTRL] = 0x080,
> > + [TEMPADCVALIDMASK] = 0x084,
> > + [TEMPADCVOLTAGESHIFT] = 0x088,
> > + [TEMPADCWRITECTRL] = 0x08c,
> > + [TEMPMSR0] = 0x090,
> > + [TEMPMSR1] = 0x094,
> > + [TEMPMSR2] = 0x098,
> > + [TEMPADCHADDR] = 0x09c,
> > + [TEMPIMMD0] = 0x0a0,
> > + [TEMPIMMD1] = 0x0a4,
> > + [TEMPIMMD2] = 0x0a8,
> > + [TEMPMONIDET3] = 0x0b0,
> > + [TEMPADCPNP3] = 0x0b4,
> > + [TEMPMSR3] = 0x0b8,
> > + [TEMPIMMD3] = 0x0bc,
> > + [TEMPPROTCTL] = 0x0c0,
> > + [TEMPPROTTA] = 0x0c4,
> > + [TEMPPROTTB] = 0x0c8,
> > + [TEMPPROTTC] = 0x0cc,
> > + [TEMPSPARE0] = 0x0f0,
> > + [TEMPSPARE1] = 0x0f4,
> > + [TEMPSPARE2] = 0x0f8,
> > + [TEMPSPARE3] = 0x0fc,
> > + [TEMPMSR0_1] = 0x190,
> > + [TEMPMSR1_1] = 0x194,
> > + [TEMPMSR2_1] = 0x198,
> > + [TEMPMSR3_1] = 0x1b8,
> > + [DESCHAR] = 0xc00,
> > + [TEMPCHAR] = 0xc04,
> > + [DETCHAR] = 0xc08,
> > + [AGECHAR] = 0xc0c,
> > + [DCCONFIG] = 0xc10,
> > + [AGECONFIG] = 0xc14,
> > + [FREQPCT30] = 0xc18,
> > + [FREQPCT74] = 0xc1c,
> > + [LIMITVALS] = 0xc20,
> > + [VBOOT] = 0xc24,
> > + [DETWINDOW] = 0xc28,
> > + [CONFIG] = 0xc2c,
> > + [TSCALCS] = 0xc30,
> > + [RUNCONFIG] = 0xc34,
> > + [SVSEN] = 0xc38,
> > + [INIT2VALS] = 0xc3c,
> > + [DCVALUES] = 0xc40,
> > + [AGEVALUES] = 0xc44,
> > + [VOP30] = 0xc48,
> > + [VOP74] = 0xc4c,
> > + [TEMP] = 0xc50,
> > + [INTSTS] = 0xc54,
> > + [INTSTSRAW] = 0xc58,
> > + [INTEN] = 0xc5c,
> > + [CHKINT] = 0xc60,
> > + [CHKSHIFT] = 0xc64,
> > + [STATUS] = 0xc68,
> > + [VDESIGN30] = 0xc6c,
> > + [VDESIGN74] = 0xc70,
> > + [DVT30] = 0xc74,
> > + [DVT74] = 0xc78,
> > + [AGECOUNT] = 0xc7c,
> > + [SMSTATE0] = 0xc80,
> > + [SMSTATE1] = 0xc84,
> > + [CTL0] = 0xc88,
> > + [DESDETSEC] = 0xce0,
> > + [TEMPAGESEC] = 0xce4,
> > + [CTRLSPARE0] = 0xcf0,
> > + [CTRLSPARE1] = 0xcf4,
> > + [CTRLSPARE2] = 0xcf8,
> > + [CTRLSPARE3] = 0xcfc,
> > + [CORESEL] = 0xf00,
> > + [THERMINTST] = 0xf04,
> > + [INTST] = 0xf08,
> > + [THSTAGE0ST] = 0xf0c,
> > + [THSTAGE1ST] = 0xf10,
> > + [THSTAGE2ST] = 0xf14,
> > + [THAHBST0] = 0xf18,
> > + [THAHBST1] = 0xf1c,
> > + [SPARE0] = 0xf20,
> > + [SPARE1] = 0xf24,
> > + [SPARE2] = 0xf28,
> > + [SPARE3] = 0xf2c,
> > + [THSLPEVEB] = 0xf30,
> > +};
> > +
> > +struct thermal_parameter {
>
> In general, not only in this struct, would be good have some
> documentation to have a better undestanding of the fields. That makes
> the job of the reviewer a bit easier.
Ok. Could you share a documentation example to us? We'll share the
information as much as we can. Thanks a lot.
>
> > + int adc_ge_t;
> > + int adc_oe_t;
> > + int ge;
> > + int oe;
> > + int gain;
> > + int o_vtsabb;
> > + int o_vtsmcu1;
> > + int o_vtsmcu2;
> > + int o_vtsmcu3;
> > + int o_vtsmcu4;
> > + int o_vtsmcu5;
> > + int degc_cali;
> > + int adc_cali_en_t;
> > + int o_slope;
> > + int o_slope_sign;
> > + int ts_id;
> > +};
> > +
> > +struct svs_bank_ops {
> > + void (*set_freqs_pct)(struct mtk_svs *svs);
> > + void (*get_vops)(struct mtk_svs *svs);
> > +};
> > +
> > +struct svs_bank {
> > + struct svs_bank_ops *ops;
> > + struct completion init_completion;
> > + struct device *dev;
> > + struct regulator *buck;
> > + struct mutex lock; /* Lock to protect update voltage process */
> > + bool suspended;
> > + bool mtcmos_request;
> > + s32 volt_offset;
> > + u32 mode_support;
> > + u32 opp_freqs[16];
> > + u32 freqs_pct[16];
> > + u32 opp_volts[16];
> > + u32 init02_volts[16];
> > + u32 volts[16];
> > + u32 reg_data[3][reg_num];
> > + u32 freq_base;
> > + u32 vboot;
> > + u32 volt_step;
> > + u32 volt_base;
> > + u32 init01_volt_flag;
> > + u32 phase;
> > + u32 vmax;
> > + u32 vmin;
> > + u32 bts;
> > + u32 mts;
> > + u32 bdes;
> > + u32 mdes;
> > + u32 mtdes;
> > + u32 dcbdet;
> > + u32 dcmdet;
> > + u32 dthi;
> > + u32 dtlo;
> > + u32 det_window;
> > + u32 det_max;
> > + u32 age_config;
> > + u32 age_voffset_in;
> > + u32 agem;
> > + u32 dc_config;
> > + u32 dc_voffset_in;
> > + u32 dvt_fixed;
> > + u32 vco;
> > + u32 chk_shift;
> > + u32 svs_temp;
> > + u32 upper_temp_bound;
> > + u32 lower_temp_bound;
> > + u32 high_temp_threashold;
> > + u32 high_temp_offset;
> > + u32 low_temp_threashold;
> > + u32 low_temp_offset;
> > + u32 core_sel;
> > + u32 opp_count;
> > + u32 int_st;
> > + u32 systemclk_en;
> > + u32 sw_id;
> > + u32 bank_id;
> > + u32 ctl0;
> > + u8 *of_compatible;
> > + u8 *name;
> > + u8 *tzone_name;
> > + u8 *buck_name;
> > +};
> > +
> > +struct svs_platform {
> > + struct svs_bank *banks;
> > + bool (*efuse_parsing)(struct mtk_svs *svs);
> > + bool fake_efuse;
> > + bool need_hw_reset;
> > + const u32 *regs;
> > + unsigned long irqflags;
> > + u32 bank_num;
> > + u32 efuse_num;
> > + u32 efuse_check;
> > + u32 thermal_efuse_num;
> > + u8 *name;
> > +};
> > +
> > +struct mtk_svs {
> > + const struct svs_platform *platform;
> > + struct svs_bank *bank;
> > + struct device *dev;
> > + void __iomem *base;
> > + struct clk *main_clk;
> > + u32 *efuse;
> > + u32 *thermal_efuse;
> > +};
> > +
> > +unsigned long claim_mtk_svs_lock(void)
> > + __acquires(&mtk_svs_lock)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&mtk_svs_lock, flags);
> > +
> > + return flags;
> > +}
> > +EXPORT_SYMBOL_GPL(claim_mtk_svs_lock);
>
> Is this used for an external module? AFAICS no, so no need to export
> and you can replace directly the callers of this with the
> spin_lock_irqsave directly. If you plan to use this in the future for
> other modules, export when is really needed to be exported.
This export API is used by mt8183 thermal.
https://patchwork.kernel.org/patch/11452835/
>
> > +
> > +void release_mtk_svs_lock(unsigned long flags)
> > + __releases(&mtk_svs_lock)
> > +{
> > + spin_unlock_irqrestore(&mtk_svs_lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(release_mtk_svs_lock);
> > +
>
> ditto
>
> > +static u32 percent(u32 numerator, u32 denominator)
> > +{
> > + u32 pct;
> > +
> > + /* If not divide 1000, "numerator * 100" would be data overflow. */
> > + numerator /= 1000;
> > + denominator /= 1000;
> > + pct = ((numerator * 100) + denominator - 1) / denominator;
> > + pct &= GENMASK(7, 0);
> > +
> > + return pct;
> > +}
> > +
> > +static u32 svs_readl(struct mtk_svs *svs, enum reg_index rg_i)
> > +{
> > + return readl(svs->base + svs->platform->regs[rg_i]);
> > +}
> > +
> > +static void svs_writel(struct mtk_svs *svs, u32 val, enum reg_index rg_i)
> > +{
> > + writel(val, svs->base + svs->platform->regs[rg_i]);
> > +}
> > +
> > +static void svs_switch_bank(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > +
> > + svs_writel(svs, svsb->core_sel, CORESEL);
> > +}
> > +
> > +static u32 svsb_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
> > + u32 svsb_volt_base)
> > +{
> > + u32 u_volt;
> > +
> > + u_volt = (svsb_volt * svsb_volt_step) + svsb_volt_base;
> > +
> > + return u_volt;
> > +}
> > +
> > +static int svsb_get_zone_temperature(struct svs_bank *svsb, int *tzone_temp)
> > +{
> > + struct thermal_zone_device *tzd;
> > +
> > + tzd = thermal_zone_get_zone_by_name(svsb->tzone_name);
> > + if (IS_ERR(tzd))
> > + return PTR_ERR(tzd);
> > +
> > + return thermal_zone_get_temp(tzd, tzone_temp);
> > +}
> > +
> > +static int svsb_set_volts(struct svs_bank *svsb, bool force_update)
> > +{
> > + u32 i, svsb_volt, opp_volt, temp_offset = 0;
> > + int tzone_temp, ret;
> > +
> > + mutex_lock(&svsb->lock);
> > +
> > + /*
> > + * If bank is suspended, it means signed-off voltages are applied.
> > + * Don't need to update opp voltage anymore.
> > + */
> > + if (svsb->suspended && !force_update) {
> > + pr_notice("%s: bank is suspended\n", svsb->name);
>
> Replace all pr_* for dev_* and Its better to be quiet, this messages
> looks to me for a dev_dbg candidate, not a notice.
1. Ok. I'll use dev_* instead. Thanks.
2. Usually, no voltages need to be updated by this svs bank after it is suspended.
Therefore, if this log is printed, it means something needs to be noticed.
>
> > + mutex_unlock(&svsb->lock);
> > + return -EPERM;
> > + }
> > +
> > + /* Get thermal effect */
> > + if (svsb->phase == SVSB_PHASE_MON) {
> > + if (svsb->svs_temp > svsb->upper_temp_bound &&
> > + svsb->svs_temp < svsb->lower_temp_bound) {
> > + pr_err("%s: svs_temp is abnormal (0x%x)?\n",
> > + svsb->name, svsb->svs_temp);
>
> dev_err and I am not sure if the error level is appropriate here
1. svs_temp reflects SVS HW condition. If SVS bank informs to update voltages
but its svs_temp is abnormal. It's a warning heads-up to us
2, I'll change it to dev_warning.
>
> > + mutex_unlock(&svsb->lock);
> > + return -EINVAL;
> > + }
> > +
> > + ret = svsb_get_zone_temperature(svsb, &tzone_temp);
> > + if (ret) {
> > + pr_err("%s: cannot get zone \"%s\" temperature(%d)\n",
> > + svsb->name, svsb->tzone_name, ret);
> > + pr_err("%s: set signed-off voltage this time.\n",
> > + svsb->name);
> > + svsb->phase = SVSB_PHASE_ERROR;
> > + }
> > +
> > + if (tzone_temp >= svsb->high_temp_threashold)
> > + temp_offset += svsb->high_temp_offset;
> > +
> > + if (tzone_temp <= svsb->low_temp_threashold)
> > + temp_offset += svsb->low_temp_offset;
> > + }
> > +
> > + /* vmin <= svsb_volt (opp_volt) <= signed-off voltage */
> > + for (i = 0; i < svsb->opp_count; i++) {
> > + if (svsb->phase == SVSB_PHASE_MON) {
> > + svsb_volt = max((svsb->volts[i] + svsb->volt_offset +
> > + temp_offset), svsb->vmin);
> > + opp_volt = svsb_volt_to_opp_volt(svsb_volt,
> > + svsb->volt_step,
> > + svsb->volt_base);
> > + } else if (svsb->phase == SVSB_PHASE_INIT02) {
> > + svsb_volt = max((svsb->init02_volts[i] +
> > + svsb->volt_offset), svsb->vmin);
> > + opp_volt = svsb_volt_to_opp_volt(svsb_volt,
> > + svsb->volt_step,
> > + svsb->volt_base);
> > + } else if (svsb->phase == SVSB_PHASE_ERROR) {
> > + opp_volt = svsb->opp_volts[i];
> > + } else {
> > + pr_err("%s: unknown phase: %u?\n",
> > + svsb->name, svsb->phase);
>
> I think that using a goto here and some places below will help the readability.
Ok. I'll replace it with goto. Thanks.
> ret = -EINVAL;
> goto unlock_mutex
> [...]
> unlock_mutex:
> mutex_unlock(&svsb->lock);
> return ret;
>
> > + mutex_unlock(&svsb->lock);
> > + return -EINVAL;
> > + }
> > +
> > + opp_volt = min(opp_volt, svsb->opp_volts[i]);
> > + ret = dev_pm_opp_adjust_voltage(svsb->dev, svsb->opp_freqs[i],
> > + opp_volt, opp_volt,
> > + svsb->opp_volts[i]);
> > + if (ret) {
> > + pr_err("%s: set voltage failed: %d\n", svsb->name, ret);
> > + mutex_unlock(&svsb->lock);
> > + return ret;
> > + }
> > + }
> > +
> > + mutex_unlock(&svsb->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
> > +{
> > + u32 vy;
> > +
> > + if (v0 == v1 || f0 == f1)
> > + return v0;
> > +
> > + /* *100 to have decimal fraction factor, +99 for rounding up. */
> > + vy = (v0 * 100) - ((((v0 - v1) * 100) / (f0 - f1)) * (f0 - fx));
> > + vy = (vy + 99) / 100;
> > +
> > + return vy;
> > +}
> > +
> > +static void svs_get_vops_v2(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > + u32 temp, i;
> > +
> > + temp = svs_readl(svs, VOP30);
> > + svsb->volts[6] = (temp >> 24) & GENMASK(7, 0);
> > + svsb->volts[4] = (temp >> 16) & GENMASK(7, 0);
> > + svsb->volts[2] = (temp >> 8) & GENMASK(7, 0);
> > + svsb->volts[0] = (temp & GENMASK(7, 0));
> > +
> > + temp = svs_readl(svs, VOP74);
> > + svsb->volts[14] = (temp >> 24) & GENMASK(7, 0);
> > + svsb->volts[12] = (temp >> 16) & GENMASK(7, 0);
> > + svsb->volts[10] = (temp >> 8) & GENMASK(7, 0);
> > + svsb->volts[8] = (temp & GENMASK(7, 0));
> > +
> > + for (i = 0; i <= 7; i++) {
> > + if (i < 7) {
> > + svsb->volts[(i * 2) + 1] =
> > + interpolate(svsb->freqs_pct[i * 2],
> > + svsb->freqs_pct[(i + 1) * 2],
> > + svsb->volts[i * 2],
> > + svsb->volts[(i + 1) * 2],
> > + svsb->freqs_pct[(i * 2) + 1]);
> > + } else if (i == 7) {
> > + svsb->volts[(i * 2) + 1] =
> > + interpolate(svsb->freqs_pct[(i - 1) * 2],
> > + svsb->freqs_pct[i * 2],
> > + svsb->volts[(i - 1) * 2],
> > + svsb->volts[i * 2],
> > + svsb->freqs_pct[(i * 2) + 1]);
> > + }
> > + }
> > +}
> > +
> > +static void svs_set_freqs_pct_v2(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > +
> > + svs_writel(svs,
> > + ((svsb->freqs_pct[6] << 24) & GENMASK(31, 24)) |
> > + ((svsb->freqs_pct[4] << 16) & GENMASK(23, 16)) |
> > + ((svsb->freqs_pct[2] << 8) & GENMASK(15, 8)) |
> > + (svsb->freqs_pct[0] & GENMASK(7, 0)),
> > + FREQPCT30);
> > + svs_writel(svs,
> > + ((svsb->freqs_pct[14] << 24) & GENMASK(31, 24)) |
> > + ((svsb->freqs_pct[12] << 16) & GENMASK(23, 16)) |
> > + ((svsb->freqs_pct[10] << 8) & GENMASK(15, 8)) |
> > + ((svsb->freqs_pct[8]) & GENMASK(7, 0)),
> > + FREQPCT74);
> > +}
> > +
> > +static void svs_set_phase(struct mtk_svs *svs, u32 target_phase)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > + u32 des_char, temp_char, det_char, limit_vals;
> > + u32 init2vals, ts_calcs, val, filter, i;
> > +
> > + svs_switch_bank(svs);
> > +
> > + des_char = ((svsb->bdes << 8) & GENMASK(15, 8)) |
> > + (svsb->mdes & GENMASK(7, 0));
> > + svs_writel(svs, des_char, DESCHAR);
> > +
> > + temp_char = ((svsb->vco << 16) & GENMASK(23, 16)) |
> > + ((svsb->mtdes << 8) & GENMASK(15, 8)) |
> > + (svsb->dvt_fixed & GENMASK(7, 0));
> > + svs_writel(svs, temp_char, TEMPCHAR);
> > +
> > + det_char = ((svsb->dcbdet << 8) & GENMASK(15, 8)) |
> > + (svsb->dcmdet & GENMASK(7, 0));
> > + svs_writel(svs, det_char, DETCHAR);
> > +
> > + svs_writel(svs, svsb->dc_config, DCCONFIG);
> > + svs_writel(svs, svsb->age_config, AGECONFIG);
> > +
> > + if (!svsb->agem) {
> > + svs_writel(svs, RUNCONFIG_DEFAULT, RUNCONFIG);
> > + } else {
> > + val = 0x0;
> > +
> > + for (i = 0; i < 24; i += 2) {
> > + filter = 0x3 << i;
> > +
> > + if (!(svsb->age_config & filter))
> > + val |= (0x1 << i);
> > + else
> > + val |= (svsb->age_config & filter);
> > + }
> > + svs_writel(svs, val, RUNCONFIG);
> > + }
> > +
> > + svsb->ops->set_freqs_pct(svs);
> > +
> > + limit_vals = ((svsb->vmax << 24) & GENMASK(31, 24)) |
> > + ((svsb->vmin << 16) & GENMASK(23, 16)) |
> > + ((svsb->dthi << 8) & GENMASK(15, 8)) |
> > + (svsb->dtlo & GENMASK(7, 0));
> > + svs_writel(svs, limit_vals, LIMITVALS);
> > + svs_writel(svs, (svsb->vboot & GENMASK(7, 0)), VBOOT);
> > + svs_writel(svs, (svsb->det_window & GENMASK(15, 0)), DETWINDOW);
> > + svs_writel(svs, (svsb->det_max & GENMASK(15, 0)), CONFIG);
> > +
> > + if (svsb->chk_shift)
> > + svs_writel(svs, (svsb->chk_shift & GENMASK(7, 0)), CHKSHIFT);
> > +
> > + if (svsb->ctl0)
> > + svs_writel(svs, svsb->ctl0, CTL0);
> > +
> > + svs_writel(svs, INTSTS_CLEAN, INTSTS);
> > +
> > + switch (target_phase) {
> > + case SVSB_PHASE_INIT01:
> > + svs_writel(svs, INTEN_INIT0x, INTEN);
> > + svs_writel(svs, SVSEN_INIT01, SVSEN);
> > + break;
> > + case SVSB_PHASE_INIT02:
> > + svs_writel(svs, INTEN_INIT0x, INTEN);
> > + init2vals = ((svsb->age_voffset_in << 16) & GENMASK(31, 16)) |
> > + (svsb->dc_voffset_in & GENMASK(15, 0));
> > + svs_writel(svs, init2vals, INIT2VALS);
> > + svs_writel(svs, SVSEN_INIT02, SVSEN);
> > + break;
> > + case SVSB_PHASE_MON:
> > + ts_calcs = ((svsb->bts << 12) & GENMASK(23, 12)) |
> > + (svsb->mts & GENMASK(11, 0));
> > + svs_writel(svs, ts_calcs, TSCALCS);
> > + svs_writel(svs, INTEN_MONVOPEN, INTEN);
> > + svs_writel(svs, SVSEN_MON, SVSEN);
> > + break;
> > + default:
> > + WARN_ON(1);
> > + break;
> > + }
> > +}
> > +
> > +static inline void svs_init01_isr_handler(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > + enum reg_index rg_i;
> > +
> > + pr_notice("%s: %s: VDN74:0x%08x, VDN30:0x%08x, DCVALUES:0x%08x\n",
> > + svsb->name, __func__, svs_readl(svs, VDESIGN74),
> > + svs_readl(svs, VDESIGN30), svs_readl(svs, DCVALUES));
> > +
> > + for (rg_i = TEMPMONCTL0; rg_i < reg_num; rg_i++)
> > + svsb->reg_data[SVSB_PHASE_INIT01][rg_i] = svs_readl(svs, rg_i);
> > +
> > + svsb->phase = SVSB_PHASE_INIT01;
> > + svsb->dc_voffset_in = ~(svs_readl(svs, DCVALUES) & GENMASK(15, 0)) + 1;
> > + if (svsb->init01_volt_flag == SVSB_INIT01_VOLT_IGNORE)
> > + svsb->dc_voffset_in = 0;
> > + else if ((svsb->dc_voffset_in & DC_SIGNED_BIT) &&
> > + (svsb->init01_volt_flag == SVSB_INIT01_VOLT_INC_ONLY))
> > + svsb->dc_voffset_in = 0;
> > +
> > + svsb->age_voffset_in = svs_readl(svs, AGEVALUES) & GENMASK(15, 0);
> > +
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_COMPLETE, INTSTS);
> > +
> > + /* svs init01 clock gating */
> > + svsb->core_sel &= ~svsb->systemclk_en;
> > + complete(&svsb->init_completion);
> > +}
> > +
> > +static inline void svs_init02_isr_handler(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > + enum reg_index rg_i;
> > +
> > + pr_notice("%s: %s: VOP74:0x%08x, VOP30:0x%08x, DCVALUES:0x%08x\n",
> > + svsb->name, __func__, svs_readl(svs, VOP74),
> > + svs_readl(svs, VOP30), svs_readl(svs, DCVALUES));
> > +
> > + for (rg_i = TEMPMONCTL0; rg_i < reg_num; rg_i++)
> > + svsb->reg_data[SVSB_PHASE_INIT02][rg_i] = svs_readl(svs, rg_i);
> > +
> > + svsb->phase = SVSB_PHASE_INIT02;
> > + svsb->ops->get_vops(svs);
> > + memcpy(svsb->init02_volts, svsb->volts, sizeof(u32) * svsb->opp_count);
> > +
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_COMPLETE, INTSTS);
> > +
> > + complete(&svsb->init_completion);
> > +}
> > +
> > +static inline void svs_mon_mode_isr_handler(struct mtk_svs *svs)
> > +{
> > + struct svs_bank *svsb = svs->bank;
> > + enum reg_index rg_i;
> > +
> > + for (rg_i = TEMPMONCTL0; rg_i < reg_num; rg_i++)
> > + svsb->reg_data[SVSB_PHASE_MON][rg_i] = svs_readl(svs, rg_i);
> > +
> > + svsb->phase = SVSB_PHASE_MON;
> > + svsb->svs_temp = svs_readl(svs, TEMP) & GENMASK(7, 0);
> > + svsb->ops->get_vops(svs);
> > +
> > + svs_writel(svs, INTSTS_MONVOP, INTSTS);
> > +}
> > +
> > +static inline void svs_error_isr_handler(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb = svs->bank;
> > + enum reg_index rg_i;
> > +
> > + pr_err("%s(): %s(%s)", __func__, svsp->name, svsb->name);
> > + pr_err("CORESEL(0x%x) = 0x%08x\n",
> > + svsp->regs[CORESEL], svs_readl(svs, CORESEL)),
> > + pr_err("SVSEN(0x%x) = 0x%08x, INTSTS(0x%x) = 0x%08x\n",
> > + svsp->regs[SVSEN], svs_readl(svs, SVSEN),
> > + svsp->regs[INTSTS], svs_readl(svs, INTSTS));
> > + pr_err("SMSTATE0(0x%x) = 0x%08x, SMSTATE1(0x%x) = 0x%08x\n",
> > + svsp->regs[SMSTATE0], svs_readl(svs, SMSTATE0),
> > + svsp->regs[SMSTATE1], svs_readl(svs, SMSTATE1));
> > + pr_err("TEMP(0x%x) = 0x%08x, TEMPMSR0(0x%x) = 0x%08x\n",
> > + svsp->regs[TEMP], svs_readl(svs, TEMP),
> > + svsp->regs[TEMPMSR0], svs_readl(svs, TEMPMSR0));
> > + pr_err("TEMPMSR1(0x%x) = 0x%08x, TEMPMSR2(0x%x) = 0x%08x\n",
> > + svsp->regs[TEMPMSR1], svs_readl(svs, TEMPMSR1),
> > + svsp->regs[TEMPMSR2], svs_readl(svs, TEMPMSR2));
> > + pr_err("TEMPMONCTL0(0x%x) = 0x%08x, TEMPMSRCTL1(0x%x) = 0x%08x\n",
> > + svsp->regs[TEMPMONCTL0], svs_readl(svs, TEMPMONCTL0),
> > + svsp->regs[TEMPMSRCTL1], svs_readl(svs, TEMPMSRCTL1));
> > +
> > + for (rg_i = TEMPMONCTL0; rg_i < reg_num; rg_i++)
> > + svsb->reg_data[SVSB_PHASE_MON][rg_i] = svs_readl(svs, rg_i);
> > +
> > + svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> > +
> > + if (svsb->phase != SVSB_PHASE_INIT01)
> > + svsb->phase = SVSB_PHASE_ERROR;
> > +
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_CLEAN, INTSTS);
> > +}
> > +
> > +static irqreturn_t svs_isr(int irq, void *data)
> > +{
> > + struct mtk_svs *svs = (struct mtk_svs *)data;
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb = NULL;
> > + unsigned long flags;
> > + u32 idx, int_sts, svs_en;
> > +
> > + flags = claim_mtk_svs_lock();
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svs->bank = svsb;
> > +
> > + if (svsb->int_st & svs_readl(svs, INTST))
> > + continue;
> > +
> > + if (!svsb->suspended) {
> > + svs_switch_bank(svs);
> > + int_sts = svs_readl(svs, INTSTS);
> > + svs_en = svs_readl(svs, SVSEN);
> > +
> > + if (int_sts == INTSTS_COMPLETE &&
> > + ((svs_en & SVSEN_MASK) == SVSEN_INIT01))
> > + svs_init01_isr_handler(svs);
> > + else if ((int_sts == INTSTS_COMPLETE) &&
> > + ((svs_en & SVSEN_MASK) == SVSEN_INIT02))
> > + svs_init02_isr_handler(svs);
> > + else if (!!(int_sts & INTSTS_MONVOP))
> > + svs_mon_mode_isr_handler(svs);
> > + else
> > + svs_error_isr_handler(svs);
> > + }
> > +
> > + break;
> > + }
> > + release_mtk_svs_lock(flags);
> > +
> > + if (svsb->phase != SVSB_PHASE_INIT01)
> > + svsb_set_volts(svsb, false);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void svs_mon_mode(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + unsigned long flags;
> > + u32 idx;
> > +
> > + flags = claim_mtk_svs_lock();
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svs->bank = svsb;
> > +
> > + if (!(svsb->mode_support & SVSB_MODE_MON))
> > + continue;
> > +
> > + svs_set_phase(svs, SVSB_PHASE_MON);
> > + }
> > + release_mtk_svs_lock(flags);
> > +}
> > +
> > +static int svs_init02(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + unsigned long flags, time_left;
> > + u32 idx;
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svs->bank = svsb;
> > +
> > + if (!(svsb->mode_support & SVSB_MODE_INIT02))
> > + continue;
> > +
> > + reinit_completion(&svsb->init_completion);
> > + flags = claim_mtk_svs_lock();
> > + svs_set_phase(svs, SVSB_PHASE_INIT02);
> > + release_mtk_svs_lock(flags);
> > + time_left =
> > + wait_for_completion_timeout(&svsb->init_completion,
> > + msecs_to_jiffies(2000));
> > + if (!time_left) {
> > + pr_err("%s: init02 completion timeout\n", svsb->name);
> > + return -EBUSY;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int svs_init01(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + struct pm_qos_request *qos_request;
> > + unsigned long flags, time_left;
> > + bool search_done;
> > + int ret = -EPERM;
> > + u32 opp_freqs, opp_vboot, buck_volt, idx, i;
> > +
> > + qos_request = kzalloc(sizeof(*qos_request), GFP_KERNEL);
> > + if (!qos_request)
> > + return -ENOMEM;
> > +
> > + /* Let CPUs leave idle-off state for initializing svs_init01. */
> > + cpu_latency_qos_add_request(qos_request, 0);
> > +
> > + /*
> > + * Sometimes two svs banks use the same buck.
> > + * Therefore, we set each svs bank to vboot voltage first.
> > + */
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + search_done = false;
> > +
> > + ret = regulator_set_mode(svsb->buck, REGULATOR_MODE_FAST);
> > + if (ret)
> > + pr_notice("%s: fail to set fast mode: %d\n",
> > + svsb->name, ret);
> > +
> > + if (svsb->mtcmos_request) {
> > + ret = regulator_enable(svsb->buck);
> > + if (ret) {
> > + pr_err("%s: fail to enable %s power: %d\n",
> > + svsb->name, svsb->buck_name, ret);
> > + goto init01_finish;
> > + }
> > +
> > + ret = dev_pm_domain_attach(svsb->dev, false);
> > + if (ret) {
> > + pr_err("%s: attach pm domain fail: %d\n",
> > + svsb->name, ret);
> > + goto init01_finish;
> > + }
> > +
> > + pm_runtime_enable(svsb->dev);
> > + ret = pm_runtime_get_sync(svsb->dev);
> > + if (ret < 0) {
> > + pr_err("%s: turn mtcmos on fail: %d\n",
> > + svsb->name, ret);
> > + goto init01_finish;
> > + }
> > + }
> > +
> > + /*
> > + * Find the fastest freq that can be run at vboot and
> > + * fix to that freq until svs_init01 is done.
> > + */
> > + opp_vboot = svsb_volt_to_opp_volt(svsb->vboot,
> > + svsb->volt_step,
> > + svsb->volt_base);
> > +
> > + for (i = 0; i < svsb->opp_count; i++) {
> > + opp_freqs = svsb->opp_freqs[i];
> > + if (!search_done && svsb->opp_volts[i] <= opp_vboot) {
> > + ret = dev_pm_opp_adjust_voltage(svsb->dev,
> > + opp_freqs,
> > + opp_vboot,
> > + opp_vboot,
> > + opp_vboot);
> > + if (ret) {
> > + pr_err("%s: set voltage failed: %d\n",
> > + svsb->name, ret);
> > + goto init01_finish;
> > + }
> > +
> > + search_done = true;
> > + } else {
> > + dev_pm_opp_disable(svsb->dev,
> > + svsb->opp_freqs[i]);
> > + }
> > + }
> > + }
> > +
> > + /* svs bank init01 begins */
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svs->bank = svsb;
> > +
> > + if (!(svsb->mode_support & SVSB_MODE_INIT01))
> > + continue;
> > +
> > + opp_vboot = svsb_volt_to_opp_volt(svsb->vboot,
> > + svsb->volt_step,
> > + svsb->volt_base);
> > +
> > + buck_volt = regulator_get_voltage(svsb->buck);
> > + if (buck_volt != opp_vboot) {
> > + pr_err("%s: buck voltage: %u, expected vboot: %u\n",
> > + svsb->name, buck_volt, opp_vboot);
> > + ret = -EPERM;
> > + goto init01_finish;
> > + }
> > +
> > + init_completion(&svsb->init_completion);
> > + flags = claim_mtk_svs_lock();
> > + svs_set_phase(svs, SVSB_PHASE_INIT01);
> > + release_mtk_svs_lock(flags);
> > + time_left =
> > + wait_for_completion_timeout(&svsb->init_completion,
> > + msecs_to_jiffies(2000));
> > + if (!time_left) {
> > + pr_err("%s: init01 completion timeout\n", svsb->name);
> > + ret = -EBUSY;
> > + goto init01_finish;
> > + }
> > + }
> > +
> > +init01_finish:
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > +
> > + for (i = 0; i < svsb->opp_count; i++)
> > + dev_pm_opp_enable(svsb->dev, svsb->opp_freqs[i]);
> > +
> > + if (regulator_set_mode(svsb->buck, REGULATOR_MODE_NORMAL))
> > + pr_notice("%s: fail to set normal mode: %d\n",
> > + svsb->name, ret);
> > +
> > + if (svsb->mtcmos_request) {
> > + if (pm_runtime_put_sync(svsb->dev))
> > + pr_err("%s: turn mtcmos off fail: %d\n",
> > + svsb->name, ret);
> > + pm_runtime_disable(svsb->dev);
> > + dev_pm_domain_detach(svsb->dev, 0);
> > + if (regulator_disable(svsb->buck))
> > + pr_err("%s: fail to disable %s power: %d\n",
> > + svsb->name, svsb->buck_name, ret);
> > + }
> > + }
> > +
> > + cpu_latency_qos_remove_request(qos_request);
> > + kfree(qos_request);
> > +
> > + return ret;
> > +}
> > +
> > +static int svs_start(struct mtk_svs *svs)
> > +{
> > + int ret;
> > +
> > + ret = svs_init01(svs);
> > + if (ret)
> > + return ret;
> > +
> > + ret = svs_init02(svs);
> > + if (ret)
> > + return ret;
> > +
> > + svs_mon_mode(svs);
> > +
> > + return ret;
> > +}
> > +
> > +static bool svs_mt8183_efuse_parsing(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct thermal_parameter tp;
> > + struct svs_bank *svsb;
> > + bool mon_mode_support = true;
> > + int format[6], x_roomt[6], tb_roomt = 0;
> > + struct nvmem_cell *cell;
> > + size_t len;
> > + u32 idx, i, ft_pgm, mts, temp0, temp1, temp2;
> > +
> > + if (svsp->fake_efuse) {
> > + pr_notice("fake efuse\n");
> > + svs->efuse[0] = 0x00310080;
> > + svs->efuse[1] = 0xabfbf757;
> > + svs->efuse[2] = 0x47c747c7;
> > + svs->efuse[3] = 0xabfbf757;
> > + svs->efuse[4] = 0xe7fca0ec;
> > + svs->efuse[5] = 0x47bf4b88;
> > + svs->efuse[6] = 0xabfb8fa5;
> > + svs->efuse[7] = 0xabfb217b;
> > + svs->efuse[8] = 0x4bf34be1;
> > + svs->efuse[9] = 0xabfb670d;
> > + svs->efuse[16] = 0xabfbc653;
> > + svs->efuse[17] = 0x47f347e1;
> > + svs->efuse[18] = 0xabfbd848;
> > + }
> > +
> > + for (i = 0; i < svsp->efuse_num; i++)
> > + if (svs->efuse[i])
> > + pr_notice("M_HW_RES%d: 0x%08x\n", i, svs->efuse[i]);
> > +
> > + /* svs efuse parsing */
> > + ft_pgm = (svs->efuse[0] >> 4) & GENMASK(3, 0);
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + if (ft_pgm <= 1)
> > + svsb->init01_volt_flag = SVSB_INIT01_VOLT_IGNORE;
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + svsb->bdes = svs->efuse[16] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[16] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[16] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[16] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = (svs->efuse[17] >> 16) & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 10;
> > + else
> > + svsb->volt_offset += 2;
> > + break;
> > + case SVS_CPU_BIG:
> > + svsb->bdes = svs->efuse[18] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[18] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[18] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[18] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = svs->efuse[17] & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 15;
> > + else
> > + svsb->volt_offset += 12;
> > + break;
> > + case SVS_CCI:
> > + svsb->bdes = svs->efuse[4] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[4] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[4] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[4] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = (svs->efuse[5] >> 16) & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 10;
> > + else
> > + svsb->volt_offset += 2;
> > + break;
> > + case SVS_GPU:
> > + svsb->bdes = svs->efuse[6] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[6] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[6] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[6] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = svs->efuse[5] & GENMASK(7, 0);
> > +
> > + if (ft_pgm >= 2) {
> > + svsb->freq_base = 800000000; /* 800MHz */
> > + svsb->dvt_fixed = 2;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (svsp->fake_efuse) {
> > + svs->thermal_efuse[0] = 0x02873f69;
> > + svs->thermal_efuse[1] = 0xa11d9142;
> > + svs->thermal_efuse[2] = 0xa2526900;
> > + } else {
> > + /* Get thermal efuse by nvmem */
> > + cell = nvmem_cell_get(svs->dev, "calibration-data");
> > + if (IS_ERR(cell)) {
> > + pr_err("no thermal efuse? disable mon mode\n");
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->mode_support &= ~SVSB_MODE_MON;
> > + }
> > +
> > + return true;
> > + }
> > +
> > + svs->thermal_efuse = (u32 *)nvmem_cell_read(cell, &len);
> > + nvmem_cell_put(cell);
> > + }
> > +
> > + /* Thermal efuse parsing */
> > + tp.adc_ge_t = (svs->thermal_efuse[1] >> 22) & GENMASK(9, 0);
> > + tp.adc_oe_t = (svs->thermal_efuse[1] >> 12) & GENMASK(9, 0);
> > +
> > + tp.o_vtsmcu1 = (svs->thermal_efuse[0] >> 17) & GENMASK(8, 0);
> > + tp.o_vtsmcu2 = (svs->thermal_efuse[0] >> 8) & GENMASK(8, 0);
> > + tp.o_vtsmcu3 = svs->thermal_efuse[1] & GENMASK(8, 0);
> > + tp.o_vtsmcu4 = (svs->thermal_efuse[2] >> 23) & GENMASK(8, 0);
> > + tp.o_vtsmcu5 = (svs->thermal_efuse[2] >> 5) & GENMASK(8, 0);
> > + tp.o_vtsabb = (svs->thermal_efuse[2] >> 14) & GENMASK(8, 0);
> > +
> > + tp.degc_cali = (svs->thermal_efuse[0] >> 1) & GENMASK(5, 0);
> > + tp.adc_cali_en_t = svs->thermal_efuse[0] & BIT(0);
> > + tp.o_slope_sign = (svs->thermal_efuse[0] >> 7) & BIT(0);
> > +
> > + tp.ts_id = (svs->thermal_efuse[1] >> 9) & BIT(0);
> > + tp.o_slope = (svs->thermal_efuse[0] >> 26) & GENMASK(5, 0);
> > +
> > + if (tp.adc_cali_en_t == 1) {
> > + if (!tp.ts_id)
> > + tp.o_slope = 0;
> > +
> > + if ((tp.adc_ge_t < 265 || tp.adc_ge_t > 758) ||
> > + (tp.adc_oe_t < 265 || tp.adc_oe_t > 758) ||
> > + (tp.o_vtsmcu1 < -8 || tp.o_vtsmcu1 > 484) ||
> > + (tp.o_vtsmcu2 < -8 || tp.o_vtsmcu2 > 484) ||
> > + (tp.o_vtsmcu3 < -8 || tp.o_vtsmcu3 > 484) ||
> > + (tp.o_vtsmcu4 < -8 || tp.o_vtsmcu4 > 484) ||
> > + (tp.o_vtsmcu5 < -8 || tp.o_vtsmcu5 > 484) ||
> > + (tp.o_vtsabb < -8 || tp.o_vtsabb > 484) ||
> > + (tp.degc_cali < 1 || tp.degc_cali > 63)) {
> > + pr_err("bad thermal efuse data, disable mon mode\n");
> > + mon_mode_support = false;
> > + }
> > + } else {
> > + pr_err("no thermal efuse data, disable mon mode\n");
> > + mon_mode_support = false;
> > + }
> > +
> > + if (!mon_mode_support) {
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->mode_support &= ~SVSB_MODE_MON;
> > + }
> > +
> > + return true;
> > + }
> > +
> > + tp.ge = ((tp.adc_ge_t - 512) * 10000) / 4096;
> > + tp.oe = (tp.adc_oe_t - 512);
> > + tp.gain = (10000 + tp.ge);
> > +
> > + format[0] = (tp.o_vtsmcu1 + 3350 - tp.oe);
> > + format[1] = (tp.o_vtsmcu2 + 3350 - tp.oe);
> > + format[2] = (tp.o_vtsmcu3 + 3350 - tp.oe);
> > + format[3] = (tp.o_vtsmcu4 + 3350 - tp.oe);
> > + format[4] = (tp.o_vtsmcu5 + 3350 - tp.oe);
> > + format[5] = (tp.o_vtsabb + 3350 - tp.oe);
> > +
> > + for (i = 0; i < 6; i++)
> > + x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / tp.gain;
> > +
> > + temp0 = (10000 * 100000 / tp.gain) * 15 / 18;
> > +
> > + if (!tp.o_slope_sign)
> > + mts = (temp0 * 10) / (1534 + tp.o_slope * 10);
> > + else
> > + mts = (temp0 * 10) / (1534 - tp.o_slope * 10);
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->mts = mts;
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + tb_roomt = x_roomt[3];
> > + break;
> > + case SVS_CPU_BIG:
> > + tb_roomt = x_roomt[4];
> > + break;
> > + case SVS_CCI:
> > + tb_roomt = x_roomt[3];
> > + break;
> > + case SVS_GPU:
> > + tb_roomt = x_roomt[1];
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + temp0 = (tp.degc_cali * 10 / 2);
> > + temp1 = ((10000 * 100000 / 4096 / tp.gain) *
> > + tp.oe + tb_roomt * 10) * 15 / 18;
> > +
> > + if (!tp.o_slope_sign)
> > + temp2 = temp1 * 100 / (1534 + tp.o_slope * 10);
> > + else
> > + temp2 = temp1 * 100 / (1534 - tp.o_slope * 10);
> > +
> > + svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static bool svs_is_supported(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct nvmem_cell *cell;
> > + size_t len;
> > + bool ret;
> > +
> > + if (svsp->fake_efuse) {
> > + len = svsp->efuse_num * sizeof(u32);
> > + svs->efuse = devm_kzalloc(svs->dev, len, GFP_KERNEL);
> > + if (!svs->efuse) {
> > + pr_err("no memory for allocating svs_efuse\n");
> > + return false;
> > + }
> > +
> > + len = svsp->thermal_efuse_num * sizeof(u32);
> > + svs->thermal_efuse = devm_kzalloc(svs->dev, len, GFP_KERNEL);
> > + if (!svs->thermal_efuse) {
> > + pr_err("no memory for allocating svs_thermal_efuse\n");
> > + return false;
> > + }
> > +
> > + goto svsp_efuse_parsing;
> > + }
> > +
> > + /* Get svs efuse by nvmem */
> > + cell = nvmem_cell_get(svs->dev, "svs-calibration-data");
> > + if (IS_ERR(cell)) {
> > + pr_err("no \"svs-calibration-data\" from dts? disable svs\n");
> > + return false;
> > + }
> > +
> > + svs->efuse = (u32 *)nvmem_cell_read(cell, &len);
> > + nvmem_cell_put(cell);
> > +
> > + if (!svs->efuse[svsp->efuse_check]) {
> > + pr_err("svs_efuse[%u] = 0x%x?\n",
> > + svsp->efuse_check, svs->efuse[svsp->efuse_check]);
> > + return false;
> > + }
> > +
> > +svsp_efuse_parsing:
> > + ret = svsp->efuse_parsing(svs);
> > +
> > + return ret;
> > +}
> > +
> > +static int svs_resource_setup(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + struct platform_device *pdev;
> > + struct device_node *np = NULL;
> > + struct dev_pm_opp *opp;
> > + unsigned long freq;
> > + int count, ret;
> > + u32 idx, i;
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + svsb->name = "SVS_CPU_LITTLE";
> > + break;
> > + case SVS_CPU_BIG:
> > + svsb->name = "SVS_CPU_BIG";
> > + break;
> > + case SVS_CCI:
> > + svsb->name = "SVS_CCI";
> > + break;
> > + case SVS_GPU:
> > + svsb->name = "SVS_GPU";
> > + break;
> > + default:
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > +
> > + /* Add svs bank device for opp-table/mtcmos/buck control */
> > + pdev = platform_device_alloc(svsb->name, 0);
> > + if (!pdev) {
> > + pr_err("%s: fail to alloc pdev for svs_bank\n",
> > + svsb->name);
> > + return -ENOMEM;
> > + }
> > +
> > + for_each_child_of_node(svs->dev->of_node, np) {
> > + if (of_device_is_compatible(np, svsb->of_compatible)) {
> > + pdev->dev.of_node = np;
> > + break;
> > + }
> > + }
> > +
> > + ret = platform_device_add(pdev);
> > + if (ret) {
> > + pr_err("%s: fail to add svs_bank device: %d\n",
> > + svsb->name, ret);
> > + return ret;
> > + }
> > +
> > + svsb->dev = &pdev->dev;
> > + dev_set_drvdata(svsb->dev, svs);
> > + ret = dev_pm_opp_of_add_table(svsb->dev);
> > + if (ret) {
> > + pr_err("%s: fail to add opp table: %d\n",
> > + svsb->name, ret);
> > + return ret;
> > + }
> > +
> > + mutex_init(&svsb->lock);
> > +
> > + svsb->buck = devm_regulator_get_optional(svsb->dev,
> > + svsb->buck_name);
> > + if (IS_ERR(svsb->buck)) {
> > + pr_err("%s: cannot get regulator \"%s-supply\"\n",
> > + svsb->name, svsb->buck_name);
> > + return PTR_ERR(svsb->buck);
> > + }
> > +
> > + count = dev_pm_opp_get_opp_count(svsb->dev);
> > + if (svsb->opp_count != count) {
> > + pr_err("%s: opp_count not \"%u\" but get \"%d\"?\n",
> > + svsb->name, svsb->opp_count, count);
> > + return count;
> > + }
> > +
> > + for (i = 0, freq = U32_MAX; i < svsb->opp_count; i++, freq--) {
> > + opp = dev_pm_opp_find_freq_floor(svsb->dev, &freq);
> > + if (IS_ERR(opp)) {
> > + pr_err("%s: error opp entry!!, err = %ld\n",
> > + svsb->name, PTR_ERR(opp));
> > + return PTR_ERR(opp);
> > + }
> > +
> > + svsb->opp_freqs[i] = freq;
> > + svsb->opp_volts[i] = dev_pm_opp_get_voltage(opp);
> > + svsb->freqs_pct[i] = percent(svsb->opp_freqs[i],
> > + svsb->freq_base);
> > + dev_pm_opp_put(opp);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int svs_suspend(struct device *dev)
> > +{
> > + struct mtk_svs *svs = dev_get_drvdata(dev);
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + unsigned long flags;
> > + int ret;
> > + u32 idx;
> > +
> > + /* Wait if there is processing svs_isr(). Suspend all banks. */
> > + flags = claim_mtk_svs_lock();
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svs->bank = svsb;
> > + svs_switch_bank(svs);
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_CLEAN, INTSTS);
> > + svsb->suspended = true;
> > + }
> > + release_mtk_svs_lock(flags);
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + if (svsb->phase != SVSB_PHASE_INIT01) {
> > + svsb->phase = SVSB_PHASE_ERROR;
> > + svsb_set_volts(svsb, true);
> > + }
> > + }
> > +
> > + if (svsp->need_hw_reset) {
> > + ret = device_reset(svs->dev);
> > + if (ret) {
> > + pr_err("%s: ret = %d\n", __func__, ret);
> > + return ret;
> > + }
> > + }
> > +
> > + clk_disable_unprepare(svs->main_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int svs_resume(struct device *dev)
> > +{
> > + struct mtk_svs *svs = dev_get_drvdata(dev);
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + int ret;
> > + u32 idx;
> > +
> > + ret = clk_prepare_enable(svs->main_clk);
> > + if (ret) {
> > + pr_err("cannot enable main_clk, disable svs\n");
> > + return ret;
> > + }
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->suspended = false;
> > + }
> > +
> > + ret = svs_init02(svs);
> > + if (ret)
> > + return ret;
> > +
> > + svs_mon_mode(svs);
> > +
> > + return 0;
> > +}
> > +
> > +static int svs_debug_proc_show(struct seq_file *m, void *v)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)m->private;
> > +
> > + if (svsb->phase == SVSB_PHASE_INIT01)
> > + seq_puts(m, "init1\n");
> > + else if (svsb->phase == SVSB_PHASE_INIT02)
> > + seq_puts(m, "init2\n");
> > + else if (svsb->phase == SVSB_PHASE_MON)
> > + seq_puts(m, "mon mode\n");
> > + else if (svsb->phase == SVSB_PHASE_ERROR)
> > + seq_puts(m, "disabled\n");
> > + else
> > + seq_puts(m, "unknown\n");
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t svs_debug_proc_write(struct file *file,
> > + const char __user *buffer,
> > + size_t count, loff_t *pos)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)PDE_DATA(file_inode(file));
> > + struct mtk_svs *svs = dev_get_drvdata(svsb->dev);
> > + unsigned long flags;
> > + int enabled, ret;
> > + char *buf = NULL;
> > +
> > + if (count >= PAGE_SIZE)
> > + return -EINVAL;
> > +
> > + buf = (char *)memdup_user_nul(buffer, count);
> > + if (IS_ERR(buf))
> > + return PTR_ERR(buf);
> > +
> > + ret = kstrtoint(buf, 10, &enabled);
> > + if (ret)
> > + return ret;
> > +
> > + if (!enabled) {
> > + flags = claim_mtk_svs_lock();
> > + svs->bank = svsb;
> > + svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> > + svs_switch_bank(svs);
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_CLEAN, INTSTS);
> > + release_mtk_svs_lock(flags);
> > +
> > + svsb->phase = SVSB_PHASE_ERROR;
> > + svsb_set_volts(svsb, true);
> > + }
> > +
> > + kfree(buf);
> > +
> > + return count;
> > +}
> > +
> > +proc_fops_rw(svs_debug);
> > +
> > +static int svs_dump_proc_show(struct seq_file *m, void *v)
> > +{
> > + struct mtk_svs *svs = (struct mtk_svs *)m->private;
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + unsigned long svs_reg_addr;
> > + u32 idx, i, j;
> > +
> > + for (i = 0; i < svsp->efuse_num; i++)
> > + if (svs->efuse && svs->efuse[i])
> > + seq_printf(m, "M_HW_RES%d = 0x%08x\n",
> > + i, svs->efuse[i]);
> > +
> > + for (i = 0; i < svsp->thermal_efuse_num; i++)
> > + if (svs->thermal_efuse)
> > + seq_printf(m, "THERMAL_EFUSE%d = 0x%08x\n",
> > + i, svs->thermal_efuse[i]);
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > +
> > + for (i = SVSB_PHASE_INIT01; i <= SVSB_PHASE_MON; i++) {
> > + seq_printf(m, "Bank_number = %u\n", svsb->bank_id);
> > +
> > + if (i < SVSB_PHASE_MON)
> > + seq_printf(m, "mode = init%d\n", i + 1);
> > + else
> > + seq_puts(m, "mode = mon\n");
> > +
> > + for (j = TEMPMONCTL0; j < reg_num; j++) {
> > + svs_reg_addr = (unsigned long)(svs->base +
> > + svsp->regs[j]);
> > + seq_printf(m, "0x%08lx = 0x%08x\n",
> > + svs_reg_addr, svsb->reg_data[i][j]);
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +proc_fops_ro(svs_dump);
> > +
> > +static int svs_status_proc_show(struct seq_file *m, void *v)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)m->private;
> > + struct dev_pm_opp *opp;
> > + unsigned long freq;
> > + int tzone_temp, ret;
> > + u32 i;
> > +
> > + ret = svsb_get_zone_temperature(svsb, &tzone_temp);
> > + if (ret)
> > + seq_printf(m, "%s: cannot get zone \"%s\" temperature\n",
> > + svsb->name, svsb->tzone_name);
> > + else
> > + seq_printf(m, "%s: temperature = %d\n", svsb->name, tzone_temp);
> > +
> > + for (i = 0, freq = U32_MAX; i < svsb->opp_count; i++, freq--) {
> > + opp = dev_pm_opp_find_freq_floor(svsb->dev, &freq);
> > + if (IS_ERR(opp)) {
> > + seq_printf(m, "%s: error opp entry!!, err = %ld\n",
> > + svsb->name, PTR_ERR(opp));
> > + return PTR_ERR(opp);
> > + }
> > +
> > + seq_printf(m, "opp_freqs[%02u]: %lu, volts[%02u]: %lu, ",
> > + i, freq, i, dev_pm_opp_get_voltage(opp));
> > + seq_printf(m, "svsb_volts[%02u]: 0x%x, freqs_pct[%02u]: %u\n",
> > + i, svsb->volts[i], i, svsb->freqs_pct[i]);
> > + dev_pm_opp_put(opp);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +proc_fops_ro(svs_status);
> > +
> > +static int svs_volt_offset_proc_show(struct seq_file *m, void *v)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)m->private;
> > +
> > + seq_printf(m, "%d\n", svsb->volt_offset);
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t svs_volt_offset_proc_write(struct file *file,
> > + const char __user *buffer,
> > + size_t count, loff_t *pos)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)PDE_DATA(file_inode(file));
> > + char *buf = NULL;
> > + s32 volt_offset;
> > +
> > + if (count >= PAGE_SIZE)
> > + return -EINVAL;
> > +
> > + buf = (char *)memdup_user_nul(buffer, count);
> > + if (IS_ERR(buf))
> > + return PTR_ERR(buf);
> > +
> > + if (!kstrtoint(buf, 10, &volt_offset)) {
> > + svsb->volt_offset = volt_offset;
> > + svsb_set_volts(svsb, true);
> > + }
> > +
> > + kfree(buf);
> > +
> > + return count;
> > +}
> > +
> > +proc_fops_rw(svs_volt_offset);
> > +
> > +static int svs_create_svs_procfs(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + struct proc_dir_entry *svs_dir, *bank_dir;
> > + u32 idx, i;
> > +
> > + struct pentry {
> > + const char *name;
> > + const struct proc_ops *fops;
> > + };
> > +
> > + struct pentry svs_entries[] = {
> > + proc_entry(svs_dump),
> > + };
> > +
> > + struct pentry bank_entries[] = {
> > + proc_entry(svs_debug),
> > + proc_entry(svs_status),
> > + proc_entry(svs_volt_offset),
> > + };
> > +
> > + svs_dir = proc_mkdir("svs", NULL);
> > + if (!svs_dir) {
> > + pr_err("mkdir /proc/svs failed\n");
> > + return -EPERM;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(svs_entries); i++) {
> > + if (!proc_create_data(svs_entries[i].name, 0664,
> > + svs_dir, svs_entries[i].fops, svs)) {
> > + pr_err("create /proc/svs/%s failed\n",
> > + svs_entries[i].name);
> > + return -EPERM;
> > + }
> > + }
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > +
> > + if (svsb->mode_support == SVSB_MODE_ALL_DISABLE)
> > + continue;
> > +
> > + bank_dir = proc_mkdir(svsb->name, svs_dir);
> > + if (!bank_dir) {
> > + pr_err("mkdir /proc/svs/%s failed\n", svsb->name);
> > + return -EPERM;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(bank_entries); i++) {
> > + if (!proc_create_data(bank_entries[i].name, 0664,
> > + bank_dir, bank_entries[i].fops,
> > + svsb)) {
> > + pr_err("create /proc/svs/%s/%s failed\n",
> > + svsb->name, bank_entries[i].name);
> > + return -EPERM;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct svs_bank_ops svs_mt8183_banks_ops = {
> > + .set_freqs_pct = svs_set_freqs_pct_v2,
> > + .get_vops = svs_get_vops_v2,
> > +};
> > +
> > +static struct svs_bank svs_mt8183_banks[4] = {
> > + {
> > + .of_compatible = "mediatek,mt8183-svs-cpu-little",
> > + .sw_id = SVS_CPU_LITTLE,
> > + .bank_id = 0,
> > + .ops = &svs_mt8183_banks_ops,
> > + .tzone_name = "tzts4",
> > + .buck_name = "vcpu-little",
> > + .mtcmos_request = false,
> > + .init01_volt_flag = SVSB_INIT01_VOLT_INC_ONLY,
> > + .mode_support = SVSB_MODE_INIT01 | SVSB_MODE_INIT02,
> > + .opp_count = 16,
> > + .freq_base = 1989000000,
> > + .vboot = 0x30,
> > + .volt_step = 6250,
> > + .volt_base = 500000,
> > + .volt_offset = 0,
> > + .vmax = 0x64,
> > + .vmin = 0x18,
> > + .dthi = 0x1,
> > + .dtlo = 0xfe,
> > + .det_window = 0xa28,
> > + .det_max = 0xffff,
> > + .age_config = 0x555555,
> > + .agem = 0,
> > + .dc_config = 0x555555,
> > + .dvt_fixed = 0x7,
> > + .vco = 0x10,
> > + .chk_shift = 0x77,
> > + .upper_temp_bound = 0x64,
> > + .lower_temp_bound = 0xb2,
> > + .high_temp_threashold = HIGH_TEMP_MAX,
> > + .low_temp_threashold = 25000,
> > + .low_temp_offset = 0,
> > + .core_sel = 0x8fff0000,
> > + .systemclk_en = BIT(31),
> > + .int_st = BIT(0),
> > + .ctl0 = 0x00010001,
> > + },
> > + {
> > + .of_compatible = "mediatek,mt8183-svs-cpu-big",
> > + .sw_id = SVS_CPU_BIG,
> > + .bank_id = 1,
> > + .ops = &svs_mt8183_banks_ops,
> > + .tzone_name = "tzts5",
> > + .buck_name = "vcpu-big",
> > + .mtcmos_request = false,
> > + .init01_volt_flag = SVSB_INIT01_VOLT_INC_ONLY,
> > + .mode_support = SVSB_MODE_INIT01 | SVSB_MODE_INIT02,
> > + .opp_count = 16,
> > + .freq_base = 1989000000,
> > + .vboot = 0x30,
> > + .volt_step = 6250,
> > + .volt_base = 500000,
> > + .volt_offset = 0,
> > + .vmax = 0x58,
> > + .vmin = 0x10,
> > + .dthi = 0x1,
> > + .dtlo = 0xfe,
> > + .det_window = 0xa28,
> > + .det_max = 0xffff,
> > + .age_config = 0x555555,
> > + .agem = 0,
> > + .dc_config = 0x555555,
> > + .dvt_fixed = 0x7,
> > + .vco = 0x10,
> > + .chk_shift = 0x77,
> > + .upper_temp_bound = 0x64,
> > + .lower_temp_bound = 0xb2,
> > + .high_temp_threashold = HIGH_TEMP_MAX,
> > + .low_temp_threashold = 25000,
> > + .low_temp_offset = 0,
> > + .core_sel = 0x8fff0001,
> > + .systemclk_en = BIT(31),
> > + .int_st = BIT(1),
> > + .ctl0 = 0x00000001,
> > + },
> > + {
> > + .of_compatible = "mediatek,mt8183-svs-cci",
> > + .sw_id = SVS_CCI,
> > + .bank_id = 2,
> > + .ops = &svs_mt8183_banks_ops,
> > + .tzone_name = "tzts4",
> > + .buck_name = "vcci",
> > + .mtcmos_request = false,
> > + .init01_volt_flag = SVSB_INIT01_VOLT_INC_ONLY,
> > + .mode_support = SVSB_MODE_INIT01 | SVSB_MODE_INIT02,
> > + .opp_count = 16,
> > + .freq_base = 1196000000,
> > + .vboot = 0x30,
> > + .volt_step = 6250,
> > + .volt_base = 500000,
> > + .volt_offset = 0,
> > + .vmax = 0x64,
> > + .vmin = 0x18,
> > + .dthi = 0x1,
> > + .dtlo = 0xfe,
> > + .det_window = 0xa28,
> > + .det_max = 0xffff,
> > + .age_config = 0x555555,
> > + .agem = 0,
> > + .dc_config = 0x555555,
> > + .dvt_fixed = 0x7,
> > + .vco = 0x10,
> > + .chk_shift = 0x77,
> > + .upper_temp_bound = 0x64,
> > + .lower_temp_bound = 0xb2,
> > + .high_temp_threashold = HIGH_TEMP_MAX,
> > + .low_temp_threashold = 25000,
> > + .low_temp_offset = 0,
> > + .core_sel = 0x8fff0002,
> > + .systemclk_en = BIT(31),
> > + .int_st = BIT(2),
> > + .ctl0 = 0x00100003,
> > + },
> > + {
> > + .of_compatible = "mediatek,mt8183-svs-gpu",
> > + .sw_id = SVS_GPU,
> > + .bank_id = 3,
> > + .ops = &svs_mt8183_banks_ops,
> > + .tzone_name = "tzts2",
> > + .buck_name = "vgpu",
> > + .mtcmos_request = true,
> > + .init01_volt_flag = SVSB_INIT01_VOLT_INC_ONLY,
> > + .mode_support = SVSB_MODE_INIT01 | SVSB_MODE_INIT02 |
> > + SVSB_MODE_MON,
> > + .opp_count = 16,
> > + .freq_base = 900000000,
> > + .vboot = 0x30,
> > + .volt_step = 6250,
> > + .volt_base = 500000,
> > + .volt_offset = 0,
> > + .vmax = 0x40,
> > + .vmin = 0x14,
> > + .dthi = 0x1,
> > + .dtlo = 0xfe,
> > + .det_window = 0xa28,
> > + .det_max = 0xffff,
> > + .age_config = 0x555555,
> > + .agem = 0,
> > + .dc_config = 0x555555,
> > + .dvt_fixed = 0x3,
> > + .vco = 0x10,
> > + .chk_shift = 0x77,
> > + .upper_temp_bound = 0x64,
> > + .lower_temp_bound = 0xb2,
> > + .high_temp_threashold = HIGH_TEMP_MAX,
> > + .low_temp_threashold = 25000,
> > + .low_temp_offset = 3,
> > + .core_sel = 0x8fff0003,
> > + .systemclk_en = BIT(31),
> > + .int_st = BIT(3),
> > + .ctl0 = 0x00050001,
> > + },
> > +};
> > +
> > +static const struct svs_platform svs_mt8183_platform = {
> > + .name = "mt8183-svs",
> > + .banks = svs_mt8183_banks,
> > + .efuse_parsing = svs_mt8183_efuse_parsing,
> > + .regs = svs_regs_v2,
> > + .irqflags = IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + .need_hw_reset = false,
> > + .fake_efuse = false,
> > + .bank_num = 4,
> > + .efuse_num = 25,
> > + .efuse_check = 2,
> > + .thermal_efuse_num = 3,
> > +};
> > +
> > +static const struct of_device_id mtk_svs_of_match[] = {
> > + {
> > + .compatible = "mediatek,mt8183-svs",
> > + .data = &svs_mt8183_platform,
> > + }, {
> > + /* Sentinel */
> > + },
> > +};
> > +
> > +static int svs_add_thermal_device_link(struct platform_device *svs_pdev)
> > +{
> > + struct device_node *therm_node;
> > + struct platform_device *therm_pdev;
> > + struct device_link *sup_link;
> > + char const *therm_node_name[] = {"thermal"};
> > + u32 i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(therm_node_name); i++) {
> > + therm_node = of_find_node_by_name(NULL, therm_node_name[i]);
> > + if (therm_node)
> > + break;
> > + }
> > +
> > + if (!therm_node) {
> > + pr_err("no available thermal node? pass device link\n");
> > + return 0;
> > + }
> > +
> > + therm_pdev = of_find_device_by_node(therm_node);
> > + if (!therm_pdev) {
> > + pr_err("no \"%pOF\" platform device? pass device link\n",
> > + therm_node);
> > + of_node_put(therm_node);
> > + return 0;
> > + }
> > +
> > + of_node_put(therm_node);
> > +
> > + sup_link = device_link_add(&svs_pdev->dev, &therm_pdev->dev,
> > + DL_FLAG_AUTOREMOVE_CONSUMER);
> > +
> > + if (sup_link->status == DL_STATE_DORMANT)
> > + return -EPROBE_DEFER;
> > +
> > + return 0;
> > +}
> > +
> > +static int svs_probe(struct platform_device *pdev)
> > +{
> > + const struct of_device_id *of_dev_id;
> > + struct mtk_svs *svs;
> > + int ret;
> > + u32 svs_irq;
> > +
> > + ret = svs_add_thermal_device_link(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + svs = devm_kzalloc(&pdev->dev, sizeof(*svs), GFP_KERNEL);
> > + if (!svs)
> > + return -ENOMEM;
> > +
> > + svs->dev = &pdev->dev;
> > + if (!svs->dev->of_node) {
> > + pr_err("cannot find device node\n");
> > + return -ENODEV;
> > + }
> > +
> > + svs->base = of_iomap(svs->dev->of_node, 0);
> > + if (IS_ERR(svs->base)) {
> > + pr_err("cannot find svs register base\n");
> > + return PTR_ERR(svs->base);
> > + }
> > +
> > + of_dev_id = of_match_node(mtk_svs_of_match, svs->dev->of_node);
> > + if (!of_dev_id || !of_dev_id->data)
> > + return -EINVAL;
> > +
> > + svs->platform = of_dev_id->data;
> > + dev_set_drvdata(svs->dev, svs);
> > +
> > + svs_irq = irq_of_parse_and_map(svs->dev->of_node, 0);
> > + ret = devm_request_threaded_irq(svs->dev, svs_irq, NULL, svs_isr,
> > + svs->platform->irqflags, "mtk-svs",
> > + svs);
> > + if (ret) {
> > + pr_err("register irq(%d) failed: %d\n", svs_irq, ret);
> > + return ret;
> > + }
> > +
> > + svs->main_clk = devm_clk_get(svs->dev, "main");
> > + if (IS_ERR(svs->main_clk)) {
> > + pr_err("failed to get clock: %ld\n", PTR_ERR(svs->main_clk));
> > + return PTR_ERR(svs->main_clk);
> > + }
> > +
> > + ret = clk_prepare_enable(svs->main_clk);
> > + if (ret) {
> > + pr_err("cannot enable main clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + if (!svs_is_supported(svs)) {
> > + pr_notice("svs is not supported\n");
> > + ret = -EPERM;
> > + goto svs_probe_fail;
> > + }
> > +
> > + ret = svs_resource_setup(svs);
> > + if (ret)
> > + goto svs_probe_fail;
> > +
> > + ret = svs_start(svs);
> > + if (ret)
> > + goto svs_probe_fail;
> > +
> > + ret = svs_create_svs_procfs(svs);
> > + if (ret)
> > + goto svs_probe_fail;
> > +
> > + return 0;
> > +
> > +svs_probe_fail:
> > + clk_disable_unprepare(svs->main_clk);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct dev_pm_ops svs_pm_ops = {
> > + .suspend = svs_suspend,
> > + .resume = svs_resume,
> > +};
>
> Better use the PM macros to define that struct
Ok. I'll use PM macros instead. Thanks.
>
> > +
> > +static struct platform_driver svs_driver = {
> > + .probe = svs_probe,
> > + .driver = {
> > + .name = "mtk-svs",
> > + .pm = &svs_pm_ops,
> > + .of_match_table = of_match_ptr(mtk_svs_of_match),
> > + },
> > +};
> > +
> > +static int __init svs_init(void)
> > +{
> > + int ret;
> > +
> > + ret = platform_driver_register(&svs_driver);
> > + if (ret) {
> > + pr_err("svs platform driver register failed: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +late_initcall_sync(svs_init);
>
> Why? Can't be this a module_platform_driver?
There is an error when SVS uses module_platform_driver?() as below.
- In mt8183, GPU OPP table associates with "2" opp-microvolt property (OPP table generally supplies "1")
and it cause SVS GPU fails to do dev_pm_opp_of_add_table() when using module_platform_driver().
- Looks like SVS-GPU needs GPU driver handle its "two opp-microvolt property" first.
#GPU OPP table with two opp-microvolt property
https://patchwork.kernel.org/patch/11423009/
[ 8.504165] platform SVS_GPU.0: opp_parse_supplies: Invalid number of elements in opp-microvolt property (2) with supplies (1)
[ 8.515545] platform SVS_GPU.0: _of_add_opp_table_v2: Failed to add OPP, -22
[ 8.522600] [mtk_svs] SVS_GPU: fail to add opp table: -22
>
> > +
> > +MODULE_DESCRIPTION("MediaTek SVS Driver v1.0");
>
> Don't put the version in the description, there is a MODULE_VERSION
> for that purpose.
>
> Also would be good have the MODULE_AUTHOR
Sure. Thanks for pointing it out.
>
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/power/mtk_svs.h b/include/linux/power/mtk_svs.h
> > new file mode 100644
> > index 000000000000..5c03982e3576
> > --- /dev/null
> > +++ b/include/linux/power/mtk_svs.h
>
> I don't think this file is needed just remove it.
This export is used by mt8183 thermal.
https://patchwork.kernel.org/patch/11452835/
>
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Copyright (C) 2020 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_SVS_H__
> > +#define __MTK_SVS_H__
> > +
> > +#if IS_ENABLED(CONFIG_MTK_SVS)
> > +unsigned long claim_mtk_svs_lock(void);
> > +void release_mtk_svs_lock(unsigned long flags);
> > +#else
> > +static inline unsigned long claim_mtk_svs_lock(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void release_mtk_svs_lock(unsigned long flags)
> > +{
> > +}
> > +#endif /* CONFIG_MTK_SVS */
> > +
> > +#endif /* __MTK_SVS_H__ */
> > --
> > 2.18.0
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply
* Re: [PATCH 09/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus
From: Robin Murphy @ 2020-05-22 9:42 UTC (permalink / raw)
To: Rob Herring, Lorenzo Pieralisi
Cc: devicetree, Catalin Marinas, Will Deacon, Diana Craciun, PCI,
Sudeep Holla, Rafael J. Wysocki, Makarand Pawagi, linux-acpi,
Linux IOMMU, Marc Zyngier, Hanjun Guo, Bjorn Helgaas,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_Jsq+h18gH2D3B-OZku6ACCgonPUJcUnrN8a5=jApsXHdB5Q@mail.gmail.com>
On 2020-05-22 00:10, Rob Herring wrote:
> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> The existing bindings cannot be used to specify the relationship
>> between fsl-mc devices and GIC ITSes.
>>
>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>> msi-map property.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>> .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> index 9134e9bcca56..b0813b2d0493 100644
>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit value called an ICID
>> the requester.
>>
>> The generic 'iommus' property is insufficient to describe the relationship
>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>> -the set of possible ICIDs under a root DPRC and how they map to
>> -an IOMMU.
>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
>> +to define the set of possible ICIDs under a root DPRC and how they map to
>> +an IOMMU and a GIC ITS respectively.
>>
>> For generic IOMMU bindings, see
>> Documentation/devicetree/bindings/iommu/iommu.txt.
>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>> For arm-smmu binding, see:
>> Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>
>> +For GICv3 and GIC ITS bindings, see:
>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
>> +
>> Required properties:
>>
>> - compatible
>> @@ -119,6 +122,15 @@ Optional properties:
>> associated with the listed IOMMU, with the iommu-specifier
>> (i - icid-base + iommu-base).
>>
>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>> + data.
>> +
>> + The property is an arbitrary number of tuples of
>> + (icid-base,iommu,iommu-base,length).
>
> I'm confused because the example has GIC ITS phandle, not an IOMMU.
>
> What is an iommu-base?
Right, I was already halfway through writing a reply to say that all the
copy-pasted "iommu" references here should be using the terminology from
the pci-msi.txt binding instead.
>> +
>> + Any ICID in the interval [icid-base, icid-base + length) is
>> + associated with the listed GIC ITS, with the iommu-specifier
>> + (i - icid-base + iommu-base).
>> Example:
>>
>> smmu: iommu@5000000 {
>> @@ -128,6 +140,16 @@ Example:
>> ...
>> };
>>
>> + gic: interrupt-controller@6000000 {
>> + compatible = "arm,gic-v3";
>> + ...
>> + its: gic-its@6020000 {
>> + compatible = "arm,gic-v3-its";
>> + msi-controller;
>> + ...
>> + };
>> + };
>> +
>> fsl_mc: fsl-mc@80c000000 {
>> compatible = "fsl,qoriq-mc";
>> reg = <0x00000008 0x0c000000 0 0x40>, /* MC portal base */
>> @@ -135,6 +157,8 @@ Example:
>> msi-parent = <&its>;
Side note: is it right to keep msi-parent here? It rather implies that
the MC itself has a 'native' Device ID rather than an ICID, which I
believe is not strictly true. Plus it's extra-confusing that it doesn't
specify an ID either way, since that makes it look like the legacy PCI
case that gets treated implicitly as an identity msi-map, which makes no
sense at all to combine with an actual msi-map.
>> /* define map for ICIDs 23-64 */
>> iommu-map = <23 &smmu 23 41>;
>> + /* define msi map for ICIDs 23-64 */
>> + msi-map = <23 &its 23 41>;
>
> Seeing 23 twice is odd. The numbers to the right of 'its' should be an
> ITS number space.
On about 99% of systems the values in the SMMU Stream ID and ITS Device
ID spaces are going to be the same. Nobody's going to bother carrying
*two* sets of sideband data across the interconnect if they don't have to ;)
Robin.
>> #address-cells = <3>;
>> #size-cells = <1>;
>>
>> --
>> 2.26.1
>>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
^ permalink raw reply
* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Dongchun Zhu @ 2020-05-22 9:44 UTC (permalink / raw)
To: Tomasz Figa
Cc: Rob Herring, linus.walleij, bgolaszewski, mchehab,
andriy.shevchenko, mark.rutland, sakari.ailus, drinkcat,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang, dongchun.zhu
In-Reply-To: <20200521193525.GB14214@chromium.org>
Hi Tomasz, Rob,
On Thu, 2020-05-21 at 19:35 +0000, Tomasz Figa wrote:
> Hi Rob,
>
> On Mon, May 11, 2020 at 11:02:07AM -0500, Rob Herring wrote:
> > On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
> > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > > .../bindings/media/i2c/ovti,ov02a10.yaml | 184 +++++++++++++++++++++
> > > MAINTAINERS | 7 +
> > > 2 files changed, 191 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > new file mode 100644
> > > index 0000000..5468d1b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > @@ -0,0 +1,184 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright (c) 2020 MediaTek Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > > +
> > > +maintainers:
> > > + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +
> > > +description: |-
> > > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > > + image sensor, which is the latest production derived from Omnivision's CMOS
> > > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > > + sensor output is available via CSI-2 serial data output.
> > > +
> > > +properties:
> > > + compatible:
> > > + const: ovti,ov02a10
> > > +
> > > + reg:
> > > + description: I2C device address
> >
> > Drop this. Nothing specific to this device.
> >
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + items:
> > > + - description: top mux camtg clock
> > > + - description: devider clock
> >
> > typo
> >
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: eclk
> > > + - const: freq_mux
> > > +
> > > + clock-frequency:
> > > + description:
> > > + Frequency of the eclk clock in Hertz.
> > > +
> > > + dovdd-supply:
> > > + description:
> > > + Definition of the regulator used as interface power supply.
> > > +
> > > + avdd-supply:
> > > + description:
> > > + Definition of the regulator used as analog power supply.
> > > +
> > > + dvdd-supply:
> > > + description:
> > > + Definition of the regulator used as digital power supply.
> > > +
> > > + powerdown-gpios:
> > > + maxItems: 1
> > > +
> > > + reset-gpios:
> > > + maxItems: 1
>
> I asked a question about defining GPIO polarities some time ago, but I
> guess it slipped through.
>
> The chip documentation calls the reset pin as "RST_PAD (low level
> reset)". Where should the inversion be handled, in the driver or here,
> by having the DT include a necessary flag in the specifier?
>
> Best regards,
> Tomasz
For powerdown-gpios and reset-gpios, I actually defined two totally
different GPIO polarities in DT according to OV02A10 chip documentation.
One is GPIO_ACTIVE_LOW, the other is GPIO_ACTIVE_HIGH (see examples
below).
So I'm wondering if we could add such one polarity-flag that Tomasz
suggested.
^ permalink raw reply
* Re: [PATCH 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
From: Lorenzo Pieralisi @ 2020-05-22 9:53 UTC (permalink / raw)
To: Makarand Pawagi
Cc: Laurentiu Tudor, linux-arm-kernel@lists.infradead.org,
Diana Madalina Craciun (OSS), iommu@lists.linux-foundation.org,
linux-acpi@vger.kernel.org, devicetree@vger.kernel.org,
linux-pci@vger.kernel.org, Rob Herring, Rafael J. Wysocki,
Joerg Roedel, Hanjun Guo, Bjorn Helgaas, Sudeep Holla,
Robin Murphy, Catalin Marinas, Will Deacon, Marc Zyngier
In-Reply-To: <AM6PR04MB49847931D51AD92057043D92EBB40@AM6PR04MB4984.eurprd04.prod.outlook.com>
On Fri, May 22, 2020 at 05:32:02AM +0000, Makarand Pawagi wrote:
[...]
> > Subject: Re: [PATCH 12/12] bus: fsl-mc: Add ACPI support for fsl-mc
> >
> > Hi Lorenzo,
> >
> > On 5/21/2020 4:00 PM, Lorenzo Pieralisi wrote:
> > > From: Diana Craciun <diana.craciun@oss.nxp.com>
> > >
> > > Add ACPI support in the fsl-mc driver. Driver parses MC DSDT table to
> > > extract memory and other resources.
> > >
> > > Interrupt (GIC ITS) information is extracted from the MADT table by
> > > drivers/irqchip/irq-gic-v3-its-fsl-mc-msi.c.
> > >
> > > IORT table is parsed to configure DMA.
> > >
> > > Signed-off-by: Makarand Pawagi <makarand.pawagi@nxp.com>
> > > Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
> > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > > ---
> >
> > The author of this patch should be Makarand. I think I accidentaly broke it when
> > we exchanged the patches. Very sorry about it.
> >
>
> Will you be able to correct this or should I post another patch?
I will update it.
Lorenzo
^ permalink raw reply
* Re: [PATCH v2 0/3] Re-introduce TX FIFO resize for larger EP bursting
From: Felipe Balbi @ 2020-05-22 9:54 UTC (permalink / raw)
To: Wesley Cheng, agross, bjorn.andersson, robh+dt, gregkh
Cc: linux-arm-msm, devicetree, linux-kernel, linux-usb, jackp,
Wesley Cheng
In-Reply-To: <1590050169-30747-1-git-send-email-wcheng@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 398 bytes --]
Wesley Cheng <wcheng@codeaurora.org> writes:
> Changes in V2:
> - Modified TXFIFO resizing logic to ensure that each EP is reserved a
> FIFO.
> - Removed dev_dbg() prints and fixed typos from patches
> - Added some more description on the dt-bindings commit message
>
> Reviewed-by: Felipe Balbi <balbi@kernel.org>
I don't remember giving you a Reviewed-by, did I?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 09/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus
From: Diana Craciun OSS @ 2020-05-22 9:57 UTC (permalink / raw)
To: Robin Murphy, Rob Herring, Lorenzo Pieralisi
Cc: devicetree, Catalin Marinas, Will Deacon, PCI, Sudeep Holla,
Rafael J. Wysocki, Makarand Pawagi, linux-acpi, Linux IOMMU,
Marc Zyngier, Hanjun Guo, Bjorn Helgaas,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <abca6ecb-5d93-832f-ff7c-de53bb6203f3@arm.com>
On 5/22/2020 12:42 PM, Robin Murphy wrote:
> On 2020-05-22 00:10, Rob Herring wrote:
>> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>>
>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>
>>> The existing bindings cannot be used to specify the relationship
>>> between fsl-mc devices and GIC ITSes.
>>>
>>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>>> msi-map property.
>>>
>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> ---
>>> .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30
>>> +++++++++++++++++--
>>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> index 9134e9bcca56..b0813b2d0493 100644
>>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit
>>> value called an ICID
>>> the requester.
>>>
>>> The generic 'iommus' property is insufficient to describe the
>>> relationship
>>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>>> -the set of possible ICIDs under a root DPRC and how they map to
>>> -an IOMMU.
>>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties
>>> are used
>>> +to define the set of possible ICIDs under a root DPRC and how they
>>> map to
>>> +an IOMMU and a GIC ITS respectively.
>>>
>>> For generic IOMMU bindings, see
>>> Documentation/devicetree/bindings/iommu/iommu.txt.
>>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>>> For arm-smmu binding, see:
>>> Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>>
>>> +For GICv3 and GIC ITS bindings, see:
>>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
>>>
>>> +
>>> Required properties:
>>>
>>> - compatible
>>> @@ -119,6 +122,15 @@ Optional properties:
>>> associated with the listed IOMMU, with the iommu-specifier
>>> (i - icid-base + iommu-base).
>>>
>>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>>> + data.
>>> +
>>> + The property is an arbitrary number of tuples of
>>> + (icid-base,iommu,iommu-base,length).
>>
>> I'm confused because the example has GIC ITS phandle, not an IOMMU.
>>
>> What is an iommu-base?
>
> Right, I was already halfway through writing a reply to say that all
> the copy-pasted "iommu" references here should be using the
> terminology from the pci-msi.txt binding instead.
Right, will change it.
>
>>> +
>>> + Any ICID in the interval [icid-base, icid-base + length) is
>>> + associated with the listed GIC ITS, with the iommu-specifier
>>> + (i - icid-base + iommu-base).
>>> Example:
>>>
>>> smmu: iommu@5000000 {
>>> @@ -128,6 +140,16 @@ Example:
>>> ...
>>> };
>>>
>>> + gic: interrupt-controller@6000000 {
>>> + compatible = "arm,gic-v3";
>>> + ...
>>> + its: gic-its@6020000 {
>>> + compatible = "arm,gic-v3-its";
>>> + msi-controller;
>>> + ...
>>> + };
>>> + };
>>> +
>>> fsl_mc: fsl-mc@80c000000 {
>>> compatible = "fsl,qoriq-mc";
>>> reg = <0x00000008 0x0c000000 0 0x40>, /* MC
>>> portal base */
>>> @@ -135,6 +157,8 @@ Example:
>>> msi-parent = <&its>;
>
> Side note: is it right to keep msi-parent here? It rather implies that
> the MC itself has a 'native' Device ID rather than an ICID, which I
> believe is not strictly true. Plus it's extra-confusing that it
> doesn't specify an ID either way, since that makes it look like the
> legacy PCI case that gets treated implicitly as an identity msi-map,
> which makes no sense at all to combine with an actual msi-map.
Before adding msi-map, the fsl-mc code assumed that ICID and streamID
are equal and used msi-parent just to get the reference to the ITS node.
Removing msi-parent will break the backward compatibility of the already
existing systems. Maybe we should mention that this is legacy and not to
be used for newer device trees.
>
>>> /* define map for ICIDs 23-64 */
>>> iommu-map = <23 &smmu 23 41>;
>>> + /* define msi map for ICIDs 23-64 */
>>> + msi-map = <23 &its 23 41>;
>>
>> Seeing 23 twice is odd. The numbers to the right of 'its' should be an
>> ITS number space.
>
> On about 99% of systems the values in the SMMU Stream ID and ITS
> Device ID spaces are going to be the same. Nobody's going to bother
> carrying *two* sets of sideband data across the interconnect if they
> don't have to ;)
>
> Robin.
Diana
>
>>> #address-cells = <3>;
>>> #size-cells = <1>;
>>>
>>> --
>>> 2.26.1
>>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
^ permalink raw reply
* [PATCH] drm/mediatek: dsi: fix scrolling of panel with small hfp or hbp
From: Jitao Shi @ 2020-05-22 10:12 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Matthias Brugger, Daniel Vetter,
David Airlie, dri-devel, linux-kernel
Cc: linux-mediatek, devicetree, linux-arm-kernel, srv_heupstream,
yingjoe.chen, eddie.huang, cawa.cheng, bibby.hsieh, ck.hu,
stonea168, huijuan.xie, Jitao Shi
If panel has too small hfp or hbp, horizontal_frontporch_byte or
horizontal_backporch_byte may become very small value or negative
value. This patch adjusts their values so that they are greater
than minimum value and keep total of them unchanged.
Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 0ede69830a9d..aebaafd90ceb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -148,6 +148,9 @@
(type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
(type == MIPI_DSI_DCS_READ))
+#define MIN_HFP_BYTE 0x02
+#define MIN_HBP_BYTE 0x02
+
struct mtk_phy_timing {
u32 lpx;
u32 da_hs_prepare;
@@ -450,6 +453,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
u32 horizontal_sync_active_byte;
u32 horizontal_backporch_byte;
u32 horizontal_frontporch_byte;
+ s32 signed_hfp_byte, signed_hbp_byte;
u32 dsi_tmp_buf_bpp, data_phy_cycles;
struct mtk_phy_timing *timing = &dsi->phy_timing;
@@ -519,6 +523,20 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
}
}
+ signed_hfp_byte = (s32)horizontal_frontporch_byte;
+ signed_hbp_byte = (s32)horizontal_backporch_byte;
+
+ if (signed_hfp_byte + signed_hbp_byte < MIN_HFP_BYTE + MIN_HBP_BYTE) {
+ DRM_WARN("Calculated hfp_byte and hbp_byte are too small, "
+ "panel may not work properly.\n");
+ } else if (signed_hfp_byte < MIN_HFP_BYTE) {
+ horizontal_frontporch_byte = MIN_HFP_BYTE;
+ horizontal_backporch_byte -= MIN_HFP_BYTE - signed_hfp_byte;
+ } else if (signed_hbp_byte < MIN_HBP_BYTE) {
+ horizontal_frontporch_byte -= MIN_HBP_BYTE - signed_hbp_byte;
+ horizontal_backporch_byte = MIN_HBP_BYTE;
+ }
+
writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC);
writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v7 13/24] iommu/arm-smmu-v3: Enable broadcast TLB maintenance
From: Jean-Philippe Brucker @ 2020-05-22 10:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: Will Deacon, iommu, devicetree, linux-arm-kernel, linux-pci,
linux-mm, joro, catalin.marinas, robin.murphy, kevin.tian,
baolu.lu, Jonathan.Cameron, jacob.jun.pan, christian.koenig,
felix.kuehling, zhangfei.gao, jgg, xuzaibo, fenghua.yu, hch,
eric.auger
In-Reply-To: <0c896ad27b43b2de554cf772f9453d0a@kernel.org>
[+Eric]
On Thu, May 21, 2020 at 03:38:35PM +0100, Marc Zyngier wrote:
> On 2020-05-21 15:17, Will Deacon wrote:
> > [+Marc]
> >
> > On Tue, May 19, 2020 at 07:54:51PM +0200, Jean-Philippe Brucker wrote:
> > > The SMMUv3 can handle invalidation targeted at TLB entries with shared
> > > ASIDs. If the implementation supports broadcast TLB maintenance,
> > > enable it
> > > and keep track of it in a feature bit. The SMMU will then be
> > > affected by
> > > inner-shareable TLB invalidations from other agents.
> > >
> > > A major side-effect of this change is that stage-2 translation
> > > contexts
> > > are now affected by all invalidations by VMID. VMIDs are all shared
> > > and
> > > the only ways to prevent over-invalidation, since the stage-2 page
> > > tables
> > > are not shared between CPU and SMMU, are to either disable BTM or
> > > allocate
> > > different VMIDs. This patch does not address the problem.
> >
> > This sounds like a potential performance issue, particularly as we
> > expose
> > stage-2 contexts via VFIO directly.
Yes it's certainly going to affect SMMU performance, though I haven't
measured it. QEMU and kvmtool currently use stage-1 translations instead
of stage-2, so it won't be a problem until they start using nested
translation (and unless the SMMU only supports stage-2).
In the coming month I'd like to have a look at coordinating VMID
allocation between KVM and SMMU, for guest SVA. If the guest wants to
share page tables with the SMMU, the SMMU has to use the same VMIDs as the
VM to receive broadcast TLBI.
Similarly to patch 06 ("arm64: mm: Pin down ASIDs for sharing mm with
devices") the SMMU would request a VMID allocated by KVM, when setting up
a nesting VFIO container. One major downside is that the VMID is pinned
and cannot be recycled on rollover while it's being used for DMA.
I wonder if we could use this even when page tables aren't shared between
CPU and SMMU, to avoid splitting the VMID space.
> > Maybe we could reserve some portion
> > of
> > VMID space for the SMMU? Marc, what do you reckon?
>
> Certainly doable when we have 16bits VMIDs. With smaller VMID spaces (like
> on
> v8.0), this is a bit more difficult (we do have pretty large v8.0 systems
> around).
It's only an issue if those systems have an SMMUv3 supporting DVM. With
any luck that doesn't exist?
> How many VMID bits are we talking about?
That's anyone's guess... One passed-through device per VM would halve the
VMID space. But the SMMU allocates one VMID for each device assigned to a
guest, not one per VM (well one per domain, or VFIO container, but I think
it boils down to one per device with QEMU). So with SR-IOV for example it
should be pretty easy to reach 256 VMIDs in the SMMU.
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver
From: Jon Hunter @ 2020-05-22 10:23 UTC (permalink / raw)
To: Sandipan Patra, treding, robh+dt, u.kleine-koenig
Cc: bbasu, ldewangan, linux-pwm, devicetree, linux-tegra,
linux-kernel
In-Reply-To: <1587398043-18767-1-git-send-email-spatra@nvidia.com>
On 20/04/2020 16:54, Sandipan Patra wrote:
> Added support for dynamic clock freq configuration in pwm kernel driver.
> Earlier the pwm driver used to cache boot time clock rate by pwm clock
> parent during probe. Hence dynamically changing pwm frequency was not
> possible for all the possible ranges. With this change, dynamic calculation
> is enabled and it is able to set the requested period from sysfs knob
> provided the value is supported by clock source.
>
> Changes mainly have 2 parts:
> - T186 and later chips [1]
> - T210 and prior chips [2]
>
> For [1] - Changes implemented to set pwm period dynamically and
> also checks added to allow only if requested period(ns) is
> below or equals to higher range.
>
> For [2] - Only checks if the requested period(ns) is below or equals
> to higher range defined by max clock limit. The limitation
> in T210 or prior chips are due to the reason of having only
> one pwm-controller supporting multiple channels. But later
> chips have multiple pwm controller instances each having
> single channel support.
>
> Signed-off-by: Sandipan Patra <spatra@nvidia.com>
> ---
> V2:
> 1. Min period_ns calculation is moved to probe.
> 2. Added descriptioins for PWM register bits and regarding behaviour
> of the controller when new configuration is applied or pwm is disabled.
> 3. Setting period with possible value when supplied period is below limit.
> 4. Corrected the earlier code comment:
> plus 1 instead of minus 1 during pwm calculation
>
> drivers/pwm/pwm-tegra.c | 110 +++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 94 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index d26ed8f..7a36325 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -4,8 +4,39 @@
> *
> * Tegra pulse-width-modulation controller driver
> *
> - * Copyright (c) 2010, NVIDIA Corporation.
> - * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <s.hauer@pengutronix.de>
> + * Copyright (c) 2010-2020, NVIDIA Corporation.
> + *
> + * Overview of Tegra Pulse Width Modulator Register:
> + * 1. 13-bit: Frequency division (SCALE)
> + * 2. 8-bit : Puls division (DUTY)
> + * 3. 1-bit : Enable bit
> + *
> + * The PWM clock frequency is divided by 256 before subdividing it based
> + * on the programmable frequency division value to generate the required
> + * frequency for PWM output. The maximum output frequency that can be
> + * achieved is (max rate of source clock) / 256.
> + * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
> + * 408 MHz/256 = 1.6 MHz.
> + * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
> + *
> + * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
> + * To achieve 100% duty cycle, program Bit [24] of this register to
> + * 1’b1. In which case the other bits [23:16] are set to don't care.
> + *
> + * Limitations and known facts:
> + * - When PWM is disabled, the output is driven to 0.
> + * - It does not allow the current PWM period to complete and
> + * stops abruptly.
> + *
> + * - If the register is reconfigured while pwm is running,
> + * It does not let the currently running period to complete.
> + *
> + * - Pulse width of the pwm can never be out of bound.
> + * It's taken care at HW and SW
> + * - If the user input duty is below limit, then driver sets it to
> + * minimum possible value.
> + * - If anything else goes wrong for setting duty or period,
> + * -EINVAL is returned.
> */
>
> #include <linux/clk.h>
> @@ -41,6 +72,7 @@ struct tegra_pwm_chip {
> struct reset_control*rst;
>
> unsigned long clk_rate;
> + unsigned long min_period_ns;
>
> void __iomem *regs;
>
> @@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> - unsigned long long c = duty_ns, hz;
> - unsigned long rate;
> + unsigned long long p_width = duty_ns, period_hz;
> + unsigned long rate, required_clk_rate;
> + unsigned long pfm; /* Frequency divider */
If it is not necessary to change the variable names, then I would prefer
we keep them as is as then changes would be less.
> u32 val = 0;
> int err;
>
> @@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
> * nearest integer during division.
> */
> - c *= (1 << PWM_DUTY_WIDTH);
> - c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
> + p_width *= (1 << PWM_DUTY_WIDTH);
> + p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
>
> - val = (u32)c << PWM_DUTY_SHIFT;
> + val = (u32)p_width << PWM_DUTY_SHIFT;
> +
> + /*
> + * Period in nano second has to be <= highest allowed period
> + * based on max clock rate of the pwm controller.
> + *
> + * higher limit = max clock limit >> PWM_DUTY_WIDTH
> + * lower limit = min clock limit >> PWM_DUTY_WIDTH >> PWM_SCALE_WIDTH
> + */
> + if (period_ns < pc->min_period_ns) {
> + period_ns = pc->min_period_ns;
> + pr_warn("Period is adjusted to allowed value (%d ns)\n",
> + period_ns);
I see that other drivers (pwm-img.c) consider this to be an error and
return an error. I wonder if adjusting the period makes sense here?
I wonder if the handling of the min_period, should be a separate change?
> + }
>
> /*
> * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
> * cycles at the PWM clock rate will take period_ns nanoseconds.
> */
> - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> + if (pc->soc->num_channels == 1) {
Are you using num_channels to determine if Tegra uses the BPMP? If so
then the above is not really correct, because num_channels is not really
related to what is being done here. So maybe you need a new SoC
attribute in the soc data.
> + /*
> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it matches
> + * with the hieghest applicable rate that the controller can
s/hieghest/highest/
> + * provide. Any further lower value can be derived by setting
> + * PFM bits[0:12].
> + * Higher mark is taken since BPMP has round-up mechanism
> + * implemented.
> + */
> + required_clk_rate =
> + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> +
Should be we checking the rate against the max rate supported?
> + err = clk_set_rate(pc->clk, required_clk_rate);
> + if (err < 0)
> + return -EINVAL;
> +
> + rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
Do we need to update the pwm->clk_rate here?
> + } else {
> + /*
> + * This is the case for SoCs who support multiple channels:
> + *
> + * clk_set_rate() can not be called again in config because
> + * T210 or any prior chip supports one pwm-controller and
> + * multiple channels. Hence in this case cached clock rate
> + * will be considered which was stored during probe.
> + */
> + rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> + }
>
> /* Consider precision in PWM_SCALE_WIDTH rate calculation */
> - hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> - rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> + period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> + pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
>
> /*
> * Since the actual PWM divider is the register's frequency divider
> - * field minus 1, we need to decrement to get the correct value to
> + * field plus 1, we need to decrement to get the correct value to
> * write to the register.
> */
> - if (rate > 0)
> - rate--;
> + if (pfm > 0)
> + pfm--;
>
> /*
> - * Make sure that the rate will fit in the register's frequency
> + * Make sure that pfm will fit in the register's frequency
> * divider field.
> */
> - if (rate >> PWM_SCALE_WIDTH)
> + if (pfm >> PWM_SCALE_WIDTH)
> return -EINVAL;
>
> - val |= rate << PWM_SCALE_SHIFT;
> + val |= pfm << PWM_SCALE_SHIFT;
>
> /*
> * If the PWM channel is disabled, make sure to turn on the clock
> @@ -205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> */
> pwm->clk_rate = clk_get_rate(pwm->clk);
>
> + /* Set minimum limit of PWM period for the IP */
> + pwm->min_period_ns =
> + (NSEC_PER_SEC / (pwm->soc->max_frequency >> PWM_DUTY_WIDTH)) + 1;
> +
> pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
> if (IS_ERR(pwm->rst)) {
> ret = PTR_ERR(pwm->rst);
> @@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("NVIDIA Corporation");
> +MODULE_AUTHOR("Sandipan Patra <spatra@nvidia.com>");
> MODULE_ALIAS("platform:tegra-pwm");
>
--
nvpublic
^ permalink raw reply
* Re: [PATCH v3 1/2] arm64: dts: imx8mm-evk: correct ldo1/ldo2 voltage range
From: Fabio Estevam @ 2020-05-22 10:24 UTC (permalink / raw)
To: Robin Gong
Cc: Rob Herring, Shawn Guo, Sascha Hauer, Yongcai Huang, Peng Fan,
Leonard Crestez, Dong Aisheng, Sascha Hauer,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, NXP Linux Team
In-Reply-To: <1590144291-18526-1-git-send-email-yibin.gong@nxp.com>
On Thu, May 21, 2020 at 11:44 PM Robin Gong <yibin.gong@nxp.com> wrote:
>
> Correct ldo1 voltage range from wrong high group(3.0V~3.3V) to low group
> (1.6V~1.9V) because the ldo1 should be 1.8V. Actually, two voltage groups
> have been supported at bd718x7-regulator driver, hence, just corrrect the
> voltage range to 1.6V~3.3V. For ldo2@0.8V, correct voltage range too.
> Otherwise, ldo1 would be kept @3.0V and ldo2@0.9V which violate i.mx8mm
> datasheet as the below warning log in kernel:
>
> [ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
> [ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
>
> Fixes: 78cc25fa265d ("arm64: dts: imx8mm-evk: Add BD71847 PMIC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply
* Re: [PATCH v3 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2 voltage range
From: Fabio Estevam @ 2020-05-22 10:24 UTC (permalink / raw)
To: Robin Gong
Cc: Rob Herring, Shawn Guo, Sascha Hauer, Yongcai Huang, Peng Fan,
Leonard Crestez, Dong Aisheng, Sascha Hauer,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, NXP Linux Team
In-Reply-To: <1590144291-18526-2-git-send-email-yibin.gong@nxp.com>
On Thu, May 21, 2020 at 11:44 PM Robin Gong <yibin.gong@nxp.com> wrote:
>
> Correct ldo1 voltage range from wrong high group(3.0V~3.3V) to low group
> (1.6V~1.9V) because the ldo1 should be 1.8V. Actually, two voltage groups
> have been supported at bd718x7-regulator driver, hence, just corrrect the
> voltage range to 1.6V~3.3V. For ldo2@0.8V, correct voltage range too.
> Otherwise, ldo1 would be kept @3.0V and ldo2@0.9V which violate i.mx8mn
> datasheet as the below warning log in kernel:
>
> [ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
> [ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
>
> Fixes: 3e44dd09736d ("arm64: dts: imx8mn-ddr4-evk: Add rohm,bd71847 PMIC support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 1/2] drivers: thermal: tsens: Add zeroc interrupt support
From: manafm @ 2020-05-22 10:32 UTC (permalink / raw)
To: Amit Kucheria
Cc: Andy Gross, Bjorn Andersson, Zhang Rui, Daniel Lezcano,
Rob Herring, Linux PM list, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <CAHLCerNct=4HGXiwxWaupBTm15+Z2yY1RW_BRBgiGGAk62svJw@mail.gmail.com>
On 2020-05-20 17:12, Amit Kucheria wrote:
> On Sun, May 17, 2020 at 4:17 PM Manaf Meethalavalappu Pallikunhi
> <manafm@codeaurora.org> wrote:
>>
>> TSENS IP v2.6+ adds zeroc interrupt support. It triggers set
>
> As I re-read through these patches, shouldn't we just call it the
> "cold" interrupt?
Renamed zeroc with cold everywhere
>
>> interrupt when aggregated minimum temperature of all TSENS falls
>> below zeroc preset threshold and triggers reset interrupt when
>
> Again, cold would just capture the intent much better given that it
> doesn't even triggered at zero but at 5 degrees. And this value could
> change in firmware.
>
>> aggregated minimum temperature of all TSENS crosses above reset
>> threshold. Add support for this interrupt in the driver.
>>
>> It adds another sensor to the of-thermal along with all individual
>> TSENS. It enables to add any mitigation for zeroc interrupt.
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi
>> <manafm@codeaurora.org>
>> ---
>> drivers/thermal/qcom/tsens-v2.c | 5 ++
>> drivers/thermal/qcom/tsens.c | 107
>> +++++++++++++++++++++++++++++++-
>> drivers/thermal/qcom/tsens.h | 10 +++
>> 3 files changed, 120 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>> b/drivers/thermal/qcom/tsens-v2.c
>> index b293ed32174b..8f30b859ab22 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -23,6 +23,7 @@
>> #define TM_Sn_UPPER_LOWER_THRESHOLD_OFF 0x0020
>> #define TM_Sn_CRITICAL_THRESHOLD_OFF 0x0060
>> #define TM_Sn_STATUS_OFF 0x00a0
>> +#define TM_ZEROC_INT_STATUS_OFF 0x00e0
>> #define TM_TRDY_OFF 0x00e4
>> #define TM_WDOG_LOG_OFF 0x013c
>>
>> @@ -86,6 +87,9 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> REG_FIELD_FOR_EACH_SENSOR16(CRITICAL_STATUS, TM_Sn_STATUS_OFF,
>> 19, 19),
>> REG_FIELD_FOR_EACH_SENSOR16(MAX_STATUS, TM_Sn_STATUS_OFF,
>> 20, 20),
>>
>> + /* ZEROC INTERRUPT STATUS */
>> + [ZEROC_STATUS] = REG_FIELD(TM_ZEROC_INT_STATUS_OFF, 0, 0),
>> +
>> /* TRDY: 1=ready, 0=in progress */
>> [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>> };
>> @@ -93,6 +97,7 @@ static const struct reg_field
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>> static const struct tsens_ops ops_generic_v2 = {
>> .init = init_common,
>> .get_temp = get_temp_tsens_valid,
>> + .get_zeroc_status = get_tsens_zeroc_status,
>> };
>>
>> struct tsens_plat_data data_tsens_v2 = {
>> diff --git a/drivers/thermal/qcom/tsens.c
>> b/drivers/thermal/qcom/tsens.c
>> index 8d3e94d2a9ed..dd0172f05eb6 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -205,7 +205,8 @@ static void tsens_set_interrupt_v1(struct
>> tsens_priv *priv, u32 hw_id,
>> index = LOW_INT_CLEAR_0 + hw_id;
>> break;
>> case CRITICAL:
>> - /* No critical interrupts before v2 */
>> + case ZEROC:
>> + /* No critical and zeroc interrupts before v2 */
>> return;
>> }
>> regmap_field_write(priv->rf[index], enable ? 0 : 1);
>> @@ -236,6 +237,9 @@ static void tsens_set_interrupt_v2(struct
>> tsens_priv *priv, u32 hw_id,
>> index_mask = CRIT_INT_MASK_0 + hw_id;
>> index_clear = CRIT_INT_CLEAR_0 + hw_id;
>> break;
>> + case ZEROC:
>> + /* Nothing to handle for zeroc interrupt */
>> + return;
>> }
>>
>> if (enable) {
>> @@ -367,6 +371,35 @@ static inline u32 masked_irq(u32 hw_id, u32 mask,
>> enum tsens_ver ver)
>> return 0;
>> }
>>
>> +/**
>> + * tsens_zeroc_irq_thread - Threaded interrupt handler for zeroc
>> interrupt
>> + * @irq: irq number
>> + * @data: tsens controller private data
>> + *
>> + * Whenever interrupt triggers notify thermal framework using
>> + * thermal_zone_device_update().
>> + *
>> + * Return: IRQ_HANDLED
>> + */
>> +
>> +irqreturn_t tsens_zeroc_irq_thread(int irq, void *data)
>> +{
>> + struct tsens_priv *priv = data;
>> + struct tsens_sensor *s = &priv->sensor[priv->num_sensors];
>> + int temp, ret;
>> +
>> + ret = regmap_field_read(priv->rf[ZEROC_STATUS], &temp);
>> + if (ret)
>> + return ret;
>> +
>> + dev_dbg(priv->dev, "[%u] %s: zeroc interrupt is %s\n",
>> + s->hw_id, __func__, temp ? "triggered" : "cleared");
>
> Rename temp to cold or something similar since you're not really
> returning temperature but a boolean state on whether we're in cold
> zone or not.
Renamed zeroc with cold everywhere
>
>> +
>> + thermal_zone_device_update(s->tzd, THERMAL_EVENT_UNSPECIFIED);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> /**
>> * tsens_critical_irq_thread() - Threaded handler for critical
>> interrupts
>> * @irq: irq number
>> @@ -575,6 +608,20 @@ void tsens_disable_irq(struct tsens_priv *priv)
>> regmap_field_write(priv->rf[INT_EN], 0);
>> }
>>
>> +int get_tsens_zeroc_status(const struct tsens_sensor *s, int *temp)
>> +{
>> + struct tsens_priv *priv = s->priv;
>> + int last_temp = 0, ret;
>> +
>> + ret = regmap_field_read(priv->rf[ZEROC_STATUS], &last_temp);
>> + if (ret)
>> + return ret;
>> +
>> + *temp = last_temp;
>
> same here. Don't use temp and last_temp. Use cold and prev_cold, for
> example.
Done
>
>> +
>> + return 0;
>> +}
>> +
>> int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
>> {
>> struct tsens_priv *priv = s->priv;
>> @@ -843,6 +890,19 @@ int __init init_common(struct tsens_priv *priv)
>> regmap_field_write(priv->rf[CC_MON_MASK], 1);
>> }
>>
>> + if (tsens_version(priv) > VER_1_X && ver_minor > 5) {
>> + /* ZEROC interrupt is present only on v2.6+ */
>> + priv->feat->zeroc_int = 1;
>> + priv->rf[ZEROC_STATUS] = devm_regmap_field_alloc(
>> + dev,
>> + priv->tm_map,
>> +
>> priv->fields[ZEROC_STATUS]);
>> + if (IS_ERR(priv->rf[ZEROC_STATUS])) {
>> + ret = PTR_ERR(priv->rf[ZEROC_STATUS]);
>> + goto err_put_device;
>> + }
>> + }
>> +
>> spin_lock_init(&priv->ul_lock);
>> tsens_enable_irq(priv);
>> tsens_debug_init(op);
>> @@ -852,6 +912,17 @@ int __init init_common(struct tsens_priv *priv)
>> return ret;
>> }
>>
>> +static int tsens_zeroc_get_temp(void *data, int *temp)
>
> Add kernel doc to this function since it doesn't return temperature,
> but a cold state, 0 or 1, on success.
Done
>
> One question: You need to poll this value from userspace, right? For
> the userspace interface being discussed on the list currently, you
> would still not get automatic notifications for this interrupt unless
> you plan to add trip points that will cause thermal core to trigger
> userspace events.
As discussed, we will be adding a thermal zone devicetree node with trip
threshold equal to 1 which could use either userspace or stepwise
governor.
I will update thermal zone example in binding doc.
>
>> +{
>> + struct tsens_sensor *s = data;
>> + struct tsens_priv *priv = s->priv;
>> +
>> + if (priv->ops->get_zeroc_status)
>> + return priv->ops->get_zeroc_status(s, temp);
>> +
>> + return -ENOTSUPP;
>> +}
>> +
>> static int tsens_get_temp(void *data, int *temp)
>> {
>> struct tsens_sensor *s = data;
>> @@ -923,6 +994,10 @@ static const struct thermal_zone_of_device_ops
>> tsens_of_ops = {
>> .set_trips = tsens_set_trips,
>> };
>>
>> +static const struct thermal_zone_of_device_ops tsens_zeroc_of_ops = {
>> + .get_temp = tsens_zeroc_get_temp,
>> +};
>> +
>> static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>> irq_handler_t thread_fn)
>> {
>> @@ -980,6 +1055,21 @@ static int tsens_register(struct tsens_priv
>> *priv)
>> ret = tsens_register_irq(priv, "critical",
>> tsens_critical_irq_thread);
>>
>> + if (priv->feat->zeroc_int && priv->zeroc_en) {
>> + priv->sensor[priv->num_sensors].priv = priv;
>> + tzd = devm_thermal_zone_of_sensor_register(priv->dev,
>> +
>> priv->sensor[priv->num_sensors].hw_id,
>> +
>> &priv->sensor[priv->num_sensors],
>> + &tsens_zeroc_of_ops);
>> + if (IS_ERR(tzd)) {
>> + ret = 0;
>> + return ret;
>> + }
>> +
>> + priv->sensor[priv->num_sensors].tzd = tzd;
>> + ret = tsens_register_irq(priv, "zeroc",
>> tsens_zeroc_irq_thread);
>> + }
>> +
>> return ret;
>> }
>>
>> @@ -992,6 +1082,7 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> const struct tsens_plat_data *data;
>> const struct of_device_id *id;
>> u32 num_sensors;
>> + bool zeroc_en = false;
>>
>> if (pdev->dev.of_node)
>> dev = &pdev->dev;
>> @@ -1016,6 +1107,12 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> return -EINVAL;
>> }
>>
>> + /* Check whether zeroc interrupt is enabled or not */
>> + if (platform_get_irq_byname(pdev, "zeroc") > 0) {
>> + zeroc_en = true;
>
> This check can be done in tsens_register_irq() from tsens_register. It
> is OK to have an extra struct tsens_sensor allocated even on platforms
> that don't have it.
Done
>
>> + num_sensors++;
>> + }
>> +
>> priv = devm_kzalloc(dev,
>> struct_size(priv, sensor, num_sensors),
>
> make this num_sensors + 1 to account for the zeroc virtual sensor. See
> below.
Changed this code in v3
>
>> GFP_KERNEL);
>> @@ -1023,7 +1120,7 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> return -ENOMEM;
>>
>> priv->dev = dev;
>> - priv->num_sensors = num_sensors;
>> + priv->num_sensors = zeroc_en ? num_sensors - 1 : num_sensors;
>> priv->ops = data->ops;
>> for (i = 0; i < priv->num_sensors; i++) {
>> if (data->hw_ids)
>> @@ -1031,6 +1128,12 @@ static int tsens_probe(struct platform_device
>> *pdev)
>> else
>> priv->sensor[i].hw_id = i;
>> }
>> +
>> + if (zeroc_en) {
>> + priv->zeroc_en = zeroc_en;
>> + priv->sensor[num_sensors].hw_id =
>> data->feat->max_sensors;
>
> This is going to break as soon as we have a platform that actually
> uses all 16 sensors. i.e. you won't have a spare one to use if
> num_sensors >= max_sensors.
>
> I think you should introduce a new member 'struct tsens_sensor
> zeroc_sensor' in tsens_priv and avoid playing with num_sensors
> completely.
Removed this code in v3
>
>> + }
>> +
>> priv->feat = data->feat;
>> priv->fields = data->fields;
>>
>> diff --git a/drivers/thermal/qcom/tsens.h
>> b/drivers/thermal/qcom/tsens.h
>> index 59d01162c66a..34d24332b320 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -34,6 +34,7 @@ enum tsens_irq_type {
>> LOWER,
>> UPPER,
>> CRITICAL,
>> + ZEROC,
>> };
>>
>> /**
>> @@ -64,6 +65,7 @@ struct tsens_sensor {
>> * @suspend: Function to suspend the tsens device
>> * @resume: Function to resume the tsens device
>> * @get_trend: Function to get the thermal/temp trend
>> + * @get_zeroc_status: Function to get the zeroc interrupt status
>> */
>> struct tsens_ops {
>> /* mandatory callbacks */
>> @@ -76,6 +78,7 @@ struct tsens_ops {
>> int (*suspend)(struct tsens_priv *priv);
>> int (*resume)(struct tsens_priv *priv);
>> int (*get_trend)(struct tsens_sensor *s, enum thermal_trend
>> *trend);
>> + int (*get_zeroc_status)(const struct tsens_sensor *s, int
>> *temp);
>
> this fn doesn't return temp, but a boolean status of whether we're in
> cold zone or not. Replace 'int *temp' with 'boolean *zeroc'?
Updated
>
>> };
>>
>> #define REG_FIELD_FOR_EACH_SENSOR11(_name, _offset, _startbit,
>> _stopbit) \
>> @@ -485,6 +488,8 @@ enum regfield_ids {
>> MAX_STATUS_14,
>> MAX_STATUS_15,
>>
>> + ZEROC_STATUS, /* ZEROC INTERRUPT status */
>
> Indent comment at same level as others above it
Done
>
>
>> +
>> /* Keep last */
>> MAX_REGFIELDS
>> };
>> @@ -497,6 +502,7 @@ enum regfield_ids {
>> * @srot_split: does the IP neatly splits the register space into
>> SROT and TM,
>> * with SROT only being available to secure boot
>> firmware?
>> * @has_watchdog: does this IP support watchdog functionality?
>> + * @zeroc_int: does this IP support ZEROC interrupt ?
>> * @max_sensors: maximum sensors supported by this version of the IP
>> */
>> struct tsens_features {
>> @@ -505,6 +511,7 @@ struct tsens_features {
>> unsigned int adc:1;
>> unsigned int srot_split:1;
>> unsigned int has_watchdog:1;
>> + unsigned int zeroc_int:1;
>> unsigned int max_sensors;
>> };
>>
>> @@ -549,6 +556,7 @@ struct tsens_context {
>> * @feat: features of the IP
>> * @fields: bitfield locations
>> * @ops: pointer to list of callbacks supported by this device
>> + * @zeroc_en: variable to check zeroc interrupt sensor is enabled or
>> not
>> * @debug_root: pointer to debugfs dentry for all tsens
>> * @debug: pointer to debugfs dentry for tsens controller
>> * @sensor: list of sensors attached to this device
>> @@ -568,6 +576,7 @@ struct tsens_priv {
>> struct tsens_features *feat;
>> const struct reg_field *fields;
>> const struct tsens_ops *ops;
>> + bool zeroc_en;
>>
>> struct dentry *debug_root;
>> struct dentry *debug;
>> @@ -580,6 +589,7 @@ void compute_intercept_slope(struct tsens_priv
>> *priv, u32 *pt1, u32 *pt2, u32 mo
>> int init_common(struct tsens_priv *priv);
>> int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
>> int get_temp_common(const struct tsens_sensor *s, int *temp);
>> +int get_tsens_zeroc_status(const struct tsens_sensor *s, int *temp);
>>
>> /* TSENS target */
>> extern struct tsens_plat_data data_8960;
>> --
>> 2.26.2
^ permalink raw reply
* Re: [PATCH v2 1/4] dt-bindings: iio: imu: bmi160: convert txt format to yaml
From: Daniel Baluta @ 2020-05-22 10:47 UTC (permalink / raw)
To: Jonathan Albrieux, Jonathan Cameron
Cc: Jonathan Cameron, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
Rob Herring
In-Reply-To: <20200522082208.GB19742@ict14-OptiPlex-980>
>>>>> +
>>>>> +maintainers:
>>>>> + - can't find a mantainer, author is Daniel Baluta <daniel.baluta@intel.com>
>>>> Daniel is still active in the kernel, just not at Intel any more. +CC
>>>>
>>> Oh ok thank you! Daniel are you still maintaining this driver?
I can do reviews if requested but I'm not actively maintaining this
driver. If anyone wants
to take this over, will be more than happy.
Other than that we can add my gmail address: Daniel Baluta
<daniel.baluta@gmail.com>
^ permalink raw reply
* RE: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver
From: Sandipan Patra @ 2020-05-22 11:01 UTC (permalink / raw)
To: Jonathan Hunter, Thierry Reding, robh+dt@kernel.org,
u.kleine-koenig@pengutronix.de
Cc: Bibek Basu, Laxman Dewangan, linux-pwm@vger.kernel.org,
devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <34ae18f5-494c-7bc7-0f30-2d1455bbcb55@nvidia.com>
Thanks Jonathan,
Please help reviewing further with my replies inline.
Thanks & Regards,
Sandipan
> -----Original Message-----
> From: Jonathan Hunter <jonathanh@nvidia.com>
> Sent: Friday, May 22, 2020 3:54 PM
> To: Sandipan Patra <spatra@nvidia.com>; Thierry Reding
> <treding@nvidia.com>; robh+dt@kernel.org; u.kleine-koenig@pengutronix.de
> Cc: Bibek Basu <bbasu@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM
> driver
>
>
> On 20/04/2020 16:54, Sandipan Patra wrote:
> > Added support for dynamic clock freq configuration in pwm kernel driver.
> > Earlier the pwm driver used to cache boot time clock rate by pwm clock
> > parent during probe. Hence dynamically changing pwm frequency was not
> > possible for all the possible ranges. With this change, dynamic
> > calculation is enabled and it is able to set the requested period from
> > sysfs knob provided the value is supported by clock source.
> >
> > Changes mainly have 2 parts:
> > - T186 and later chips [1]
> > - T210 and prior chips [2]
> >
> > For [1] - Changes implemented to set pwm period dynamically and
> > also checks added to allow only if requested period(ns) is
> > below or equals to higher range.
> >
> > For [2] - Only checks if the requested period(ns) is below or equals
> > to higher range defined by max clock limit. The limitation
> > in T210 or prior chips are due to the reason of having only
> > one pwm-controller supporting multiple channels. But later
> > chips have multiple pwm controller instances each having
> > single channel support.
> >
> > Signed-off-by: Sandipan Patra <spatra@nvidia.com>
> > ---
> > V2:
> > 1. Min period_ns calculation is moved to probe.
> > 2. Added descriptioins for PWM register bits and regarding behaviour
> > of the controller when new configuration is applied or pwm is disabled.
> > 3. Setting period with possible value when supplied period is below limit.
> > 4. Corrected the earlier code comment:
> > plus 1 instead of minus 1 during pwm calculation
> >
> > drivers/pwm/pwm-tegra.c | 110
> > +++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 94 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index
> > d26ed8f..7a36325 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -4,8 +4,39 @@
> > *
> > * Tegra pulse-width-modulation controller driver
> > *
> > - * Copyright (c) 2010, NVIDIA Corporation.
> > - * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer
> > <s.hauer@pengutronix.de>
> > + * Copyright (c) 2010-2020, NVIDIA Corporation.
> > + *
> > + * Overview of Tegra Pulse Width Modulator Register:
> > + * 1. 13-bit: Frequency division (SCALE)
> > + * 2. 8-bit : Puls division (DUTY)
> > + * 3. 1-bit : Enable bit
> > + *
> > + * The PWM clock frequency is divided by 256 before subdividing it
> > + based
> > + * on the programmable frequency division value to generate the
> > + required
> > + * frequency for PWM output. The maximum output frequency that can be
> > + * achieved is (max rate of source clock) / 256.
> > + * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
> > + * 408 MHz/256 = 1.6 MHz.
> > + * This 1.6 MHz frequency can further be divided using SCALE value in PWM.
> > + *
> > + * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
> > + * To achieve 100% duty cycle, program Bit [24] of this register to
> > + * 1’b1. In which case the other bits [23:16] are set to don't care.
> > + *
> > + * Limitations and known facts:
> > + * - When PWM is disabled, the output is driven to 0.
> > + * - It does not allow the current PWM period to complete and
> > + * stops abruptly.
> > + *
> > + * - If the register is reconfigured while pwm is running,
> > + * It does not let the currently running period to complete.
> > + *
> > + * - Pulse width of the pwm can never be out of bound.
> > + * It's taken care at HW and SW
> > + * - If the user input duty is below limit, then driver sets it to
> > + * minimum possible value.
> > + * - If anything else goes wrong for setting duty or period,
> > + * -EINVAL is returned.
> > */
> >
> > #include <linux/clk.h>
> > @@ -41,6 +72,7 @@ struct tegra_pwm_chip {
> > struct reset_control*rst;
> >
> > unsigned long clk_rate;
> > + unsigned long min_period_ns;
> >
> > void __iomem *regs;
> >
> > @@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,
> struct pwm_device *pwm,
> > int duty_ns, int period_ns)
> > {
> > struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> > - unsigned long long c = duty_ns, hz;
> > - unsigned long rate;
> > + unsigned long long p_width = duty_ns, period_hz;
> > + unsigned long rate, required_clk_rate;
> > + unsigned long pfm; /* Frequency divider */
>
> If it is not necessary to change the variable names, then I would prefer we keep
> them as is as then changes would be less.
The earlier name was misleading so thought to use a specific name for
which it can be helpful to follow up with the TRM. Since its recommended
to retain the variable names, I will change this in next patch.
>
> > u32 val = 0;
> > int err;
> >
> > @@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip *chip,
> struct pwm_device *pwm,
> > * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
> > * nearest integer during division.
> > */
> > - c *= (1 << PWM_DUTY_WIDTH);
> > - c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
> > + p_width *= (1 << PWM_DUTY_WIDTH);
> > + p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
> >
> > - val = (u32)c << PWM_DUTY_SHIFT;
> > + val = (u32)p_width << PWM_DUTY_SHIFT;
> > +
> > + /*
> > + * Period in nano second has to be <= highest allowed period
> > + * based on max clock rate of the pwm controller.
> > + *
> > + * higher limit = max clock limit >> PWM_DUTY_WIDTH
> > + * lower limit = min clock limit >> PWM_DUTY_WIDTH >>
> PWM_SCALE_WIDTH
> > + */
> > + if (period_ns < pc->min_period_ns) {
> > + period_ns = pc->min_period_ns;
> > + pr_warn("Period is adjusted to allowed value (%d ns)\n",
> > + period_ns);
>
> I see that other drivers (pwm-img.c) consider this to be an error and return an
> error. I wonder if adjusting the period makes sense here?
>
> I wonder if the handling of the min_period, should be a separate change?
I think I misunderstood one of the discussions in initial patch and added this change
to apply the minimum possible value. Understood and will revert this change
with returning error in such case.
>
> > + }
> >
> > /*
> > * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
> > * cycles at the PWM clock rate will take period_ns nanoseconds.
> > */
> > - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> > + if (pc->soc->num_channels == 1) {
>
> Are you using num_channels to determine if Tegra uses the BPMP? If so then the
> above is not really correct, because num_channels is not really related to what is
> being done here. So maybe you need a new SoC attribute in the soc data.
Here, it tries to find if pwm controller uses multiple channels (like in Tegra210 or older)
or single channel for every pwm instance (i.e. T186, T194). If found multiple channels on
a single controller then it is not correct to configure separate clock rates to each of the
channels. So to distinguish the controller and channel type, num_channels is referred.
>
> > + /*
> > + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it
> matches
> > + * with the hieghest applicable rate that the controller can
>
> s/hieghest/highest/
Got it.
>
> > + * provide. Any further lower value can be derived by setting
> > + * PFM bits[0:12].
> > + * Higher mark is taken since BPMP has round-up mechanism
> > + * implemented.
> > + */
> > + required_clk_rate =
> > + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> > +
>
> Should be we checking the rate against the max rate supported?
If the request rate is beyond max supported rate, then the clk_set_rate will be failing
and can get caught with error check followed by. Otherwise it will fail through fitting in
the register's frequency divider filed. So I think it is not required to check against max rate.
Please advise if I am not able to follow with what you are suggesting.
>
> > + err = clk_set_rate(pc->clk, required_clk_rate);
> > + if (err < 0)
> > + return -EINVAL;
> > +
> > + rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
>
> Do we need to update the pwm->clk_rate here?
This return rate is basically from the factor that requested clk_set_rate and the actual rate set
mostly will have a little deviation based on the clock divider and other factors while setting
a new rate. So capturing the actual rate for further calculation and conversion to Hz.
Whenever it is required to use pwm->clk_rate we are no longer depending upon the cached value
for num_channels == 1. So in my opinion it does not need to be cached. However it is kept
stored for the SoCs having num_channels > 1.
Please suggest if I am missing any case where we need to keep the value stored.
>
> > + } else {
> > + /*
> > + * This is the case for SoCs who support multiple channels:
> > + *
> > + * clk_set_rate() can not be called again in config because
> > + * T210 or any prior chip supports one pwm-controller and
> > + * multiple channels. Hence in this case cached clock rate
> > + * will be considered which was stored during probe.
> > + */
> > + rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> > + }
> >
> > /* Consider precision in PWM_SCALE_WIDTH rate calculation */
> > - hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> > - rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> > + period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,
> period_ns);
> > + pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
> >
> > /*
> > * Since the actual PWM divider is the register's frequency divider
> > - * field minus 1, we need to decrement to get the correct value to
> > + * field plus 1, we need to decrement to get the correct value to
> > * write to the register.
> > */
> > - if (rate > 0)
> > - rate--;
> > + if (pfm > 0)
> > + pfm--;
> >
> > /*
> > - * Make sure that the rate will fit in the register's frequency
> > + * Make sure that pfm will fit in the register's frequency
> > * divider field.
> > */
> > - if (rate >> PWM_SCALE_WIDTH)
> > + if (pfm >> PWM_SCALE_WIDTH)
> > return -EINVAL;
> >
> > - val |= rate << PWM_SCALE_SHIFT;
> > + val |= pfm << PWM_SCALE_SHIFT;
> >
> > /*
> > * If the PWM channel is disabled, make sure to turn on the clock @@
> > -205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device
> *pdev)
> > */
> > pwm->clk_rate = clk_get_rate(pwm->clk);
> >
> > + /* Set minimum limit of PWM period for the IP */
> > + pwm->min_period_ns =
> > + (NSEC_PER_SEC / (pwm->soc->max_frequency >>
> PWM_DUTY_WIDTH)) +
> > +1;
> > +
> > pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
> > if (IS_ERR(pwm->rst)) {
> > ret = PTR_ERR(pwm->rst);
> > @@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
> >
> > MODULE_LICENSE("GPL");
> > MODULE_AUTHOR("NVIDIA Corporation");
> > +MODULE_AUTHOR("Sandipan Patra <spatra@nvidia.com>");
> > MODULE_ALIAS("platform:tegra-pwm");
> >
>
> --
> nvpublic
^ permalink raw reply
* Re: [PATCH v4 06/16] spi: dw: Parameterize the DMA Rx/Tx burst length
From: Andy Shevchenko @ 2020-05-22 11:06 UTC (permalink / raw)
To: Serge Semin
Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree,
Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula, linux-spi,
linux-kernel
In-Reply-To: <20200522000806.7381-7-Sergey.Semin@baikalelectronics.ru>
On Fri, May 22, 2020 at 03:07:55AM +0300, Serge Semin wrote:
> It isn't good to have numeric literals in the code especially if there
> are multiple of them and they are related. Let's replace the Tx and Rx
> burst level literals with the corresponding constants.
You missed my tag.
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v3:
> - Discard the dws->fifo_len utilization in the Tx FIFO DMA threshold
> setting.
> ---
> drivers/spi/spi-dw-mid.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index c39bc8758339..1598c36c905f 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -20,7 +20,9 @@
>
> #define WAIT_RETRIES 5
> #define RX_BUSY 0
> +#define RX_BURST_LEVEL 16
> #define TX_BUSY 1
> +#define TX_BURST_LEVEL 16
>
> static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
> {
> @@ -198,7 +200,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
> memset(&txconf, 0, sizeof(txconf));
> txconf.direction = DMA_MEM_TO_DEV;
> txconf.dst_addr = dws->dma_addr;
> - txconf.dst_maxburst = 16;
> + txconf.dst_maxburst = TX_BURST_LEVEL;
> txconf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> txconf.dst_addr_width = convert_dma_width(dws->n_bytes);
> txconf.device_fc = false;
> @@ -273,7 +275,7 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_rx(struct dw_spi *dws,
> memset(&rxconf, 0, sizeof(rxconf));
> rxconf.direction = DMA_DEV_TO_MEM;
> rxconf.src_addr = dws->dma_addr;
> - rxconf.src_maxburst = 16;
> + rxconf.src_maxburst = RX_BURST_LEVEL;
> rxconf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> rxconf.src_addr_width = convert_dma_width(dws->n_bytes);
> rxconf.device_fc = false;
> @@ -298,8 +300,8 @@ static int mid_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
> {
> u16 imr = 0, dma_ctrl = 0;
>
> - dw_writel(dws, DW_SPI_DMARDLR, 0xf);
> - dw_writel(dws, DW_SPI_DMATDLR, 0x10);
> + dw_writel(dws, DW_SPI_DMARDLR, RX_BURST_LEVEL - 1);
> + dw_writel(dws, DW_SPI_DMATDLR, TX_BURST_LEVEL);
>
> if (xfer->tx_buf) {
> dma_ctrl |= SPI_DMA_TDMAE;
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 12/16] spi: dw: Add DW SPI DMA/PCI/MMIO dependency on the DW SPI core
From: Andy Shevchenko @ 2020-05-22 11:07 UTC (permalink / raw)
To: Serge Semin
Cc: Mark Brown, Serge Semin, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree, John Garry,
Chuanhong Guo, Joe Perches, Chris Packham, Tomer Maimon,
Masahisa Kojima, Krzysztof Kozlowski, linux-spi, linux-kernel
In-Reply-To: <20200522000806.7381-13-Sergey.Semin@baikalelectronics.ru>
On Fri, May 22, 2020 at 03:08:01AM +0300, Serge Semin wrote:
> Seeing all of the DW SPI driver components like DW SPI DMA/PCI/MMIO
> depend on the DW SPI core code it's better to use the if-endif
> conditional kernel config statement to signify that common dependency.
>
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Here and for the future, when you add somebody's tag, drop their appearance in
Cc. git-send-email automatically converts known tags to Cc.
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox