diff for duplicates of <65bf307d.170a0220.4d544.f32b@mx.google.com> diff --git a/a/1.txt b/N1/1.txt index dd77d40..1b5d756 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,186 +1,190 @@ -Hi Stefan,\r -\r -Thanks for your review.\r -Please see my reply below inline.\r -\r ->> From: Clark Wang <xiaoning.wang@nxp.com>\r ->>\r ->> This fixes the pwm output bug when decrease the duty cycle.\r ->> This is a limited workaround for the PWM IP issue TKT0577206.\r ->this looks like a patch from the vendor tree.\r -\r -[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\r -\r ->Could you please provide a link to the origin or at least to the\r ->document which describes TKT0577206?\r -[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.\r -\r ->As a i.MX6ULL user i couldn't find this issue in the chip errata. So are\r ->you sure that every PWM IP handled by this driver is affected?\r -[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.\r -\r ->> Root cause:\r ->> When the SAR FIFO is empty, the new write value will be directly applied\r ->> to SAR even the current period is not over.\r ->> If the new SAR value is less than the old one, and the counter is\r ->> greater than the new SAR value, the current period will not filp the\r ->s/filp/flip/ ?\r ->> level. This will result in a pulse with a duty cycle of 100%.\r ->>\r ->> Workaround:\r ->> Add an old value SAR write before updating the new duty cycle to SAR.\r ->> This will keep the new value is always in a not empty fifo, and can be\r ->> wait to update after a period finished.\r ->>\r ->> Limitation:\r ->> This workaround can only solve this issue when the PWM period is longer\r ->> than 2us(or <500KHz).\r ->>\r ->> Reviewed-by: Jun Li <jun.li@nxp.com>\r ->> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>\r ->> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\r ->> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>\r ->> ---\r ->> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>\r ->> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/\r ->>\r ->> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---\r ->> 1 file changed, 62 insertions(+), 5 deletions(-)\r ->>\r ->> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c\r ->> index 7d9bc43f12b0..1e500a5bf564 100644\r ->> --- a/drivers/pwm/pwm-imx27.c\r ->> +++ b/drivers/pwm/pwm-imx27.c\r ->> @@ -21,11 +21,13 @@\r ->> #include <linux/platform_device.h>\r ->> #include <linux/pwm.h>\r ->> #include <linux/slab.h>\r ->> +#include <linux/spinlock.h>\r ->>\r ->> #define MX3_PWMCR 0x00 /* PWM Control Register */\r ->> #define MX3_PWMSR 0x04 /* PWM Status Register */\r ->> #define MX3_PWMSAR 0x0C /* PWM Sample Register */\r ->> #define MX3_PWMPR 0x10 /* PWM Period Register */\r ->> +#define MX3_PWMCNR 0x14 /* PWM Counter Register */\r ->>\r ->> #define MX3_PWMCR_FWM GENMASK(27, 26)\r ->> #define MX3_PWMCR_STOPEN BIT(25)\r ->> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {\r ->> * value to return in that case.\r ->> */\r ->> unsigned int duty_cycle;\r ->> + spinlock_t lock;\r ->> };\r ->>\r ->> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip)\r ->> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\r ->>\r ->> sr = readl(imx->mmio_base + MX3_PWMSR);\r ->> fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);\r ->> - if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {\r ->> + if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {\r ->> period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),\r ->> NSEC_PER_MSEC);\r ->> - msleep(period_ms);\r ->> + msleep(period_ms * (fifoav - 2));\r ->This touches a different workaround ("pwm: imx: Avoid sample FIFO\r ->overflow for i.MX PWM version2") without any explanation.\r -[Pratik]: Sure, I will look into this. Thanks!\r ->>\r ->> sr = readl(imx->mmio_base + MX3_PWMSR);\r ->> if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))\r ->> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\r ->> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\r ->> const struct pwm_state *state)\r ->> {\r ->> - unsigned long period_cycles, duty_cycles, prescale;\r ->> + unsigned long period_cycles, duty_cycles, prescale, counter_check, flags;\r ->> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);\r ->> + void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;\r ->> + __force u32 sar_last, sar_current;\r ->> struct pwm_state cstate;\r ->> unsigned long long c;\r ->> unsigned long long clkrate;\r ->> int ret;\r ->> - u32 cr;\r ->> + u32 cr, timeout = 1000;\r ->>\r ->> pwm_get_state(pwm, &cstate);\r ->>\r ->> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\r ->> pwm_imx27_sw_reset(chip);\r ->> }\r ->>\r ->> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\r ->> + /*\r ->> + * This is a limited workaround. When the SAR FIFO is empty, the new\r ->> + * write value will be directly applied to SAR even the current period\r ->> + * is not over.\r ->> + * If the new SAR value is less than the old one, and the counter is\r ->> + * greater than the new SAR value, the current period will not filp\r ->The same typo as in the commit message.\r ->> + * the level. This will result in a pulse with a duty cycle of 100%.\r ->> + * So, writing the current value of the SAR to SAR here before updating\r ->> + * the new SAR value can avoid this issue.\r ->> + *\r ->> + * Add a spin lock and turn off the interrupt to ensure that the\r ->> + * real-time performance can be guaranteed as much as possible when\r ->> + * operating the following operations.\r ->> + *\r ->> + * 1. Add a threshold of 1.5us. If the time T between the read current\r ->> + * count value CNR and the end of the cycle is less than 1.5us, wait\r ->> + * for T to be longer than 1.5us before updating the SAR register.\r ->> + * This is to avoid the situation that when the first SAR is written,\r ->> + * the current cycle just ends and the SAR FIFO that just be written\r ->> + * is emptied again.\r ->> + *\r ->> + * 2. Use __raw_writel() to minimize the interval between two writes to\r ->> + * the SAR register to increase the fastest pwm frequency supported.\r ->> + *\r ->> + * When the PWM period is longer than 2us(or <500KHz), this workaround\r ->> + * can solve this problem.\r ->> + */\r ->> + if (duty_cycles < imx->duty_cycle) {\r ->> + c = clkrate * 1500;\r ->> + do_div(c, NSEC_PER_SEC);\r ->> + counter_check = c;\r ->> + sar_last = (__force u32) cpu_to_le32(imx->duty_cycle);\r ->> + sar_current = (__force u32) cpu_to_le32(duty_cycles);\r ->> +\r ->> + spin_lock_irqsave(&imx->lock, flags);\r ->> + if (state->period >= 2000) {\r ->> + while ((period_cycles -\r ->> + readl_relaxed(imx->mmio_base + MX3_PWMCNR))\r ->> + < counter_check) {\r ->> + if (!--timeout)\r ->> + break;\r ->> + };\r ->> + }\r ->> + if (!(MX3_PWMSR_FIFOAV &\r ->> + readl_relaxed(imx->mmio_base + MX3_PWMSR)))\r ->> + __raw_writel(sar_last, reg_sar);\r ->> + __raw_writel(sar_current, reg_sar);\r ->> + spin_unlock_irqrestore(&imx->lock, flags);\r ->> + } else\r ->> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\r ->> +\r ->This is hard to believe that checkpatch.pl is fine with this patch.\r ->Please use it before submission.\r -[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!\r -\r ->> writel(period_cycles, imx->mmio_base + MX3_PWMPR);\r ->>\r ->> /*\r ->> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)\r ->> return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),\r ->> "failed to get peripheral clock\n");\r ->>\r ->> + spin_lock_init(&imx->lock);\r ->> + imx->duty_cycle = 0;\r ->This line looks unrelated and unnecessary.\r -[Pratik]: Right. I will remove this line in next patch version.\r -\r ->Best regards\r ->> imx->chip.ops = &pwm_imx27_ops;\r ->> imx->chip.dev = &pdev->dev;\r +Hi Stefan, + +Thanks for your review. +Please see my reply below inline. + +>> From: Clark Wang <xiaoning.wang@nxp.com> +>> +>> This fixes the pwm output bug when decrease the duty cycle. +>> This is a limited workaround for the PWM IP issue TKT0577206. +>this looks like a patch from the vendor tree. + +[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317 + +>Could you please provide a link to the origin or at least to the +>document which describes TKT0577206? +[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf. + +>As a i.MX6ULL user i couldn't find this issue in the chip errata. So are +>you sure that every PWM IP handled by this driver is affected? +[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP. + +>> Root cause: +>> When the SAR FIFO is empty, the new write value will be directly applied +>> to SAR even the current period is not over. +>> If the new SAR value is less than the old one, and the counter is +>> greater than the new SAR value, the current period will not filp the +>s/filp/flip/ ? +>> level. This will result in a pulse with a duty cycle of 100%. +>> +>> Workaround: +>> Add an old value SAR write before updating the new duty cycle to SAR. +>> This will keep the new value is always in a not empty fifo, and can be +>> wait to update after a period finished. +>> +>> Limitation: +>> This workaround can only solve this issue when the PWM period is longer +>> than 2us(or <500KHz). +>> +>> Reviewed-by: Jun Li <jun.li@nxp.com> +>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> +>> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317 +>> Tested-by: Pratik Manvar <pratik.manvar@ifm.com> +>> --- +>> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com> +>> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/ +>> +>> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++--- +>> 1 file changed, 62 insertions(+), 5 deletions(-) +>> +>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c +>> index 7d9bc43f12b0..1e500a5bf564 100644 +>> --- a/drivers/pwm/pwm-imx27.c +>> +++ b/drivers/pwm/pwm-imx27.c +>> @@ -21,11 +21,13 @@ +>> #include <linux/platform_device.h> +>> #include <linux/pwm.h> +>> #include <linux/slab.h> +>> +#include <linux/spinlock.h> +>> +>> #define MX3_PWMCR 0x00 /* PWM Control Register */ +>> #define MX3_PWMSR 0x04 /* PWM Status Register */ +>> #define MX3_PWMSAR 0x0C /* PWM Sample Register */ +>> #define MX3_PWMPR 0x10 /* PWM Period Register */ +>> +#define MX3_PWMCNR 0x14 /* PWM Counter Register */ +>> +>> #define MX3_PWMCR_FWM GENMASK(27, 26) +>> #define MX3_PWMCR_STOPEN BIT(25) +>> @@ -91,6 +93,7 @@ struct pwm_imx27_chip { +>> * value to return in that case. +>> */ +>> unsigned int duty_cycle; +>> + spinlock_t lock; +>> }; +>> +>> #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) +>> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip, +>> +>> sr = readl(imx->mmio_base + MX3_PWMSR); +>> fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr); +>> - if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) { +>> + if (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) { +>> period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), +>> NSEC_PER_MSEC); +>> - msleep(period_ms); +>> + msleep(period_ms * (fifoav - 2)); +>This touches a different workaround ("pwm: imx: Avoid sample FIFO +>overflow for i.MX PWM version2") without any explanation. +[Pratik]: Sure, I will look into this. Thanks! +>> +>> sr = readl(imx->mmio_base + MX3_PWMSR); +>> if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr)) +>> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip, +>> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, +>> const struct pwm_state *state) +>> { +>> - unsigned long period_cycles, duty_cycles, prescale; +>> + unsigned long period_cycles, duty_cycles, prescale, counter_check, flags; +>> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); +>> + void __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR; +>> + __force u32 sar_last, sar_current; +>> struct pwm_state cstate; +>> unsigned long long c; +>> unsigned long long clkrate; +>> int ret; +>> - u32 cr; +>> + u32 cr, timeout = 1000; +>> +>> pwm_get_state(pwm, &cstate); +>> +>> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, +>> pwm_imx27_sw_reset(chip); +>> } +>> +>> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); +>> + /* +>> + * This is a limited workaround. When the SAR FIFO is empty, the new +>> + * write value will be directly applied to SAR even the current period +>> + * is not over. +>> + * If the new SAR value is less than the old one, and the counter is +>> + * greater than the new SAR value, the current period will not filp +>The same typo as in the commit message. +>> + * the level. This will result in a pulse with a duty cycle of 100%. +>> + * So, writing the current value of the SAR to SAR here before updating +>> + * the new SAR value can avoid this issue. +>> + * +>> + * Add a spin lock and turn off the interrupt to ensure that the +>> + * real-time performance can be guaranteed as much as possible when +>> + * operating the following operations. +>> + * +>> + * 1. Add a threshold of 1.5us. If the time T between the read current +>> + * count value CNR and the end of the cycle is less than 1.5us, wait +>> + * for T to be longer than 1.5us before updating the SAR register. +>> + * This is to avoid the situation that when the first SAR is written, +>> + * the current cycle just ends and the SAR FIFO that just be written +>> + * is emptied again. +>> + * +>> + * 2. Use __raw_writel() to minimize the interval between two writes to +>> + * the SAR register to increase the fastest pwm frequency supported. +>> + * +>> + * When the PWM period is longer than 2us(or <500KHz), this workaround +>> + * can solve this problem. +>> + */ +>> + if (duty_cycles < imx->duty_cycle) { +>> + c = clkrate * 1500; +>> + do_div(c, NSEC_PER_SEC); +>> + counter_check = c; +>> + sar_last = (__force u32) cpu_to_le32(imx->duty_cycle); +>> + sar_current = (__force u32) cpu_to_le32(duty_cycles); +>> + +>> + spin_lock_irqsave(&imx->lock, flags); +>> + if (state->period >= 2000) { +>> + while ((period_cycles - +>> + readl_relaxed(imx->mmio_base + MX3_PWMCNR)) +>> + < counter_check) { +>> + if (!--timeout) +>> + break; +>> + }; +>> + } +>> + if (!(MX3_PWMSR_FIFOAV & +>> + readl_relaxed(imx->mmio_base + MX3_PWMSR))) +>> + __raw_writel(sar_last, reg_sar); +>> + __raw_writel(sar_current, reg_sar); +>> + spin_unlock_irqrestore(&imx->lock, flags); +>> + } else +>> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); +>> + +>This is hard to believe that checkpatch.pl is fine with this patch. +>Please use it before submission. +[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors! + +>> writel(period_cycles, imx->mmio_base + MX3_PWMPR); +>> +>> /* +>> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev) +>> return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per), +>> "failed to get peripheral clock\n"); +>> +>> + spin_lock_init(&imx->lock); +>> + imx->duty_cycle = 0; +>This line looks unrelated and unnecessary. +[Pratik]: Right. I will remove this line in next patch version. + +>Best regards +>> imx->chip.ops = &pwm_imx27_ops; +>> imx->chip.dev = &pdev->dev; >> imx->chip.npwm = 1; +_______________________________________________ +linux-arm-kernel mailing list +linux-arm-kernel@lists.infradead.org +http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/a/content_digest b/N1/content_digest index 5b7d50e..16bb0b0 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -22,191 +22,195 @@ " xiaoning.wang@nxp.com\0" "\00:1\0" "b\0" - "Hi Stefan,\r\n" - "\r\n" - "Thanks for your review.\r\n" - "Please see my reply below inline.\r\n" - "\r\n" - ">> From: Clark Wang <xiaoning.wang@nxp.com>\r\n" - ">>\r\n" - ">> This fixes the pwm output bug when decrease the duty cycle.\r\n" - ">> This is a limited workaround for the PWM IP issue TKT0577206.\r\n" - ">this looks like a patch from the vendor tree.\r\n" - "\r\n" - "[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\r\n" - "\r\n" - ">Could you please provide a link to the origin or at least to the\r\n" - ">document which describes TKT0577206?\r\n" - "[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.\r\n" - "\r\n" - ">As a i.MX6ULL user i couldn't find this issue in the chip errata. So are\r\n" - ">you sure that every PWM IP handled by this driver is affected?\r\n" - "[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.\r\n" - "\r\n" - ">> Root cause:\r\n" - ">> When the SAR FIFO is empty, the new write value will be directly applied\r\n" - ">> to SAR even the current period is not over.\r\n" - ">> If the new SAR value is less than the old one, and the counter is\r\n" - ">> greater than the new SAR value, the current period will not filp the\r\n" - ">s/filp/flip/ ?\r\n" - ">> level. This will result in a pulse with a duty cycle of 100%.\r\n" - ">>\r\n" - ">> Workaround:\r\n" - ">> Add an old value SAR write before updating the new duty cycle to SAR.\r\n" - ">> This will keep the new value is always in a not empty fifo, and can be\r\n" - ">> wait to update after a period finished.\r\n" - ">>\r\n" - ">> Limitation:\r\n" - ">> This workaround can only solve this issue when the PWM period is longer\r\n" - ">> than 2us(or <500KHz).\r\n" - ">>\r\n" - ">> Reviewed-by: Jun Li <jun.li@nxp.com>\r\n" - ">> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>\r\n" - ">> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\r\n" - ">> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>\r\n" - ">> ---\r\n" - ">> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>\r\n" - ">> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/\r\n" - ">>\r\n" - ">> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---\r\n" - ">> 1 file changed, 62 insertions(+), 5 deletions(-)\r\n" - ">>\r\n" - ">> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c\r\n" - ">> index 7d9bc43f12b0..1e500a5bf564 100644\r\n" - ">> --- a/drivers/pwm/pwm-imx27.c\r\n" - ">> +++ b/drivers/pwm/pwm-imx27.c\r\n" - ">> @@ -21,11 +21,13 @@\r\n" - ">> #include <linux/platform_device.h>\r\n" - ">> #include <linux/pwm.h>\r\n" - ">> #include <linux/slab.h>\r\n" - ">> +#include <linux/spinlock.h>\r\n" - ">>\r\n" - ">> #define MX3_PWMCR\t\t\t0x00 /* PWM Control Register */\r\n" - ">> #define MX3_PWMSR\t\t\t0x04 /* PWM Status Register */\r\n" - ">> #define MX3_PWMSAR\t\t\t0x0C /* PWM Sample Register */\r\n" - ">> #define MX3_PWMPR\t\t\t0x10 /* PWM Period Register */\r\n" - ">> +#define MX3_PWMCNR\t\t\t0x14 /* PWM Counter Register */\r\n" - ">>\r\n" - ">> #define MX3_PWMCR_FWM\t\t\tGENMASK(27, 26)\r\n" - ">> #define MX3_PWMCR_STOPEN\t\tBIT(25)\r\n" - ">> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {\r\n" - ">> \t * value to return in that case.\r\n" - ">> \t */\r\n" - ">> \tunsigned int duty_cycle;\r\n" - ">> +\tspinlock_t lock;\r\n" - ">> };\r\n" - ">>\r\n" - ">> #define to_pwm_imx27_chip(chip)\tcontainer_of(chip, struct pwm_imx27_chip, chip)\r\n" - ">> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\r\n" - ">>\r\n" - ">> \tsr = readl(imx->mmio_base + MX3_PWMSR);\r\n" - ">> \tfifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);\r\n" - ">> -\tif (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {\r\n" - ">> +\tif (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {\r\n" - ">> \t\tperiod_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),\r\n" - ">> \t\t\t\t\t NSEC_PER_MSEC);\r\n" - ">> -\t\tmsleep(period_ms);\r\n" - ">> +\t\tmsleep(period_ms * (fifoav - 2));\r\n" - ">This touches a different workaround (\"pwm: imx: Avoid sample FIFO\r\n" - ">overflow for i.MX PWM version2\") without any explanation.\r\n" - "[Pratik]: Sure, I will look into this. Thanks!\r\n" - ">>\r\n" - ">> \t\tsr = readl(imx->mmio_base + MX3_PWMSR);\r\n" - ">> \t\tif (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))\r\n" - ">> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\r\n" - ">> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\r\n" - ">> \t\t\t const struct pwm_state *state)\r\n" - ">> {\r\n" - ">> -\tunsigned long period_cycles, duty_cycles, prescale;\r\n" - ">> +\tunsigned long period_cycles, duty_cycles, prescale, counter_check, flags;\r\n" - ">> \tstruct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);\r\n" - ">> +\tvoid __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;\r\n" - ">> +\t__force u32 sar_last, sar_current;\r\n" - ">> \tstruct pwm_state cstate;\r\n" - ">> \tunsigned long long c;\r\n" - ">> \tunsigned long long clkrate;\r\n" - ">> \tint ret;\r\n" - ">> -\tu32 cr;\r\n" - ">> +\tu32 cr, timeout = 1000;\r\n" - ">>\r\n" - ">> \tpwm_get_state(pwm, &cstate);\r\n" - ">>\r\n" - ">> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\r\n" - ">> \t\tpwm_imx27_sw_reset(chip);\r\n" - ">> \t}\r\n" - ">>\r\n" - ">> -\twritel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\r\n" - ">> +\t/*\r\n" - ">> +\t * This is a limited workaround. When the SAR FIFO is empty, the new\r\n" - ">> +\t * write value will be directly applied to SAR even the current period\r\n" - ">> +\t * is not over.\r\n" - ">> +\t * If the new SAR value is less than the old one, and the counter is\r\n" - ">> +\t * greater than the new SAR value, the current period will not filp\r\n" - ">The same typo as in the commit message.\r\n" - ">> +\t * the level. This will result in a pulse with a duty cycle of 100%.\r\n" - ">> +\t * So, writing the current value of the SAR to SAR here before updating\r\n" - ">> +\t * the new SAR value can avoid this issue.\r\n" - ">> +\t *\r\n" - ">> +\t * Add a spin lock and turn off the interrupt to ensure that the\r\n" - ">> +\t * real-time performance can be guaranteed as much as possible when\r\n" - ">> +\t * operating the following operations.\r\n" - ">> +\t *\r\n" - ">> +\t * 1. Add a threshold of 1.5us. If the time T between the read current\r\n" - ">> +\t * count value CNR and the end of the cycle is less than 1.5us, wait\r\n" - ">> +\t * for T to be longer than 1.5us before updating the SAR register.\r\n" - ">> +\t * This is to avoid the situation that when the first SAR is written,\r\n" - ">> +\t * the current cycle just ends and the SAR FIFO that just be written\r\n" - ">> +\t * is emptied again.\r\n" - ">> +\t *\r\n" - ">> +\t * 2. Use __raw_writel() to minimize the interval between two writes to\r\n" - ">> +\t * the SAR register to increase the fastest pwm frequency supported.\r\n" - ">> +\t *\r\n" - ">> +\t * When the PWM period is longer than 2us(or <500KHz), this workaround\r\n" - ">> +\t * can solve this problem.\r\n" - ">> +\t */\r\n" - ">> +\tif (duty_cycles < imx->duty_cycle) {\r\n" - ">> +\t\tc = clkrate * 1500;\r\n" - ">> +\t\tdo_div(c, NSEC_PER_SEC);\r\n" - ">> +\t\tcounter_check = c;\r\n" - ">> +\t\tsar_last = (__force u32) cpu_to_le32(imx->duty_cycle);\r\n" - ">> +\t\tsar_current = (__force u32) cpu_to_le32(duty_cycles);\r\n" - ">> +\r\n" - ">> +\t\tspin_lock_irqsave(&imx->lock, flags);\r\n" - ">> +\t\tif (state->period >= 2000) {\r\n" - ">> +\t\t\twhile ((period_cycles -\r\n" - ">> +\t\t\t\treadl_relaxed(imx->mmio_base + MX3_PWMCNR))\r\n" - ">> +\t\t\t\t< counter_check) {\r\n" - ">> +\t\t\t\tif (!--timeout)\r\n" - ">> +\t\t\t\t\tbreak;\r\n" - ">> +\t\t\t};\r\n" - ">> +\t\t}\r\n" - ">> +\t\tif (!(MX3_PWMSR_FIFOAV &\r\n" - ">> +\t\t readl_relaxed(imx->mmio_base + MX3_PWMSR)))\r\n" - ">> +\t\t\t__raw_writel(sar_last, reg_sar);\r\n" - ">> +\t\t__raw_writel(sar_current, reg_sar);\r\n" - ">> +\t\tspin_unlock_irqrestore(&imx->lock, flags);\r\n" - ">> +\t} else\r\n" - ">> +\t\twritel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\r\n" - ">> +\r\n" - ">This is hard to believe that checkpatch.pl is fine with this patch.\r\n" - ">Please use it before submission.\r\n" - "[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!\r\n" - "\r\n" - ">> \twritel(period_cycles, imx->mmio_base + MX3_PWMPR);\r\n" - ">>\r\n" - ">> \t/*\r\n" - ">> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)\r\n" - ">> \t\treturn dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),\r\n" - ">> \t\t\t\t \"failed to get peripheral clock\\n\");\r\n" - ">>\r\n" - ">> +\tspin_lock_init(&imx->lock);\r\n" - ">> +\timx->duty_cycle = 0;\r\n" - ">This line looks unrelated and unnecessary.\r\n" - "[Pratik]: Right. I will remove this line in next patch version.\r\n" - "\r\n" - ">Best regards\r\n" - ">> \timx->chip.ops = &pwm_imx27_ops;\r\n" - ">> \timx->chip.dev = &pdev->dev;\r\n" - ">> \timx->chip.npwm = 1;" + "Hi Stefan,\n" + "\n" + "Thanks for your review.\n" + "Please see my reply below inline.\n" + "\n" + ">> From: Clark Wang <xiaoning.wang@nxp.com>\n" + ">>\n" + ">> This fixes the pwm output bug when decrease the duty cycle.\n" + ">> This is a limited workaround for the PWM IP issue TKT0577206.\n" + ">this looks like a patch from the vendor tree.\n" + "\n" + "[Pratik]: Yes, this is the patch from NXP. Please see original link of the patch https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\n" + "\n" + ">Could you please provide a link to the origin or at least to the\n" + ">document which describes TKT0577206?\n" + "[Pratik]: Please refer i.MX8MN errata #ERR051198 in https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf.\n" + "\n" + ">As a i.MX6ULL user i couldn't find this issue in the chip errata. So are\n" + ">you sure that every PWM IP handled by this driver is affected?\n" + "[Pratik]: Yes, looks like this issue is on all platforms which uses this PWM IP.\n" + "\n" + ">> Root cause:\n" + ">> When the SAR FIFO is empty, the new write value will be directly applied\n" + ">> to SAR even the current period is not over.\n" + ">> If the new SAR value is less than the old one, and the counter is\n" + ">> greater than the new SAR value, the current period will not filp the\n" + ">s/filp/flip/ ?\n" + ">> level. This will result in a pulse with a duty cycle of 100%.\n" + ">>\n" + ">> Workaround:\n" + ">> Add an old value SAR write before updating the new duty cycle to SAR.\n" + ">> This will keep the new value is always in a not empty fifo, and can be\n" + ">> wait to update after a period finished.\n" + ">>\n" + ">> Limitation:\n" + ">> This workaround can only solve this issue when the PWM period is longer\n" + ">> than 2us(or <500KHz).\n" + ">>\n" + ">> Reviewed-by: Jun Li <jun.li@nxp.com>\n" + ">> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>\n" + ">> Link: https://github.com/nxp-imx/linux-imx/commit/16181cc4eee61d87cbaba0e5a479990507816317\n" + ">> Tested-by: Pratik Manvar <pratik.manvar@ifm.com>\n" + ">> ---\n" + ">> V1 -> V2: fix sparse warnings reported-by: kernel test robot <lkp@intel.com>\n" + ">> Closes: https://lore.kernel.org/oe-kbuild-all/202312300907.RGtYsKxb-lkp@intel.com/\n" + ">>\n" + ">> drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++---\n" + ">> 1 file changed, 62 insertions(+), 5 deletions(-)\n" + ">>\n" + ">> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c\n" + ">> index 7d9bc43f12b0..1e500a5bf564 100644\n" + ">> --- a/drivers/pwm/pwm-imx27.c\n" + ">> +++ b/drivers/pwm/pwm-imx27.c\n" + ">> @@ -21,11 +21,13 @@\n" + ">> #include <linux/platform_device.h>\n" + ">> #include <linux/pwm.h>\n" + ">> #include <linux/slab.h>\n" + ">> +#include <linux/spinlock.h>\n" + ">>\n" + ">> #define MX3_PWMCR\t\t\t0x00 /* PWM Control Register */\n" + ">> #define MX3_PWMSR\t\t\t0x04 /* PWM Status Register */\n" + ">> #define MX3_PWMSAR\t\t\t0x0C /* PWM Sample Register */\n" + ">> #define MX3_PWMPR\t\t\t0x10 /* PWM Period Register */\n" + ">> +#define MX3_PWMCNR\t\t\t0x14 /* PWM Counter Register */\n" + ">>\n" + ">> #define MX3_PWMCR_FWM\t\t\tGENMASK(27, 26)\n" + ">> #define MX3_PWMCR_STOPEN\t\tBIT(25)\n" + ">> @@ -91,6 +93,7 @@ struct pwm_imx27_chip {\n" + ">> \t * value to return in that case.\n" + ">> \t */\n" + ">> \tunsigned int duty_cycle;\n" + ">> +\tspinlock_t lock;\n" + ">> };\n" + ">>\n" + ">> #define to_pwm_imx27_chip(chip)\tcontainer_of(chip, struct pwm_imx27_chip, chip)\n" + ">> @@ -203,10 +206,10 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\n" + ">>\n" + ">> \tsr = readl(imx->mmio_base + MX3_PWMSR);\n" + ">> \tfifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);\n" + ">> -\tif (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {\n" + ">> +\tif (fifoav >= MX3_PWMSR_FIFOAV_3WORDS) {\n" + ">> \t\tperiod_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),\n" + ">> \t\t\t\t\t NSEC_PER_MSEC);\n" + ">> -\t\tmsleep(period_ms);\n" + ">> +\t\tmsleep(period_ms * (fifoav - 2));\n" + ">This touches a different workaround (\"pwm: imx: Avoid sample FIFO\n" + ">overflow for i.MX PWM version2\") without any explanation.\n" + "[Pratik]: Sure, I will look into this. Thanks!\n" + ">>\n" + ">> \t\tsr = readl(imx->mmio_base + MX3_PWMSR);\n" + ">> \t\tif (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))\n" + ">> @@ -217,13 +220,15 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,\n" + ">> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\n" + ">> \t\t\t const struct pwm_state *state)\n" + ">> {\n" + ">> -\tunsigned long period_cycles, duty_cycles, prescale;\n" + ">> +\tunsigned long period_cycles, duty_cycles, prescale, counter_check, flags;\n" + ">> \tstruct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);\n" + ">> +\tvoid __iomem *reg_sar = imx->mmio_base + MX3_PWMSAR;\n" + ">> +\t__force u32 sar_last, sar_current;\n" + ">> \tstruct pwm_state cstate;\n" + ">> \tunsigned long long c;\n" + ">> \tunsigned long long clkrate;\n" + ">> \tint ret;\n" + ">> -\tu32 cr;\n" + ">> +\tu32 cr, timeout = 1000;\n" + ">>\n" + ">> \tpwm_get_state(pwm, &cstate);\n" + ">>\n" + ">> @@ -264,7 +269,57 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,\n" + ">> \t\tpwm_imx27_sw_reset(chip);\n" + ">> \t}\n" + ">>\n" + ">> -\twritel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\n" + ">> +\t/*\n" + ">> +\t * This is a limited workaround. When the SAR FIFO is empty, the new\n" + ">> +\t * write value will be directly applied to SAR even the current period\n" + ">> +\t * is not over.\n" + ">> +\t * If the new SAR value is less than the old one, and the counter is\n" + ">> +\t * greater than the new SAR value, the current period will not filp\n" + ">The same typo as in the commit message.\n" + ">> +\t * the level. This will result in a pulse with a duty cycle of 100%.\n" + ">> +\t * So, writing the current value of the SAR to SAR here before updating\n" + ">> +\t * the new SAR value can avoid this issue.\n" + ">> +\t *\n" + ">> +\t * Add a spin lock and turn off the interrupt to ensure that the\n" + ">> +\t * real-time performance can be guaranteed as much as possible when\n" + ">> +\t * operating the following operations.\n" + ">> +\t *\n" + ">> +\t * 1. Add a threshold of 1.5us. If the time T between the read current\n" + ">> +\t * count value CNR and the end of the cycle is less than 1.5us, wait\n" + ">> +\t * for T to be longer than 1.5us before updating the SAR register.\n" + ">> +\t * This is to avoid the situation that when the first SAR is written,\n" + ">> +\t * the current cycle just ends and the SAR FIFO that just be written\n" + ">> +\t * is emptied again.\n" + ">> +\t *\n" + ">> +\t * 2. Use __raw_writel() to minimize the interval between two writes to\n" + ">> +\t * the SAR register to increase the fastest pwm frequency supported.\n" + ">> +\t *\n" + ">> +\t * When the PWM period is longer than 2us(or <500KHz), this workaround\n" + ">> +\t * can solve this problem.\n" + ">> +\t */\n" + ">> +\tif (duty_cycles < imx->duty_cycle) {\n" + ">> +\t\tc = clkrate * 1500;\n" + ">> +\t\tdo_div(c, NSEC_PER_SEC);\n" + ">> +\t\tcounter_check = c;\n" + ">> +\t\tsar_last = (__force u32) cpu_to_le32(imx->duty_cycle);\n" + ">> +\t\tsar_current = (__force u32) cpu_to_le32(duty_cycles);\n" + ">> +\n" + ">> +\t\tspin_lock_irqsave(&imx->lock, flags);\n" + ">> +\t\tif (state->period >= 2000) {\n" + ">> +\t\t\twhile ((period_cycles -\n" + ">> +\t\t\t\treadl_relaxed(imx->mmio_base + MX3_PWMCNR))\n" + ">> +\t\t\t\t< counter_check) {\n" + ">> +\t\t\t\tif (!--timeout)\n" + ">> +\t\t\t\t\tbreak;\n" + ">> +\t\t\t};\n" + ">> +\t\t}\n" + ">> +\t\tif (!(MX3_PWMSR_FIFOAV &\n" + ">> +\t\t readl_relaxed(imx->mmio_base + MX3_PWMSR)))\n" + ">> +\t\t\t__raw_writel(sar_last, reg_sar);\n" + ">> +\t\t__raw_writel(sar_current, reg_sar);\n" + ">> +\t\tspin_unlock_irqrestore(&imx->lock, flags);\n" + ">> +\t} else\n" + ">> +\t\twritel(duty_cycles, imx->mmio_base + MX3_PWMSAR);\n" + ">> +\n" + ">This is hard to believe that checkpatch.pl is fine with this patch.\n" + ">Please use it before submission.\n" + "[Pratik]: I used the checkpatch.pl in this patch and that runs without any warnings/errors!\n" + "\n" + ">> \twritel(period_cycles, imx->mmio_base + MX3_PWMPR);\n" + ">>\n" + ">> \t/*\n" + ">> @@ -324,6 +379,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)\n" + ">> \t\treturn dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),\n" + ">> \t\t\t\t \"failed to get peripheral clock\\n\");\n" + ">>\n" + ">> +\tspin_lock_init(&imx->lock);\n" + ">> +\timx->duty_cycle = 0;\n" + ">This line looks unrelated and unnecessary.\n" + "[Pratik]: Right. I will remove this line in next patch version.\n" + "\n" + ">Best regards\n" + ">> \timx->chip.ops = &pwm_imx27_ops;\n" + ">> \timx->chip.dev = &pdev->dev;\n" + ">> \timx->chip.npwm = 1;\n" + "_______________________________________________\n" + "linux-arm-kernel mailing list\n" + "linux-arm-kernel@lists.infradead.org\n" + http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -b94dd9bac4b9955d8d9983a5997d90340c3887c341253ba29a8cd8329685df2e +c9481c1fc58a990829866234063cf585d24fd21cc901c31f53c9457da9dca187
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.