Re: [PATCH v4 2/2] Add hardware monitoring driver for Moortec MR75203 PVT controller
Hi Andy
On 3/10/2020 2:11 am, Andy Shevchenko wrote:
> On Fri, Oct 02, 2020 at 03:04:27PM +0800, Rahul Tanwar wrote:
>> PVT controller (MR75203) is used to configure & control
>> Moortec embedded analog IP which contains temprature
>> sensor(TS), voltage monitor(VM) & process detector(PD)
>> modules. Add hardware monitoring driver to support
>> MR75203 PVT controller.
> Some nit-picks below.
> Reviewed-by: Andy Shevchenko
>
>> Signed-off-by: Rahul Tanwar
>> ---
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/mr75203.c | 651
>>
>> 3 files changed, 662 insertions(+)
>> create mode 100644 drivers/hwmon/mr75203.c
[...]
>> +pvt_temp.config = temp_config;
>> +
>> +pvt_info[index++] = _temp;
>> +}
>> +
>> +if (pd_num) {
>> +ret = pvt_get_regmap(pdev, "pd", pvt);
>> +if (ret)
>> +return ret;
>> +}
>> +
>> +if (vm_num) {
>> +u32 num = vm_num;
>> +
>> +ret = pvt_get_regmap(pdev, "vm", pvt);
>> +if (ret)
>> +return ret;
>> +
>> +pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
>> + GFP_KERNEL);
>> +if (!pvt->vm_idx)
>> +return -ENOMEM;
>> +for (i = 0; i < vm_num; i++)
>> +pvt->vm_idx[i] = i;
> What the point if you are replace them below in one case?
>
>> +ret = device_property_read_u8_array(dev, "intel,vm-map",
>> +pvt->vm_idx, vm_num);
>> +if (!ret)
> Misses {} and because of above
>
> if (ret) {
> for () ...
> } else {
> for () ...
> }
>
>> +for (i = 0; i < vm_num; i++)
>> +if (pvt->vm_idx[i] >= vm_num ||
>> +pvt->vm_idx[i] == 0xff) {
>> +num = i;
>> +break;
>> +}
> Or looking in this, perhaps move the incremental for-loop here and start it
> with num which is 0.
Not able to understand what exactly you are suggesting here. Presently
it is like below:
1. Init vm_idx array with incremental values.
2. Read array from device property.
3. If success, figure out the last valid value and assign to num.
Can you please elaborate and explain more clearly? Thanks.
Regards,
Rahul
Re: [PATCH v3 2/2] Add hardware monitoring driver for Moortec MR75203 PVT controller
Hi Guenter, On 2/10/2020 2:26 am, Guenter Roeck wrote: > On 9/29/20 1:59 AM, Rahul Tanwar wrote: >> PVT controller (MR75203) is used to configure & control >> Moortec embedded analog IP which contains temprature >> sensor(TS), voltage monitor(VM) & process detector(PD) >> modules. Add hardware monitoring driver to support >> MR75203 PVT controller. >> >> Signed-off-by: Rahul Tanwar >> --- >> drivers/hwmon/Kconfig | 10 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/mr75203.c | 605 >> >> 3 files changed, 616 insertions(+) >> create mode 100644 drivers/hwmon/mr75203.c >> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 8dc28b26916e..2defb46677b4 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1112,6 +1112,16 @@ config SENSORS_MENF21BMC_HWMON >>This driver can also be built as a module. If so the module >>will be called menf21bmc_hwmon. >> >> +config SENSORS_MR75203 >> +tristate "Moortec Semiconductor MR75203 PVT Controller" >> +select REGMAP_MMIO >> +help >> + If you say yes here you get support for Moortec MR75203 >> + PVT controller. >> + >> + This driver can also be built as a module. If so, the module >> + will be called mr75203. >> + >> config SENSORS_ADCXX >> tristate "National Semiconductor ADCxxxSxxx" >> depends on SPI_MASTER >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index a8f4b35b136b..bb4bd92a5149 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MCP3021)+= mcp3021.o >> obj-$(CONFIG_SENSORS_TC654) += tc654.o >> obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o >> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o >> +obj-$(CONFIG_SENSORS_MR75203) += mr75203.o >> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o >> obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o >> obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o >> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c >> new file mode 100644 >> index ..30a70a3ae82b >> --- /dev/null >> +++ b/drivers/hwmon/mr75203.c >> @@ -0,0 +1,605 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2020 MaxLinear, Inc. >> + * >> + * This driver is a hardware monitoring driver for PVT controller >> + * (MR75203) which is used to configure & control Moortec embedded >> + * analog IP to enable multiple embedded temprature sensor(TS), > > temperature Well noted. >> + * voltage monitor(VM) & process detector(PD) modules. >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* PVT Common register */ >> +#define PVT_IP_CONFIG 0x04 >> +#define TS_NUM_MSK GENMASK(4, 0) >> +#define TS_NUM_SFT 0 >> +#define PD_NUM_MSK GENMASK(12, 8) >> +#define PD_NUM_SFT 8 >> +#define VM_NUM_MSK GENMASK(20, 16) >> +#define VM_NUM_SFT 16 >> +#define CH_NUM_MSK GENMASK(31, 24) >> +#define CH_NUM_SFT 24 >> + >> +/* Macro Common Register */ >> +#define CLK_SYNTH 0x00 >> +#define CLK_SYNTH_LO_SFT0 >> +#define CLK_SYNTH_HI_SFT8 >> +#define CLK_SYNTH_HOLD_SFT 16 >> +#define CLK_SYNTH_ENBIT(24) >> +#define CLK_SYS_CYCLES_MAX 514 >> +#define CLK_SYS_CYCLES_MIN 2 >> +#define HZ_PER_MHZ 100L >> + >> +#define SDIF_DISABLE0x04 >> + >> +#define SDIF_STAT 0x08 >> +#define SDIF_BUSY BIT(0) >> +#define SDIF_LOCK BIT(1) >> + >> +#define SDIF_W 0x0c >> +#define SDIF_PROG BIT(31) >> +#define SDIF_WRN_W BIT(27) >> +#define SDIF_WRN_R 0x00 >> +#define SDIF_ADDR_SFT 24 >> + >> +#define SDIF_HALT 0x10 >> +#define SDIF_CTRL 0x14 >> +#define SDIF_SMPL_CTRL 0x20 >> + >> +/* TS & PD Individual Macro Register */ >> +#define COM_REG_SIZE0x40 >> + >> +#define SDIF_DONE(n)(COM_REG_SIZE + 0x14 + 0x40 * (n)) >> +#define SDIF_SMPL_DONE BIT(0) >> + >> +#define SDIF_DATA(n)(COM_REG_SIZE + 0x18 + 0x40 * (n)) >> +#define SAMPLE_DATA_MSK GENMASK(15, 0) >> + >> +#define HILO_RESET(n) (COM_REG_SIZE + 0x2c + 0x40 * (n)) >> + >> +/* VM Individual Macro Register */ >> +#define VM_COM_REG_SIZE 0x200 >> +#define VM_SDIF_DONE(n) (VM_COM_REG_SIZE + 0x34 + 0x200 * (n)) >> +#define VM_SDIF_DATA(n) (VM_COM_REG_SIZE + 0x40 + 0x200 * (n)) >> + >> +/* SDA Slave Register */ >> +#define IP_CTRL 0x00 >> +#define IP_RST_REL BIT(1) >> +#define IP_RUN_CONT BIT(3) >> +#define IP_AUTO BIT(8) >> +#define IP_VM_MODE BIT(10) >> + >> +#define IP_CFG 0x01 >> +#define CFG0_MODE_2 BIT(0) >> +#define CFG0_PARALLEL_OUT 0 >> +#define CFG0_12_BIT 0 >> +#define CFG1_VOL_MEAS_MODE 0 >> +#define CFG1_PARALLEL_OUT 0 >> +#define CFG1_14_BIT 0 >> + >> +#define IP_DATA
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
Thanks for review & feedback.
On 24/9/2020 2:55 pm, Uwe Kleine-König wrote:
> Hello,
>
> (hhm Thierry already announced to have taken this patch, so my review is
> late.)
>
> On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar
>> Reviewed-by: Andy Shevchenko
>> ---
>> drivers/pwm/Kconfig | 11 ++
>> drivers/pwm/Makefile| 1 +
>> drivers/pwm/pwm-intel-lgm.c | 246
>>
>> 3 files changed, 258 insertions(+)
>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 7dbcf6973d33..4949c51fe90b 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -232,6 +232,17 @@ 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 HAS_IOMEM
>> +depends on (OF && X86) || COMPILE_TEST
>> +select REGMAP_MMIO
>> +help
>> + Generic PWM fan controller driver for LGM SoC.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-intel-lgm.
>> +
>> config PWM_IQS620A
>> tristate "Azoteq IQS620A PWM support"
>> depends on MFD_IQS62X || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 2c2ba0a03557..e9431b151694 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_IQS620A) += pwm-iqs620a.o
>> obj-$(CONFIG_PWM_JZ4740)+= pwm-jz4740.o
>> obj-$(CONFIG_PWM_LP3943)+= pwm-lp3943.o
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index ..ea3df75a5971
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,246 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Intel Corporation.
>> + *
>> + * Limitations:
>> + * - The hardware supports fixed period which is dependent on 2/3 or 4
>> + * wire fan mode.
> The driver now hardcodes 2-wire mode. IMHO that is worth mentioning.
Will update.
>> +static void lgm_clk_disable(void *data)
>> +{
>> +struct lgm_pwm_chip *pc = data;
>> +
>> +clk_disable_unprepare(pc->clk);
>> +}
>> +
>> +static int lgm_clk_enable(struct device *dev, struct lgm_pwm_chip *pc)
>> +{
>> +int ret;
>> +
>> +ret = clk_prepare_enable(pc->clk);
>> +if (ret)
>> +return ret;
>> +
>> +return devm_add_action_or_reset(dev, lgm_clk_disable, pc);
>> +}
> My first reflex here was to point out that lgm_clk_disable() isn't the
> counter part to lgm_clk_enable() and so lgm_clk_disable() needs
> adaption. On a second look this is correct and so I think the function
> names are wrong. The usual naming would be to use _release instead of
> _disable. Having said that the enable function could be named
> devm_clk_enable and live in drivers/clk/clk-devres.c. (Or
> devm_clk_get_enabled()?)
Will change function name from _disable to _release. But i think addition
of a generic devm_clk_enable in drivers/clk/clk-devres.c can be taken as
a separate task in a future patch (not clubbed with this driver)..
>> +static void lgm_reset_control_assert(void *data)
>> +{
>> +struct lgm_pwm_chip *pc = data;
>> +
>> +reset_control_assert(pc->rst);
>> +}
>> +
>> +static int lgm_reset_control_deassert(struct device *dev, struct
>> lgm_pwm_chip *pc)
>> +{
>> +int ret;
>> +
>> +ret = reset_control_deassert(pc->rst);
>> +if (ret)
>> +return ret;
>> +
>> +return devm_add_action_or_reset(dev, lgm_reset_control_assert, pc);
>> +}
> A similar comment applies here.
>
>> +static int lgm_pwm_probe(struct platform_device *pdev)
>> +{
>> +struct device *dev = >dev;
>> +struct lgm_pwm_chip *pc;
>> +void __iomem *io_base;
>> +int ret;
>> +
>> +pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>> +if (!pc)
>> +return -ENOMEM;
>> +
>> +platform_set_drvdata(pdev, pc);
>> +
>> +io_base = devm_platform_ioremap_resource(pdev, 0);
>> +if (IS_ERR(io_base))
>> +return PTR_ERR(io_base);
>> +
>> +pc->regmap = devm_regmap_init_mmio(dev, io_base,
>> _pwm_regmap_config);
>> +if (IS_ERR(pc->regmap))
>> +return dev_err_probe(dev, PTR_ERR(pc->regmap),
>> + "failed to init register map\n");
>> +
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
On 24/9/2020 3:12 pm, Thierry Reding wrote: > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: >> Hello, >> >> (hhm Thierry already announced to have taken this patch, so my review is >> late.) > There's also a build warning in linux-next caused by this patch, so I'm > going to back it out. > > Rahul, please address Uwe's comments and make sure to fix the build > warning as well. Well noted, will do. Regards, Rahul > Thierry
Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller
On 10/9/2020 6:35 pm, Philipp Zabel wrote:
> On Wed, 2020-09-09 at 14:52 +0800, Rahul Tanwar wrote:
>> PVT controller (MR75203) is used to configure & control
>> Moortec embedded analog IP which contains temprature
>> sensor(TS), voltage monitor(VM) & process detector(PD)
>> modules. Add driver to support MR75203 PVT controller.
>>
>> Signed-off-by: Rahul Tanwar
>> ---
> [...]
>> +static int mr75203_probe(struct platform_device *pdev)
>> +{
>> +const struct hwmon_channel_info **pvt_info;
>> +u32 ts_num, vm_num, pd_num, val, index, i;
>> +struct device *dev = >dev;
>> +u32 *temp_config, *in_config;
>> +struct device *hwmon_dev;
>> +struct pvt_device *pvt;
>> +int ret;
>> +
>> +pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
>> +if (!pvt)
>> +return -ENOMEM;
>> +
>> +ret = pvt_get_regmap(pdev, "common");
>> +if (ret)
>> +return ret;
>> +
>> +pvt->rst = devm_reset_control_get(dev, NULL);
> Please use devm_reset_control_get_exclusive().
Well noted. Missed that, thanks.
Regards,
Rahul
Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller
On 10/9/2020 5:46 pm, Andy Shevchenko wrote:
> On Thu, Sep 10, 2020 at 03:27:11PM +0800, Tanwar, Rahul wrote:
>> On 9/9/2020 11:05 pm, Guenter Roeck wrote:
>>> On 9/8/20 11:52 PM, Rahul Tanwar wrote:
> ...
>
>>>> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
>>>> +{
>>>> + struct device *dev = >dev;
>>>> + struct pvt_device *pvt = platform_get_drvdata(pdev);
>>> I am quite at loss how this is supposed to work. Platform driver
>>> data is not initialized with a pointer to struct pvt_device at this point.
>>> How does this code not crash ? What am I missing ?
>> Big mistake on my part. Last minute change based on internal review feedback
>> about moving platform_set_drvdata() at the end of probe. I will fix it in v2.
>> Thanks.
> Since IIRC it was me who suggested this I should say that reviewer can make
> mistakes, on the other hand contributor should have had known their code to
> refuse certain changes.
>
Totally agree. Overlooked in a hasty change..
Regards,
Rahul
Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller
Hi Andy,
Thanks for the review & feedback.
On 9/9/2020 6:33 pm, Andy Shevchenko wrote:
> On Wed, Sep 09, 2020 at 02:52:05PM +0800, Rahul Tanwar wrote:
>> PVT controller (MR75203) is used to configure & control
>> Moortec embedded analog IP which contains temprature
>> sensor(TS), voltage monitor(VM) & process detector(PD)
>> modules. Add driver to support MR75203 PVT controller.
> ...
>
>> +#include
>> +#include
>> +#include
>> +#include
>
>> +#include
> I don't see anything special about OF here.
> Perhaps
> mod_devicetable.h
> property.h
> ?
of.h is needed because of of_property_read_u8_array(). I will add
mod_devicetable.h.
property.h seems not required at all.
>> +#include
>> +#include
>> +#include
> ...
>
>> +#define PVT_POLL_TIMEOUT2
> Units?
Well noted.
> ...
>
>> +sys_freq = clk_get_rate(pvt->clk) / 100;
> HZ_PER_MHZ ?
Well noted.
>> +while (high >= low) {
>> +middle = DIV_ROUND_CLOSEST(low + high, 2);
> I'm wondering would it be better in the code like
>
> middle = (low + high + 1) / 2;
Will update.
>> +key = DIV_ROUND_CLOSEST(sys_freq, middle);
>> +if (key > 514) {
>> +low = middle + 1;
>> +continue;
>> +} else if (key < 2) {
>> +high = middle - 1;
>> +continue;
>> +}
>> +
>> +break;
>> +}
>> +
>> +key = clamp_val(key, 2, 514) - 2;
> I guess above deserves a comment with formulas.
Hmm..I will try to add some more info. Problem is that the datasheet doesn't
explain it clearly.
> ...
>
>> +regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
> For non-constants better would be BIT(p_num) - 1.
Well noted.
> ...
>
>> +regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
>> +regmap_write(v_map, SDIF_HALT, 0x0);
>> +regmap_write(v_map, SDIF_DISABLE, 0);
> In some you have 0, in some 0x0 over the file, can it be consistent?
Yes, missed that, will update.
> ...
>
>> +static struct regmap_config pvt_regmap_config = {
>> +.reg_bits = 32,
>> +.reg_stride = 4,
>> +.val_bits = 32,
> How do you use regmap's lock?
We mutex lock whenever read temperature or voltage values from the registers.
All non-probe/non-init paths. We do not override regmap's internal lock.
>> +};
> ...
>
>> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
>> +{
>> +struct device *dev = >dev;
>> +struct pvt_device *pvt = platform_get_drvdata(pdev);
> Can it be first line in definition block?
Well noted.
>> +struct regmap **reg_map;
>> +void __iomem *io_base;
>> +struct resource *res;
>> +
>> +if (!strcmp(reg_name, "common"))
>> +reg_map = >c_map;
>> +else if (!strcmp(reg_name, "ts"))
>> +reg_map = >t_map;
>> +else if (!strcmp(reg_name, "pd"))
>> +reg_map = >p_map;
>> +else if (!strcmp(reg_name, "vm"))
>> +reg_map = >v_map;
>> +else
>> +return -EINVAL;
>> +
>> +res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
>> +io_base = devm_ioremap_resource(dev, res);
> io_base = devm_platform_ioremap_resource_by_name(pdev, reg_name);
>
> ?
Well noted.
>> +if (IS_ERR(io_base))
>> +return PTR_ERR(io_base);
>> +
>> +pvt_regmap_config.name = reg_name;
> Hmm... regmap API keeps it in devres. Why is there a copy?
Just populating the name in regmap config because of multiple register
regions..
>> +*reg_map = devm_regmap_init_mmio(dev, io_base, _regmap_config);
>> +if (IS_ERR(*reg_map)) {
>> +dev_err(dev, "failed to init register map\n");
>> +return PTR_ERR(*reg_map);
>> +}
>> +
>> +return 0;
>> +}
> ...
>
>> +for (i = 0; i < num; i++)
>> +in_config[i] = HWMON_I_INPUT;
> memset32() ?
>
Well noted. Thanks.
Regards,
Rahul
Re: [PATCH v10 2/2] Add PWM fan controller driver for LGM SoC
Hi Andy,
On 24/8/2020 4:17 pm, Andy Shevchenko wrote:
> On Mon, Aug 24, 2020 at 11:36:37AM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
> ...
>
>> +pc->regmap = devm_regmap_init_mmio(dev, io_base,
>> _pwm_regmap_config);
>> +if (IS_ERR(pc->regmap)) {
>> +ret = PTR_ERR(pc->regmap);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err_probe(dev, ret, "failed to init register
>> map\n");
>> +return ret;
>> +}
>> +
>> +pc->clk = devm_clk_get(dev, NULL);
>> +if (IS_ERR(pc->clk)) {
>> +ret = PTR_ERR(pc->clk);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err_probe(dev, ret, "failed to get clock\n");
>> +return ret;
>> +}
>> +
>> +pc->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +if (IS_ERR(pc->rst)) {
>> +ret = PTR_ERR(pc->rst);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err_probe(dev, ret, "failed to get reset
>> control\n");
>> +return ret;
>> +}
>> +
>> +ret = reset_control_deassert(pc->rst);
>> +if (ret) {
>> +if (ret != -EPROBE_DEFER)
>> +dev_err_probe(dev, ret, "cannot deassert reset
>> control\n");
>> +return ret;
>> +}
> Please, spend a bit of time to understand the changes you are doing. There are
> already few examples how to use dev_err_probe() properly.
I guess your point is that the check of (ret !- -EPROBE_DEFER) is not needed
when using dev_err_probe() as it encapsulates it. Sorry, i missed it. Will
fix it. I am not able to find any other missing point after referring to
two driver examples which uses dev_err_probe() ?
>> +ret = clk_prepare_enable(pc->clk);
>> +if (ret) {
>> +dev_err(dev, "failed to enable clock\n");
>> +return ret;
>> +}
>> +
>> +ret = devm_add_action_or_reset(dev, lgm_pwm_action, pc);
>> +if (ret)
>> +return ret;
> You have also ordering issues here.
>
> So, what I can see about implementation is that
>
>
> static void ..._clk_disable(void *data)
> {
> clk_disable_unprepare(data);
> }
>
> static int ..._clk_enable(...)
> {
> int ret;
>
> ret = clk_preare_enable(...);
> if (ret)
> return ret;
> return devm_add_action_or_reset(..., ..._clk_disable);
> }
>
>
> Similar for reset control.
>
> Then in the ->probe() something like this:
>
> ret = devm_reset_control_get...;
> if (ret)
> return ret;
>
> ret = ..._reset_deassert(...);
> if (ret)
> return ret;
>
> followed by similar section for the clock.
>
Regarding ordering: In early rounds of review, feedback about ordering was that
it is recommended to be reverse of the sequence in probe i.e.
if in probe:
1. reset_control_deassert()
2. clk_prepare_enable()
then in remove:
1. clk_disable_uprepare()
2. reset_control_assert()
That's the reason i added a generic action() which reverses order.
I understand your suggested way as explained above but not sure if that would
ensure reverse ordering during unwind.
Thanks.
Regards,
Rahul
Re: [PATCH v9 2/2] Add PWM fan controller driver for LGM SoC
Hi Andy, On 21/8/2020 6:56 pm, Andy Shevchenko wrote: > On Fri, Aug 21, 2020 at 05:32:11PM +0800, Rahul Tanwar wrote: >> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller. >> This PWM controller does not have any other consumer, it is a >> dedicated PWM controller for fan attached to the system. Add >> driver for this PWM fan controller. > ... > >> +#include >> +#include >> +#include >> +#include > We haven't settle this yet... I investigated more about it. I was getting build error because we were relying on of_device.h for including platform_device.h. You are right that we are not using anything from of_device.h. So i removed of_device.h from driver and added include & build is ok. Regarding mod_devicetable.h header, it gets included indirectly from which includes of.h which includes mod_devicetable.h. So i think no point including it again in the driver. Regards, Rahul >> +#include >> +#include >> +#include
Re: [PATCH v8 2/2] Add PWM fan controller driver for LGM SoC
On 20/8/2020 6:52 pm, Andy Shevchenko wrote: > On Thu, Aug 20, 2020 at 12:50:46PM +0800, Rahul Tanwar wrote: >> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller. >> This PWM controller does not have any other consumer, it is a >> dedicated PWM controller for fan attached to the system. Add >> driver for this PWM fan controller. > ... > +#include > +#include > +#include >> +#include > This should be mod_devicetable.h. Inlcuding mod_devicetable.h instead of of_device.h results in build failure related to all platform calls. Build is ok if i remove it. Just FYI.. Regards, Rahul
Re: [PATCH v8 2/2] Add PWM fan controller driver for LGM SoC
Hi Andy,
On 20/8/2020 6:52 pm, Andy Shevchenko wrote:
> On Thu, Aug 20, 2020 at 12:50:46PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
> ...
>
>> +config PWM_INTEL_LGM
>> +tristate "Intel LGM PWM support"
>> +depends on OF && HAS_IOMEM
>> +depends on X86 || COMPILE_TEST
> For better test coverage you may rewrite this
>
> depends on HAS_IOMEM
> depends on (OF && X86) || COMPILE_TEST
Sure, will update.
>> +select REGMAP_MMIO
>> +help
>> + Generic PWM fan controller driver for LGM SoC.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-intel-lgm.
> ...
>
>> +#include
>> +#include
>> +#include
>> +#include
> This should be mod_devicetable.h.
Well noted.
>> +#include
>> +#include
>> +#include
> ...
>
>> +#define LGM_PWM_PERIOD_2WIRE_NSECS 4000
> NSECS -> NS
> 4000 -> (40 * NSEC_PER_MSEC)
Well noted.
> ...
>
>> +if (state->polarity != PWM_POLARITY_NORMAL ||
>> +state->period < pc->period)
> It can be one line.
Ok.
>> +return -EINVAL;
> ...
>
>> +if (!state->enabled) {
>> +ret = lgm_pwm_enable(chip, 0);
>> +return ret;
> What is the point?
I guess you mean to change it to return lgm_pwm_enable(chip, 0);
Will do, thanks.
>> +}
> ...
>
>> +ret = lgm_pwm_enable(chip, 1);
>> +
>> +return ret;
> Ditto.
>
> ...
>
>> +state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
>> + LGM_PWM_MAX_DUTY_CYCLE);
> One line?
Ok.
> ...
>
>> +struct lgm_pwm_chip *pc;
>> +struct device *dev = >dev;
> Use reversed xmas tree order.
Sure, will update.
>> +void __iomem *io_base;
>> +int ret;
> ...
>
>> +pc->regmap = devm_regmap_init_mmio(dev, io_base,
>> _pwm_regmap_config);
>> +if (IS_ERR(pc->regmap)) {
>> +ret = PTR_ERR(pc->regmap);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err(dev, "failed to init register map: %pe\n",
>> +pc->regmap);
>> +return ret;
> dev_err_probe()
Will update. Thanks.
>> +}
> ...
>
>> +pc->clk = devm_clk_get(dev, NULL);
>> +if (IS_ERR(pc->clk)) {
>> +ret = PTR_ERR(pc->clk);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err(dev, "failed to get clock: %pe\n", pc->clk);
>> +return ret;
> Ditto.
>
>> +}
>> +
>> +pc->rst = devm_reset_control_get_exclusive(dev, NULL);
>> +if (IS_ERR(pc->rst)) {
>> +ret = PTR_ERR(pc->rst);
>> +if (ret != -EPROBE_DEFER)
>> +dev_err(dev, "failed to get reset control: %pe\n",
>> +pc->rst);
>> +return ret;
> Ditto.
>
>> +}
>> +
>> +ret = reset_control_deassert(pc->rst);
>> +if (ret) {
>> +if (ret != -EPROBE_DEFER)
>> +dev_err(dev, "cannot deassert reset control: %pe\n",
>> +ERR_PTR(ret));
>> +return ret;
> Ditto.
>
>> +}
> ...
>
>> +ret = clk_prepare_enable(pc->clk);
> Wrap it with devm_add_action_or_reset(). Same for reset_control_deassert().
> You probably can even put them under one function.
I did some study and research for using devm_add_action_or_reset(). But
still i have some doubts. Below steps is what i intend to do in order to
switch to using this API. Please do review and let me know it is ok and
i am not missing anything else. Thanks.
1. Call reset_control_assert()
2. Call clk_prepare_enable()
3. Call pwmchip_add()
4. Call devm_add_action_or_reset(dev, my_action, pc)
5. Remove explicit calls to unprepare/reset_control_assert from probe in
failure cases.
6. static void my_action(void *pc)
{
pwmchip_remove(>chip);
clk_disable_upprepare(pc->clk);
reset_control_assert(pc->rst);
}
7. Remove platform_driver.remove() entirely.
>> +if (ret) {
>> +dev_err(dev, "failed to enable clock\n");
>> +reset_control_assert(pc->rst);
>> +return ret;
>> +}
> ...
>
>> +ret = pwmchip_add(>chip);
>> +if (ret < 0) {
> Does ' < 0' have any meaning?
I use < 0 because this API's return code is mentioned as below:
Returns: 0 on success or a negative error code on failure.
Also, all other PWM drivers check for <0 for this call.
>> +dev_err(dev, "failed to add PWM chip: %pe\n", ERR_PTR(ret));
>> +clk_disable_unprepare(pc->clk);
>> +reset_control_assert(pc->rst);
>> +return ret;
>> +}
> ...
>
>> +ret = pwmchip_remove(>chip);
>> +if (ret < 0)
> Ditto.
Same as above.
>> +return ret;
Thanks.
Regards,
Rahul
Re: [PATCH v7 0/2] pwm: intel: Add PWM driver for a new SoC
Hi Andy, On 19/8/2020 3:54 pm, Andy Shevchenko wrote: > On Wed, Aug 19, 2020 at 7:18 AM Tanwar, Rahul > wrote: >> >> Hi Andy, >> >> On 18/8/2020 4:38 pm, Andy Shevchenko wrote: >>> On Tue, Aug 18, 2020 at 01:48:59PM +0800, Rahul Tanwar wrote: >>>> Patch 1 adds dt binding document in YAML format. >>>> Patch 2 add PWM fan controller driver for LGM SoC. >>>> >>>> v7: >>>> - Address code quality related review concerns. >>>> - Rename fan related property to pwm-*. >>>> - Fix one make dt_binding_check reported error. >>> I guess it misses the answer why pwm-fan can't be integrated into the soup? >>> >> Can you please elaborate more? I could not understand your point clearly. > It's not mine, it's Uwe's. There is an hwmon module called pwm-fan. As > far as *I* understand this, it can be utilized to control fans via PWM > APIs. And Uwe asked you if you considered that and why you don't > integrated (coupled) it here. Thanks for clarification. I now understand what Rob, Uwe & you mean by pwm-fan. I will check in detail about it if we can integrate it with pwm-fan hwmon driver. Regards, Rahul
Re: [PATCH v7 0/2] pwm: intel: Add PWM driver for a new SoC
Hi Andy, On 18/8/2020 4:38 pm, Andy Shevchenko wrote: > On Tue, Aug 18, 2020 at 01:48:59PM +0800, Rahul Tanwar wrote: >> Patch 1 adds dt binding document in YAML format. >> Patch 2 add PWM fan controller driver for LGM SoC. >> >> v7: >> - Address code quality related review concerns. >> - Rename fan related property to pwm-*. >> - Fix one make dt_binding_check reported error. > I guess it misses the answer why pwm-fan can't be integrated into the soup? > Can you please elaborate more? I could not understand your point clearly. Regards, Rahul
Re: [PATCH v6 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC
Hi Rob, On 1/8/2020 2:19 am, Rob Herring wrote: > On Tue, Jul 28, 2020 at 04:52:12PM +0800, Rahul Tanwar wrote: >> Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller >> which is only used to control the fan attached to the system. This >> PWM controller does not have any other consumer other than fan. >> Add DT bindings documentation for this PWM fan controller. >> >> Signed-off-by: Rahul Tanwar >> --- >> .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 54 >> ++ >> 1 file changed, 54 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> >> diff --git a/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> new file mode 100644 >> index ..9879972470dc >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> @@ -0,0 +1,54 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pwm/intel,lgm-pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: LGM SoC PWM fan controller >> + >> +maintainers: >> + - Rahul Tanwar >> + >> +properties: >> + compatible: >> +const: intel,lgm-pwm >> + >> + reg: >> +maxItems: 1 >> + >> + "#pwm-cells": >> +const: 2 >> + >> + clocks: >> +maxItems: 1 >> + >> + resets: >> +maxItems: 1 >> + >> + intel,fan-wire: >> +$ref: '/schemas/types.yaml#/definitions/uint32' >> +description: Specifies fan mode. Default when unspecified is 2. >> + >> + intel,max-rpm: >> +$ref: '/schemas/types.yaml#/definitions/uint32' >> +description: >> + Specifies maximum RPM of fan attached to the system. >> + Default when unspecified is 4000. > Again, these are properties of a fan, not the pwm controller. They > belong in a fan node. Our PWM controller is actually a PWM fan controller dedicated for controlling fan. I am looking for some suggestions from you on how to handle fan related optional properties in such a scenario. Should i create a separate child node for fan with PWM node being the parent? Is that what you are suggesting? Thanks. Regards, Rahul
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
On 27/7/2020 3:01 pm, Uwe Kleine-König wrote:
> On Mon, Jul 27, 2020 at 02:04:56PM +0800, Tanwar, Rahul wrote:
>> Hi Uwe,
>>
>> On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
>>>> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> + const struct pwm_state *state)
>>>> +{
>>>> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>>>> + u32 duty_cycle, val;
>>>> + int ret;
>>>> +
>>>> + if (!state->enabled) {
>>>> + ret = lgm_pwm_enable(chip, 0);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /*
>>>> + * HW only supports NORMAL polarity
>>>> + * HW supports fixed period which can not be changed/configured by user
>>>> + */
>>>> + if (state->polarity != PWM_POLARITY_NORMAL ||
>>>> + state->period != pc->period)
>>>> + return -EINVAL;
>>> At least for state->polarity you have to check before state->enabled, as
>>> the expectation on
>>>
>>> .enabled = false
>>> .polarity = PWM_POLARITY_INVERSED
>>>
>>> is that the output becomes constant high. Also as confirmed at the end
>>> of v4, state->period < pc->period was the right check to do.
>> For below case:
>>
>> .enabled = false
>> .polarity = PWM_POLARITY_INVERSED
>>
>> Since our HW does not support inversed polarity, the output for above case
>> is expected to be constant low. And if we disable PWM before checking for
>> polarity, the output becomes constant low. The code just does that. Sorry,
>> i could not understand what is wrong with the code. It looks correct to me.
> As your hardware can only support normal polarity, the code must have:
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> return -EINVAL;
>
> if (!state->enabled) {
> ret = lgm_pwm_enable(chip, 0);
> return ret;
> }
>
> That's what I meant writing: "At least for state->polarity you have to
> check before state->enabled".
Ok, i understand your point now.
>> Given the fact that we support fixed period, if we allow
>> state->period < pc->period case then the duty cycle will be evaluated as
>> higher than the requested one because the state->period is lesser than
>> the actual fixed period supported by the HW. Can you please elaborate
>> on why you think we should allow state->period < pc->period case?
> You should not allow it. In v4 you had:
>
> if (state->polarity != PWM_POLARITY_NORMAL ||
> state->period < pc->period)
> return -EINVAL;
>
> That's the right thing to do (even though I was unsettled at one point
> and wrote it was wrong). The check in v5 with state->period !=
> pc->period is wrong.
>
Does that mean we should allow state->period >= pc->period cases?
If the state->period is greater than HW supported pc->period and
if we allow it then the duty cycle will again be evaluated to be
incorrect/higher than requested duty cycle. Am i missing something
else? Thanks.
Regards,
Rahul
Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:
>> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> +struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +u32 duty_cycle, val;
>> +int ret;
>> +
>> +if (!state->enabled) {
>> +ret = lgm_pwm_enable(chip, 0);
>> +return ret;
>> +}
>> +
>> +/*
>> + * HW only supports NORMAL polarity
>> + * HW supports fixed period which can not be changed/configured by user
>> + */
>> +if (state->polarity != PWM_POLARITY_NORMAL ||
>> +state->period != pc->period)
>> +return -EINVAL;
> At least for state->polarity you have to check before state->enabled, as
> the expectation on
>
> .enabled = false
> .polarity = PWM_POLARITY_INVERSED
>
> is that the output becomes constant high. Also as confirmed at the end
> of v4, state->period < pc->period was the right check to do.
For below case:
.enabled = false
.polarity = PWM_POLARITY_INVERSED
Since our HW does not support inversed polarity, the output for above case
is expected to be constant low. And if we disable PWM before checking for
polarity, the output becomes constant low. The code just does that. Sorry,
i could not understand what is wrong with the code. It looks correct to me.
Given the fact that we support fixed period, if we allow
state->period < pc->period case then the duty cycle will be evaluated as
higher than the requested one because the state->period is lesser than
the actual fixed period supported by the HW. Can you please elaborate
on why you think we should allow state->period < pc->period case?
Thanks,
Regards,
Rahul
Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
Thanks for the feedback.
On 14/7/2020 3:10 am, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar
>> ---
>> drivers/pwm/Kconfig | 11 ++
>> drivers/pwm/Makefile| 1 +
>> drivers/pwm/pwm-intel-lgm.c | 266
>>
>> 3 files changed, 278 insertions(+)
>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
[...]
>> +
>> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> +struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +u32 duty_cycle, val;
>> +unsigned int period;
>> +
>> +if (!state->enabled) {
>> +lgm_pwm_enable(chip, 0);
>> +return 0;
>> +}
>> +
>> +period = min_t(u64, state->period, pc->period);
>> +
>> +if (state->polarity != PWM_POLARITY_NORMAL ||
>> +period < pc->period)
>> +return -EINVAL;
> This check looks wrong. If you refuse period < pc->period there isn't
> much configuration possible.
I am kind of stuck here. I made this change of adding a check
period < pc->period based on your feedback on v2 patch.
In fact, you had specified this code in v2 review feedback
and i used the same exact code.
How should we handle it when the hardware supports fixed period.
We don't want user to change period and allow just changing
duty_cycle. With that intention, i had first added a strict check
which refused configuration if period != pc->period. Period is
intended to be a read only value.
How do you suggest we handle the fixed period hardware support?
Would you have any reference example of other drivers which also
supports fixed period? Thanks.
[...]
>> +static int lgm_pwm_remove(struct platform_device *pdev)
>> +{
>> +struct lgm_pwm_chip *pc = platform_get_drvdata(pdev);
>> +int ret;
>> +
>> +ret = pwmchip_remove(>chip);
>> +if (ret < 0)
>> +return ret;
>> +
>> +clk_disable_unprepare(pc->clk);
>> +reset_control_assert(pc->rst);
> Please swap the two previous lines to match the error patch of .probe.
Again, i had made this change based on your below review feedback
for v1. IMO, reverse of probe makes more sense.
"In .probe() you first release reset and then enable the clock. It's good
style to do it the other way round in .remove()."
Regards,
Rahul
Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
On 14/7/2020 3:10 am, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar
>> ---
>> drivers/pwm/Kconfig | 11 ++
>> drivers/pwm/Makefile| 1 +
>> drivers/pwm/pwm-intel-lgm.c | 266
>>
>> 3 files changed, 278 insertions(+)
>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cb8d739067d2..3486edab09c4 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -232,6 +232,17 @@ 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 OF && HAS_IOMEM
>> +depends on X86 || COMPILE_TEST
>> +select REGMAP_MMIO
>> +help
>> + Generic PWM fan controller driver for LGM SoC.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-intel-lgm.
>> +
>> config PWM_IQS620A
>> tristate "Azoteq IQS620A PWM support"
>> depends on MFD_IQS62X || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index a59c710e98c7..db154a6b4f51 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_IQS620A) += pwm-iqs620a.o
>> obj-$(CONFIG_PWM_JZ4740)+= pwm-jz4740.o
>> obj-$(CONFIG_PWM_LP3943)+= pwm-lp3943.o
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index ..fddfedd56fc3
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,266 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Intel Corporation.
>> + *
>> + * Notes & Limitations:
> I'd like to have this "Limitations:" only to make it easily greppable.
>
>> + * - The hardware supports fixed period which is dependent on 2/3 or 4
>> + * wire fan mode.
>> + * - Supports normal polarity. Does not support changing polarity.
>> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
>> + * keep track of running period.
>> + * - When duty cycle is changed, PWM output may be a mix of previous setting
>> + * and new setting for the first period. From second period, the output is
>> + * based on new setting.
>> + * - It is a dedicated PWM fan controller. There are no other consumers for
>> + * this PWM controller.
>> + */
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define LGM_PWM_FAN_CON00x0
>> +#define LGM_PWM_FAN_EN_EN BIT(0)
>> +#define LGM_PWM_FAN_EN_DIS 0x0
>> +#define LGM_PWM_FAN_EN_MSK BIT(0)
>> +#define LGM_PWM_FAN_MODE_2WIRE 0x0
>> +#define LGM_PWM_FAN_MODE_4WIRE 0x1
>> +#define LGM_PWM_FAN_MODE_MSKBIT(1)
>> +#define LGM_PWM_FAN_DC_MSK GENMASK(23, 16)
>> +
>> +#define LGM_PWM_FAN_CON10x4
>> +#define LGM_PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
>> +
>> +#define LGM_PWM_MAX_RPM (BIT(16) - 1)
>> +#define LGM_PWM_DEFAULT_RPM 4000
>> +#define LGM_PWM_MAX_DUTY_CYCLE (BIT(8) - 1)
>> +
>> +#define LGM_PWM_DC_BITS 8
>> +
>> +#define LGM_PWM_PERIOD_2WIRE_NSECS 4000
>> +#define LGM_PWM_PERIOD_4WIRE_NSECS 4
>> +
>> +struct lgm_pwm_chip {
>> +struct pwm_chip chip;
>> +struct regmap *regmap;
>> +struct clk *clk;
>> +struct reset_control *rst;
>> +u32 period;
>> +};
>> +
>> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
>> +{
>> +return container_of(chip, struct lgm_pwm_chip, chip);
>> +}
>> +
>> +static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
>> +{
>> +struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +struct regmap *regmap = pc->regmap;
>> +
>> +regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK,
>> + enable ? LGM_PWM_FAN_EN_EN : LGM_PWM_FAN_EN_DIS);
> regmap_update_bits has a return value. I think it is supposed to be
> checked.
>
>> +
>> +return 0;
>> +}
>> +
>> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> +struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +u32
Re: [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC
Hi Rob, On 14/7/2020 12:46 am, Rob Herring wrote: > On Tue, Jun 30, 2020 at 03:55:31PM +0800, Rahul Tanwar wrote: >> Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller >> which is only used to control the fan attached to the system. This >> PWM controller does not have any other consumer other than fan. >> Add DT bindings documentation for this PWM fan controller. >> >> Signed-off-by: Rahul Tanwar >> --- >> .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 50 >> ++ >> 1 file changed, 50 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> >> diff --git a/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> new file mode 100644 >> index ..120bf3d85a24 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> @@ -0,0 +1,50 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pwm/intel,lgm-pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: LGM SoC PWM fan controller >> + >> +maintainers: >> + - Rahul Tanwar >> + >> +properties: >> + compatible: >> +const: intel,lgm-pwm >> + >> + reg: >> +maxItems: 1 >> + >> + clocks: >> +maxItems: 1 >> + >> + resets: >> +maxItems: 1 >> + >> + intel,fan-wire: >> +$ref: '/schemas/types.yaml#/definitions/uint32' >> +description: Specifies fan mode. Default when unspecified is 2. >> + >> + intel,max-rpm: >> +$ref: '/schemas/types.yaml#/definitions/uint32' >> +description: >> + Specifies maximum RPM of fan attached to the system. >> + Default when unspecified is 4000. > These are properties of the fan, not the PWM. And probably if you > need these properties then others would too, so they should be > common. Look at the pwm-fan.txt binding. I checked pwm-fan.txt. I don't find any common property which matches our fan properties of fan wire mode & max-rpm. Are you suggesting to add these properties additionally in pwm-fan.txt as new common properties and then use newly added generic name in our driver? Also, we have a dedicated PWM fan controller. So do you think this driver belongs to drivers/hwmon instead of drivers/pwm? Thanks. Regards, Rahul
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
On 25/6/2020 1:58 pm, Uwe Kleine-König wrote:
> On Thu, Jun 25, 2020 at 12:23:54PM +0800, Tanwar, Rahul wrote:
>> Hi Philipp,
>>
>> On 18/6/2020 8:25 pm, Philipp Zabel wrote:
>>> Hi Rahul,
>>>
>>> On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
>>>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>>>> This PWM controller does not have any other consumer, it is a
>>>> dedicated PWM controller for fan attached to the system. Add
>>>> driver for this PWM fan controller.
>>>>
>>>> Signed-off-by: Rahul Tanwar
>>>> ---
>>>> drivers/pwm/Kconfig | 9 +
>>>> drivers/pwm/Makefile| 1 +
>>>> drivers/pwm/pwm-intel-lgm.c | 400
>>>>
>>>> 3 files changed, 410 insertions(+)
>>>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>>>
>>> [...]
>>>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>>>> new file mode 100644
>>>> index ..3c7077acb161
>>>> --- /dev/null
>>>> +++ b/drivers/pwm/pwm-intel-lgm.c
>>>> @@ -0,0 +1,400 @@
>>> [...]
>>>> +static int lgm_pwm_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct lgm_pwm_chip *pc;
>>>> + struct device *dev = >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))
>>>> + return PTR_ERR(io_base);
>>>> +
>>>> + pc->regmap = devm_regmap_init_mmio(dev, io_base, _regmap_config);
>>>> + if (IS_ERR(pc->regmap)) {
>>>> + ret = PTR_ERR(pc->regmap);
>>>> + dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
>>>> + 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: %pe\n", pc->clk);
>>>> + 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: %pe\n", pc->rst);
>>>> + return ret;
>>>> + }
>>> Please use devm_reset_control_get_exclusive() to make it explicit an
>>> that exclusive reset control is requested. Given how the reset control
>>> is used, I think this driver could also use
>>> devm_reset_control_get_shared() to potentially allow sharing a reset
>>> line with other devices.
>> devm_reset_control_get() is a wrapper for devm_reset_control_get_exclusive().
>> Code as below:
>> static inline struct reset_control *devm_reset_control_get(
>> struct device *dev, const char *id)
>> {
>> return devm_reset_control_get_exclusive(dev, id);
>> }
>> Am i missing something else?
> Obviously you're missing the comment above of_reset_control_get about
> some functions being compatibility wrappers.
Oops, so sorry totally missed/overlooked that. Will update in v3. Thanks.
Regards,
Rahul
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Hi Uwe,
Thanks for your valuable feedback.
On 19/6/2020 2:02 pm, Uwe Kleine-König wrote:
> Hello Rahul,
>
> On Thu, Jun 18, 2020 at 08:05:13PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar
>> ---
>> drivers/pwm/Kconfig | 9 +
>> drivers/pwm/Makefile| 1 +
>> drivers/pwm/pwm-intel-lgm.c | 400
>>
>> 3 files changed, 410 insertions(+)
>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cb8d739067d2..a3303e22d5fa 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 fan controller driver for LGM SoC.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-intel-lgm.
>> +
>> config PWM_IQS620A
>> tristate "Azoteq IQS620A PWM support"
>> depends on MFD_IQS62X || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index a59c710e98c7..db154a6b4f51 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_IQS620A) += pwm-iqs620a.o
>> obj-$(CONFIG_PWM_JZ4740)+= pwm-jz4740.o
>> obj-$(CONFIG_PWM_LP3943)+= pwm-lp3943.o
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index ..3c7077acb161
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,400 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Intel Corporation.
>> + *
>> + * Notes & Limitations:
>> + * - The hardware supports fixed period which is dependent on 2/3 or 4
>> + * wire fan mode.
>> + * - Supports normal polarity. Does not support changing polarity.
>> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
>> + * keep track of running period.
>> + * - When duty cycle is changed, PWM output may be a mix of previous setting
>> + * and new setting for the first period. From second period, the output is
>> + * based on new setting.
>> + * - Supports 100% duty cycle.
>> + * - It is a dedicated PWM fan controller. There are no other consumers for
>> + * this PWM controller.
>> + */
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define PWM_FAN_CON00x0
>> +#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_MSKBIT(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_CON10x4
>> +#define PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
>> +
>> +#define PWM_FAN_STAT0x10
>> +#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 DC_BITS 8
>> +#define TWO_TENTH 204
>> +
>> +#define PERIOD_2WIRE_NSECS 4000
>> +#define PERIOD_4WIRE_NSECS 4
>> +
>> +#define TWO_SECONDS 2000
>> +#define IGNORE_FIRST_ERR1
>> +#define THIRTY_SECS_WINDOW 15
>> +#define ERR_CNT_THRESHOLD 6
>> +
>> +struct lgm_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;
>> +u32 period;
>> +struct delayed_work work;
>> +};
>> +
>> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
>> +{
>> +return container_of(chip, struct lgm_pwm_chip, chip);
>> +}
>> +
>> +static int lgm_pwm_update_dc(struct lgm_pwm_chip *pc, u32 val)
>> +{
>> +return
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Hi Philipp,
On 18/6/2020 8:25 pm, Philipp Zabel wrote:
> Hi Rahul,
>
> On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar
>> ---
>> drivers/pwm/Kconfig | 9 +
>> drivers/pwm/Makefile| 1 +
>> drivers/pwm/pwm-intel-lgm.c | 400
>>
>> 3 files changed, 410 insertions(+)
>> create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
> [...]
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index ..3c7077acb161
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,400 @@
> [...]
>> +static int lgm_pwm_probe(struct platform_device *pdev)
>> +{
>> +struct lgm_pwm_chip *pc;
>> +struct device *dev = >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))
>> +return PTR_ERR(io_base);
>> +
>> +pc->regmap = devm_regmap_init_mmio(dev, io_base, _regmap_config);
>> +if (IS_ERR(pc->regmap)) {
>> +ret = PTR_ERR(pc->regmap);
>> +dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
>> +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: %pe\n", pc->clk);
>> +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: %pe\n", pc->rst);
>> +return ret;
>> +}
> Please use devm_reset_control_get_exclusive() to make it explicit an
> that exclusive reset control is requested. Given how the reset control
> is used, I think this driver could also use
> devm_reset_control_get_shared() to potentially allow sharing a reset
> line with other devices.
devm_reset_control_get() is a wrapper for devm_reset_control_get_exclusive().
Code as below:
static inline struct reset_control *devm_reset_control_get(
struct device *dev, const char *id)
{
return devm_reset_control_get_exclusive(dev, id);
}
Am i missing something else?
Regards,
Rahul
Re: [PATCH v1 2/2] Add PWM driver for LGM
On 27/5/2020 5:15 pm, Andy Shevchenko wrote: > On Wed, May 27, 2020 at 02:28:53PM +0800, Tanwar, Rahul wrote: >> On 22/5/2020 4:56 pm, Uwe Kleine-König wrote: >>> On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote: > ... > >>> 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. >> Well noted about all other review concerns. I will rework the driver in v2. >> However, i am not very sure about the above point - of having a separate >> dedicated driver for tach_work because its logic is tightly coupled with >> this driver. > Actually I agree with Uwe. > Here is layering violation, i.e. provider and consumer in the same pot. It's > not good from design perspective. > Just to clarify, the PWM controller in our SoC serves just one purpose which is to control the fan. Its actually named as PWM Fan Controller. There is no other generic usage or any other consumer for this PWM driver. So separating out this part seems redundant to me. Also, if we separate it out as a dedicated driver, this will endup as a very small daemon which is going to be very hard to justify while upstreaming.. Regards, Rahul
Re: [PATCH v1 2/2] Add PWM driver for LGM
Hi Uwe,
Thanks for review.
On 22/5/2020 4:56 pm, Uwe Kleine-König wrote:
> 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
>> ---
>> 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.
[...]
>> +};
>> +
>> +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.
Well noted about all other review concerns. I will rework the driver in v2.
However, i am not very sure about the above point - of having a separate
dedicated driver for tach_work because its logic is tightly coupled with
this driver.
Regards,
Rahul
Re: [PATCH v8 2/2] clk: intel: Add CGU clock driver for a new SoC
Hi Stephen, On 27/5/2020 10:10 am, Stephen Boyd wrote: > Quoting Rahul Tanwar (2020-04-16 22:54:47) >> diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c >> new file mode 100644 >> index ..802a7fa88535 >> --- /dev/null >> +++ b/drivers/clk/x86/clk-cgu.c >> @@ -0,0 +1,636 @@ > [...] >> + ctx->membase = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(ctx->membase)) >> + return PTR_ERR(ctx->membase); >> + >> + ctx->np = np; >> + ctx->dev = dev; >> + spin_lock_init(>lock); >> + >> + ret = lgm_clk_register_plls(ctx, lgm_pll_clks, >> + ARRAY_SIZE(lgm_pll_clks)); >> + if (ret) >> + return ret; >> + >> + ret = lgm_clk_register_branches(ctx, lgm_branch_clks, >> + ARRAY_SIZE(lgm_branch_clks)); >> + if (ret) >> + return ret; >> + >> + ret = lgm_clk_register_ddiv(ctx, lgm_ddiv_clks, >> + ARRAY_SIZE(lgm_ddiv_clks)); >> + if (ret) >> + return ret; >> + >> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, >> + >clk_data); >> + if (ret) >> + return ret; > Are any of the clks unregistered on failure? It looks like devm_ isn't > used for registration so nothing can be undone? Please fix this in a > future patch. Thanks a lot for accepting the patch series. I went through all of your comments and i agree with all of them. Will fix it & address other review concerns in a future patch once 5.8 is released. Regards, Rahul
Re: [PATCH] serial: lantiq: Add x86 in Kconfig dependencies for Lantiq serial driver
On 6/5/2020 2:41 pm, Greg KH wrote: > On Wed, May 06, 2020 at 12:49:57PM +0800, Tanwar, Rahul wrote: >> On 5/5/2020 10:25 pm, Greg KH wrote: >>> On Mon, May 04, 2020 at 04:03:52PM +0800, Rahul Tanwar wrote: >>>> Lantiq serial driver/IP is reused for a x86 based SoC as well. >>>> Update the Kconfig accordingly. >>>> >>>> Signed-off-by: Rahul Tanwar >>>> --- >>>> drivers/tty/serial/Kconfig | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >>>> index 0aea76cd67ff..4b0a7b98f8c7 100644 >>>> --- a/drivers/tty/serial/Kconfig >>>> +++ b/drivers/tty/serial/Kconfig >>>> @@ -1035,7 +1035,7 @@ config SERIAL_SIFIVE_CONSOLE >>>> >>>> config SERIAL_LANTIQ >>>>bool "Lantiq serial driver" >>>> - depends on LANTIQ >>>> + depends on (LANTIQ || X86) || COMPILE_TEST >>>>select SERIAL_CORE >>>>select SERIAL_CORE_CONSOLE >>>>select SERIAL_EARLYCON >>>> -- >>>> 2.11.0 >>>> >>> Any reason this can't also be a module? >> Thanks a lot for accepting the patch. This driver is also used for >> console during bootup so we always have it as built in. > So no generic kernel images can ever be made for this driver? That's > not good, what about systems that have this serial port but does not > care about the console? > > That's just ensuring that it will not be built into any distro kernel > images, I suggest fixing this up please. I understand your concern. I will send out another incremental patch series to fix it assuming that this Kconfig change is already merged in tty tree. Thanks. Regards, Rahul > thanks, > > greg k-h
Re: [PATCH] serial: lantiq: Add x86 in Kconfig dependencies for Lantiq serial driver
On 5/5/2020 10:25 pm, Greg KH wrote: > On Mon, May 04, 2020 at 04:03:52PM +0800, Rahul Tanwar wrote: >> Lantiq serial driver/IP is reused for a x86 based SoC as well. >> Update the Kconfig accordingly. >> >> Signed-off-by: Rahul Tanwar >> --- >> drivers/tty/serial/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >> index 0aea76cd67ff..4b0a7b98f8c7 100644 >> --- a/drivers/tty/serial/Kconfig >> +++ b/drivers/tty/serial/Kconfig >> @@ -1035,7 +1035,7 @@ config SERIAL_SIFIVE_CONSOLE >> >> config SERIAL_LANTIQ >> bool "Lantiq serial driver" >> -depends on LANTIQ >> +depends on (LANTIQ || X86) || COMPILE_TEST >> select SERIAL_CORE >> select SERIAL_CORE_CONSOLE >> select SERIAL_EARLYCON >> -- >> 2.11.0 >> > Any reason this can't also be a module? Thanks a lot for accepting the patch. This driver is also used for console during bootup so we always have it as built in. Regards, Rahul > thanks, > > greg k-h
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
Hi Linus,
Thanks for taking time out to review.
On 5/10/2019 4:28 AM, Linus Walleij wrote:
>> +config PINCTRL_EQUILIBRIUM
>> + tristate "Generic pinctrl and GPIO driver for Intel Lightning
>> Mountain SoC"
>> + select PINMUX
>> + select PINCONF
>> + select GPIOLIB
>> + select GPIOLIB_IRQCHIP
> Nice use of the GPIOLIB_IRQCHIP.
>
> Are you sure you can't just use GPIO_GENERIC as well?
> This is almost always usable when you have a register with
> n consecutive bits representing GPIO lines.
>
> Look how we use bgpio_init() in e.g. drivers/gpio/gpio-ftgpio010.c
> to cut down on boilerplate code, and we also get a spinlock
> protection and .get/.set_multiple() implementation for free.
I went through gpio-mmio.c & gpio-ftgpio010.c code. My understanding is
that GPIO_GENERIC can support a max of 64 consecutive bits representing
GPIO lines. We have more than 100 GPIO's and they are spread out across
4 different banks with non consecutive registers i.e. DATA_IN_0~31@offset0x0,
DATA_IN_32~63@offset0x100 and so on. In other words, i think we can not
support memory mapped GPIO controller.
>> +#include
>> +#include
> Why do you need these two includes?
Yes, these are redundant. I will remove them. Thanks.
>> +static const struct pin_config pin_cfg_type[] = {
>> + {"intel,pullup",PINCONF_TYPE_PULL_UP},
>> + {"intel,pulldown", PINCONF_TYPE_PULL_DOWN},
>> + {"intel,drive-current", PINCONF_TYPE_DRIVE_CURRENT},
>> + {"intel,slew-rate", PINCONF_TYPE_SLEW_RATE},
>> + {"intel,open-drain",PINCONF_TYPE_OPEN_DRAIN},
>> + {"intel,output",PINCONF_TYPE_OUTPUT},
>> +};
> So... if we are adding a new driver with a new DT binding,
> why use the made-up "intel," prefixed flags when we have the
> standard DT flags from
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> already handled by the core?
Yes, Andy & Rob have also raised same concerns. I will switch to using
standard DT properties & generic pinconf and remove redundant code.
Thanks.
>> +static inline void eqbr_set_val(void __iomem *addr, u32 offset,
>> + u32 mask, u32 set, raw_spinlock_t *lock)
>> +{
>> + u32 val;
>> + unsigned long flags;
>> +
>> + raw_spin_lock_irqsave(lock, flags);
>> + val = readl(addr) & ~(mask << offset);
>> + writel(val | ((set & mask) << offset), addr);
>> + raw_spin_unlock_irqrestore(lock, flags);
>> +}
> Mask-and-set is usually achieved with regmap-mmio if you
> dont use GPIO_GENERIC, but I think you can just use
> GPIO_GENERIC. All of these:
As mentioned above, we cannot use GPIO_GENERIC. And there was no specific
reason/motivation for us to use regmap-mmio because most of the driver's
that we referenced were using readl()/write(). Do you recommend us to remove
readl()/writel() and switch to regmap-mmio based design ?
>> +static int intel_eqbr_gpio_get_dir(struct gpio_chip *gc, unsigned int
>> offset)
>> +static int intel_eqbr_gpio_dir_input(struct gpio_chip *gc, unsigned int
>> offset)
>> +static int intel_eqbr_gpio_dir_output(struct gpio_chip *gc, unsigned int
>> offset,
>> +static void intel_eqbr_gpio_set(struct gpio_chip *gc,
>> + unsigned int offset, int dir)
>> +static int intel_eqbr_gpio_get(struct gpio_chip *gc, unsigned int offset)
> Look very bit-per-bit mapped.
>
>> +static int intel_eqbr_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
>> +{
>> + struct intel_gpio_desc *desc = gpiochip_get_data(gc);
> Since struct gpio_desc means a per-line state container
> and struct intel_gpio_desc refers to the whole chip, I think this
> struct should be renamed something like struct eqbr_gpio.
Just to clarify, we have 4 different GPIO banks and each GPIO bank is
implemented as a separate gpio_chip. So we have 4 instances of gpio_desc
each one having its own gpio_chip. What i mean to say is that gpio_desc
is actually not a per-line state container, its a per gpio_chip container.
>> + unsigned int virq;
>> +
>> + if (!desc->irq_domain)
>> + return -ENODEV;
>> +
>> + virq = irq_find_mapping(desc->irq_domain, offset);
>> + if (virq)
>> + return virq;
>> + else
>> + return irq_create_mapping(desc->irq_domain, offset);
>> +}
>> +
>> +static int gpiochip_setup(struct device *dev, struct intel_gpio_desc *desc)
>> +{
> (...)
>> + gc->to_irq = intel_eqbr_gpio_to_irq;
> You don't need any of this funku stuff. The GPIOLIB_IRQCHIP
> provides default implementations to do all this for you.
> Just look in drivers/gpio/gpio-ftgpio010.c and follow
> the pattern (look how I set up struct gpio_irq_chip using
> *girq etc). If you need anything custom we need some
> special motivation here.
Yes, i checked gpio-ftgpio010.c and agree that this is already handled
under GPIOLIB_IRQCHIP. I will make
Re: [PATCH v1 0/2] pinctrl: Add new pinctrl/GPIO driver
Hi Mika,
On 13/9/2019 4:18 PM, Mika Westerberg wrote:
> On Thu, Sep 12, 2019 at 11:11:32AM +0100, Linus Walleij wrote:
>> Hi Rahul,
>>
>> thanks for your patches!
>>
>> On Thu, Sep 12, 2019 at 8:59 AM Rahul Tanwar
>> wrote:
>>
>>> This series is to add pinctrl & GPIO controller driver for a new SoC.
>>> Patch 1 adds pinmux & GPIO controller driver.
>>> Patch 2 adds the dt bindings document & include file.
>>>
>>> Patches are against Linux 5.3-rc5 at below Git tree:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
>> OK nice, I think you need to include Mika Westerberg on this review
>> as well, because I think he likes to stay on top of all things intel
>> in pin control. (Also included two other Intel folks in Finland who usually
>> take an interest in these things.)
> Thanks Linus for looping me in.
>
> Even if this is not directly based on the stuff we have under
> drivers/pinctrl/intel/*, I have a couple of comments. I don't have this
> patch series in my inbox so I'm commenting here.
>
> Since the driver name is equilibrium I suggest you to name
> intel_pinctrl_driver and the like (probe, remove) to follow that
> convention to avoid confusing this with the Intel pinctrl drivers under
> drivers/pinctrl/intel/*.
>
> Maybe use eqbr prefix so then intel_pinctrl_driver becomes
> eqbr_pinctrl_driver and so on. Also all the structures like
> intel_pinctrl_drv_data should be changed accordingly.
>
> Ditto for:
>
> MODULE_DESCRIPTION("Intel Pinctrl Driver for LGM SoC");
>
> I think better would be:
>
> MODULE_DESCRIPTION("Pinctrl Driver for LGM SoC (Equilibrium)");
>
> Anyway you get the idea :)
Yes, i understand your point. Will update in v2. Thanks.
Regards,
Rahul
Re: [PATCH v1 1/2] pinctrl: Add pinmux & GPIO controller driver for new SoC
Hi Andy,
Thanks for your comments. I agree & will address all your review concerns in
v2 except below mentioned points where i need more clarification.
On 12/9/2019 10:30 PM, Andy Shevchenko wrote:
>> +static const struct pin_config pin_cfg_type[] = {
>> +{"intel,pullup",PINCONF_TYPE_PULL_UP},
>> +{"intel,pulldown", PINCONF_TYPE_PULL_DOWN},
>> +{"intel,drive-current", PINCONF_TYPE_DRIVE_CURRENT},
>> +{"intel,slew-rate", PINCONF_TYPE_SLEW_RATE},
>> +{"intel,open-drain",PINCONF_TYPE_OPEN_DRAIN},
>> +{"intel,output",PINCONF_TYPE_OUTPUT},
>> +};
> Doesn't DT provide a generic naming scheme for these?
For pinctrl multiplexing/configuration nodes, DT does provide generic names
for some properties but it does not seem to mandate it. It states that the
content of pin mux/conf nodes is defined entirely by the binding of the pin
controller device. There are many other examples of pinctrl drivers which use
different prop names than generic ones. For e.g. Samsung. Our understanding
is: if the node is generic i.e. handled by framework, then it should use
generic name. But if the node is private to driver, then it is better to
prefix it with driver name to avoid any conflicts.
>> +virq = irq_find_mapping(desc->irq_domain, offset);
>> +if (virq)
>> +return virq;
>>
>> +else
>> +return irq_create_mapping(desc->irq_domain, offset);
> Don't we have more clever helper for this? AFAIR something like this is done
> in
> IRQ framework when you get a mapping from certain domain.
>
I guess, you mean irq_domain_add_simple(). This function does optionally map
the IRQs but only statically assigned IRQs. We need dynamic gpio_to_irq
mappings which is why the gpio_chip->to_irq() is optionally provided so the
drivers requiring dynamic IRQ mappings can override this function.
But i can definitely get rid of redundant irq_find_mapping() because
irq_create_mapping() anyways invokes irq_find_mapping() first. I will remove
irq_find_mapping() and just use irq_create_mapping() here.
>> +static void eqbr_irq_handler(struct irq_desc *desc)
>> +{
>> +struct intel_gpio_desc *gc;
>> +struct irq_chip *ic;
>> +u32 pins, offset;
>> +unsigned int virq;
>> +
>> +gc = irq_desc_get_handler_data(desc);
>> +ic = irq_desc_get_chip(desc);
>> +
>> +chained_irq_enter(ic, desc);
>> +pins = readl(gc->membase + GPIO_IRNCR);
>> +
>> +for_each_set_bit(offset, (unsigned long *), gc->bank->nr_pins) {
>> +virq = irq_linear_revmap(gc->irq_domain, offset);
>> +if (!virq)
>> +pr_err("gc[%s]:pin:%d irq not registered!\n",
>> + gc->name, offset);
> dev_err() ? But Why is it needed? Shouldn't be registered as a spurious IRQ
> for
> later debugging?
IMHO, spurious IRQ can only be registered if none of the pins are valid.
Please see below alternative way of handling it & let me know if this
makes more sense.
@@ -313,6 +313,7 @@ static void eqbr_irq_handler(struct irq_desc *desc)
struct irq_chip *ic;
u32 pins, offset;
unsigned int virq;
+ int handled = 0;
gc = irq_desc_get_handler_data(desc);
ic = irq_desc_get_chip(desc);
@@ -322,12 +323,16 @@ static void eqbr_irq_handler(struct irq_desc *desc)
for_each_set_bit(offset, (unsigned long *), gc->bank->nr_pins) {
virq = irq_linear_revmap(gc->irq_domain, offset);
- if (!virq)
- pr_err("gc[%s]:pin:%d irq not registered!\n",
- gc->name, offset);
- else
+ if (virq) {
generic_handle_irq(virq);
+ handled++;
+ }
}
+
+ /* Spurious interrupt */
+ if (handled == 0)
+ handle_bad_irq(desc);
+
chained_irq_exit(ic, desc);
}
>> +static int add_config(struct intel_pinctrl_drv_data *drvdata,
>> + unsigned long **confs, unsigned int *nr_conf,
>> + unsigned long pinconf)
>> +{
>> +unsigned long *configs;
>> +struct device *dev = drvdata->dev;
>> +unsigned int num_conf = *nr_conf + 1;
>> +
>> +if (!(*nr_conf)) {
>> +configs = devm_kcalloc(dev, 1, sizeof(pinconf), GFP_KERNEL);
>> +if (!configs)
>> +return -ENOMEM;
>> +} else {
>> +configs = devm_kmemdup(dev, *confs,
>> + num_conf * sizeof(pinconf), GFP_KERNEL);
>> +if (!configs)
>> +return -ENOMEM;
>> +devm_kfree(dev, *confs);
> This a red flag for using devm_*().
> Either a sign of bad design or misplacement of devm_*().
I can switch to non devm versions i.e. kmalloc/kfree for these buffer
allocations. But this leaves me with a fundamental question. As i
understand, devm*() variants are to allocate
Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
Hi Martin, On 4/9/2019 2:53 AM, Martin Blumenstingl wrote: My understanding is that if we do not use syscon, then there is no point in using regmap because this driver uses simple 32 bit register access. Can directly read/write registers using readl() & writel(). Would you agree ? if there was only the LGM SoC then I would say: drop regmap however, last year a driver for the GRX350/GRX550 SoCs was proposed: [0] this was never updated but it seems to use the same "framework" as the LGM driver with this in mind I am for keeping regmap support because. I think it will be easier to add support for old SoCs like GRX350/GRX550 (but also VRX200), because the PLL sub-driver (I am assuming that it is similar on all SoCs) or some other helpers can be re-used across various SoCs instead of "duplicating" code (where one variant would use regmap and the other readl/writel). Earlier, we had discussed about it in our team. There are no plans to upstream mips based platform code, past up-streaming efforts for mips platforms were also dropped. GRX350/GRX550/VRX200 are all mips based platforms. Plan is to upstream only x86 based platforms. In-fact, i had removed GRX & other older SoCs support from this driver before sending for review. So we can consider only x86 based LGM family of SoCs for this driver & all of them will be reusing same IP. [...] + select OF_EARLY_FLATTREE there's not a single other "select OF_EARLY_FLATTREE" in driver/clk I'm not saying this is wrong but it makes me curious why you need this We need OF_EARLY_FLATTREE for LGM. But adding a new x86 platform for LGM is discouraged because that would lead to too many platforms. Only differentiating factor for LGM is CPU model ID but it can differentiate only at run time. So i had no option other then enabling it with some LGM specific core system module driver and CGU seemed perfect for this purpose. so when my x86 kernel maintainer enables CONFIG_INTEL_LGM_CGU_CLK then OF_EARLY_FLATTREE is enabled as well. does this hurt any existing x86 platform? if not: why can't we enable it for x86 unconditionally? IMHO, it will not hurt any other existing x86 platform but enabling it for x86 unconditionally also doesn't sound like a good idea. I now get your point that enabling OF_EARLY_FLATTREE here is a bit odd. I will remove it in next patch. Regards, Rahul I went through meson & qcom regmap clock code. Agree, it can be reused for mux, divider and gate. But as mentioned above, i am now considering to move away from using regmap. thank you for evaluating them. let's continue the discussion above whether regmap should be used - after that we decide (if needed) which regmap implementation to use Martin [0] https://patchwork.kernel.org/patch/10554401/
Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
Hi Martin,
On 3/9/2019 6:20 AM, Martin Blumenstingl wrote:
Hello,
I only noticed this patchset today and I don't have much time left.
Here's my initial impressions without going through the code in detail.
I'll continue my review in the next days (as time permits).
As with all other Intel LGM patches: I don't have access to the
datasheets, so it's possible that I don't understand
feel free to correct me in this case (I appreciate an explanation where
I was wrong, so I can learn from it)
[...]
--- /dev/null
+++ b/drivers/clk/intel/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+ depends on COMMON_CLK
+ select MFD_SYSCON
can you please explain the reason why you need to use syscon?
also please see [0] for a comment from Rob on another LGM dt-binding
regarding syscon
Actually, there is no need to use syscon for CGU in LGM. It got carried
over from older SoCs (Falcon Mountain) where CGU was a MFD device
because it included PHY registers as well. And PHY drivers were using
syscon node to access CGU regmap. But for LGM, this is not the case.
My understanding is that if we do not use syscon, then there is no
point in using regmap because this driver uses simple 32 bit register
access. Can directly read/write registers using readl() & writel().
Would you agree ?
Yi Xin, please correct me if you think i am mistaken anywhere. If not,
i will change the driver to not use regmap & instead use readl() &
writel().
+ select OF_EARLY_FLATTREE
there's not a single other "select OF_EARLY_FLATTREE" in driver/clk
I'm not saying this is wrong but it makes me curious why you need this
We need OF_EARLY_FLATTREE for LGM. But adding a new x86
platform for LGM is discouraged because that would lead to too
many platforms. Only differentiating factor for LGM is CPU model
ID but it can differentiate only at run time. So i had no option
other then enabling it with some LGM specific core system module
driver and CGU seemed perfect for this purpose.
[...]
diff --git a/drivers/clk/intel/clk-cgu.h b/drivers/clk/intel/clk-cgu.h
new file mode 100644
index ..e44396b4aad7
--- /dev/null
+++ b/drivers/clk/intel/clk-cgu.h
@@ -0,0 +1,278 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright(c) 2018 Intel Corporation.
+ * Zhu YiXin
+ */
+
+#ifndef __INTEL_CLK_H
+#define __INTEL_CLK_H
+
+struct intel_clk_mux {
+ struct clk_hw hw;
+ struct device *dev;
+ struct regmap *map;
+ unsigned int reg;
+ u8 shift;
+ u8 width;
+ unsigned long flags;
+};
+
+struct intel_clk_divider {
+ struct clk_hw hw;
+ struct device *dev;
+ struct regmap *map;
+ unsigned int reg;
+ u8 shift;
+ u8 width;
+ unsigned long flags;
+ const struct clk_div_table *table;
+};
+
+struct intel_clk_ddiv {
+ struct clk_hw hw;
+ struct device *dev;
+ struct regmap *map;
+ unsigned int reg;
+ u8 shift0;
+ u8 width0;
+ u8 shift1;
+ u8 width1;
+ u8 shift2;
+ u8 width2;
+ unsigned int mult;
+ unsigned int div;
+ unsigned long flags;
+};
+
+struct intel_clk_gate {
+ struct clk_hw hw;
+ struct device *dev;
+ struct regmap *map;
+ unsigned int reg;
+ u8 shift;
+ unsigned long flags;
+};
I know at least two existing regmap clock implementations:
- drivers/clk/qcom/clk-regmap*
- drivers/clk/meson/clk-regmap*
it would be great if we could decide to re-use one of those for the
"generic" clock types (mux, divider and gate).
Stephen, do you have any preference here?
personally I like the meson one, but I'm biased because I've used it
a lot in the past and I haven't used the qcom one at all.
I went through meson & qcom regmap clock code. Agree, it can be
reused for mux, divider and gate. But as mentioned above, i am now
considering to move away from using regmap.
Regards,
Rahul
Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
Hi Andy,
Thanks for your review comments.
On 28/8/2019 11:09 PM, Andy Shevchenko wrote:
On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
From: rtanwar
Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming
Intel network processor SoC. It provides programming interfaces to control
& configure all CPU & peripheral clocks. Add common clock framework based
clock controller driver for CGU.
drivers/clk/intel/Kconfig | 13 +
drivers/clk/intel/Makefile | 4 +
Any plans what to do with existing x86 folder there?
I checked the x86 folder. This driver's clock controller IP is totally
different than other clock drivers inside x86. So having a common
driver source is not a option. It is of course possible to move this
driver inside x86 folder. Please let me know if you think moving
this driver inside x86 folder makes more sense.
+++ b/drivers/clk/intel/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+ depends on COMMON_CLK
+ select MFD_SYSCON
+ select OF_EARLY_FLATTREE
+ bool "Intel Clock Genration Unit support"
Is it for X86? Don't you need a dependency?
Yes agree, will update in v2.
+/*
+ * Calculate formula:
+ * rate = (prate * mult + (prate * frac) / frac_div) / div
+ */
+static unsigned long
+intel_pll_calc_rate(unsigned long prate, unsigned int mult,
+ unsigned int div, unsigned int frac, unsigned int frac_div)
+{
+ u64 crate, frate, rate64;
+
+ rate64 = prate;
+ crate = rate64 * mult;
+
+ if (frac) {
This seems unnecessary.
I think you would like to check for frac_div instead?
Though I would rather to use frac = 0, frac_div = 1 and drop this conditional
completely.
frac_div value is fixed to BIT(24) i.e. always a non zero value. mult & div
are directly read from registers and by design the register values for
mult & div is also always a non zero value. However, frac can logically
be zero. So, I still find if (frac) condition most suitable here.
+ frate = rate64 * frac;
+ do_div(frate, frac_div);
+ crate += frate;
+ }
+ do_div(crate, div);
+
+ return (unsigned long)crate;
+}
+static struct clk_hw
+*intel_clk_register_pll(struct intel_clk_provider *ctx,
* is part of type.
Yes, will update in v2.
+ const struct intel_pll_clk_data *list)
+{
+ struct clk_init_data init;
+ struct intel_clk_pll *pll;
+ struct device *dev = ctx->dev;
+ struct clk_hw *hw;
+ int ret;
+
+ init.ops = _lgm_pll_ops;
+ init.name = list->name;
+ init.parent_names = list->parent_names;
+ init.num_parents = list->num_parents;
+
+ pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
+ if (!pll)
+ return ERR_PTR(-ENOMEM);
+
+ pll->map = ctx->map;
+ pll->dev = ctx->dev;
+ pll->reg = list->reg;
+ pll->flags = list->flags;
+ pll->type = list->type;
+ pll->hw.init =
+
+ hw = >hw;
Seems redundant temporary variable.
Agree, will update in v2.
+ ret = clk_hw_register(dev, hw);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return hw;
+}
+void intel_clk_register_plls(struct intel_clk_provider *ctx,
+const struct intel_pll_clk_data *list,
+ unsigned int nr_clk)
Indentation issues.
Will fix in v2.
+{
+ struct clk_hw *hw;
+ int i;
+
+ for (i = 0; i < nr_clk; i++, list++) {
+ hw = intel_clk_register_pll(ctx, list);
+ if (IS_ERR(hw)) {
+ dev_err(ctx->dev, "failed to register pll: %s\n",
+ list->name);
Is it fatal or not?
Agree that its fatal if PLL registration fails, just that its highly
unlikely.
I will modify it to return error & make probe fail in failure cases.
+ continue;
+ }
+
+ intel_clk_add_lookup(ctx, hw, list->id);
+ }
No error to return? Are all PLLs optional?
Will change it to return error code.
+}
+#endif /* __INTEL_CLK_PLL_H */
One TAB is enough.
Will fix in v2.
+/*
+ * Copyright (C) 2018 Intel Corporation.
+ * Zhu YiXin
On space after asterisk is enough.
Will fix in v2.
+ */
+#define to_intel_clk_divider(_hw) \
+ container_of(_hw, struct intel_clk_divider, hw)
One TAB is enough.
Will fix in v2.
+ pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);
Is this useful?
Yes, IMO, this proves very useful for system wide clock issues
debugging during bootup.
+/*
+ * Below table defines the pair's of regval & effective dividers.
+ * It's more efficient to provide an explicit table due to non-linear
+ * relation between values.
+ */
+static const struct clk_div_table pll_div[] = {
Does val == 0 follows the table, i.e. makes
Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC
Hi Andy,
On 23/8/2019 8:56 PM, Andy Shevchenko wrote:
get_wallclock() and set_wallclock() are function pointers of platform_ops
which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()
at init time. Since adding a new platform to override these functions is
discouraged, so the only way is to modify RTC get/set functions.
Shouldn't it be platform agnostic code?
So, my point is, instead of hacking two functions, perhaps better to avoid them
at all.
Sorry, i could not understand your point. The changes are platform
agnostic i.e. it doesn't break existing use cases. Are you recommending
to add a new platform and make changes there ?
Nope, I propose to do something like
void __init foo()
{
if (platform has RTC)
return;
set_wallclock = noop;
get_wallclock = noop;
}
Thanks. I will work out a V2 patch as per your suggestion
and send out for review again.
Regards,
Rahul
Re: [PATCH] x86/cpu: Add new Airmont variant to Intel family
Hi Peter,
On 23/8/2019 5:03 PM, Peter Zijlstra wrote:
On Thu, Aug 22, 2019 at 01:35:44PM -0700, Luck, Tony wrote:
From: Tony Luck
One of the use cases for this processor is as a network
processor. So give it an "_NP" tag for now. Could be changed
later if it turns out to group with some other tag.
Signed-off-by: Tony Luck
---
arch/x86/include/asm/intel-family.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/intel-family.h
b/arch/x86/include/asm/intel-family.h
index 5c05b2d389c3..23ed388a3a56 100644
--- a/arch/x86/include/asm/intel-family.h
+++ b/arch/x86/include/asm/intel-family.h
@@ -95,6 +95,7 @@
#define INTEL_FAM6_ATOM_AIRMONT 0x4C /* Cherry Trail, Braswell */
#define INTEL_FAM6_ATOM_AIRMONT_MID 0x5A /* Moorefield */
+#define INTEL_FAM6_ATOM_AIRMONT_NP 0x75 /* Lightning Mountain */
#define INTEL_FAM6_ATOM_GOLDMONT 0x5C /* Apollo Lake */
#define INTEL_FAM6_ATOM_GOLDMONT_D0x5F /* Denverton */
Since it is 'just another airmont' with bits on, should we not then also
add it to all ATOM_AIRMONT sites already present in the kernel?
something like the below; except there were a few sites I skipped
because 'no clue'.
Also, while going over that, it looked like we missed AIRMONT_MID from a
few sites.
I'm thinking we want to add as many of these sites as possible and
correct when adding a new define; esp. for older microarchs that are
already well supported.
[PATCH v2 3/3] that i had sent with this series adds these changes which
are applicable to _NP. Please see below link:
https://lkml.org/lkml/2019/8/16/170
Above patch might have missed few additional points which still apply.
Regards,
Rahul
---
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 648260b5f367..56e6875b6882 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -4647,6 +4647,7 @@ __init int intel_pmu_init(void)
case INTEL_FAM6_ATOM_SILVERMONT_MID:
case INTEL_FAM6_ATOM_AIRMONT:
case INTEL_FAM6_ATOM_AIRMONT_MID:
+ case INTEL_FAM6_ATOM_AIRMONT_NP:
memcpy(hw_cache_event_ids, slm_hw_cache_event_ids,
sizeof(hw_cache_event_ids));
memcpy(hw_cache_extra_regs, slm_hw_cache_extra_regs,
diff --git a/arch/x86/events/intel/cstate.c b/arch/x86/events/intel/cstate.c
index 688592b34564..1e999092de22 100644
--- a/arch/x86/events/intel/cstate.c
+++ b/arch/x86/events/intel/cstate.c
@@ -602,6 +602,7 @@ static const struct x86_cpu_id intel_cstates_match[]
__initconst = {
X86_CSTATES_MODEL(INTEL_FAM6_ATOM_SILVERMONT, slm_cstates),
X86_CSTATES_MODEL(INTEL_FAM6_ATOM_SILVERMONT_X, slm_cstates),
X86_CSTATES_MODEL(INTEL_FAM6_ATOM_AIRMONT, slm_cstates),
+ X86_CSTATES_MODEL(INTEL_FAM6_ATOM_AIRMONT_NP, slm_cstates),
X86_CSTATES_MODEL(INTEL_FAM6_BROADWELL_CORE, snb_cstates),
X86_CSTATES_MODEL(INTEL_FAM6_BROADWELL_XEON_D, snb_cstates),
diff --git a/arch/x86/events/msr.c b/arch/x86/events/msr.c
index 9431447541e9..fe2323f114c0 100644
--- a/arch/x86/events/msr.c
+++ b/arch/x86/events/msr.c
@@ -72,6 +72,7 @@ static bool test_intel(int idx, void *data)
case INTEL_FAM6_ATOM_SILVERMONT:
case INTEL_FAM6_ATOM_SILVERMONT_X:
case INTEL_FAM6_ATOM_AIRMONT:
+ case INTEL_FAM6_ATOM_AIRMONT_NP:
case INTEL_FAM6_ATOM_GOLDMONT:
case INTEL_FAM6_ATOM_GOLDMONT_X:
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 5cc2d51cc25e..a3ccee6a16a5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1053,6 +1053,7 @@ static const __initconst struct x86_cpu_id
cpu_vuln_whitelist[] = {
VULNWL_INTEL(ATOM_SILVERMONT_X, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
VULNWL_INTEL(ATOM_SILVERMONT_MID, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
VULNWL_INTEL(ATOM_AIRMONT, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
+ VULNWL_INTEL(ATOM_AIRMONT_NP, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
VULNWL_INTEL(XEON_PHI_KNL, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
VULNWL_INTEL(XEON_PHI_KNM, NO_SSB | NO_L1TF | MSBDS_ONLY |
NO_SWAPGS),
diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 067858fe4db8..bde7a0c8fa8b 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -64,6 +64,7 @@ static const struct x86_cpu_id tsc_msr_cpu_ids[] = {
INTEL_CPU_FAM6(ATOM_SILVERMONT, freq_desc_byt),
INTEL_CPU_FAM6(ATOM_SILVERMONT_MID, freq_desc_tng),
INTEL_CPU_FAM6(ATOM_AIRMONT,freq_desc_cht),
+ INTEL_CPU_FAM6(ATOM_AIRMONT_NP, freq_desc_cht),
INTEL_CPU_FAM6(ATOM_AIRMONT_MID,freq_desc_ann),
{}
};
diff --git a/arch/x86/platform/atom/punit_atom_debug.c
b/arch/x86/platform/atom/punit_atom_debug.c
index ee6b0780bea1..52990f68af70
Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC
Hi Andy, On 22/8/2019 9:04 PM, Andy Shevchenko wrote: On Thu, Aug 22, 2019 at 05:26:33PM +0800, Tanwar, Rahul wrote: On 22/8/2019 5:02 PM, Andy Shevchenko wrote: On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote: Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before. Can't we rather update ->get_wallclock() and ->set_wallclock() based on this? get_wallclock() and set_wallclock() are function pointers of platform_ops which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss() at init time. Since adding a new platform to override these functions is discouraged, so the only way is to modify RTC get/set functions. Shouldn't it be platform agnostic code? So, my point is, instead of hacking two functions, perhaps better to avoid them at all. Sorry, i could not understand your point. The changes are platform agnostic i.e. it doesn't break existing use cases. Are you recommending to add a new platform and make changes there ? Regards, Rahul
Re: [PATCH v1 2/2] dt-bindings: rtc: Add optional status property
On 22/8/2019 5:09 PM, Alexandre Belloni wrote: On 22/08/2019 11:56:59+0300, Andy Shevchenko wrote: On Thu, Aug 22, 2019 at 03:44:04PM +0800, Rahul Tanwar wrote: Some products may not support MC146818 RTC CMOS device. Introduce a optional 'status' standard property for RTC-CMOS to indicate if the MC146818 RTC device is available (status="okay") or not (status="disabled") This needs to be converted to YAML Well, I think the status property doesn't even need to be documented because it is simply the generic behaviour. Thanks for your comments. I now realize it. I will omit posting this again. Regards, Rahul
Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC
Hi Andy, On 22/8/2019 5:02 PM, Andy Shevchenko wrote: On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote: Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before. Can't we rather update ->get_wallclock() and ->set_wallclock() based on this? get_wallclock() and set_wallclock() are function pointers of platform_ops which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss() at init time. Since adding a new platform to override these functions is discouraged, so the only way is to modify RTC get/set functions. Regards, Rahul
Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well
Hi Thomas, On 22/8/2019 12:47 AM, Andy Shevchenko wrote: For DT we can actually avoid that completely. See below. For ACPI not unfortunately as the stupid GSI mapping is hard coded. The below works better for my case, so, if you are going with that Tested-by: Andy Shevchenko 8<- --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -2438,7 +2438,13 @@ unsigned int arch_dynirq_lower_bound(uns * dmar_alloc_hwirq() may be called before setup_IO_APIC(), so use * gsi_top if ioapic_dynirq_base hasn't been initialized yet. */ - return ioapic_initialized ? ioapic_dynirq_base : gsi_top; + if (!ioapic_initialized) + return gsi_top; + /* +* For DT enabled machines ioapic_dynirq_base is irrelevant and not +* updated. So simply return @from if ioapic_dynirq_base == 0. +*/ + return ioapic_dynirq_base ? : from; } #ifdef CONFIG_X86_32 I have also tested above and it works fine. In fact, my first patch to resolve it during internal review was exactly on similar lines. So if you are going to add above then i will stop following up on this topic further. Thanks. Regards, Rahul
Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well
On 21/8/2019 4:34 PM, Thomas Gleixner wrote: Secondly, this link is irrelevant. ioapic_dynirq_base has nothing to do with virtual IRQ number 0. It's a boundary for the dynamic allocation of virtual interrupt numbers so that the core allocator does not pick interrupts out of the IOAPIC's fixed interrupt number space. This can be legitimately 0 when IOAPIC is not enabled at all. Can you please explain what kind of problem you were seing and what this really fixes? The problem is that device tree infrastructure considers 0 IRQ value as invalid/error value whereas for ACPI, 0 is a valid value. Without this change, the problem that we see is that the first driver using of_irq_get_xx() or its variants fails because of 0 IRQ number. With this change, allocated IRQ number is never 0 so it works ok.
Re: [PATCH v2 2/3] x86/cpu: Add new Intel Atom CPU model name
On 20/8/2019 10:57 PM, Peter Zijlstra wrote:
On Tue, Aug 20, 2019 at 12:48:05PM +, Luck, Tony wrote:
+#define INTEL_FAM6_ATOM_AIRMONT_NP0x75 /* Lightning Mountain */
What's _NP ?
Network Processor. But that is too narrow a descriptor. This is going to be
used in
other areas besides networking.
I’m contemplating calling it AIRMONT2
What would describe the special sause that warranted a new SOC? If this
thing is marketed as 'Network Processor' then I suppose we can actually
use it, esp. if we're going to see this more, like the MID thing -- that
lived for a while over multiple uarchs.
Note that for the big cores we added the NNPI thing, which was for
Neural Network Processing something.
INTEL_FAM6_ATOM_AIRMONT_NP was used keeping in mind the recommended
symbol naming form i.e. INTEL_FAM6{OPTFAMILY}_{MICROARCH}{OPTDIFF}
where OPTDIFF is supposed to be the market segment.
This SoC uses AMT (Admantium/Airmont) configuration which is supposed to be
a higher configuration. Looking at other existing examples, it seems that
INTEL_FAM6_ATOM_AIRMONT_PLUS is most appropriate. Would you have any
concerns with _PLUS name ? Thanks.
Re: [PATCH 2/3] x86: cpu: Add new Intel Atom CPU type
On 16/8/2019 2:43 PM, Borislav Petkov wrote: Now to another question: you see how I put my reply to the previous mail *below* the quoted text. Why is yours ontop? Why not put it after mine since you're replying to it, like it is usually done on the mailing lists and thus not confuse the reading order? All I'm trying to say is, please do not top-post. So sorry for missing out on this point. Will always keep in mind from now on. Regards, Rahul
Re: [PATCH 2/3] x86: cpu: Add new Intel Atom CPU type
Hi Boris, Well noted, will have Tony in loop from now on. Thanks. Regards, Rahul On 15/8/2019 8:22 PM, Borislav Petkov wrote: On Thu, Aug 15, 2019 at 05:46:46PM +0800, Rahul Tanwar wrote: This patch adds a new variant of Intel Atom Airmont CPU model used in a network processor SoC named Lightning Mountain. Signed-off-by: Rahul Tanwar --- arch/x86/include/asm/intel-family.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h index 0278aa66ef62..cbbb8250370f 100644 --- a/arch/x86/include/asm/intel-family.h +++ b/arch/x86/include/asm/intel-family.h @@ -73,6 +73,7 @@ #define INTEL_FAM6_ATOM_AIRMONT 0x4C /* Cherry Trail, Braswell */ #define INTEL_FAM6_ATOM_AIRMONT_MID 0x5A /* Moorefield */ +#define INTEL_FAM6_ATOM_AIRMONT_NP 0x75 /* Lightning Mountain */ #define INTEL_FAM6_ATOM_GOLDMONT 0x5C /* Apollo Lake */ #define INTEL_FAM6_ATOM_GOLDMONT_X0x5F /* Denverton */ -- Also, in addition to what Thomas said, due to the fact that all the different groups within Intel are sending patches with model names, please synchronize that model naming and patch sending with Tony from now on: https://git.kernel.org/tip/5ed1c835ed8b522ce25071cc2d56a9a09bd5b59e He'll document the naming scheme and pay attention to what goes where so make sure you CC him, talk to him or have him in the loop, in general. Thx.
Re: [PATCH 1/3] x86: cpu: Use constant definitions for CPU type
Hi Thomas,
Thanks for your comments.
On 15/8/2019 6:31 PM, Thomas Gleixner wrote:
Rahul,
On Thu, 15 Aug 2019, Rahul Tanwar wrote:
Please use the proper prefix for your patches. x86 uses
x86/subsystem: not x86: subsystem:
Well noted.
This patch replaces direct values usage with constant definitions usage
when access CPU models.
Please do not use 'This patch'. We already know that this is a patch
otherwise you wouldn't have sent it with [PATCH] on the subject line,
right?
See Documentation/process/submitting-patches.rst and search for 'This
patch'.
Well noted.
Signed-off-by: Rahul Tanwar
Suggested-by: Andy Shevchenko
---
arch/x86/kernel/cpu/intel.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 8d6d92ebeb54..0419fba1ea56 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -265,9 +265,9 @@ static void early_init_intel(struct cpuinfo_x86 *c)
/* Penwell and Cloverview have the TSC which doesn't sleep on S3 */
if (c->x86 == 6) {
switch (c->x86_model) {
- case 0x27: /* Penwell */
- case 0x35: /* Cloverview */
- case 0x4a: /* Merrifield */
+ case INTEL_FAM6_ATOM_SALTWELL_MID: /* Penwell */
+ case INTEL_FAM6_ATOM_SALTWELL_TABLET: /* Cloverview */
+ case INTEL_FAM6_ATOM_SILVERMONT_MID:/* Merrifield */
Are these comments really still useful now that the defines are used? I
don't think so.
Agree that these comments can be removed here. These comments are useful to
associate the CPU model with the product name. But, i think, the right
place to have
these comments is intel-family.h. I will remove these comments from here
in V2.
Regards,
Rahul
