<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 9, 2016 at 4:35 PM, Joel Stanley <span dir="ltr"><<a href="mailto:joel@jms.id.au" target="_blank">joel@jms.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder<br>
Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>> wrote:<br>
> Add binding documentation update for ASPEED AST2500 PWM driver.<br>
> The ASPEED AST2500 PWM driver supports 8 PWM output ports.<br>
> The parent node indicates a PWM controller and has options to:<br>
> provide register set information, provide pinctrl information,<br>
> enable PWM and Fan Tach clock, select clock source, provide settings for<br>
> three different PWM sources (type M, N and O).<br>
><br>
> Each sub node indicates a PWM output port and has options to:<br>
> enable PWM port, select the PWM source for the PWM port, provide<br>
> duty cycle values to be used at startup.<br>
><br>
> v2:<br>
> - Made binding documentation update as a separate patch.<br>
> - Included AST2500 in the description.<br>
> - Changed format to "aspeed,ast2500-pwm".<br>
> - Included explanation for types M ,N and O and calculation example to<br>
> determine L, H, and period bits.<br>
><br>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>><br>
> ---<br>
> .../bindings/hwmon/aspeed,<wbr>ast2500-pwm.txt | 116 +++++++++++++++++++++<br>
> 1 file changed, 116 insertions(+)<br>
> create mode 100644 Documentation/devicetree/<wbr>bindings/hwmon/aspeed,ast2500-<wbr>pwm.txt<br>
><br>
> diff --git a/Documentation/devicetree/<wbr>bindings/hwmon/aspeed,ast2500-<wbr>pwm.txt b/Documentation/devicetree/<wbr>bindings/hwmon/aspeed,ast2500-<wbr>pwm.txt<br>
> new file mode 100644<br>
> index 0000000..2c3f2b6<br>
> --- /dev/null<br>
> +++ b/Documentation/devicetree/<wbr>bindings/hwmon/aspeed,ast2500-<wbr>pwm.txt<br>
> @@ -0,0 +1,116 @@<br>
> +ASPEED AST2500 PWM device driver<br>
> +<br>
> +The ASPEED AST2500 PWM controller can support 8 PWM outputs.<br>
> +<br>
> +Required properties:<br>
> +- #address-cells : should be 1.<br>
> +- #size-cells : should be 1.<br>
> +- reg : address and length of the register set for the device.<br>
> +- pinctrl-names : A pinctrl state named "default" must be defined.<br>
> +- pinctrl-0 : Phandle referencing pin configuration of the aspeed pwm ports.<br>
> +- compatible : should be "aspeed,ast2500-pwm".<br>
> +- clock_enable : option to enable PWM and fan tach clock. should be 1.<br>
> + (D[0] in PTCR00).<br>
> +- clock_source : option for clock source selection. 0 indicates 24MHz clock<br>
> + and 1 indicates MCLK (D[1] in PTCR00).<br>
> +- typem_pwm_clock : PWM types M, N and 0 are three types just to have three<br>
> + independent PWM source. Each could be assigned to the 8 PWM channels<br>
> + with it's own settings<br>
> + This array contains 4 values.<br>
> + The first value indicates if the values for type M PWM are valid.<br>
> + If the first value is 1, then it indicates that the rest of the values<br>
> + are valid. If it is 0, then invalid.<br>
> + The second value indicates the type M PWM clock division L bit (D[3:0]<br>
> + in PTCR04). (value 0 in D[3:0] indicates 1, value 1 in D[3:0] indicates 2,<br>
> + value 2 in D[3:0] indicates 4, value 3 in D[3:0] indicates 6 and so on<br>
> + with increments in multiples of two).<br>
> + The third value indicates type M PWM clock divison H bit (D[7:4] in PTCR04).<br>
> + (value 0 in D[7:4] indicates 1, value 1 in D[7:4] indicates 2,<br>
> + value 2 in D[7:4] indicates 4, value 3 in D[7:4] indicates 8 and<br>
> + so on with increments in powers of two).<br>
> + The fourth value indicates type M PWM period bit (D[15:8] in PTCR04).<br>
> + If type M is chosen and clock source is 24 MHz then:<br>
> + The PWM base clock = 24Mhz / (type M clock division L bit *<br>
> + type M clock division H bit).<br>
<br>
</div></div>We should request the APB struct clk and calculate the PWM base clock<br>
from that. See drivers that do of_clk_get. This means the binding<br>
would require a phandle to the clock node that we have in our device<br>
tree.<br>
<br>
We don't need to describe the clock calculation in the bindings document.<br>
<span class="gmail-"><br></span></blockquote><div>Should I indicate the clock-frequency (desired PWM base clock) in the devicetree clock node , get that clock-frequency using clk_get_rate and calculate type M clock division L bit and type M clock division H bit to match that clock-frequency (having fixed clock source and type M clock period bit values from the devicetree) ?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> + The PWM frequency = The PWM base clock / (type M PWM period bit + 1)<br>
> + If we plan to output 25Khz PWM frequency then we can use:<br>
> + typem_pwm_clock = <1 5 0 95><br>
> + The PWM frequency = 24Mhz / (10 * 1 * (95 + 1)) = 25Khz<br>
> +- typen_pwm_clock : This array contains 4 values.<br>
> + The first value indicates if the values for type N PWM are valid.<br>
> + If the first value is 1, then it indicates that the rest of the values<br>
> + are valid. If it is 0, then invalid.<br>
> + The second value indicates the type N PWM clock division L bit (D[19:16]<br>
> + in PTCR04).<br>
> + The third value indicates type N PWM clock division H bit<br>
> + (D[23:20] in PTCR04).<br>
> + The fourth value indicates type N PWM period bit (D[31:24] in PTCR04).<br>
> + Calculations similar as for type M.<br>
> +- typeo_pwm_clock : This array contains 4 values.<br>
> + The first value indicates indicates if the values for type O PWM are valid.<br>
> + If the first value is 1, then it indicates that the rest of the values<br>
> + are valid. If it is 0, then invalid.<br>
> + The second value indicates if the type O PWM clock division L bit<br>
> + (D[3:0] in PTCR44).<br>
> + The third value indicates type O PWM clock divison H bit (D[7:4] in PTCR44).<br>
> + The fourth value indicates type O PWM period bit (D[15:8] in PTCR44).<br>
> + Calculations similar as for type M.<br>
<br>
</span>This large block is hard to read. Try adding some whitespace :)<br>
<br>
Spend some time comparing your bindings to others in the tree for the<br>
style of both the documentation and the bindings themselves.<br>
<span class="gmail-"><br>
> +<br>
> +Each subnode represents a PWM output port.<br>
> +<br>
> +Subnode Format<br>
> +--------------<br>
> +Required properties:<br>
> +- pwm_port : Indicates the PWM port. Values 0 through 7 indicate PWM ports<br>
> + A through H respectively.<br>
> +- pwm_enable : Option to enable the PWM port indicated above. 0 is enable<br>
> + and 1 is disable.<br>
> + D[8] in PTCR00 indicates enabling PWM port A,<br>
> + D[9] in PTCR00 indicates enabling PWM port B,<br>
> + D[10] in PTCR00 indicates enabling PWM port C,<br>
> + D[11] in PTCR00 indicates enabling PWM port D,<br>
> + D[8] in PTCR40 indicates enabling PWM port E,<br>
> + D[9] in PTCR40 indicates enabling PWM port F,<br>
> + D[10] in PTCR40 indicates enabling PWM port G,<br>
> + D[11] in PTCR40 indicates enabling PWM port H.<br>
<br>
</span>This appears to be describing the register layout, not the bindings.<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
> +- pwm_type : Indicates the clock type for the PWM port. value 0 is type M<br>
> + PWM clock. value 1 is type N PWM clock. value 2 is type O PWM clock.<br>
> + D[12] in PTCR00 indicates type selection bit of PWM A port,<br>
> + D[13] in PTCR00 indicates type selection bit of PWM B port,<br>
> + D[14] in PTCR00 indicates type selection bit of PWM C port,<br>
> + D[15] in PTCR00 indicates type selection bit of PWM D port,<br>
> + D[12] in PTCR40 indicates type selection bit of PWM E port,<br>
> + D[13] in PTCR40 indicates type selection bit of PWM F port,<br>
> + D[14] in PTCR40 indicates type selection bit of PWM G port,<br>
> + D[15] in PTCR40 indicates type selection bit of PWM H port.<br>
> +- pwm_duty_cycle_percent : duty cycle value.<br>
> +<br>
> +Examples:<br>
> +<br>
> +pwm: pwm-controller@1e786000 {<br>
> + #address-cells = <1>;<br>
> + #size-cells = <1>;<br>
> + reg = <0x1E786000 0x78>;<br>
> + pinctrl-names = "default";<br>
> + pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default<br>
> + &pinctrl_pwm2_default &pinctrl_pwm3_default>;<br>
> + compatible = "aspeed,ast2500-pwm";<br>
> + clock_enable = /bits/ 8 <0x01>;<br>
> + clock_source = /bits/ 8 <0x00>;<br>
> + typem_pwm_clock = <1 5 0 95>;<br>
> + typen_pwm_clock = <0 0 0 0>;<br>
> + typeo_pwm_clock = <0 0 0 0>;<br>
> + pwm_port0 {<br>
> + pwm_port = /bits/ 8 <0x00>;<br>
> + pwm_enable = /bits/ 8 <0x01>;<br>
> + pwm_type = /bits/ 8 <0x00>;<br>
> + pwm_duty_cycle_percent = /bits/ 8 <0x5A>;<br>
> + };<br>
> + pwm_port1 {<br>
> + pwm_port = /bits/ 8 <0x01>;<br>
> + pwm_enable = /bits/ 8 <0x01>;<br>
> + pwm_type = /bits/ 8 <0x00>;<br>
> + pwm_duty_cycle_percent = /bits/ 8 <0x5A>;<br>
> + };<br>
> +};<br>
> +<br>
> --<br>
> 2.8.0.rc3.226.g39d4020<br>
><br>
</div></div></blockquote></div><br></div></div>