[PATCH v3] pwm: add CSR SiRFSoC PWM driver

[Romain Izard]

2014-02-28 11:07 GMT+01:00 Barry Song <21cnbao at gmail.com>:
> 2014-02-28 17:06 GMT+08:00 Romain Izard <romain.izard.pro at gmail.com>:
>> 2014-02-28 4:01 GMT+01:00 Barry Song <21cnbao at gmail.com>:
>>>
>>> 1.  fclk should be named for pwm not for rtc, osc and pll, so the
>>> names might be ugly by fclk0~N.
>>>
>> For me, it's important to keep the number in the name of the clock as it is
>> the source of the information for binding the device tree clock on one side
>> and the clock source index in the selection register on the other side. If
>> we do not do it this way, we cannot easily handle new clocks in the future
>> in the binding.
>>
>> On the other hand, I strongly dislike the current clock specification
>> as <&clks #x>.
>> It would be probably a good thing to add a header linking the numbers
>> to symbols,
>> and we would then get:
>>
>>         clocks = <&clks SIRFCLK_A6_PWM>,
>>                 <&clks SIRFCLK_A6_OSC>,
>>                 <&clks SIRFCLK_A6_PLL1>,
>>                 <&clks SIRFCLK_A6_PLL2>,
>>                 <&clks SIRFCLK_A6_RTC>,
>>                 <&clks SIRFCLK_16_PLL3>;
>>         clock-names = "iclk", "fclk0", "fclk1", "fclk2", "fclk3", "fclk4";
>>
>> Note that as the binary output of the device tree would not change with this, we
>> do not need to worry about backwards compatibility in this precise case.
>
> this pwm controller is prima2-compatible, so it is actually a pwm
> controller highly bound with the special SoC.
> really strange things are we actually care about the clock types. we
> actually can't think fclk0~fclk4 as same things. for pll, i think it
> is buggy to use. for rtc, it only service special target for providing
> bypass 32KHz clock.
>
If you're not using DVFS, it may be useful to use the PLLs. I do not think
it's a good thing to say 'we will never use it' or 'we will not get it to work'.

I now remember our use case for low-speed PWM: buzzers. here we will
need to have a output range of typically 40-2000 Hz, which cannot be provided
by the 26 MHz input source.

> if we look this controller a separate IP, we need to look 5 clock
> source to be coequal. but it is not the real case.
> i think people will hate the things that we have a separate bypass
> mode for fclk3, why it is 3 but not 2, 1, 4 and 5?

I believe we can make the code to handle all clocks almost as equals,
because for the IP it is the case. The DVFS case has an inpact on the
PLLs, so it's not perfectly the case, but from the register point of view,
the only difference is the value in the input selection register.

If we want to output a 26 MHz clock, we need to enable bypass also
on fclk0: otherwise we're limited by the minimum divisor of 2. But for
me, the bypass should not be a 'visible' feature of the PWM for its users.
The users specify a PWM line, and a period. We give them what they
ask, or we return them an error if we cannot do that with the available
input clocks.

> that is why i move to a MACRO 26MHz in the original codes as i think
> it is a prima2 controller but not a controller used by prima2 even
> though we always want a IP module to be a IP module suitable for all
> SoCs.

The PWM controller is already used in prima2 and atlas6, it's almost the
same as the controller in atlas4/5 which won't get supported in the kernel
anytime soon. I expect the controller to be reused in future products: at
the end, the sirfsoc family has multiple SoCs. And the xin clock is 12
MHz on atlas5 but 26 MHz on atlas6, so the clock specification through
device tree is a pretty good idea.

>>> 2.  the driver need to manage all of the clocks. yes, it can. but it
>>> might be ugly again. we can either request the clk at the time of pwm
>>> configuration, or get all directy in probe and maintain an array.
>>
>> For me, what is important right now is to get the binding right. If the driver
>> cannot do everything the binding allows, this is not a problem. But we define
>> a binding that cannot easily be expanded, we will have problems when we
>> need the new features.
>>
>>> i am thinking what is the best way for it. if we do that by an
>>> incremental patch, it might be more concentrated and understandable.
>
> i think bypass mode for rtc will be included immediately since there
> is a real user.
>
For reviewing, I believe it is better if the features are added one by one.
This makes it somehow easier to understand each separate feature.
But I recognize it can be tiring as each patch needs to be tested separately.

-- 
Romain Izard