On 22.08.2017 15:24, Benjamin Gaignard wrote: > 2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@xxxxxxxxxxxxx>: >> Hi Thierry, >> >> I added few other comments below. Please let me know what you think. >> >> Thank you, >> Claudiu >> >> On 21.08.2017 14:25, Thierry Reding wrote: >>> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: >>>> Hi Thierry, >>>> >>>> Thank you for your response. I added few comments below. >>>> >>>> Thank you, >>>> Claudiu >>>> >>>> On 21.08.2017 11:25, Thierry Reding wrote: >>>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >>>>>> Hi all, >>>>>> >>>>>> Please give feedback on these patches which extends the PWM >>>>>> framework in order to support multiple PWM signal types. >>>>>> Since I didn't receive any inputs on RFC series I'm resending it as >>>>>> normal patch series. >>>>>> >>>>>> The current patch series add the following PWM signal types: >>>>>> - PWM complementary signals >>>>>> - PWM push-pull signal >>>>>> >>>>>> Definition of PWM complementary mode: >>>>>> For a PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals >>>>>> have opposite levels, same duration and same starting times, >>>>>> as in the following diagram: >>>>>> >>>>>> __ __ __ __ >>>>>> PWMH __| |__| |__| |__| |__ >>>>>> __ __ __ __ __ >>>>>> PWML |__| |__| |__| |__| >>>>>> <--T--> >>>>>> Where T is the signal period. >>>>>> >>>>>> Definition of PWM push-pull mode: >>>>>> For PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >>>>>> have same levels, same duration and are delayed until the begining >>>>>> of the next period, as in the following diagram: >>>>>> >>>>>> __ __ >>>>>> PWMH __| |________| |________ >>>>>> __ __ >>>>>> PWML ________| |________| |__ >>>>>> <--T--> >>>>>> >>>>>> Where T is the signal period. >>>>>> >>>>>> These output signals could be configured by setting PWM mode >>>>>> (a new input in sysfs has been added in order to support this >>>>>> operation). >>>>>> >>>>>> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >>>>>> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >>>>>> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >>>>>> >>>>>> The PWM push-pull mode could be usefull in applications like >>>>>> half bridge converters. >>>>> >>>>> Sorry for taking an absurdly long time to get back to you on this. >>>>> >>>>> One problem I see with this is that the PWM framework is built on the >>>>> assumption that we have a single output per PWM channel. This becomes >>>>> important when you start adding features such as this because now the >>>>> users have no way of determining whether or not the PWM they retrieve >>>>> will actually be able to do what they want. >>>> I was thinking that the framework is there to offer support in configuring >>>> the underlying hardware without taking into account how many outputs per >>>> PWM channels are actually present. It is true I only worked with Atmel/Microchip >>>> PWM controllers which for instance, SAMA5 SoC has a PWM controller >>>> with 2 outputs per PWM channel. Taking into account that the driver is >>>> already mainlined I though that the user is aware of what he is using >>>> (either a one output per PWM channel controller, or 2 outputs or >>>> more than 2) and about the underlying hardware support. I understand your >>>> position on this. I'm just explaining myself on the approach I've chose >>>> for this implementation. >>> >>> So currently we assume that there will be (at least) one output per PWM >>> channel. However there are currently no drivers that actually use any >>> output other than the "primary" one. >>> >>> That means, even if there are currently PWM controllers that support >>> multiple outputs per PWM channel, we have no code making assumptions >>> about it. >>> >>> Before we can add code that makes any assumptions about the number of >>> outputs per PWM channel, we have to add facitilities for such code to >>> detect capabilities, so that they can deal with PWMs that don't match >>> what they need. >>> >>>>> For example, let's say you have a half bridge converter driver in the >>>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing >>>>> preventing it from using a simple one-output PWM and there's no way to >>>>> check that we're actually doing something wrong. >>>> I understand your position here. I've chose this approach taking into >>>> account that the user of the PWM will be aware of the capabilities >>>> of the underlying hardware because I worked on a controller which already >>>> has a mainlined driver and which has more than one output per PWM channel >>>> and I believe there are also other controllers with this kind of capability >>>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver >>>> I saw a bool type variable in driver specific data structure (struct stm32_pwm): >>>> "bool have_complementary_output" which let me think that their controller >>>> also could support more than one output per PWM channel (I will also try >>>> to find the controller specific datasheet). At a first look I saw that also >>>> TI ECAP PWM controller supports this (it is true that I am not aware of how >>>> it is initialized in kernel, I need to check the datasheet, if it is public). >>> >>> Yes, it's quite possible that many of the controllers we currently >>> support have this feature or not. But that's not really relevant. The >>> only important bit is that none of the users know about it. We don't >>> have the means to configure anything beyond just one output. So the only >>> guarantee you will get with the current framework is that there will be >>> one output signal per PWM channel. >>> >>>>> I think any in-kernel API would have to be more fully-featured, >>>>> otherwise we're bound to run into problems. At the very least I think >>>>> we'd have to expose some sort of capabilities list. >>>> About the exposing of these capabilities would you prefer to have the >>>> supported PWM modes registered in driver probe as a new field mask >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_modes_mask; /* bitfield of supported signal types */ >>>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> And having in driver_probe() something like this: >>>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; >>>> pwmchip_add(&driver_data->chip); >>>> Since the PWM push-pull mode imply more than one output per PWM channel. And >>>> validate the supported PWM modes when trying to configure one. >>>> >>>> Or registering, as a field in pwm_chip, how many outputs per channel are actually >>>> supported by the PWM controller and then validate the supported PWM mode based >>>> on this? >>>> e.g.: >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_outputs; /* Number of supported outputs per channel */>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> In driver_probe(): >>>> //... >>>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ >>>> pwmchip_add(&driver_data->chip); >>> >>> I think I'd prefer something more flexible. I also think this can't be >>> per-chip, but really has to be at the granularity of single PWM >>> channels. How about something like this: >>> >>> struct pwm_caps { >>> ... >>> }; >>> >>> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> From a high-level perspective that would give users an easy way to >>> obtain the capabilities of the PWM they're handed. >> I forgot to mention, I don't know if you had time to look over the other >> patch series I send to extend the PWM framework: >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" >> (v2 since the auto build robot prompted me some compilation errors). >> In that series I proposed some extensions to be able to configure >> dead-times for PWM channels. Those changes are also specific to PWM >> controllers which has more than 1 output per PWM channel. Taking into >> account that: >> - this is also specific to PWM controllers with more than 1 output per >> PWM channel and >> - your proposed below to introduce capabilities specific to more than >> one output per PWM channel and not to introduce something to specify that >> the PWM chip or PWM device supports a number of X outputs per PWM channel and >> - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with >> lower delays b/w the edges of complementary outputs: >> Do you think it would be ok (in order to also have a way to configure >> PWM dead-times) to merge the changes of >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the >> changes that I will do for push-pull mode (with your below proposals) >> and have a single PWM mode for both PWM push-pull and PWM dead-times >> (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used >> to configure the dead-times that will work only when the PWM device supports >> push-pull mode and is configured in push-pull mode? >> Something like this: >> >> root@sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_re <<<< interface for configuring dead-time rising edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_fe <<<< interface for configuring dead-time falling edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode <<<< interface for PWM mode configuration >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >> >> When user switch to push-pull mode it could start configure >> the dead-times (in nanoseconds) to obtain a signal of a form similar to: >> ____0 D____P ____ ____ ____ >> PWM signal __| |____| |____| |____| |____| |___ >> __ __ __ __ __ >> PWMH ____| |____re| |______| |______| |______| |___ >> __ __ __ __ __ __ >> PWML |______| |____fe| |______| |______| |______| >> >> (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover >> letter), >> and starting from a driver specific dead-time value (e.g. on Atmel/Microchip >> PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM >> outputs to actually switch to PWM push-pull mode (in this mode there is a delay >> of a period introduce on a PWM ouptut) to obtain a signal similar to this one: >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> >> >> Then we can start >>> filling in that structure. >>> >>> struct pwm_caps { >>> unsigned long modes; >>> }; >>> >>> #define PWM_MODE_NORMAL (1 << 0) >>> #define PWM_MODE_COMPLEMENTARY (1 << 1) >>> #define PWM_MODE_PUSH_PULL (1 << 2) >>> >>> static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, >>> unsigned long mode) >>> { >>> return (caps->modes & mode) != 0; >>> } >>> >>> and maybe something like this as a shortcut: >>> >>> static inline bool pwm_supports_mode(struct pwm_device *pwm, >>> unsigned long mode) >>> { >>> struct pwm_caps caps; >>> int err; >>> >>> err = pwm_get_caps(pwm, &caps); >>> if (err < 0) >>> return false; >>> >>> return pwm_caps_supports_mode(&caps, mode); >>> } >>> >>> I think that gives us a lot of flexibility and easy extensibility for >>> other capabilities. The implementation of pwm_get_caps() should probably >>> call back into the driver.> >>> But that would cause a lot of boilerplate for simple cases, and would be >>> a lot of work to convert all existing drivers up front. So I think we >>> can add helpers to solve the normal cases. Something along these lines >>> perhaps: >>> >>> struct pwm_chip { >>> ... >>> >>> int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> const struct pwm_caps *caps; >>> }; >>> >>> static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >>> { >>> if (!pwm || !caps || !pwm->chip->caps) >>> return -EINVAL; >>> >>> *caps = *pwm->chip->caps; >>> >>> return 0; >>> } >>> >>> And then perhaps even do something like this: >>> >>> const struct pwm_caps pwm_default_caps = { >>> .modes = PWM_MODE_NORMAL, >>> }; >>>> Now we can either have a patch that sets pwm_default_caps and >>> pwm_simple_get_caps() for all existing drivers, or even easier add this >>> to the pwmchip_add_with_polarity() function: >>> >>> int pwmchip_add_with_polarity(struct pwm_chip *chip, >>> enum pwm_polarity polarity) >>> { >>> ... >>> >>> if (!chip->get_caps && !chip->caps) { >>> chip->get_caps = pwm_simple_get_caps; >>> chip->caps = pwm_default_caps; >>> } >>> >>> ... >>> } >>> >> What about having a new member in "struct pwm_ops" something like this: >> struct pwm_ops { >> // ... >> int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps); >> // ... >> }; >> to get from the driver the per PWM capabilities. >> >> And maybe to have, in core.c: >> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >> { >> struct pwm_chip *chip = pwm->chip; >> int ret = 0; >> >> if (!pwm || !chip || !chip->ops || !caps) >> return -EINVAL; >> >> if (chip->ops->get_caps) >> ret = chip->ops->get_caps(chip, pwm, caps); >> else >> caps = &pwm_default_caps; >> >> return ret; >> } >> >>> That way, every driver, unless upgraded with a custom ->get_caps() would >>> get the defaults, which encode the current assumptions of the framework. >>> >>> I think what we'd also want to do is make sure that every driver always >>> at least implement NORMAL mode and perhaps even fail to register those >>> that don't. Not sure we want to go that far, but those are the current >>> assumptions, so devices must be supporting it already anyway. >>> >>>>> A possibly simpler approach might be to restrict this to the driver that >>>>> you need this for. It looks like you will be mainly using this via sysfs >>>>> and in that case you might be able to just extend what information is >>>>> exposed in sysfs for the particular PWM you're dealing with. >>>> By this you mean exporting the sysfs attributes only for my driver or possibly >>>> other drivers interested in this new feature? I may have another in kernel driver >>>> which may try to use this feature. >>> >>> If you've got another driver that will want to use this, I'm leaning >>> towards going with the fully featured approach. >> I'm thinking at PWM regulator driver. I worked lately on adding code to PWM >> framework in order to support what our PWM controller calls PWM triggers. >> These are external input received directly by the PWM controller which let >> it change the PWM output waveform, as long as the trigger is active, only >> at the controller level (no interaction with kernel code interaction - >> no IRQ received on trigger activation). >> This could be used, e.g., in regulators with feedback. I'm thinking that this >> could be useful for PWM regulator driver when it is using a PWM channel >> in push-pull mode and with trigger configuration. >> >> I have create that code on top of push-pull mode modifications since >> I want to take advantage of push-pull mode while using PWM triggers. >> Of course this is something I didn't introduce (I can give more details >> on this if you want). > > STM32 PWM have the same kind of triggers features but, since the harware > block can do more than PWM, we have implemented the trigger part in IIO. > It allow use to controls (start/stop) PWMs on events or levels. It is true! I am aware of that and followed latest changes on that STM32 PWM driver. Atmel/Microchip PWM hardware is only PWM aware. I though at taking advantage of the IIO subsystem and integrate the PWM in there but it looked complicated to integrate both subsystems. Basically, on the Atmel/Microchip PWM controller setting a specific trigger involves setting a particular register bit. And, yes, as you said, based on external SoC events, the PWM output waveforms is disabled/enabled (based on the external events). Thank you, Claudiu > >> >> Thank you, >> Claudiu >> >> Ideally you would try to >>> implement both at once so we can get a better feeling about whether or >>> not the framework changes are adequate to cover at least the first two >>> cases, which is much more reassuring than just being able to cover a >>> single case. >>> >>> One thing to consider is still that if you only use this from sysfs, >>> it'll be quite a lot of work to add in-kernel infrastructure when it's >>> never needed. There's still an advantage to do it anyway because it >>> allows us to abstract everything away properly from the start and avoid >>> fragmentation of the sysfs interface. >>> >>> Thierry >>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html